891116 - Click-to-play permissions set in private browsing stay around after exiting private browsing (privacy leak)

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

STR:
Go to Youtube.com
Set a site permission for Flash to "Always Ask" to be run
Enter private browsing
Try to play a video, note that you have to choose either "Allow Now" or "Allow and Remember"
Choose one of the options
Exit private browsing mode

Expected:
Permissions changes during pb mode will not be remembered

Actual:
They are remembered and persisted.

This is a regression from the previous CTP implementation in that there is no "run once" setting. Both options will persist now.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

See https://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1768 for how some permission prompts prevent this.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Apparently we currently can't store permissions temporarily and have the original ones restored after the private window or the session ended.
This means that, with the current architecture, the only thing we can do without leaking data into the rest of the session is activating plugins in the current page.

I think the viable short-term solution is to block everything except the "allow now" action for blocked/ask-to-activate plugins. 
This will leak into the rest of the session and reset per-site ask-to-activate settings, but without it we don't always have usable plugins in private mode.

It seems to me like the reasonable long-term solution would be a separate permission manager per private-browsing window - although i'm unaware if there are technical hurdles.

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> It seems to me like the reasonable long-term solution would be a separate
> permission manager per private-browsing window - although i'm unaware if
> there are technical hurdles.

Alright, via jdm on IRC:
* the permission manager was historically a singleton
* fixing it didn't make it in the private browsing changes (time constraints, cross-tree breakage potentials, ...)
* fixing it is not a high-priority right now

Need to follow up with jdm on possible opt-in, per-PB-window permission managers, for possibly allowing progressive adoption as an alternative path.

For now though i'll have to go with the short-term solution.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

This is the bad part about it: If you set Allow Now we will grant the permission for 60 minutes, but the permission will still be in the database even after those 60 minutes, it will just be disregarded. This is the bad part of the privacy leak.

Comment 5

[Benjamin Smedberg]

Which database are you talking about? It will be in memory, but it is not saved on disk (allow now is a session-only permission).

Comment 6

[Georg Fritzsche [:gfritzsche]]

I don't see a nicer option without rewriting bigger parts of this handling. And if we only do "run once" instead of the "allow now" paradigm, we'd be back to the very basic, and in parts website-breaking (youtube playlist, timing out on plugin activation, ...), previous behavior.

Note that from comment 1 and comment 3 other session-oriented permissions also leak into the current session, so i think we should find a decent compromise now and put further resources in fixing that underlying issue.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Which database are you talking about? It will be in memory, but it is not
> saved on disk (allow now is a session-only permission).

Sorry, this was a misunderstanding on my behalf.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Disable parts of the CtP UI for private browsing

Finally sorted this out today after having figured some misconceptions with the testing...

This implements what i suggested in comment 2 as a temporary measure: 
Block everything except the "allow now" action for blocked/ask-to-activate plugins

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Tests

This provides test-coverage for the currently expected CtP UI behavior in private browsing windows.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=aece4537830d

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 776383 [details] [diff] [review]
Tests

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

::: browser/base/content/test/browser_private_clicktoplay.js
@@ +100,5 @@
> +  ok(!objLoadingContent.activated, "Test 2a, Plugin should not be activated");
> +
> +  // Simulate clicking the "Allow Now" button.
> +  popupNotification.reshow();
> +  PopupNotifications.panel.firstChild._secondaryButton.click();

This should be _primaryButton right? Because _secondaryButton should be hidden.

@@ +121,5 @@
> +
> +  // Check the button status
> +  popupNotification.reshow();
> +  let buttonContainer = gPrivateWindow.PopupNotifications.panel.firstChild._buttonContainer;
> +  ok(buttonContainer.hidden, "Test 2c, Activated plugin in a private window should not have visible buttons");

