Closed Bug 1264649 Opened 3 years ago Closed 3 years ago

ESLint rule to prevent usage of chrome privileged APIs in devtools/client code

Categories

(DevTools :: General, enhancement, P1)

enhancement

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Iteration:
50.4 - Aug 1
Tracking Status
firefox50 --- fixed

People

(Reporter: pbro, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 1 obsolete file)

As we're moving to a no-XUL no-chrome world, we need to make sure we don't introduce new usages of inIDOMUtils in our client code.

We have a number of them now: 
https://dxr.mozilla.org/mozilla-central/search?q=inIDOMUtils+path%3Adevtools%2Fclient&redirect=false&case=false

All of which will need to be removed during the course of the devtools.html project.

We should make sure we don't re-introduce new usages in the future.
To do this, I suggest an ESLint rule that checks for inIDOMUtils in files.
And each line that use it today can be annotated with // eslint-disable-line

Then it'd be up to reviewers to check that new usages with this annotations aren't added.
Severity: normal → enhancement
This should also check for Ci.nsIDOM* constants.
We should create a more general rule. Looking at the inspector code (as an example), there are things like: Cu.import, Ci.nsIClipboardHelper, Ci.nsIThread, Ci.nsIFocusManager, Ci.nsIXULRuntime, Ci.nsIPrincipal, Ci.nsIDOMCSSRule, Ci.nsIIOService, Ci.nsIXULChromeRegistry, Ci.nsIPrefLocalizedString, Ci.nsIDOMParser, ...

Basically everything we created a shim for in the devtools.html prototype in 2015:
https://github.com/jlongster/ff-devtools-libs/blob/master/sham/chrome.js
Summary: ESLint rule to prevent usage of inIDOMUtils in devtools/client code → ESLint rule to prevent usage of chrome privileged APIs in devtools/client code
Whiteboard: [devtools-html]
Blocks: devtools-html-phase2
No longer blocks: devtools-html-3
When going through the files I think we've been looking for uses of require("chrome"), Ci,
Cu, Cc, Cr, Services, imports of jsms, and requires of things from sdk/.
We probably also must reject chrome: and resource: URLs, and require()s of .jsm files.
I have a rule to selectively reject require()s based on what is being required.
It uses:

/^(chrome|chrome:.*|resource:.*|devtools\/server\/.*)/.test(path)

However the difficulty is that we can't actually enable it until we're further along.
(In reply to Tom Tromey :tromey from comment #5)
> I have a rule to selectively reject require()s based on what is being
> required.
> It uses:
> 
> /^(chrome|chrome:.*|resource:.*|devtools\/server\/.*)/.test(path)
> 
> However the difficulty is that we can't actually enable it until we're
> further along.
Or we could only enable it for the few files that don't require chrome stuff anymore.
Since this is only ever going to be important for client code, we could add an .eslintrc file in devtools/client that enables it. And then add comments that disable the rule in the files that haven't been converted yet.
Or if it's too soon/too many files, maybe we can add the .eslintrc just in the devtools/client/inspector subfolder for the moment.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Just a reminder that adding new rules need to be packaged for automation:

1. Bump the version number[1] of the ESLint plugin
2. Update the version the mach command checks for to match (so local users know to update)
3. Update the ESLint archive in tooltool using the update script[3] so automation uses the new code

[1]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/package.json
[2]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/mach_commands.py#36
[3]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/update

The tooltool steps need special access.  Ping me or Mike for help with this.
Comment on attachment 8773314 [details]
Bug 1264649 - add reject-some-requires eslint rule;

https://reviewboard.mozilla.org/r/66058/#review63152

::: devtools/.eslintrc:39
(Diff revision 1)
>      "mozilla/no-aArgs": 1,
>      "mozilla/no-cpows-in-tests": 2,
>      "mozilla/no-single-arg-cu-import": 2,
>      // See bug 1224289.
>      "mozilla/reject-importGlobalProperties": 2,
> +    "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|\\.jsm)$"],

