Assuming the following test case: You are on a page on http://mochi.test:8888. That page has an iframe to test1.mochi.test:8888/frame1 and test2.mochi.test:8888/frame2. test2.mochi.test:8888/frame2 further has a subframe test2.mochi.test:8888/subframe. test1.mochi.test:8888/frame1 can navigate the subframe in test2.mochi.test:8888/frame2 if document.domain is set to mochi.test:8888. When doing this navigation I would expect the triggeringPrincipal to be test1.mochi.test:8888 and test2.mochi.test:8888 the loadingPrincipal. However, they are both set to mochi.test:8888/frame2.html.

the referrer is set to test1.mochi.test:8888/frame1 in this scenario

> However, they are both set to mochi.test:8888/frame2.html. That seems ... unlikely. What are they actually set to? Is it "http://test2.mochi.test:8888/frame2"? How exactly is the navigation being performed? Link to a testcase?

they are both actually set to mochi.test:8888/frame2.html, i.e. using the domain set through document.domain. See bug 1091883 comment 27 for easier test case (didn't check that one yet, but it has the same setup).

> i.e. using the domain set through document.domain. I see. That's a bit odd; I can see us using the principal's URI or the document's URI as referrer (if you look at nsLocation::CheckURL), but neither of those should be affected by document.domain as far as I know. Worth stepping through that function for this testcase to see what's going on.

Here is another one of Franziskus' test cases: http://myzilla.cf/iframeTestWithDocumentDomainRP.html www.myzilla.cf/iframeTestWithDocumentDomain.html contains two iframes test1.myzilla.cf/frame1 and test2.myzilla.cf/frame2, which contains another iframe test2.myzilla.cf/subFrameWDD. The link clickme in frame1 sets parent.frames[1].frames[0].location = "http://test2.myzilla.cf/subFrame2.html". document.domain is set on frame1 and the subframe. And here is the output I get from DocShell: https://etherpad-mozilla.org/docShellPrincipals-subframes triggeringPrincipal Location is wrong here; it is set to subFrameWDD.html. subFrameWDD.html is the original frame that is getting navigated, but it didn't cause the navigation. The navigation is caused by frame1WDD.html, which is accurately reflected by aReferrerURI. But triggeringPrincipal isn't set to that.

Yes, that's exactly what I've been saying about this docshell codepath for a while: we need to use for the "triggering principal" the principal that corresponds to the caller of the location setter or the principal of the anchor node that triggered the navigation etc.

In this bug, we should fix both the triggeringPrincipal and the requestingPrincipal passed to content policies (which should be equivalent). (copied from https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c12) triggeringPrincipal: * The triggeringprincipal for toplevel loads that are triggered from a webpage behave like normal. I.e. the page calling window.open or that contains the <a href=... (target=...)> is used as triggeringPrincipal. We get this from the owner passed into nsDocShell::InternalLoad or nsDocShell::DoURILoad. * For bookmarks and urls that users type into the address bar (which always load in toplevel) attempt to create a nsIPrincipal from the bookmark/typed URL and use that principal as triggeringPrincipal. The goal is: ** http(s) and ftp URLs should get a normal codebase principal. (Note that we also discussed using null here, but Jonas was in favor of using the codebase principal. That way addons won't end up in situations where they have a null loading and triggeringPrincipal, and don't know whether to allow or reject the load.) ** chrome:// (and maybe resource:// ?) URLs get a systemprincipal ** data: URLs get a nullprincipal (does checkLoadURI(nullprincipal, data:-uri) fail?) ** javascript: use the principal of the current page. This one likely needs some form of special-casing. This is needed to get bookmarklets to work. Unclear if the specialcasing should be based on the "javascript" scheme, or some protocol flag. Look at what other code does. * The code that handles aLoadType == LOAD_NORMAL_EXTERNAL likely does everything we need to safely treat things like desktop shortcuts and commandline options as normal bookmarks.

Copied from https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c38 After much debate, I’ve conceded that requestingPrincipal should be the loads “trigger” and not the context in which the load is taking place. Content Policy consumers should be able to get the context in which the load is taking place by getting the principal associated with the requestingContext (as long as the requestingContext is a node). Hence, passing the trigger provides more information to the policies. In the case of TYPE_DOCUMENT loads, we don’t have a requestingContext, but the fact that the load is TYPE_DOCUMENT should tell the Content Policy all it needs to know - it is being loaded as a top level page. Moreover, the behavior of requestingPrincipal (the principal passed to content policies) is not consistent today. This is because there are many problems with the “owner” used to derive requestingPrincipal (see 4 below for more on this). Hence, policies that depend on it are likely already broken. We should document this clearly in the Content Policy API.

Can we mark this one as a dup of Bug 1105556?

Tanvi already marked this one as a duplicate of Bug 1066868 - Closing.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11) > Tanvi already marked this one as a duplicate of Bug 1066868 - Closing. Sorry, the other bug was marked as a dup of this one. We have to keep this one around.

