The default bug view has changed. See this FAQ.

Assert history loads pass a valid triggeringPrincipal for docshell loads

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
6 months ago
17 days ago

People

(Reporter: ckerschb, Assigned: ckerschb, NeedInfo)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 attachments, 15 obsolete attachments)

3.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
7.39 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
92.03 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

6 months ago
Blocks: 1182569
Priority: -- → P2
Whiteboard: [domsecurity-active]
(Assignee)

Updated

6 months ago
Assignee: nobody → ckerschb
(Assignee)

Comment 1

6 months ago
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
Status: NEW → ASSIGNED
Hi Chris,

Any update on this bug?
Flags: needinfo?(ckerschb)
(Assignee)

Comment 3

5 months ago
Created attachment 8808773 [details] [diff] [review]
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
Flags: needinfo?(ckerschb)
(Assignee)

Comment 4

5 months ago
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=19dcbb7c05ad7b1c37e18b477e42d30c7e58bda2
(Assignee)

Comment 5

5 months ago
Created attachment 8809417 [details] [diff] [review]
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.
Attachment #8808773 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 6

5 months ago
Created attachment 8809418 [details] [diff] [review]
bug_1307736_test_updates.patch
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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 8

4 months ago
Created 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 #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?
Attachment #8809417 - Attachment is obsolete: true
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 10

