Have loadURI provide the correct triggeringPrincipal

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: jkt, Assigned: jkt)

Tracking

(Blocks 1 bug)

62 Branch
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(11 attachments)

46 bytes, text/x-phabricator-request
surkov
: review+
Details | Review
46 bytes, text/x-phabricator-request
mossop
: review+
Details | Review
46 bytes, text/x-phabricator-request
mossop
: review+
Details | Review
46 bytes, text/x-phabricator-request
bgrins
: review+
Details | Review
46 bytes, text/x-phabricator-request
bzbarsky
: review+
Details | Review
46 bytes, text/x-phabricator-request
Nika
: review+
Details | Review
46 bytes, text/x-phabricator-request
dbaron
: review+
Details | Review
46 bytes, text/x-phabricator-request
Gijs
: review+
Details | Review
46 bytes, text/x-phabricator-request
kmag
: review+
Details | Review
46 bytes, text/x-phabricator-request
mossop
: review+
Details | Review
52 bytes, text/x-github-pull-request
Details | Review
Assignee

Description

10 months ago
loadURI in browser.js doesn't throw if we don't have a valid triggeringPrincipal.

Comment 11

10 months ago
Comment on attachment 9004920 [details]
Bug 1485305 - misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs

:Gijs (he/him) has approved the revision.
Attachment #9004920 - Flags: review+
Comment on attachment 9004907 [details]
Bug 1485305 - accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov

alexander :surkov (:asurkov) has approved the revision.
Attachment #9004907 - Flags: review+
Comment on attachment 9004915 [details]
Bug 1485305 - devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins

Brian Grinstead [:bgrins] has approved the revision.
Attachment #9004915 - Flags: review+
Comment on attachment 9004909 [details]
Bug 1485305 - browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Dave Townsend [:mossop] has approved the revision.
Attachment #9004909 - Flags: review+
Comment on attachment 9004923 [details]
Bug 1485305 - toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Dave Townsend [:mossop] has approved the revision.
Attachment #9004923 - Flags: review+
Comment on attachment 9004917 [details]
Bug 1485305 - dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika

:Nika Layzell has approved the revision.
Attachment #9004917 - Flags: review+
Comment on attachment 9004908 [details]
Bug 1485305 - browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Dave Townsend [:mossop] (he/him) has approved the revision.
Attachment #9004908 - Flags: review+
Comment on attachment 9004921 [details]
Bug 1485305 - toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Kris Maglione [:kmag] has approved the revision.
Attachment #9004921 - Flags: review+
Assignee

Comment 19

10 months ago
Chris please could you take a look over this now that you are back :)

Thanks!
Flags: needinfo?(ckerschb)
(In reply to Jonathan Kingston [:jkt] from comment #19)
> Chris please could you take a look over this now that you are back :)

Overall this looks pretty good. I haven't looked at any of the tests because it seems they mostly rely on BrowserTestUtils anyway and we have covered that in other bugs/patches. Anyway, leaving a few notes here:

* Patch: "browser/ tests Ensure loadURI always passes a triggeringPrincipal()"
Within browser/ you sometimes use SystemPrincipal and sometimes you use a NullPrincipal, I couldn't figure out when you used which one.
Why not always use NullPrincipal? It seems it should work?

* Patch: devtools/ Ensure loadURI always passes a triggeringPrincipal()
Within devtools/client/responsive.html/browser/swap.js you use systemPrincipal for the ContainerURL which seems scary, can't we use NullPrincipal in that case? In addition I think it needs the right userContextId.

Additonal note: Within web-navigation.js there is a function loadURIWithOptions where we fall back to using a NullPrincipal in case the triggeringPrincipal is not passed in. Is it worth making sure that this function always receives a non-null triggeringPrincipal or is the fallback fine?

*Patch: dom/ Ensure loadURI always passes a triggeringPrincipal()
Why are we falling back to using SystemPrincipal within toolkit/actors/WebNavigationChild.jsm? That doesn't seem right to me.
Also toolkit/components/extensions/ExtensionXPCShellUtils.jsm uses SystemPrincipal within loadURI, that seems scary to me as well.
Flags: needinfo?(ckerschb)
Assignee

Comment 21

9 months ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> 
> * Patch: "browser/ tests Ensure loadURI always passes a
> triggeringPrincipal()"
> Within browser/ you sometimes use SystemPrincipal and sometimes you use a
> NullPrincipal, I couldn't figure out when you used which one.
> Why not always use NullPrincipal? It seems it should work?

- For data urls to simlify checking I used System rather than creating a codebase etc.
- I see many tests loading pages like about:preferences or other system pages which wouldn't be openable via the web.
- Mostly I have used System in tests to reduce the amount of checking for these cases.
- When loading about:blank or other explicit web URLs, I explicitly used NullPrincipal as it's clear it will always work. (The intent is to safely land these patches with minimal rework, overall even the test SystemPrincipals we can hopefully remove where possible).

