Merge pull request #12090 from backstage/fix/broken-zip-files

Throw on corrupt ZIP files instead of hanging indefinitely

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',
+    );
+  });
 });

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<ReadTreeResponseFile[]> {
-    this.onlyOnce();
+  private async streamToTemporaryFile(
+    stream: Readable,
+  ): Promise<{ fileName: string; cleanup: () => Promise<void> }> {
+    const tmpDir = await fs.mkdtemp(
+      platformPath.join(this.workDir, 'backstage-tmp'),
+    );
+    const tmpFile = platformPath.join(tmpDir, 'tmp.zip');
 
-    const files = Array<ReadTreeResponseFile>();
+    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<void>,
+  ): Promise<void> {
+    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<ReadTreeResponseFile[]> {
+    this.onlyOnce();
+    const files = Array<ReadTreeResponseFile>();
+    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<string> {
     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;
   }

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<Buffer> => {
+  return new Promise(async (resolve, reject) => {
+    try {
+      await pipeline(stream, concatStream(resolve));
+    } catch (ex) {
+      reject(ex);
+    }
+  });
+};