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

Fix up COOP HTTP header errors #38568

Merged
merged 5 commits into from
Mar 14, 2025
Merged

Fix up COOP HTTP header errors #38568

merged 5 commits into from
Mar 14, 2025

Conversation

hamishwillee
Copy link
Collaborator

@hamishwillee hamishwillee commented Mar 11, 2025

There is a typo in the description for the HTTP COOP header, for the case of opening a page in a popup (using window.open() that makes the table wrong.

The steps come from https://html.spec.whatwg.org/multipage/browsers.html#check-browsing-context-group-switch-coop-value-popup. This directly matches the spec (I suspect I tried to invert the logic originally, hence the mismatch).

@hamishwillee hamishwillee requested a review from a team as a code owner March 11, 2025 05:26
@hamishwillee hamishwillee requested review from bsmth and removed request for a team March 11, 2025 05:26
@github-actions github-actions bot added the Content:HTTP HTTP docs label Mar 11, 2025
@hamishwillee hamishwillee requested a review from wbamberg March 11, 2025 05:27
@github-actions github-actions bot added the size/xs [PR only] 0-5 LoC changed label Mar 11, 2025
Copy link
Contributor

github-actions bot commented Mar 11, 2025

Preview URLs

Flaws (6)

URL: /en-US/docs/Web/HTTP/Reference/Headers/Cross-Origin-Opener-Policy
Title: Cross-Origin-Opener-Policy
Flaw count: 6

  • broken_links:
    • /en-US/docs/Glossary/response_header is ill cased
  • macros:
    • Macro produces link /en-US/docs/Glossary/response_header which is a redirect
    • Macro produces link /en-US/docs/Web/HTTP/Headers/Cross-Origin-Embedder-Policy which is a redirect
    • Macro produces link /en-US/docs/Web/HTTP/Headers/Permissions-Policy/cross-origin-isolated which is a redirect
    • Macro produces link /en-US/docs/Web/HTTP/Headers/Cross-Origin-Embedder-Policy which is a redirect
  • unknown:
    • No generic content config found

(comment last updated: 2025-03-13 13:12:08)

@github-actions github-actions bot added size/s [PR only] 6-50 LoC changed and removed size/xs [PR only] 0-5 LoC changed labels Mar 11, 2025
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec links in the comments were very useful to cross-check, and looks good, so a +1 from me - thanks 👍🏻

@wbamberg
Copy link
Collaborator

wbamberg commented Mar 12, 2025

I think this is definitely better. I don't have time to try a full review of this page, but of course happy to merge this as an incremental improvement. But a couple of other things you might consider now:

Line 91: BCD->BCG

Line 103: you could simplify this:

  • neither document is unsafe-none, their policy values are the same, and they are same-origin.

...by removing the first clause: if the policies have to be the same, and we already know they are not both unsafe-none, then it's determined that neither can be unsafe-none. In fact at that point it might be better not to have a separate referenced "matching policies" definition, because you can express it quite concisely as: "either both polices are unsafe-none, or the policies are the same and the documents are same-origin".

In general it's worth being really precise about language, because this is complex. For example, you have:

True: opened noopener-allow-popups

What does "opened" mean here? I think it is "The opened document has a policy of noopener-allow-popups" but it doesn't say exactly that. In the state of uncertainty I'm already in when trying to understand this, I don't want any additional uncertainty.

I think the way the table expresses the choice (New/Same) is better than the way the text does (true/false), because with the text there's another logical link (what is true?) and of course that got flipped in the original here. So I would consider rewriting the algorithm text like:

When opening a document using Window.open(), the new document is opened in a new BCG according to the following rules, which are evaluated in order:

  • If the new document has COOP set to noopener-allow-popups => open the new document in a new BCG
  • If the new document has COOP set to unsafe-none and the opener document has COOP set to either same-origin-allow-popups or noopener-allow-popups => open the new document in the same BCG
  • If the new document and the opening document have [matching COOP policies](make this a link) => open the new document in the same BCG
  • Otherwise, open the new document in a new BCG

Getting into more extensive territory:

@github-actions github-actions bot added the merge conflicts 🚧 [PR only] label Mar 13, 2025
Copy link
Contributor

This pull request has merge conflicts that must be resolved before it can be merged.

@github-actions github-actions bot removed the merge conflicts 🚧 [PR only] label Mar 13, 2025
Copy link
Contributor

Preview URLs

Flaws (6)

URL: /en-US/docs/Web/HTTP/Reference/Headers/Cross-Origin-Opener-Policy
Title: Cross-Origin-Opener-Policy
Flaw count: 6

  • broken_links:
    • /en-US/docs/Glossary/response_header is ill cased
  • macros:
    • Macro produces link /en-US/docs/Glossary/response_header which is a redirect
    • Macro produces link /en-US/docs/Web/HTTP/Headers/Cross-Origin-Embedder-Policy which is a redirect
    • Macro produces link /en-US/docs/Web/HTTP/Headers/Permissions-Policy/cross-origin-isolated which is a redirect
    • Macro produces link /en-US/docs/Web/HTTP/Headers/Cross-Origin-Embedder-Policy which is a redirect
  • unknown:
    • No generic content config found

@hamishwillee
Copy link
Collaborator Author

Thanks for the reviews.

@wbamberg I have integrated your "easy feedback" pretty much verbatim - much better. What was there before is what you get when you spend way too long looking at something and end up deciding that matching the spec is the safest option.

I am not going near those examples. I still have PTSD from the last time I was involved in this document.

@hamishwillee hamishwillee merged commit ee0dc07 into mdn:main Mar 14, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:HTTP HTTP docs size/s [PR only] 6-50 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants