Have loadOneTab() provide the correct triggeringPrincipal

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Assignee: nobody → ckerschb
Blocks: 1362034
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
(Assignee)

Updated

a year ago
Depends on: 1364016
(Assignee)

Comment 1

11 months ago
Created attachment 8870462 [details] [diff] [review]
bug_1363975_loadonetab_triggeringprincipal_tests.patch
Attachment #8870462 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 2

11 months ago
Created attachment 8870463 [details] [diff] [review]
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.
Attachment #8870463 - Flags: feedback?(gijskruitbosch+bugs)

Comment 3

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

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

rs=me
Attachment #8870462 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 4

11 months ago
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...
Attachment #8870463 - Flags: feedback?(gijskruitbosch+bugs) → feedback+

Comment 5

11 months ago
(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?
Flags: needinfo?(miket)
(Assignee)

Comment 6

11 months ago
(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
(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).
Flags: needinfo?(miket)
(Assignee)

Comment 9

11 months ago
Created attachment 8872598 [details] [diff] [review]
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
Attachment #8870463 - Attachment is obsolete: true
Attachment #8872598 - Flags: review?(gijskruitbosch+bugs)
Attachment #8872598 - Flags: review?(bugs)

Updated

11 months ago
Attachment #8872598 - Flags: review?(bugs) → review+

Comment 10

11 months ago
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 :-)
Attachment #8872598 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 11

11 months ago
Pushed by mozilla@christophkerschbaumer.com:
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 13

11 months ago
backed out for eslint failures like https://treeherder.mozilla.org/logviewer.html#?job_id=103425268&repo=mozilla-inbound
Flags: needinfo?(ckerschb)
(Assignee)

Comment 14

11 months ago
Thanks Carsten, I'll look into that.
Flags: needinfo?(ckerschb)

Comment 15

11 months ago
Backout by cbook@mozilla.com:
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
BTW, I think this made bug 1348509 basically permafail too.
(Assignee)

Comment 17

11 months ago
(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

11 months ago
Pushed by mozilla@christophkerschbaumer.com:
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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5ef0facc604a
https://hg.mozilla.org/mozilla-central/rev/16af57faa3e0
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.