1359092 - Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

I think we need to make our way (small patches) till we can finally assert that loadURIWithOptions recceives a non null triggeringPrincipal within Bug 1333030.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1359092_extend_loaduri_with_triggeringprincipal.patch

Boris, ultimately we would like to add an assertion that LoadURI[WithOptions]() always receives a valid triggeringPrincipal which initiated the load (See Bug 1333030).

This patch adds the groundwork by extending loadURI with an optional argument triggeringPrincipal. Since all those principal decisions are critical I would like to land a bunch of smaller sized patches, each blocking Bug 1333030 so we can focus on making sure we pass the right principal for each callsite.

Please note that this patch really only contains C++ callsites of loadURI. Acceptable?

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8861142 [details] [diff] [review]
bug_1359092_extend_loaduri_with_triggeringprincipal.patch

>@@ -444,17 +445,18 @@ nsDSURIContentListener::CheckFrameOption

This should probably use a nullprincipal or something instead of system.  Otherwise we're relying on our various complicated "oh, but don't inherit from triggering principal if it's system, etc" logic.  There's no need for that here.

Followup OK for this part.

>@@ -999,17 +1000,18 @@ nsDocShellTreeOwner::HandleEvent(nsIDOME

This one seems pretty fishy to me too...  I guess this is just preserving behavior for now?

>@@ -7875,17 +7875,18 @@ nsGlobalWindow::HomeOuter(ErrorResult& a
>+                           GetPrincipal());

It would be better to change the IDL here to pass in the caller principal and use that as the triggering principal.

r=me with the HomeOuter thing fixed, and followups filed as needed to investigate some of the other callsites.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1359092_extend_loaduri_with_triggeringprincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> >@@ -444,17 +445,18 @@ nsDSURIContentListener::CheckFrameOption
> 
> This should probably use a nullprincipal

Updated to nullPrincipal.

> 
> >@@ -999,17 +1000,18 @@ nsDocShellTreeOwner::HandleEvent(nsIDOME
> 
> This one seems pretty fishy to me too...  I guess this is just preserving
> behavior for now?

Yes - I filed Bug 1359191 to investigate.

> >@@ -7875,17 +7875,18 @@ nsGlobalWindow::HomeOuter(ErrorResult& a
> >+                           GetPrincipal());
> 
> It would be better to change the IDL here to pass in the caller principal
> and use that as the triggering principal.

done.

> r=me with the HomeOuter thing fixed
Carrying over r+ from bz.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e42f84996ea
Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument. r=bz

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f609798e5007
Backed out changeset 3e42f84996ea for causing assertion failures

Comment 7

[Carsten Book [:Tomcat]]

Hi Christoph, seems this push cause assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=93949697&repo=mozilla-inbound - also it seems the try run from comment #4 confirms this since there are 3 failures with this assertion failure. Could you take a look, thanks!

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1359092_extend_loaduri_with_triggeringprincipal.patch

Ah, wrong Origin attributes, that problem didn't occur with the systemPrincipal in the initial patch that was r+ed by Boris. In fact I took a closer look at this callsite again and I think it's best if we reuse the triggeringPrincipal from the cancelled channel and fall back to using the SystemPrincipal as we had in the initial patch. I filed Bug 1359358 to investigate whether we should fall back to systemPrincipal or rather to a new principal (using the right OA).

+++ b/docshell/base/nsDSURIContentListener.cpp
@@ -444,20 +444,23 @@ nsDSURIContentListener::CheckFrameOption
         if (webNav) {
+          nsCOMPtr<nsILoadInfo> loadInfo = httpChannel->GetLoadInfo();
+          nsCOMPtr<nsIPrincipal> triggeringPrincipal = loadInfo
+            ? loadInfo->TriggeringPrincipal()
+            : nsContentUtils::GetSystemPrincipal();
           webNav->LoadURI(u"about:blank",
                           0, nullptr, nullptr, nullptr,
+                          triggeringPrincipal);

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Now everything should be fine, but let's make sure:
   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae84b2b4fad5b209640f5dc60bfe218d113b8067

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4dc8b2f6af
Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument. r=bz

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb4dc8b2f6af