If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Disable downloads and extension installs in guest mode

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 36
All
Android
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox34 ?, firefox35 affected, firefox36 verified, fennec34+)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
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...

Comment 1

3 years ago
I think this makes sense.
tracking-fennec: --- → ?

Updated

3 years ago
Assignee: nobody → wjohnston
tracking-fennec: ? → 34+
(Assignee)

Comment 2

3 years ago
Created attachment 8471340 [details] [diff] [review]
Patch

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)
(Assignee)

Updated

3 years ago
Blocks: 815682
(Assignee)

Comment 3

3 years ago
Created attachment 8471341 [details] [diff] [review]
Patch 2 - Disable downloads
Attachment #8471341 - Flags: review?(margaret.leibovic)

Comment 4

3 years ago
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 5

3 years ago
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 6

3 years ago
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

Comment 8

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

3 years ago
Flags: qe-verify+
(Assignee)

Updated

3 years ago
Blocks: 1058149
(Assignee)

Updated

3 years ago
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
status-firefox34: --- → verified
status-firefox35: --- → 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?
(Assignee)

Comment 18

3 years ago
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
status-firefox34: verified → ?
status-firefox35: verified → affected
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
(Assignee)

Comment 21

3 years ago
Created attachment 8502066 [details] [diff] [review]
Patch

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.
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/9901d2c678c7
https://hg.mozilla.org/mozilla-central/rev/9901d2c678c7
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 34 → Firefox 36

Comment 26

3 years ago
Verified as fixed in build 36.0a1 (2014-11-19);
Device: Nexus 4 (Android 4.4.4).
status-firefox36: --- → verified

Updated

3 years ago
Blocks: 1134553
You need to log in before you can comment on or make changes to this bug.