887773 - CTP plugins no longer work on data: URLs

Status: VERIFIED FIXED

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

Description

[Frank Wein [:mcsmurf]]

I've noticed that since Bug 880735 landed, activating CTP plugins on data: URLs no longer works when there are multiple CTP plugins on the page (yes, this is an edge case in some way :).

To reproduce:
1. Load a data: URL with multiple CTP plugins, for example
data:text/html,<object type="application/x-java-applet"></object><object type="application/x-shockwave-flash"></object>

2. Try to activate any CTP plugin
3. Observe this error in Browser Console:
[15:11:07.090] NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host] @ chrome://browser/content/browser.js:3791

That's this line in browser.js code:
pluginInfo.pluginPermissionHost = browser.currentURI.host;

Comment 1

[Benjamin Smedberg]

We can certainly wrap that line of code. I guess I'm wondering a couple things, though:

* I presume that this data: URI was entered directly in the address bar, so it has a null principal. This means that we can't store permissions for the URL and the only real option for the user is "run all the plugins on the page right now"

* The situation *should* change if the data: URI loads from a different page, and inherits that page's principal. In that case we should load plugins based on the principal of the original page, but I suspect that we don't do that correctly (either in objectloadingcontent, or in the frontend, or both).

This seems like enough of an edge case that I'm not sure how much we should spend time making it perfect. Just fixing the JS exception so that the doorhanger works is probably important to fix for 24, though.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Test-case: In-page data: links

Same problem also exists with links to such data: URLs.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Band-aid

We do have a null-principal here both when the data: URL is directly copied into the nav-bar and when navigating to such an URL from another page.

What would we to do about the host in the doorhanger message though ("Allow <host> to run <plugin>?").

Comment 4

[Benjamin Smedberg]

That's interesting. I thought data: URIs were supposed to inherit their principal if you navigated from an existing page.

In any case for the null principal I'm fine with "allow (null) to run <plugin>". Doing much more than that would require adding a new set of strings, which doesn't sound worth it.

Comment 5

[Jesse Ruderman]

Are you sure you have a null-principal?  I think the problem is just that you're trying to get the host from the page URL rather than the principal.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Jesse Ruderman from comment #5)
> Are you sure you have a null-principal?

This principal here:
http://hg.mozilla.org/mozilla-central/annotate/4ffb23062b3b/browser/base/content/browser-plugins.js#l624
... has:
* URI.scheme=="moz-nullprincipal"
* origin=="moz-nullprincipal:{...}"
... and the permissionObject here has no host:
http://hg.mozilla.org/mozilla-central/annotate/4ffb23062b3b/browser/base/content/browser-plugins.js#l650

Comment 7

[Jesse Ruderman]

To get the principal for the document rather than the document's URL, try replacing

let principal = Services.scriptSecurityManager.getNoAppCodebasePrincipal(browser.currentURI);

with

let principal = contentWindow.document.nodePrincipal;

Comment 8

[Jesse Ruderman]

