Temporarily fall back to SystemPrincipal if History entry does not have a valid triggeringPrincipal

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment)

Bug 1334875 seems intermittent and hard to reproduce. Completely backing out Bug 1307736 seems not like a good way forward. After discussing with Tanvi over IRC we both think the best way forward is to keep the assertion within LoadHistoryEntry within Bug 1307736 but revert to the old semantics and still fall back to the SystemPrincipal if triggeringPrincipal is a nullptr.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8829576&action=diff
(Assignee)

Updated

a year ago
Assignee: nobody → ckerschb
Blocks: 1307736
Priority: -- → P1
Whiteboard: [domsecurity-active]
(Assignee)

Updated

a year ago
Blocks: 1334875
(Assignee)

Comment 1

a year ago
Created attachment 8834724 [details] [diff] [review]
bug_1337622_fall_back_systemprincipal_history_loads.patch

Tanvi, Boris, as explained in comment 0, we could make the hard fail within LoadHistoryEntry a soft fail. Which means, we keep the assertion but fall back to using the SystemPrincipal for history loads not having a triggeringPrincipal. Which "basically" means we temporarily go back to using the old semantics before Bug 1307736 landed [1].

To make sure we are on the same page, I hope that fixes the problem. I would imagine that if the history load would have a valid principal and the history load would be blocked because of security checks, then we would receive an error message in the console, which apparently is not happening. Hence I think the problem must be that we return an error from LoadHistoryEntry if we don't receive a valid principal.

Bug 1334875 seems intermittent and hard to reproduce. Kamil is going to take a look at Bug 1334875 and tries to find reliable STRs. If we can find STRs we can still try to get the actual problem fixed and uplifted. As a first measure I propose to get this patch landed.

What do you think? If Kamil can't find any STRs and you don't feel comfortable landing this patch, I suppose we have to bite the bullet and back out Bug 1307736.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8829576&action=diff
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1334875#c12
Attachment #8834724 - Flags: review?(tanvi)
Attachment #8834724 - Flags: review?(bzbarsky)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Comment on attachment 8834724 [details] [diff] [review]
bug_1337622_fall_back_systemprincipal_history_loads.patch

r=me.  Thank you for picking this up!
Attachment #8834724 - Flags: review?(bzbarsky) → review+
Comment on attachment 8834724 [details] [diff] [review]
bug_1337622_fall_back_systemprincipal_history_loads.patch

Let's try hard to find the root cause of this and stop falling back to systemprincipal in FF 53+.
Attachment #8834724 - Flags: review?(tanvi) → review+
(Assignee)

Comment 4

a year ago
(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #3)
> Comment on attachment 8834724 [details] [diff] [review]
> bug_1337622_fall_back_systemprincipal_history_loads.patch
> 
> Let's try hard to find the root cause of this and stop falling back to
> systemprincipal in FF 53+.

Yes, I already filed Bug 1338009 to make debugging easier which will help us find the root cause. thanks!
Keywords: checkin-needed
(Assignee)

Comment 5

a year ago
Comment on attachment 8834724 [details] [diff] [review]
bug_1337622_fall_back_systemprincipal_history_loads.patch

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1307736 - Assert history loads pass a valid triggeringPrincipal for docshell loads

[User impact if declined]:
We haven't identified the root cause yet, but it seems that intermittently some users might not be able to restore sessions (See Bug 1334875 - page not loading on session restoration).

[Is this code covered by automated tests?]:
Nope, but instead of failing hard if the sessions history entry does not have a valid triggeringPrincipal, we got back to all semantics and fall back to using the systemPrincipal which will *always* allow the history entry to load.

[Has the fix been verified in Nightly?]:
Not yet - Kamil is on that. As soon as he has verified that, he will post a comment here.

[Needs manual test from QE? If yes, steps to reproduce]:
Still having trouble to reproduce. See Bug 1334875.

[List of other uplifts needed for the feature/fix]:
none

[Is the change risky?]:
No, in fact it reverts a hard fail to a soft fail, which means we keep the MOZ_ASSERT if a history load does not have a valid triggeringPrincipal which will help us to find the root cause, but fall back to the systemPrincipal for the actual load.

[Why is the change risky/not risky?]:
see above.

[String changes made/needed]:
no
Attachment #8834724 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 6

a year ago
Kamil, once you have found reliable STRs to reproduce Bug 1334875, can you please verify that this patch fixed the issue?
Flags: needinfo?(kjozwiak)

Comment 7

a year ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39a912717bb6
Temporarily fall back to SystemPrincipal if History entry does not have a valid triggeringPrincipal. r=tanvi,bz
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39a912717bb6
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Modified from Uplift Dashboard.
status-firefox53: --- → affected
Comment on attachment 8834724 [details] [diff] [review]
bug_1337622_fall_back_systemprincipal_history_loads.patch

Use old semantics to help find root cause. Aurora53+.
Attachment #8834724 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 11

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6e2ea518c2b2
status-firefox53: affected → fixed
(Assignee)

Comment 12

a year ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Kamil, once you have found reliable STRs to reproduce Bug 1334875, can you
> please verify that this patch fixed the issue?

Clearing my ni? queue at the moment. Given that no more issues arised in the past 4 months I am clearing my ni? for Kamil.
(Assignee)

Updated

a year ago
Flags: needinfo?(kjozwiak)
You need to log in before you can comment on or make changes to this bug.