832481 - Require blocklist-induced click-to-play activation from the doorhanger

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

CTP is not meant to be immune to link-jacking, but when used as a security feature the ease of tricking users into activating the plugin is concerning.

One idea I discussed with jaws was to have clicking the CTP frame simply show the doorhanger, so a click on browser chrome is required to confirm the activation of vulnerable plugins.

Comment 1

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

In doing so, we would need to change the verbiage on the overlay to say something along the lines of "Click for more information" instead of the text that says "Click here to activate the X plugin"

Comment 2

[Larissa Co [:lco]]

I like this idea for vulnerable plugins (I'm discussing the entire CTP issue with the security team right now). But just to be clear: this proposal is for known vulnerable plugins only, not all CTP plugins, right? Because I think it could be a potentially unwieldy user experience when we're not even sure there's a vulnerability.

Comment 3

[Justin Dolske [:Dolske]]

Yes, I think that's correct.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: prototype

Here's a quick implementation of what this could look like.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

It's my understanding that we're moving forward with this. Here's the change with a test.

Comment 6

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

Comment on attachment 727424 [details] [diff] [review]
patch

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

::: browser/base/content/browser-plugins.js
@@ +475,5 @@
> +              objLoadingContent.pluginFallbackType !=
> +                Ci.nsIObjectLoadingContent.PLUGIN_VULNERABLE_NO_UPDATE)
> +            gPluginHandler.activateSinglePlugin(aEvent.target.ownerDocument.defaultView.top, aPlugin);
> +          else
> +            gPluginHandler._showClickToPlayNotification(browser, true);

Nit, please reorder the if/else to have the positive in the if case and fallback in the else. For example:

> if (pluginFallbackType == VULNERABLE_UPDATABLE ||
>     pluginFallbackType == VULNERABLE_NO_UPDATE)
>   showClickToPlayNotification();
> else
>   activateSinglePlugin();

I prefer this way as I believe it is more readable.

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v1.1

Thanks for the quick review!
Nit fixed - carrying over r+.
Try: https://tbpl.mozilla.org/?tree=Try&rev=dc1abe17bc9b

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5477c7a2231a

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5477c7a2231a

Comment 10

[Paul Silaghi, QA [:pauly]]

IMO, this is extremely annoying.

Comment 12

[Paul Silaghi, QA [:pauly]]

Now clicking on "Activate" popup activates all instances of Flash from that page. Is it expected?

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Paul Silaghi [QA] from comment #12)
> Now clicking on "Activate" popup activates all instances of Flash from that
> page. Is it expected?

No. Was this actually triggered by this bug or some later change?

Comment 14

[Paul Silaghi, QA [:pauly]]

Actually "activate" button has always been doing this. Is just that before this you were able to activate flash instances one at a time by clicking on the plugin area.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Yes, it always did that. 
While i misread the patch and it doesn't try to change that, i'm not sure if we actually intended to activate all instances with this change as this means there is now no way to activate single instances of vulnerable plugins.

Comment 16

[Benjamin Smedberg]

That was the intended change. We are going to stop exposing activation of single instances and leave that feature to addons.

Comment 17

[Paul Silaghi, QA [:pauly]]

Being unable to block the adds around my favorite game doesn't seem such a good idea to me.

Comment 18

[Benjamin Smedberg]

Ad blocking is the not the problem we're trying to solve with this feature. Users who want ad blocking should install adblock.