1466801 - openUILink's disallowInheritPrincipal is a footgun and a no-op for non-current-tab opened links

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[:Gijs (he/him)]

openUILink supports a disallowInheritPrincipal flag. There are 2 issues with it:

- It is a bit of a misnomer because it still allows script loads.
- Separately, if the link gets opened in a new tab or window, it's a no-op.


We should fix both of these things.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> - Separately, if the link gets opened in a new tab or window, it's a no-op.

Well, so...

The point of this flag originally is to avoid inheriting the principal from whatever was currently in the docshell.  This was back when loads from the UI had no triggering principal so we just used whatever was currently in the docshell as the triggering principal.

In the "new tab or window" case, when the thing currently in the docshell was an about:blank, there was no point to avoiding inheriting it.

But now that we're passing in actual triggering principals, we may want to think about whether to avoid inheriting from _those_.

> - It is a bit of a misnomer because it still allows script loads.

Sort of.  If the flag gets propagated to docshell, the principal of the script will be a new nullprincipal, so the script won't run.

Comment 2

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> In the "new tab or window" case, when the thing currently in the docshell
> was an about:blank, there was no point to avoiding inheriting it.
> 
> But now that we're passing in actual triggering principals, we may want to
> think about whether to avoid inheriting from _those_.

Right, sorry, that's why I filed this... I assume docshell will use the passed triggering principals to inherit from.

> > - It is a bit of a misnomer because it still allows script loads.
> 
> Sort of.  If the flag gets propagated to docshell, the principal of the
> script will be a new nullprincipal, so the script won't run.

Aha - except the problem is that openUILink (see utilityOverlay.js) explicitly avoids passing the flag to docshell in the javascript: URI case, so this too doesn't happen. :-(

Really, we should probably try to move to having "disallow inheriting principals" be the default for document/docshell loads, but that's another discussion...

Comment 3

[Jonathan Kingston [:jkt] he/him]

> "disallow inheriting principals"

Some of this is likely going to be ironed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1456440 disallowing for all principals likely would be much more work though.

Comment 4

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #3)
> > "disallow inheriting principals"
> 
> Some of this is likely going to be ironed out in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1456440 disallowing for all
> principals likely would be much more work though.

For toplevel documents? I mean, I'm not saying there shouldn't be a way to opt-in. I'm saying that right now it's opt-out, and it should be opt-in. Whereas I think bug 1456440 is about making it completely impossible to inherit system principals, right?

Comment 5

[Jonathan Kingston [:jkt] he/him]

> Whereas I think bug 1456440 is about making it completely impossible to inherit system principals, right?

Yeah sorry orthogonal to this issue.

Opt-in sounds like a worthwhile bug also and would fit within the work I am doing.

Comment 6

[Jonathan Kingston [:jkt] he/him]

:gijs, Do we expect the new tab or window case to use null principal when the protocol is javascript:? I assume this should then stop this call being a no-op. I personally would rather throw a sensible error for this case and just keep preventing it, given that the use-case for doing this appears to be limited.

I started work on a patch that tries to flip the logic such that allowInheritPricipal is required to be passed if it's needed, this appears to be working for common ui. We shall see how much it breaks on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9e11abd7f3743f26ecf8dbac94cda7ebdae60a0

Comment 7

[:Gijs (he/him)]

