1694993 - noopener window.open() shouldn't inherit principal

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

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

Per specs (HTML + URL) noopener window open uses opaque origin.

https://searchfox.org/mozilla-central/rev/df23c6e58c6be1eb8399e475878f73d4867bef87/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1248 should probably use LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL when noopener is passed to window.open.

Comment 1

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

This is low priority, since it is very hard to detect this.

Comment 2

[Neha Kochar [:neha]]

This is not Fission specific.

Comment 3

[Anne (:annevk)]

Changing severity to S4 because N/A results in autonag emails.

Comment 4

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

Attachment: Bug 1694993 - Part 1: Be more explicit about principals used when creating pop-up windows, r=smaug!

Previously we would pull this information frequently from the subject
principal, which is unreliable. With this new approach, we more explicitly pass
the principals around as-needed into where they're going to be used.

Some assertions about the subject principal were introduced to ensure that
assumptions made about chrome windows and the system principal are not
incorrect.

Comment 5

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

Attachment: Bug 1694993 - Part 2: Explicitly select the principal for newly created windows in nsWindowWatcher, r=smaug!

When opening a window with noopener, we will no longer inherit the
subject principal into the newly created window's initial about:blank
document, instead creating a new null principal.

This patch also makes the system/expanded principal -> null principal
translation happen earlier (it previously happened in
SetInitialPrincipalToSubject), so that it can be followed more easily
when reading the code.

Finally, the load started by nsWindowWatcher in new windows is updated
to specify LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL when noopener is
specified such that the explicit about:blank load also cannot inherit
the subject principal.

This change does make it so that the global is not re-used between the
initial and loaded about:blank document, however this shouldn't be
visible due to noopener being specified, preventing any references to
the initial document from existing.

Noopener loads with javascript: URIs will be rejected early during the
load due to the mismatch between the triggering principal and the
initial about:blank document's principal.

Depends on D154563

Comment 6

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

Attachment: Bug 1694993 - Part 3: Rename some variables in nsWindowWatcher for clarity, r=smaug!

The newBC and newDocShell variables were potentially confusing, as they
could also be existing windows selected by named targeting or the window
provider, so they have been renamed. Some other variables were also renamed for
consistency and clarity.

Depends on D154564

Comment 7

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

Attachment: Bug 1694993 - Part 4: Copy session storage before loading, and only for newly created windows, r=smaug!

Previously we would copy session storage even if we were not opening a new
window, meaning that a targeted load could re-trigger a copy. This was not
specified in the standard so is being changed to only copy when a new window is
created. In addition, the copy was moved before navigaton starts, again for
more consistency with ordering for the standard, such that things like
javascript: URI loads will oberve the up-to-date session storage.

Depends on D154565

Comment 8

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

Attachment: Bug 1694993 - Part 5: Add a simple test for javascript URIs with noopener, r=smaug!

Depends on D154566

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3821545ab46b
Part 1: Be more explicit about principals used when creating pop-up windows, r=smaug
https://hg.mozilla.org/integration/autoland/rev/05d7cbbfe9e2
Part 2: Explicitly select the principal for newly created windows in nsWindowWatcher, r=smaug
https://hg.mozilla.org/integration/autoland/rev/c9585ce37fe5
Part 3: Rename some variables in nsWindowWatcher for clarity, r=smaug
https://hg.mozilla.org/integration/autoland/rev/3b412d5fbdcf
Part 4: Copy session storage before loading, and only for newly created windows, r=smaug
https://hg.mozilla.org/integration/autoland/rev/1d21d911b3e7
Part 5: Add a simple test for javascript URIs with noopener, r=smaug

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35553 for changes under testing/web-platform/tests

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failure on browser_cross_process_csp_inheritance.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/4e648a00f8a1282918758dcd928ef8bdd808cdac
Log link: https://treeherder.mozilla.org/logviewer?job_id=388090744&repo=autoland&lineNumber=20104

Comment 12

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 13

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

Attachment: Bug 1694993 - Part 6: Preserve precursor principal through LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL, r=ckerschb!,smaug!

If we don't preserve the precursor principal in this case, we'll end up
doing an unnecessary process switch in some cases, which lead to a
test failure.

This patch also cleans up some logic around the first party origin
attribute with null principals, as the logic was only used in one place
and generally added some unnecessary complexity to NullPrincipal
itself.

Depends on D154567

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5c6608a07e9
Part 1: Be more explicit about principals used when creating pop-up windows, r=smaug
https://hg.mozilla.org/integration/autoland/rev/be9c1ebd0413
Part 2: Explicitly select the principal for newly created windows in nsWindowWatcher, r=smaug
https://hg.mozilla.org/integration/autoland/rev/84cf4510639e
Part 3: Rename some variables in nsWindowWatcher for clarity, r=smaug
https://hg.mozilla.org/integration/autoland/rev/54e339586d26
Part 4: Copy session storage before loading, and only for newly created windows, r=smaug
https://hg.mozilla.org/integration/autoland/rev/93b6f01e9320
Part 5: Add a simple test for javascript URIs with noopener, r=smaug
https://hg.mozilla.org/integration/autoland/rev/4d404c79681a
Part 6: Preserve precursor principal through LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL, r=ckerschb,smaug

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on test_alwaysOnTop_windows.html

