unexpected CallExpression in normalizeMethodName

[glacambre]

Hi, I believe I found a bug, reproducible with the following line:

nvimGuifont.innerHTML = `* { ${Object(_utils_CSSUtils__WEBPACK_IMPORTED_MODULE_1__["guifontsToCSS"])(value)} }`;

Which results in the following messages:

UNSAFE_VAR_ASSIGNMENT   Unsafe assignment to innerHTML                 Due to both security and performance concerns, this may not be set using dynamic values   nvimui.js   1      1
                                                                       which have not been adequately sanitized. This can lead to security issues or fairly
                                                                       serious performance degradation.
UNSAFE_VAR_ASSIGNMENT   Error in no-unsanitized: Unexpected            Due to both security and performance concerns, this may not be set using dynamic values   nvimui.js   1      32
                        callable. Please report a minimal code         which have not been adequately sanitized. This can lead to security issues or fairly
                        snippet to the developers at                   serious performance degradation.
                        https://github.com/mozilla/eslint-plugin-no…
                        -unsanitized/issues/new?title=unexpected%20…
                        CallExpression%20in%20normalizeMethodName

I believe this is preventing me from uploading new versions of my addon to AMO, I get stuck on the upload phase (not getting any warnings back). Would it be possible to temporarily roll back this change? I have a pretty big bug in my addon that I would like to deploy a fix for.

[glacambre]

Looks like the issue I was having with the AMO upload process not finishing is gone, so it was probably unrelated to the issue I'm reporting here.

[mozfreddyb]

The former violation seems like a valid violation in your code that you ought to fix. The second is interesting and likely needs a patch & test. Looking.

[mozfreddyb]

Uh. Can you supply code for the violation that caused the "unexpected callable" error? I don't think I can reproduce...

[glacambre]

The code was obtained by compiling my extension and the error triggered by running web-extlint on it. I can consistently reproduce this issue with the one-liner I gave you so having the whole code base probably is the difference between what I'm doing and what you're doing.

I've attached my extension to this comment, but if you don't want to go through the hurdle of setting up a webextension development environment, feel free to ignore this issue. The urgency of this problem dropped a lot for me when I managed to release a new version on AMO, so I can investigate this bug myself later this week :).

firenvim-0.1.29.zip

[glacambre]

Here's a smaller reproducer: x.innerHTML = y()().

The error is triggered with the following stack trace:

    at RuleHelper.reportUnsupported (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:267:21)
    at RuleHelper.normalizeMethodCall (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:126:18)
    at RuleHelper.getCodeName (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:141:43)
    at RuleHelper.isAllowedCallExpression (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:93:31)
    at RuleHelper.allowedExpression (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:73:28)
    at RuleHelper.checkProperty (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/ruleHelper.js:259:23)
    at AssignmentExpression (/home/me/prog/firenvim/node_modules/eslint-plugin-no-unsanitized/lib/rules/property.js:63:36)
    at listeners.(anonymous function).forEach.listener (/home/me/prog/firenvim/node_modules/eslint/lib/util/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/home/me/prog/firenvim/node_modules/eslint/lib/util/safe-emitter.js:45:38)

The node that triggers the error looks like this:

Node {
  type: 'CallExpression',
  start: 14,
  end: 17,
  loc:
   SourceLocation {
     start: Position { line: 1, column: 14 },
     end: Position { line: 1, column: 17 } },
  range: [ 14, 17 ],
  callee:
   Node {
     type: 'Identifier',
     start: 14,
     end: 15,
     loc: SourceLocation { start: [Position], end: [Position] },
     range: [ 14, 15 ],
     name: 'y',
     parent: [Circular] },
  arguments: [],
  parent:
   Node {
     type: 'CallExpression',
     start: 14,
     end: 19,
     loc: SourceLocation { start: [Position], end: [Position] },
     range: [ 14, 19 ],
     callee: [Circular],
     arguments: [],
     parent:
      Node {
        type: 'AssignmentExpression',
        start: 0,
        end: 19,
        loc: [SourceLocation],
        range: [Array],
        operator: '=',
        left: [Node],
        right: [Circular],
        parent: [Node] } } }

So the problem is that we try to find the name of the function being called in normalizeMethodCall and we can't, because the function being called is dynamic. I don't think this is a problem that can be solved (unless you're hardore and want to go with abstract interpretation) - so the solution would probably be to make CallExpressions behave the same way as ArrowFunctionExpressions in normalizeMethodCall and have them return an empty string for methodName.

Does this explanation seem right to you? If yes, is my proposed solution acceptable to you? If yes, do you want me to open a PR? :)

[mozfreddyb]

So the problem is that we try to find the name of the function being called in normalizeMethodCall and we can't, because the function being called is dynamic. I don't think this is a problem that can be solved (unless you're hardore and want to go with abstract interpretation) - so the solution would probably be to make CallExpressions behave the same way as ArrowFunctionExpressions in normalizeMethodCall and have them return an empty string for methodName.

Does this explanation seem right to you? If yes, is my proposed solution acceptable to you? If yes, do you want me to open a PR? :)

Yeah. Thanks for digging into it and coming up with an explanation. I agree that we can not look into what "y()" returns and need to bail out (i.e., allow). Please go ahead and issue a pull request, but please include a test.

[mozfreddyb]

I think this should be fixed with recent pull requests (was it #171?). Let me know if this is not the case and I'll happily reopen!