Breaking: throw an error for invalid global configs (refs #11338) (#1…

…1517)

lib/config/config-ops.js

@@ -377,6 +377,7 @@ module.exports = {
      * @param {(boolean|string|null)} configuredValue The value given for a global in configuration or in
      * a global directive comment
      * @returns {("readable"|"writeable"|"off")} The value normalized as a string
+     * @throws Error if global value is invalid
      */
     normalizeConfigGlobal(configuredValue) {
         switch (configuredValue) {
@@ -396,9 +397,8 @@ module.exports = {
             case "readonly":
                 return "readable";
 
-            // Fallback to minimize compatibility impact
             default:
-                return "writeable";
+                throw new Error(`'${configuredValue}' is not a valid configuration for a global (use 'readonly', 'writable', or 'off')`);
         }
     }
 };

lib/config/config-validator.js

@@ -13,7 +13,8 @@ const path = require("path"),
     ajv = require("../util/ajv"),
     lodash = require("lodash"),
     configSchema = require("../../conf/config-schema.js"),
-    util = require("util");
+    util = require("util"),
+    ConfigOps = require("./config-ops");
 
 const ruleValidators = new WeakMap();
 
@@ -177,6 +178,27 @@ function validateRules(rulesConfig, ruleMapper, source = null) {
     });
 }
 
+/**
+ * Validates a `globals` section of a config file
+ * @param {Object} globalsConfig The `glboals` section
+ * @param {string|null} source The name of the configuration source to report in the event of an error.
+ * @returns {void}
+ */
+function validateGlobals(globalsConfig, source = null) {
+    if (!globalsConfig) {
+        return;
+    }
+
+    Object.entries(globalsConfig)
+        .forEach(([configuredGlobal, configuredValue]) => {
+            try {
+                ConfigOps.normalizeConfigGlobal(configuredValue);
+            } catch (err) {
+                throw new Error(`ESLint configuration of global '${configuredGlobal}' in ${source} is invalid:\n${err.message}`);
+            }
+        });
+}
+
 /**
  * Formats an array of schema validation errors.
  * @param {Array} errors An array of error messages to format.
@@ -252,6 +274,7 @@ function validate(config, ruleMapper, envContext, source = null) {
     validateConfigSchema(config, source);
     validateRules(config.rules, ruleMapper, source);
     validateEnvironment(config.env, envContext, source);
+    validateGlobals(config.globals, source);
 
     for (const override of config.overrides || []) {
         validateRules(override.rules, ruleMapper, source);

lib/linter.js

@@ -73,8 +73,13 @@ const commentParser = new ConfigCommentParser();
 function addDeclaredGlobals(globalScope, configGlobals, commentDirectives) {
     const mergedGlobalsInfo = Object.assign(
         {},
+
+        /*
+         * `ConfigOps.normalizeConfigGlobal` will throw an error if a configured global value is invalid. However, these errors would
+         * typically be caught when validating a config anyway (validity for inline global comments is checked separately).
+         */
         lodash.mapValues(configGlobals, value => ({ sourceComment: null, value: ConfigOps.normalizeConfigGlobal(value) })),
-        lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value: ConfigOps.normalizeConfigGlobal(value) }))
+        lodash.mapValues(commentDirectives.enabledGlobals, ({ comment, value }) => ({ sourceComment: comment, value }))
     );
 
     Object.keys(mergedGlobalsInfo)
