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

goog.window.open with "no-referrer" should stop using document.write, because of COOP:same-origin-allow-popup. #1137

Open
ArthurSonzogni opened this issue Aug 17, 2021 · 18 comments

Comments

@ArthurSonzogni
Copy link

ArthurSonzogni commented Aug 17, 2021

To strip referrer in goog.window.open() with the ''no-referrer" option. The closure library is using document.write:

goog.dom.safe.documentWrite(newDoc, safeHtml);

document.write taint the initial empty document. This prevents COOP:same-origin-allow-popup to allow the popup to stay within the same browsing context group. This breaks OAUTH flow of some websites.

The COOP:same-origin-allow-popup and document.write behavior is implemented by:

  • The html specification.
  • Firefox.
  • Safari (Soon)
  • Chrome (Soon).

It was implemented by Chrome, but reverted because it broke some Google users of the closure library. Chrome intends to apply the specification at some point after resolving this bug.

Would not using document.write be an option for you?

See related issues:

+CC @ddworken @camillelamy @cdumez

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}
@kjin
Copy link
Contributor

kjin commented Aug 17, 2021

Is there an alternative we can use to accomplish the same thing as calling document.write? I'm not super familiar with the code, but it looks like it's fairly central to what window.open is supposed to do.

@ddworken
Copy link

@kjin I have the same question. I spent some time exploring the various options here and wasn't able to come up with any way of doing so.

It seems like the best option may be to just drop support for this parameter and rely on the fact that modern browsers no longer include the full referrer. Internally, I've been advising services using this parameter to do exactly that unless it is critical to hide the referrer in older browsers.

@shicks
Copy link
Member

shicks commented Aug 17, 2021

Within google, it seems that only 10% or so of the calls pass the noreferrer option, less than 100 total. Unfortunately, it's unlikely that any tests would break if we removed the feature and it was actually still necessary, so it's not clear how we'd actually communicate this change safely.

FWIW, we're also looking into deprecating the entire method in favor of exposing a safer variant in https://github.com/google/safevalues.

@shicks
Copy link
Member

shicks commented Aug 19, 2021

@ddworken Can you give some more details about this? I don't fully grok what's going on here. When did the behavior change? Can you point to when/where the spec changed that would not send the full referrer? Is this in reference to point 4 in the bug you linked? It sounds like that's only really a partial solution and can be opted out, which will cause privacy issues.

It's possible we could change the behavior only for "modern browsers" and keep doing the same thing we've always done on older browsers - but what is the cutoff and how do you recommend detecting it?

That said, even if we update Closure Library, we can't guarantee the new version is picked up ubiquitously enough - any given Google property will be rebuilt against head at worst every year (and usually much more frequently), but there are external properties still running against versions of the library from 2017 or earlier, and there's nothing we can do to push out any updates. Are we concerned about breaking them?

My impression is that the spec itself is problematic - can we change it to better align with reality and not break the web?

@ddworken
Copy link

@ddworken Can you give some more details about this? I don't fully grok what's going on here. When did the behavior change? Can you point to when/where the spec changed that would not send the full referrer? Is this in reference to point 4 in the bug you linked? It sounds like that's only really a partial solution and can be opted out, which will cause privacy issues.

Referer-Policy is a new way for sites to restrict the data that is included in the referer header. Over the past year or two, all major browsers have changed it so the default referer-policy doesn't include the full path in the referer, so there is only a risk of data leakage if the site specifically opts out of this new default. See here for Chrome's announcement.

But yes, you're right that a site could be setting a referer policy of unsafe-url and then using Closure to try to hide the referer for a specific popup.

It's possible we could change the behavior only for "modern browsers" and keep doing the same thing we've always done on older browsers - but what is the cutoff and how do you recommend detecting it?

Yeah, that could work. It could in theory be done by user agent sniffing and including a list of the versions where each browser started hiding the full referer by default.

That said, even if we update Closure Library, we can't guarantee the new version is picked up ubiquitously enough - any given Google property will be rebuilt against head at worst every year (and usually much more frequently), but there are external properties still running against versions of the library from 2017 or earlier, and there's nothing we can do to push out any updates. Are we concerned about breaking them?

Personally, I'm not too concerned with this. My suspicion is that any sites using old versions of the library probably aren't deploying COOP since it is a very cutting edge security standard. But there is still a risk there. But at least by dropping support for no-referrer, we would make it so that sites deploying COOP with a modern version of closure are safe.

My impression is that the spec itself is problematic - can we change it to better align with reality and not break the web?

I personally agree with this. As I understand it, the issue here is more that it is hard to make the way COOP is specced allow for this, even though conceptually it doesn't seem problematic. @camillelamy would know more about this.

@domenic
Copy link

domenic commented Aug 19, 2021

The issue is really with how document.write() is specced. It changes the URL of a page, which means it trips up a lot of security-related checks in the spec and implementation.

If you want to just inject content, using e.g. openedWindow.document.body.innerHTML += "stuff" would accomplish the goal without mucking with something as fundamental as the opened document's URL. I'm not sure if that works for you though.

@ddworken
Copy link

Ah, that's perfect, thank you @domenic! I assumed that setting innerHTML also changed isInitialAboutBlank.

@shicks In that case, we have an easier cross-browser fix. It should be as simple as changing this line to:

        goog.dom.safe.setInnerHtml(newDoc, safeHtml);

@ArthurSonzogni
Copy link
Author

I'm not sure if that works for you though.

Yes, to be verified, I don't know if this will strip the referer and works across every web browsers.

@domfarolino who know about referrer might be able to provide useful informations.

If this works or another solution works great! Otherwise, I am wondering if the websites at Google that used this "noreferrer" option and want to keep the openee/opener access couldn't simply stop using this option? It feels weird for me wanting both at the same time. WDYT @ddworken ?

@camillelamy
Copy link

The problem for changing the COOP spec is that there is no way for the browser to distinguish between "I used document.write to open a popup without a referrer" and "I used document.write to create a complete HTML page, which navigated 20mn later to another page", and we'd really like to trigger a browsing context group switch in the second case.

Since your goal is not to actually create a new page but strip the referrer, using document.write might not be the thing you want here.

@ddworken
Copy link

So after some testing, it appears as though setting innerHTML doesn't quite work. For reasons that are unclear to me, it works if the URL in the meta redirect is an absolute URL (ie https://example.com/foo) but doesn't work if the URL is a relative URL (ie /foo). Whereas this works for both relative and absolute URLs if done via document.write. See https://coop-reporting-chrome-86.glitch.me/open_no_ref for a demo of this.

Otherwise, I am wondering if the websites at Google that used this "noreferrer" option and want to keep the openee/opener access couldn't simply stop using this option? It feels weird for me wanting both at the same time. WDYT @ddworken ?