We need to consider how originAttributes should be handled in the proposal in comment 7.

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #13) > We need to consider how originAttributes should be handled in the proposal > in comment 7. See https://bugzilla.mozilla.org/show_bug.cgi?id=1278751#c11

Franziskus, thanks for filing this bug. Before moving on and trying to fix this problem I would like to make sure we are all on the same page and I interpreted your expected behavior from comment 0 correctly. I (think) I converted your test from comment 0 into an automated test. I was wondering if you can take a look at the test to make sure I got it all correct. The current output of the test is: > [PASS] LOADINGPRINCIPAL: http://test2.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame2 > [FAIL] TRIGGERINGPRINCIPAL: http://test2.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?subframe but should be http://test1.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame1 Is that what you expect as well?

That test case looks like the test case I describe if I recall correctly. You might also want to check that the correct referrer is set. In the test page from comment 5 you see that the referrer (coming from the triggering principal) is not correct. This is how I found the bug originally as it's the observably wrong behaviour.

Boris, please see comment 15 for current behavior which causes the test to fail. TriggeringPrincipal is incorrectly set to [1] instead of [2]. [1] http://test2.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?subframe [2] http://test1.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame1

Boris, I am really not sure if this is the right change to fix the problem, but as far as I can tell (modulo side effects) it is working. Here are all relevant values within nsLocation::CheckURL [1] when running the attached testcase for navigating a sub-iframe: > aURI: http://test2.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?subframe_nav > docOriginalURI: http://test1.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame1 > docCurrentURI : http://test1.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame1 > owner(Principal): http://test1.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame1 > referrerURI (sourceURI): http://test1.mochi.test:8888/tests/docshell/test/navigation/file_triggeringprincipal_frame_nav_server.sjs?frame1 Is this a patch you would consider to accept? Am I missing something entirely? Is there more information I can provide so we can fix the problem described in comment 0? Thanks for your help! [1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsLocation.cpp#104