3 months ago
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?
Flags: needinfo?(bzbarsky)
[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.
tracking-firefox53: --- → ?
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.
Flags: needinfo?(bzbarsky)
Tracking 53+ per Comment 11.
status-firefox53: --- → affected
tracking-firefox53: ? → +
(Assignee)

Comment 14

2 months ago
Created attachment 8826197 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch
Attachment #8809838 - Attachment is obsolete: true
Attachment #8826197 - Flags: review?(bzbarsky)
(Assignee)

Comment 15

2 months ago
Created attachment 8826198 [details] [diff] [review]
bug_1307736_test_updates.patch
Attachment #8809418 - Attachment is obsolete: true
Attachment #8826198 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a3caf06f676954f453d7bd0cc90113f90ed6610
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...
Attachment #8826197 - Flags: review?(bzbarsky) → review+
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?
Attachment #8826198 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 19

2 months ago
Created attachment 8826508 [details] [diff] [review]
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.
Attachment #8826197 - Attachment is obsolete: true
Attachment #8826508 - Flags: review+
(Assignee)

Comment 20

2 months ago
Created attachment 8826509 [details] [diff] [review]
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.
Attachment #8826198 - Attachment is obsolete: true
Attachment #8826509 - Flags: review?(mdeboer)
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 ?
Attachment #8826509 - Flags: review?(mdeboer) → review-
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.
(Assignee)

Comment 23

2 months ago
Created attachment 8827069 [details] [diff] [review]
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.
Attachment #8826508 - Attachment is obsolete: true
Attachment #8827069 - Flags: review+
(Assignee)

Comment 24

2 months ago
Created attachment 8827070 [details] [diff] [review]
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.
Attachment #8826509 - Attachment is obsolete: true
Attachment #8827070 - Flags: review?(mdeboer)
(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 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.
Attachment #8827070 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 27

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

2 months ago
Pushed by mozilla@christophkerschbaumer.com:
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
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
(Assignee)

Comment 30

2 months ago
Created attachment 8827272 [details] [diff] [review]
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.
Attachment #8827070 - Attachment is obsolete: true
Attachment #8827272 - Flags: review+
(Assignee)

Comment 31

2 months ago
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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 32

2 months ago
(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
Flags: needinfo?(gijskruitbosch+bugs)
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);
>
(Assignee)

Updated

2 months ago
Depends on: 1332310
(Assignee)

Comment 34

2 months ago
(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.
(Assignee)

Comment 35

2 months ago
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
(Assignee)

Comment 36

2 months ago
Clearing flags - see comment 35 for details.
status-firefox53: affected → ---
tracking-firefox53: + → ---
(Assignee)

Comment 37

2 months ago
Created attachment 8829576 [details] [diff] [review]
bug_1307736_assert_history_loads_pass_valid_triggeringPrincipal.patch

Rebasing patches since Bug 1332310 landed. Carrying over r+ from bz.
Attachment #8827069 - Attachment is obsolete: true
Attachment #8829576 - Flags: review+
(Assignee)

Comment 38

2 months ago
Created attachment 8829577 [details] [diff] [review]
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.
Attachment #8827272 - Attachment is obsolete: true
Attachment #8829577 - Flags: review+
(Assignee)

Comment 39

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

Comment 40

2 months ago
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

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

Comment 42

2 months ago
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
(Assignee)

Comment 43

2 months ago
No need for an additional ni? besides the feedback. Clearing the flag - thanks!
\
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 44

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1564de9d00a544f0f41ea755bf4cbad1a7a52db5
(Assignee)

Comment 45

2 months ago
Created attachment 8830646 [details] [diff] [review]
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
Attachment #8829581 - Attachment is obsolete: true
Attachment #8830646 - Flags: review?(mdeboer)
(Assignee)

Comment 46

2 months ago
Created attachment 8830647 [details] [diff] [review]
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!
Attachment #8829577 - Attachment is obsolete: true
Attachment #8830647 - Flags: review?(mdeboer)
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.
Attachment #8830646 - Flags: review?(mdeboer) → review-
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!
Attachment #8830647 - Flags: review?(mdeboer)
(Assignee)

Comment 49

2 months ago
(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!
(Assignee)

Comment 50

2 months ago
Created attachment 8830855 [details] [diff] [review]
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.
Attachment #8830646 - Attachment is obsolete: true
Attachment #8830855 - Flags: review?(mdeboer)
(Assignee)

Comment 51

2 months ago
Created attachment 8830856 [details] [diff] [review]
bug_1307736_test_updates.patch
Attachment #8830647 - Attachment is obsolete: true
Attachment #8830856 - Flags: review?(mdeboer)
Comment on attachment 8830855 [details] [diff] [review]
bug_1307736_update_session_management.patch

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

Very nice, thanks!
Attachment #8830855 - Flags: review?(mdeboer) → review+
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.
Attachment #8830856 - Flags: review?(mdeboer) → review+
(Assignee)

Comment 54

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

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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f6379ae84275
https://hg.mozilla.org/mozilla-central/rev/d3b4dbb64632
https://hg.mozilla.org/mozilla-central/rev/eaf9152703d8
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 57

2 months ago
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!
Flags: needinfo?(tanvi)
Depends on: 1334875
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
Flags: needinfo?(tanvi)
Attachment #8829576 - Flags: approval-mozilla-aurora?
Comment on attachment 8830855 [details] [diff] [review]
bug_1307736_update_session_management.patch

Approval Request Comment
See comment 58.
Attachment #8830855 - Flags: approval-mozilla-aurora?
Comment on attachment 8830856 [details] [diff] [review]
bug_1307736_test_updates.patch

Approval Request Comment
See comment 58.
Attachment #8830856 - Flags: approval-mozilla-aurora?

Comment 61

2 months ago
Modified from Uplift Dashboard.
status-firefox53: --- → affected

Comment 62

2 months ago
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
Attachment #8830856 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8829576 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8830855 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 63

2 months ago
bugherderuplift
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
status-firefox53: affected → fixed
Flags: in-testsuite+
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?
Flags: needinfo?(ckerschb)
To give more context. I'm hitting this consistently for pinned tabs.
(Assignee)

Comment 66

2 months ago
(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!
Flags: needinfo?(ckerschb) → needinfo?(hv1989)
Depends on: 1334780
(Assignee)

Updated

2 months ago
Depends on: 1337622

Updated

2 months ago
Depends on: 1336799
Depends on: 1341589
(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.
(Assignee)

Updated

26 days ago
Depends on: 1341754
(Assignee)

Updated

17 days ago
Blocks: 1345433
You need to log in before you can comment on or make changes to this bug.