Yeah, I think this is plausible (and is what I've been recommending to teams). The main caveats are that:

  1. This is another "gap" in COOP reporting that teams have to be aware of when deploying COOP, and people aren't always going to be aware that they fall into this gap.
  2. Since many applications use client-side navigation, I think it is actually somewhat plausible that someone could set SOAP because one part of their application needs to open and interact with popups, but set no-referrer because they want to hide it somewhere else. But this is somewhat moot since referrers are hidden by default.

If we think this is the best path forward, I'm fine with just not changing closure and adding this to our list of things that we warn product teams about. But there is a non-insignificant risk to this.

@domenic
Copy link

domenic commented Aug 20, 2021

For reasons that are unclear to me, it works if the URL in the meta redirect is an absolute URL (ie https://example.com/foo) but doesn't work if the URL is a relative URL (ie /foo).

Right. This is because, unlike document.write(), setting document.body.innerHTML does not update the URL of the opened window to equal the caller URL. So, you are trying to resolve /foo relative to about:blank, which is an error.

I suggest absolutizing the URL using (new URL(possiblyRelativeURL, location.href)).href.

@ddworken
Copy link

Thanks Domenic! I tried implementing that (and adding logic to only do it for modern browsers that support URL/COOP), and it turns out Firefox doesn't trigger a redirect if you set the meta redirect via innerHTML.

What do people think we should do? Is our best solution just to document this as a limitation (and I can drive a change internally to minimize our use of noreferrer)?

@shicks
Copy link
Member

shicks commented Aug 26, 2021

IIUC, @ddworken is saying that absolutizing the URL is insufficient because Firefox won't trigger the redirect at all in the first place. Otherwise, it sounds like innerHTML (aside: would appendChild not work here?) would be the ideal solution and would solve all problems.

Assuming that's not feasible, here's the problematic combinations:

  1. Site uses the default ReferrerPolicy and drops noreferrer expecting to rely on new default
    • leads to a privacy leak in older browsers regardless of what Closure does (since it's not calling into Closure)
  2. Site overrides default ReferrerPolicy and uses noreferrer
    • leads to a privacy leak in all browsers if Closure simply ignores no-referrer
  3. Site uses no-referrer and opts into COOP
    • breaks site on newer browsers if Closure uses document.write

Situation 1 I think is entirely an education problem - we shouldn't recommend folks do this without understanding all the ramifications. Situation 2 is also primarily an education problem, but only if Closure decides to ignore noreferrer - in which case anyone who has a legitimate reason to do this is left stranded, I think?

Is there any way to determine in JS if a site has opted into COOP? (assuming that's even where it happens? I'm not as clear on how it's supposed to work) If so, then could we make it an error to use no-referrer at the same time? This would "fix" the breakage, but would introduce migration hurdles (or would make it harder to flip the switch by default - not sure if that's planned).

Regardless of all this, driving down use of noreferrer sounds like a good plan.

@domenic
Copy link

domenic commented Aug 26, 2021

would appendChild not work here?

It should work, and I wonder if it might even fix the Firefox problem; maybe they have weird logic about parser-triggered navigations. (The spec has no such logic so this is just a Firefox bug...)

Is there any way to determine in JS if a site has opted into COOP?

You can use window.crossOriginIsolated to determine if a site has opted into both COOP and COEP, but I don't think you can determine if the site opted in to COOP alone... Still it might help a bit.

@ddworken
Copy link

It should work, and I wonder if it might even fix the Firefox problem; maybe they have weird logic about parser-triggered navigations. (The spec has no such logic so this is just a Firefox bug...)

I checked and sadly that doesn't work in Firefox either. :/

You can use window.crossOriginIsolated to determine if a site has opted into both COOP and COEP, but I don't think you can determine if the site opted in to COOP alone... Still it might help a bit.

This is my understanding too. At least at Google, we have deployed COOP widely but haven't deployed COEP very much. We also generally are encouraging services to deploy COOP before COEP. So I don't think this will help too much.

Situation 2 is also primarily an education problem, but only if Closure decides to ignore noreferrer - in which case anyone who has a legitimate reason to do this is left stranded, I think?

Personally, this seems mostly okay to me. At least at Google, I think there is only 1 service that overrides the referrer policy to unsafe-none. So I think this is probably pretty rare.

@camillelamy
Copy link

Unfortunately, this case is specific to COOP same-origin-allow-popups, which does not enable crossOriginIsolation. So we can't rely on checking if the document is crossOriginIsolated. To be clear, there is nothing preventing us from exposing the COOP status of a document, so we could add that to the platform if needed.

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
@shicks
Copy link
Member

shicks commented Sep 14, 2021

We had a realtime chat about this and came up with the following plan:

  • Ignore no-referrer in modern browsers that support COOP (since we can't detect specifically whether it's opted into COOP without COEP), as detected by presence of the window.crossOriginIsolated boolean (regardless of whether it's true or false).
  • If a modern browser uses no-referrer and has an unsafe document.referrerPolicy (IIUC we need to construct but not execute a request to determine this), then throw, so that nobody depends on no-referrer in a situation where it would matter but be ignored.

@ddworken is going to prepare a change for this, and the Closure team will help if it's nontrivial to land.

@ddworken
Copy link

Fixed by f2c85d1.

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
Projects
None yet
Development

No branches or pull requests

6 participants