Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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
  • Loading branch information
anikethsaha committed Jun 16, 2020
1 parent aed46f6 commit 7fb45cf
Show file tree
Hide file tree
Showing 7 changed files with 119 additions and 1 deletion.
29 changes: 28 additions & 1 deletion lib/cli-engine/config-array-factory.js
Expand Up @@ -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)));

This comment has been minimized.

Copy link
@marcmenn

marcmenn Jun 19, 2020

This breaks configurations that use Infinity in their options


// Flatten `extends`.
for (const extendName of extendList.filter(Boolean)) {
yield* this._loadExtends(extendName, ctx);
Expand Down Expand Up @@ -731,7 +758,7 @@ class ConfigArrayFactory {
processor,
reportUnusedDisableDirectives,
root,
rules,
rules: clonedRulesConfig,
settings
};

Expand Down
@@ -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 }
};
4 changes: 4 additions & 0 deletions tests/fixtures/config-file/cloned-config/eslintConfig.js
@@ -0,0 +1,4 @@
let errorConfig = ["error", {}];
module.exports = {
rules: { camelcase: errorConfig, "default-case": errorConfig }
};
13 changes: 13 additions & 0 deletions 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
}
]
}
};
1 change: 1 addition & 0 deletions tests/fixtures/config-file/cloned-config/index.js
@@ -0,0 +1 @@
var someValue = "bar";
3 changes: 3 additions & 0 deletions tests/fixtures/config-file/cloned-config/inlineText.js
@@ -0,0 +1,3 @@
/*eslint default-case: ["error", {}]*/
/*eslint camelcase: ["error", {}]*/
var someValue = "bar";
55 changes: 55 additions & 0 deletions tests/lib/cli.js
Expand Up @@ -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");
Expand Down Expand Up @@ -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);
}
});
});
});

0 comments on commit 7fb45cf

Please sign in to comment.