@@ -196,10 +201,33 @@ function getDirectiveComments(filename, ast, ruleMapper) {
                     break;
 
                 case "globals":
-                case "global":
-                    Object.assign(enabledGlobals, commentParser.parseStringConfig(directiveValue, comment));
-                    break;
+                case "global": {
+                    const updatedGlobals = commentParser.parseStringConfig(directiveValue, comment);
+
+                    Object.keys(updatedGlobals).forEach(globalName => {
+                        const { value } = updatedGlobals[globalName];
+                        let normalizedValue;
+
+                        try {
+                            normalizedValue = ConfigOps.normalizeConfigGlobal(value);
+                        } catch (err) {
+                            problems.push({
+                                ruleId: null,
+                                severity: 2,
+                                message: err.message,
+                                line: comment.loc.start.line,
+                                column: comment.loc.start.column + 1,
+                                endLine: comment.loc.end.line,
+                                endColumn: comment.loc.end.column + 1,
+                                nodeType: null
+                            });
+                            return;
+                        }
 
+                        enabledGlobals[globalName] = { comment, value: normalizedValue };
+                    });
+                    break;
+                }
                 case "eslint-disable":
                     disableDirectives.push(...createDisableDirectives("disable", comment.loc.start, directiveValue));
                     break;

tests/lib/config/config-ops.js

@@ -911,12 +911,18 @@ describe("ConfigOps", () => {
             ["writable", "writeable"],
             ["readable", "readable"],
             ["readonly", "readable"],
-            ["writable", "writeable"],
-            ["something else", "writeable"]
+            ["writable", "writeable"]
         ].forEach(([input, output]) => {
             it(util.inspect(input), () => {
                 assert.strictEqual(ConfigOps.normalizeConfigGlobal(input), output);
             });
         });
+
+        it("throws on other inputs", () => {
+            assert.throws(
+                () => ConfigOps.normalizeConfigGlobal("something else"),
+                /^'something else' is not a valid configuration for a global \(use 'readonly', 'writable', or 'off'\)$/u
+            );
+        });
     });
 });

tests/lib/config/config-validator.js

@@ -115,7 +115,7 @@ describe("Validator", () => {
             validator.validate(
                 {
                     root: true,
-                    globals: { globalFoo: "bar" },
+                    globals: { globalFoo: "readonly" },
                     parser: "parserFoo",
                     env: { browser: true },
                     plugins: ["pluginFoo", "pluginBar"],
@@ -325,6 +325,15 @@ describe("Validator", () => {
             });
         });
 
+        describe("globals", () => {
+            it("should disallow globals set to invalid values", () => {
+                assert.throws(
+                    () => validator.validate({ globals: { foo: "AAAAA" } }, () => {}, linter.environments, "tests"),
+                    "ESLint configuration of global 'foo' in tests is invalid:\n'AAAAA' is not a valid configuration for a global (use 'readonly', 'writable', or 'off')"
+                );
+            });
+        });
+
         describe("overrides", () => {
             it("should not throw with an empty overrides array", () => {
                 validator.validate({ overrides: [] }, ruleMapper, linter.environments, "tests");

tests/lib/linter.js

@@ -3490,6 +3490,34 @@ describe("Linter", () => {
             assert(ok);
         });
 
+        it("should report a linting error when a global is set to an invalid value", () => {
+            const results = linter.verify("/* global foo: AAAAA, bar: readonly */\nfoo;\nbar;", { rules: { "no-undef": "error" } });
+
+            assert.deepStrictEqual(results, [
+                {
+                    ruleId: null,
+                    severity: 2,
+                    message: "'AAAAA' is not a valid configuration for a global (use 'readonly', 'writable', or 'off')",
+                    line: 1,
+                    column: 1,
+                    endLine: 1,
+                    endColumn: 39,
+                    nodeType: null
+                },
+                {
+                    ruleId: "no-undef",
+                    messageId: "undef",
+                    severity: 2,
+                    message: "'foo' is not defined.",
+                    line: 2,
+                    column: 1,
+                    endLine: 2,
+                    endColumn: 4,
+                    nodeType: "Identifier"
+                }
+            ]);
+        });
+
         it("should not crash when we reuse the SourceCode object", () => {
             linter.verify("function render() { return <div className='test'>{hello}</div> }", { parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } });
             linter.verify(linter.getSourceCode(), { parserOptions: { ecmaVersion: 6, ecmaFeatures: { jsx: true } } });