distinguishing between global functions and classmethods (Improve 'import' keyword warning)

[erosman]

transferred from: https://github.com/mozilla/addons-linter/issues/3182

When uploading my addon I noticed a warning by the validator: It appears that the class method named import generates the warning meant for JavaScript method import.

Unsafe call to this.import for argument 0

Due to both security and performance concerns, this may not be set using dynamic values which have not been adequately sanitized. This can lead to security issues or fairly serious performance degradation. content/pref.js line 25 column 71

Unsafe call to Pref.import for argument 0

Due to both security and performance concerns, this may not be set using dynamic values which have not been adequately sanitized. This can lead to security issues or fairly serious performance degradation. content/options.js line 722 column 67

The code in question (simplified):

class Pref {

  static importExport() {
    document.getElementById('file').addEventListener('change', (e) => this.import(e));
  }

  static import(e) {
    // code
  }

  static export() {
    // code
  }
}

[mozfreddyb]

Indeed, we should find out if there's a way to distinguish between built-ins and class methods. According to astexplorer.net, it seems we could whitelist MethodDefinitions and treat them differently.

[mozfreddyb]

Hm, so this is a tiny bit trickier than I thought. the syntax tree is as follows (excerpt): We have a CallExpression (which is where we start looking), in which "this.import" is called (that's a MemberExpression with a ThisExpression and an Identifier called "import").

Looking up the syntax tree, we'll eventually find that we're in a FunctionExpression that belongs to a MethodDefinition within a ClassBody, however: Do we have to account for shenanigans which change the notion of "this"? I noticed that Class Bodys and method definitions execute in strict mode, which disallows with() statements. Is that all though?

OTOH, our thread model does not accept "code that wouldn't pass peer review".

[erosman]

If you are asking me, is there a way to establish whether this refers to a specific object or it is global (e.g. window object)?

[mozfreddyb]

Hm. The way dynamic imports work is still not completely finished, it seems. There are significant ast differences between parsers on astexplorer.net. It's hard to tell between good and bad cases when the bad cases are still changing :|

As a temporary workaround, I suggest using a different function name. But I will get back to this!

[erosman]

As a temporary workaround, I suggest using a different function name.

It is only a warning and does not (should not) complicate the review process (I am an add-on reviewer myself).

I have already made some changes to get rid of the using dynamic values which is the heart of the warning e.g. (e) => this.import(e), by not using the arrow function to bind this to the Class Object. e.g.:

document.getElementById('file').addEventListener('change', this.import);

The report merely highlights the possibility of conflict with other keywords (not only import) and worth resolving.

[gijsk]

So this is blocking updating the in-mozilla-central (ie Firefox's) version of this library because we use ChromeUtils.import all over the shop (although addmittedly it's relatively rare to be using an identifier as the first argument).

@mozfreddyb would you take a patch to not warn for method calls to import? As far as I can tell, in modern JS parsers, import is treated as a special keyword, and this.import or global.import does not actually work, and you cannot access import as if it were a function (so you can't assign a reference to it to a different variable).

[mozfreddyb]

As I mentioned previously, there's still lots of differences in how dynamic imports are parsed into ASTs. I hope we can account for that (details in your pull request) and resolve this. Thanks for taking a look @gijsk