1797806 - New private browsing mode doesn't honor the DisablePrivateBrowsing policy

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect, P3)

Description

[Mike Kaply [:mkaply]]

The newly added private_browsing.exe works even when you have private browsing disabled via policy.

It's not actually in private mode (you get history when you restart), but the UI shows that you are in private mode.

We should show the regular UI if private browsing has been disabled.

Comment 1

[[e-mail] (:bhearsum)]

I have a feeling firefox.exe -private-browsing will have the same behaviour. I'll look into this as soon as possible - it's arguably a privacy issue as it looks like we shouldn't save history, but we will.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to [e-mail] (:bhearsum) from comment #1)

I have a feeling firefox.exe -private-browsing will have the same behaviour. I'll look into this as soon as possible - it's arguably a privacy issue as it looks like we shouldn't save history, but we will.

Indeed -- firefox.exe -private-browsing suffers from the same issue -- so this has been an issue for awhile, it's just been highlighted by the fact that we now create Private Browsing shortcuts. Now...what do we do about it?

At the very least, I think we ought to do the following when this policy is enabled:

• Ensure we don't create Private Browsing shortcuts at runtime (which I suspect we do a lot for Enterprise installs, if they have removed the installer created shortcuts)
• Ensure we never theme windows as Private
• Ensure we never use the Private Browsing AUMID (to avoid a separate taskbar icon)

Doing these things should cover most cases. Unfortunately, we cannot get rid of the private_browsing.exe wrapper, so there's still some potentially for users to find it and run it. I think once the above issues are fixed it might be a livable situation though -- Firefox will still launch, but it will look like a regular browsing window. It will attempt to open about:privatebrowsing, which will show the "Your organization has blocked access..." UI -- which seems like the right thing to do? (The only alternative I can come up with is trying to load about:newtab instead - which I'd argue is worse because it may mislead users into thinking that they're in a private session.)

