fix: Add an INVALID_ZIPFILE lint error on zip file with invalid chars…

… in filenames (#3940)

* fix(deps): update dependency addons-scanner-utils to v5

* fix: Add an INVALID_ZIPFILE lint error on zip file with invalid chars in filenames

Co-authored-by: Renovate Bot <bot@renovateapp.com>

docs/index.html

@@ -225,11 +225,6 @@ <h2 id="package-layout" tabindex="-1">Package layout <a class="header-anchor" hr
 <td>Flagged file extensions found</td>
 </tr>
 <tr>
-<td><code>DUPLICATE_XPI_ENTRY</code></td>
-<td>warning</td>
-<td>Package contains duplicate entries</td>
-</tr>
-<tr>
 <td><code>ALREADY_SIGNED</code></td>
 <td>warning</td>
 <td>Already signed</td>
@@ -240,9 +235,19 @@ <h2 id="package-layout" tabindex="-1">Package layout <a class="header-anchor" hr
 <td>Firefox add-ons are not allowed to run coin miners.</td>
 </tr>
 <tr>
+<td><code>DUPLICATE_XPI_ENTRY</code></td>
+<td>error</td>
+<td>Package contains duplicate entries.</td>
+</tr>
+<tr>
+<td><code>INVALID_XPI_ENTRY</code></td>
+<td>error</td>
+<td>Package contains invalid entries (e.g. invalid characters in entries path name).</td>
+</tr>
+<tr>
 <td><code>BAD_ZIPFILE</code></td>
 <td>error</td>
-<td>Bad zip file</td>
+<td>Bad zip file.</td>
 </tr>
 <tr>
 <td><code>FILE_TOO_LARGE</code></td>

docs/rules.md

@@ -52,17 +52,18 @@ Rules are sorted by severity.
 
 ## Package layout
 
-| Message code               | Severity | Description                                         |
-| -------------------------- | -------- | --------------------------------------------------- |
-| `MOZILLA_COND_OF_USE`      | notice   | Mozilla conditions of use violation.                |
-| `FLAGGED_FILE_TYPE`        | notice   | (Binary) Flagged file type found.                   |
-| `FLAGGED_FILE_EXTENSION`   | warning  | Flagged file extensions found                       |
-| `DUPLICATE_XPI_ENTRY`      | warning  | Package contains duplicate entries                  |
-| `ALREADY_SIGNED`           | warning  | Already signed                                      |
-| `COINMINER_USAGE_DETECTED` | warning  | Firefox add-ons are not allowed to run coin miners. |
-| `BAD_ZIPFILE`              | error    | Bad zip file                                        |
-| `FILE_TOO_LARGE`           | error    | File is too large to parse                          |
-| `RESERVED_FILENAME`        | error    | Reserved filename detected.                         |
+| Message code               | Severity | Description                                                                      |
+| -------------------------- | -------- | -------------------------------------------------------------------------------- |
+| `MOZILLA_COND_OF_USE`      | notice   | Mozilla conditions of use violation.                                             |
+| `FLAGGED_FILE_TYPE`        | notice   | (Binary) Flagged file type found.                                                |
+| `FLAGGED_FILE_EXTENSION`   | warning  | Flagged file extensions found                                                    |
+| `ALREADY_SIGNED`           | warning  | Already signed                                                                   |
+| `COINMINER_USAGE_DETECTED` | warning  | Firefox add-ons are not allowed to run coin miners.                              |
+| `DUPLICATE_XPI_ENTRY`      | error    | Package contains duplicate entries.                                              |
+| `INVALID_XPI_ENTRY`        | error    | Package contains invalid entries (e.g. invalid characters in entries path name). |
+| `BAD_ZIPFILE`              | error    | Bad zip file.                                                                    |
+| `FILE_TOO_LARGE`           | error    | File is too large to parse                                                       |
+| `RESERVED_FILENAME`        | error    | Reserved filename detected.                                                      |
 
 ## Type detection
 

package.json

@@ -45,7 +45,7 @@
   "dependencies": {
     "@mdn/browser-compat-data": "4.0.5",
     "addons-moz-compare": "1.2.0",
-    "addons-scanner-utils": "4.11.0",
+    "addons-scanner-utils": "5.0.0",
     "ajv": "6.12.6",
     "ajv-merge-patch": "4.1.0",
     "chalk": "4.1.2",

src/linter.js

@@ -5,6 +5,10 @@ import chalk from 'chalk';
 import Dispensary from 'dispensary';
 import { oneLine } from 'common-tags';
 import { lstat } from 'addons-scanner-utils/dist/io/utils';
+import {
+  DuplicateZipEntryError,
+  InvalidZipFileError,
+} from 'addons-scanner-utils/dist/errors';
 import { Directory, Xpi, Crx } from 'addons-scanner-utils/dist/io';
 
 import { terminalWidth } from 'cli';
@@ -98,19 +102,42 @@ export default class Linter {
   }
 
   handleError(err, _console = console) {
-    if (err.message.includes('DuplicateZipEntry')) {
+    // The zip files contains invalid entries (likely path names using invalid
+    // characters like '\\'), the linter can inspect the package but Firefox
+    // would fail to load it.
+    if (err instanceof InvalidZipFileError) {
+      this.collector.addError({
+        ...messages.INVALID_XPI_ENTRY,
+        message: err.message,
+      });
+      this.print(_console);
+      return true;
+    }
+
+    // The zip file contains multiple entries with the exact same file name.
+    if (err instanceof DuplicateZipEntryError) {
       this.collector.addError(messages.DUPLICATE_XPI_ENTRY);
       this.print(_console);
-    } else if (err.message.includes(constants.ZIP_LIB_CORRUPT_FILE_ERROR)) {
+      return true;
+    }
+
+    // The zip file fails to open successfully, the linter can't inspect it
+    // at all.
+    if (err.message.includes(constants.ZIP_LIB_CORRUPT_FILE_ERROR)) {
       this.collector.addError(messages.BAD_ZIPFILE);
       this.print(_console);
-    } else if (this.config.stack === true) {
+      return true;
+    }
+
+    if (this.config.stack === true) {
       _console.error(err.stack);
     } else {
       _console.error(this.chalk.red(err.message || err));
     }
 
     this.closeIO();
+
+    return false;
   }
 
   print(_console = console) {
@@ -552,7 +579,9 @@ export default class Linter {
         process.exit(exitCode);
       }
     } catch (err) {
-      this.handleError(err, deps._console);
+      if (this.handleError(err, deps._console)) {
+        return;
+      }
       throw err;
     }
   }

src/messages/layout.js

@@ -11,6 +11,23 @@ export const DUPLICATE_XPI_ENTRY = {
     and re-zipping your add-on package and try again.`),
 };
 
+export const INVALID_XPI_ENTRY = {
+  code: 'INVALID_XPI_ENTRY',
+  // `message` will be replaced with the `InvalidZipFileError` message
+  // got from the addons-scanner-utils dependency when we were reading
+  // the zip file entries (in particular this would be triggered by
+  // a zipfile entry using invalid characters, like '\' as a path
+  // separator, and the underlying yauzl error message follows the
+  // format:
+  //   `invalid characters in fileName: nameOfTheInvalidZipFileEntry`
+  message: 'Invalid ZIP file entry',
+  description: i18n._(oneLine`The package is invalid. It may contain
+    entries using invalid characters, as an example using '\\' as a
+    path separator is not allowed in Firefox. Try to recreate your
+    add-on package (ZIP) and make sure all entries are using '/' as the
+    path separator.`),
+};
+
 export const BAD_ZIPFILE = {
   code: 'BAD_ZIPFILE',
   message: 'Corrupt ZIP file',

tests/unit/test.linter.js

@@ -3,6 +3,10 @@ import fs from 'fs';
 
 import { oneLine } from 'common-tags';
 import { Xpi } from 'addons-scanner-utils/dist/io';
+import {
+  DuplicateZipEntryError,
+  InvalidZipFileError,
+} from 'addons-scanner-utils/dist/errors';
 import { createFakeStderr } from 'addons-scanner-utils/dist/test-helpers';
 
 import Linter from 'linter';
@@ -137,9 +141,7 @@ describe('Linter', () => {
     // Stub print to prevent output.
     addonLinter.print = sinon.stub();
     expect(addonLinter.collector.errors.length).toEqual(0);
-    await expect(addonLinter.scan()).rejects.toThrow(
-      constants.ZIP_LIB_CORRUPT_FILE_ERROR
-    );
+    await addonLinter.scan();
     expect(addonLinter.collector.errors.length).toEqual(1);
     expect(addonLinter.collector.errors[0].code).toEqual(
       messages.BAD_ZIPFILE.code
@@ -329,7 +331,7 @@ describe('Linter', () => {
     addonLinter.checkFileExists = fakeCheckFileExists;
     addonLinter.collector.addError = sinon.stub();
     addonLinter.print = sinon.stub();
-    const expectedError = new Error('DuplicateZipEntry the zip has dupes!');
+    const expectedError = new DuplicateZipEntryError('the zip has dupes!');
     class FakeXpi extends FakeIOBase {
       async getFiles() {
         throw expectedError;
@@ -339,16 +341,39 @@ describe('Linter', () => {
         return this.getMetadata();
       }
     }
-    await expect(addonLinter.scan({ _Xpi: FakeXpi })).rejects.toThrow(
-      expectedError
-    );
+    await addonLinter.scan({ _Xpi: FakeXpi });
     sinon.assert.calledWith(
       addonLinter.collector.addError,
       messages.DUPLICATE_XPI_ENTRY
     );
     sinon.assert.calledOnce(addonLinter.print);
   });
 
+  it('should call addError when Xpi rejects with InvalidZipFileError', async () => {
+    const addonLinter = new Linter({ _: ['bar'] });
+    addonLinter.checkFileExists = fakeCheckFileExists;
+    addonLinter.collector.addError = sinon.stub();
+    addonLinter.print = sinon.stub();
+    const expectedError = new InvalidZipFileError(
+      'invalid characters in fileName: fake\\file.txt'
+    );
+    class FakeXpi extends FakeIOBase {
+      async getFiles() {
+        throw expectedError;
+      }
+
+      getFilesByExt() {
+        return this.getMetadata();
+      }
+    }
+    await addonLinter.scan({ _Xpi: FakeXpi });
+    sinon.assert.calledWith(addonLinter.collector.addError, {
+      ...messages.INVALID_XPI_ENTRY,
+      message: expectedError.message,
+    });
+    sinon.assert.calledOnce(addonLinter.print);
+  });
+
   it('should throw if invalid type is passed to colorize', () => {
     const addonLinter = new Linter({ _: ['bar'] });
     expect(() => {