Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Ensure that extra data is not accidentally stored in the cache file #17760

Merged
merged 1 commit into from Nov 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 18 additions & 6 deletions lib/cli-engine/lint-result-cache.js
Expand Up @@ -128,16 +128,28 @@ class LintResultCache {
return null;
}

const cachedResults = fileDescriptor.meta.results;

// Just in case, not sure if this can ever happen.
if (!cachedResults) {
return cachedResults;
}

/*
* Shallow clone the object to ensure that any properties added or modified afterwards
* will not be accidentally stored in the cache file when `reconcile()` is called.
* https://github.com/eslint/eslint/issues/13507
* All intentional changes to the cache file must be done through `setCachedLintResults()`.
*/
const results = { ...cachedResults };

// If source is present but null, need to reread the file from the filesystem.
if (
fileDescriptor.meta.results &&
fileDescriptor.meta.results.source === null
) {
if (results.source === null) {
debug(`Rereading cached result source from filesystem: ${filePath}`);
fileDescriptor.meta.results.source = fs.readFileSync(filePath, "utf-8");
results.source = fs.readFileSync(filePath, "utf-8");
}

return fileDescriptor.meta.results;
return results;
}

/**
Expand Down
99 changes: 99 additions & 0 deletions tests/lib/eslint/eslint.js
Expand Up @@ -2993,6 +2993,105 @@ describe("ESLint", () => {
assert.deepStrictEqual(result, cachedResult, "result should be the same with or without cache");
});

// https://github.com/eslint/eslint/issues/13507
it("should not store `usedDeprecatedRules` in the cache file", async () => {
cacheFilePath = getFixturePath(".eslintcache");
doDelete(cacheFilePath);
assert(!shell.test("-f", cacheFilePath), "the cache file already exists and wasn't successfully deleted");

const deprecatedRuleId = "space-in-parens";

eslint = new ESLint({
cwd: path.join(fixtureDir, ".."),
useEslintrc: false,

// specifying cache true the cache will be created
cache: true,
cacheLocation: cacheFilePath,
overrideConfig: {
rules: {
[deprecatedRuleId]: 2
}
}
});

const filePath = fs.realpathSync(getFixturePath("cache/src", "test-file.js"));

/*
* Run linting on the same file 3 times to cover multiple cases:
* Run 1: Lint result wasn't already cached.
* Run 2: Lint result was already cached. The cached lint result is used but the cache is reconciled before the run ends.
* Run 3: Lint result was already cached. The cached lint result was being used throughout the previous run, so possible
* mutations in the previous run that occured after the cache was reconciled may have side effects for this run.
*/
for (let i = 0; i < 3; i++) {
const [result] = await eslint.lintFiles([filePath]);

assert(
result.usedDeprecatedRules && result.usedDeprecatedRules.some(rule => rule.ruleId === deprecatedRuleId),
"the deprecated rule should have been in result.usedDeprecatedRules"
);

assert(shell.test("-f", cacheFilePath), "the cache for eslint should have been created");

const fileCache = fCache.create(cacheFilePath);
const descriptor = fileCache.getFileDescriptor(filePath);

assert(typeof descriptor === "object", "an entry for the file should have been in the cache file");
assert(typeof descriptor.meta.results === "object", "lint result for the file should have been in its cache entry in the cache file");
assert(typeof descriptor.meta.results.usedDeprecatedRules === "undefined", "lint result in the cache file contains `usedDeprecatedRules`");
}

});

// https://github.com/eslint/eslint/issues/13507
it("should store `source` as `null` in the cache file if the lint result has `source` property", async () => {
cacheFilePath = getFixturePath(".eslintcache");
doDelete(cacheFilePath);
assert(!shell.test("-f", cacheFilePath), "the cache file already exists and wasn't successfully deleted");

eslint = new ESLint({
cwd: path.join(fixtureDir, ".."),
useEslintrc: false,

// specifying cache true the cache will be created
cache: true,
cacheLocation: cacheFilePath,
overrideConfig: {
rules: {
"no-unused-vars": 2
}
}
});

const filePath = fs.realpathSync(getFixturePath("cache/src", "fail-file.js"));

/*
* Run linting on the same file 3 times to cover multiple cases:
* Run 1: Lint result wasn't already cached.
* Run 2: Lint result was already cached. The cached lint result is used but the cache is reconciled before the run ends.
* Run 3: Lint result was already cached. The cached lint result was being used throughout the previous run, so possible
* mutations in the previous run that occured after the cache was reconciled may have side effects for this run.
*/
for (let i = 0; i < 3; i++) {
const [result] = await eslint.lintFiles([filePath]);

assert(typeof result.source === "string", "the result should have contained the `source` property");

assert(shell.test("-f", cacheFilePath), "the cache for eslint should have been created");

const fileCache = fCache.create(cacheFilePath);
const descriptor = fileCache.getFileDescriptor(filePath);

assert(typeof descriptor === "object", "an entry for the file should have been in the cache file");
assert(typeof descriptor.meta.results === "object", "lint result for the file should have been in its cache entry in the cache file");

// if the lint result contains `source`, it should be stored as `null` in the cache file
assert.strictEqual(descriptor.meta.results.source, null, "lint result in the cache file contains non-null `source`");
}

});

describe("cacheStrategy", () => {
it("should detect changes using a file's modification time when set to 'metadata'", async () => {
cacheFilePath = getFixturePath(".eslintcache");
Expand Down
99 changes: 99 additions & 0 deletions tests/lib/eslint/flat-eslint.js
Expand Up @@ -2876,6 +2876,105 @@ describe("FlatESLint", () => {
assert.deepStrictEqual(result, cachedResult, "result should be the same with or without cache");
});

// https://github.com/eslint/eslint/issues/13507
it("should not store `usedDeprecatedRules` in the cache file", async () => {
cacheFilePath = getFixturePath(".eslintcache");
doDelete(cacheFilePath);
assert(!shell.test("-f", cacheFilePath), "the cache file already exists and wasn't successfully deleted");

const deprecatedRuleId = "space-in-parens";

eslint = new FlatESLint({
cwd: path.join(fixtureDir, ".."),
overrideConfigFile: true,

// specifying cache true the cache will be created
cache: true,
cacheLocation: cacheFilePath,
overrideConfig: {
rules: {
[deprecatedRuleId]: 2
}
}
});

const filePath = fs.realpathSync(getFixturePath("cache/src", "test-file.js"));

/*
* Run linting on the same file 3 times to cover multiple cases:
* Run 1: Lint result wasn't already cached.
* Run 2: Lint result was already cached. The cached lint result is used but the cache is reconciled before the run ends.
* Run 3: Lint result was already cached. The cached lint result was being used throughout the previous run, so possible
* mutations in the previous run that occured after the cache was reconciled may have side effects for this run.
*/
for (let i = 0; i < 3; i++) {
const [result] = await eslint.lintFiles([filePath]);

assert(
result.usedDeprecatedRules && result.usedDeprecatedRules.some(rule => rule.ruleId === deprecatedRuleId),
"the deprecated rule should have been in result.usedDeprecatedRules"
);

assert(shell.test("-f", cacheFilePath), "the cache for eslint should have been created");

const fileCache = fCache.create(cacheFilePath);
const descriptor = fileCache.getFileDescriptor(filePath);

assert(typeof descriptor === "object", "an entry for the file should have been in the cache file");
assert(typeof descriptor.meta.results === "object", "lint result for the file should have been in its cache entry in the cache file");
assert(typeof descriptor.meta.results.usedDeprecatedRules === "undefined", "lint result in the cache file contains `usedDeprecatedRules`");
}

});

// https://github.com/eslint/eslint/issues/13507
it("should store `source` as `null` in the cache file if the lint result has `source` property", async () => {
cacheFilePath = getFixturePath(".eslintcache");
doDelete(cacheFilePath);
assert(!shell.test("-f", cacheFilePath), "the cache file already exists and wasn't successfully deleted");

eslint = new FlatESLint({
cwd: path.join(fixtureDir, ".."),
overrideConfigFile: true,

// specifying cache true the cache will be created
cache: true,
cacheLocation: cacheFilePath,
overrideConfig: {
rules: {
"no-unused-vars": 2
}
}
});

const filePath = fs.realpathSync(getFixturePath("cache/src", "fail-file.js"));

/*
* Run linting on the same file 3 times to cover multiple cases:
* Run 1: Lint result wasn't already cached.
* Run 2: Lint result was already cached. The cached lint result is used but the cache is reconciled before the run ends.
* Run 3: Lint result was already cached. The cached lint result was being used throughout the previous run, so possible
* mutations in the previous run that occured after the cache was reconciled may have side effects for this run.
*/
for (let i = 0; i < 3; i++) {
const [result] = await eslint.lintFiles([filePath]);

assert(typeof result.source === "string", "the result should have contained the `source` property");

assert(shell.test("-f", cacheFilePath), "the cache for eslint should have been created");

const fileCache = fCache.create(cacheFilePath);
const descriptor = fileCache.getFileDescriptor(filePath);

assert(typeof descriptor === "object", "an entry for the file should have been in the cache file");
assert(typeof descriptor.meta.results === "object", "lint result for the file should have been in its cache entry in the cache file");

// if the lint result contains `source`, it should be stored as `null` in the cache file
assert.strictEqual(descriptor.meta.results.source, null, "lint result in the cache file contains non-null `source`");
}

});

describe("cacheStrategy", () => {
it("should detect changes using a file's modification time when set to 'metadata'", async () => {
cacheFilePath = getFixturePath(".eslintcache");
Expand Down