(In reply to Jonathan Kingston [:jkt] from comment #6)
> :gijs, Do we expect the new tab or window case to use null principal when
> the protocol is javascript:?

I mean, if there's an obvious content opener, like middle-clicking in-content links on foo.com/bar.html, then it should be that principal, but otherwise I think yes.

> I assume this should then stop this call being
> a no-op. I personally would rather throw a sensible error for this case and
> just keep preventing it, given that the use-case for doing this appears to
> be limited.

I'm not sure I understand. :-(

I should have time to discuss on vidyo tomorrow afternoon if that helps.

> I started work on a patch that tries to flip the logic such that
> allowInheritPricipal is required to be passed if it's needed, this appears
> to be working for common ui. We shall see how much it breaks on try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e9e11abd7f3743f26ecf8dbac94cda7ebdae60a0

Seems the answer is "a lot"...

Comment 8

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1466801 - Flipping disallowInheritPrincipal to be allow.

Comment 9

[Jonathan Kingston [:jkt] he/him]

I mostly got a clean try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=324234820578eada4cb209e3be8fd0cc428c8bc4

So dt3 for linux is ignorable because it's looking at the console logs I added, which I removed in the latest patch.
The Eslint warning comment has been removed.

My only concern is wpt1, wpt12, wpt14 and wpt16 which I can't seem to tell why these happen but they don't seem related at all.

Comment 10

[Phabricator Automation]

Comment on attachment 8991618 [details]
Bug 1466801 - Flipping disallowInheritPrincipal to be allow.

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D2096

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Ah, one last thing, can we add a test for this somehow?

Comment 12

[Phabricator Automation]

Comment on attachment 8991618 [details]
Bug 1466801 - Flipping disallowInheritPrincipal to be allow.

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

https://phabricator.services.mozilla.com/D2096

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Thanks for adding the test Jonathan, I just looked at it and it looks great. Just one nit since you are already on it, maybe not only check:
  ok(content.document.nodePrincipal.isCodebasePrincipal,
     "sanity: correct doc.nodePrincipal");
but also do another sanity check to make sure the nodePrincipal matches the expected URI:
  ok(content.document.nodePrincipal.URI.asciiSpee, ...)

Thanks, r=me!

Comment 14

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1466801 - Flipping disallowInheritPrincipal to be allow.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Jonathan, both patches here are identical. Shall one of them land, or does it need an update?

Comment 16

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8994505 [details]
Bug 1466801 - Flipping disallowInheritPrincipal to be allow.

Sorry the one with the reviews is the correct one, phabricator decided to give me another update id. thanks!

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cba8b6eb37a007b60485167576b9b435d3cb2ad

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e6d270ea05451c72fc4301b7a02fa3c03e1a6d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4cba8b6eb37a007b60485167576b9b435d3cb2ad&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Some Linux debug wpt autoplay tests started failing, e.g. https://treeherder.mozilla.org/logviewer.html#?job_id=189877842&repo=mozilla-inbound

[task 2018-07-24T18:47:27.540Z] 18:47:27     INFO - TEST-START | /html/semantics/embedded-content/media-elements/ready-states/autoplay.html
[task 2018-07-24T18:47:28.027Z] 18:47:28     INFO - PID 4786 | ++DOCSHELL 0xd8570800 == 9 [pid = 4786] [id = {a47961da-210b-48b7-81d0-90077018bd21}]
[task 2018-07-24T18:47:28.028Z] 18:47:28     INFO - PID 4786 | ++DOMWINDOW == 26 (0xda2c9540) [pid = 4786] [serial = 26] [outer = (nil)]
[task 2018-07-24T18:47:28.069Z] 18:47:28     INFO - PID 4786 | ++DOMWINDOW == 27 (0xd999f800) [pid = 4786] [serial = 27] [outer = 0xda2c9540]
[task 2018-07-24T18:47:28.247Z] 18:47:28     INFO - PID 4786 | ++DOMWINDOW == 28 (0xd99a1c00) [pid = 4786] [serial = 28] [outer = 0xda2c9540]
[task 2018-07-24T18:47:31.411Z] 18:47:31     INFO - 
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/embedded-content/media-elements/ready-states/autoplay.html | audio.autoplay - assert_array_equals: property 1, expected "play" but got "canplaythrough"
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - expect_events/callback<@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/ready-states/autoplay.html:13:7
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1562:20
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - EventListener.handleEvent*expect_events/<@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/ready-states/autoplay.html:21:7
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - expect_events@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/ready-states/autoplay.html:19:5
[task 2018-07-24T18:47:31.414Z] 18:47:31     INFO - autoplay_test/<@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/ready-states/autoplay.html:29:5
[task 2018-07-24T18:47:31.415Z] 18:47:31     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
[task 2018-07-24T18:47:31.415Z] 18:47:31     INFO - async_test@http://web-platform.test:8000/resources/testharness.js:562:13
[task 2018-07-24T18:47:31.416Z] 18:47:31     INFO - autoplay_test@http://web-platform.test:8000/html/semantics/embedded-content/media-elements/ready-states/autoplay.html:25:3
[task 2018-07-24T18:47:31.417Z] 18:47:31     INFO - @http://web-platform.test:8000/html/semantics/embedded-content/media-elements/ready-states/autoplay.html:71:1
[task 2018-07-24T18:47:31.417Z] 18:47:31     INFO - 

Browser-chrome browser/base/content/test/contextMenu/browser_utilityOverlay.js | example.org loaded - Got about:blank, expected http://example.org/ seems to have become more frequent, not sure if it's from this patch or reorganization of the tests chunks. Check out the TV failures on the push (and bc jobs for later pushes).

Comment 19

[Dão Gottwald [:dao]]

When you reland this, could you please maintain the alphabetical order here? https://hg.mozilla.org/integration/mozilla-inbound/rev/4cba8b6eb37a007b60485167576b9b435d3cb2ad#l3.66

Comment 20

[Jonathan Kingston [:jkt] he/him]

> Browser-chrome browser/base/content/test/contextMenu/browser_utilityOverlay.js 

This was already an intermittent anyway, the test is somehow broken and I couldn't work out how to fix it. This is why in the end I ended up making a new test file. I could move back this file as it doesn't impact anything.

I turned off the other tests as they seems to be slow running and are impossible to reproduce.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This landed as https://hg.mozilla.org/integration/mozilla-inbound/rev/c9b0ce46ad4f59e7c07fc3b7db2b16646140ae46 and got backed out as https://hg.mozilla.org/integration/mozilla-inbound/rev/919250fdd7c2b335346f28ff841f5febdffb44e1 for for pause-remove-from-document-networkState.html failures.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c9b0ce46ad4f59e7c07fc3b7db2b16646140ae46&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=Linux%20debug%20Web%20platform%20tests%20test-linux32%2Fdebug-web-platform-tests-12%20W(wpt12)
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=193382768&repo=mozilla-inbound&lineNumber=8554

[task 2018-08-10T23:33:11.405Z] 23:33:11     INFO - TEST-START | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html
[task 2018-08-10T23:33:11.614Z] 23:33:11     INFO - PID 3565 | ++DOCSHELL 0xd25d7800 == 14 [pid = 3565] [id = {c35f8a20-e085-45b2-82f9-30786ef05ddf}]
[task 2018-08-10T23:33:11.615Z] 23:33:11     INFO - PID 3565 | ++DOMWINDOW == 40 (0xdff19940) [pid = 3565] [serial = 40] [outer = (nil)]
[task 2018-08-10T23:33:11.644Z] 23:33:11     INFO - PID 3565 | ++DOMWINDOW == 41 (0xd25d9800) [pid = 3565] [serial = 41] [outer = 0xdff19940]
[task 2018-08-10T23:33:11.761Z] 23:33:11     INFO - PID 3565 | ++DOMWINDOW == 42 (0xd25dc800) [pid = 3565] [serial = 42] [outer = 0xdff19940]
[task 2018-08-10T23:33:12.195Z] 23:33:12     INFO - 
[task 2018-08-10T23:33:12.195Z] 23:33:12     INFO - TEST-UNEXPECTED-FAIL | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html | paused state when removing from a document when networkState is NETWORK_EMPTY - assert_false: paused after stable state expected false got true
[task 2018-08-10T23:33:12.196Z] 23:33:12     INFO - @http://web-platform.test:8000/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html:15:5
[task 2018-08-10T23:33:12.196Z] 23:33:12     INFO - Test.prototype.step_timeout/<@http://web-platform.test:8000/resources/testharness.js:1596:20
[task 2018-08-10T23:33:12.196Z] 23:33:12     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
[task 2018-08-10T23:33:12.197Z] 23:33:12     INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1562:20
[task 2018-08-10T23:33:12.197Z] 23:33:12     INFO - setTimeout handler*Test.prototype.step_timeout@http://web-platform.test:8000/resources/testharness.js:1595:16
[task 2018-08-10T23:33:12.197Z] 23:33:12     INFO - @http://web-platform.test:8000/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html:12:3
[task 2018-08-10T23:33:12.198Z] 23:33:12     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1538:20
[task 2018-08-10T23:33:12.198Z] 23:33:12     INFO - async_test@http://web-platform.test:8000/resources/testharness.js:562:13
[task 2018-08-10T23:33:12.198Z] 23:33:12     INFO - @http://web-platform.test:8000/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html:9:1
[task 2018-08-10T23:33:12.198Z] 23:33:12     INFO - TEST-OK | /html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document-networkState.html | took 791ms

Comment 22

[Jonathan Kingston [:jkt] he/him]

Just pushed again into inbound, the test failure is like the others that was disabled. Video playing on 32 debug linux. I amended Bug 1482405 which covers the tests I have disabled as part of this bug.

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e9fdddf12e598962d6282a4b053aa66b7aabda

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b5e9fdddf12e

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This seems like something which should ride the trains, do you agree?

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Per our IRC discussion.

Comment 27

[Jonathan Kingston [:jkt] he/him]

We agreed on irc this should remain private but ride the trains at the moment due to the breakages with bookmarklets and also it has a wide ranging impact on all the method calls we have.