1307736 - Assert history loads pass a valid triggeringPrincipal for docshell loads

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Important to reference is:
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c23

In summary, it mentions that ctrl+r or also ctrl+shift+R the sequence of calls is:
nsSHistory::Reload
nsSHistory::LoadEntry
nsSHistory::InitiateLoad
nsDocShell::LoadURI
nsDocShell::LoadHistoryEntry
nsDocShell::InternalLoad

It seems that InitateLoad does not pass the triggeringPrincipal or also not the principalToInherit from the session history entry to the docshell loadinfo:
https://dxr.mozilla.org/mozilla-central/source/docshell/shistory/nsSHistory.cpp#1753

Comment 2

[Tanvi Vyas[:tanvi]]

Hi Chris,

Any update on this bug?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

As discussed in our meeting, we should deny the load if the entry does not hold a valid triggeringPrincipal. Let's see how TRY does:

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

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Boris, it seems we can enforce that history entries have a valid triggeringPrincipal by only updating 2 things. I am not entirely sure though if I am using the right triggeringPrincipal in those two cases:

a) nsDocShell::AddState() is the only place that calls AddToSessionHistory() without providing a valid triggeringPrincipal. Looking through the code of AddState() I think using document->NodePrincipal() as the triggeringPrincipal as well as principalToInherit is the right choice.

b) nsDocShell::CreateContentViewer potentially calls onNewURI() where failedChannel is null. Since the argument triggeringPrincipal is always null, we end up with a null triggeringPrincipal on the history entry. E.g. within test browser_moz_action_link.js we fall back to using 'about:blank' since "failedURI" is null. In that case we would end up with a nullptr for failedChannel as well as a an explicit nullptr for triggeringPrincipal. If desired, we could also move the triggeringPrincipal creation to within:

    if (!failedURI) {
      // We need a URI object to store a session history entry, so make up a URI
      NS_NewURI(getter_AddRefs(failedURI), "about:blank");
      triggeringPrincial = nsContentUtils::GetSystemPrincipal();
    }

That approach has the downside that there is potentially a codepath where we still end up with a null failedChannel and a null triggeringPrincipal. Upside of that approach is, that we would know explicitly, but potentially history loads get denied.

Since we are forcing history entries to have a valid triggeringPrincipal, we have to update some tests to provide an explicit triggeringprincipal for those tests that create a history entry.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

Comment 7

[Boris Zbarsky [:bzbarsky]]

Using document->NodePrincipal() for the triggering principal in AddState() makes sense.  I'm not sure it makes sense to use it for the principalToInherit.  It may make more sense to use null for principalToInherit there.  Why would you ever want the result of the AddState() call to inherit the current document's principal on reload?

> nsDocShell::CreateContentViewer potentially calls onNewURI() where failedChannel is null.

This can basically only happen when loading an error page and either the error is "could not create an nsIURI from this URI" or "tried to navigate during printing or print preview".  In either case, we're doing an error page load, and should probably use system principal for the triggering principal.

I don't see how this patch forces session history entries to have a valid triggering principal, though.  Random chrome JS (e.g. in extensions) can just create them without a triggering principal, no?  Asserts won't catch that.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #7)
> I don't see how this patch forces session history entries to have a valid
> triggering principal, though.  Random chrome JS (e.g. in extensions) can
> just create them without a triggering principal, no?  Asserts won't catch
> that.

Agreed, it's hard to enforce that all entries have a valid triggeringPrincipal at creation time of an entry. At least for code within our codebase we have an assertion which allows to pinpoint the actual problem right away instead of waiting for the entry to be used within LoadHistoryEntry() which would then deny the load if the entry does not have a valid triggeringPrincipal. Agreed?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> At least for code within our codebase we have an assertion which allows to pinpoint the actual problem right away

We do?  Where?

Anyway, we can't rely on assertions here, afaict.  We need to actually enforce things somewhere, not just assert them.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8809838 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> > At least for code within our codebase we have an assertion which allows to pinpoint the actual problem right away
> 
> We do?  Where?
> 
> Anyway, we can't rely on assertions here, afaict.  We need to actually
> enforce things somewhere, not just assert them.

