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

[mozfreddyb]

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.

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

[rpl]

@mozfreddyb let me restate the expected outcomes to be sure that I'm looking at this from the right perspective:

• Expected outcome: This changes are expected to reduce linting errors collected for non literal values that can still be tracked back to a safe value assigned to a variable in the same scope of the validated expression.

• Rationale: a "regular user" of this eslint plugin would be likely suppressing those linting errors using an eslint inline comment (for "regular users" of this eslint plugin I mean a developer that decide to run this checks on its own sources, vs. the slightly unconventional use cases like the addons-linter where the developer can't configure which rules to enable/disable, the system is deciding what to check and on which linting erros are "blocking issues").

The changes proposed in this PR looks reasonable to me 👍

As we discussed over slack, from an addons-linter perspective I'd like to double-check if these changes may increase the amount of memory needed to validate some addons that includes bigger javascript files (which did already happen in the past, e.g. when we migrated between two eslint major release versions), I'm going to double-check with my teammates if we can track back some of those extensions and compare the addons-linter memory usage while validating those.

Besides that, I do have just the two review comments that follows (one related to provide some additional pointers about what did make the expression unsafe as part of the linting messages, the second one is about increasing the test coverage with other cases that we would like to be sure that are considered unsafe).

[rpl]

@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]

@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]

@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):

• Addon Validation Error addons#1330
• Addon Validation Error addons#1263
• Addon Validation Error addons#1188
• JavaScript heap out of memory validation error generated for addons with large JS files addons-linter#2506

[mozfreddyb]

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?! 🤔

[rpl]

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]

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]

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 👍

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).

[rpl]

@mozfreddyb my apologies for the time I needed to be able to get back to this pull request.

Follows a new round of review comments, focused mainly on the changes to RuleHelper.allowedExpression (some nitpicks and/or proposed changed related to the readability, some other potential corner cases that we may want to look into and/or special case and add test coverage for).

[rpl]

@mozfreddyb Below a couple more request for changes (they should all small tweaks and shouldn't require any bit rework).

I guess that the option you did mention in #169 (comment) is going to be introduced in a separate follow up and before a new version is released to npm, is that right?
(in particular I wanted to double-check that with you explicitly, to be sure to keep in mind which parts may be out of scope from reviewing this particular PR and better fit in that separate follow up).

[rpl]

@mozfreddyb I gave this another review pass along with trying to run an addons-linter instance using this version of the plugin on the extensions that Will linked us on #169 (comment)

Based on that:

• I found out a couple of cases that we should expect and handle while trying to track back variables (FunctionDeclaration, FunctionExpression and ArrowFunctionExpression), described in more detail below along with a diff for 3 additional test case to cover them explicitly
• The error message for the unexpected expression types while looking for the variables does have line and number set as undefined because we are not retrieving them from from loc.start as we do for the other error.

On the bright side, I took a look to comparing (but without actually profiling) the linter running validations on all the 4 extensions linked and:

• the validation results got were the same
• using the same limit that we are using at the moment in the AMO validators workers (using node --max-old-space-size=2048 ...) the validations did complete with both versions
• the time to complete the run were almost the same (in some cases slightly higher or slightly shorter, but both runs were in the same order in terms of cpu usage and time)

I marked this as request changes to handle the cases described below, but it would also fine to handle them in a separate follow up if we prefer (let me know if you prefer that, and I'll approve this PR and be ready to review the follow up).

[rpl]

@mozfreddyb Thanks! The details error messages looks even better.

This version looks great to me! 🎉