1485305 - Have loadURI provide the correct triggeringPrincipal

Status: RESOLVED FIXED

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

Description

[Jonathan Kingston [:jkt] he/him]

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

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Depends on D4550

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Depends on D4551

Comment 4

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins

Depends on D4552

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins

Depends on D4555

Comment 6

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika

Depends on D4556

Comment 7

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs

Depends on D4557

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs

Depends on D4558

Comment 9

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Depends on D4560

Comment 10

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1485305 - toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Depends on D4561

Comment 11

[:Gijs (he/him)]

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

:Gijs (he/him) has approved the revision.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 9004907 [details]
Bug 1485305 - accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov

alexander :surkov (:asurkov) has approved the revision.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 9004915 [details]
Bug 1485305 - devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins

Brian Grinstead [:bgrins] has approved the revision.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 9004909 [details]
Bug 1485305 - browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Dave Townsend [:mossop] has approved the revision.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 9004923 [details]
Bug 1485305 - toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Dave Townsend [:mossop] has approved the revision.

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9004917 [details]
Bug 1485305 - dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika

:Nika Layzell has approved the revision.

Comment 17

[Dave Townsend [:mossop]]

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.

Comment 18

[Kris Maglione [:kmag]]

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.

Comment 19

[Jonathan Kingston [:jkt] he/him]

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

Thanks!

Comment 20

[Christoph Kerschbaumer [: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.

Comment 21

[Jonathan Kingston [:jkt] he/him]

(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.

Comment 22

[Jonathan Kingston [:jkt] he/him]

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!

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

This is not in my phabricator review queue.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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.

Comment 25

[Boris Zbarsky [: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.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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 28

[Tiberius Oros[:tiberius_oros]]

Backed out 10 changesets for failures at browser/content/browser.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/64c64fc5e163e3299803d71606582a058e2cf43b

Push with the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=199925981&revision=63c50fd60ae4d343533a4b5541489d706409a230

Failure link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=199926336&revision=63c50fd60ae4d343533a4b5541489d706409a230

Relevant log file: https://treeherder.mozilla.org/logviewer.html#?job_id=199926336&repo=mozilla-inbound&lineNumber=9972

Comment 29

[Pulsebot]

Pushed by [e-mail]:
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

Comment 30

[Arthur Iakab [arthur_iakab]]

Backed out 10 changesets (bug 1485305)for failing browser chrome tests on browser_loadDisallowInherit.js
Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/95a55456863eb47670c06465b331188a403b474d
Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=199982956&revision=138b8596a9cd09c3614279a8a3d24561b07c3121
And more exactly the bc3 failures started from revision https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaccb374a5922905f56f1e76dd5e676b22dda01
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199982956&repo=mozilla-inbound for bc3. This failure migrated on bc1,bc6,bc7

:jkt could you please take a look?

Comment 31

[Pulsebot]

Pushed by [e-mail]:
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 32

[Cristian Brindusan [:cbrindusan]]

Backed out 10 changesets (Bug 1485305) for browser-chrome failures on docshell/test/browser/browser_loadURI.js.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4824d6f1357859bc4a374433ab282d7741c28b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=success,testfailed,busted,exception&revision=50439ec0166116f4d6e80e1af2616be1cf22e95e&selectedJob=200195358

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=200195358&repo=mozilla-inbound&lineNumber=3374

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d9840fe72bb5
https://hg.mozilla.org/mozilla-central/rev/a3ee5b2a7bf2
https://hg.mozilla.org/mozilla-central/rev/9b125055e5e0
https://hg.mozilla.org/mozilla-central/rev/e1bac78c8df5
https://hg.mozilla.org/mozilla-central/rev/55115a3dfb82
https://hg.mozilla.org/mozilla-central/rev/542c2d7a2c1d
https://hg.mozilla.org/mozilla-central/rev/a253b24f69b2
https://hg.mozilla.org/mozilla-central/rev/c1da667c41b3
https://hg.mozilla.org/mozilla-central/rev/cc21f29f364b
https://hg.mozilla.org/mozilla-central/rev/c249c91dc8ca

Comment 35

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4445

Comment 36

[[github robot]]

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)