> * Patch: devtools/ Ensure loadURI always passes a triggeringPrincipal()
> Within devtools/client/responsive.html/browser/swap.js you use
> systemPrincipal for the ContainerURL which seems scary, can't we use
> NullPrincipal in that case? In addition I think it needs the right
> userContextId.

The current use to swapToInnerBrowser is currently hard coded uses to links like: "chrome://devtools/content/responsive.html/index.xhtml"

> 
> Additonal note: Within web-navigation.js there is a function
> loadURIWithOptions where we fall back to using a NullPrincipal in case the
> triggeringPrincipal is not passed in. Is it worth making sure that this
> function always receives a non-null triggeringPrincipal or is the fallback
> fine?
 
I think we should do this as a follow up also.

> *Patch: dom/ Ensure loadURI always passes a triggeringPrincipal()
> Why are we falling back to using SystemPrincipal within
> toolkit/actors/WebNavigationChild.jsm? That doesn't seem right to me.

This is due to things like SessionRestore where the triggering principal can't be unserialized. I will file a follow up to this as I agree but it broke a fair number of things.

> Also toolkit/components/extensions/ExtensionXPCShellUtils.jsm uses
> SystemPrincipal within loadURI, that seems scary to me as well.

I can't remember the exact failures here but again I'll file a bug and add it to the comment.
Assignee

Comment 22

9 months ago
Hey :smaug / :bz, Not sure if you got a notification for https://phabricator.services.mozilla.com/D4556 are you able to review or give to someone else? Thanks!

> https://phabricator.services.mozilla.com/D4560 adds the default of systemPrincipal for BrowserTestUtils.
> https://phabricator.services.mozilla.com/D4551 adds the requirement in browser.js that a triggeringPrincipal is explicitly passed.
> The motivation is to pass an explicit triggeringPrincipal to all loads going into docshell so we can stop using an implied systemPrincipal as a default in the docshell code.

Hey :heycam / :dbaron are you able to review https://phabricator.services.mozilla.com/D4558 also, I put in the mentioned fixes.

Sorry for pinging you all, just these patches bitrot quite fast and I would like to get them in early into the cycle to fix any fallout it causes. Thanks in advance!
Flags: needinfo?(dbaron)
Flags: needinfo?(cam)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
This is not in my phabricator review queue.
Flags: needinfo?(bugs)
I got notifications, but I've been on a bunch of unexpected PTO over the last few weeks so I'm way behind.  Looking now.
Flags: needinfo?(bzbarsky)
Comment on attachment 9004916 [details]
Bug 1485305 - docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.
Attachment #9004916 - Flags: review+
Comment on attachment 9004918 [details]
Bug 1485305 - layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.
Attachment #9004918 - Flags: review+

Comment 27

9 months ago
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6263695b3cb8
accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee48378f73
browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/03632075ac2c
browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/f40900bf8621
devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/35bd32f99f60
docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/e02038177b6b
dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9da73786c5f
layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/721871bb64f1
misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0f2adb765e
toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/63c50fd60ae4
toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Comment 29

9 months ago
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb33bf25d6f
accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/bca973d90acc
browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/61aa1cfb0b01
browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a111e9e5c9c
devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaccb374a59
docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/8085d1eefd7c
dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8a5a03ccb5
layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd063d8afe4e
misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f04aeeeef7
toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/138b8596a9cd
toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
Assignee

Updated

9 months ago
Flags: needinfo?(jkt)
Flags: needinfo?(dbaron)
Flags: needinfo?(cam)

Comment 31

9 months ago
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/238d92348159
accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5c9d5929fc
browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b74152cabc1
browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/323773a395cc
devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/acce14683c13
docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/e698f2fc1c1a
dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/75220b2f6669
layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c99b97b4348b
misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/a05e40ef7215
toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/50439ec01661
toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Comment 33

9 months ago
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9840fe72bb5
accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ee5b2a7bf2
browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b125055e5e0
browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bac78c8df5
devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/55115a3dfb82
docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/542c2d7a2c1d
dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika
https://hg.mozilla.org/integration/mozilla-inbound/rev/a253b24f69b2
layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1da667c41b3
misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc21f29f364b
toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop
https://hg.mozilla.org/integration/mozilla-inbound/rev/c249c91dc8ca
toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Updated

9 months ago
See Also: → 1492777
Assignee

Updated

9 months ago
Flags: needinfo?(jkt)

Comment 36

9 months ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/fc1be991a4d06316edb0e588ae98f7b7577fa1b9
Port Bug 1485305 - browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop (#4445)
You need to log in before you can comment on or make changes to this bug.