760625 - Use the blocklist to inform click-to-play plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

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

We want the ability to make vulnerable plugins click-to-play much in the way we have the ability to block plugins via the blocklist right now.

Comment 1

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

Attachment: prototype

Comment 2

[Ian Melven :imelven]

Comment on attachment 629282 [details] [diff] [review]
prototype

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

::: content/base/src/nsObjectLoadingContent.h
@@ +44,1 @@
>  };

please comment what these are (and what VUA and VNU) mean, as you have in other parts of the patch

::: dom/plugins/base/nsPluginHost.h
@@ +91,1 @@
>  

any particular reason you're not adding this to nsIPluginHost ? just curious.. :)

Comment 3

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

I assume you mean GetBlocklistStateByType? It didn't occur to me to add it to the interface, because every time nsIPluginHost is used in nsObjectLoadingContent.cpp, it's static_casted to nsPluginHost, so I just re-used that style. Is there a good reason for this behavior? Also, should I add the function to nsIPluginHost?

Comment 4

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

(In reply to David Keeler from comment #1)
> Created attachment 629282 [details] [diff] [review]
> prototype

Sorry I didn't get to this review yet. I will get to it later today.

Comment 5

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

Comment on attachment 629282 [details] [diff] [review]
prototype

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

I didn't do a great review of the nsPluginHost.cpp and related files since I'm not super comfortable with some of those code areas.

::: browser/base/content/browser.js
@@ +7343,5 @@
>      let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
>      // The overlay is null if the XBL binding is not attached (element is display:none).
>      if (overlay) {
>        overlay.addEventListener("click", function(aEvent) {
> +        if (aEvent.target instanceof XULElement && 

Out of curiosity, what does checking for XULElement get us here?

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2047,5 @@
> +    if (pluginHost) {
> +      rv = pluginHost->GetBlocklistStateByType(aContentType.get(), &state);
> +      if (NS_SUCCEEDED(rv)) {
> +        if (state == nsIBlocklistService::STATE_UPDATE_AVAILABLE)
> +          return ePluginClickToPlayVUA;

nit: In Gecko code, there should be braces on all if() { } else if {} else {}. Now's a good time to fix it for this function.

::: dom/plugins/base/nsPluginHost.cpp
@@ +1286,5 @@
> +  if (plugin) {
> +    nsCOMPtr<nsIBlocklistService> blocklist = do_GetService("@mozilla.org/extensions/blocklist;1");
> +    if (blocklist) {
> +      nsresult rv = blocklist->GetPluginBlocklistState(plugin, EmptyString(),
> +                                                       EmptyString(), aState);

What are the EmptyString()s here for?

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +46,5 @@
>  const MAX_BLOCK_LEVEL                 = 3;
>  const SEVERITY_OUTDATED               = 0;
> +const CLICKTOPLAY_NONE                = 0;
> +const CLICKTOPLAY_UPDATE_AVAILABLE    = 1;
> +const CLICKTOPLAY_NO_UPDATE           = 2;

The difference in naming between CLICKTOPLAY_NONE and CLICKTOPLAY_NO_UPDATE is hard to discern when only looking at these three lines. Can you pick a better name for this?

Comment 6

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

Attachment: patch

While we don't have the ui/ux nailed down yet, this patch implements the functionality we want. I've incorporated the feedback I've gotten and restructured some things so this patch no longer depends on any other bug, so we can get moving faster on this.

Comment 7

[Josh Aas]

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

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

I wonder if this patch should be split up into two pieces - one to re-do the mShouldPlay and gClickToPlayPlugins variable usage, and the other to do what you really want here. I'm worried that the logic is getting too complicated to be dealt with carefully. I'm not saying this is necessary, but if you wanted to do it I wouldn't mind.

r- mostly for the IsPluginClickToPlay return value, but I'm also concerned about the meaning of nsObjectLoadingContent::mShouldPlay. Let's talk about the latter on irc if you're unsure about what to do about it.

::: browser/base/content/browser-plugins.js
@@ +142,5 @@
>  
> +      case "PluginVulnerableUpdatable":
> +        let updateLink = doc.getAnonymousElementByAttribute(plugin, "class", "checkForUpdatesLink");
> +        self.addLinkClickCallback(updateLink, "updatePlugins");
> +        /* FALLTHRU */

If you fall through here are you going to open the update page and then play the plugin instance anyway?

@@ +161,2 @@
>        let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (overlay && self.isTooSmall(plugin, overlay))

I don't understand the changes to this 'if' block - can you explain?

@@ +232,5 @@
>    },
> + 
> +  // Callback for user clicking on the link in a click-to-play plugin
> +  // (where the plugin has an update)
> +  updatePlugins: function (aEvent) {

'updatePlugins' is a little vague - maybe use something more descriptive like "openPluginUpdateURL"?

::: content/base/src/nsObjectLoadingContent.cpp
@@ +479,5 @@
>    }
>  
> +  nsresult result = pluginHost->IsPluginClickToPlayForType(aMIMEType.get());
> +  if (NS_FAILED(result)) {
> +    mShouldPlay = true;

This is pretty awkward logic - I don't like the mapping of success/fail exception codes to a boolean answer. I wouldn't naturally expect a failed code to mean to go ahead and play the instance. Why not just make "IsPluginClickToPlayForType" return a bool?

@@ +574,5 @@
> +  if (!mShouldPlay) {
> +    return false;
> +  } else {
> +    return enabled;
> +  }

The above usage of "mShouldPlay" makes me think its meaning is getting a little too vague. It's not clear under what conditions it is true or false. Does it reflect whether a plugin is enabled or disabled? Is it simply whether a plugin is click-to-play or not? How does it relate to content permissions? This ambiguity is going to make the code harder to understand.

::: dom/plugins/base/nsPluginHost.h
@@ +84,5 @@
>                                 nsIURI *aURL,
>                                 nsIPluginInstanceOwner *aOwner);
>    nsresult IsPluginEnabledForType(const char* aMimeType);
>    nsresult IsPluginEnabledForExtension(const char* aExtension, const char* &aMimeType);
> +  nsresult IsPluginClickToPlayForType(const char *aMimeType);

Make this return a bool, this is not a good use of nsresult (which is essentially an exception code).

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

*sigh* The blocklist service really needs an overhaul.

Is there any reason why we would mark a plugin as outdated, but not enable click-to-play? We'd only mark a plugin as outdated if there is a benefit in telling the user - it seems like any such reason would benefit from click-to-play (potential security issues aren't the only reason for click-to-play). And it would be nice to avoid any unnecessary complexity here - we could just have STATE_OUTDATED and STATE_OUTDATED_UPDATE_AVAILABLE.

Comment 9

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

If we want to get rid of STATE_OUTDATED and the associated outdated plugin warning mechanisms, I'd be all for it, but that should probably be a separate bug. Also, STATE_NO_UPDATE is intended to convey something a bit stronger than STATE_OUTDATED - that is, the plugin is known to be vulnerable and there is no update, rather than it's simply old (any suggestions for a better name that conveys this yet isn't 80 characters long would be appreciated).

Comment 10

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

Attachment: patch v2

Comment 11

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

I pushed v2 of the patch to tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=0e3a9dfd3993

Comment 12

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

Comment on attachment 638517 [details] [diff] [review]
patch v2

I'm working on fixing the browser chrome test, but in the meantime, I thought it would be good to have another set of eyes on this, so I'm asking Blair for a second review.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 638517 [details] [diff] [review]
patch v2

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

Ok, then I think we should frame the blocklist API changes to better reflect comment 9, as they're quite disconnected at the moment. The existing blocklist code is describing the status, not what to do - would like to keep that consistent. And the naming suggestions below end up mapping nicely to the event names PluginVulnerableUpdatable and PluginVulnerableNoUpdate.

You'll also need to update the Add-on Manager UI (toolkit/mozapps/extensions/content/extensions.js and extensions.xul), since we display notifications for STATE_OUTDATED there. Will need to display similar notifications for the two new states.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +46,5 @@
>  const MAX_BLOCK_LEVEL                 = 3;
>  const SEVERITY_OUTDATED               = 0;
> +const CLICKTOPLAY_NOT_APPLICABLE      = 0; // indicates "ignore this field"
> +const CLICKTOPLAY_UPDATE_AVAILABLE    = 1;
> +const CLICKTOPLAY_NO_UPDATE           = 2;

CLICKTOPLAY_NOT_APPLICABLE -> VULNERABILITYSTATUS_NONE
etc

@@ +1051,5 @@
>    if (versionRangeElement && versionRangeElement.hasAttribute("severity"))
>      this.severity = versionRangeElement.getAttribute("severity");
>    else
>      this.severity = DEFAULT_SEVERITY;
> +  if (versionRangeElement && versionRangeElement.hasAttribute("clicktoplay"))

"clicktoplay" -> "vulnerabilitystatus"

@@ +1052,5 @@
>      this.severity = versionRangeElement.getAttribute("severity");
>    else
>      this.severity = DEFAULT_SEVERITY;
> +  if (versionRangeElement && versionRangeElement.hasAttribute("clicktoplay"))
> +    this.clicktoplay = versionRangeElement.getAttribute("clicktoplay");

this.clicktoplay -> this.vulnerabilityStatus

::: xpcom/system/nsIBlocklistService.idl
@@ +24,5 @@
>    const unsigned long STATE_OUTDATED    = 3;
> +  // Indicates that the item is vulnerable and there is an update.
> +  const unsigned long STATE_UPDATE_AVAILABLE   = 4;
> +  // Indicates that the item is vulnerable and there is no update.
> +  const unsigned long STATE_NO_UPDATE   = 5;

STATE_VULNERABLE_UPDATE_AVAILABLE and STATE_VULNERABLE_NO_UPDATE

Comment 14

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

Attachment: patch v3

I've updated the names of the constants and fields.
A concern about the UI: we're looking to land this before the migration date. Since there's additional UI work we have to do anyway (that we're pushing to the next release), would you be alright with me doing the addon manager UI later as a follow-up to this?

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to David Keeler from comment #14)
> would you be alright with me doing the addon
> manager UI later as a follow-up to this?

Yep, that's fine. Can you file a dependent followup bug so it doesn't get forgotten?

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 640664 [details] [diff] [review]
patch v3

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

r+ with a couple of nits fixed.

::: toolkit/locales/en-US/chrome/mozapps/plugins/plugins.dtd
@@ +25,5 @@
>  <!ENTITY tapToPlayPlugin                                     "Tap here to activate plugin.">
>  <!ENTITY clickToPlayPlugin                                   "Click here to activate plugin.">
> +<!ENTITY clickToPlayPluginVulnerableUpdateAvailable          "Click here to activate vulnerable plugin.">
> +<!ENTITY clickToPlayPluginVulnerableNoUpdate                 "Click here to activate vulnerable plugin (no update available).">
> +<!ENTITY checkForUpdates                                     "Check for updates…">

I assume these strings are part of the followup UI work? I guess they'll do if we don't use it until the followup work is done.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +954,5 @@
>          else if (!plugin.disabled && state != Ci.nsIBlocklistService.STATE_NOT_BLOCKED) {
>            if (state == Ci.nsIBlocklistService.STATE_OUTDATED) {
>              gPref.setBoolPref(PREF_PLUGINS_NOTIFYUSER, true);
>            }
> +          else if (state != Ci.nsIBlocklistService.STATE_VULNERABLE_UPDATE_AVAILABLE && state != Ci.nsIBlocklistService.STATE_VULNERABLE_NO_UPDATE) {

Wrap this line, line break at the &&

@@ +1051,5 @@
>      this.severity = versionRangeElement.getAttribute("severity");
>    else
>      this.severity = DEFAULT_SEVERITY;
> +  if (versionRangeElement && versionRangeElement.hasAttribute("vulnerabilitystatus")) {
> +    this.vulnerabilitystatus = versionRangeElement.getAttribute("vulnerabilitystatus");

Camel case the property name (this.vulnerabilityStatus)
(Not the attribute - that should stay as-is)

Comment 17

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

Attachment: patch v4

Fixed nits. Carrying over r+.
Try run: https://tbpl.mozilla.org/?tree=Try&rev=90f965562bbd
Also filed bug 772897 for the ui follow-up (and yes, those strings are just placeholders until we get the final ui).

Comment 18

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

Between that try run and this one: https://tbpl.mozilla.org/?tree=Try&rev=e1b52fbda573 (an earlier one where the only changes were identifier names), I think it's safe to call this good to go. Marking checkin-needed.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18d69de4ff67

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18d69de4ff67