Wow, last update on this bug was several weeks ago :-( Let me try to summarize my question real quick:

I agree, we can't enforce that random JS code creates history entries, but we can enforce that a history entry has a valid triggeringPrincipal within LoadHistoryEntry and otherwise abort the load. Within LoadHistoryEntry we don't only assert but we also return an error. Isn't that what we want ultimately?

Comment 11

[Tanvi Vyas[:tanvi]]

[Tracking Requested - why for this release]:

Because https://bugzilla.mozilla.org/show_bug.cgi?id=1182569 landed and I'm worried about missing triggeringPrincipals that fall back to system, please land this in 53.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here....

> we can enforce that a history entry has a valid triggeringPrincipal within LoadHistoryEntry and otherwise abort the load.

Yes, agreed.  I see now that we are in fact doing that in LoadHistoryEntry.

OK, then this seems pretty reasonable.

Comment 13

[Marcia Knous [:marcia]]

Tracking 53+ per Comment 11.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8826197 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

>+        // a triggeringPrincipal for the history entry, e.g. for
>+        // 'about:blank' loads.

I don't see how about:blank could ever end up here, much less without a failedChannel.  What is this comment really trying to say?

>+  if(!triggeringPrincipal) {

Space after "if", please.

> nsSHEntry::SetTriggeringPrincipal(nsIPrincipal* aTriggeringPrincipal)

Could have this return failure on null too, in addition to asserting.

r=me with that comment fixed to make sense...

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8826198 [details] [diff] [review]
bug_1307736_test_updates.patch

r=me, but might be good to have the sessionstore folks review this too.  In any case, I assume you'll fold it with the other patch when landing, to avoid a bad intermediate state, right?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17)
> I don't see how about:blank could ever end up here, much less without a
> failedChannel.  What is this comment really trying to say?

I tried to explain within comment 5 b). Anyway, I moved the creation of the principal exactly after the line where we fall back to about:blank to be more explicit in what case we need the principal fallback.

Carrying over r+ from bz.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #18)
> r=me, but might be good to have the sessionstore folks review this too.

Mike, can you be that person to have a look at those tests to make sure everything is fine?

> In any case, I assume you'll fold it with the other patch when landing, to
> avoid a bad intermediate state, right?

I will land the two patches together, yes.

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8826509 [details] [diff] [review]
bug_1307736_test_updates.patch

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

I'd like to have one more quick look at the final patch. Thanks!

Can you show us a green try build?

::: browser/components/preferences/in-content/tests/browser_bug1184989_prevent_scrolling_when_preferences_flipped.js
@@ +1,3 @@
>  const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>  
> +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});

nit: `const` is fine to use here too.

@@ +1,5 @@
>  const ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
>  
> +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});
> +const SYSTEMPRINCIPAL = Services.scriptSecurityManager.getSystemPrincipal();
> +let triggeringPrincipal = Utils.serializePrincipal(SYSTEMPRINCIPAL);

Please use `var` or `const` in the root scope.

Some comment applies to the other test files ;)

::: browser/components/sessionstore/test/browser_394759_purge.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Components.utils.import("resource://gre/modules/ForgetAboutSite.jsm");
>  
> +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});

Can you put this import and the two constants below at the top of the head.js file in browser/components/sessionstore/test/head.js ?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8826508 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

OK, this is somewhat clearer.

There can certainly be cases in which we have a failedURI but not a failedChannel: any time the NS_NewChannel failed after NS_NewURI succeeded.  I think if we want to set triggeringPrincipal to system exactly when failedChannel is null (which would make sense), then we should do exactly that.  So move the new bit into the "else" of the "if (failedChannel) {" block.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #22)
> I think if we want to set triggeringPrincipal to system exactly when
> failedChannel is null (which would make sense), then we should do exactly
> that.  So move the new bit into the "else" of the "if (failedChannel) {"
> block.