On a different note, do we really need |aOwner| and |aReferrer| within internalLoad [1], shouldn't those two values always be the same? Potentially it's worth unifying those two values into one argument |nsIPrincipal aTriggeringPrincipal|. Or am I missing something important? [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#160

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #19) > On a different note, do we really need |aOwner| and |aReferrer| within > internalLoad [1], shouldn't those two values always be the same? Apparently that's not always that easy as Bug 986439 shows, because the owner (triggeringPrincipal) might be a nullPrincipal as well in which case we can't extract the referrer URI from the triggeringPricnipal. I suppose we have to keep both, the triggeringPrincipal (or also owner) as well as the referrerURI; What we could do though is the following: * Change |nsISupports* owner;| to |nsIPricnipal triggeringPrincipal;| and make that argument mandatory (be non null) within InternalLoad(). That way we can eliminate that scary Systemprincipal fallback mechanism for triggeringPrincipal within InternalLoad() [1] and push setting that argument (even if it needs to be the systemPrincipal) closer to the places that initiate the load, e.g within: * nsDocShell * nsFrameLoader * ServiceWorkerWindowClient * nsWindowWatcher * We can then assert within InternalLoad that triggeringPrincipal->URI and referrerURI are the same, which should be the case most of the time and just special case those few exceptions like e.g. where the triggeringPrincipal is the nullPrincipal. [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10787

Comment on attachment 8768815 [details] [diff] [review] bug_1181370_iframe_triggeringpringipal.patch The commit message for this patch doesn't seem related to the actual changes being made; what's changing is the behavior of Location, not of iframes. Please write a commit message that actually explains what the goal of the patch is; that will also make it easier to figure out whether the patch actually achieves its goal.

Comment on attachment 8768814 [details] [diff] [review] bug_1181370_iframe_triggeringpringipal_tests.patch Why does this need to be an sjs? Just having some helper files (and ^headers^ files if needed) seems like it would be much easier to read. If you _do_ need to dump your HTML in JS, please use strings in `` to do so. That way it will at least have a chance to be readable. I'm happy to review this on substance once the readability issue is addressed and there is a clear explanation of what we're trying to test.

(In reply to Boris Zbarsky [:bz] from comment #22) > The commit message for this patch doesn't seem related to the actual changes > being made; what's changing is the behavior of Location, not of iframes. Hey Boris, sorry for the confusion. I updated the commit message to reflect the actual changes being performed. Since I already have you here I have two questions regarding follow up bugs before we can convert docshell loads to rely on AsyncOpen2(). a) Referrer and TriggeringPrincipal should be identical in most of the cases (so far I only know of one edge case where the triggeringPricipal might be a nullPrincipal). I think we can/should file a follow up to make sure that is the case and assert that triggeringPrincipal-URI and referrer are the same. Agreed or am I missing something? b) Currently docshell uses the term 'owner' but the loadInfo uses the term TriggeringPrincipal. Would it make sense to update |nsISupports owner;| to become |nsIPrincipal triggeringPrincipal;| [2]. I crafted a first patch and it seems owner is always an nsIPrincipal so I guess there is no need that it remains an nsISupports, or am I forgetting about something? [1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10782 [2] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#164

(In reply to Boris Zbarsky [:bz] from comment #23) > Why does this need to be an sjs? Just having some helper files (and > ^headers^ files if needed) seems like it would be much easier to read. No particular need. I removed the *.sjs and dumped all the 'frames' into their own *.html file. Should be readable now. Sorry for the confusion.

Comment on attachment 8770134 [details] [diff] [review] bug_1181370_change_location_behavior.patch OK. So given the new summary... that's not what the patch is doing. There are three separate URIs involved in the logic here: 1) The principal's URI. This is normally the original URI of the document, except when the document is sandboxed or has the system principal inherited a principal from another document. 2) The document's original URI. This is the URI it was loaded from. 3) The document's current URI. This starts out equal to the originalURI and can then be modified by (1) anchor scrolls and (2) pushState and company. What the Location code is trying to do is use the current URI as the referrer in the common case when the principal URI is identical to the document's originalURI. When it's not, we use the principal's URI (e.g. so that the referrer for an about:blank won't be about:blank but rather the thing that the about:blank inherited origin from), except in the sandboxed case, in which we simply leave sourceURI null. So after this patch, in the "urisEqual" case you will have the referrer be the current URI (as modified by pushState, etc) and the triggering principal be the principal of the document (which may have a quite different URI, at least in terms of the path). In the !urisEqual case the referrer would keep being the URI of doc->NodePrincipal() (unless that's a nullprincipal) and the triggering principal would still get set to nsContentUtils::SubjectPrincipal() as now, so nothing would change and the two would still not match. In particular, after this patch if I have a page that has a javascript: subframe (so they have the same principal), scripts in that page and subframe that set location on some third document in the sort of document.domain-munging setup that this bug is talking about would end up with _different_ triggering principals. That seems obviously wrong (hence the r- on this patch). We should at the very least be consistent on the two branches of the urisEqual check, because it has nothing to do with who triggered the load, but only with whether docCurrentURI is likely to be a reasonable referrer URI. I suspect all this referrer logic doesn't really follow the spec and doesn't agree with other browsers, by the way. So given all that... what behavior are we _actually_ aiming for here and why? Note that it would make some sense to me to use doc->NodePrincipal() unconditionally (if doc is not null) as the triggering principal. But we're also sort of trying to get rid of incumbent globals, as I understand, so it may well be that nsContentUtils::SubjectPrincipal ends up being the _only_ thing you can do here at some point.

I'm going to hold off on reviewing the tests until we're clear on what the desired behavior actually is. ccing bholley for his opinions on the use of incumbent global, but note that he's on PTO until early next week.

(In reply to Boris Zbarsky [:bz] from comment #27) > So given all that... what behavior are we _actually_ aiming for here and why? The short answer is that I simply don't know. Given your answer from Comment 27 there are simply too many cases I haven't thought about and also didn't know about. But I agree that we should be at least consistent. > Note that it would make some sense to me to use doc->NodePrincipal() > unconditionally (if doc is not null) as the triggering principal. But we're > also sort of trying to get rid of incumbent globals, as I understand, so it > may well be that nsContentUtils::SubjectPrincipal ends up being the _only_ > thing you can do here at some point. I think for now I am going to wait for Bobby's input before moving this patch any further. Bobby, any thoughts on how to move forward with this bug given Comment 27 and Comment 28? [Note, I can't needinfo him at the moment since he is not accepting ni? requests. I will ping him early next week once he is back from PTO].

Comment 7 covers what we want the triggering principal to be in most cases. Are there cases that it misses? Of course, the current patch doesn't seem to implement that at all?

Hey Bobby, can you share your thoughts around incumbent globals (see Comment 27 and Comment 28 for details)? Thanks!

Unless we come up with some other plan for referrer on location navigation, I think we're stuck with incumbent global, and so I'm fine with using it in places that make really make sense (but would much prefer to use other things if possible). I didn't look at the bug context here in much detail. If there's a more specific question you have let me know.

(In reply to Boris Zbarsky [:bz] from comment #27) > So given all that... what behavior are we _actually_ aiming for here and why? > > Note that it would make some sense to me to use doc->NodePrincipal() > unconditionally (if doc is not null) as the triggering principal. But we're > also sort of trying to get rid of incumbent globals, as I understand, so it > may well be that nsContentUtils::SubjectPrincipal ends up being the _only_ > thing you can do here at some point. Boris, given Bobby's response within comment 32 it seems that incumbent globals are not going to go away soonish. I restructured the code within nsLocation.cpp and added comments (as good as possible) to explain what the behavior should be. In particular in what cases the referrer and the triggeringPrincipal are *not* identical. I think that's what we are aiming for within this patch. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5c6e01e3773

Comment on attachment 8772783 [details] [diff] [review] bug_1181370_change_location_behavior.patch >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >@@ -10744,20 +10744,16 @@ nsDocShell::DoURILoad(nsIURI* aURI, > return NS_OK; > } > } > > bool isSandBoxed = mSandboxFlags & SANDBOXED_ORIGIN; > // only inherit if we have a triggeringPrincipal > bool inherit = false; > >- // Get triggeringPrincipal. This code should be updated by bug 1181370. >- // Until then, we cannot rely on the triggeringPrincipal for TYPE_DOCUMENT >- // or TYPE_SUBDOCUMENT loads. Notice the triggeringPrincipal falls back to >- // systemPrincipal below. We shouldn't remove this comment, since we still do fallback to systemPrincipal below. This bug has been repurposed a bit, and reduced the likelihood of the systemPrincipal fallback, but doesn't remove it. We could file another bug and change the comment to reflect that bug number instaed. > nsCOMPtr<nsIPrincipal> triggeringPrincipal = do_QueryInterface(aOwner); > if (triggeringPrincipal) { > inherit = nsContentUtils::ChannelShouldInheritPrincipal( > triggeringPrincipal, > aURI, > true, // aInheritForAboutBlank > isSrcdoc); > } else if (!triggeringPrincipal && aReferrerURI) {

If this bug isn't implementing what's described in comment 7, then we definitely need to file a followup to implement that. To be honest I no longer understand what we're trying to accomplish in this bug.

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #34) > >- // Get triggeringPrincipal. This code should be updated by bug 1181370. > >- // Until then, we cannot rely on the triggeringPrincipal for TYPE_DOCUMENT > >- // or TYPE_SUBDOCUMENT loads. Notice the triggeringPrincipal falls back to > >- // systemPrincipal below. > We shouldn't remove this comment, since we still do fallback to > systemPrincipal below. This bug has been repurposed a bit, and reduced the > likelihood of the systemPrincipal fallback, but doesn't remove it. We could > file another bug and change the comment to reflect that bug number instaed. Sure, we can keep that comment and just update the bug number(s) within that comment. Looking at docshell for a while I don't think it will ever be possible (without substantial refactoring) to remove the SystemPrincipal fallback. (In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #35) > If this bug isn't implementing what's described in comment 7, then we > definitely need to file a followup to implement that. We can file follow up bugs to accomplish what's described in comment 7, but that's too much for one bug. One step at a time. Let's fix only the problem described in the description, which is also what the testcase is designed for.

Comment on attachment 8772783 [details] [diff] [review] bug_1181370_change_location_behavior.patch Tanvi is right: the nsDocShell change here is wrong, because this patch doesn't really help with that code. >+ // If possible, the referrer and the URI of owner (replaced by triggeringPrincipal >+ // within Bug 1286472) Please either remove this comment in this bug or in bug 1286472, whichever lands later. >+ // * If the document's principal URI does not match the current document's >+ // URI, This is very misleading, because the test is done against doc->GetOriginalURI(), not doc->GetDocumentURI()... It seems to me that the thing this comment should _really_ say is something this: The triggering principal for this load should be the principal of the incumbent document (which matches where the referrer information is coming from) when there is an incumbent document, and the subject principal otherwise. Note that the URI in the triggering principal may not match the referrer URI in various cases, notably including the cases when the incumbent document's document URI was modified after the document was loaded. Or something along those lines. I don't know that it's worth re-describing in prose the logic of the code for the other edge cases where the two URIs don't match, unless we talk about _why_ they shouldn't match in those cases. Having _that_ information in the comment might be worth it. >+ // No document; determine triggeringPrincipal by quering the "querying" >+ // subjectPrincipal, wich is the principal of the current JS "which". >or a null principal in case there is no >+ // compartment yet. "yet" is an odd term to use here, since it can just as well be "again" or "already".... The current compartment can be nulled out and then restored. But really, this whole comment explaining what the "subject principal" is should just go away from here. It's ok documentation for the SubjectPrincipal() function, not for the callsite. r=me on the actual code change with the comments fixed up. Please do resummarize this bug to reflect what it got mutated to, I guess, since the current summary has nothing to do with this patch.

Comment on attachment 8770135 [details] [diff] [review] bug_1181370_change_location_behavior_tests.patch >+++ b/docshell/test/navigation/file_triggeringprincipal_frame_1.html The indentation inside receiveMessage is broken. Please fix. >+++ b/docshell/test/navigation/test_triggeringprincipal_frame_nav.html >+ * (*) Frame1 and Subframe set it's document.domain to mochi.test s/it's/their/? >+ * (*) TriggeringPrincipal for the SubFrame navigation should be "Subframe", and same later in the same comment. >+const TRIGGERINGPRINCIPALURI = Might be clearer to define this in terms of BASEURL1. >+const LOADINGPRINCIPALURI = And same here, in terms of BASEURL2. >+ is(event.data.triggeringPrincipalURI, >+ TRIGGERINGPRINCIPALURI, Weird indent. Also in the next is() statement. r=me with those nits.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b0a671de425 Update CheckURL to set the incumbent document's principal as the triggeringPrincipal. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/6d182ecbd61e Test that triggeringPrincipal(owner) and referrer are identical for (sub) iframe navigations. r=bz

https://hg.mozilla.org/mozilla-central/rev/1b0a671de425 https://hg.mozilla.org/mozilla-central/rev/6d182ecbd61e

I think we should rename this bug, since it fixes a subset of the triggeringPrincipal issues

I did ask for that in comment 37 as part of my review comments.... Too bad it didn't happen. But the new summary is _still_ wrong. This bug had absolutely nothing to do with frame navigations.

(In reply to Boris Zbarsky [:bz] from comment #42) > I did ask for that in comment 37 as part of my review comments.... Too bad > it didn't happen. Boris, I didn't ignore your suggestion, but I updated the commit message to: > Update CheckURL to set the incumbent document's principal as the triggeringPrincipal. r=bz instead of updating the summary. I agree that the summary of this bug is now more aligned with what we are actually changing - sorry about that.

I didn't say you ignored my suggestion. Just that the summary of this bug never got changed, for whatever reason...