TypeError: Cannot read properties of undefined (reading \'type\')

[diox]

We saw the following error happen in production (sentry traceback) when validating an add-on through the linter:

Cannot read properties of undefined (reading \'type\')
Occurred while linting service-worker.js:2
Rule: "no-unsanitized/method"

/data/olympia/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:53
       switch(expression.type) {
                          ^
TypeError: Cannot read properties of undefined (reading \'type\')
Occurred while linting service-worker.js:2
Rule: "no-unsanitized/method"
    at RuleHelper.allowedExpression (/data/olympia/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:53:27)
    at /data/olympia/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:394:30
    at Array.forEach (<anonymous>)
    at RuleHelper.checkMethod (/data/olympia/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:390:34)
    at checkCallExpression (/data/olympia/node_modules/eslint-plugin-no-unsanitized/lib/rules/method.js:74:24)
    at CallExpression (/data/olympia/node_modules/eslint-plugin-no-unsanitized/lib/rules/method.js:170:17)
    at ruleErrorHandler ..

I'll try to grab the add-on it occurred on if I can...

[rpl]

@mozfreddyb I gathered some additional details from the service-worker.js file that @diox shared privately with me to help investigating this and determine what should be the correct way on handling these cases, let me know if you need some other details that may be useful to investigate this bug and that I may have missed to mention explicitly in this comment.

The issue is triggered due to the use of the spread operator in a call to insertAdjacentHTML, which per config will be trying to validate for safety the second argument of the insertAdjacentHTML calls:

• code: s[c].insertAdjacentHTML(...l)
• resulting AST node.arguments:

  [
  Node {
  type: 'SpreadElement',
  start: ...,
  end: ..,
  loc: [SourceLocation],
  range: [Array],
  argument: [Node],
  parent: [Node]
  }
  ]

To be totally fair, the same error would also be triggered if a call to insertAdjacentHTML doesn't have a second argument, and so currently the following two also trigger the same exception:

• s[c].insertAdjacentHTML(something)

I'm not sure if we would want to add variable tracing along with fixing this bug, but in the meantime I collected some more details in case we want to consider that:

-If node.arguments[0].type is SpreadElement and node.arguments[0].argument is of type Identifier

• then the variable name l would be actually the node.arguments[0].argument

and so in theory we may even consider to track back the variable and if it is a literal array then we may be able to confirm if the second insertAdjacentHTML argument was safe or not (but we should also take into account that the argument of a SpreadElement may also be something else, e.g. a CallExpression as in n.insertAdjacentHTML(...(someMethod(param)))).

[mozfreddyb]

That's extremely useful. I'll get to it first thing tomorrow.

[mozfreddyb]

So, fixing this error is easy. But satisfying all of your requirements is a bit complicated. We can't try and find some close-ish declaration of arrays for spread-statements, but we'll lose context for most variables relatively easily.

Again, as per our threat model we do not care nor should we complain about code that is minified/obfuscated.

I can do some backtracing, but the code will give up relatively soon. Either way, I'll have a patch ready sometime this week.

[diox]

Should we have a release with the fix, or are we waiting for a fix for #214 as well ?