fix: Use ESLint Linter API to more directly control parser and rules module loading (#4078)

* fix: Use ESLint Linter API to more directly control parser and rules module loading
* chore: Add smoke test to verify conflicting eslint version do not break addons-linter

.circleci/config.yml

@@ -123,6 +123,7 @@ jobs:
       # This should run after we persist the workspace because this command
       # invokes `npm install`.
       - run: npm run webext-test-functional
+      - run: npm run smoke-test-eslint-version-conflicts
       - store_artifacts:
           path: coverage
 
@@ -140,6 +141,7 @@ jobs:
       # production-like environment
       - run: npm run test-integration:production
       - run: npm run webext-test-functional
+      - run: npm run smoke-test-eslint-version-conflicts
 
   test-alternate:
     <<: *defaults-alternate
@@ -155,6 +157,7 @@ jobs:
       # production-like environment
       - run: npm run test-integration:production
       - run: npm run webext-test-functional
+      - run: npm run smoke-test-eslint-version-conflicts
 
   release-tag:
     <<: *defaults

.eslintignore

@@ -2,6 +2,7 @@ scripts/download-import-tag
 scripts/list-firefox-tags
 scripts/run-l10n-extraction
 scripts/webext-test-functional
+scripts/smoke-test-eslint-version-conflicts
 dist
 coverage
 tests/fixtures/**

.npmignore

@@ -8,9 +8,3 @@
 !bin/addons-linter
 !dist/addons-linter.*
 !dist/locale/**
-!dist/rules/**
-
-# Overridden .eslintignore file to prevent addons-linter
-# from loading an arbirary .eslintignore from the cwd
-# (See https://github.com/mozilla/addons-linter/issues/3390)
-!addons-linter.eslintignore

.prettierignore

@@ -6,6 +6,7 @@ scripts/download-import-tag
 scripts/list-firefox-tags
 scripts/run-l10n-extraction
 scripts/webext-test-functional
+scripts/smoke-test-eslint-version-conflicts
 package-lock.json
 LICENSE
 

addons-linter.eslintignore

@@ -1,3 +0,0 @@
-# Overridden .eslintignore file to prevent addons-linter
-# from loading an arbirary .eslintignore from the cwd
-# (See https://github.com/mozilla/addons-linter/issues/3390)

bin/addons-linter

@@ -3,7 +3,6 @@
 const path = require('path');
 
 const absoluteAppRoot = path.resolve(path.join(__dirname, '..'));
-global.appRoot = path.relative(process.cwd(), absoluteAppRoot);
 global.localesRoot = path.join(absoluteAppRoot, 'dist', 'locale');
 global.nodeRequire = require;
 

package.json

@@ -31,6 +31,7 @@
     "build-rules": "scripts/build-rules && cp node_modules/github-markdown-css/github-markdown.css docs/github-markdown.css",
     "gen-contributing-toc": "doctoc CONTRIBUTING.md",
     "webext-test-functional": "scripts/webext-test-functional",
+    "smoke-test-eslint-version-conflicts": "scripts/smoke-test-eslint-version-conflicts",
     "update-hashes": "scripts/dispensary > src/dispensary/hashes.txt"
   },
   "repository": {

scripts/smoke-test-eslint-version-conflicts

@@ -0,0 +1,36 @@
+#!/usr/bin/env bash
+
+set -e
+
+LINTER_PATH=$(pwd)
+TARGET_TEST_PATH="$LINTER_PATH/tmp/test-project"
+FIXTURE_TEST_PATH="$LINTER_PATH/tests/fixtures/eslint-versions-conflicts-package"
+LINTER_VERSION=$(node -p 'require("./package.json").version')
+
+# Fail if the target path already exists.
+[[ -f "$TARGET_TEST_PATH" ]] && (
+  echo "ERROR: Temporary test dir '$TARGET_TEST_PATH' already exists."
+  exit 1
+)
+
+echo -e "INFO: Initializing test environment\n"
+
+# Build addons-linter
+npm ci
+npm run build
+
+# Create target test path directory and cleanup on exit.
+cp -rf "$FIXTURE_TEST_PATH" "$TARGET_TEST_PATH"
+trap 'rm -rf "$TARGET_TEST_PATH"' EXIT
+
+pushd "$TARGET_TEST_PATH"
+npm pack "$LINTER_PATH"
+npm install
+npm install --save-dev "./addons-linter-$LINTER_VERSION.tgz"
+
+echo -e "INFO: Run addons-linter on the test fixture project\n"
+
+npx addons-linter ./src || (
+  echo "ERROR: addons-linter validation failed".
+  exit 2
+)

src/rules/javascript/index.js

@@ -0,0 +1,25 @@
+import content_scripts_file_absent from './content-scripts-file-absent';
+import deprecated_entities from './deprecated-entities';
+import event_listener_fourth from './event-listener-fourth';
+import global_require_arg from './global-require-arg';
+import opendialog_nonlit_uri from './opendialog-nonlit-uri';
+import opendialog_remote_uri from './opendialog-remote-uri';
+import webextension_api from './webextension-api';
+import webextension_api_compat from './webextension-api-compat';
+import webextension_api_compat_android from './webextension-api-compat-android';
+import webextension_deprecated_api from './webextension-deprecated-api';
+import webextension_unsupported_api from './webextension-unsupported-api';
+
+export default {
+  'content-scripts-file-absent': content_scripts_file_absent,
+  'deprecated-entities': deprecated_entities,
+  'event-listener-fourth': event_listener_fourth,
+  'global-require-arg': global_require_arg,
+  'opendialog-nonlit-uri': opendialog_nonlit_uri,
+  'opendialog-remote-uri': opendialog_remote_uri,
+  'webextension-api': webextension_api,
+  'webextension-api-compat': webextension_api_compat,
+  'webextension-api-compat-android': webextension_api_compat_android,
+  'webextension-deprecated-api': webextension_deprecated_api,
+  'webextension-unsupported-api': webextension_unsupported_api,
+};

src/scanners/javascript.js

@@ -1,7 +1,5 @@
-/* global appRoot */
-import path from 'path';
-
 import ESLint from 'eslint';
+import noUnsanitized from 'eslint-plugin-no-unsanitized';
 import { oneLine } from 'common-tags';
 import * as espree from 'espree';
 import * as vk from 'eslint-visitor-keys';
@@ -11,7 +9,7 @@ import { ESLINT_RULE_MAPPING, ESLINT_TYPES } from 'const';
 import * as messages from 'messages';
 import { ensureFilenameExists } from 'utils';
 
-const IGNORE_FILE = 'addons-linter.eslintignore';
+import customEslintRules from '../rules/javascript';
 
 export default class JavaScriptScanner {
   disabledRules = [];
@@ -45,10 +43,9 @@ export default class JavaScriptScanner {
     _ESLint = ESLint,
     _messages = messages,
     _ruleMapping = ESLINT_RULE_MAPPING,
-    // This tells ESLint where to expect the ESLint rules for addons-linter.
-    // Its default value is defined below. This property is mainly used for
-    // testing purposes.
-    _rulePaths = undefined,
+    // This property is used to inject additional custom eslint rules
+    // as part of tests.
+    _rules = undefined,
   } = {}) {
     const detectedSourceType = this.detectSourceType(this.filename);
     this.sourceType = detectedSourceType.sourceType;
@@ -61,145 +58,145 @@ export default class JavaScriptScanner {
       }
     });
 