What does it look like with no visible buttons? What can the user do here? Can they stop allowing it for the session?

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: CtP notification for 1 plugin, no buttons

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Jared Wein [:jaws] from comment #11)
> Comment on attachment 776383 [details] [diff] [review]
> Tests
> 
> Review of attachment 776383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/browser_private_clicktoplay.js
> @@ +100,5 @@
> > +  ok(!objLoadingContent.activated, "Test 2a, Plugin should not be activated");
> > +
> > +  // Simulate clicking the "Allow Now" button.
> > +  popupNotification.reshow();
> > +  PopupNotifications.panel.firstChild._secondaryButton.click();
> 
> This should be _primaryButton right? Because _secondaryButton should be
> hidden.

That is in the standard test window. The idea here is to use the standard (non-PB) window to set the plugin permissions and then test that the UI is as expected in a private window.

> 
> @@ +121,5 @@
> > +
> > +  // Check the button status
> > +  popupNotification.reshow();
> > +  let buttonContainer = gPrivateWindow.PopupNotifications.panel.firstChild._buttonContainer;
> > +  ok(buttonContainer.hidden, "Test 2c, Activated plugin in a private window should not have visible buttons");
> 
> What does it look like with no visible buttons? What can the user do here?
> Can they stop allowing it for the session?

See the attached image for how it looks - there is nothing for the user to do, it can't be blocked from the notification.

Alternatively we could not hide "block" and limit it to the session. But this would mean more leaking (i.e. this kills previously set "allow and remember" permission, leaving traces after the session) and some special-cased handling for the actions for private windows.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Larissa, what do you think about an in-actionable doorhanger?

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Jared Wein [:jaws] from comment #14)
> Larissa, what do you think about an in-actionable doorhanger?

To shortly summarize the above: this is supposed to be a temporary fix and would only be seen in private browsing windows (background in comment 2).

Comment 16

[Larissa Co [:lco]]

(In reply to Jared Wein [:jaws] from comment #14)
> Larissa, what do you think about an in-actionable doorhanger?

What is the behavior again? exactly the same as "allow now" (60 min timer), except that the user doesn't get to choose? And does the 60 min timer override the user's site preferences when he leaves private browsing mode still?

In the short term, I like the suggestion that we disable everything except for "Allow Now". However, I'd like to keep the two-click design (maybe we even remove the "allow and Remember" button from the doorhanger)for consistency and security.

In general, it feels weird to me to have a notification-style doorhanger that isn't actionable. For one, it's extraneous for in-content plugins because the user will see his content load anyway; he doesn't need to see another notification. 

I'm also having misgivings about the fact that an inactionable doorhanger doesn't protect the user from click-jacking. It's even worse than the old CtP because ALL the plugins on the page load, not just one.

Also, what about the case where the plugin is invisible? The user needs to be able to confirm his choice.

---

On a different note, if the user has selected "allow and remember" in normal mode, that carries over to private browsing mode, right?

Comment 17

[Georg Fritzsche [:gfritzsche]]

(In reply to Larissa Co [:lco] from comment #16)
> On a different note, if the user has selected "allow and remember" in normal mode,
> that carries over to private browsing mode, right?

All the CtP choices carry over to private browsing mode.

> What is the behavior again? exactly the same as "allow now" (60 min timer),
> except that the user doesn't get to choose? And does the 60 min timer
> override the user's site preferences when he leaves private browsing mode
> still?

This still has the same behavior, including two-click, as otherwise. Only in private browsing windows now certain buttons or list entries are not available. I hope this makes the available options clear:
https://docs.google.com/spreadsheet/ccc?key=0AhKmHVpmAzfcdFV5MTUzRlNmRDI1UE15SUdPbG5BM0E&usp=sharing

So if the plugin(s) were already allowed for that site before private browsing ("allow now" or "allow and remember"), no user choice would be available and the plugin(s) stay activated.
If the plugin(s) were blocked before private browsing mode, the only available choice would be "allow now".

This shouldn't change anything regarding click-jacking or invisible plugins.

Comment 18

[Larissa Co [:lco]]

ok, thanks for clarifying. The screenshot was misleading to me :) I'm ok with this design.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 776383 [details] [diff] [review]
Tests

So i think this is back on for review.

Comment 20

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 776383 [details] [diff] [review]
Tests

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

I still wish we wouldn't show an inactionable doorhanger, but I don't know how we could do that without fixing the root cause of permissions in private windows.

Larissa, should we at least add some text to say that these buttons are missing because this is a private window?

Comment 21

[Larissa Co [:lco]]

(In reply to Jared Wein [:jaws] from comment #20)
> Comment on attachment 776383 [details] [diff] [review]
> Tests
> 
> Review of attachment 776383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still wish we wouldn't show an inactionable doorhanger, but I don't know
> how we could do that without fixing the root cause of permissions in private
> windows.
> 
I don't understand. i thought gfritzsche said that it would behave exactly the same as a door hanger in regular browsing mode, but only have one button for the user to allow now? this is what I assumed from comment 17

Comment 22

[Georg Fritzsche [:gfritzsche]]

(In reply to Larissa Co [:lco] from comment #21)
> I don't understand. i thought gfritzsche said that it would behave exactly
> the same as a door hanger in regular browsing mode, but only have one button
> for the user to allow now? this is what I assumed from comment 17

As mentioned, there are no actions/buttons available when the plugin(s) are already allowed.

Comment 23

[Larissa Co [:lco]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #22)

> As mentioned, there are no actions/buttons available when the plugin(s) are
> already allowed.

Ok, I think I finally get it now. Sorry for being this slow. Maybe we can add a link when plugins are already enabled so that it looks like: 

"<Plugin X> is enabled on <site.com>. [How to disable the plugin]"

The link would go to the Click-to-play SUMO page link, preferably to an anchor where we explain why they can't disable the plugin in private browsing mode. In all likelihood, there'll be a small number of people who'll want to disable the plugin anyway.

Other suggestions for link phrases are:
"Disabling the plugin"
"How to disable"

Comment 24

[JDev]

This also occurs when running a blocked plugin in private browsing.  Selecting "Activate Plugin" & "Allow Now", closing private browser window then opening a new private browsing window the "Allow Now" pages persist.

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Latest screenshot

I have rebased the patches and it seems that this last remaining addition is already there:

(In reply to Larissa Co [:lco] from comment #23)
> Ok, I think I finally get it now. Sorry for being this slow. Maybe we can
> add a link when plugins are already enabled so that it looks like: 
> 
> "<Plugin X> is enabled on <site.com>. [How to disable the plugin]"
> 
> The link would go to the Click-to-play SUMO page link, preferably to an
> anchor where we explain why they can't disable the plugin in private
> browsing mode. In all likelihood, there'll be a small number of people
> who'll want to disable the plugin anyway.
> 
> Other suggestions for link phrases are:
> "Disabling the plugin"
> "How to disable"

As you can see in the screenshot, the link phrase is "Learn more..." and it takes you to the CTP SUMO page. Is this phrase OK, or do we want to modify it in this particular case? Personally, I think it works as it is.

If we do want to change the text, will we add a new section to the existing page or create a new one? It seems rather focused on the opposite problem of how to activate the plugin:

https://support.mozilla.org/kb/why-do-i-have-click-activate-plugins

Comment 26

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Disable most privacy-leaking click-to-play UI for private browsing.

This is the combined and rebased patch. I had to fix a few issues with the test and make it work in e10s, but nothing substantial enough to require a new review. We discussed the question from comment 25 in our daily meeting and the consensus was to keep the current text and revisit it in the future if we deem it necessary.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8652961 [details] [diff] [review]
Disable most privacy-leaking click-to-play UI for private browsing.

Carrying over the r+.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b99b616a37ed

Comment 28

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/14733eceb2ed

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14733eceb2ed

Comment 30

[Marco Mucci [:MarcoM]]

Hi Panos, can you set this bug to either qe-verify+ or qe-verify-

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

I think we set qe-verify- for things we have automated tests for.