Closed Bug 1046941 Opened 5 years ago Closed 5 years ago

Disable downloads and extension installs in guest mode

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 --- ?
firefox35 --- affected
firefox36 --- verified
fennec 34+ ---

People

(Reporter: wesj, Assigned: wesj)

References

Details

Attachments

(3 files)

With the lockscreen widget, I don't think if we should allow you to download anything (or install extensions that could do malicious things) when you're in Guest mode.

I'm not sure if this lockscreen guest mode is different than normal guest mode or not in that sense though...
I think this makes sense.
tracking-fennec: --- → ?
Assignee: nobody → wjohnston
tracking-fennec: ? → 34+
Attached patch PatchSplinter Review
This is the same thing we do for webapps.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/WebappRT.js#28
Attachment #8471340 - Flags: review?(margaret.leibovic)
Attachment #8471341 - Flags: review?(margaret.leibovic)
Comment on attachment 8471340 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/browser.js
@@ +448,5 @@
>  
>      if (this._startupStatus)
>        this.onAppUpdated();
>  
> +    if (isGuest) {

I think this needs to be `this.isGuest`.

Did you test this? :)

@@ +452,5 @@
> +    if (isGuest) {
> +      // Disable extension installs
> +      Services.prefs.setIntPref("extensions.enabledScopes", 1);
> +      Services.prefs.setIntPref("extensions.autoDisableScopes", 1);
> +      Services.prefs.setBoolPref("xpinstall.enabled", false);

I suppose it's fine to have prefs that persist, since they'll be written to the guest profile we're throwing away anyway.

Do we just create a new regular profile for guest mode? Is there any special logic specific to creating guest profiles? If so, maybe it would be better to put this closer to that logic, but I'm not sure if that's possible.
Attachment #8471340 - Flags: review?(margaret.leibovic) → feedback+
Comment on attachment 8471341 [details] [diff] [review]
Patch 2 - Disable downloads

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

Looks good to me. Make sure you get UX review on this toast behavior/string.

::: mobile/android/chrome/content/browser.js
@@ +448,5 @@
>  
>      if (this._startupStatus)
>        this.onAppUpdated();
>  
> +    if (this.isGuest) {

Oh, heh, you fixed it here.
Attachment #8471341 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8471340 [details] [diff] [review]
Patch

Bumping to r+, since you fixed the issue in the next patch. You can just fold them together when you land.
Attachment #8471340 - Flags: feedback+ → review+
If rely on prefs to block extension installs, you'd better make sure I can't go to about:config in Guest Mode… otherwise I'm just going to turn off your blocks and install my malicious add-on.

I noticed in Bug 1052254 that we should probably have some guest mode short-circuiting in the file analysis code in HelperAppDialog. Linking these two bugs.
Blocks: 1052254
Status: NEW → ASSIGNED
OS: Linux → Android
Hardware: x86_64 → All
Summary: Consider disabling downloads/extensions in guest mode → Disable downloads and extension installs in guest mode
(In reply to (Away Aug 14-24) Richard Newman [:rnewman] from comment #7)
> If rely on prefs to block extension installs, you'd better make sure I can't
> go to about:config in Guest Mode… otherwise I'm just going to turn off your
> blocks and install my malicious add-on.

That is a very good point. Do we have logic in place to block changing prefs in a guest profile? Can we block about:config? We also wouldn't want a malicious person to be able to turn on remote debugging, although hopefully in this lock screen mode the device would block USB debugging.
(In reply to :Margaret Leibovic from comment #8)
> Can we block about:config?

That's why I suggested blocking about: pages in Bug 1052254.
https://hg.mozilla.org/integration/fx-team/rev/12e582732409

I'm going to file a follow up for doing this extension blocking more robustly (and with some better error reporting).
https://hg.mozilla.org/mozilla-central/rev/12e582732409
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify+
Blocks: 1058149
Blocks: 1065752
Verified as fixed in builds:
Firefox for Android 34.0a2 (2014-09-29)
Firefox for Android 35.a01 (2014-09-29)

Device: Nexus 4 Android 4.4.4
Status: RESOLVED → VERIFIED
I just installed LastPass using the lockscreen/guest-mode. From the homescreen, I clicked on Add-ons, then installed LastPass. The browser restarted, and I went to about:support and saw that LastPass was installed and enabled.
Flags: needinfo?(wjohnston)
But then when I exit guest mode, the extension is no longer listed. Are we sandboxing extension installs?
The guest profile is deleted when you exit guest mode, along with its add-ons.
(In reply to Richard Newman [:rnewman] from comment #15)
> The guest profile is deleted when you exit guest mode, along with its
> add-ons.

So the bug summary (disable ... extension installs in Guest Mode) is a bit misleading. They're not truly disabled, they just don't live past exiting guest mode. Was this intentional, or did we want to actually block installation of them in guest mode too?
They're *supposed* to be blocked, so this bug is not resolved. I just reproduced in current Nightly 35.

Wes, would you rather reopen this or file another bug?
Reopen is fine. They should be blocked. Something is up... maybe RestrictedProfiles broke this.
Flags: needinfo?(wjohnston)
See also Bug 1077706, which would help avoid the temptation!
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
On Aurora (2014-10-05), when I try to download a file in gust mode 'Downloads are disabled in guest mode' toast notification is displayed. When I tap on 'Add to Firefox' for installing an add-on, nothing happens, the add-on is not downloaded.
Nightly (2014-09-30) has the same behavior. 

Regression window:
Last good: 2014-09-30
First bad: 2014-10-01
Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7c24470b6b3a&tochange=14665b1de5ee
Attached patch PatchSplinter Review
Spelling error. I also renamed the java restriction so that we were at least consistent between the two. Happy to go the other way if you want.
Attachment #8502066 - Flags: review?(mark.finkle)
Attachment #8502066 - Flags: review?(mark.finkle) → review+
Comment on attachment 8502066 [details] [diff] [review]
Patch

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

::: mobile/android/chrome/content/browser.js
@@ +464,5 @@
>  
>      if (this._startupStatus)
>        this.onAppUpdated();
>  
> +    if (!ParentalControls.isAllowed(ParentalControls.INSTALL_EXTENSION)) {

Should http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/RestrictedProfiles.java?rev=be045ef85f3f#147 fail if it is passed 0 (or whatever the undefined ENUM value is)? Line 152 looks like it silently says that it is allowed.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #22)

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1080304 for this.
https://hg.mozilla.org/mozilla-central/rev/9901d2c678c7
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 34 → Firefox 36
Verified as fixed in build 36.0a1 (2014-11-19);
Device: Nexus 4 (Android 4.4.4).
Blocks: 1134553
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.