• backout: https://hg.mozilla.org/integration/autoland/rev/6425a97c5857cc708bb41423cd780506d15161ad
• push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=4d404c79681a03b30932648a722853a2e6d0f22c&group_state=expanded
• push where failures appeared : https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=UQr7Ma3-SbC8kTvGkBApog.0&revision=8e57fab1c5e5e4b2658b909ada847d0bd89a9e86&searchStr=mochitest-chrome&group_state=expanded
• failure log: https://treeherder.mozilla.org/logviewer?job_id=388641264&repo=autoland&lineNumber=129500

[task 2022-08-27T00:12:57.135Z] 00:12:57     INFO - TEST-START | toolkit/components/windowwatcher/test/test_alwaysOnTop_windows.html
[task 2022-08-27T00:17:57.204Z] 00:17:57     INFO - TEST-INFO | started process screenshot
[task 2022-08-27T00:17:57.272Z] 00:17:57     INFO - TEST-INFO | screenshot: exit 0
[task 2022-08-27T00:17:57.279Z] 00:17:57     INFO - Buffered messages logged at 00:12:58
[task 2022-08-27T00:17:57.279Z] 00:17:57     INFO - add_task | Entering 
[task 2022-08-27T00:17:57.280Z] 00:17:57     INFO - Buffered messages finished
[task 2022-08-27T00:17:57.280Z] 00:17:57     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/windowwatcher/test/test_alwaysOnTop_windows.html | Test timed out. - 
[task 2022-08-27T00:17:58.213Z] 00:17:58     INFO - Not taking screenshot here: see the one that was previously logged
[task 2022-08-27T00:17:58.217Z] 00:17:58     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/windowwatcher/test/test_alwaysOnTop_windows.html | [SimpleTest.finish()] No checks actually run. (You need to call ok(), is(), or similar functions at least once.  Make sure you use SimpleTest.waitForExplicitFinish() if you need it.) 
[task 2022-08-27T00:17:58.217Z] 00:17:58     INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:414:16
[task 2022-08-27T00:17:58.217Z] 00:17:58     INFO - afterCleanup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1413:18
[task 2022-08-27T00:17:58.218Z] 00:17:58     INFO - executeCleanupFunction@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1489:7
[task 2022-08-27T00:17:58.218Z] 00:17:58     INFO - SimpleTest.finish@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:1509:3
[task 2022-08-27T00:17:58.218Z] 00:17:58     INFO - killTest@chrome://mochikit/content/tests/SimpleTest/TestRunner.js:195:22
[task 2022-08-27T00:17:58.233Z] 00:17:58     INFO - GECKO(864) | MEMORY STAT | vsize 2104390MB | vsizeMaxContiguous 65864728MB | residentFast 291MB | heapAllocated 156MB
[task 2022-08-27T00:17:58.239Z] 00:17:58     INFO - TEST-OK | toolkit/components/windowwatcher/test/test_alwaysOnTop_windows.html | took 301104ms

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nika, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 18

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

Attachment: Bug 1694993 - Part 7: Don't set LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL for chrome windows, r=smaug

Depends on D155277

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/62cc9e155d56
Part 1: Be more explicit about principals used when creating pop-up windows, r=smaug
https://hg.mozilla.org/integration/autoland/rev/221e28fc994d
Part 2: Explicitly select the principal for newly created windows in nsWindowWatcher, r=smaug
https://hg.mozilla.org/integration/autoland/rev/2b181e351637
Part 3: Rename some variables in nsWindowWatcher for clarity, r=smaug
https://hg.mozilla.org/integration/autoland/rev/f2b6ca0547cb
Part 4: Copy session storage before loading, and only for newly created windows, r=smaug
https://hg.mozilla.org/integration/autoland/rev/adc9146d8329
Part 5: Add a simple test for javascript URIs with noopener, r=smaug
https://hg.mozilla.org/integration/autoland/rev/484fd2c68e87
Part 6: Preserve precursor principal through LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL, r=ckerschb,smaug
https://hg.mozilla.org/integration/autoland/rev/4ec21c88e8c9
Part 7: Don't set LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL for chrome windows, r=smaug

Comment 20

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/62cc9e155d56
https://hg.mozilla.org/mozilla-central/rev/221e28fc994d
https://hg.mozilla.org/mozilla-central/rev/2b181e351637
https://hg.mozilla.org/mozilla-central/rev/f2b6ca0547cb
https://hg.mozilla.org/mozilla-central/rev/adc9146d8329
https://hg.mozilla.org/mozilla-central/rev/484fd2c68e87
https://hg.mozilla.org/mozilla-central/rev/4ec21c88e8c9

Comment 21

[Mathew Hodson]

Bug 1786376 was assigned to the wrong field. I shouldn't be in the Regressed By field. It's reversed. Then the regression keyword can be removed from this bug.

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot