chore: Show an explicit error if the requested manifest version range is invalid

bin/addons-linter

@@ -7,13 +7,19 @@ global.appRoot = path.relative(process.cwd(), absoluteAppRoot);
 global.localesRoot = path.join(absoluteAppRoot, 'dist', 'locale');
 global.nodeRequire = require;
 
+(async () => {
+  let instance;
+
   try {
     // `../dist/addons-linter` doesn't exist if the linter has not been
-  // built yet. Disable this eslinting so that users don't get confused
+    // built yet. Disable this eslint rule so that users don't get confused
     // needlessly and aren't required to build the linter for eslint
     // to pass.
     // eslint-disable-next-line import/no-unresolved
-  require('../dist/addons-linter').createInstance({ runAsBinary: true }).run();
+    instance = require('../dist/addons-linter').createInstance({
+      runAsBinary: true,
+    });
+    await instance.run();
   } catch (err) {
     if (err.code === 'MODULE_NOT_FOUND') {
       console.log('You did not build addons-linter yet.');
@@ -21,9 +27,24 @@ try {
         'Please run `npm install` and `npm run build` or see README.md for more information.'
       );
 
+      // Setting the `process.exitCode` and return from this entry point nodejs script
+      // is mostly the same as calling `process.exit(anExitCode)`.
+      //
+      // We prefer this approach because it would make it easier to cover this file in
+      // a unit test if we want to.
       process.exitCode = 1;
       return;
     }
 
+    // Print the error message without the stack if it is a user error
+    // and --stack cli option wasn't part of the cli option passed to the
+    // linter.
+    if (err.name === 'AddonsLinterUserError' && !instance.config.stack) {
+      console.error(err.message);
+      process.exitCode = 2;
+      return;
+    }
+
     throw err;
   }
+})();

src/linter.js

@@ -16,6 +16,7 @@ import {
   i18n,
   couldBeMinifiedCode,
   getLineAndColumnFromMatch,
+  AddonsLinterUserError,
 } from 'utils';
 import log from 'logger';
 import Collector from 'collector';
@@ -60,6 +61,20 @@ export default class Linter {
     return this._config;
   }
 