-    const root =
-      typeof appRoot !== 'undefined' ? appRoot : path.join(__dirname, '..');
+    const linter = new _ESLint.Linter();
+
+    // Load additional rules injected by unit tests.
+    if (_rules) {
+      for (const ruleName of Object.keys(_rules)) {
+        linter.defineRule(ruleName, _rules[ruleName]);
+      }
+    }
+
+    // Load custom eslint rules embedded into addons-linter bundle.
+    for (const key of Object.keys(customEslintRules)) {
+      linter.defineRule(key, customEslintRules[key]);
+    }
+
+    // Load plugins rules.
+    const pluginRules = noUnsanitized.rules;
+    for (const key of Object.keys(pluginRules)) {
+      linter.defineRule(`no-unsanitized/${key}`, pluginRules[key]);
+    }
+
+    linter.defineParser('addons-linter-espree', espree);
+
     const eslintConfig = {
-      resolvePluginsRelativeTo: path.resolve(root),
-      // The default value for `rulePaths` is configured so that it finds the
-      // files exported by webpack when this project is built.
-      rulePaths: _rulePaths || [path.join(root, 'dist', 'rules', 'javascript')],
-      allowInlineConfig: false,
+      env: {
+        browser: true,
+        es6: true,
+        webextensions: true,
+      },
 
-      // Avoid loading the addons-linter .eslintrc file
-      useEslintrc: false,
+      // Ensure we use the same parser and parserOptions used to detect
+      // the sourceType.
+      parser: 'addons-linter-espree',
+      parserOptions: {
+        ecmaVersion: ECMA_VERSION,
+        sourceType: this.sourceType,
+      },
 
-      // Avoid loading the .eslintignore file from the cwd
-      // by explicitly configuring eslint with a custom ignore file
-      // packaged with the addons-linter npm package.
-      ignorePath: path.join(path.resolve(root), IGNORE_FILE),
+      rules,
+      plugins: ['no-unsanitized'],
 
-      baseConfig: {
-        env: {
-          browser: true,
-          es6: true,
-          webextensions: true,
-        },
-
-        // It's the default but also shouldn't change since we're using
-        // espree to parse javascript files below manually to figure out
-        // if they're modules or not
-        parser: 'espree',
-        parserOptions: {
-          ecmaVersion: ECMA_VERSION,
-          sourceType: this.sourceType,
-        },
-
-        rules,
-        plugins: ['no-unsanitized'],
-
-        // Scan files in `node_modules/` as well as dotfiles. As of ESLInt 7.0,
-        // bower files are scanned.
-        // See: https://github.com/mozilla/addons-linter/issues/1288
-        // See: https://eslint.org/docs/user-guide/migrating-to-7.0.0#default-ignore-patterns-have-changed
-        ignorePatterns: ['!node_modules/*', '!.*'],
-        settings: {
-          addonMetadata: this.options.addonMetadata,
-          existingFiles: this.options.existingFiles,
-        },
+      // Scan files in `node_modules/` as well as dotfiles. As of ESLInt 7.0,
+      // bower files are scanned.
+      // See: https://github.com/mozilla/addons-linter/issues/1288
+      // See: https://eslint.org/docs/user-guide/migrating-to-7.0.0#default-ignore-patterns-have-changed
+      ignorePatterns: ['!node_modules/*', '!.*'],
+      settings: {
+        addonMetadata: this.options.addonMetadata,
+        existingFiles: this.options.existingFiles,
       },
     };
 
-    const cli = new _ESLint.ESLint(eslintConfig);
-    const results = await cli.lintText(this.code, {
-      filePath: this.filename,
-      warnIgnored: true,
+    const results = linter.verify(this.code, eslintConfig, {
+      allowInlineConfig: false,
+      filename: this.filename,
     });
 
     // eslint prepends the filename with the current working directory,
     // strip that out.
     this.scannedFiles.push(this.filename);
 
-    results.forEach((result) => {
-      result.messages.forEach((message) => {
-        let extraShortDescription = '';
+    results.forEach((message) => {
+      let extraShortDescription = '';
 
-        // Fatal error messages (like SyntaxErrors) are a bit different, we
-        // need to handle them specially. Messages related to parsing errors do
-        // not have a `ruleId`, which is why we check that, too.
-        if (message.fatal === true && message.ruleId === null) {
-          // If there was a parsing error during the sourceType detection, we
-          // want to add it to the short description. We start by adding it to
-          // a temporary variable in case there are other messages we want to
-          // append to the final short description (which will be the `message`
-          // in the final output).
-          if (detectedSourceType.parsingError !== null) {
-            const { type, error } = detectedSourceType.parsingError;
-            extraShortDescription = `(Parsing as ${type} error: ${error})`;
-          }
-
-          // If there was another error, we want to append it to the short
-          // description as well. `message.message` will contain the full
-          // exception message, which likely includes a prefix that we don't
-          // want to keep.
-          const formattedError = message.message.replace('Parsing error: ', '');
-          extraShortDescription = [
-            extraShortDescription,
-            oneLine`(Parsing as ${this.sourceType} error: ${formattedError} at
-              line: ${message.line} and column: ${message.column})`,
-          ].join(' ');
-          // eslint-disable-next-line no-param-reassign
-          message.message = _messages.JS_SYNTAX_ERROR.code;
+      // Fatal error messages (like SyntaxErrors) are a bit different, we
+      // need to handle them specially. Messages related to parsing errors do
+      // not have a `ruleId`, which is why we check that, too.
+      if (message.fatal === true && message.ruleId === null) {
+        // If there was a parsing error during the sourceType detection, we
+        // want to add it to the short description. We start by adding it to
+        // a temporary variable in case there are other messages we want to
+        // append to the final short description (which will be the `message`
+        // in the final output).
+        if (detectedSourceType.parsingError !== null) {
+          const { type, error } = detectedSourceType.parsingError;
+          extraShortDescription = `(Parsing as ${type} error: ${error})`;
         }
 
-        if (typeof message.message === 'undefined') {
-          throw new Error(
-            oneLine`JS rules must pass a valid message as
-            the second argument to context.report()`
-          );
+        // If there was another error, we want to append it to the short
+        // description as well. `message.message` will contain the full
+        // exception message, which likely includes a prefix that we don't
+        // want to keep.
+        const formattedError = message.message.replace('Parsing error: ', '');
+        extraShortDescription = [
+          extraShortDescription,
+          oneLine`(Parsing as ${this.sourceType} error: ${formattedError} at
+            line: ${message.line} and column: ${message.column})`,
+        ].join(' ');
+        // eslint-disable-next-line no-param-reassign
+        message.message = _messages.JS_SYNTAX_ERROR.code;
+      }
+
+      if (typeof message.message === 'undefined') {
+        throw new Error(
+          oneLine`JS rules must pass a valid message as
+          the second argument to context.report()`
+        );
+      }
+
+      // Fallback to looking up the message object by the message
+      let code = message.message;
+      let shortDescription;
+      let description;
+
+      // Support 3rd party eslint rules that don't have our internal
+      // message structure and allow us to optionally overwrite
+      // their `message` and `description`.
+      if (Object.prototype.hasOwnProperty.call(_messages, code)) {
+        ({ message: shortDescription, description } = _messages[code]);
+      } else if (
+        Object.prototype.hasOwnProperty.call(
+          messages.ESLINT_OVERWRITE_MESSAGE,
+          message.ruleId
+        )
+      ) {
+        const overwrites = messages.ESLINT_OVERWRITE_MESSAGE[message.ruleId];
+        shortDescription = overwrites.message || message.message;
+        description = overwrites.description || message.description;
+
+        if (overwrites.code) {
+          code = overwrites.code;
         }
+      } else {
+        shortDescription = code;
+        description = null;
+      }
 
-        // Fallback to looking up the message object by the message
-        let code = message.message;
-        let shortDescription;
-        let description;
+      if (extraShortDescription.length) {
+        shortDescription += ` ${extraShortDescription}`;
+      }
 
-        // Support 3rd party eslint rules that don't have our internal
-        // message structure and allow us to optionally overwrite
-        // their `message` and `description`.
-        if (Object.prototype.hasOwnProperty.call(_messages, code)) {
-          ({ message: shortDescription, description } = _messages[code]);
-        } else if (
-          Object.prototype.hasOwnProperty.call(
-            messages.ESLINT_OVERWRITE_MESSAGE,
-            message.ruleId
-          )
-        ) {
-          const overwrites = messages.ESLINT_OVERWRITE_MESSAGE[message.ruleId];
-          shortDescription = overwrites.message || message.message;
-          description = overwrites.description || message.description;
-
-          if (overwrites.code) {
-            code = overwrites.code;
-          }
-        } else {
-          shortDescription = code;
-          description = null;
-        }
-
-        if (extraShortDescription.length) {
-          shortDescription += ` ${extraShortDescription}`;
-        }
-
-        this.linterMessages.push({
-          code,
-          column: message.column,
-          description,
-          file: this.filename,
-          line: message.line,
-          message: shortDescription,
-          sourceCode: message.source,
-          type: ESLINT_TYPES[message.severity],
-        });
+      this.linterMessages.push({
+        code,
+        column: message.column,
+        description,
+        file: this.filename,
+        line: message.line,
+        message: shortDescription,
+        sourceCode: message.source,
+        type: ESLINT_TYPES[message.severity],
       });
     });
 

tests/fixtures/eslint-versions-conflicts-package/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "eslint-versions-conflicts-package",
+  "version": "1.0.0",
+  "description": "test issues with conflicting eslint versions under the same package",
+  "keywords": [],
+  "author": "",
+  "license": "MPL-v2",
+  "devDependencies": {
+    "eslint": "^7.32.0"
+  }
+}

tests/fixtures/eslint-versions-conflicts-package/src/background.js

@@ -0,0 +1 @@
+console.log("background page loaded");

tests/fixtures/eslint-versions-conflicts-package/src/manifest.json

@@ -0,0 +1,11 @@
+{
+  "manifest_version": 2,
+  "name": "test-extension",
+  "version": "0.1",
+  "permissions": [],
+  "background": {
+    "scripts": [
+      "background.js"
+    ]
+  }
+}

tests/setup.js

@@ -25,9 +25,7 @@ jest.mock('cli', () => {
   };
 });
 
-global.appRoot = path.join(__dirname, '..');
-
-if (!fs.existsSync(path.join(global.appRoot, 'dist'))) {
+if (!fs.existsSync(path.join(__dirname, '..', 'dist'))) {
   throw new Error('Please run `npm run build` before running the test suite.');
 }
 

tests/unit/helpers.js

@@ -368,14 +368,17 @@ export const getJsRulePathForRule = (ruleName) => {
   return path.join(FIXTURES_DIR, 'rules', 'javascript', ruleName);
 };
 
-export function runJsScanner(
+export async function runJsScanner(
   jsScanner,
   { fixtureRules = [], scanOptions } = {}
 ) {
-  const ruleSource = path.join(global.appRoot, 'src/rules/javascript');
-  const fixturePaths = fixtureRules.map(getJsRulePathForRule);
+  const _rules = {};
+  for (const ruleName of fixtureRules) {
+    const mod = await import(getJsRulePathForRule(ruleName));
+    _rules[ruleName] = mod.default ? mod.default : mod;
+  }
   return jsScanner.scan({
     ...scanOptions,
-    _rulePaths: [ruleSource].concat(fixturePaths),
+    _rules,
   });
 }

tests/unit/rules/javascript/test.eslintRulesObject.js

@@ -4,15 +4,30 @@ import { readdirSync } from 'fs';
 import { ESLINT_RULE_MAPPING } from 'const';
 
 describe('Eslint rules object', () => {
-  it('should have files that match the keys', () => {
-    const files = readdirSync('src/rules/javascript');
-    files.forEach((fileName) => {
+  const files = readdirSync('src/rules/javascript').filter(
+    (fileName) => fileName !== 'index.js'
+  );
+
+  it.each(files)(
+    '%s rule module should have a matching rule mapping',
+    (fileName) => {
       expect(
         Object.prototype.hasOwnProperty.call(
           ESLINT_RULE_MAPPING,
           path.parse(fileName).name
         )
       ).toBe(true);
-    });
-  });
+    }
+  );
+
+  it.each(files)(
+    '%s rule module should be exported by the index.js module',
+    async (fileName) => {
+      const modName = path.parse(fileName).name;
+      const indexMod = (await import('rules/javascript')).default;
+      const ruleMod = (await import(`rules/javascript/${fileName}`)).default;
+      expect(typeof indexMod[modName]?.create).toBe('function');
+      expect(indexMod[modName]).toBe(ruleMod);
+    }
+  );
 });

tests/unit/rules/javascript/test.unsupported_mv2_browser_api.js

@@ -4,16 +4,17 @@ import * as messages from 'messages';
 
 import { runJsScanner } from '../../helpers';
 
+jest.mock('schema/browser-apis.js', () => {
+  return {
+    hasBrowserApi: () => false,
+    isMV2RemovedApi: () => true,
+  };
+});
+
 describe('unsupported manifest v2 APIs tested with mock', () => {
   beforeAll(() => jest.resetModules());
 
   it('returns expected message for APIs unsupported in manifest_version >= 3', async () => {
-    jest.doMock('schema/browser-apis.js', () => {
-      return {
-        hasBrowserApi: () => false,
-        isMV2RemovedApi: () => true,
-      };
-    });
     const jsScanner = new JavaScriptScanner(
       'browser.pageAction.show();',
       'code.js',

tests/unit/scanners/test.javascript.js

@@ -241,23 +241,18 @@ describe('JavaScript Scanner', () => {
   });
 
   it('should reject on missing message code', async () => {
-    class FakeESLintClass {
-      async lintText() {
-        return Promise.resolve([
-          {
-            filePath: 'badcode.js',
-            messages: [
-              {
-                fatal: false,
-              },
-            ],
-          },
-        ]);
+    class FakeLinterClass {
+      defineRule() {}
+
+      defineParser() {}
+
+      verify() {
+        return [{ fatal: false }];
       }
     }
 
     const FakeESLint = {
-      ESLint: FakeESLintClass,
+      Linter: FakeLinterClass,
     };
 
     const jsScanner = new JavaScriptScanner('whatever', 'badcode.js');
@@ -317,7 +312,6 @@ describe('JavaScript Scanner', () => {
     const fakeMetadata = {
       addonMetadata: validMetadata({ guid: 'snowflake' }),
     };
-    const fakeESLintMapping = { 'metadata-not-passed': ESLINT_ERROR };
 
     const jsScanner = new JavaScriptScanner(
       'var hello = "something";',
@@ -328,7 +322,7 @@ describe('JavaScript Scanner', () => {
     const { linterMessages } = await runJsScanner(jsScanner, {
       scanOptions: {
         _messages: fakeMessages,
-        _ruleMapping: fakeESLintMapping,
+        _ruleMapping: { 'metadata-not-passed': ESLINT_ERROR },
       },
       fixtureRules: ['metadata-not-passed'],
     });

webpack.config.js

@@ -1,5 +1,4 @@
 /* eslint import/order: 0 */
-const glob = require('glob');
 const path = require('path');
 
 // eslint-disable-next-line import/no-extraneous-dependencies
@@ -12,10 +11,6 @@ module.exports = {
   // webpack3 bundling step.
   mode: 'none',
   entry: {
-    ...glob.sync('./src/rules/javascript/*.js').reduce((acc, file) => {
-      acc[file.replace(/^\.\/src\//, '').replace('.js', '')] = file;
-      return acc;
-    }, {}),
     'addons-linter': './src/main.js',
   },
   target: 'node',