Open Bug 1240258 Opened 4 years ago Updated 3 years ago

Missing or wrong Owner in nsDocShell

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

REOPENED

People

(Reporter: tanvi, Unassigned)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-backlog])

The "owner" passed to nsDocShell::InternalLoad is often missing or wrong.
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9538

The owner should give us an indication of who triggered the load.  The owner is used for the requestingPrincipal sent to nsIContentPolicy and the triggeringPrincipal set in loadInfo.  It is then used to make security decisions and so we need to get it right.
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9538

We should also rename nsISupports aOwner to nsIPrincipal triggeringPrincipal, but we may do that in a followup bug.

This bug is to:
* identify the places where owner is missing and correct that (ex: by making sure the owner is passed to nsDocShell::AddToSessionHistory() calls  - see https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c36 and https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c30)

* identify the places where owner is wrong and correct that (ex: https://www.evernote.com/shard/s200/sh/88f37f17-eccb-4891-be69-26c17472700b/2939af86018e8eba, https://public.etherpad-mozilla.org/p/docShellPrincipals-subframes, https://bugzilla.mozilla.org/show_bug.cgi?id=1181370#c5)
Depends on: 1240096
One more thing we need to do in this bug:
* assert that the owner and the referrer are same origin.

The reason we have both an owner and a referrerURI is because the referrerURI provides us with path information.  Push state could effect the referrerURI, but doesn't affect the owner.  Since push state only works same origin though, the origin should not change.
Blocks: 1181370
Whiteboard: [domsecurity-backlog]
Let's handle all of the required changes regarding the owner within Bug 1181370.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1181370
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Let's handle all of the required changes regarding the owner within Bug
> 1181370.
> 
> *** This bug has been marked as a duplicate of bug 1181370 ***

Hey Chris,
Does Bug 1181370 cover all cases where owner is missing (ex: reload)?  Perhaps you could assert(aOwner) in nsDocShell:;InternalLoad() and push to try to see if there are failures.  Thanks!
Flags: needinfo?(ckerschb)
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #3)
> Does Bug 1181370 cover all cases where owner is missing (ex: reload)? 
> Perhaps you could assert(aOwner) in nsDocShell:;InternalLoad() and push to
> try to see if there are failures.  Thanks!

Yes, Bug 1181370 will hopefully cover all of those cases. Worst case, we have to file another follow up bug :-(
Flags: needinfo?(ckerschb)
Can you check the reload case?
I would rather leave this open until we can confirm that the issue is really fixed.
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #5)
> Can you check the reload case?

Sure, if you prefer, we can leave this bug open until then - sorry for closing it prematurely.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Chris, can you add a couple assertions?

1) MOZ_ASSERT(owner) here http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1482 and push to try.

2) if(ownerIsExplicit) { MOZ_ASSERT(owner) } and push to try.


Paste the try links and we can see how many failures there are.  You mentioned you may have already done this, and hence may already have the try pushes.

Thanks!
Flags: needinfo?(ckerschb)
(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #8)
> Chris, can you add a couple assertions?

Sure, this bug is on my radar anyway. Let me fix up a few other docshell owner (triggeringprincipal) issues first, then I'll get to this one.
Flags: needinfo?(ckerschb)
This bug was spawned form Bug 1105556.  That bug is huge and too much to read through.  So I've extracted the individual relevant comments.

For more context on the reason this bug was filed see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c18 - part [2]
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c20 - starting with "> A. When we Refresh a page"
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c23
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c24
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c30 (In reply to comment 23 and 24)
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c36
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c38 - number 4)
You need to log in before you can comment on or make changes to this bug.