1488289 - Figure out firstPartyDomain policy for null principal'd about:blank and friends

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

Background: I'm trying to land some form of the patch in bug 1397365 to turn off initial about:blank creation for the first/initial browser in browser.xul, thereby fixing bug 1362774, which should have some performance benefits. To pave the way for this, I'm adjusting tests that explicitly or implicitly depend on the initial about:blank document being loaded how it is now.


One of these (sets of) tests is/are

browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js 

and

browser/components/originattributes/test/browser/browser_firstPartyIsolation_js_uri.js

I've read and re-read the comments in bug 1397365 (esp. bug 1397365 comment 60) and I'm... confused.

First off, with the patch as-is ( https://hg.mozilla.org/try/rev/2e3207a1ea6f9469ffc9f00fd08a410d40663890 ) the docshell leak log for the first test looks like this:

 0:04.63 GECKO(36840) [Child 36844: Main Thread]: D/nsDocShellLeak DOCSHELL 0x10e648800 created
 0:04.68 GECKO(36840) [Child 36844: Main Thread]: D/nsDocShellLeak DOCSHELL 0x10e648800 SetCurrentURI about:blank
 0:04.79 GECKO(36840) [Child 36844: Main Thread]: D/nsDocShellLeak nsDocShell[0x10e648800]: loading about:blank with flags 0x00000000
 0:04.79 GECKO(36840) [Child 36844: Main Thread]: D/nsDocShellLeak DOCSHELL 0x10e648800 InternalLoad about:blank
 0:04.79 GECKO(36840) [Child 36844: Main Thread]: D/nsDocShellLeak DOCSHELL 0x10e648800 SetCurrentURI about:blank
 0:04.84 PASS should be a remote browser - true == true - 
 0:04.85 INFO origin moz-nullprincipal:{991c9baa-276d-0a45-b679-ce7e681e29a8}^firstPartyDomain=about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla
 0:04.85 PASS The principal of remote about:blank should be a NullPrincipal. - true == true - 
 0:04.85 FAIL remote about:blank should have firstPartyDomain set to 991c9baa-276d-0a45-b679-ce7e681e29a8.mozilla - "about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla" == "991c9baa-276d-0a45-b679-ce7e681e29a8.mozilla" - 
Stack trace:
resource://testing-common/content-task.js line 59 > eval:null:11
 0:04.85 INFO Leaving test bound test_remote_window_open_aboutBlank

So there are no longer 2 docloads - just 1 call to InternalLoad, that no longer has a LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL .


My understanding is that firstPartyDomain is supposed to reflect the "first party" that is loading/framing the document, so that we can have per-first-party cookie jars and so on. For data: and about:blank loads I would expect this to mean that the first party depends on what loads them - if `mozilla.org` loads them, that's going to be their 1st party domain. If a page somehow forces a user to open such a page as a toplevel page without inheriting principals, I expect it to be a null principal.

I assume that browser_firstPartyIsolation_aboutPages.js is supposed to be testing that last case.

The problem is that it does it by relying on the initial about:blank load in a new browser window. Almost by definition, that load isn't controlled by a webpage. It would be if it was triggered by a webpage, or a link on a webpage or anything like that, but it isn't here. It's just a test wrapper that creates a new browser window, which (until we fix bug 1362774 / bug 1397365) happens to create an about:blank content doc in the initial browser.


I'm guessing that it would be fine to adjust the test to more purposefully create the type of content document it cares about. The problem is... I'm not entirely sure how to do that. That is, I can trivially create such a document by calling `loadURI` with the right URL + load flags. But I assume we care about such a document being created by web content, except the "whole point" of this exercise is to figure out the initiating "first party" at all times, so I would assume that we do all we can to make it impossible for web content to ever land itself in this situation.

What am I missing? What's a better way of creating the type of null principal'd document we care about here (ie with no first party) ? Olli, can you help?

Comment 1

[:Gijs (he/him)]

Resolved via IRC:

<@smaug> Gijs: ok, so I guess the test could just explicit rely on fpd to be about.ef2a7dd5-93bc-417f-a698-142c3116864f.mozilla
<@smaug> without your changes it is testing null principal
<Gijs> right.
<Gijs> smaug: OK. What about the js URI case in the other test file?
<@smaug> with generated null principal fpd
<Gijs> smaug: should we have a separate test for the generated null principal fpd?
<Gijs> or do we already test that in other test files?
<@smaug> there are other tests
<@smaug> Gijs: so in js test, are the tests loading nothing
<Gijs> smaug: I expect they are running JS that inherits the same about: principal.
<Gijs> smaug: and therefore that fPD.
<@smaug> yeah, that makes sense
<@smaug> checking that about fpd is there is ok
<Gijs> ok!

I'll put up a patch and get review here, and then land it together with everything else in bug 1362774.

Comment 2

[:Gijs (he/him)]

Attachment: Bug 1488289 - update firstPartyDomain expectations once we avoid loading the initial about:blank document with nodefaultsrc, r?smaug

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9007281 [details]
Bug 1488289 - update firstPartyDomain expectations once we avoid loading the initial about:blank document with nodefaultsrc, r?smaug

Olli Pettay [:smaug] has approved the revision.

Comment 4

[:Gijs (he/him)]

This needs changing now, because bug 1393570 changed how we inherit principals, which broke the patch here.

The js_uri parts are now unnecessary, and if I apply the patch from bug 1362774, the non-remote about: case (in browser_firstPartyIsolation_aboutPages.js ) starts behaving like the remote case does pre-patch (ie null principal rather than the about: codebase principal). I think that's fine, but I'd like to understand the "why" better. Rob, any suggestions would be appreciated, otherwise I'll try to dig into this tomorrow.

Comment 5

[Rob Wu [:robwu]]

The relevant patches of bug 1393570 are:
https://hg.mozilla.org/mozilla-central/rev/c42695f1399c
https://hg.mozilla.org/mozilla-central/rev/c37ea7ae71ef

The change is that gBrowser.loadTabs has a new option to toggle principal inheritance, defaulting to not inheriting [1].
This is used when a new browser window is opened, with an array of URLs as first argument [2]. This options was added because I wanted to ensure that about:blank windows continued to have a null principal even if triggeringPrincipal is set (previously it was the system principal).

Your test uses BrowserTestUtils.openNewBrowserWindow to open a new window [3], which opens a window with a string argument, which also ends up calling loadTabs, but always with a system principal [4].

I assumed that the system principal would always be converted to a null principal, but apparently that is not always true.
I think that the "answer" to the "why" question can be found here:
https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/docshell/base/nsDocShell.cpp#873-951

If this is the case, please explain the "why", since I haven't figured out what is happening in this case (I got stuck on trying to imagine what "current document" in the above link means in the context of your test [3]).


[1]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/tabbrowser.js#1464-1466
[2]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser.js#1701-1702

[3]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/components/originattributes/test/browser/browser_firstPartyIsolation_aboutPages.js#41
[4]: https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/browser/base/content/browser.js#1739

Comment 6

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #5)
> The relevant patches of bug 1393570 are:
> https://hg.mozilla.org/mozilla-central/rev/c42695f1399c
> https://hg.mozilla.org/mozilla-central/rev/c37ea7ae71ef
> 
> The change is that gBrowser.loadTabs has a new option to toggle principal
> inheritance, defaulting to not inheriting [1].
> This is used when a new browser window is opened, with an array of URLs as
> first argument [2]. This options was added because I wanted to ensure that
> about:blank windows continued to have a null principal even if
> triggeringPrincipal is set (previously it was the system principal).
> 
> Your test uses BrowserTestUtils.openNewBrowserWindow to open a new window
> [3], which opens a window with a string argument, which also ends up calling
> loadTabs, but always with a system principal [4].

This is the difference - it doesn't [call loadTabs, right now]. Specifically, right now, https://searchfox.org/mozilla-central/rev/a7f4d3ba4fbfe3efbde832869f1d672fce7122f6/browser/base/content/browser.js#1680-1684

avoids calling loadTabs() if the URL to load in the initial tab is about:blank. The about:blank load is triggered by the docshell creating an initial document in itself when something attempts to access the content window/document.

The patch for bug 1362774 removes that check because if a callsite actually wants to load about:blank, we should load it.

So the difference here is caused by the fact that prior to the patch, the load happens via docshell, apparently with allowInheritPrincipal (which makes sense - if we're creating an *initial* about:blank, we should be inheriting something for that, at least in a web context), and we then inherit something. 

After the patch, we indeed take the loadTabs() route, and we convert to a null principal because we pass DISALLOW_INHERIT_PRINCIPAL - so the remote and non-remote case come into sync, which is a Good Thing.

> I assumed that the system principal would always be converted to a null
> principal, but apparently that is not always true.
> I think that the "answer" to the "why" question can be found here:
> https://searchfox.org/mozilla-central/rev/
> eef79962ba73f7759fd74da658f6e5ceae0fc730/docshell/base/nsDocShell.cpp#873-951
> 
> If this is the case, please explain the "why", since I haven't figured out
> what is happening in this case (I got stuck on trying to imagine what
> "current document" in the above link means in the context of your test [3]).

I hope this is sufficient explanation. Thanks for the pointers to the docshell code, it made adding a bunch of printfs here and figuring out what happened more straightforward than it would have been otherwise.

I'll obsolete the patch here, and put up a new one for smaug to stamp.

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1488289 - fix non-remote about:blank firstPartyDomain stuff, r?smaug

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0d3f3133db
fix non-remote about:blank firstPartyDomain stuff, r=smaug

Comment 9

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4a0d3f3133db