+  validateConfig() {
+    const { minManifestVersion, maxManifestVersion } = this.config;
+    if (maxManifestVersion < minManifestVersion) {
+      throw new AddonsLinterUserError(
+        i18n._(oneLine`
+        Invalid manifest version range requested:
+        --min-manifest-version (currently set to ${minManifestVersion})
+        should not be greater than
+        --max-manifest-version (currently set to ${maxManifestVersion}).
+      `)
+      );
+    }
+  }
+
   colorize(type) {
     switch (type) {
       case constants.VALIDATION_ERROR:
@@ -543,6 +558,11 @@ export default class Linter {
   }
 
   async run(deps = {}) {
+    // Validate the config options from a linter perspective (in addition to the
+    // yargs validation that already happened when the options are being parsed)
+    // and throws if there are invalid options.
+    this.validateConfig();
+
     if (this.config.metadata === true) {
       try {
         await this.extractMetadata(deps);

src/schema/validator.js

@@ -255,6 +255,18 @@ export class SchemaValidator {
         maxManifestVersion == null
           ? getDefaultConfigValue('max-manifest-version')
           : maxManifestVersion;
+
+      // Make sure the version range is valid, if it is not:
+      // raise an explicit error.
+      if (maxManifestVersion < minManifestVersion) {
+        throw new Error(
+          `Invalid manifest version range requested: ${JSON.stringify({
+            maxManifestVersion,
+            minManifestVersion,
+          })}`
+        );
+      }
+
       schemaData = deepPatch(this.schemaObject, {
         types: {
           ManifestBase: {

src/utils.js

@@ -13,6 +13,21 @@ import { PACKAGE_TYPES, LOCAL_PROTOCOLS } from 'const';
 
 const SOURCE_MAP_RE = new RegExp(/\/\/[#@]\s(source(?:Mapping)?URL)=\s*(\S+)/);
 
+// Represents an error condition related to a user error (e.g. an invalid
+// configuration option passed to the linter class, usually through the
+// command line arguments).
+//
+// In bin/addons-linter instances of this error are recognized through the
+// error name property and by default they will be logged on stderr as
+// plain error messages and the error stack trace omitted (unless explicitly
+// requested by passing --stack as an additional CLI options, useful for
+// debugging reasons).
+export class AddonsLinterUserError extends Error {
+  get name() {
+    return 'AddonsLinterUserError';
+  }
+}
+
 export function errorParamsToUnsupportedVersionRange(errorParams) {
   const { min_manifest_version, max_manifest_version } = errorParams || {};
   if (min_manifest_version != null || max_manifest_version != null) {

tests/integration/addons-linter/integration.run.js

@@ -1,5 +1,7 @@
 import path from 'path';
 
+import { oneLine } from 'common-tags';
+
 import { executeScript } from '../helpers';
 
 const expectedResult = '"summary":{"errors":0,"notices":0,"warnings":0}';
@@ -17,8 +19,8 @@ describe('Integration/smoke tests', () => {
       fixture,
     ]);
     expect(stdout).toContain(expectedResult);
-    expect(stderr).toBe('');
+    expect(stderr).toStrictEqual('');
-    expect(exitCode).toBe(0);
+    expect(exitCode).toEqual(0);
   });
 
   it('should ignore .eslintignore files', async () => {
@@ -34,8 +36,8 @@ describe('Integration/smoke tests', () => {
     );
 
     expect(stdout).toContain(expectedResult);
-    expect(stderr).toBe('');
+    expect(stderr).toStrictEqual('');
-    expect(exitCode).toBe(0);
+    expect(exitCode).toEqual(0);
   });
 
   it('should pass if ran on a simple valid CRX extension', async () => {
@@ -46,7 +48,24 @@ describe('Integration/smoke tests', () => {
       fixture,
     ]);
     expect(stdout).toContain('"summary":{"errors":0,"notices":0,"warnings":1}');
-    expect(stderr).toBe('');
+    expect(stderr).toStrictEqual('');
-    expect(exitCode).toBe(0);
+    expect(exitCode).toEqual(0);
+  });
+
+  it('should log the expected error message on invalid --min/max-manifest-version range', async () => {
+    const fixture = resolveFixturePath('webextension_es6_module');
+    const { exitCode, stderr } = await executeScript('addons-linter', [
+      '--min-manifest-version=3',
+      '--max-manifest-version=2',
+      fixture,
+    ]);
+    const expectedMessage = oneLine`
+        Invalid manifest version range requested:
+        --min-manifest-version (currently set to 3)
+        should not be greater than
+        --max-manifest-version (currently set to 2).
+      `;
+    expect(stderr).toStrictEqual(`${expectedMessage}\n`);
+    expect(exitCode).toEqual(2);
   });
 });

tests/unit/schema/test.schema.js

@@ -3,6 +3,21 @@ import { validateAddon, getValidator } from 'schema/validator';
 import { getValidatorWithFakeSchema, validManifest } from './helpers';
 
 describe('getValidator', () => {
+  it('throws on invalid manifest version range options', () => {
+    expect(() => {
+      const validator = getValidator({
+        minManifestVersion: 3,
+        maxManifestVersion: 2,
+        // No need to cache this validator instance.
+        forceNewValidatorInstance: true,
+      });
+      // Trigger the validator instance initialization
+      // to ensure we hit the error on invalid manifest
+      // version range.
+      validator._lazyInit();
+    }).toThrow(/Invalid manifest version range requested:/);
+  });
+
   it('returns different instances on different options', () => {
     const validatorDefault = getValidator({});
     const validatorMinV2 = getValidator({ minManifestVersion: 2 });

tests/unit/test.linter.js

@@ -15,6 +15,7 @@ import FilenameScanner from 'scanners/filename';
 import JavaScriptScanner from 'scanners/javascript';
 import JSONScanner from 'scanners/json';
 import LangpackScanner from 'scanners/langpack';
+import { AddonsLinterUserError } from 'utils';
 
 import {
   fakeMessageData,
@@ -51,6 +52,26 @@ class FakeIOBase {
 }
 
 describe('Linter', () => {
+  describe('validateConfig', () => {
+    it('should throw on invalid manifest version range options', async () => {
+      const addonLinter = new Linter({
+        _: ['foo'],
+        minManifestVersion: 3,
+        maxManifestVersion: 2,
+      });
+
+      sinon.spy(addonLinter, 'validateConfig');
+
+      await expect(addonLinter.run()).rejects.toThrow(AddonsLinterUserError);
+      sinon.assert.calledOnce(addonLinter.validateConfig);
+
+      await expect(addonLinter.run()).rejects.toThrow(
+        /Invalid manifest version range requested/
+      );
+      sinon.assert.calledTwice(addonLinter.validateConfig);
+    });
+  });
+
   it('should detect an invalid file with ENOENT', async () => {
     const addonLinter = new Linter({ _: ['foo'] });
     addonLinter.handleError = sinon.stub();

tests/unit/test.utils.js

@@ -2,6 +2,7 @@ import { oneLine } from 'common-tags';
 import bcd from '@mdn/browser-compat-data';
 
 import {
+  AddonsLinterUserError,
   basicCompatVersionComparison,
   buildI18nObject,
   checkMinNodeVersion,
@@ -714,3 +715,21 @@ describe('errorParamsToUnsupportedVersionRange', () => {
     }
   );
 });
+
+describe('AddonsLinterUserError', () => {
+  it('should be an instance of Error', () => {
+    const error = new AddonsLinterUserError();
+    expect(error instanceof Error).toStrictEqual(true);
+  });
+
+  it('should have name set to the expected error name', () => {
+    const error = new AddonsLinterUserError();
+    expect(error.name).toStrictEqual('AddonsLinterUserError');
+  });
+
+  it('should have message set to the expected string', () => {
+    const errorMessage = 'Expected Error Message';
+    const error = new AddonsLinterUserError(errorMessage);
+    expect(error.message).toStrictEqual(errorMessage);
+  });
+});