diff --git a/.changeset/nine-mails-crash.md b/.changeset/nine-mails-crash.md new file mode 100644 index 0000000000..ead5553d1c --- /dev/null +++ b/.changeset/nine-mails-crash.md @@ -0,0 +1,9 @@ +--- +'@backstage/backend-common': patch +--- + +The `ZipArchiveResponse` now correctly handles corrupt ZIP archives. + +Before this change, certain corrupt ZIP archives either cause the inflater to throw (as expected), or will hang the parser indefinitely. + +By switching out the `zip` parsing library, we now write to a temporary directory, and load from disk which should ensure that the parsing of the `.zip` files are done correctly because `streaming` of `zip` paths is technically impossible without being able to parse the headers at the end of the file. diff --git a/packages/backend-common/package.json b/packages/backend-common/package.json index bd8e7cf9f5..a21f66ccfa 100644 --- a/packages/backend-common/package.json +++ b/packages/backend-common/package.json @@ -41,6 +41,7 @@ "@backstage/integration": "^1.2.2", "@backstage/types": "^1.0.0", "@google-cloud/storage": "^6.0.0", + "@keyv/redis": "^2.2.3", "@manypkg/get-packages": "^1.1.3", "@octokit/rest": "^19.0.3", "@types/cors": "^2.8.6", @@ -64,7 +65,6 @@ "jose": "^4.6.0", "keyv": "^4.0.3", "keyv-memcache": "^1.2.5", - "@keyv/redis": "^2.2.3", "knex": "^2.0.0", "lodash": "^4.17.21", "logform": "^2.3.2", @@ -78,8 +78,8 @@ "selfsigned": "^2.0.0", "stoppable": "^1.1.0", "tar": "^6.1.2", - "unzipper": "^0.10.11", "winston": "^3.2.1", + "yauzl": "^2.10.0", "yn": "^4.0.0" }, "peerDependencies": { @@ -106,7 +106,7 @@ "@types/stoppable": "^1.1.0", "@types/supertest": "^2.0.8", "@types/tar": "^6.1.1", - "@types/unzipper": "^0.10.3", + "@types/yauzl": "^2.10.0", "@types/webpack-env": "^1.15.2", "aws-sdk-mock": "^5.2.1", "better-sqlite3": "^7.5.0", diff --git a/packages/backend-common/src/reading/__fixtures__/mock-corrupted.zip b/packages/backend-common/src/reading/__fixtures__/mock-corrupted.zip new file mode 100644 index 0000000000..5130a72f6f Binary files /dev/null and b/packages/backend-common/src/reading/__fixtures__/mock-corrupted.zip differ diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts index a54bac0c4a..f1915cf99b 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.test.ts @@ -22,6 +22,9 @@ import { ZipArchiveResponse } from './ZipArchiveResponse'; const archiveData = fs.readFileSync( resolvePath(__filename, '../../__fixtures__/mock-main.zip'), ); +const archiveDataCorrupted = fs.readFileSync( + resolvePath(__filename, '../../__fixtures__/mock-corrupted.zip'), +); const archiveDataWithExtraDir = fs.readFileSync( resolvePath(__filename, '../../__fixtures__/mock-with-extra-root-dir.zip'), ); @@ -31,6 +34,7 @@ describe('ZipArchiveResponse', () => { mockFs({ '/test-archive.zip': archiveData, '/test-archive-with-extra-root-dir.zip': archiveDataWithExtraDir, + '/test-archive-corrupted.zip': archiveDataCorrupted, '/tmp': mockFs.directory(), }); }); @@ -55,6 +59,7 @@ describe('ZipArchiveResponse', () => { content: expect.any(Function), }, ]); + const contents = await Promise.all(files.map(f => f.content())); expect(contents.map(c => c.toString('utf8').trim())).toEqual([ 'site_name: Test', @@ -129,7 +134,6 @@ describe('ZipArchiveResponse', () => { const res = new ZipArchiveResponse(stream, 'docs/', '/tmp', 'etag'); const dir = await res.dir(); - expect(dir).toMatch(/^[\/\\]tmp[\/\\].*$/); await expect( fs.readFile(resolvePath(dir, 'index.md'), 'utf8'), @@ -152,4 +156,15 @@ describe('ZipArchiveResponse', () => { fs.pathExists(resolvePath(dir, 'docs/index.md')), ).resolves.toBe(false); }); + + it('should throw on invalid archive', async () => { + const stream = fs.createReadStream('/test-archive-corrupted.zip'); + + const res = new ZipArchiveResponse(stream, '', '/tmp', 'etag'); + const filesPromise = res.files(); + + await expect(filesPromise).rejects.toThrow( + 'invalid comment length. expected: 55. found: 0', + ); + }); }); diff --git a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts index e07cedde87..1a6658b0c2 100644 --- a/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts +++ b/packages/backend-common/src/reading/tree/ZipArchiveResponse.ts @@ -15,15 +15,16 @@ */ import archiver from 'archiver'; +import yauzl, { Entry } from 'yauzl'; import fs from 'fs-extra'; import platformPath from 'path'; import { Readable } from 'stream'; -import unzipper, { Entry } from 'unzipper'; import { ReadTreeResponse, ReadTreeResponseDirOptions, ReadTreeResponseFile, } from '../types'; +import { streamToBuffer } from './util'; /** * Wraps a zip archive stream into a tree response reader. @@ -67,43 +68,85 @@ export class ZipArchiveResponse implements ReadTreeResponse { private shouldBeIncluded(entry: Entry): boolean { if (this.subPath) { - if (!entry.path.startsWith(this.subPath)) { + if (!entry.fileName.startsWith(this.subPath)) { return false; } } if (this.filter) { - return this.filter(this.getInnerPath(entry.path), { - size: - (entry.vars as { uncompressedSize?: number }).uncompressedSize ?? - entry.vars.compressedSize, + return this.filter(this.getInnerPath(entry.fileName), { + size: entry.uncompressedSize, }); } return true; } - async files(): Promise { - this.onlyOnce(); + private async streamToTemporaryFile( + stream: Readable, + ): Promise<{ fileName: string; cleanup: () => Promise }> { + const tmpDir = await fs.mkdtemp( + platformPath.join(this.workDir, 'backstage-tmp'), + ); + const tmpFile = platformPath.join(tmpDir, 'tmp.zip'); - const files = Array(); + const writeStream = fs.createWriteStream(tmpFile); - await this.stream - .pipe(unzipper.Parse()) - .on('entry', (entry: Entry) => { - if (entry.type === 'Directory') { - entry.resume(); + return new Promise((resolve, reject) => { + writeStream.on('error', reject); + writeStream.on('finish', () => + resolve({ fileName: tmpFile, cleanup: () => fs.remove(tmpFile) }), + ); + stream.pipe(writeStream); + }); + } + + private forEveryZipEntry( + zip: string, + callback: (entry: Entry, content: Readable) => Promise, + ): Promise { + return new Promise((resolve, reject) => { + yauzl.open(zip, { lazyEntries: true }, (err, zipfile) => { + if (err || !zipfile) { + reject(err || new Error(`Failed to open zip file ${zip}`)); return; } - if (this.shouldBeIncluded(entry)) { - files.push({ - path: this.getInnerPath(entry.path), - content: () => entry.buffer(), - }); - } else { - entry.autodrain(); - } - }) - .promise(); + zipfile.on('entry', async (entry: Entry) => { + // Check that the file is not a directory, and that is matches the filter. + if (!entry.fileName.endsWith('/') && this.shouldBeIncluded(entry)) { + zipfile.openReadStream(entry, async (openErr, readStream) => { + if (openErr || !readStream) { + reject( + openErr || + new Error(`Failed to open zip entry ${entry.fileName}`), + ); + return; + } + + await callback(entry, readStream); + }); + } + zipfile.readEntry(); + }); + zipfile.once('end', () => resolve()); + zipfile.on('error', e => reject(e)); + zipfile.readEntry(); + }); + }); + } + + async files(): Promise { + this.onlyOnce(); + const files = Array(); + const temporary = await this.streamToTemporaryFile(this.stream); + + await this.forEveryZipEntry(temporary.fileName, async (entry, content) => { + files.push({ + path: this.getInnerPath(entry.fileName), + content: async () => await streamToBuffer(content), + }); + }); + + temporary.cleanup(); return files; } @@ -116,45 +159,46 @@ export class ZipArchiveResponse implements ReadTreeResponse { } const archive = archiver('zip'); - await this.stream - .pipe(unzipper.Parse()) - .on('entry', (entry: Entry) => { - if (entry.type === 'File' && this.shouldBeIncluded(entry)) { - archive.append(entry, { name: this.getInnerPath(entry.path) }); - } else { - entry.autodrain(); - } - }) - .promise(); + const temporary = await this.streamToTemporaryFile(this.stream); + + await this.forEveryZipEntry(temporary.fileName, async (entry, content) => { + archive.append(await streamToBuffer(content), { + name: this.getInnerPath(entry.fileName), + }); + }); + archive.finalize(); + temporary.cleanup(); + return archive; } async dir(options?: ReadTreeResponseDirOptions): Promise { this.onlyOnce(); - const dir = options?.targetDir ?? (await fs.mkdtemp(platformPath.join(this.workDir, 'backstage-'))); - await this.stream - .pipe(unzipper.Parse()) - .on('entry', async (entry: Entry) => { - // Ignore directory entries since we handle that with the file entries - // as a zip can have files with directories without directory entries - if (entry.type === 'File' && this.shouldBeIncluded(entry)) { - const entryPath = this.getInnerPath(entry.path); - const dirname = platformPath.dirname(entryPath); - if (dirname) { - await fs.mkdirp(platformPath.join(dir, dirname)); - } - entry.pipe(fs.createWriteStream(platformPath.join(dir, entryPath))); - } else { - entry.autodrain(); - } - }) - .promise(); + const temporary = await this.streamToTemporaryFile(this.stream); + + await this.forEveryZipEntry(temporary.fileName, async (entry, content) => { + const entryPath = this.getInnerPath(entry.fileName); + const dirname = platformPath.dirname(entryPath); + + if (dirname) { + await fs.mkdirp(platformPath.join(dir, dirname)); + } + return new Promise(async (resolve, reject) => { + const file = fs.createWriteStream(platformPath.join(dir, entryPath)); + file.on('finish', resolve); + + content.on('error', reject); + content.pipe(file); + }); + }); + + temporary.cleanup(); return dir; } diff --git a/packages/backend-common/src/reading/tree/util.ts b/packages/backend-common/src/reading/tree/util.ts index cfb986a0b0..63192102f4 100644 --- a/packages/backend-common/src/reading/tree/util.ts +++ b/packages/backend-common/src/reading/tree/util.ts @@ -14,12 +14,28 @@ * limitations under the License. */ +import { Readable, pipeline as pipelineCb } from 'stream'; +import { promisify } from 'util'; +import concatStream from 'concat-stream'; + +const pipeline = promisify(pipelineCb); + // Matches a directory name + one `/` at the start of any string, // containing any character except `/` one or more times, and ending with a `/` // e.g. Will match `dirA/` in `dirA/dirB/file.ext` const directoryNameRegex = /^[^\/]+\//; - // Removes the first segment of a forward-slash-separated path export function stripFirstDirectoryFromPath(path: string): string { return path.replace(directoryNameRegex, ''); } + +// Collect the stream into a buffer and return +export const streamToBuffer = (stream: Readable): Promise => { + return new Promise(async (resolve, reject) => { + try { + await pipeline(stream, concatStream(resolve)); + } catch (ex) { + reject(ex); + } + }); +}; diff --git a/yarn.lock b/yarn.lock index a1140da74b..ea8ec6ce0b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7638,6 +7638,13 @@ resolved "https://registry.npmjs.org/@types/yarnpkg__lockfile/-/yarnpkg__lockfile-1.1.5.tgz#9639020e1fb65120a2f4387db8f1e8b63efdf229" integrity sha512-8NYnGOctzsI4W0ApsP/BIHD/LnxpJ6XaGf2AZmz4EyDYJMxtprN4279dLNI1CPZcwC9H18qYcaFv4bXi0wmokg== +"@types/yauzl@^2.10.0": + version "2.10.0" + resolved "https://registry.npmjs.org/@types/yauzl/-/yauzl-2.10.0.tgz#b3248295276cf8c6f153ebe6a9aba0c988cb2599" + integrity sha512-Cn6WYCm0tXv8p6k+A8PvbDG763EDpBoTzHdA+Q/MF6H3sapGjCm9NzoaJncJS9tUKSuCoDs9XHxYYsQDgxR6kw== + dependencies: + "@types/node" "*" + "@types/yauzl@^2.9.1": version "2.9.2" resolved "https://registry.npmjs.org/@types/yauzl/-/yauzl-2.9.2.tgz#c48e5d56aff1444409e39fa164b0b4d4552a7b7a" @@ -26786,7 +26793,7 @@ yarn-lock-check@^1.0.5: yauzl@^2.10.0: version "2.10.0" resolved "https://registry.npmjs.org/yauzl/-/yauzl-2.10.0.tgz#c7eb17c93e112cb1086fa6d8e51fb0667b79a5f9" - integrity sha1-x+sXyT4RLLEIb6bY5R+wZnt5pfk= + integrity sha512-p4a9I6X6nu6IhoGmBqAcbJy1mlC4j27vEPZX9F4L4/vZT3Lyq1VkFHw/V/PUcB9Buo+DG3iHkT0x3Qya58zc3g== dependencies: buffer-crc32 "~0.2.3" fd-slicer "~1.1.0"