1363975 - Have loadOneTab() provide the correct triggeringPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363975_loadonetab_triggeringprincipal_tests.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363975_loadonetab_triggeringprincipal.patch

Gijs, thanks for your continous help with providing the right principal. Overall I think this patch is already pretty close, but there are still some open questions:

* Within browser-social.js, how do I query the triggeringPrincipal which I serialized within content.js. Is the principal stored within aMessage.data.triggeringPrincipal?

* Within browser.js it seems delayedOpenTab() is never called, can we remove it? I think also delayedOpenWindow() is never called.

* In the other cases where we load about:blank or also about:printpreview I think using the systemPrincipal is fine. In those cases the URL to be loaded can not be influenced by the web, hence I suppose using systemprincipal is fine.

* Within tabbrowser.xml, can I use Utils.deserializePrincipal()?

* Within WebCompatReporter.jsm, I suppose using systemPrincipal is wrong. I don't know what this code is doing and also don't know what principal we could use.

* Within TelemetryReportingPolicy.jsm, it seems firstRunPolicyURL comes from a preference, hence I suppose using systemPrincipal is fine.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8870462 [details] [diff] [review]
bug_1363975_loadonetab_triggeringprincipal_tests.patch

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

rs=me

Comment 4

[:Gijs (he/him)]

Comment on attachment 8870463 [details] [diff] [review]
bug_1363975_loadonetab_triggeringprincipal.patch

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

This generally looks sane enough. Detailed notes below. :-)

::: browser/base/content/browser-social.js
@@ +13,5 @@
>  (function() {
>  "use strict";
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Utils",
> +  "resource://gre/modules/sessionstore/Utils.jsm");

browser-social runs in the same global as browser.js, so not sure this is required.

@@ +207,5 @@
>            // a background tab
> +          let triggeringPrincipal = Utils.deserializePrincipal(aMessage.data.triggeringPrincipal);
> +          gBrowser.loadOneTab(provider.postActivationURL, {
> +            inBackground: SocialShare.panel.state == "open",
> +            triggeringPrincipal,

I think this should work, yes, but I haven't tested.

::: browser/base/content/browser.js
@@ -2289,5 @@
>  }
>  
> -/* Required because the tab needs time to set up its content viewers and get the load of
> -   the URI kicked off before becoming the active content area. */
> -function delayedOpenTab(aUrl, aReferrer, aCharset, aPostData, aAllowThirdPartyFixup) {

Welcome to add-ons! This is apparently not quite dead yet. Please don't remove. Just use system principal for now. We can remove it in 57 (followup for that please so we don't forget :-) ).

::: browser/base/content/tabbrowser.xml
@@ +5188,5 @@
>                  postData: null,
>                  allowThirdPartyFixup: true,
>                  relatedToCurrent: true,
>                  isPrerendered: true,
> +                triggeringPrincipal: Utils.deserializePrincipal(data.triggeringPricnipal),

This is misspelled. :-)

::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm
@@ +120,5 @@
>      const WEBCOMPAT_ORIGIN = new win.URL(WebCompatReporter.endpoint).origin;
>  
>      let tab = gBrowser.loadOneTab(
>        `${WebCompatReporter.endpoint}?url=${encodeURIComponent(tabData.url)}&src=desktop-reporter`,
> +      {inBackground: false, triggeringPrincipal: Services.securityManager.getSystemPrincipal()});

This is probably correct, actually. It's for reporting URLs to the web compat service. I'm not sure if this needs fixing in a separate repo... Mike Taylor would know.

::: dom/base/nsDocument.cpp
@@ +3144,5 @@
>    nsCOMPtr<nsIWebBrowserChrome3> wbc3;
>    tabChild->GetWebBrowserChrome(getter_AddRefs(wbc3));
>    NS_ENSURE_TRUE(wbc3, false);
>  
> +  rv = wbc3->StartPrerenderingDocument(aHref, referrer, NodePrincipal());

I don't know enough about this to help...

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs from comment #4)
> ::: browser/base/content/browser.js
> @@ -2289,5 @@
> >  }
> >  
> > -/* Required because the tab needs time to set up its content viewers and get the load of
> > -   the URI kicked off before becoming the active content area. */
> > -function delayedOpenTab(aUrl, aReferrer, aCharset, aPostData, aAllowThirdPartyFixup) {
> 
> Welcome to add-ons! This is apparently not quite dead yet. Please don't
> remove. Just use system principal for now. We can remove it in 57 (followup
> for that please so we don't forget :-) ).

See: https://dxr.mozilla.org/addons/search?q=delayedOpenTab(&redirect=true .

> ::: browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm
> @@ +120,5 @@
> >      const WEBCOMPAT_ORIGIN = new win.URL(WebCompatReporter.endpoint).origin;
> >  
> >      let tab = gBrowser.loadOneTab(
> >        `${WebCompatReporter.endpoint}?url=${encodeURIComponent(tabData.url)}&src=desktop-reporter`,
> > +      {inBackground: false, triggeringPrincipal: Services.securityManager.getSystemPrincipal()});
> 
> This is probably correct, actually. It's for reporting URLs to the web
> compat service. I'm not sure if this needs fixing in a separate repo... Mike
> Taylor would know.

--> ni. Mike, is there a separate repo for the web compat reporter, or do we just fix things in m-c?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #4)
> > -function delayedOpenTab(aUrl, aReferrer, aCharset, aPostData, aAllowThirdPartyFixup) {
> 
> Welcome to add-ons! This is apparently not quite dead yet. Please don't
> remove. Just use system principal for now. We can remove it in 57 (followup
> for that please so we don't forget :-) ).

I filed Bug 1367168 so we remember to remove that function.

> ::: dom/base/nsDocument.cpp
> > +  rv = wbc3->StartPrerenderingDocument(aHref, referrer, NodePrincipal());
> I don't know enough about this to help...

Let's see how we are doing on TRY. If we are good, I would appreciate you reviewing the frontend bits and we could ask smaug to review the backend changes in dom/.

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

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ab7ac428101b7c50e34ac6a843a9cce26dc9525

Comment 8

[Mike Taylor [:miketaylr]]

(In reply to :Gijs from comment #5)
> > This is probably correct, actually. It's for reporting URLs to the web
> > compat service. I'm not sure if this needs fixing in a separate repo... Mike
> > Taylor would know.
> 
> --> ni. Mike, is there a separate repo for the web compat reporter, or do we
> just fix things in m-c?

Just in m-c (we have a separate repo for the web-extension based addons, mostly for other browsers).

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1363975_loadonetab_triggeringprincipal.patch

@Gijs: Could you please take a look at the browser/ changes? I incorporated your suggestions from comment 4, but please note that browser-social.js needs to include "Utils.jsm" at the top, otherwise we get complaints about Utils not defined, e.g. when running browser_aboutHome_activation.js.

@smaug: Could you please reivew changes within dom/ toolkit/ and xpfe/?

Thanks!

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

Comment 10

[:Gijs (he/him)]

Comment on attachment 8872598 [details] [diff] [review]
bug_1363975_loadonetab_triggeringprincipal.patch

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

::: browser/base/content/browser.js
@@ +2274,5 @@
> +    charset: aCharset,
> +    postData: aPostData,
> +    inBackground: false,
> +    allowThirdPartyFixup: aAllowThirdPartyFixup,
> +    // Bug 1367168: only use systempPrincipal till we can remove that function

systemPrincipal :-)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b6ac0c2211d
Have loadOneTab() tests provide the correct triggeringPrincipal. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/a311d7c6ce20
Have loadOneTab() provide the correct triggeringPrincipal. r=gijs,smaug

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d4f2814ac3
Fixing eslint issues. r=me

Comment 13

[Carsten Book [:Tomcat]]

backed out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=103425268&repo=mozilla-inbound

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Thanks Carsten, I'll look into that.

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c28f6c34febb
Backed out changeset c3d4f2814ac3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5a1808d7fd
Backed out changeset a311d7c6ce20 
https://hg.mozilla.org/integration/mozilla-inbound/rev/559e6af0aed6
Backed out changeset 9b6ac0c2211d for eslint failures on a CLOSED TREE

Comment 16

[Ryan VanderMeulen [:RyanVM]]

BTW, I think this made bug 1348509 basically permafail too.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> BTW, I think this made bug 1348509 basically permafail too.

Thanks Ryan, in fact my patch was causing that problem because of a typo within WebCompatReporter.jsm.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef0facc604a
Have loadOneTab() tests provide the correct triggeringPrincipal. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/16af57faa3e0
Have loadOneTab() provide the correct triggeringPrincipal. r=gijs,smaug

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ef0facc604a
https://hg.mozilla.org/mozilla-central/rev/16af57faa3e0