Agreed - carrying over r+ from bz.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> Can you show us a green try build?

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

> > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});
> nit: `const` is fine to use here too.

Updated to use const everywhere in the root scope.

> ::: browser/components/sessionstore/test/browser_394759_purge.js
> > +var {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});
> 
> Can you put this import and the two constants below at the top of the
> head.js file in browser/components/sessionstore/test/head.js ?

Most certainly - thanks for the speedy review.

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=5ad241df12b6df7398f1fe1add59ca3157ece304

Well, that's not entirely green I say ;-)

Comment 26

[Mike de Boer [:mikedeboer]]

Comment on attachment 8827070 [details] [diff] [review]
bug_1307736_test_updates.patch

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

When you moved the constants to head.js, you forgot to remove them from the individual test files. So if you do that, it's likely that you mochitest-bc suites will turn green again.
It usually pays off to run the tests you changed locally for a quick verification before you push them all to Try.

r=me with that change. I don't know why the Marionette tests are timing out - its signature is different from an intermittent.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mike de Boer [:mikedeboer] from comment #26)
> suites will turn green again.
> It usually pays off to run the tests you changed locally for a quick
> verification before you push them all to Try.

Totally agree, just verified browser_394759_purge.js that's where you pointed out I should move it to the head.js.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a71a0d9aa90
Ensure History loads pass valid triggeringPrincipal. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/338dd2c343f8
Ensure tests performing a history load pass valid triggeringPrincipal. r=bz,mdeboer

Comment 29

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/28ace1abfb5c for https://treeherder.mozilla.org/logviewer.html#?job_id=69346504&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=69346532&repo=mozilla-inbound

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

Dunno how come I missed browser/components/sessionstore/test/browser_490040.js. Anyway, I updated this test in the same way we updated all the other tests. I imagine mdeboer is fine with that. Qfolded that change in this patch and carrying over r+ from mdeboer.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Localizing and fixing the other problem is harder, the problem is buried within test_refresh_firefox.py. We are creating a history entry of "about:welcomeback" [1] which does not have a triggeringPrincipal and hence we do not allow the load.

Gijs, you wrote most of that test, any chance you could provide some pointers how I could debug that? I think we must create an nsISHEntry somewhere but only set the URI on it but *not* the triggeringPrincipal and hence we don't allow the load. Any ideas?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/marionette/test_refresh_firefox.py#262

Comment 32

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> Localizing and fixing the other problem is harder, the problem is buried
> within test_refresh_firefox.py. We are creating a history entry of
> "about:welcomeback" [1] which does not have a triggeringPrincipal and hence
> we do not allow the load.
> 
> Gijs, you wrote most of that test, any chance you could provide some
> pointers how I could debug that? I think we must create an nsISHEntry
> somewhere but only set the URI on it but *not* the triggeringPrincipal and
> hence we don't allow the load. Any ideas?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/browser/components/migration/
> tests/marionette/test_refresh_firefox.py#262

It's not going to be a test problem, as it's not the test that does the loading of that file. This is the bottom of the rabbithole: https://dxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionMigration.jsm#61-63 .

I don't know off-hand exactly where we then restore that state, but I suspect this would mean your test might also break loading other chrome URIs from session restore, which might be easier to reproduce and figure out. That is,

1. create new profile
2. set to restore tabs on startup
3. open e.g. about:preferences (the options/prefs) in a tab
4. restart firefox

Comment 33

[Tanvi Vyas[:tanvi]]

Comment on attachment 8827069 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Even if this whole patch with assertions doesn't land in 53, can we just land this part:

