Repository: eslint/eslint
Pull request #11356 (2019-02-06 01:44:48)

Docs: clarify motivation for no-prototype-builtins
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Documentation update

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

The first section in the no-prototype-builtins docs mentions objects created with Object.create(null), but does not mention builtin shadowing (which is arguably a much more compelling reason to use the rule). This commit updates the description to clarify the risks of using prototype builtins on user input.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular



Patch:
diff --git a/docs/rules/no-prototype-builtins.md b/docs/rules/no-prototype-builtins.md
index e2dd4f5b9..7132bcbdf 100644
--- a/docs/rules/no-prototype-builtins.md
+++ b/docs/rules/no-prototype-builtins.md
@@ -2,6 +2,10 @@
 
 In ECMAScript 5.1, `Object.create` was added, which enables the creation of objects with a specified `[[Prototype]]`. `Object.create(null)` is a common pattern used to create objects that will be used as a Map. This can lead to errors when it is assumed that objects will have properties from `Object.prototype`. This rule prevents calling some `Object.prototype` methods directly from an object.
 
+Additionally, objects can have properties that shadow the builtins on `Object.prototype`, potentially causing unintended behavior or denial-of-service security vulnerabilities. For example, it would be unsafe for a webserver to parse JSON input from a client and call `hasOwnProperty` directly on the resulting object, because a malicious client could send a JSON value like `{"hasOwnProperty": 1}` and cause the server to crash.
+
+To avoid subtle bugs like this, it's better to always call these methods from `Object.prototype`. For example, `foo.hasOwnProperty("bar")` should be replaced with `Object.prototype.hasOwnProperty.call(foo, "bar")`.
+
 ## Rule Details
 
 This rule disallows calling some `Object.prototype` methods directly on object instances.
@@ -32,4 +36,4 @@ var barIsEnumerable = {}.propertyIsEnumerable.call(foo, "bar");
 
 ## When Not To Use It
 
-You may want to turn this rule off if you will never use an object that shadows an `Object.prototype` method or which does not inherit from `Object.prototype`.
+You may want to turn this rule off if your code only touches objects with hardcoded keys, and you will never use an object that shadows an `Object.prototype` method or which does not inherit from `Object.prototype`.
diff --git a/docs/rules/no-shadow-restricted-names.md b/docs/rules/no-shadow-restricted-names.md
index 0624a82aa..9dfc03719 100644
--- a/docs/rules/no-shadow-restricted-names.md
+++ b/docs/rules/no-shadow-restricted-names.md
@@ -19,7 +19,7 @@ function NaN(){}
 
 !function(Infinity){};
 
-var undefined;
+var undefined = 5;
 
 try {} catch(eval){}
 ```
@@ -32,6 +32,9 @@ Examples of **correct** code for this rule:
 var Object;
 
 function f(a, b){}
+
+// Exception: `undefined` may be shadowed if the variable is never assigned a value.
+var undefined;
 ```
 
 ## Further Reading
diff --git a/docs/rules/no-useless-call.md b/docs/rules/no-useless-call.md
index 0356c1270..196cc6df3 100644
--- a/docs/rules/no-useless-call.md
+++ b/docs/rules/no-useless-call.md
@@ -37,6 +37,7 @@ obj.foo.call(otherObj, 1, 2, 3);
 obj.foo.apply(otherObj, [1, 2, 3]);
 
 // The argument list is variadic.
+// Those are warned by the `prefer-spread` rule.
 foo.apply(undefined, args);
 foo.apply(null, args);
 obj.foo.apply(obj, args);
@@ -66,3 +67,7 @@ a[++i].foo.call(a[i], 1, 2, 3);
 ## When Not To Use It
 
 If you don't want to be notified about unnecessary `.call()` and `.apply()`, you can safely disable this rule.
+
+## Related Rules
+
+* [prefer-spread](prefer-spread.md)
diff --git a/docs/rules/one-var.md b/docs/rules/one-var.md
index c35082ede..3c964c4a4 100644
--- a/docs/rules/one-var.md
+++ b/docs/rules/one-var.md
@@ -324,7 +324,9 @@ Examples of **correct** code for this rule with the `{ separateRequires: true }`
 
 var foo = require("foo");
 var bar = "bar";
+```
 
+```js
 var foo = require("foo"),
     bar = require("bar");
 ```
diff --git a/lib/config/config-file.js b/lib/config/config-file.js
index dde79cb40..f76b92830 100644
--- a/lib/config/config-file.js
+++ b/lib/config/config-file.js
@@ -280,15 +280,38 @@ function writeYAMLConfigFile(config, filePath) {
  * Writes a configuration file in JavaScript format.
  * @param {Object} config The configuration object to write.
  * @param {string} filePath The filename to write to.
+ * @throws {Error} If an error occurs linting the config file contents.
  * @returns {void}
  * @private
  */
 function writeJSConfigFile(config, filePath) {
     debug(`Writing JS config file: ${filePath}`);
 
-    const content = `module.exports = ${stringify(config, { cmp: sortByKey, space: 4 })};`;
+    let contentToWrite;
+    const stringifiedContent = `module.exports = ${stringify(config, { cmp: sortByKey, space: 4 })};`;
 
-    fs.writeFileSync(filePath, content, "utf8");
+    try {
+        const CLIEngine = require("../cli-engine");
+        const linter = new CLIEngine({
+            baseConfig: config,
+            fix: true,
+            useEslintrc: false
+        });
+        const report = linter.executeOnText(stringifiedContent);
+
+        contentToWrite = report.results[0].output || stringifiedContent;
+    } catch (e) {
+        debug("Error linting JavaScript config file, writing unlinted version");
+        const errorMessage = e.message;
+
+        contentToWrite = stringifiedContent;
+        e.message = "An error occurred while generating your JavaScript config file. ";
+        e.message += "A config file was still generated, but the config file itself may not follow your linting rules.";
+        e.message += `\nError: ${errorMessage}`;
+        throw e;
+    } finally {
+        fs.writeFileSync(filePath, contentToWrite, "utf8");
+    }
 }
 
 /**
diff --git a/lib/rules/no-shadow-restricted-names.js b/lib/rules/no-shadow-restricted-names.js
index 9bdd50868..f09f3767d 100644
--- a/lib/rules/no-shadow-restricted-names.js
+++ b/lib/rules/no-shadow-restricted-names.js
@@ -4,6 +4,19 @@
  */
 "use strict";
 
+/**
+ * Determines if a variable safely shadows undefined.
+ * This is the case when a variable named `undefined` is never assigned to a value (i.e. it always shares the same value
+ * as the global).
+ * @param {eslintScope.Variable} variable The variable to check
+ * @returns {boolean} true if this variable safely shadows `undefined`
+ */
+function safelyShadowsUndefined(variable) {
+    return variable.name === "undefined" &&
+        variable.references.every(ref => !ref.isWrite()) &&
+        variable.defs.every(def => def.node.type === "VariableDeclarator" && def.node.init === null);
+}
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -24,12 +37,13 @@ module.exports = {
 
     create(context) {
 
-        const RESTRICTED = ["undefined", "NaN", "Infinity", "arguments", "eval"];
+
+        const RESTRICTED = new Set(["undefined", "NaN", "Infinity", "arguments", "eval"]);
 
         return {
             "VariableDeclaration, :function, CatchClause"(node) {
                 for (const variable of context.getDeclaredVariables(node)) {
-                    if (variable.defs.length > 0 && RESTRICTED.includes(variable.name)) {
+                    if (variable.defs.length > 0 && RESTRICTED.has(variable.name) && !safelyShadowsUndefined(variable)) {
                         context.report({
                             node: variable.defs[0].name,
                             message: "Shadowing of global property '{{idName}}'.",
diff --git a/package.json b/package.json
index c3ad0008a..f1776d804 100644
--- a/package.json
+++ b/package.json
@@ -47,7 +47,7 @@
     "espree": "^5.0.0",
     "esquery": "^1.0.1",
     "esutils": "^2.0.2",
-    "file-entry-cache": "^2.0.0",
+    "file-entry-cache": "^5.0.1",
     "functional-red-black-tree": "^1.0.1",
     "glob": "^7.1.2",
     "globals": "^11.7.0",


Test patch:
diff --git a/tests/lib/cli-engine.js b/tests/lib/cli-engine.js
index fd961cb63..f2cfb0318 100644
--- a/tests/lib/cli-engine.js
+++ b/tests/lib/cli-engine.js
@@ -21,6 +21,8 @@ const assert = require("chai").assert,
 
 const proxyquire = require("proxyquire").noCallThru().noPreserveCache();
 
+const fCache = require("file-entry-cache");
+
 //------------------------------------------------------------------------------
 // Tests
 //------------------------------------------------------------------------------
@@ -2375,11 +2377,12 @@ describe("CLIEngine", () => {
 
                 assert.isTrue(shell.test("-f", cacheFile), "the cache for eslint was created");
 
-                const cache = JSON.parse(fs.readFileSync(cacheFile));
+                const fileCache = fCache.createFromFile(cacheFile);
+                const { cache } = fileCache;
 
-                assert.isTrue(typeof cache[goodFile] === "object", "the entry for the good file is in the cache");
+                assert.isTrue(typeof cache.getKey(goodFile) === "object", "the entry for the good file is in the cache");
 
-                assert.isTrue(typeof cache[badFile] === "object", "the entry for the bad file is in the cache");
+                assert.isTrue(typeof cache.getKey(badFile) === "object", "the entry for the bad file is in the cache");
 
                 const cachedResult = engine.executeOnFiles([badFile, goodFile]);
 
@@ -2412,9 +2415,10 @@ describe("CLIEngine", () => {
 
                 engine.executeOnFiles([badFile, goodFile, toBeDeletedFile]);
 
-                let cache = JSON.parse(fs.readFileSync(cacheFile));
+                const fileCache = fCache.createFromFile(cacheFile);
+                let { cache } = fileCache;
 
-                assert.isTrue(typeof cache[toBeDeletedFile] === "object", "the entry for the file to be deleted is in the cache");
+                assert.isTrue(typeof cache.getKey(toBeDeletedFile) === "object", "the entry for the file to be deleted is in the cache");
 
                 // delete the file from the file system
                 fs.unlinkSync(toBeDeletedFile);
@@ -2456,9 +2460,10 @@ describe("CLIEngine", () => {
 
                 engine.executeOnFiles([badFile, goodFile, testFile2]);
 
-                let cache = JSON.parse(fs.readFileSync(cacheFile));
+                let fileCache = fCache.createFromFile(cacheFile);
+                let { cache } = fileCache;
 
-                assert.isTrue(typeof cache[testFile2] === "object", "the entry for the test-file2 is in the cache");
+                assert.isTrue(typeof cache.getKey(testFile2) === "object", "the entry for the test-file2 is in the cache");
 
                 /*
                  * we pass a different set of files minus test-file2
@@ -2467,9 +2472,10 @@ describe("CLIEngine", () => {
                  */
                 engine.executeOnFiles([badFile, goodFile]);
 
-                cache = JSON.parse(fs.readFileSync(cacheFile));
+                fileCache = fCache.createFromFile(cacheFile);
+                cache = fileCache.cache;
 
-                assert.isTrue(typeof cache[testFile2] === "object", "the entry for the test-file2 is in the cache");
+                assert.isTrue(typeof cache.getKey(testFile2) === "object", "the entry for the test-file2 is in the cache");
             });
 
             it("should not delete cache when executing on text", () => {
@@ -2590,11 +2596,12 @@ describe("CLIEngine", () => {
 
                     assert.isTrue(shell.test("-f", customCacheFile), "the cache for eslint was created");
 
-                    const cache = JSON.parse(fs.readFileSync(customCacheFile));
+                    const fileCache = fCache.createFromFile(customCacheFile);
+                    const { cache } = fileCache;
 
-                    assert.isTrue(typeof cache[goodFile] === "object", "the entry for the good file is in the cache");
+                    assert.isTrue(typeof cache.getKey(goodFile) === "object", "the entry for the good file is in the cache");
 
-                    assert.isTrue(typeof cache[badFile] === "object", "the entry for the bad file is in the cache");
+                    assert.isTrue(typeof cache.getKey(badFile) === "object", "the entry for the bad file is in the cache");
 
                     const cachedResult = engine.executeOnFiles([badFile, goodFile]);
 
diff --git a/tests/lib/config/config-file.js b/tests/lib/config/config-file.js
index 944ed8426..c884c7b15 100644
--- a/tests/lib/config/config-file.js
+++ b/tests/lib/config/config-file.js
@@ -20,6 +20,7 @@ const Module = require("module"),
     espree = require("espree"),
     ConfigFile = require("../../../lib/config/config-file"),
     Linter = require("../../../lib/linter"),
+    CLIEngine = require("../../../lib/cli-engine"),
     Config = require("../../../lib/config");
 
 const userHome = os.homedir();
@@ -1262,6 +1263,51 @@ describe("ConfigFile", () => {
 
         });
 
+        it("should make sure js config files match linting rules", () => {
+            const fakeFS = leche.fake(fs);
+
+            const singleQuoteConfig = {
+                rules: {
+                    quotes: [2, "single"]
+                }
+            };
+
+            sandbox.mock(fakeFS).expects("writeFileSync").withExactArgs(
+                "test-config.js",
+                sinon.match(value => !value.includes("\"")),
+                "utf8"
+            );
+
+            const StubbedConfigFile = proxyquire("../../../lib/config/config-file", {
+                fs: fakeFS
+            });
+
+            StubbedConfigFile.write(singleQuoteConfig, "test-config.js");
+        });
+
+        it("should still write a js config file even if linting fails", () => {
+            const fakeFS = leche.fake(fs);
+            const fakeCLIEngine = sandbox.mock().withExactArgs(sinon.match({
+                baseConfig: config,
+                fix: true,
+                useEslintrc: false
+            }));
+
+            fakeCLIEngine.prototype = leche.fake(CLIEngine.prototype);
+            sandbox.stub(fakeCLIEngine.prototype, "executeOnText").throws();
+
+            sandbox.mock(fakeFS).expects("writeFileSync").once();
+
+            const StubbedConfigFile = proxyquire("../../../lib/config/config-file", {
+                fs: fakeFS,
+                "../cli-engine": fakeCLIEngine
+            });
+
+            assert.throws(() => {
+                StubbedConfigFile.write(config, "test-config.js");
+            });
+        });
+
         it("should throw error if file extension is not valid", () => {
             assert.throws(() => {
                 ConfigFile.write({}, getFixturePath("yaml/.eslintrc.class"));
diff --git a/tests/lib/rules/no-shadow-restricted-names.js b/tests/lib/rules/no-shadow-restricted-names.js
index 3c014a70f..cfc40a82c 100644
--- a/tests/lib/rules/no-shadow-restricted-names.js
+++ b/tests/lib/rules/no-shadow-restricted-names.js
@@ -24,6 +24,13 @@ ruleTester.run("no-shadow-restricted-names", rule, {
         {
             code: "try {} catch {}",
             parserOptions: { ecmaVersion: 2019 }
+        },
+        "var undefined;",
+        "var undefined; doSomething(undefined);",
+        "var undefined; var undefined;",
+        {
+            code: "let undefined",
+            parserOptions: { ecmaVersion: 2015 }
         }
     ],
     invalid: [
@@ -39,13 +46,12 @@ ruleTester.run("no-shadow-restricted-names", rule, {
             ]
         },
         {
-            code: "function undefined(undefined) { var undefined; !function undefined(undefined) { try {} catch(undefined) {} }; }",
+            code: "function undefined(undefined) { !function undefined(undefined) { try {} catch(undefined) {} }; }",
             errors: [
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" },
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" },
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" },
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" },
-                { message: "Shadowing of global property 'undefined'.", type: "Identifier" },
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" }
             ]
         },
@@ -110,6 +116,12 @@ ruleTester.run("no-shadow-restricted-names", rule, {
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" },
                 { message: "Shadowing of global property 'undefined'.", type: "Identifier" }
             ]
+        },
+        {
+            code: "var undefined; undefined = 5;",
+            errors: [
+                { message: "Shadowing of global property 'undefined'.", type: "Identifier" }
+            ]
         }
     ]
 });