You're removing this in the next commit. So there's no need to add it here in the first place.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-some-requires.js:35
(Diff revision 1)
> +          node.arguments.length == 1 &&
> +          node.arguments[0].type == "Literal") {
> +        let path = node.arguments[0].value;
> +
> +        if (RX.test(path)) {
> +          context.report(node, `invalid require(${path}) for content code`);

I think this needs rephrasing. This rule is a general one that people may find useful for all sorts of stuff. We happen to use it to prevent some kinds of require in "content" code, but not everyone is going to use it for this.

So, maybe something like:

`requiring a module from ${path} is not allowed`
Attachment #8773314 - Flags: review?(pbrosset) → review+
Attachment #8773315 - Flags: review?(pbrosset) → review+
Comment on attachment 8773315 [details]
Bug 1264649 - enable reject-some-requires rule for inspector;

https://reviewboard.mozilla.org/r/66060/#review63156

::: devtools/client/inspector/.eslintrc:6
(Diff revision 1)
> +{
> +  // Extend from the devtools eslintrc.
> +  "extends": "../../.eslintrc",
> +
> +  "rules": {
> +    "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|\\.jsm)$"],

I think we should add a comment here because this is an ongoing effort. Only 9 out of 22 files in the inspector already comply to this rule, so there's still a lot to do.
Don't know if this is the best place, but I couldn't think of any other.
I'd add something like:

// The inspector is being migrated to HTML and cleaned of chrome-privileged code, so this rule disallows requiring chrome code. Some files in the inspector disable this rule still. The goal is to enable the rule globally on all files.
Blocks: devtools-html-3
No longer blocks: devtools-html-phase2
Iteration: --- → 50.4 - Aug 1
Flags: qe-verify-
Priority: -- → P1
(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #10)

> > +    "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|\\.jsm)$"],
> 
> You're removing this in the next commit. So there's no need to add it here
> in the first place.

Thanks for noticing that.  It was a leftover from some testing, oops.
(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #11)

> I think we should add a comment here because this is an ongoing effort. Only
> 9 out of 22 files in the inspector already comply to this rule, so there's
> still a lot to do.

I've also updated the patch to re-enable the rule after each bad require.
So now a given require might look like:

/* eslint-disable mozilla/reject-some-requires */
var { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
/* eslint-enable mozilla/reject-some-requires */


The idea here is that it will be obvious now that removing the require should also
mean removing the eslint comment.
(In reply to Tom Tromey :tromey from comment #13)
> /* eslint-disable mozilla/reject-some-requires */
> var { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm");
> /* eslint-enable mozilla/reject-some-requires */
Great idea!
I belatedly realized that this has to check lazyRequireGetter as well.
Comment on attachment 8773314 [details]
Bug 1264649 - add reject-some-requires eslint rule;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66058/diff/1-2/
Comment on attachment 8773315 [details]
Bug 1264649 - enable reject-some-requires rule for inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66060/diff/1-2/
:jryans - pinging about tooltool
Flags: needinfo?(jryans)
Attached patch tooltool upload addendum (obsolete) — Splinter Review
Please fold this additional patch into your new rule patch.

With this added in, you can see the effect of your changes on try and should be ready to land if it's green.
Flags: needinfo?(jryans)
Comment on attachment 8773314 [details]
Bug 1264649 - add reject-some-requires eslint rule;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66058/diff/2-3/
Comment on attachment 8773315 [details]
Bug 1264649 - enable reject-some-requires rule for inspector;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66060/diff/2-3/
Comment on attachment 8773857 [details] [diff] [review]
tooltool upload addendum

Rolled in.
Attachment #8773857 - Attachment is obsolete: true
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a81edfa43440
add reject-some-requires eslint rule; r=pbro
https://hg.mozilla.org/integration/autoland/rev/eb5d2f565ba4
enable reject-some-requires rule for inspector; r=pbro
https://hg.mozilla.org/mozilla-central/rev/a81edfa43440
https://hg.mozilla.org/mozilla-central/rev/eb5d2f565ba4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
No longer blocks: devtools-html-phase2
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.