875454 - Check site-specific plugin permissions for all plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Benjamin Smedberg]

Currently we only check site-specific plugin permissions for plugins which have a global enabled status of click-to-play. This patch refactors the permission behavior so that we always check with the permission manager before we check the plugin enabled-state. This will allow users to disable plugins on certain sites even if the global state is "enabled" and conversely enable a plugin on a particular site even if they have it "disabled" for all sites.

This patch is necessary in order to make the plugin doorhanger work in a reasonable way and is part of the larger changes needed for that refactoring.

Comment 1

[Benjamin Smedberg]

Attachment: Site-specific permissions always take precedence, rev. 1

jaws, I wasn't sure if you'd want to review this or delegate it to keeler since he did the first pass at all this.

Comment 2

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

Comment on attachment 753425 [details] [diff] [review]
Site-specific permissions always take precedence, rev. 1

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

Looks good to me - just a couple of comments/questions.

::: browser/base/content/pageinfo/permissions.js
@@ +274,5 @@
>    let attrs = [
>      [ ".permPluginTemplateLabel", "value", aPluginName ],
>      [ ".permPluginTemplateRadioGroup", "id", aPermissionString + "RadioGroup" ],
> +    [ ".permPluginTemplateRadioDefault", "id", aPermissionString + "#0" ],
> +    [ ".permPluginTemplateRadioAsk", "id", aPermissionString + "#3" ],

I recall there being some equivalence between the numbers after the hashes and the values stored in the permission database (unfortunate, I know...) - does changing the "ask" number from 0 to 3 work? Also, do we want to keep this list sorted in some way for readability?

::: browser/base/content/test/browser_pageInfo_plugins.js
@@ +59,4 @@
>    doOnPageLoad(gHttpTestRoot + "plugin_two_types.html", testPart1a);
>  }
>  
>  // By default, everything should be click-to-play. So: no plugins should be

looks like this comment needs to be updated

@@ -181,5 @@
>    gPageInfo.close();
> -  doOnPageLoad(gHttpTestRoot + "plugin_two_types.html", testPart5a);
> -}
> -
> -// check that if there are no click-to-play plugins, the plugin row is hidden

do we not still want to test this behavior? (or does this not happen, now?)

::: dom/plugins/base/nsIPluginHost.idl
@@ -97,4 @@
>  
>    ACString getPermissionStringForType(in AUTF8String mimeType);
>  
> -  bool isPluginClickToPlayForType(in AUTF8String mimeType);

There are two tests that use this function, but it should be pretty easy to convert them to the new api:
toolkit/mozapps/extensions/test/xpcshell/test_pluginBlocklistCtp.js
dom/plugins/test/mochitest/test_plugin_tag_clicktoplay.html

Comment 3

[John Schoenick [:johns]]

Comment on attachment 753425 [details] [diff] [review]
Site-specific permissions always take precedence, rev. 1

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