>@@ -11988,18 +11996,19 @@ nsDocShell::AddState(JS::Handle<JS::Valu
>     GetCurScrollPos(ScrollOrientation_Y, &cy);
>     mOSHE->SetScrollPosition(cx, cy);
> 
>     bool scrollRestorationIsManual = false;
>     mOSHE->GetScrollRestorationIsManual(&scrollRestorationIsManual);
> 
>     // Since we're not changing which page we have loaded, pass
>     // true for aCloneChildren.
>-    rv = AddToSessionHistory(newURI, nullptr, nullptr, nullptr, true,
>-                             getter_AddRefs(newSHEntry));
>+    rv = AddToSessionHistory(newURI, nullptr, 
>+                             document->NodePrincipal(), // triggeringPrincipal
>+                             nullptr, true, getter_AddRefs(newSHEntry));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     NS_ENSURE_TRUE(newSHEntry, NS_ERROR_FAILURE);
> 
>     // Session history entries created by pushState inherit scroll restoration
>     // mode from the current entry.
>     newSHEntry->SetScrollRestorationIsManual(scrollRestorationIsManual);
>

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas - behind on bugmail [:tanvi] from comment #33)
> Even if this whole patch with assertions doesn't land in 53, can we just
> land this part:
> 
> >-    rv = AddToSessionHistory(newURI, nullptr, nullptr, nullptr, true,
> >-                             getter_AddRefs(newSHEntry));
> >+    rv = AddToSessionHistory(newURI, nullptr, 
> >+                             document->NodePrincipal(), // triggeringPrincipal
> >+                             nullptr, true, getter_AddRefs(newSHEntry));

I filed Bug 1332310 - Update TriggeringPrincipal when adding to session history within docshell.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

As agreed with Tanvi and bz we are going to land a partial fix for this bug within Bug 1332310 [1]. The partial fix is why Tanvi requested tracking for 55 (see comment 33). Hence I will clear the tracking flags for this bug. We will land this bug within 54.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1332310#c5

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Clearing flags - see comment 35 for details.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Rebasing patches since Bug 1332310 landed. Carrying over r+ from bz.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

After the backout of these patches I took a closer look and had to update a few more tests within browser/components/sessionstore/test, but they all follow the same schemata using the triggeringPrincipal defined within head.js. Carrying over r+ from bz and mdeboer.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_update_session_management.patch

(In reply to :Gijs from comment #32)
> I don't know off-hand exactly where we then restore that state, but I
> suspect this would mean your test might also break loading other chrome URIs
> from session restore, which might be easier to reproduce and figure out.
> That is,
> 
> 1. create new profile
> 2. set to restore tabs on startup
> 3. open e.g. about:preferences (the options/prefs) in a tab
> 4. restart firefox

Hey Gijs, I followed those steps and found that we have to update some more code within sessionstore that creates and manipulates session history entries. I am not entirely sure if it's even OK to use the systemPrincipal for those loads. Anyway, at the moment restoring a session from Firefox works again (following your provided STR), but the marionette test test_refresh_firefox.py still fails (see details underneath). Any idea what I can do to debug, or how I can possibly find out where that error occurs? Thanks for your help!


1:04.48 TEST_END: MainThread FAIL, expected PASS
Traceback (most recent call last):
  File "/home/ckerschb/moz/mc/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 166, in run
    testMethod()
  File "/home/ckerschb/moz/mc/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 418, in testReset
    self.checkProfile(hasMigrated=True)
  File "/home/ckerschb/moz/mc/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 288, in checkProfile
    self.checkSession()
  File "/home/ckerschb/moz/mc/browser/components/migration/tests/marionette/test_refresh_firefox.py", line 278, in checkSession
    self.assertSequenceEqual(tabURIs, ["about:blank"] + self._expectedURLs)
AssertionError: Sequences differ: [u'about:robots', u'about:mozi... != ['about:blank', 'about:robots'...

First differing element 0:
about:robots
about:blank

Second sequence contains 1 additional elements.
First extra element 2:
about:mozilla

- [u'about:robots', u'about:mozilla']
?                   -

+ ['about:blank', 'about:robots', 'about:mozilla']

Comment 40

[:Gijs (he/him)]

I've seen that error locally myself but never on infra, and I haven't had time to figure out why it happens (in part because it's hard because marionette is REALLY GOOD at swallowing any kind of logging, it has no debugging infrastructure, and so figuring out the cause is basically like stabbing in the dark while drunk.

I would suggest a trypush, and look more if it happens on infra, which seems unlikely.

Comment 41

[:Gijs (he/him)]

Comment on attachment 8829581 [details] [diff] [review]
bug_1307736_update_session_management.patch

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

This looks vaguely OK... might make sense to also doublecheck with mikedeboer when going for a 'real' r+.

::: browser/components/sessionstore/SessionMigration.jsm
@@ +23,5 @@
>  });
>  
> +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});
> +const SYSTEMPRINCIPAL = Services.scriptSecurityManager.getSystemPrincipal();
> +const serialized_triggeringPrincipal = Utils.serializePrincipal(SYSTEMPRINCIPAL);

