Closed Bug 1429185 Opened 6 years ago Closed 6 years ago

Policy: Disable Developer Tools

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox59 --- wontfix
firefox60 --- verified

People

(Reporter: Felipe, Assigned: jdescottes)

References

Details

Attachments

(5 files)

There are hacky ways around this that CCK2 does. We should figure out the scope of this for the MVP
Patrick Brosset directed us to Julian Descottes as the best subject matter expert for questions (and gave Julian a heads up). He has recently been working on decoupling DevTools from the rest of the browser code so that it's possible to disable it.
I did some quick tests for fun and if we can somehow just make devtools-startup not happen, then non of devtools is available in the browser. The menu is still there, though, with view source.

It also leaves behind inspect element, but it does noething.

Julian, do you have any ideas how we could easily prevent the startup of devtools, similar to what y'all were thinking in the decoupling of devtools?
Flags: needinfo?(jdescottes)
(In reply to Mike Kaply [:mkaply] from comment #2)
> I did some quick tests for fun and if we can somehow just make
> devtools-startup not happen, then non of devtools is available in the
> browser. The menu is still there, though, with view source.
> 
> It also leaves behind inspect element, but it does noething.
> 
> Julian, do you have any ideas how we could easily prevent the startup of
> devtools, similar to what y'all were thinking in the decoupling of devtools?

That shouldn't be too difficult, the main difference in our plans for "disabling" devtools is that entry points remain, but lead to an installation/enabling screen instead of opening the tools. But at least Firefox is supposed to work fine now when DevTools are not started or even loaded. 

There are four startup components for devtools that need to be disabled or not included in the build:
- devtools/shim/devtools-startup.js
- devtools/shim/aboutdebugging-registration.js
- devtools/shim/aboutdevtoolstoolbox-registration.js
- devtools/shim/aboutdevtools/aboutdevtools-registration.js

Disabling devtools-startup will effectively:
- disable all DevTools keyboard shortcuts
- prevent DevTools menus from being populated
- disable all DevTools command line arguments

If devtools-startup is disabled, the 3 other components will not have much impact but would still register about: urls (eg about:debugging). So it would be nice to stop registering them if we want to hide devtools completely.

To disable the components, we have several options depending on how much you want to remove devtools from those builds:
- use a preference if we want to keep the option to use devtools. Can be useful for ourselves if we need to investigate something on one of those builds?
- introduce a new value for MOZ_DEVTOOLS (https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/browser/confvars.sh#62). (MOZ_DEVTOOLS=server might be enough here?)

How much do we want to prevent users of those builds from accessing devtools? How much do we want to keep those builds debug-able?

Regarding Inspect Element and the other Developer menus, they are not created by devtools-startup. We will need additional code to read the preference (or MOZ_DEVTOOLS) and hide the entries.

Random thought regarding testing: I don't know how the CI runs for those builds but we will need to make sure that all the devtools tests are disabled, as well as the webextensions tests that rely on DevTools.
Flags: needinfo?(jdescottes)
This would definitely need to be a runtime thing, not a build time thing. Goal would be that on startup, the policy manager would check if devtools should be disabled and if so, disable it. We already have the ability to override about screens, so we could do that separately.

So a preference would be perfect. Where we set and lock that preference to disable devtools.

See the similar option in Chrome:

https://www.chromium.org/administrators/policy-list-3#DeveloperToolsDisabled
(In reply to Julian Descottes [:jdescottes][:julian] from comment #3)
> Random thought regarding testing: I don't know how the CI runs for those
> builds but we will need to make sure that all the devtools tests are
> disabled, as well as the webextensions tests that rely on DevTools.

I'm not sure there is any CI/tests being run against these policy manager features. Do we?
My comprehension is that it should work against regular Firefox builds?

devtools-startup is registered as a command-line-handler,
Would the policy manager have any control on this already?

Otherwise, hacking a new "devtools.disabled" or similar pref into devtools-startup.js:DevToolsStartup.handle() to:
* return early and prevent registering any devtools specifics,
* remove UI you don't want. or refactor the UI elements like devtools menus to be added only by devtools-startup.

But may be it is easier to remove UI when disabled, for performance reasons. To optimize the common behavior, where devtools are enabled.
> I'm not sure there is any CI/tests being run against these policy manager features. Do we?
> My comprehension is that it should work against regular Firefox builds?

Yes, there should be no problem with tests. There will be a test specifically that disables developer tools in the policy engine. It would not affect other tests.

> devtools-startup is registered as a command-line-handler,
> Would the policy manager have any control on this already?

That was actually my first thought. The policy manager is initialized before devtools so I'm not sure if we can prevent it. If it happened after, we could register a new component.

> But may be it is easier to remove UI when disabled, for performance reasons. To optimize the common behavior, where devtools are enabled.

I would rather just simply have devtools not even initialize in this case. Since this shouldn't be a dynamic (runtime) thing.
It looks like just a throw() during starting is enough to make devtools not initialize at all.

You don't get the Javascript console either, but I'm not that concerned about that.

I did find a bug though. When devtools isn't initialized, you don't get page source in the web developer menu in the hamburger menu, but you do get it in the menubar menu.
(In reply to Mike Kaply [:mkaply] from comment #7)
> I did find a bug though. When devtools isn't initialized, you don't get page
> source in the web developer menu in the hamburger menu, but you do get it in
> the menubar menu.

Looks like that's related to this comment[1] in browser-menus.js.  Perhaps it should be removed from browser.xul and dynamically added by DevTools startup instead, if we want to hide even view source here (seems innocuous to me since you can't edit things, but not sure of the requirements here in detail).

[1]: https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/devtools/client/framework/browser-menus.js#225
(In reply to Mike Kaply [:mkaply] from comment #7)
> It looks like just a throw() during starting is enough to make devtools not
> initialize at all.

Yes, an early return in handle() method should be enough to disable most of devtools.
I introduced this xpcom in FF57 to ensure devtools were really lazy loading everything
until the user really intent to use them.

> You don't get the Javascript console either, but I'm not that concerned
> about that.

Note that it will also disable devtools command line args, like --jsconsole, --jsdebugger, --start-debugger-console.
It makes Firefox completely non-debuggable, but I imagine that's an expected side effect.

> I did find a bug though. When devtools isn't initialized, you don't get page
> source in the web developer menu in the hamburger menu, but you do get it in
> the menubar menu.

This is what I was thinking about when I said:

> * remove UI you don't want.

Now, you can also do the removal from policy manager, but may be it is best to keep that in devtools land?
And so let devtools-startup auto disable itself and remove all UI that need to disappear.

I imagine you would have to do:
  CustomizableUI.destroyWidget("developer-button")
  To remove the hamburger menu entry.
  But watchout, this is a default widget that may be expected to exists from other places:
    https://searchfox.org/mozilla-central/search?q=developer-button&case=false&regexp=false&path=
And:
  let menu = doc.getElementById("menuWebDeveloperPopup");
  menu.remove();
  To get rid of "Web Developer" system menu completely.
If this should be a runtime thing only, then let's go for a preference (I would call it devtools.disabled, but that will be extremely confusing next to devtools.enabled!) and let devtools-startup remove unneeded UI elements.

If I understand correctly the policy manager works with regular Firefox builds, and starts before we initialize the command line handlers? Here it would simply force the preference before we initialize devtools-startup.

> Note that it will also disable devtools command line args, like --jsconsole, --jsdebugger, 
> --start-debugger-console. It makes Firefox completely non-debuggable, but I imagine that's an expected side effect.

Thinking about that, we could maybe just keep "--start-debugger-server" available in order to connect to such Firefox instances?
> If this should be a runtime thing only, then let's go for a preference (I would call it devtools.disabled, but that will be extremely confusing next to devtools.enabled!) and let devtools-startup remove unneeded UI elements.

I can reuse devtools.enabled but make it so that if it locked it means completely disable devtools at startup.

> If I understand correctly the policy manager works with regular Firefox builds, and starts before we initialize the command line handlers? Here it would simply force the preference before we initialize devtools-startup.

Yep.

> Thinking about that, we could maybe just keep "--start-debugger-server" available in order to connect to such Firefox instances?

Not a bad idea. I was wondering if there was a way to keep the js console as well, but then I realized you could run Components commands on the console command line, so not very secure :).
(In reply to Mike Kaply [:mkaply] from comment #11)
> I was wondering if there was a way to keep the js console as
> well, but then I realized you could run Components commands on the console
> command line, so not very secure :).

If the pref `devtools.chrome.enabled` is false, the browser-level JS console is read-only (there is no input field).  Perhaps that's useful to keep around?
First pass is up. Because of how well the devtools startup was done, this ended up being much simpler than I thought.

Our only real goal is to prevent user access to devtools, so all I did was hook into the window startup and not handle that part (and hide the menus). Everything else starts up fine. And page source is still available via the page context menu, so we aren't losing that.

This also means in theory we should be able to hook up just the javascript console if we wanted (I'm looking at that). So we could have a "DisableDeveloper tools but enable JS console"

Also, when bug 1421707, we'll get disabling about:devtools.

This still needs a test.

Two things I've learned from this.

1. Devtools did a great job with the separation of devtools startup. Compared to how I've had to mess with devtools removal in the past, this is so much easier.
2. Felipe did a great job with the design of the policy engine itself. Made doing this a breeze.
Comment on attachment 8949466 [details]
Bug 1429185 - Implement policy for disabling devtools

https://reviewboard.mozilla.org/r/218786/#review224564


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/shim/devtools-startup.js:251
(Diff revision 1)
>     * top-level window.
>     */
>    onWindowReady(window) {
> +    if (!Services.policies.isAllowed("devtools")) {
> +      window.document.getElementById("webDeveloperMenu").setAttribute("hidden", "true");
> +      window.document.getElementById("appMenu-developer-button").setAttribute("hidden", "true");

Error: Line 251 exceeds the maximum line length of 90. [eslint: max-len]
Comment on attachment 8949466 [details]
Bug 1429185 - Implement policy for disabling devtools

https://reviewboard.mozilla.org/r/218786/#review224602

This is exciting! Here's a random drive-by.

Minor nit: please add the DisableDeveloperTools entries in Policies.jsm and policies-schema.json in alphabetical order
(some are already wrong there but I'll fix them when I change the first ones to CamelCase)

::: browser/components/enterprisepolicies/Policies.jsm:99
(Diff revision 1)
>  
> +  "DisableDeveloperTools": {
> +    onBeforeAddons(manager, param) {
> +      if (param == true) {
> +        setAndLockPref("devtools.enabled", false);
> +        manager.disallowFeature("devtools", true);

since the code calling .isAllowed("devtools") is only running in the parent process, you shouldn't use the second parameter here as `true`, to avoid this information being sent down to the parent process.

::: browser/components/enterprisepolicies/Policies.jsm:100
(Diff revision 1)
> +  "DisableDeveloperTools": {
> +    onBeforeAddons(manager, param) {
> +      if (param == true) {
> +        setAndLockPref("devtools.enabled", false);
> +        manager.disallowFeature("devtools", true);
> +        manager.disallowFeature("about:devtools", true);

Similarly here, it looks like about:devtools is only opened in the parent process (because https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/devtools/shim/aboutdevtools/aboutdevtools-registration.js#34 ) doesn't have URI_CAN_LOAD_IN_CHILD.

So you don't need true in the second parameter.

The style has been to just omit the second param instead of using `false`
For testing, I imagine the devtools startup is super-early, and I don't know if it will dynamically respect the change of devtools.enabled to false. If that's the case, you'll need to set up the test in a way that the browser already starts with the policy engine active (different from most tests where the test file dynamically restarts the engine with the browser running).

(even if it does respect the pref change, it's probably a good idea to test the normal startup path instead of the pref observer path)

The good news is that Kirk is just about to land a super easy way to do this, in bug 1429150, since it was also needed to test the app update policy.
Comment on attachment 8949466 [details]
Bug 1429185 - Implement policy for disabling devtools

https://reviewboard.mozilla.org/r/218786/#review228252

::: browser/base/content/nsContextMenu.js:445
(Diff revision 1)
>      this.showItem("context-viewsource", shouldShow);
>      this.showItem("context-viewinfo", shouldShow);
>      // The page info is broken for WebExtension popups, as the browser is
>      // destroyed when the popup is closed.
>      this.setItemAttr("context-viewinfo", "disabled", this.webExtBrowserType === "popup");
> +    if (Services.policies.isAllowed("devtools")) {

Should reuse Services.policies.isAllowed("devtools") to compute the showInspect variable above rather than introducing a new if.

::: devtools/shim/devtools-startup.js:249
(Diff revision 1)
>    /**
>     * Called when receiving the "browser-delayed-startup-finished" event for a new
>     * top-level window.
>     */
>    onWindowReady(window) {
> +    if (!Services.policies.isAllowed("devtools")) {

I would prefer to use a preference here, eg devtools.hidden? Or maybe we introduce a devtools.status preference and allow several values to support "disabled", "not enabled" and "enabled" with a single pref. This will allow us to test disabling devtools simply playing with preferences and you can still lock it from Policies.jsm
Mike, how do you want to proceed here? 

Should we implement something based on a pref to disable devtools (and menus etc...) and then you can create a patch on top of that, forcing the pref from the policy engine? Is this something we want to ship for 60?
Flags: needinfo?(mozilla)
> Should we implement something based on a pref to disable devtools (and menus etc...) and then you can create a patch on top of that, forcing the pref from the policy engine?

If you could to that, that would be great.

> Is this something we want to ship for 60?

Yes, we need this for 60.

Is that possible? OR should I clean up my patch for now?
Flags: needinfo?(mozilla)
Should be ok for 60, let me get a patch ready for the non-policy part. I think I'll use a dedicated preference for now and will attempt to merge them in 61 cycle.
Comment on attachment 8953126 [details]
Bug 1429185 - Disable all devtools entry points with devtools.policy.disabled;

Alex, can you have a look at the patch and let me know if that seems reasonable to you? If yes I'll start adding tests.

As I said above, I'd like to merge the enabled and policy.disabled prefs in a single pref, but I would prefer to do that in cycle 61.
Attachment #8953126 - Flags: feedback?(poirot.alex)
Comment on attachment 8953126 [details]
Bug 1429185 - Disable all devtools entry points with devtools.policy.disabled;

Looks good to me.

I think I'm also looking forward an answer to your previous question:
> Thinking about that, we could maybe just keep "--start-debugger-server" available in order to connect to such Firefox instances?

Sys admins may want to really lock down the firefoxes and disallow any privileged access. --start-debugger-server will allow to connect to firefox and have a "browser toolbox", which allows any arbitrary execution of chrome code. I imagine they may control that, in parallel, by locking devtools.debugger.remote-enabled to false. Locking down devtools.chrome.enabled to false will also disable this feature. But that's something to document around this new policy.

Any thought on that Mike, Felipe?
Attachment #8953126 - Flags: feedback?(poirot.alex) → feedback+
If someone is locking down Firefox, I would imagine they locked down the shortuct and command line as well.

I'm honestly not sure. This is something we can not worry about in this version and if it becomes an issue, we can fix.
Added an explicit comment about the "--start-debugger-server". Our options are either to keep the implementation as it is now, and the policy engine can force devtools.debugger.remote-enabled in order to really prevent any debugging. Or I can change the implementation to block it via devtools.policy.disabled.

Also added a test and a last changeset to avoid registering about: pages. 

Regarding this last part, I'm afraid that reading preferences directly in the registration code is not a great idea. I couldn't find a good way to dynamically disable those pages, so the best for now is probably to disable them from the policy engine. As a reminder the pages to disable are:
- about:devtools
- about:debugging
- about:devtools-toolbox
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Comment on attachment 8953523 [details]
Bug 1429185 - Implement policy for disabling devtools;

https://reviewboard.mozilla.org/r/222750/#review228776

::: browser/components/enterprisepolicies/Policies.jsm:97
(Diff revision 2)
>    },
>  
> +  "DisableDeveloperTools": {
> +    onBeforeAddons(manager, param) {
> +      if (param) {
> +        setAndLockPref("devtools.policy.disabled", true);

I think this should also lock devtools.chrome.enabled.

Also, while you're at here, I believe the DisableAboutConfig policy should also lock devtools.chrome.enabled to false. That would be an alternative for sysadmins to prevent arbitrary meddling with Firefox while also letting devtools to run if they wish.

::: browser/components/enterprisepolicies/tests/browser/disable_developer_tools/browser_policy_disable_developer_tools.js:19
(Diff revision 2)
> +
> +  Services.prefs.setBoolPref("devtools.policy.disabled", false);
> +
> +  is(Services.prefs.getBoolPref("devtools.policy.disabled"), true,
> +     "devtools dedicated disabled pref can not be updated");
> +

I know the other test browser_shim_disable_devtools.js is testing this pref, but as a sanity check in this test too, could you check at least one devtools UI entry to make sure it took effect?  Whatever is easier to check: menu, button...
Attachment #8953523 - Flags: review?(felipc) → review+
Comment on attachment 8953126 [details]
Bug 1429185 - Disable all devtools entry points with devtools.policy.disabled;

https://reviewboard.mozilla.org/r/222394/#review229104

I tried to test all the entrypoints I'm aware of and it looks good overall.

Note that there are special ones like:
chrome://devtools/content/inspector/inspector.xhtml?type=process
Same with console, toolbox, ... you may also use type=tab if you manage to guess the document id.
May be the policy manager knows how to unregister a chrome package?

I'm wondering if you should force devtools.enabled to false when the policy pref is set.
Would that help mitigate these quite unexpected entrypoint?

It is still unclear how important it is to block devtools at 100% and/or limit the access to chrome privileges.
So I'm not sure these particular entrypoints are really important to address.

::: devtools/shim/devtools-startup.js:219
(Diff revision 2)
>        Services.obs.addObserver(this.onWindowReady, "browser-delayed-startup-finished");
>  
>        if (AppConstants.MOZ_DEV_EDITION) {
>          // On DevEdition, the developer toggle is displayed by default in the navbar area
>          // and should be created before the first paint.
>          this.hookDeveloperToggle();

You may want to also disable this call if the policy is set.
Attachment #8953126 - Flags: review?(poirot.alex) → review+
Comment on attachment 8953522 [details]
Bug 1429185 - Add test for DevTools disabled by policy;

https://reviewboard.mozilla.org/r/222744/#review229098

::: devtools/shim/tests/browser/browser_shim_disable_devtools.js:52
(Diff revision 1)
> +  ok(inspectElementItem.hidden, "The inspect element item is hidden in the context menu");
> +
> +  let toolsMenu = win.document.getElementById("webDeveloperMenu");
> +  ok(toolsMenu.hidden, "The Web Developer item of the tools menu is hidden");
> +  let hamburgerMenu = win.document.getElementById("appMenu-developer-button");
> +  ok(hamburgerMenu.hidden, "The Web Developer item of the hamberger menu is hidden");

s/hamberger/hamburger/

::: devtools/shim/tests/browser/browser_shim_disable_devtools.js:54
(Diff revision 1)
> +  let toolsMenu = win.document.getElementById("webDeveloperMenu");
> +  ok(toolsMenu.hidden, "The Web Developer item of the tools menu is hidden");
> +  let hamburgerMenu = win.document.getElementById("appMenu-developer-button");
> +  ok(hamburgerMenu.hidden, "The Web Developer item of the hamberger menu is hidden");
> +
> +  win.BrowserTryToCloseWindow();

It may be safer (if we ever run more than one test in this folder) to wait for window full closing before proceeding:
https://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_580512.js#26-33
Attachment #8953522 - Flags: review?(poirot.alex) → review+
Thanks for the reviews, normally addressed all comments.

 (In reply to Alexandre Poirot [:ochameau] from comment #32)
> Comment on attachment 8953126 [details]
> Bug 1429185 - Disable all devtools entry points with
> devtools.policy.disabled;
> 
> https://reviewboard.mozilla.org/r/222394/#review229104
> 
> I tried to test all the entrypoints I'm aware of and it looks good overall.
> 
> Note that there are special ones like:
> chrome://devtools/content/inspector/inspector.xhtml?type=process
> Same with console, toolbox, ... you may also use type=tab if you manage to
> guess the document id.
> May be the policy manager knows how to unregister a chrome package?
> 
> I'm wondering if you should force devtools.enabled to false when the policy
> pref is set.
> Would that help mitigate these quite unexpected entrypoint?
> 

Good point! Sadly the only way I know to block those entry points from devtools would be to silence the loader. I'll wait for Felipe to say if blocking such URLs is critical (and if it can be done with the policy manager?).
Flags: needinfo?(felipc)
Comment on attachment 8954102 [details]
Bug 1429185 - BlockAboutConfig policy should lock devtools.chrome.enabled to false;

https://reviewboard.mozilla.org/r/223246/#review229272
Attachment #8954102 - Flags: review?(felipc) → review+
There's no handy way to disable these URLs with the policy manager like there is with about: URLs.
Two things that can be done:
- use Services.policies.isAllowed() inside these pages and make them self-disable their features
- write code on this policy to unregister the chrome package

But honestly if none of that is super easy I don't think that's necessary right now. These entry points are obscure enough that no user would end up on them by accident.
Flags: needinfo?(felipc)
Thanks for the feedback, sounds good to me! Try push at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a8bbdfe7acc8d4f37dd789e9de429d7fd118d1c

Will land when green
Testing the context menu as part of the policy test kept failing on test verification. Removed this part of the test, only checking the menu items for now (it is still tested as part of the devtools test)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc5ec0ec67da1d4394ba705b3def60c40e671461
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fcc02c1022c1
Disable all devtools entry points with devtools.policy.disabled;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/166d98f786e8
Add test for DevTools disabled by policy;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6e7a8511567d
Implement policy for disabling devtools;r=Felipe
https://hg.mozilla.org/integration/autoland/rev/013496665183
BlockAboutConfig policy should lock devtools.chrome.enabled to false;r=Felipe
Wontfix for 59 since we are aiming this policy feature at the 60 release.
Depends on: 1442451
We tested “disable devtools” policy using JSON file.  Devtools can be blocked by this policy and we verified this manually as fixed.

Test steps and runs are available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The bug will also be retested with ADMX files when it is ready for testing.
We retested this on beta builds with ADM and JSON policy formats and it is verified as fixed.
Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.