Policy: Disable safe mode

VERIFIED FIXED in Firefox 60

Status

()

VERIFIED FIXED
a year ago
11 months ago

People

(Reporter: yuki, Assigned: yuki)

Tracking

(Depends on: 2 bugs)

Trunk
Firefox 61
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox60+ verified, firefox61 verified)

Details

Attachments

(1 attachment, 10 obsolete attachments)

10.81 KB, patch
Felipe
: review+
jimm
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
Safe mode is available to run Firefox without extensions deployed by the system administrator. A policy to kill the safe mode is required.
(Assignee)

Comment 1

a year ago
I've tried to implement this policy. Safe-mode can be started from the "Restart with Add-ons Disabled..." help menu item and "about:support", so the policy disables both features.
Assignee: nobody → yuki
Attachment #8956001 - Flags: review?(felipc)
Comment on attachment 8956001 [details] [diff] [review]
Patch to implement policy "DisableSafeMode"

Thanks Yuki. This policy is more complicated than it looks like, so we hadn't originally put it in the MVP list. However, there's a reasonable solution that we'd love if you could implement it..

The problem is that these are the UI entry points for safe mode, but there's another way to get it: holding SHIFT during the startup process (on Windows, Option on Mac).

The problem is that the code that checks that runs very very early in the startup process, at a point in which the prefs haven't been initialized yet, so the policy engine cannot be started.

This is the code: https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/toolkit/xre/nsAppRunner.cpp#3506-3518

The hacky solution for that would be to make this code directly check the windows registry to see if the DisableSafeMode policy exists.
This means that this will be only fully supported on:
  - Windows
  - If using GPO or the Registry, but not the json file

However, that's a good first step for a more complete solution later.. And there's another solution for people not covered by this: set an env var that blocks it

So, could you implement it for us? It would need to check the keys:

HKEY_CURRENT_USER\\SOFTWARE\\Policies\\Mozilla\\Firefox\\DisableSafeMode
HKEY_LOCAL_MACHINE\\SOFTWARE\\Policies\\Mozilla\\Firefox\\DisableSafeMode
Attachment #8956001 - Flags: review?(felipc) → feedback+
Comment on attachment 8956001 [details] [diff] [review]
Patch to implement policy "DisableSafeMode"

Review of attachment 8956001 [details] [diff] [review]:
-----------------------------------------------------------------

Other comments on the patch:

