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

New: Rule class-methods-use-this option exceptMethods accepts regex #12305

Closed
wants to merge 13 commits into from
2 changes: 1 addition & 1 deletion docs/rules/class-methods-use-this.md
Expand Up @@ -94,7 +94,7 @@ class A {
"class-methods-use-this": [<enabled>, { "exceptMethods": [<...exceptions>] }]
```

The `exceptMethods` option allows you to pass an array of method names for which you would like to ignore warnings. For example, you might have a spec from an external library that requires you to overwrite a method as a regular function (and not as a static method) and does not use `this` inside the function body. In this case, you can add that method to ignore in the warnings.
The `exceptMethods` option allows you to pass an array of method names (accept regex) for which you would like to ignore warnings. For example, you might have a spec from an external library that requires you to overwrite a method as a regular function (and not as a static method) and does not use `this` inside the function body. In this case, you can add that method to ignore in the warnings.

Examples of **incorrect** code for this rule when used without exceptMethods:

Expand Down
4 changes: 2 additions & 2 deletions lib/rules/class-methods-use-this.js
Expand Up @@ -45,7 +45,7 @@ module.exports = {
},
create(context) {
const config = Object.assign({}, context.options[0]);
const exceptMethods = new Set(config.exceptMethods || []);
const exceptMethods = (config.exceptMethods || []).map(e => new RegExp(e, "u"));
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accepting a regex opens the rule up to a bunch of CVEs. is there a reason this can’t be simple globs?

Copy link
Author

@derjones derjones Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm okay, in my case globs would be enough. But other rules also use RegExp for exceptions: camelcase, lines-around-comment e.g.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a security issue if the regexes come from a config. If someone has the ability to edit a config file and add malicious regexes, they probably also have the ability to create a JS file and do worse things anyway.

If inline config comments are enabled, someone could maybe create a piece of code that takes a very long time to lint, but I think that's an accepted risk at this point.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s true, but that won’t stop tons of false positive CVEs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With an exception or two, it seems to have stopped them so far. False-positive CVEs are sometimes a problem, but it seems silly to not use a feature for fear of false positive security reports. (Maybe someone can convince V8 to use linear-time regex matching for regexes that don't have backreferences, and then we stop tons of real CVEs.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a general question if regex is allowed in a rule config or not. Since some rules use a regex in the config already you would have to remove either all or allow in general


const stack = [];

Expand Down Expand Up @@ -77,7 +77,7 @@ module.exports = {
*/
function isIncludedInstanceMethod(node) {
return isInstanceMethod(node) &&
(node.computed || !exceptMethods.has(node.key.name));
(node.computed || !exceptMethods.find(e => e.test(node.key.name)));
}

/**
Expand Down