Issues when multiple plugins use multiple parsers

[nickdeis]

Hey ESLint Team,
I maintain a plugin eslint-plugin-no-secrets that uses the parser feature to detect if secure token are in JSON files and JS files so they don't accidentally get committed. I user of mine recently ran into this issue where if multiple plugins use multiple parsers, weird things happen. Currently this behavior appears when used with eslint-plugin-json and eslint-plugin-json-format. The issues are different for each plugin, but I think they have same root cause. If you want me to file multiple issues, let me know.

Tell us about your environment

• ESLint Version: 6.8.0 and 7.3.0
• Node Version: 14.3.0
• npm Version: 6.14.4

What parser (default, Babel-ESLint, etc.) are you using?

None

Please show your full configuration:

Configuration

Multiple, since this is a plugin

{
    "plugins": [
      "no-secrets",
      "json",
    ],
    "rules": {
      "no-secrets/no-secrets": "error",
      "json/undefined":"error"
    }
  }

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

has-secrets.json

{
    "SECRET":"AAAAAAAAAAAAAAAAAAAAAMLheAAAAAAA0%2BuSeid%2BULvsea4JtiGRiSDSJSI%3DEUifiRBkKG5E2XzMDjRfl76ZC9Ub0wnz4XsNiRVBChTYbJcE3F"
}

I'm using CLIEngine to perform e2e testing, but I think the equvialent command would be

eslint has-secrets.json

What did you expect to happen?
This should of been detected by eslint-plugin-no-secrets as a high entropy string

What actually happened? Please include the actual, raw output from ESLint.
If I include the plugin eslint-plugin-json, it doesn't detect any high entropy strings.

Are you willing to submit a pull request to fix this bug?
If I can get some help debugging this, of course!

[kaicataldo]

Can you share the full configuration? Or at least a minimal valid config? It's hard to tell with the snippets. In particular, it would be helpful to see what overrides you are using.

[nickdeis]

Hey @kaicataldo ,
Thank you for your reply!
It seems like my markdown got rendered incorrectly, I have corrected it.
Here's the config:

{
    "plugins": [
      "no-secrets",
      "json",
    ],
    "rules": {
      "no-secrets/no-secrets": "error",
      "json/undefined":"error"
    }
  }

[kaicataldo]

Any chance you can share the relevant part of the code where you use CLIEngine? My guess is that it's not actually finding the JSON file.

[nickdeis]

Hey @kaicataldo,
If you'd like, I can create a branch on my repo that includes the failing e2e test.
Here's the test, if I remove the json plugin and rules this test doesn't fail:

const CLIEngine = require("eslint7").CLIEngine;
const assert = require('assert');


const TESTS = {
    './staging.config.js':[
        {
            name:'Should not detect non-secrets',
            file:'./staging/has-no-secret.json',
            errorCount: 0
        },
        {
            name:'Should detect secrets in json files',
            file:'./staging/has-secret.json',
            errorCount: 1
        }
    ]
}


describe('JSON compat testing',() => {
    const configs = Object.entries(TESTS);
    for(const [config,tests] of configs){
        const cli = new CLIEngine(require(config));
        const files = tests.map(test => test.file);
        const {results} = cli.executeOnFiles(files);
        describe(config,() => {
            for(let i=0;i < tests.length;i++){
                const test = tests[i];
                const report = results[i];
                it(test.name,() => {
                    assert.strictEqual(test.errorCount,report.errorCount);
                });
            }
        });
    }
});

[kaicataldo]

So, a few questions/things to check:

1. What is eslint7?
2. Are you explicitly passing file names to cli.executeOnFiles()? Or are they directories?
3. Are you using the extensions configuration option?

[kaicataldo]

If none of that works, would you be able to make a repo with the minimal reproduction case for this? Easier for us to debug it this way :)

[nickdeis]

1. Eslint7 is an alias for npm:eslint@^7.3.0 using npm install alias feature
2. The filenames themselves
3. Just moved my tests to use the new ESLint class, same problem even when using extensions:['.js','.json']

For your second comment: Do you want me to create one branch where everything works, and another where it doesn't?
Or should I have all the unit tests in one branch?

[kaicataldo]

Just having one that doesn't work would be fine!

[nickdeis]

Hey @kaicataldo,
I have made a branch with the unit tests here.
Here are the unit test results
Thank you so much!

[kaicataldo]

Alrighty, so it looks like the issue is plugin ordering. When I switch the two elements in this array to

"plugins": [
  "json",
  "self",
],

the tests pass. My guess is that the JSON plugin has some overrides that are overriding the configuration set in self.

[kaicataldo]

To clarify a bit more, plugins are processed in the order in which they occur, so later plugins can override previous plugins in the plugins array,

[nickdeis]

Hey @kaicataldo,
Thank you for your input. So is there any way to write a parser that works in any order? I don't want to break other plugins, and I don't want to break my plugin 😂 . Is that not possible?
Best,
Nick

[kaicataldo]

Looking at #11035, I don't think we allow for a processor chain or for two separate processors to be run on the same file, but this isn't a part of the codebase I'm deeply knowledgeable about. This would require re-reading the file (if autofixes are performed...Otherwise, we could just keep it cached in memory), running the processor to extract the text to be linted, and running all the rules for each processor specified. ESLint was originally designed to just lint JavaScript, and this feels like a limitation (or just a feature that wasn't necessary at the time ESLint was created) of the time in which it was created.

Seems like there are two possible ways forward:

1. Create an RFC for this feature. This would be particularly useful for situations where a file might contain multiple languages (a JavaScript file containing GraphQL or CSS, Vue single file components, etc.). This seems like an interesting thing to explore in conjunction with supporting generic ASTs.

2. Is there a reason you need to use both? Processors are intended to be used to locate JavaScript text from a file that contains other code that wouldn't be able to be parsed by ESLint. Ideally, you would only need to use one processor and enable multiple rule plugins.

[nickdeis]

Is there a reason you need to use both? Processors are intended to be used to locate JavaScript text from a file that contains other code that wouldn't be able to be parsed by ESLint. Ideally, you would only need to use one processor and enable multiple rule plugins.

This would be ideal, however, some of these processors don't return js, and sometimes they return nothing.
It seems like there is another API for plugins that doesn't use processors at all example, so maybe I can look into that.

[nickdeis]

Hey @kaicataldo ,
Is there any documentation on the Linter#_verifyWithoutProcessors? It seems to be used by a lot of multi-language plugins.

[kaicataldo]

Linter#_verifyWithoutProcessors isn't part of the public API, so it's best not to rely on it as it can could change at any time. Is there a reason none of the public API methods work for your use case?

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.