From 661d216b74dd2d1e6d54e9bb8320d97e9067062b Mon Sep 17 00:00:00 2001
From: Luca Greco <lgreco@mozilla.com>
Date: Mon, 4 Oct 2021 10:48:26 +0200
Subject: [PATCH] fix: Make sure Xpi class detects and throws an explicit error
 on invalid chars in zip entries filenames. (#190)

---
 src/errors.ts                                 |  13 ++++++++++
 src/index.ts                                  |   1 +
 src/io/xpi.spec.ts                            |  24 +++++++++++++++---
 src/io/xpi.ts                                 |  21 ++++++++++++---
 ...rchive-with-invalid-chars-in-filenames.zip | Bin 0 -> 132 bytes
 5 files changed, 52 insertions(+), 7 deletions(-)
 create mode 100644 src/errors.ts
 create mode 100644 src/tests/fixtures/io/archive-with-invalid-chars-in-filenames.zip

diff --git a/src/errors.ts b/src/errors.ts
new file mode 100644
index 00000000..4a3d051b
--- /dev/null
+++ b/src/errors.ts
@@ -0,0 +1,13 @@
+/* eslint-disable max-classes-per-file */
+
+export class InvalidZipFileError extends Error {
+  get name() {
+    return 'InvalidZipFileError';
+  }
+}
+
+export class DuplicateZipEntryError extends Error {
+  get name() {
+    return 'DuplicateZipFileEntry';
+  }
+}
diff --git a/src/index.ts b/src/index.ts
index eb9aad0b..14adeb95 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -2,3 +2,4 @@ export * from './const';
 export * from './functions';
 export * from './io';
 export * from './stdio';
+export * from './errors';
diff --git a/src/io/xpi.spec.ts b/src/io/xpi.spec.ts
index 44426510..423d5fb7 100644
--- a/src/io/xpi.spec.ts
+++ b/src/io/xpi.spec.ts
@@ -5,6 +5,7 @@ import { EventEmitter } from 'events';
 import yauzl, { Entry, ZipFile } from 'yauzl';
 import realSinon, { SinonSandbox, SinonStub } from 'sinon';
 
+import { DuplicateZipEntryError, InvalidZipFileError } from '../errors';
 import { Xpi } from './xpi';
 import { DEFLATE_COMPRESSION, NO_COMPRESSION } from './const';
 import {
@@ -306,9 +307,11 @@ describe(__filename, () => {
         entryCallback.call(null, dupeInstallFileEntry);
       };
 
-      await expect(myXpi.getFiles(onEventsSubscribed)).rejects.toThrow(
-        'DuplicateZipEntry',
+      const promise = myXpi.getFiles(onEventsSubscribed);
+      await expect(promise).rejects.toThrow(
+        'Entry "manifest.json" has already been seen',
       );
+      await expect(promise).rejects.toThrow(DuplicateZipEntryError);
     });
 
     it('should reject on errors in open()', async () => {
@@ -325,7 +328,7 @@ describe(__filename, () => {
         filePath: 'src/tests/fixtures/io/archive-with-duplicate-files.zip',
       });
 
-      await expect(xpi.getFiles()).rejects.toThrow('DuplicateZipEntry');
+      await expect(xpi.getFiles()).rejects.toThrow(DuplicateZipEntryError);
     });
 
     it('throws an exception when a duplicate entry has been found, even when autoClose is disabled', async () => {
@@ -335,7 +338,20 @@ describe(__filename, () => {
         stderr: createFakeStderr(),
       });
 
-      await expect(xpi.getFiles()).rejects.toThrow('DuplicateZipEntry');
+      await expect(xpi.getFiles()).rejects.toThrow(DuplicateZipEntryError);
+
+      xpi.close();
+    });
+
+    it('throws a InvalidZipFileError exception on xpi files with invalid characters', async () => {
+      const xpi = new Xpi({
+        autoClose: false,
+        filePath:
+          'src/tests/fixtures/io/archive-with-invalid-chars-in-filenames.zip',
+        stderr: createFakeStderr(),
+      });
+
+      await expect(xpi.getFiles()).rejects.toThrow(InvalidZipFileError);
 
       xpi.close();
     });
diff --git a/src/io/xpi.ts b/src/io/xpi.ts
index 010883f7..73a499bb 100644
--- a/src/io/xpi.ts
+++ b/src/io/xpi.ts
@@ -6,6 +6,7 @@ import stripBomStream from 'strip-bom-stream';
 import { oneLine } from 'common-tags';
 
 import { IOBaseConstructorParams, IOBase } from './base';
+import { InvalidZipFileError, DuplicateZipEntryError } from '../errors';
 
 type Files = { [filename: string]: Entry };
 
@@ -57,7 +58,17 @@ export class Xpi extends IOBase {
 
       this.zipLib.open(
         this.path,
-        { autoClose: this.autoClose },
+        {
+          autoClose: this.autoClose,
+          // Enable checks on invalid chars in zip entries filenames.
+          strictFileNames: true,
+          // Decode automatically filenames and zip entries content from buffer into strings
+          // and autodetects their encoding.
+          //
+          // NOTE: this is also mandatory because without this option set to true
+          // strictFileNames option is ignored.
+          decodeStrings: true,
+        },
         (err, zipfile) => {
           if (err) {
             return reject(err);
@@ -84,8 +95,8 @@ export class Xpi extends IOBase {
         in package`);
 
       reject(
-        new Error(oneLine`DuplicateZipEntry: Entry
-        "${entry.fileName}" has already been seen`),
+        new DuplicateZipEntryError(oneLine`Entry "${entry.fileName}" has already
+          been seen`),
       );
       return;
     }
@@ -111,6 +122,10 @@ export class Xpi extends IOBase {
     const zipfile = await this.open();
 
     return new Promise((resolve, reject) => {
+      zipfile.on('error', (err: Error) => {
+        reject(new InvalidZipFileError(err.message));
+      });
+
       zipfile.on('entry', (entry: Entry) => {
         this.handleEntry(entry, reject);
       });
diff --git a/src/tests/fixtures/io/archive-with-invalid-chars-in-filenames.zip b/src/tests/fixtures/io/archive-with-invalid-chars-in-filenames.zip
new file mode 100644
index 0000000000000000000000000000000000000000..40b8860e5234ac8bdae458afaf685f441b70f0ff
GIT binary patch
literal 132
zcmWIWW@Zs#VBp|j@S9~D%m4&TAOZ*kfVd#BBqOFIKPD|RCsnVcqJ)_tz?+fDo*B0e
duvQ?kr4htJ*B{``$_5f=1VTF?Z2;mh002x-6d3>j

literal 0
HcmV?d00001