This looks unused?

::: browser/components/sessionstore/SessionStore.jsm
@@ +188,5 @@
>    "resource://gre/modules/ViewSourceBrowser.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
>    "resource://gre/modules/AsyncShutdown.jsm");
>  
> +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});

Just Cu.import without assigning or passing a scope object (here and elsewhere).

@@ +190,5 @@
>    "resource://gre/modules/AsyncShutdown.jsm");
>  
> +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});
> +const SYSTEMPRINCIPAL = Services.scriptSecurityManager.getSystemPrincipal();
> +const serialized_triggeringPrincipal = Utils.serializePrincipal(SYSTEMPRINCIPAL);

XPCOMUtils.defineLazyGetter("SYSTEM_TRIGGERING_PRINCIPAL", () => {
  Cu.import("blah/utils.jsm");
  return Utils.serializePrincipal(Services.scriptSecurityManager.getSystemPrincipal());
});

Here and elsewhere.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

Thanks Gijs, it seems because of the latest changes we have to update a lot more tests within browser/components/sessionstore/test. Once I have a complete green try run I have to ask mdeboer to review those again. One step at a time, tests pass locally, let's see what try thinks:

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

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

No need for an additional ni? besides the feedback. Clearing the flag - thanks!
\

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_update_session_management.patch

Hey Mike, after the backout of the patch because of failing marionette tests [1] I took a closer look at what we are trying to accomplish here and I figured that we also have to modify some code within SessionHistory, SessionMigration and SessionStore to make sure we copy/pass around the correct triggeringPrincipal.

Please see green try run here:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1564de9d00a544f0f41ea755bf4cbad1a7a52db5

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307736#c31

Comment 46

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

Initially those tests were already r+, but i was wondering if you want to take another look since I basically updated all the tests within browser/components/sessionstore/test to pass a valid triggeringPrincipal. As you suggested I keep the initial creation of the triggeringPrincipal within browser/components/sessionstore/test/head.js and then use it in the all the tests.

Since this change basically affects all of the tests now I am happy to update whatever concerns you may raise.

Further I changed browser/components/sessionstore/test/browser_restore_cookies_noOriginAttributes.js to use JSON.stringify instead of passing a raw string. Lots of other tests do that and I think it's the only way to pass the needed 'triggeringPrincipal'.

Thanks!

Comment 47

[Mike de Boer [:mikedeboer]]

Comment on attachment 8830646 [details] [diff] [review]
bug_1307736_update_session_management.patch

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

I really like the fact that you define SERIALIZED_SYSTEMPRINCIPAL as a lazy getter, but it would be just more awesome if you'd make it part of sessionstore/Utils.jsm:
```js
XPCOMUtils.defineLazyGetter(this, "SERIALIZED_SYSTEMPRINCIPAL", function() {
  return Utils.serializePrincipal(Services.scriptSecurityManager.getSystemPrincipal());
});

this.Utils = Object.freeze({
  get SERIALIZED_SYSTEMPRINCIPAL() { return SERIALIZED_SYSTEMPRINCIPAL; },
  // ...
});
```

