Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[COOP] Unable to reconcile html/cross-origin-opener-policy/coop-same-origin-allow-popups-document-write.html with the specification #6960

Closed
cdumez opened this issue Aug 13, 2021 · 10 comments

Comments

@cdumez
Copy link

cdumez commented Aug 13, 2021

In html/cross-origin-opener-policy/coop-same-origin-allow-popups-document-write.html:
Document A has COOP=same-origin-allow-popups
Document A calls window.open() (Let's call Document B, the document in openee)
Document A calls document.write() on Document B, adding a <meta http-equiv="refresh"> to navigate the popup cross-origin and with coop=unsafe-none.

The test does not expect the refresh navigation to swap browsing context group and I am unable to figure out why from the specification.

Because Document A has COOP=same-origin-allow-popup, I believe Document B inherits COOP=same-origin-allow-popup as well before the document.write().

Then document.write() is called and as per the document write steps, we would call the document open steps. Step 13 of the document open steps says:

Set document's is initial about:blank to false.

So then the navigation from occurs in the popup, activeCOOP=same-origin-allow-popup and isInitialAboutBlank=false when we call the check browsing context group switch coop value steps, I believe:

  • isInitialAboutBlank=false
  • activeDocumentCOOPValue=same-origin-allow-popups
  • responseCOOPValue=unsafe-none

From my reading of the specification, this SHOULD cause a browsing context group switch because isInitialAboutBlank=false, due to the earlier call to document.write(). However, this is not what the test expects or Chrome's behavior.

What am I missing?

@cdumez
Copy link
Author

cdumez commented Aug 13, 2021

cc @camillelamy

@cdumez
Copy link
Author

cdumez commented Aug 13, 2021

cc @ArthurSonzogni who I think was involved in the WPT test (https://bugs.chromium.org/p/chromium/issues/detail?id=1216244)

@domenic
Copy link
Member

domenic commented Aug 13, 2021

/cc @rakina since it involves her favorite thing, the initial about:blank and document.write().

My totally uneducated guess is that Chrome's notion of is initial about:blank isn't fully synced between our renderer and browser processes, so it doesn't properly get the signal of isInitialAboutBlank = false.

@ArthurSonzogni
Copy link
Member

ArthurSonzogni commented Aug 14, 2021

Your reading of the spec is perfectly correct!

I initially thought this behavior was a bug in Chrome when I received bugs and wrote this regression test... Swapping browsing context group because of document.write is indeed the consequence of the specification.

There are issues with websites using the closure library. This library causes window.open(url) to translate into w = window.open() followed by a w.contentDocument.write('location = ${url}'). The goal of the library was to strip the referrer from the navigation request. This is a problem, because this prevented COOP:same-origin-allow-popup to allow the popup.
You can read:
https://bugs.chromium.org/p/chromium/issues/detail?id=1216244
to get more informations.

I think what is needed now is to invert the expectations. We should also probably fill a bug against the closure library so that they could maybe provide an alternative implementation.

@rakina
Copy link
Member

rakina commented Aug 16, 2021

This might be a really good time to separate "is initial about:blank"-ness by use cases, which we discussed here, but only for "window reuse" vs "history replacement".

Currently document.write() removes the "initial about:blank"-ness of the document, which I suspect is to ensure that the document will be retained in session history (because otherwise the next navigation will replace the initial empty document's entry). The next navigation won't reuse the window too (if it was still marked as the initial empty document, the next navigation will reuse the window, see spec and issue).

The current references to "is initial about blank" and "still on its initial about:blank" are all about those two cases (history & window reuse), except for the COOP case discussed here.

For this use case, I think we have a few options:

  • Have another "initial about:blank" state that isn't affected by document.write()
  • Keep using the "initial about:blank" state as it is right now, but ensure that current usages can handle this?
  • Reconsider whether we want to use "initial about:blank" state for the COOP case at all. Maybe we want something like "is this the first navigation that happened/triggered on this frame" or something like that instead, to ensure we are enforcing the policy correctly?

@camillelamy
Copy link
Member

So the end goal of the initial about:blank check for COOP same-origin-allow-popups is the following:

  1. That the COOP same-origin-allow-popups page can open popups without browsing context group switches.
  2. That a navigation from an existing page with COOP same-origin-allow-popups to another document with a different origin and/or COOP triggers a browsing context group switch.

At the same time, we also need popups to inherit their COOP from the opener, at least if they are same-origin. So we ended up with checking the initial about:blankness. I don't really think we have a better choice here - as I do believe that using document.write on the initial about blank document does put the subsequent navigation into case 2 of the above.

We should update the test to match the spec expectations and file a bug against libraries using this mechanism.

@ArthurSonzogni
Copy link
Member

ArthurSonzogni commented Aug 17, 2021

We should update the test to match the spec expectations and file a bug against libraries using this mechanism.

Will do both soon.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 17, 2021
Update test expectations to follow the current HTML specification.

Bug:whatwg/html#6960
Bug:google/closure-library#1137
Bug:1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 17, 2021
Update test expectations to follow the current HTML specification.

Bug: whatwg/html#6960
Bug: google/closure-library#1137
Bug: 1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097699
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912551}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 17, 2021
Update test expectations to follow the current HTML specification.

Bug: whatwg/html#6960
Bug: google/closure-library#1137
Bug: 1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097699
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912551}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Aug 17, 2021
Update test expectations to follow the current HTML specification.

Bug: whatwg/html#6960
Bug: google/closure-library#1137
Bug: 1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097699
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912551}
@domenic
Copy link
Member

domenic commented Aug 17, 2021

Thanks all for the followup here!

@domenic domenic closed this as completed Aug 17, 2021
@cdumez
Copy link
Author

cdumez commented Aug 30, 2021

Following up in web-platform-tests/wpt#30243 because I don't think the WPT test is entirely correct.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 3, 2021
…ations., a=testonly

Automatic update from web-platform-tests
COOP:SOAP vs document.write update expecations.

Update test expectations to follow the current HTML specification.

Bug: whatwg/html#6960
Bug: google/closure-library#1137
Bug: 1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097699
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912551}

--

wpt-commits: 18f738d6c0a314f7230c7f6c9f3cd943e5b5c009
wpt-pr: 30049
spinda pushed a commit to PLSysSec/cachet-firefox that referenced this issue Sep 8, 2021
…ations., a=testonly

Automatic update from web-platform-tests
COOP:SOAP vs document.write update expecations.

Update test expectations to follow the current HTML specification.

Bug: whatwg/html#6960
Bug: google/closure-library#1137
Bug: 1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097699
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912551}

--

wpt-commits: 18f738d6c0a314f7230c7f6c9f3cd943e5b5c009
wpt-pr: 30049
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Update test expectations to follow the current HTML specification.

Bug: whatwg/html#6960
Bug: google/closure-library#1137
Bug: 1216244
Change-Id: Ib8bd9e69a5a84de47577f5be1c211af9a35468be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3097699
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Camille Lamy <clamy@chromium.org>
Auto-Submit: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#912551}
NOKEYCHECK=True
GitOrigin-RevId: b83a573bae82e84554a8b2ebe8636f56d940a782
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants