Repository: import-js/eslint-plugin-import
Pull request #3062 (2024-09-15 16:21:03)

[Fix] `exportMap`: export map cache is tainted by unreliable parse results
This change addresses an issue observed with the v9 upgrade, where the `ExportMap` Cache is being tainted with a bad export map, if the parse doesn't yield a `visitorKeys` (which can happen if an incompatible parser is used (e.g. og babel eslint)).  If one run of the no-cycle rule uses an incompatible parser and taints the cache and a subsequent test is run with a compatible parser, then the bad export map will be found in the cache and used instead of attempting to proceed with the parse.   

I don't think this is likely to occur in the wild, but in the event that it does, this should help. One thing to note, though: if someone is using an incompatible parser, then this change will have a performance impact, since it will attempt to create an export map every time, until it yields one that has `visitorKeys` attached.

I also updated the `getExports` test to use a valid `parserPath`, rather than a mocked one, so the tests are acting on a valid `parserPath`.  Otherwise the export map won't be cached following this change and the test for it caching would fail.

Additional context: https://github.com/import-js/eslint-plugin-import/pull/2996#issuecomment-2351204273


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 33b4eb4f..da66f7e8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
 - `ExportMap` / flat config: include `languageOptions` in context ([#3052], thanks [@michaelfaith])
 - [`no-named-as-default`]: Allow using an identifier if the export is both a named and a default export ([#3032], thanks [@akwodkiewicz])
 - [`export`]: False positive for exported overloaded functions in TS ([#3065], thanks [@liuxingbaoyu])
+- `exportMap`: export map cache is tainted by unreliable parse results ([#3062], thanks [@michaelfaith])
 
 ### Changed
 - [Docs] [`no-relative-packages`]: fix typo ([#3066], thanks [@joshuaobrien])
@@ -1146,6 +1147,7 @@ for info on changes for earlier releases.
 [#3068]: https://github.com/import-js/eslint-plugin-import/pull/3068
 [#3066]: https://github.com/import-js/eslint-plugin-import/pull/3066
 [#3065]: https://github.com/import-js/eslint-plugin-import/pull/3065
+[#3062]: https://github.com/import-js/eslint-plugin-import/pull/3062
 [#3052]: https://github.com/import-js/eslint-plugin-import/pull/3052
 [#3043]: https://github.com/import-js/eslint-plugin-import/pull/3043
 [#3036]: https://github.com/import-js/eslint-plugin-import/pull/3036
diff --git a/src/exportMap/builder.js b/src/exportMap/builder.js
index 5348dba3..f7b9006e 100644
--- a/src/exportMap/builder.js
+++ b/src/exportMap/builder.js
@@ -92,7 +92,11 @@ export default class ExportMapBuilder {
 
     exportMap.mtime = stats.mtime;
 
-    exportCache.set(cacheKey, exportMap);
+    // If the visitor keys were not populated, then we shouldn't save anything to the cache,
+    // since the parse results may not be reliable.
+    if (exportMap.visitorKeys) {
+      exportCache.set(cacheKey, exportMap);
+    }
     return exportMap;
   }
 


Test patch:
diff --git a/tests/src/core/getExports.js b/tests/src/core/getExports.js
index 76003410..f11a2613 100644
--- a/tests/src/core/getExports.js
+++ b/tests/src/core/getExports.js
@@ -1,15 +1,18 @@
 import { expect } from  'chai';
+import fs from 'fs';
 import semver from 'semver';
 import sinon from 'sinon';
 import eslintPkg from 'eslint/package.json';
+import { test as testUnambiguous } from 'eslint-module-utils/unambiguous';
 import typescriptPkg from 'typescript/package.json';
 import * as tsConfigLoader from 'tsconfig-paths/lib/tsconfig-loader';
-import ExportMapBuilder from '../../../src/exportMap/builder';
-
-import * as fs from 'fs';
 
+import ExportMapBuilder from '../../../src/exportMap/builder';
 import { getFilename } from '../utils';
-import { test as testUnambiguous } from 'eslint-module-utils/unambiguous';
+
+const babelPath = require.resolve('babel-eslint');
+const hypotheticalLocation = babelPath.replace('index.js', 'visitor-keys.js');
+const isVisitorKeysSupported = fs.existsSync(hypotheticalLocation);
 
 describe('ExportMap', function () {
   const fakeContext = Object.assign(
@@ -21,7 +24,7 @@ describe('ExportMap', function () {
     },
     {
       settings: {},
-      parserPath: 'babel-eslint',
+      parserPath: require.resolve('babel-eslint'),
     },
   );
 
@@ -36,11 +39,20 @@ describe('ExportMap', function () {
 
   });
 
-  it('returns a cached copy on subsequent requests', function () {
+  (isVisitorKeysSupported ? it : it.skip)('returns a cached copy on subsequent requests', function () {
     expect(ExportMapBuilder.get('./named-exports', fakeContext))
       .to.exist.and.equal(ExportMapBuilder.get('./named-exports', fakeContext));
   });
 
+  it('does not return a cached copy if the parse does not yield a visitor keys', function () {
+    const mockContext = {
+      ...fakeContext,
+      parserPath: 'not-real',
+    };
+    expect(ExportMapBuilder.get('./named-exports', mockContext))
+      .to.exist.and.not.equal(ExportMapBuilder.get('./named-exports', mockContext));
+  });
+
   it('does not return a cached copy after modification', (done) => {
     const firstAccess = ExportMapBuilder.get('./mutator', fakeContext);
     expect(firstAccess).to.exist;