Primarily looking at the C++ bits. This looks good - some of the playpreview code is a mess, but there are other bugs to address that.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2756,4 @@
>      return true;
>    }
>  
> +  if (mActivated) {

This function is increasingly confusing, an overview comment to explain the order we check things or some such might be helpful here.

@@ +2772,5 @@
>      aReason = eFallbackVulnerableNoUpdate;
>    }
>  
> +  if (aReason == eFallbackClickToPlay && isPlayPreviewSpecified &&
> +      !mPlayPreviewCanceled && !ignoreCTP) {

I had to read the playPreview logic here like four times to understand what was going on, a comment might be helpful.

(bug 867626 should clean this up substantially)

@@ +2841,5 @@
> +  switch (enabledState) {
> +  case nsIPluginTag::STATE_ENABLED:
> +    return true;
> +  case nsIPluginTag::STATE_DISABLED:
> +    aReason = eFallbackDisabled;

You'll need to adjust GetTypeOfContent() to select a plugin even if it is disabled, with a comment that ShouldPlay() will handle the disabled check -- otherwise UpdateObjectParameters will not select a plugin type to begin with.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ -1795,5 @@
> -  rv = pluginHost->IsPluginClickToPlayForType(mimeType, &isClickToPlay);
> -  if (NS_FAILED(rv) || !isClickToPlay) {
> -    return;
> -  }
> -

This would be fine, since java applets without code shouldn't be scriptable anyway, but the function isn't checking this quite right:

>   for (uint16_t i = 0; i < paramCount; ++i) {
>     if (PL_strcasecmp(paramNames[i], "code") == 0) {
>       haveCodeParam = true;
>       if (strlen(paramValues[i]) > 0) {
>         isCodeParamEmpty = false;
>       }
>       break;
>     }
>   }

Java uses the last param it sees, not the first, e.g. will load a valid applet:
> <applet code=""><param name="code" value="."></applet>

This is a different bug, but it's a tiny enough fix you could just fold it in here.

Side note: line 434+ of this file is terrifying

Comment 4

[John Schoenick [:johns]]

> You'll need to adjust GetTypeOfContent() to select a plugin even if it is
> disabled, with a comment that ShouldPlay() will handle the disabled check --
> otherwise UpdateObjectParameters will not select a plugin type to begin with.

That is, assuming we want a globally disabled plugin to play if it has an explicit allow permission per site. If we don't want that, the disabled check is redundant.

Comment 5

[Benjamin Smedberg]

> ::: browser/base/content/pageinfo/permissions.js
> @@ +274,5 @@
> >    let attrs = [
> >      [ ".permPluginTemplateLabel", "value", aPluginName ],
> >      [ ".permPluginTemplateRadioGroup", "id", aPermissionString + "RadioGroup" ],
> > +    [ ".permPluginTemplateRadioDefault", "id", aPermissionString + "#0" ],
> > +    [ ".permPluginTemplateRadioAsk", "id", aPermissionString + "#3" ],
> 
> I recall there being some equivalence between the numbers after the hashes
> and the values stored in the permission database (unfortunate, I know...) -
> does changing the "ask" number from 0 to 3 work? Also, do we want to keep
> this list sorted in some way for readability?

The numbers match up with the constants in nsIPermissionManager. We weren't supposed to use "0" at all, since that officially means UNKNOWN_ACTION or "nothing is stored in the permissions database, use the default". But in this case no migration is necessary, since this UI was only shown and effective for CtP plugins, and that will be the default anyway.

This list is sorted in the same order that it is displayed onscreen: default/ask/allow/block. I think that is as logical as any other ordering ;-)


> > -// check that if there are no click-to-play plugins, the plugin row is hidden
> 
> do we not still want to test this behavior? (or does this not happen, now?)

It does not happen. This UI now shows all plugins, not just the CtP ones.

Comment 6

[Benjamin Smedberg]

I figured out that there's an important piece missing from this patch. nsIPluginTag.enabledState (and the other properties on it) don't actually return click-to-play state for plugins which are CtP-blocklisted. This was handled by IsPluginClickToPlayForType, and I didn't replace that logic in ShouldPlay.

Putting this logic directly into ShouldPlay isn't a big deal, but it does mean that you can't test correct blocklist behavior using an xpcshell test, or even really know what state a plugin is going to be in without actually creating one and trying it.

Not sure whether this is a problem, so the patch I'm going to upload next just puts this logic in ShouldPlay and removes the xpcshell test which is no longer testing anything useful. There is an equivalent mochitest which does test the correct thing.

Comment 7

[Benjamin Smedberg]

Actually I did add this logic to ShouldPlay. I just need to fix the tests. Wow.

Comment 8

[Benjamin Smedberg]

Attachment: Rework handling of per-site plugin permissions in objectloadingcontent such that users can disable or enable plugins per-site no matter what their "global" permission is (currently only plugins marked click-to-activate globally honor per-site overrides).

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ef49706a56a