Support bracket notation (computed member expressions)

[Paippi]

Hey,

Noticed that using bracket notation you may bypass these lint, e.g., document.write(foo) gets flagged but document["write"](foo) doesn't.

Could we get support for this?

BR, Paippi

[mozfreddyb]

Hi,

thank you for this suggestion. We have indeed thought about this case at various stages in the past. However, this linter rules was built for the engineering culture and practices of Mozilla's projects, specifically Firefox. At Mozilla, we require for all new code to be subject to code review and go through mandatory tests in CI.

Based on this context and our requirements, we do not see a lot of value in detecting (not to mention protecting against) various forms of obfuscation or even malice.

In fact, there are various methods to easily bypass this check, if it isn't paired with careful code review. We tried to document this in our security policy. We are open to suggestions on how to improve things - especially if they come as a pull request :-) but it's also OK if you cannot or will not contribute to a solution, in this case we will close this issue.

[Paippi]

Hey,

sorry for the late response. I have few suggestions.

1. How about if we only check against string literals? I know this isn't fool proof but perhaps a minor improvement.
2. On the contrary to what I just suggested, what if we make a special case of the document variable. So that method calls with unsanitized use of variables in bracket notation would be flagged perhaps as a warning only?

For example:

const my_func = (attr, val) => {
    document[attr](val);
}

Would be flagged

I shall create different pull requests from these! :)

[mozfreddyb]

Can you supply test cases for conforming/non-conforming code for your point (1)? I'm not sure I fully understand.

Regarding 2): It's a great idea. I think it could be cool to disallow computed properties off of the known globals (document, windows, ...) generally. But I don't think it would fit into this linter plugin here. Using the source code of other rules like https://eslint.org/docs/rules/no-useless-computed-key could help you implement those. Is this something you'd be interested as a learning experience? :-)

[Paippi]

Alright so, for the first one

// Would be flagged by this rule
document["write"](evil);
div["innerHTML"] = evil;
// Wouldn't be flagged by this rule
document[attr](evil);
div[attr] = evil; // leaving this out since divs type should be otherwise somehow detected or there will be a lot of false positives

Regarding 2) I'll have a look at that project project. I think I might almost have POC implemented. Though I'm not sure how to deal with every node type like any TS type or templates.

[mozfreddyb]

Thank you for taking the time to provide a test case and a pull request. The code you suggest should be flagged by this rule is quite...obscure imho and I'm doubtful we should take it into consideration for the linter plugin.

After all, what I said in my first comment still holds true: We expect code to be undergone peer review before being scanned and I can't imagine anyone committing code like document["write"] instead of document.write. I don't think your idea fits our threat model really well.

[Paippi]

Alright I understand. I just thought that even though it would go through peer review it might not do any harm to have this as a addition, of course it might mean more test cases to maintain.

Considering if you, e.g., ever decide to fork another repository and would like to enforce linting rules it might be helpful to just quickly fix these issues, even though it might be that this kind of code never happens, but can't ever be sure.

That said I'm all good if you don't see this change fitting for your repository and this thread can be closed on my behalf.

[mozfreddyb]

A separate linter plugin that identifies code like document["write"] and fixes it with document.write would be a super useful addition to the eslint ecosystem.

Thank you very much for coming up with the idea, example and pull request!