Trace variables back to find allow-able expression (fix #167)

mozilla / eslint-plugin-no-unsanitized

Custom ESLint rule to disallows unsafe innerHTML, outerHTML, insertAdjacentHTML and alike

Mozilla Public License 2.0

228 stars 37 forks source link

Trace variables back to find allow-able expression (fix #167) #169

Closed mozfreddyb closed 3 years ago

mozfreddyb commented 3 years ago

I realize this is quite a change, but it seems relatively easy and very useful :)

Issue #167 has a bit of an explanation and the tests carry a lot of the wright, but I'm still a bit concerned that this might break stuff. For testing, I'll explore running this on popular repos that are alraedy using this rule.

[x] Run against mozilla-central (i.e., Firefox frontend)
[x] find a Mozilla web extension to test against this new logic
[ ] find an external project that uses this plugin and see what happens

rpl commented 3 years ago

@willdurand could we track back some of the addons that did make the addons-linter to hit the memory limits while being validated on submission to AMO?

This is one of the eslint plugins we enable internally in the addons-linter and this PR is going to introduce some more checks to track back variables to their initialization and/or assignment inside the expression scope, and so it would be good to double-check if the additional checks will increase the amount of memory used while validating addons with bigger js files to parse and traverse.

rpl commented 3 years ago

@mozfreddyb There is another detail that I've been thinking of:

what would happen if RuleHelper does get to an expression that we don't support (the ones for which we add the linting errors "Please report a minimal code snippet to the developers at ...") while we recurse to check if a variable is unsafe or not?

I gave it a quick try by changing temporarily one of the test cases and I've verified that when that happens we do add two separate linting errors:

one for the unsupported expression
one for the property assignment related to the unsupported expression

(I'm including an example of the linting errors reported below at the end of this comment).

Personally I think that this behavior is fine as is:

if we can't be sure that a variable assignment is safe then it is right to still consider that property assignment to be unsafe
that same property assignment would have been marked as unsafe even without the changes in this PR
and the resulting linting errors are detailed enough for the developer to have a clear enough idea of what is going on and which code is triggering the new linting error

but I still wanted to report it back to you explicitly.

I'm not sure if it is worth explicitly cover this scenario ("unsupported expressions triggered while tracking back the variables initialization and assignments"):

on one side it may be a good idea to include one as a "smoke test" (to be sure that we don't end up missing the expected linting error on the top most expression being checked)
but on the other side using a syntax that we don't support right now is likely going to make that test case annoying in the future (because at some point we may support that expression and we would have to find another one not yet supported to still trigger the same scenario).

It may be possible to achieve it using some mocking instead of a syntax really unsupported, but I haven't really looked how tricky that would be in practice.

code snippet: var copies = '<b>safe</b>'; var copies = y()(); y.innerHTML = copies;

Linting errors reported:

 [
  {
    ruleId: 'property',
    severity: 1,
    message: 'Error in no-unsanitized: Unexpected callable. Please report a minimal code snippet to the developers at https://github.com/mozilla/eslint-plugin-no-unsanitized/issues/new?title=unexpected%20CallExpression%20in%20normalizeMethodCall',
    line: 1,
    column: 42,
    nodeType: 'CallExpression',
    endLine: 1,
    endColumn: 45
  },
  {
    ruleId: 'property',
    severity: 1,
    message: "Unsafe assignment to innerHTML (variable 'copies' initialized with unsafe value at 1:41)",
    line: 1,
    column: 49,
    nodeType: 'AssignmentExpression',
    endLine: 1,
    endColumn: 69
  }
]

willdurand commented 3 years ago

@willdurand could we track back some of the addons that did make the addons-linter to hit the memory limits while being validated on submission to AMO?

This is one of the eslint plugins we enable internally in the addons-linter and this PR is going to introduce some more checks to track back variables to their initialization and/or assignment inside the expression scope, and so it would be good to double-check if the additional checks will increase the amount of memory used while validating addons with bigger js files to parse and traverse.

Here are a few examples (with XPIs):

mozfreddyb commented 3 years ago

There's another concern that I think needs to be documented (or maybe requires us to re-think)

This will become safe with the new patch:

let tpl = `<s>hello</s>;
window["tpl"] = unsafe; // (or window[variableThatHasStringtpl] = unsafe)
foo.innerHTML = tpl;

On the one hand, we draw a line that we do not disallow shitty things that will pass code review. On the other hand, maybe we want to start by allowing only const declarations and consider let/var declarations in a follow-up?! :thinking:

rpl commented 3 years ago

There's another concern that I think needs to be documented (or maybe requires us to re-think)

This will become safe with the new patch:
let tpl = `<s>hello</s>;
window["tpl"] = unsafe; // (or window[variableThatHasStringtpl] = unsafe)
foo.innerHTML = tpl;
On the one hand, we draw a line that we do not disallow shitty things that will pass code review. On the other hand, maybe we want to start by allowing only const declarations and consider let/var declarations in a follow-up?! thinking

uhm, it's a good point, that kind of expressions will be quite tricky to trackback, but if I'm not mistaken that code snippet needs var so that the same binding will also be available on the global object (whereas let should have a lexical scope and not be available as a property of the global object).

And so if I recall correctly with the following declarations:

let tpl = "test1"
var tpl2 = "test2"

window["tpl"] => undefined
window["tpl2"] => "test2"

And so maybe we should don't do any trackback for bindings defined using var and only enable the trackback mechanisms for let and const (which in the end is not big deal, it is quite common to also require let and const instead of var as part of other commonly enabled eslint rules).

As a side note, I'm wondering if it may be worth to make it possible to opt-in or opt-out from the variables trackback mechanism (e.g. enable by default but allow the developer to disable if for any reason they prefer to be more strict, or making it opt-in and only enable it if the developer explicitly choose to use it). wdyt?

mozfreddyb commented 3 years ago

I made it so that we only allow tracing back for const and let.

As a side note, I'm wondering if it may be worth to make it possible to opt-in or opt-out from the variables trackback mechanism (e.g. enable by default but allow the developer to disable if for any reason they prefer to be more strict, or making it opt-in and only enable it if the developer explicitly choose to use it). wdyt?

I suggest we do the following: 1) put this behind an option that is disabled by default and push a minor semver-version, folks can opt in if they want to 2) then, enable this by default and push the update as a major version. people will (hopefully) know that a major bump includes major changes.

WDYT?

rpl commented 3 years ago

I made it so that we only allow tracing back for const and let.

As a side note, I'm wondering if it may be worth to make it possible to opt-in or opt-out from the variables trackback mechanism (e.g. enable by default but allow the developer to disable if for any reason they prefer to be more strict, or making it opt-in and only enable it if the developer explicitly choose to use it). wdyt?

I suggest we do the following:
1. put this behind an option that is disabled by default and push a minor semver-version, folks can opt in if they want to

2. then, enable this by default and push the update as a major version. people will (hopefully) know that a major bump includes major changes.
WDYT?

that sound a pretty reasonable approach :+1:

I'll give this another review pass asap (and also run some tests with the extensions that Will linked for us a few comments ago).