(I'm doing my best to avoid any ideas that would require private_browsing.exe to read policies or prefs, as it currently has absolutely no access to Gecko code.)

Mike, do you have any thoughts on the above?

Comment 3

[Mike Kaply [:mkaply]]

I think that sounds like a great plan.

Comment 4

[[e-mail] (:bhearsum)]

Paul, adding you as well in case you have any thoughts, IIRC you worked on some of the new window decorations.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: Bug 1797806: don't do private browsing specific things when private browsing is disabled r?mhowell!

This is not at all done -- there's still likely other code that needs updating, and we should definitely have some tests that verify the state of the UI when this is disabled by policy.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bfc380f167f3
don't do private browsing specific things when private browsing is disabled r=mhowell

Comment 7

[Donal Meehan [:dmeehan]]

:bhearsum, I was tracking this for 107 though as you point out in comment 2 this is not new.
Seems like this might be too late/risky to consider an uplift to 107
Though this is marked as an S1, so would like to get your input?
Final 107 beta builds tomorrow, hence reaching out now to give you a chance to respond even though it just landed on autoland.

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Donal Meehan [:dmeehan] from comment #7)

:bhearsum, I was tracking this for 107 though as you point out in comment 2 this is not new.
Seems like this might be too late/risky to consider an uplift to 107
Though this is marked as an S1, so would like to get your input?
Final 107 beta builds tomorrow, hence reaching out now to give you a chance to respond even though it just landed on autoland.

It's not new, technically, but it is much more likely to happen beginning with 106. Prior to that, someone would've had to manually launch with -private-window (or set up a shortcut with it) to hit this.

My gut says this is low risk - but it's difficult to be certain, mostly because of the BrowserContentHandler.jsm change, which is somewhat less targeted than the other parts. I think it's okay if this waits for 108.

Comment 9

[Donal Meehan [:dmeehan]]

(In reply to [e-mail] (:bhearsum) from comment #8)

It's not new, technically, but it is much more likely to happen beginning with 106. Prior to that, someone would've had to manually launch with -private-window (or set up a shortcut with it) to hit this.

My gut says this is low risk - but it's difficult to be certain, mostly because of the BrowserContentHandler.jsm change, which is somewhat less targeted than the other parts. I think it's okay if this waits for 108.

Thanks for the info. I will set 107 to wontfix for now. Give this time on Nightly and ride the train with 108.

If anyone disagrees, we can consider a ride-along in a dot release or in a planned dot release for 107.

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/bfc380f167f3

Comment 11

[[e-mail] (:bhearsum)]

In the review, Molly pointed out that we still may have private browsing shortcuts (they're unconditionally created by the installer) when private browsing is disbled. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1798743 to make it possible to disable that.

Comment 12

[Narcis Beleuzu [:NarcisB]]

Backed out for showing every browser window as private one (bug 1798819)

Backout link: https://hg.mozilla.org/mozilla-central/rev/7850363c75471ee017dc913c7df032ea7da8c680

Comment 14

[[e-mail] (:bhearsum)]

While looking into a fix for the awful issue the first landing of this caused, I discovered that policies aren't currently loaded before the first window is painted. They get loaded in DoStartup while the first window is painted by earlyBlankFirstPaint, which is called (very much indirectly) by AppStartup - coincidentally just a couple lines up from DoStartup.

With that in mind, the fix for this is much less clear. I imagine we could move policy loading up a bit - but that will cause I/O before the first window is painted (something we try to avoid).

I'm tempted to just punt on this specific issue for now, as once https://bugzilla.mozilla.org/show_bug.cgi?id=1798743 is fixed, the only ways to trigger it will be to dig into the install directory and run private_browsing.exe, or run firefox.exe -private-window manually.

Mike, do you have any thoughts on a fix for this, or fixing the other bug and not worry about this (at least, for now)?

Comment 15

[Mike Kaply [:mkaply]]

Mike, do you have any thoughts on a fix for this, or fixing the other bug and not worry about this (at least, for now)?

I thought we needed this fix because Firefox would still recreate the icon?

Since this is Windows only, we could just check for the registry entry explicitly. It wouldn't solve it for policies.json, but we have other things that only work via GPO/registry.

https://searchfox.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#2035

Comment 16

[[e-mail] (:bhearsum)]

(In reply to Mike Kaply [:mkaply] from comment #15)

Mike, do you have any thoughts on a fix for this, or fixing the other bug and not worry about this (at least, for now)?

I thought we needed this fix because Firefox would still recreate the icon?

Since this is Windows only, we could just check for the registry entry explicitly. It wouldn't solve it for policies.json, but we have other things that only work via GPO/registry.

https://searchfox.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#2035

Sorry - yes, we do need that part for sure (and there's no blockers to fixing that). I was referring more to the extra safeguards around window badging if someone does run with -private-window or private_browsing.exe even though a shortcut doesn't exist. Eg: is the lack of shortcut enough to make this a much less pressing issue?

Comment 17

[Mike Kaply [:mkaply]]

Ah, yes. I think lack of shortcut makes it not an issue. But I think we could do the registry check as a safeguard.

Comment 18

[[e-mail] (:bhearsum)]

(In reply to Mike Kaply [:mkaply] from comment #17)

Ah, yes. I think lack of shortcut makes it not an issue. But I think we could do the registry check as a safeguard.

I'm going to put this off for now, given this.

I'm also not sure if we want to do a registry check like this so early in startup - I believe it also counts as I/O? (We obviously could, but I know we try to avoid early I/O as much as possible.)

Comment 19

[Donal Meehan [:dmeehan]]

Removing release tracking, see comments 15 - 18

Comment 20

[Mike Kaply [:mkaply]]

I thought there was still some other work you were going to do here independent of the shortcut?

Comment 21

[[e-mail] (:bhearsum)]

(In reply to Mike Kaply [:mkaply] from comment #20)

I thought there was still some other work you were going to do here independent of the shortcut?

Yes, ugh, sorry. I've spun this part off into https://bugzilla.mozilla.org/show_bug.cgi?id=1800140 since it's clearly fixable now.

Comment 22

[[e-mail] (:bhearsum)]

Just to recap the situation here, since someone else will probably pick this up:

• I spun off the runtime shortcut creation into https://bugzilla.mozilla.org/show_bug.cgi?id=1800140, and an option to disable the shortcut in the installer into https://bugzilla.mozilla.org/show_bug.cgi?id=1798743 -- both of which are now fixed.
• This bug remains to handle ensuring that the Private Browsing taskbar icon is never used if Private Browsing is disabled by policy. The bulk of the existing patch here should be fine -- but the IsAllowed check needs to be augmented, as policies are not loaded early enough to work for the first window. If the enterprise policies are uninitialized, we need to check the registry directly for the policy. If policies have been initialized we should just use isAllowed to do so (to avoid unnecessary I/O).

Comment 23

[Nipun Shukla]

Attachment: Bug 1797806 - Implemented startup registry check for DisablePrivateBrowsing while EnterprisePolicies are uninitialized r=bhearsum,nalexander

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d18ac0161ca
Implemented startup registry check for DisablePrivateBrowsing while EnterprisePolicies are uninitialized r=bhearsum,nalexander,mkaply

Comment 25

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/4d18ac0161ca

Comment 26

[Mike Kaply [:mkaply]]

Is this easy to put on the ESR? I realized we probably should have done that.