::: browser/components/sessionstore/SessionMigration.jsm
@@ +52,5 @@
>      state.windows = aStateObj.windows.map(function(oldWin) {
>        var win = {extData: {}};
>        win.tabs = oldWin.tabs.map(function(oldTab) {
>          var tab = {};
>          // Keep only titles and urls for history entries

nit: please update this comment.

@@ +54,5 @@
>        win.tabs = oldWin.tabs.map(function(oldTab) {
>          var tab = {};
>          // Keep only titles and urls for history entries
>          tab.entries = oldTab.entries.map(function(entry) {
> +          return {url: entry.url, triggeringPrincipal_base64: entry.triggeringPrincipal_base64, title: entry.title};

nit: now you need to properly format this literal across multiple lines, I think. Or you do:
```js
let {title, triggeringPrincipal_base64, url} = entry;
return {title, triggeringPrincipal_base64, url};
```

@@ +67,5 @@
>        return win;
>      });
>      let url = "about:welcomeback";
>      let formdata = {id: {sessionData: state}, url};
> +    return {windows: [{tabs: [{entries: [{url, triggeringPrincipal_base64: SERIALIZED_SYSTEMPRINCIPAL}], formdata}]}]};

nit: same, formatting needs to change to keep things readable.

::: browser/components/sessionstore/SessionStore.jsm
@@ +631,5 @@
>              if (this._needsRestorePage(state, this._recentCrashes)) {
>                // replace the crashed session with a restore-page-only session
>                let url = "about:sessionrestore";
>                let formdata = {id: {sessionData: state}, url};
> +              state = { windows: [{ tabs: [{ entries: [{url, triggeringPrincipal_base64: SERIALIZED_SYSTEMPRINCIPAL}], formdata }] }] };

nit: please update formatting.

Comment 48

[Mike de Boer [:mikedeboer]]

Comment on attachment 8830647 [details] [diff] [review]
bug_1307736_test_updates.patch

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

::: browser/components/sessionstore/test/head.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +const {Utils} = Cu.import("resource://gre/modules/sessionstore/Utils.jsm", {});

When you've updated the previous patch, you'll have to forward that change to _ALL_ these tests as well :( Terribly sorry about that, but there's no other way I can think of.
You could make it a bit magic by doing `const triggeringPrincipal_base64 = Utils.SERIALIZED_SYSTEMPRINCIPAL;` and be able to do `{ url: "https://example.com", triggeringPrincipal_base64 }` in the tests to make the literals not grow as much.
But perhaps that's too much magic? I don't know. I'll leave it up to you!

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mike de Boer [:mikedeboer] from comment #48)
> You could make it a bit magic by doing `const triggeringPrincipal_base64 =
> Utils.SERIALIZED_SYSTEMPRINCIPAL;` and be able to do `{ url:
> "https://example.com", triggeringPrincipal_base64 }` in the tests to make
> the literals not grow as much.
> But perhaps that's too much magic? I don't know. I'll leave it up to you!

Thanks for the speedy reviews and those suggestions. I will get that 'magic' ready, but it might take some time, but i suppose it's fine if we land that early next week. Thanks again for your help here!

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_update_session_management.patch

I had a few cycles and I think I updated all of your suggestions. If not, please let me know.

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1307736_test_updates.patch

Comment 52

[Mike de Boer [:mikedeboer]]

Comment on attachment 8830855 [details] [diff] [review]
bug_1307736_update_session_management.patch

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

Very nice, thanks!

Comment 53

[Mike de Boer [:mikedeboer]]

Comment on attachment 8830856 [details] [diff] [review]
bug_1307736_test_updates.patch

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

Your changes made this _so_ much easier to review... many thanks! As a hunch, can you add Talos 'other' suite to your try push? There are sessionstore tests in there that I'd like to see working before checkin...

Thanks, Christoph.

::: browser/components/sessionstore/test/browser_aboutSessionRestore.js
@@ +44,1 @@
>    is(url, CRASH_URL, "session was restored correctly");

This is array destructuring from the result of JSON.parse, so I think you can omit this change.

Comment 54

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Mike de Boer [:mikedeboer] from comment #53)
> ::: browser/components/sessionstore/test/browser_aboutSessionRestore.js
> >    is(url, CRASH_URL, "session was restored correctly");
> This is array destructuring from the result of JSON.parse, so I think you
> can omit this change.

Indeed. So, since the changesets within this bug already got backed out once I think I am going all in and run *all* tests on try to make sure everything is fine before checking in:

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

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6379ae84275
Ensure History loads pass valid triggeringPrincipal. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3b4dbb64632
Store triggeringPrincipal with all history entries. r=mdeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf9152703d8
Ensure tests performing a history load pass valid triggeringPrincipal. r=bz,mdeboer

Comment 56

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

https://hg.mozilla.org/mozilla-central/rev/f6379ae84275
https://hg.mozilla.org/mozilla-central/rev/d3b4dbb64632
https://hg.mozilla.org/mozilla-central/rev/eaf9152703d8

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, I think we fixed this bug earlier than we thought we would. Since it's early in the cycle we could uplift the changes. What do you think? If you want to have it uplifted, could you please fill out the form - thanks!

Comment 58

[Tanvi Vyas[:tanvi]]

Comment on attachment 8829576 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Approval Request Comment
[Feature/Bug causing the regression]: 1182569 - convert docshell to use AsyncOpen2
[User impact if declined]: Potential for security bugs.  With a missing triggeringPrincipal, we fall back to systemPrincipal.  Falling back to systemPrincipal means we skip various security checks.  This bug ensures that we pass valid triggeringPrincipals.
[Is this code covered by automated tests?]: Tests included in this bug
[Has the fix been verified in Nightly?]: No; I'm not sure how to verify this as its not user facing.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: Only in that it adds an assertion which could fail in debug builds.  But the failures are what we want to capture and debug in order to find potential security issues.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None

Comment 59

[Tanvi Vyas[:tanvi]]

Comment on attachment 8830855 [details] [diff] [review]
bug_1307736_update_session_management.patch

Approval Request Comment
See comment 58.

Comment 60

[Tanvi Vyas[:tanvi]]

Comment on attachment 8830856 [details] [diff] [review]
bug_1307736_test_updates.patch

Approval Request Comment
See comment 58.

Comment 61

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Modified from Uplift Dashboard.

Comment 62

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8829576 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Given the user impact, let's take it in Aurora53

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f27e061cee9d
https://hg.mozilla.org/releases/mozilla-aurora/rev/664f2764b2be
https://hg.mozilla.org/releases/mozilla-aurora/rev/721ea96b25ca

Comment 64

[Hannes Verschore [:h4writer]]

I'm hitting this on a nightly debug check:
Assertion failure: triggeringPrincipal (need a valid triggeringPrincipal to load from history), at /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:12592

I'm now running debug since https://bugzilla.mozilla.org/show_bug.cgi?id=1331414 makes it possible to have not too many slowdowns by setting javascript.options.jit.full_debug_checks to false.

Is this important to report? What should I include in the report?

Comment 65

[Hannes Verschore [:h4writer]]

To give more context. I'm hitting this consistently for pinned tabs.

Comment 66

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Hannes Verschore [:h4writer] from comment #65)
> To give more context. I'm hitting this consistently for pinned tabs.

Hannes, can you please file a new bug blocking this one with some STRs and assign the bug to me? I am pretty busy right now but I am trying to have a look as soon as possible. Thanks!

Comment 67

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #66)
> (In reply to Hannes Verschore [:h4writer] from comment #65)
> > To give more context. I'm hitting this consistently for pinned tabs.
> 
> Hannes, can you please file a new bug blocking this one with some STRs and
> assign the bug to me? I am pretty busy right now but I am trying to have a
> look as soon as possible. Thanks!

Looks bug 1341589 has been filed for this.

Comment 68

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Hsin-Yi Tsai (55 Regression Engineering support) [:hsinyi] from comment #67)
> Looks bug 1341589 has been filed for this.

Indeed. Clearing my ni? queue at the moment, hence also clearing this ni?.