From 7fb45cf13e9908d489bd6d5fba3b7243c01508b9 Mon Sep 17 00:00:00 2001 From: Anix Date: Tue, 16 Jun 2020 06:15:26 +0530 Subject: [PATCH] Fix: clone config before validating (fixes #12592) (#13034) * Fix: clone config data before validation (fixes #12592) * Chore: added test and cloning before validation * Chore: test fix * Chore: rules tests fixes * Update: cloning config using json parse and stringify * Update: changed test description * Chore: removed unnecessary tests * Update: changed clone logic * Update: using lodash.cloneDeepWith for cloning * Update: handling circular ref cases * Update: using JSON method for cloning logic * Update: moved clone logic to normalieObjectConfigData * Chore: added comments for cloning reason --- lib/cli-engine/config-array-factory.js | 29 +++++++++- .../cloned-config/circularRefEslintConfig.js | 15 +++++ .../config-file/cloned-config/eslintConfig.js | 4 ++ .../cloned-config/eslintConfigFail.js | 13 +++++ .../config-file/cloned-config/index.js | 1 + .../config-file/cloned-config/inlineText.js | 3 + tests/lib/cli.js | 55 +++++++++++++++++++ 7 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 tests/fixtures/config-file/cloned-config/circularRefEslintConfig.js create mode 100644 tests/fixtures/config-file/cloned-config/eslintConfig.js create mode 100644 tests/fixtures/config-file/cloned-config/eslintConfigFail.js create mode 100644 tests/fixtures/config-file/cloned-config/index.js create mode 100644 tests/fixtures/config-file/cloned-config/inlineText.js diff --git a/lib/cli-engine/config-array-factory.js b/lib/cli-engine/config-array-factory.js index 7c0fba65c67..6d0992151ad 100644 --- a/lib/cli-engine/config-array-factory.js +++ b/lib/cli-engine/config-array-factory.js @@ -697,6 +697,33 @@ class ConfigArrayFactory { ctx.matchBasePath ); + /** + * Cloning the rule's config as we are setting `useDefaults` to true` + * which mutates the config with its default value if present. And when we + * refer to a same variable for config for different rules, that referred variable will + * be mutated and it will be used for both. + * + * Example: + * + * const commonRuleConfig = ['error', {}]; + * + * Now if we use this variable as a config for rules like this + * + * { + * rules: { + * "a" : commonRuleConfig, + * "b" : commonRuleConfig, + * } + * } + * + * And if these rules have default values in their schema, their + * config will be mutated with default values, the mutated `commonRuleConfig` will be used for `b` as well and it probably + * throw schema voilation errors. + * + * Refer https://github.com/eslint/eslint/issues/12592 + */ + const clonedRulesConfig = rules && JSON.parse(JSON.stringify((rules))); + // Flatten `extends`. for (const extendName of extendList.filter(Boolean)) { yield* this._loadExtends(extendName, ctx); @@ -731,7 +758,7 @@ class ConfigArrayFactory { processor, reportUnusedDisableDirectives, root, - rules, + rules: clonedRulesConfig, settings }; diff --git a/tests/fixtures/config-file/cloned-config/circularRefEslintConfig.js b/tests/fixtures/config-file/cloned-config/circularRefEslintConfig.js new file mode 100644 index 00000000000..cc9fcdbcc7c --- /dev/null +++ b/tests/fixtures/config-file/cloned-config/circularRefEslintConfig.js @@ -0,0 +1,15 @@ +let errorConfig = ["error", {}]; + +class CircularRef { + constructor() { + this.self = this; + } +} + +module.exports = { + settings: { + sharedData: new CircularRef() + }, + + rules: { camelcase: errorConfig, "default-case": errorConfig } +}; diff --git a/tests/fixtures/config-file/cloned-config/eslintConfig.js b/tests/fixtures/config-file/cloned-config/eslintConfig.js new file mode 100644 index 00000000000..8602e60b0fd --- /dev/null +++ b/tests/fixtures/config-file/cloned-config/eslintConfig.js @@ -0,0 +1,4 @@ +let errorConfig = ["error", {}]; +module.exports = { + rules: { camelcase: errorConfig, "default-case": errorConfig } +}; diff --git a/tests/fixtures/config-file/cloned-config/eslintConfigFail.js b/tests/fixtures/config-file/cloned-config/eslintConfigFail.js new file mode 100644 index 00000000000..b8702c6ffb8 --- /dev/null +++ b/tests/fixtures/config-file/cloned-config/eslintConfigFail.js @@ -0,0 +1,13 @@ +let errorConfig = ["error", {}]; +module.exports = { + rules: { + camelcase: errorConfig, + "default-case": errorConfig , + "camelcase" : [ + 'error', + { + "ignoreDestructuring": new Date().getUTCFullYear // returns a function + } + ] + } +}; diff --git a/tests/fixtures/config-file/cloned-config/index.js b/tests/fixtures/config-file/cloned-config/index.js new file mode 100644 index 00000000000..b792f78cad3 --- /dev/null +++ b/tests/fixtures/config-file/cloned-config/index.js @@ -0,0 +1 @@ +var someValue = "bar"; diff --git a/tests/fixtures/config-file/cloned-config/inlineText.js b/tests/fixtures/config-file/cloned-config/inlineText.js new file mode 100644 index 00000000000..e000713abaf --- /dev/null +++ b/tests/fixtures/config-file/cloned-config/inlineText.js @@ -0,0 +1,3 @@ +/*eslint default-case: ["error", {}]*/ +/*eslint camelcase: ["error", {}]*/ +var someValue = "bar"; diff --git a/tests/lib/cli.js b/tests/lib/cli.js index a1d9a23e491..afb225f1f7c 100644 --- a/tests/lib/cli.js +++ b/tests/lib/cli.js @@ -187,6 +187,7 @@ describe("cli", () => { }); }); + describe("when given a config with environment set to Node.js", () => { it("should execute without any errors", async () => { const configPath = getFixturePath("configurations", "env-node.json"); @@ -1160,4 +1161,58 @@ describe("cli", () => { }); }); + describe("testing the cloned config", () => { + describe("config file and input file", () => { + it("should not modify original configuration object", async () => { + const configPath = getFixturePath("config-file", "cloned-config", "eslintConfig.js"); + const filePath = getFixturePath("config-file", "cloned-config", "index.js"); + const args = `--config ${configPath} ${filePath}`; + + const exit = await cli.execute(args); + + assert.strictEqual(exit, 0); + }); + }); + + describe("config file and input file", () => { + it("should exit with 1 as camelcase has wrong property type", async () => { + const configPath = getFixturePath("config-file", "cloned-config", "eslintConfigFail.js"); + const filePath = getFixturePath("config-file", "cloned-config", "index.js"); + const args = `--config ${configPath} ${filePath}`; + + try { + await cli.execute(args); + } catch (error) { + assert.strictEqual(/Configuration for rule "camelcase" is invalid:/u.test(error), true); + } + + }); + }); + + describe("inline config and input file", () => { + it("should not modify original configuration object", async () => { + const filePath = getFixturePath("config-file", "cloned-config", "inlineText.js"); + const args = `${filePath}`; + + const exit = await cli.execute(args); + + assert.strictEqual(exit, 0); + }); + }); + + }); + + describe("handling circular reference while cloning", () => { + it("should handle circular ref", async () => { + const configPath = getFixturePath("config-file", "cloned-config", "circularRefEslintConfig.js"); + const filePath = getFixturePath("config-file", "cloned-config", "index.js"); + const args = `--config ${configPath} ${filePath}`; + + try { + await cli.execute(args); + } catch (error) { + assert.instanceOf(error, Error); + } + }); + }); });