1868766 - BrowserTestUtils.addTab uses allowInheritPrincipal=true even if allowInheritPrincipal=false

Status: RESOLVED FIXED

(Testing :: Mochitest, task)

Description

[Rob Wu [:robwu]]

BrowserTestUtils.addTab has the following implementation (source code link, added in bug 1466801), which looks very suspicious because it is impossible to set allowInheritPrincipal to false.

    if (!params.allowInheritPrincipal) {
      params.allowInheritPrincipal = true;
    }

Should !params.allowInheritPrincipal be changed to params.allowInheritPrincipal == undefined instead? Or equivalently, the above three lines be changed to:

    params.allowInheritPrincipal ??= true;

Comment 1

[Rob Wu [:robwu]]

Gijs, you added a review comment to this code at https://phabricator.services.mozilla.com/D2096#inline-7905
Do you recall the reason for this addition? If the intent was to always use allowInheritPrincipal = true, then I would expect the if (!params.allowInheritPrincipal) { part to not have been there.

Should we fix this as suggested above?

Comment 2

[:Gijs (he/him)]

What's the default in addTab in tabbrowser and/or the resulting loadURI call?

Assuming that's false, then IMO the BTU implementation should just not touch params - if the consumer specifies something, they get that behaviour, and otherwise they get the default (no inheritance allowed).

Again based on that assumption, I'd trypush dropping those 3 lines and seeing what happens. If all hell breaks loose (all tests that pass data: URIs fail or whatever), then we can reconsider. But in fact, most of these bits of code will implicitly use system principal for their triggering principal, and I really really hope we refuse to make data or JS or whatever kind of inheriting URI based docs actually inherit system... so I guess I'm not sure that there's anything depending on this at this point in time. But a trypush would tell us for sure. You can use the mochitest-bc preset and artifact builds to make it easy to check (don't think we use this helper in non-browser tests but ymmv) - it should give us an answer within an hour (or a bit more if linux is all green and then there are surprises on macOS/Windows - but generally I'd expect even just 1 platform to flag up if this is a no-go).

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1868766 - BrowserTestUtils shouldn't default to allowInheritPrincipal set to true, r?robwu

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6ee01dac41f
BrowserTestUtils shouldn't default to allowInheritPrincipal set to true, r=settings-reviewers,mossop

Comment 6

[amarc]

Backed out for causing ba failures @ browser_alert.js

• Backout link
• Push with failures
• Failure Log
• Failure line:

TEST-UNEXPECTED-FAIL | accessible/tests/browser/events/browser_alert.js | Not remote browser -

Comment 7

[:Gijs (he/him)]

Hrmpf. New trypush with that fixed: https://treeherder.mozilla.org/jobs?repo=try&revision=ca7b0d7777ad027ff95e0a3f2a83d1d9b354936f

Before landing I had a green trypush with all bc jobs but didn't realize the a11y ones were split off to ba jobs which didn't run on the trypush. I had also checked for uses of addTab from tests other than browser_ ones...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1f7cc44e79d
BrowserTestUtils shouldn't default to allowInheritPrincipal set to true, r=robwu,settings-reviewers,mossop

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing dt failures @ browser_json_refresh.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/acc2ef1ac7dc30ce9d908f60c8adaf9c68d5d05a

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_json_refresh.js

TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | sanity: correct triggeringPrincipal - true == true - 
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Buffered messages finished
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_json_refresh.js | sanity: no principalToInherit - false == true - got false, expected true (operator ==)
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Stack trace:
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - ok@resource://testing-common/SpecialPowersSandbox.sys.mjs:85:21
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - @chrome://mochitests/content/browser/devtools/client/jsonview/test/browser_json_refresh.js:46:9
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - execute@resource://testing-common/SpecialPowersSandbox.sys.mjs:139:12
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - _spawnTask@resource://testing-common/SpecialPowersChild.sys.mjs:1620:15
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - receiveMessage@resource://testing-common/SpecialPowersChild.sys.mjs:255:21
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - JSActor query*receiveMessage@resource://testing-common/SpecialPowersParent.sys.mjs:1384:14
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - JSActor query*spawn@resource://testing-common/SpecialPowersChild.sys.mjs:1549:17
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - @chrome://mochitests/content/browser/devtools/client/jsonview/test/browser_json_refresh.js:19:23
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - async*handleTask@chrome://mochikit/content/browser-test.js:1147:26
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - _runTaskBasedTest@chrome://mochikit/content/browser-test.js:1219:18
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:1360:14
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:1136:14
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - SimpleTest.waitForFocus/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1058:13
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | sanity: correct doc.nodePrincipal - "resource://devtools" == "resource://devtools" - 
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Navigating to "file:///builds/worker/workspace/build/tests/mochitest/browser/devtools/client/jsonview/test/simple_json.json"
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Waiting for page to be loaded…
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - GECKO(9734) | [Child 10053: Main Thread]: I/DocShellAndDOMWindowLeak ++DOMWINDOW == 4 (7f3a72f4f800) [pid = 10053] [serial = 4] [outer = 7f3a94271e00]
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - GECKO(9734) | [Child 10053, Main Thread] WARNING: '!ClientIsValidCreationURL(mClientInfo.PrincipalInfo(), aArgs.url())', file /builds/worker/checkouts/gecko/dom/clients/manager/ClientSource.cpp:65
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - GECKO(9734) | [Child 10053, Main Thread] WARNING: Failed to retarget HTML data delivery to the parser thread.: file /builds/worker/checkouts/gecko/parser/html/nsHtml5StreamParser.cpp:1215
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - → page loaded
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Wait for the toolbox to reload
[task 2025-02-04T00:16:40.658Z] 00:16:40     INFO - Wait for Responsive UI to reload
[task 2025-02-04T00:16:40.727Z] 00:16:40     INFO - TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | reloaded: correct channel uri - true == true - 
[task 2025-02-04T00:16:40.730Z] 00:16:40     INFO - TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | reloaded: correct contentPolicyType - 6 == 6 - 
[task 2025-02-04T00:16:40.732Z] 00:16:40     INFO - TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | reloaded: correct loadingPrincipal - null == null - 
[task 2025-02-04T00:16:40.735Z] 00:16:40     INFO - TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | reloaded: correct triggeringPrincipal - true == true - 
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - Not taking screenshot here: see the one that was previously logged
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_json_refresh.js | reloaded: no principalToInherit - false == true - got false, expected true (operator ==)
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - Stack trace:
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - ok@resource://testing-common/SpecialPowersSandbox.sys.mjs:85:21
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - @chrome://mochitests/content/browser/devtools/client/jsonview/test/browser_json_refresh.js:86:9
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - execute@resource://testing-common/SpecialPowersSandbox.sys.mjs:139:12
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - _spawnTask@resource://testing-common/SpecialPowersChild.sys.mjs:1620:15
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - receiveMessage@resource://testing-common/SpecialPowersChild.sys.mjs:255:21
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - JSActor query*receiveMessage@resource://testing-common/SpecialPowersParent.sys.mjs:1384:14
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - JSActor query*spawn@resource://testing-common/SpecialPowersChild.sys.mjs:1549:17
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - @chrome://mochitests/content/browser/devtools/client/jsonview/test/browser_json_refresh.js:58:23
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - async*handleTask@chrome://mochikit/content/browser-test.js:1147:26
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - _runTaskBasedTest@chrome://mochikit/content/browser-test.js:1219:18
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:1360:14
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:1136:14
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - SimpleTest.waitForFocus/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1058:13
[task 2025-02-04T00:16:40.738Z] 00:16:40     INFO - TEST-PASS | devtools/client/jsonview/test/browser_json_refresh.js | reloaded: correct doc.nodePrincipal - "resource://devtools" == "resource://devtools" 

Comment 10

[:Gijs (he/him)]

Bah, I should have seen that coming and checked devtools.

That failure is strange though. I can make it go away by (re-)adding the allowInheritPrincipal flag in this devtools helper, but the test is actually checking there isn't a principal to inherit, whereas the flag is supposed to allow for such a principal. And indeed with the flag present there isn't a principal to inherit, but with the flag not present there is ? That is pretty confusing. So digging at least a little bit more.

Comment 11

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #10)

Bah, I should have seen that coming and checked devtools.

That failure is strange though. I can make it go away by (re-)adding the allowInheritPrincipal flag in this devtools helper, but the test is actually checking there isn't a principal to inherit, whereas the flag is supposed to allow for such a principal. And indeed with the flag present there isn't a principal to inherit, but with the flag not present there is ? That is pretty confusing. So digging at least a little bit more.

Oh, wait, this is because we don't want to inherit system, so in the case where "don't inherit" is set, we pass an explicit null principal (so we don't inherit system). This was what the test used to check before bug 1792803. So probably just updating those assertions is a better fix than updating the helper.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36ba68332494
BrowserTestUtils shouldn't default to allowInheritPrincipal set to true, r=robwu,settings-reviewers,mossop,devtools-reviewers

Comment 13

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/36ba68332494