::: browser/base/content/browser.js
@@ +1347,5 @@
>      ToolbarIconColor.init();
>  
>      gRemoteControl.updateVisualCue(Marionette.running);
>  
> +    if (Services.policies.isAllowed("DisableSafeMode")) {

this usage of isAllowed is incorrect. I've explained it in the DisableFeedbackCommands patch, so I don't need to repeat it here

::: browser/components/enterprisepolicies/Policies.jsm
@@ +185,5 @@
> +  "DisableSafeMode": {
> +    onBeforeUIStartup(manager, param) {
> +      if (param) {
> +        manager.disallowFeature("safemode");
> +        manager.disallowFeature("about:support", true);

There's already a policy to fully block about:support, so this one shouldn't do it. It should just disable the safemode button on it.
(Assignee)

Comment 4

a year ago
Thank you for reviewing!

(In reply to :Felipe Gomes (needinfo me!) from comment #2)
> Comment on attachment 8956001 [details] [diff] [review]
> So, could you implement it for us? It would need to check the keys:
> 
> HKEY_CURRENT_USER\\SOFTWARE\\Policies\\Mozilla\\Firefox\\DisableSafeMode
> HKEY_LOCAL_MACHINE\\SOFTWARE\\Policies\\Mozilla\\Firefox\\DisableSafeMode

OK, I try to do that. Anyway I update the patch based on your review at first.
(Assignee)

Comment 5

a year ago
Updated patch. Menu item and button are just disabled.
Attachment #8956001 - Attachment is obsolete: true
Attachment #8956323 - Flags: review?(felipc)
(Assignee)

Comment 6

a year ago
Trying to add workaround on nsAppRunner.cpp.
Because currently I'm failing to prepare full-build environment on Windows yet, this patch is not tested.
Attachment #8956323 - Attachment is obsolete: true
Attachment #8956323 - Flags: review?(felipc)
Comment on attachment 8957118 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v3

Review of attachment 8957118 [details] [diff] [review]:
-----------------------------------------------------------------

Could you write an automated test for the registry read on startup?
You can copy test_start_in_safe_mode in testing/marionette/harness/marionette_harness/tests/unit/test_cli_arguments.py and add registry value tweak around the test.

::: toolkit/xre/nsAppRunner.cpp
@@ +3522,5 @@
> +    HKEY key;
> +    static const WCHAR keyName[] =
> +      L"SOFTWARE\\Policies\\Mozilla\\Firefox";
> +    if (RegOpenKeyEx(HKEY_CURRENT_USER, keyName , 0, KEY_QUERY_VALUE, &key) == ERROR_SUCCESS ||
> +        RegOpenKeyEx(HKEY_LOCAL_MACHINE, keyName , 0, KEY_QUERY_VALUE, &key) == ERROR_SUCCESS) {

HKEY_LOCAL_MACHINE should be tested before HKEY_CURRENT_USER because the machine-wide policy will take precedence over the user policy.

@@ +3528,5 @@
> +      DWORD len = sizeof(safeModeDisabled);
> +      DWORD vtype;
> +      if (RegQueryValueExW(key, L"DisableSafeMode",
> +                           0, &vtype,
> +                           reinterpret_cast<LPBYTE>(safeModeDisabled),

reinterpret_cast<LPBYTE>(&safeModeDisabled)

@@ +3531,5 @@
> +                           0, &vtype,
> +                           reinterpret_cast<LPBYTE>(safeModeDisabled),
> +                           &len) == ERROR_SUCCESS) {
> +        if (vtype == REG_DWORD || vtype == REG_QWORD) {
> +          int disabled = static_cast<int>(safeModeDisabled);

Is this extra variable and a cast necessary?

@@ +3536,5 @@
> +          if (disabled != 0) {
> +            gSafeMode = false;
> +          }
> +        }
> +      }

You leaked the registry key.
Can you implement it as an extra function that takes the root reg key as a parameter (HKEY_LOCAL_USER or HKEY_LOCAL_MACHINE)?  That way, we'll call this function only if we have detected that the keys were pressed, by adding it to the if above, like so:

> if ((GetKeyState(VK_SHIFT) & 0x8000) &&
>        !(GetKeyState(VK_CONTROL) & 0x8000) &&
>        !(GetKeyState(VK_MENU) & 0x8000) &&
>        !EnvHasValue("MOZ_DISABLE_SAFE_MODE_KEY") &&
>        !SafeModeBlockedByPolicy(HKEY_LOCAL_MACHINE) &&
>        !SafeModeBlockedByPolicy(HKEY_CURRENT_USER)) {
(Assignee)

Comment 9

a year ago
Updated patch, still work in progress.

 * manually confirmed that it works for -safe-mode command line option.
 * no automated test yet.
Attachment #8957118 - Attachment is obsolete: true
(Assignee)

Comment 10

a year ago
(In reply to Masatoshi Kimura [:emk] from comment #7)
> Could you write an automated test for the registry read on startup?
> You can copy test_start_in_safe_mode in
> testing/marionette/harness/marionette_harness/tests/unit/test_cli_arguments.
> py and add registry value tweak around the test.

Hmm, it seems impossible due to security reason. Python on MozillaBuild couldn't create sub key under "SOFTWARE\\Policies", because the current user is not permitted to write under the key even if it is under HKEY_CURRENT_USER.

----
$ python
Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:53:40) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import _winreg
>>> reg_policies = _winreg.OpenKeyEx(_winreg.HKEY_CURRENT_USER, "SOFTWARE\\Policies", 0, _winreg.KEY_WRITE)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
WindowsError: [Error 5] アクセスが拒否されました。
>>> reg_policies = _winreg.OpenKeyEx(_winreg.HKEY_CURRENT_USER, "SOFTWARE\\Mozilla", 0, _winreg.KEY_WRITE)
>>>
----

(The error says "access denied" in Japanese.)

Any good idea to bypass this restriction?
(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> Can you implement it as an extra function that takes the root reg key as a
> parameter (HKEY_LOCAL_USER or HKEY_LOCAL_MACHINE)?  That way, we'll call
> this function only if we have detected that the keys were pressed, by adding
> it to the if above, like so:
> 
> > if ((GetKeyState(VK_SHIFT) & 0x8000) &&
> >        !(GetKeyState(VK_CONTROL) & 0x8000) &&
> >        !(GetKeyState(VK_MENU) & 0x8000) &&
> >        !EnvHasValue("MOZ_DISABLE_SAFE_MODE_KEY") &&
> >        !SafeModeBlockedByPolicy(HKEY_LOCAL_MACHINE) &&
> >        !SafeModeBlockedByPolicy(HKEY_CURRENT_USER)) {

Unfortunately, I don't think this is correct.

If HKLM has DisableSafeMode=0 and HKCU has DisableSafeMode=1, safe mode will not be disabled. But if HKLM does not have DisableSafeMode and HKCU has DisableSafeMode=1, safe mode will be disabled, if I understand the Enterprise Policies spec correctly. SafeModeBlockedByPolicy should be tri-state (unset/true/false).
(In reply to YUKI "Piro" Hiroshi from comment #10)
> (In reply to Masatoshi Kimura [:emk] from comment #7)
> > Could you write an automated test for the registry read on startup?
> > You can copy test_start_in_safe_mode in
> > testing/marionette/harness/marionette_harness/tests/unit/test_cli_arguments.
> > py and add registry value tweak around the test.
> 
> Hmm, it seems impossible due to security reason. Python on MozillaBuild
> couldn't create sub key under "SOFTWARE\\Policies", because the current user
> is not permitted to write under the key even if it is under
> HKEY_CURRENT_USER.
> 
> ----
> $ python
> Python 2.7.13 (v2.7.13:a06454b1afa1, Dec 17 2016, 20:53:40) [MSC v.1500 64
> bit (AMD64)] on win32
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import _winreg
> >>> reg_policies = _winreg.OpenKeyEx(_winreg.HKEY_CURRENT_USER, "SOFTWARE\\Policies", 0, _winreg.KEY_WRITE)
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> WindowsError: [Error 5] アクセスが拒否されました。
> >>> reg_policies = _winreg.OpenKeyEx(_winreg.HKEY_CURRENT_USER, "SOFTWARE\\Mozilla", 0, _winreg.KEY_WRITE)
> >>>
> ----
> 
> (The error says "access denied" in Japanese.)
> 
> Any good idea to bypass this restriction?

Automation disables UAC and runs on an admin account, so the test will work in automation (I confirmed).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d623d78062ab9ff34971ac5585890c7e050bcd61
I also confirmed that the test works with an elevated command prompt.
(Assignee)

Comment 13

a year ago
(In reply to Masatoshi Kimura [:emk] from comment #11)
> If HKLM has DisableSafeMode=0 and HKCU has DisableSafeMode=1, safe mode will
> not be disabled. But if HKLM does not have DisableSafeMode and HKCU has
> DisableSafeMode=1, safe mode will be disabled, if I understand the
> Enterprise Policies spec correctly. SafeModeBlockedByPolicy should be
> tri-state (unset/true/false).

GPOPoliciesProvider loads policies from both HKCU and HKLM. If "DisableSafeMode" is defined under both, the value from HKCU is always overridden by the one from HKLM. (I don't know this is intentonal or not.) So, in the case "HKLM has DisableSafeMode=0 and HKCU has DisableSafeMode=1" DisableSafeMode is treated as 1.
(Assignee)

Comment 14

a year ago
(In reply to Masatoshi Kimura [:emk] from comment #12)
> Automation disables UAC and runs on an admin account, so the test will work
> in automation (I confirmed).
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d623d78062ab9ff34971ac5585890c7e050bcd61
> I also confirmed that the test works with an elevated command prompt.

Thank you for verification. Your patch looks better than mine, so can I merge it to mine?
(In reply to YUKI "Piro" Hiroshi from comment #13)
> (In reply to Masatoshi Kimura [:emk] from comment #11)
> > If HKLM has DisableSafeMode=0 and HKCU has DisableSafeMode=1, safe mode will
> > not be disabled. But if HKLM does not have DisableSafeMode and HKCU has
> > DisableSafeMode=1, safe mode will be disabled, if I understand the
> > Enterprise Policies spec correctly. SafeModeBlockedByPolicy should be
> > tri-state (unset/true/false).
> 
> GPOPoliciesProvider loads policies from both HKCU and HKLM. If
> "DisableSafeMode" is defined under both, the value from HKCU is always
> overridden by the one from HKLM. (I don't know this is intentonal or not.)
> So, in the case "HKLM has DisableSafeMode=0 and HKCU has DisableSafeMode=1"
> DisableSafeMode is treated as 1.

Really? Doesn't it contradict this comment?
https://dxr.mozilla.org/mozilla-central/rev/a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/browser/components/enterprisepolicies/EnterprisePolicies.js#435-436

But if you are right, please swap HKLM and HKCU in my comment. SafeModeBlockedByPolicy should still be tri-state anyway.
(In reply to YUKI "Piro" Hiroshi from comment #14)
> (In reply to Masatoshi Kimura [:emk] from comment #12)
> > Automation disables UAC and runs on an admin account, so the test will work
> > in automation (I confirmed).
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d623d78062ab9ff34971ac5585890c7e050bcd61
> > I also confirmed that the test works with an elevated command prompt.
> 
> Thank you for verification. Your patch looks better than mine, so can I
> merge it to mine?

Feel free to merge it. Two points:
1. The uploaded patch uses nsIWindowsRegKey to modify the registry, but I changed it to _winreg (for Python 2) or winreg (for Python 3) loccaly.
2. The patch closes HKEY manually, but it would be better to use nsAutoRegKey to make it impossible to forget closing.
(Assignee)

Comment 17

a year ago
(In reply to Masatoshi Kimura [:emk] from comment #15)
> Really? Doesn't it contradict this comment?
> https://dxr.mozilla.org/mozilla-central/rev/
> a6a32fb286fa9e5d5f6d5b3b77423ab6b96c9502/browser/components/
> enterprisepolicies/EnterprisePolicies.js#435-436

Ah, sorry, I misunderstood (and misquoted) your previous comment. I meant the thing same to yours: HKLM overrides HKCU, and you are right. In this case it should be treated as DisableSafeMode=0 from HKLM.
(In reply to YUKI "Piro" Hiroshi from comment #13)

> 
> GPOPoliciesProvider loads policies from both HKCU and HKLM. If
> "DisableSafeMode" is defined under both, the value from HKCU is always
> overridden by the one from HKLM. (I don't know this is intentonal or not.)

That is correct, and intentional. HKLM should win over HKCU for policies

> So, in the case "HKLM has DisableSafeMode=0 and HKCU has DisableSafeMode=1"
> DisableSafeMode is treated as 1.

But I think you got this inverted. In the example you gave, DisableSafeMode should be treated as 0, since it's being overriden in HKLM.

Although, we have been treating most "DisableSomething" policies set to false the same thing as unset.

I think for our purposes, as long as a DisableSafeMode=1 in HKLM enforces it, any other combination is fine. In my mind it could be simply as

disabledPolicy = DisabledByPolicy(HKLM) || DisabledByPolicy(HKCU) 

(which the version I had suggested is equivalent, I believe)


Note: I'd slightly prefer if this only happens in the if () case handling the keys, and not cancel other safe mode methods like the command line and env vars.  Running stuff from the command line should be disabled by admins in other means, and there's a fine balance between forbidding user access to it, and allowing Firefox to recover in extreme cases.  (I'm saying this because I believe safe mode is also triggered if there are various consecutive crashes, and I don't want to accidentally disable that)
Note:
(Assignee)

Comment 19

a year ago
(In reply to :Felipe Gomes (needinfo me!) from comment #18)
Thank you for comments!

> Note: I'd slightly prefer if this only happens in the if () case handling
> the keys, and not cancel other safe mode methods like the command line and
> env vars.  Running stuff from the command line should be disabled by admins
> in other means, and there's a fine balance between forbidding user access to
> it, and allowing Firefox to recover in extreme cases.  (I'm saying this
> because I believe safe mode is also triggered if there are various
> consecutive crashes, and I don't want to accidentally disable that)
> Note:

Hmm, can marionette simulate the action starting Firefox with Shift key? Otherwise I have to give up automated test for this feature.
(Assignee)

Comment 20

a year ago
And, in different viewpoint, I think disabling safe mode completely (include -safe-mode option) is reasonable on enterprise case. Because:

 * When a company security policy is enforced by an addon, it must
   not be disabled by non-admin users. "-safe-mode" and others can be backdoor.
 * System administrator can modify Windows's registry by their own
   privilege. While trouble shooting, they can disable the policy
   locally by some way (for example regedit.exe).
I did not realize that the test was using the cli option.

So we can disable that, as long as we don't disable the ENV vars case. That is used by the updater and by the crash recovery, and I think it's much harder for a non-priviliged user to bypass.
(Assignee)

Comment 22

a year ago
Updated patch.

 * This patch kills safe mode completely for all cases:
   - starting Firefox with Shift key
   - "-safe-mode" command line option
   - "MOZ_SAFE_MODE_RESTART" environment variable
 * Added automated test (with "-safe-mode" option).
 * HKCU:DisableSafeMode=1 and HKLM:DisableSafeMode=0 is
   treated as DisableSafeMode=1.
   (intentional behavior described at
    https://bugzilla.mozilla.org/show_bug.cgi?id=1440573#c18 )

How about this?
Attachment #8957410 - Attachment is obsolete: true
Attachment #8957432 - Flags: review?(felipc)
(Assignee)

Comment 23

a year ago
Sorry I posted new patch before reading new comment.

> So we can disable that, as long as we don't disable the ENV vars case.

Did you mean:

 - DisableSafeMode should disable -safe-mode CLI option
 - DisableSafeMode should not disable MOZ_SAFE_MODE_RESTART environment variable

? Then I'll update my patch.
(Assignee)

Comment 24

a year ago
Updated patch.

 * This patch kills safe mode from:
   - starting Firefox with Shift key
   - "-safe-mode" command line option (for automated tests)
 * This keeps safe mode from:
   - "MOZ_SAFE_MODE_RESTART" environment variable (for crash recovery and updater)
 * Added automated test for startup process.
 * HKCU:DisableSafeMode=1 and HKLM:DisableSafeMode=0 is
   treated as DisableSafeMode=1.
   (intentional behavior described at
    https://bugzilla.mozilla.org/show_bug.cgi?id=1440573#c18 )
Attachment #8957432 - Attachment is obsolete: true
Attachment #8957432 - Flags: review?(felipc)
Attachment #8957448 - Flags: review?(felipc)
(In reply to :Felipe Gomes (needinfo me!) from comment #18)
> Although, we have been treating most "DisableSomething" policies set to
> false the same thing as unset.

But current GPOPoliciesProvider is not implemented so. For example, if BlockAboutAddons=1 is present under HKCU and BlockAboutAddons=0 is present under HKLM, about:addons is not blocked. But if BlockAboutAddons=1 is present under HKCU and BlockAboutAddons is absent under HKLM, about:addons is blocked. Obviously false (0 in registry) is not treated the same thing as unset. Is this a bug? (I don't think so.)

I intended to match this behavior in my patch. Inconsistent behaviors will confuse enterprise administrators.
Flags: needinfo?(felipc)
Even worse, if piro's v6 patch is applied, DisableSafeMode=1 under HKCU, and DisableSafeMode~0 under HKLM, then the -safe-mode command line option is disabled, but "Restart with Add-ons Disabled..." button is enabled. Totally inconsistent.
Comment on attachment 8957448 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v6

"Restart with Add-ons Disabled..." button is present not only in about:support but also about:profiles, but apparently this patch does not take care of about:profiles. IMO only the environment variable should be the escape hatch for crash recovery etc.
(Assignee)

Comment 28

a year ago
(In reply to Masatoshi Kimura [:emk] from comment #25)
> But current GPOPoliciesProvider is not implemented so. For example, if
> BlockAboutAddons=1 is present under HKCU and BlockAboutAddons=0 is present
> under HKLM, about:addons is not blocked. But if BlockAboutAddons=1 is
> present under HKCU and BlockAboutAddons is absent under HKLM, about:addons
> is blocked. Obviously false (0 in registry) is not treated the same thing as
> unset. Is this a bug? (I don't think so.)

Originally those "BlockSomething" policies are defined with enum value [true], instad of [true, false]. To accept DWORD value "1" from registry, the enum is removed at the bug 1442418. Thus BlockSomething=0 should be treated as invalid and ignored by validator (but it not implemented yet. I think it should be tracked in another bug for the purpose.)
(Assignee)

Comment 29

a year ago
And, of course "DisableSomething" also.
(Assignee)

Comment 30

a year ago
Updated patch.

 * deactivate "Restart with Add-ons Disabled" button in "about:profiles"
Attachment #8957448 - Attachment is obsolete: true
Attachment #8957448 - Flags: review?(felipc)
Attachment #8957584 - Flags: review?(felipc)
Sorry for over-rotating here on the behavior, and thank you both for pushing to the best solution. Since the behavior is a bit complicated, I wasn't really worried about matching it perfectly. But I thought more about what Masatoshi said that it would be confusing if they are inconsistent, and I agree that it should behave as equal as possible.

So let me try to clarify the expected behavior to define the right way to implement it.

(In reply to Masatoshi Kimura [:emk] from comment #25)
> [...] For example, if
> BlockAboutAddons=1 is present under HKCU and BlockAboutAddons=0 is present
> under HKLM, about:addons is not blocked. But if BlockAboutAddons=1 is
> present under HKCU and BlockAboutAddons is absent under HKLM, about:addons
> is blocked.

What is described here is exactly intentional, and how it should behave. If there's something defined in HKLM, it should be the final answer. Otherwise, we can check HKCU. 
The way that that Yuki's patch is structured make it very easy to express this now. In almost pseudo-code:

bool SafeModeBlockedByPolicy() {
  bool value;
  if (PolicyHasRegValue(HKLM, "DisableSafeMode"), &value) {
    return value;
  }

  if (PolicyHasRegValue(HKCU, "DisableSafeMode"), &value) {
    return value;
  }

  return false;
}

So I think we should implement it basically this way. SafeModeBlockedByPolicy won't take a parameter anymore, and will handle figuring out the right value based on what's present under both roots.
Flags: needinfo?(felipc)
Comment on attachment 8957584 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v7

Review of attachment 8957584 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you! I've looked at everything else on the patch, and it looks perfect. So let's just finish it with the right behavior described on comment 25. Once that's done, I can give r+ for all the front-end parts, but I'm not a proper reviewer for using the Windows API, so we'll need to get an extra review on that by someone else.

(Is Masatoshi a reviewer for that? Basically I don't have experience to tell whether the variables and string types are correct)

(And just a nit: remove the "TODO" comment. We'll file a follow-up bug for that)
Attachment #8957584 - Flags: review?(felipc) → feedback+
(Assignee)

Comment 33

a year ago
Updated patch.

 * Policy value defined at HKLM is applied prior to
   the value defined at HKCU, even if it is "0".
 * Removed TODO comment.

This implements the behavior described at comment 25 and comment 31.
Attachment #8957584 - Attachment is obsolete: true
Attachment #8958044 - Flags: review?(felipc)
(Assignee)

Comment 34

a year ago
Renewed patch based on the latest mozilla-central.

 * Policy value defined at HKLM is applied prior to
   the value defined at HKCU, even if it is "0".
 * Removed TODO comment.

This implements the behavior described at comment 25 and comment 31.
Attachment #8958044 - Attachment is obsolete: true
Attachment #8958044 - Flags: review?(felipc)
Attachment #8958069 - Flags: review?(felipc)
Comment on attachment 8958069 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v8.1

Review of attachment 8958069 [details] [diff] [review]:
-----------------------------------------------------------------

Jim, can you review the Windows API and strings usage in the nsAppRunner.cpp portion of this patch?
Attachment #8958069 - Flags: review?(jmathies)
Comment on attachment 8958069 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v8.1

Review of attachment 8958069 [details] [diff] [review]:
-----------------------------------------------------------------

The patch uses `Services.policies` in toolkit code.. Previously I was advising to check for it (because it wouldn't exist in Thunderbird or Android), but this was getting very confusing and error-prone, so bug 1443829 is fixing that.

In short: this is perfect, but it needs to land only after bug 1443829 lands. I'll handle the landing to wait for Jim's review and for bug 1443829.
Attachment #8958069 - Flags: review?(felipc) → review+
Depends on: 1443829
Attachment #8958069 - Flags: review?(jmathies) → review+

Comment 37

a year ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83dffebb1536
Policy: Disable safe mode. r=jimm,felipe
Backed out for bc failures on browser_policy_disable_safemode.js

https://hg.mozilla.org/integration/mozilla-inbound/rev/a36f91eb3bbdc129e440489901e81b2e90620c46

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=83dffebb15369683ef0426c43d68c593f5d2b618&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=168385776&repo=mozilla-inbound&lineNumber=2774 

 TEST-PASS | browser/components/enterprisepolicies/tests/browser/browser_policy_disable_safemode.js | The `Restart with Add-ons Disabled...` item should be disabled - 
[task 2018-03-16T02:37:02.556Z] 02:37:02     INFO - Leaving test bound test_help_menu
[task 2018-03-16T02:37:02.557Z] 02:37:02     INFO - Entering test bound test_safemode_from_about_support
[task 2018-03-16T02:37:02.558Z] 02:37:02     INFO - Buffered messages finished
[task 2018-03-16T02:37:02.560Z] 02:37:02     INFO - TEST-UNEXPECTED-FAIL | browser/components/enterprisepolicies/tests/browser/browser_policy_disable_safemode.js | The `Restart with Add-ons Disabled...` button should be disabled - null == "true" - 
[task 2018-03-16T02:37:02.560Z] 02:37:02     INFO - Stack trace:
[task 2018-03-16T02:37:02.561Z] 02:37:02     INFO - resource://testing-common/content-task.js line 50 > eval:null:5
[task 2018-03-16T02:37:02.562Z] 02:37:02     INFO - Leaving test bound test_safemode_from_about_support
[task 2018-03-16T02:37:02.563Z] 02:37:02     INFO - Entering test bound test_safemode_from_about_profiles
Flags: needinfo?(yuki)
Since the failure happened on a debug build, I believe what happened is that the test used waitForLoad = false, so the check in test_safemode_from_about_support probably ran before populateActionBox() was called.

I believe this will fix it. However, I noticed another small prob with the patch. This safemode button is #ifndef ANDROID, so the code added to populateActionBox() could throw (in practice it won't because the policy engine will never say isAllowed = false there, at least not now)

And I totally forgot that I marked this as a dependency on bug 1443829. I'll fix these issues and re-land
Flags: needinfo?(yuki)
(Assignee)

Comment 40

a year ago
Now I'm trying to update the previous patch to pass tests. Is it needless?

Comment 41

a year ago
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd250ee025a5
Policy: Disable safe mode. r=jimm,felipe
Oh, I did not see your comment before, and ended up doing it :)

But answering your question: yeah, unless there's a specific reason to use waitForLoad=false, it should always be left as true (the default).

What happens is that debug builds are much slower, so the chances are higher there that the test will continue to run before the page has fully loaded (and the page itself has a "load" event listener which would be racing there)
(Assignee)

Comment 43

a year ago
Thank you for updating the patch, and I've realized what I did (I wrongly copied codes from other tests carelessly.)
The updated patch is quite same to my local version :)

Comment 44

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd250ee025a5
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
[Tracking Requested - why for this release]:
Enterprise Policy for ESR 60
tracking-firefox60: --- → ?
(Assignee)

Comment 46

a year ago
Posted patch Patch for Firefox 60 beta (obsolete) — Splinter Review
Patch based on the commit on mozilla-central https://hg.mozilla.org/mozilla-central/rev/cd250ee025a5 for mozilla-beta. "r+" is carried over the original commit on mozilla-central.

Note that this will conflict to the patch at:
https://bugzilla.mozilla.org/show_bug.cgi?id=1440574#c31
because both patches change the beginning section of the function "buildHelpMenu()".

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Missing enterprise policy around disabling safe mode to bypass system restrictions defined by the system administrator.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: This is well tested by an automated test.
[String changes made/needed]: None.
Attachment #8960095 - Flags: review+
Attachment #8960095 - Flags: approval-mozilla-beta?
tracking-firefox60: ? → +
(Assignee)

Comment 47

a year ago
Posted patch Patch for Firefox 60 beta v2 (obsolete) — Splinter Review
Updated patch with clearing "disabled" attribute for enabled case.
Test result on the try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b30a57f50719f99c84c5c69f2d736397941df91

Note that this patch will conflict to the patch at:
https://bugzilla.mozilla.org/show_bug.cgi?id=1440574#c35
because both patches change the beginning section of the function "buildHelpMenu()".

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Missing enterprise policy around disabling safe mode to bypass system restrictions defined by the system administrator.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: This is well tested by an automated test.
[String changes made/needed]: None.
Attachment #8960095 - Attachment is obsolete: true
Attachment #8960095 - Flags: approval-mozilla-beta?
Attachment #8960547 - Flags: review?(felipc)
Attachment #8960547 - Flags: approval-mozilla-beta?
We tested this on latest nightly with JSON format and it is verified as fixed. It will also be retested when the admx format implemented. 
The JSON format policy will only remove the "restart in safe mode" options from the help menu and from about:support page. 
It does not disable starting a safe-mode from a terminal, which will be implemented with ADMX format.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
Tested with admx file format as well and it is verified as fixed. With this policy, safe-mode can also be disabled for a terminal start.
status-firefox61: verified → fixed
Comment on attachment 8958069 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v8.1

Review of attachment 8958069 [details] [diff] [review]:
-----------------------------------------------------------------

I wish that for new tests under testing/marionette the peers of that component are getting asked for review. Basically the unit tests folder is not the right place to get tests like those added. :/ It should really be placed under browser/.

Further some additional comments which I really want to see fixed soon. Feel free to file a new bug for that.

::: testing/marionette/harness/marionette_harness/tests/unit/test_cli_arguments.py
@@ +46,4 @@
>  
> +    def test_safe_mode_blocked_by_policy(self):
> +        if platform.system() != 'Windows':
> +            return

This test should have been marked as skipped for platforms other than Windows. Please do not just return.

@@ +69,5 @@
> +        winreg.CloseKey(reg_firefox)
> +        winreg.DeleteKey(reg_mozilla, "Firefox")
> +        winreg.CloseKey(reg_mozilla)
> +        winreg.DeleteKey(reg_policies, "Mozilla")
> +        winreg.CloseKey(reg_policies)

I do not see that those registry entries will ever be removed if the test fails in the lines before. So it should have been wrapped into a try/finally.
Yuki, and Felipe please see my last comment. Thanks!
Flags: needinfo?(yuki)
Flags: needinfo?(felipc)
Comment on attachment 8960547 [details] [diff] [review]
Patch for Firefox 60 beta v2

The change from the original patch here is not necessary, for reasons explained in bug 1440574 comment 36, so I'll mark this patch as not needed, and move the approval request to the first patch.
Attachment #8960547 - Attachment is obsolete: true
Attachment #8960547 - Flags: review?(felipc)
Attachment #8960547 - Flags: approval-mozilla-beta?
Comment on attachment 8958069 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v8.1

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policy to Disable Safe Mode
[User impact if declined]: Missing policy
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: QA has verified it
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Limited to the usage of this policy
[String changes made/needed]: none
Attachment #8958069 - Flags: approval-mozilla-beta?
(In reply to Henrik Skupin (:whimboo) from comment #51)
> Yuki, and Felipe please see my last comment. Thanks!

Hi, thanks for letting us now. I filed bug 1448197 to clean that up.
Flags: needinfo?(yuki)
Flags: needinfo?(felipc)
Depends on: 1448197
Comment on attachment 8958069 [details] [diff] [review]
Patch to implement policy "DisableSafeMode" v8.1

enterprise policy for safe mode, beta60+
Attachment #8958069 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 56

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8bb565a35f45
status-firefox60: affected → fixed
Flags: in-testsuite+
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-firefox60: fixed → verified
status-firefox61: fixed → verified

Updated

11 months ago
Depends on: 1455110
You need to log in before you can comment on or make changes to this bug.