(I don't know if that changes things wrt navigation or frames, though.)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jesse Ruderman from comment #7)
> let principal =
> Services.scriptSecurityManager.getNoAppCodebasePrincipal(browser.currentURI);
> 
> with
> 
> let principal = contentWindow.document.nodePrincipal;

We should definitely make this change regardless - getNoAppCodebasePrincipal(browser.currentURI) isn't a good idea.

Comment 10

[Benjamin Smedberg]

Given this, it may be necessary to include both the principal (in order to later set the permission) and the domain (for display) in the pluginInfo object.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Use document.nodePrincipal, fix doorhanger host handling

Navigation & frame handling seems fine from what i looked at.

Jesse, could you give it a quick look if that matches what you thought of?
Jaws, are you able to review this?

Try run: https://tbpl.mozilla.org/?tree=Try&rev=106e276c10a7

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Test coverage for data URLs

Comment 13

[Jesse Ruderman]

Comment on attachment 770323 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling

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

::: browser/base/content/browser-plugins.js
@@ +329,5 @@
>        return false;
>  
>      let pluginHost = Cc["@mozilla.org/plugin/host;1"].getService(Ci.nsIPluginHost);
>      let permissionString = pluginHost.getPermissionStringForType(objLoadingContent.actualType);
> +    let principal = objLoadingContent.ownerDocument.defaultView.top.document.nodePrincipal;

Why the top document rather than the document with the plugin?  I guess it keeps the UI simple, but it's not really what I want for YouTube embeds.

@@ +495,5 @@
>      // not associated with any known plugin
>      if (!gPluginHandler.isKnownPlugin(objLoadingContent))
>        return;
>      let permissionString = pluginHost.getPermissionStringForType(objLoadingContent.actualType);
> +    let principal = document.nodePrincipal;

What is |document| here? I think you want doc.defaultView.top.document, like in the first hunk.

@@ +616,5 @@
>      }
>    },
>  
> +  // Match the behaviour of nsPermissionManager
> +  _getHostFromPrincipal: function PH_getHostFromPrincipal(principal) {

Trying to match the behavior of nsPermissionManager seems rather fragile.

At least add a comment in the other place that points here, so if someone changes it, they'll know to change both.

@@ +665,5 @@
>          pluginInfo.pluginPermissionHost = permissionObj.host;
>          pluginInfo.pluginPermissionType = permissionObj.expireType;
>        }
>        else {
> +        pluginInfo.pluginPermissionHost = principalHost;

Does this mean that enabling a plugin on an https page also enables it for the corresponding http page? :(

What does this do when principalHost is "(null)"?

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 770323 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling

r- for jesse's comments, but this looks like the right path!

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Jesse Ruderman from comment #13)
> Why the top document rather than the document with the plugin?  I guess it
> keeps the UI simple, but it's not really what I want for YouTube embeds.

From following the CTP re-spec, we want "activate plugins on this site". That the actual content may come from somewhere else is not really something that should matter for the user here (also, CDNs).

> Trying to match the behavior of nsPermissionManager seems rather fragile.

I'd rather inquire nsPermissionManager for the host, but currently there is nothing in public interfaces AFAICT.
 
> Does this mean that enabling a plugin on an https page also enables it for
> the corresponding http page? :(
> 
> What does this do when principalHost is "(null)"?

As is, browser-plugins delegates that decision to the permission manager (based on the top-document principal). |principalHost| is only for display purposes.
The permissions are shared over http/https, same as before.

E.g. copy-pasting a data-url in the nav bar gives a principalHost of "(null)":
* "allow now"/"allow always" unblock the plugin
* plugin still active after reload
* plugin not active anymore when copy-pasting the url again
... which seems like sensible behavior.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Attachment: Use document.nodePrincipal, fix doorhanger host handling, v2

Comment 17

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

Comment on attachment 770324 [details] [diff] [review]
Test coverage for data URLs

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

::: browser/base/content/test/browser_pluginnotification.js
@@ +819,5 @@
>    setAndUpdateBlocklist(gHttpTestRoot + "blockNoPlugins.xml",
>    function() {
>      clearAllPluginPermissions();
>      resetBlocklist();
> +    prepareTest(test25a, "about:blank");

Let's move these new tests to a separate file, since browser_pluginnotification.js already runs pretty long and I don't want it to start timing out.

@@ +868,5 @@
> +function test26a() {
> +  getTestPlugin().enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY;
> +  getTestPlugin("Second Test Plug-in").enabledState = Ci.nsIPluginTag.STATE_CLICKTOPLAY;
> +
> +  prepareTest(test26b, gHttpTestRoot + "plugin_data_url.html");

It looks like the beginning of test26a can be placed in test25d and the plugin_data_url.html page only needs to be loaded once here, instead of twice between these two functions.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Jesse, does comment 15 address your concerns?

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Test coverage for data URLs, v2

Addressed review comments. Tests are now in browser_CTP_data_urls.js

Comment 20

[:Gavin Sharp [email: [e-mail]]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> From following the CTP re-spec, we want "activate plugins on this site".
> That the actual content may come from somewhere else is not really something
> that should matter for the user here (also, CDNs).

This seems fishy. If the UI is adding permissions based on the outermost frame, the permissions checks need to match that. Is that the case? Doing permissions checks based on the outermost frame doesn't feel right and will likely lead to all sorts of weirdness.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #20)
> If the UI is adding permissions based on the outermost
> frame, the permissions checks need to match that. Is that the case?

Yes, both permission setting and checking is baed on the top document (unless i'm missing something of course). Note that this patch should merely change to using the respective (node)principals instead of the URIs, fixing some behavior related to that.

Comment 22

[Georg Fritzsche [:gfritzsche]]

So i guess the question becomes, can we move forward with this patch for now and follow-up with discussions on the validity of the approach later?

Comment 23

[Jesse Ruderman]

You didn't address or answer the second hunk of comment 15.

Please file bugs for:
* nsPermissionManager should expose the interface you need
* nsPermissionManager should allow distinguishing between http and https

Comment 24

[:Gavin Sharp [email: [e-mail]]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #21)
> Yes, both permission setting and checking is baed on the top document
> (unless i'm missing something of course).

Can you point to the permission checking code?

Comment 25

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/annotate/17fe59f6c54a/content/base/src/nsObjectLoadingContent.cpp#l2974

Comment 26

[Georg Fritzsche [:gfritzsche]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #24)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #21)
> > Yes, both permission setting and checking is baed on the top document
> > (unless i'm missing something of course).
> 
> Can you point to the permission checking code?

That...
> http://hg.mozilla.org/mozilla-central/annotate/17fe59f6c54a/content/base/src/
> nsObjectLoadingContent.cpp#l2974

... and in the front-end the parts addressed by the patch (perms.addFromPrincipal() and perms.testPermissionFromPrincipal()).

(In reply to Jesse Ruderman from comment #23)
> You didn't address or answer the second hunk of comment 15.

Right, i just fixed it with the updated patch.

> Please file bugs for:
> * nsPermissionManager should expose the interface you need
> * nsPermissionManager should allow distinguishing between http and https

Filed bug 891285 & bug 891281.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 770749 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling, v2

Gavin, do yoiu want to review this then?

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 770749 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling, v2

I don't fully understand what notification.options.centerActions[0] is (and whether it's correct to get the host from it in the first urlbarBindings.xml hunk), and I haven't verified that this is complete, but the changes from currentURI->principal look sound (the .top thing still seems a bit odd, but I suppose it makes sense). Sorry for the delay.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 770749 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling, v2

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #28)
> I don't fully understand what notification.options.centerActions[0] is (and
> whether it's correct to get the host from it in the first urlbarBindings.xml
> hunk)

centerActions is used by browser-plugins.js to set the plugin data from which the code in urlbarBindings.xml builds it state.

Benjamin, does the centerActions usage look proper to you?

Comment 30

[Benjamin Smedberg]

Comment on attachment 770749 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling, v2

Well hrm. *Normally* the pluginPermissionHost will be the same for all the centerActions. But if there is a permission set for a superDomain, it theoretically could be different. It's an edge case, and you could solve it using gPluginHandler._getHostFromPrincipal(this.notification.browser.contentWindow.document.nodePrincipal);

But it's also so weird that it's probably not worth it.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Attachment: Use document.nodePrincipal, fix doorhanger host handling, v3

Rebased, also found it works fine with the improvement from comment 30 incorporated, so lets do this and not have to worry about edge-cases.

Comment 32

[Georg Fritzsche [:gfritzsche]]

Attachment: Test coverage for data URLs, v3

Rebased.

Comment 33

[Georg Fritzsche [:gfritzsche]]

Checkin appreciated - the internet connection where i am today makes pulling the inbound updates a major pain.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to inbound. Please rebase.

Comment 35

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/43e5b71e1680
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1a5b7f6f8d

Comment 36

[Dão Gottwald [:dao]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #29)
> (In reply to :Gavin Sharp (use [e-mail] for email) from comment
> #28)
> > I don't fully understand what notification.options.centerActions[0] is (and
> > whether it's correct to get the host from it in the first urlbarBindings.xml
> > hunk)
> 
> centerActions is used by browser-plugins.js to set the plugin data from
> which the code in urlbarBindings.xml builds it state.

I don't understand the name "centerActions". Can we change it to something that makes sense?

Comment 37

[Georg Fritzsche [:gfritzsche]]

(In reply to Dão Gottwald [:dao] from comment #36)
> I don't understand the name "centerActions". Can we change it to something
> that makes sense?

Filed bug 896418.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43e5b71e1680
https://hg.mozilla.org/mozilla-central/rev/0a1a5b7f6f8d

Comment 39

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 778520 [details] [diff] [review]
Use document.nodePrincipal, fix doorhanger host handling, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 880735, CtP doorhanger redesign.
User impact if declined: CtP doorhanger not available at all on data: URLs, permission inheritance not working properly in other scenarios.
Testing completed (on m-c, etc.): Baked on m-c for a bit, has test-coverage.
Risk to taking this patch (and alternatives if risky): Slight behavior changes regarding permission inheritence, no major risk though AFAICT.
String or IDL/UUID changes made by this patch: None.

Comment 40

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 778521 [details] [diff] [review]
Test coverage for data URLs, v3

[Approval Request Comment]
See comment 39, these are the tests.

Comment 41

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/135008e6323f
https://hg.mozilla.org/releases/mozilla-aurora/rev/44e40c775ff7

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Georg, please don't resolve bugs until they land on m-c.

Comment 43

[Paul Silaghi, QA [:pauly]]

The CTP doorhanger is accessible now, but after "Allow now", the CTP overlay (Activate Java) doesn't go away only after refresh. Is it ok ?

Comment 44

[Georg Fritzsche [:gfritzsche]]

This is not specific to data: URLs, filed bug 902882.

Comment 45

[Paul Silaghi, QA [:pauly]]

Thanks Georg.
Verified fixed FF 24b1, 25.0a2 (2013-08-07)