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

Add a noopener-allow-popups value to COOP #10394

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented Jun 5, 2024

Fixes #10373

Some origins can contain different applications with different levels of security requirements. In those cases, it can be beneficial to prevent scripts running in one application from being able to open and script pages of another same-origin application.

In such cases, it can be beneficial for a document to ensure its opener cannot script it, even if the opener document is a same-origin one.

This PR adds a noopener-allow-popups Cross-Origin-Opener-Policy value that severs the opener relationship between the document loaded with this policy and its opener. At the same time, this document can open further documents (as the "allow-popups" in the name suggests) and maintain its opener relationship with them, assuming that their COOP policy allows it.

Explainer

(See WHATWG Working Mode: Changes for more details.)


💥 Error: read ECONNRESET 💥

PR Preview failed to build. (Last tried on Sep 30, 2024, 8:56 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Wattsi Server - Wattsi Server is the web service used to build the WHATWG HTML spec.

🔗 Related URL

Network error while accessing Wattsi server

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Would like to hear what @camillelamy thinks as well, but I think one change I'd like to see is that we drop "cross-origin" from the internal naming scheme now that we make it apply to same-origin scenarios.

(We probably don't want to change any of the IDs though.)

It also seems to me some other algorithms need updating here:

  • obtain a cross-origin opener policy
  • check if COOP values require a browsing context group switch

@annevk annevk added addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest labels Jun 10, 2024
@yoavweiss
Copy link
Contributor Author

I think one change I'd like to see is that we drop "cross-origin" from the internal naming scheme now that we make it apply to same-origin scenarios

Would it make sense to spin off this editorial only change to a separate PR? (happy to work on it, just wondering RE editorial vs. functional split)

  • obtain a cross-origin opener policy

Oops, added!

  • check if COOP values require a browsing context group switch

I think this is covered by the change to matching COOP, but I could be I'm missing something..

@annevk
Copy link
Member

annevk commented Jun 10, 2024

@yoavweiss I think once we have agreement for this PR, that could be a separate PR as well (to be landed first). It would be a bit of extra work, but I agree that it would be nicer.

@past past added the agenda+ To be discussed at a triage meeting label Jun 17, 2024
@past past removed the agenda+ To be discussed at a triage meeting label Jun 21, 2024
yoavweiss added a commit to yoavweiss/WebKit that referenced this pull request Jul 2, 2024
https://bugs.webkit.org/show_bug.cgi?id=275147

Reviewed by NOBODY (OOPS!).

The `noopener-allow-popups` COOP value would enable a document to ensure it can't be scripted by other same-origin documents that have opened it.

Some origins can contain different applications with different levels of security requirements.
In those cases, it can be beneficial to prevent scripts running in one application from being able to open and script pages of another same-origin application.

The noopener-allow-popups Cross-Origin-Opener-Policy value severs the opener relationship between the document loaded with this policy and its opener.
At the same time, this document can open further documents (as the "allow-popups" in the name suggests) and maintain its opener relationship with them, assuming that their COOP policy allows it.

This implements whatwg/html#10394

* LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/resources/reporting-common.js:
(const.coopHeaders): A helper to create headers in a more succinct way.
* LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/tentative/access-to-noopener-page-from-no-coop-ro.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/reporting/tentative/access-to-noopener-page-from-no-coop-ro.https.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/resources/noopener-helper.js: Added.
(getExecutorPath):
(const.test_noopener_opening_popup): The logic for the noopener tests.
(async const):
* LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/tentative/noopener/coop-noopener-allow-popups.https-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/cross-origin-opener-policy/tentative/noopener/coop-noopener-allow-popups.https.html: Added.
* Source/WebCore/loader/CrossOriginOpenerPolicy.cpp:
(WebCore::crossOriginOpenerPolicyToString): Add the "noopener-allow-popups" string.
(WebCore::crossOriginOpenerPolicyValueToEffectivePolicyString): Add the "noopener-allow-popups" string.
(WebCore::matchingCOOP): Implement the related HTML algorithm.
(WebCore::coopValuesRequireBrowsingContextGroupSwitch): Implement the switching logic related to noopener-allow-popups.
(WebCore::obtainCrossOriginOpenerPolicy): Parse the "noopener-allow-popups" value.
* Source/WebCore/loader/CrossOriginOpenerPolicy.h: Add the noopener-allow-popups enum value.
* Source/WebKit/Shared/WebCoreArgumentCoders.serialization.in: Add the noopener-allow-popups enum value.
@yoavweiss
Copy link
Contributor Author

I added @ddworken's commented risks as a note.
I thought that a "browser context group switch" also implied a potentially separate process. @arturjanc - should I understand from your comment that you'd like to see something more explicit here?

@yoavweiss yoavweiss added agenda+ To be discussed at a triage meeting and removed agenda+ To be discussed at a triage meeting labels Jul 30, 2024
@arturjanc
Copy link

@arturjanc - should I understand from your comment that you'd like to see something more explicit here?

Yes, I think it could be helpful to at least add a note somewhere saying that browser should aim to put documents with this COOP value in a separate renderer process (or something along these lines) -- otherwise, same-origin documents would still be able to read arbitrary from memory even if they can't directly access it using web-level APIs.

@annevk
Copy link
Member

annevk commented Aug 21, 2024

@yoavweiss did you open a PR already to drop "cross origin" from the internal concepts?

@yoavweiss
Copy link
Contributor Author

@yoavweiss did you open a PR already to drop "cross origin" from the internal concepts?

Was out, will do that now..

@yoavweiss yoavweiss added the agenda+ To be discussed at a triage meeting label Sep 3, 2024
domfarolino pushed a commit that referenced this pull request Sep 12, 2024
This is a refactor-only change, that serves as a precursor to #10394, and makes progress on #10373.
@past past removed the agenda+ To be discussed at a triage meeting label Sep 12, 2024
@domfarolino domfarolino added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Sep 13, 2024
Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

This is looking good, I have nothing but nits.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
Copy link
Contributor Author

@yoavweiss yoavweiss left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing!

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@shhnjk
Copy link
Contributor

shhnjk commented Sep 16, 2024

It'd be nice to also document the vector in this comment.

@yoavweiss
Copy link
Contributor Author

It'd be nice to also document the vector in this comment.

I believe it's covered in the note under https://whatpr.org/html/10394/browsers.html#coop-noopener-allow-popups

@yoavweiss
Copy link
Contributor Author

It'd be nice to also document the vector in this comment.

I believe it's covered in the note under https://whatpr.org/html/10394/browsers.html#coop-noopener-allow-popups

@shhnjk - or did you mean that the note should cover Cache API usage as well?

@shhnjk
Copy link
Contributor

shhnjk commented Sep 16, 2024

It'd be nice to also document the vector in this comment.

I believe it's covered in the note under https://whatpr.org/html/10394/browsers.html#coop-noopener-allow-popups

The note talks about Service worker installation as a risk from same-origin application, but I think if the sensitive application uses a service worker, those SW caches can be manipulated without Service worker installation from the same-origin application.

i.e.:

// A script running on /chat.
// Assuming the sensitive application has a service worker which returns cached contents.
caches.open("v1").then(cache => {
  const init = {
    headers: {
      'Content-Type': 'text/html'
    }
  };
  cache.put('/password', new Response('<img src=x onerror=alert(origin)>', init));
})

@yoavweiss
Copy link
Contributor Author

Added a separate line for "Cache API manipulation or access to sensitive data". Thanks!!

data-x="">httponly</code>.</p></li>
<li><p>localStorage access to sensitive data.</p></li>
<li><p>Service worker installation.</p></li>
<li><p>Cache API manipulation or access to sensitive data.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! Let me know if this matches what you had in mind

source Show resolved Hide resolved
<var>responseCOOPValue</var>, and <var>responseOrigin</var> is true, return false.</p></li>
<var>responseCOOPValue</var>, and <var>responseOrigin</var> is true, then return false.</p></li>

<li><p>If <var>activeDocumentCOOPValue</var> is "<code
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about this condition and how it relates to what I'm seeing in https://chromium-review.googlesource.com/c/chromium/src/+/5581251/25/content/browser/security/coop/cross_origin_opener_policy_status.cc. I'm not super familiar with COOP impl specifics, so I'm probably missing something but it seems in Chromium when an initiator has noopener-allow-popups and the destination/response has same-origin-allow-popups we only conditionally "no swap" (based on origin), but the spec here unconditionally returns false?

Is this a real difference or am I incorrectly comparing two different things?

Copy link
Member

Choose a reason for hiding this comment

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

Yes this seems like it would prevent the popup from being opened at all. When you open a popup, you create an about:blank document that inherits COOP from its creator, so in this case noopener-allow-popups. With this clause, then the navigation from noopener-allow-popups about:blank to the actual popup would always trigger a BCG swap. Which is not what you want. This is why same-origin-allow-popups has the conditions below about not swapping when you're the initial about:blank document. You should do something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@domfarolino - I think you're right and for same-origin-allow-popups we should check for an origin here.

@camillelamy - I'm not sure the scenario you're describing matches my read of the spec, so I'd love to better understand it. For a navigation from noopener-allow-popups about:blank to the actual popup, I think this line returns "false", which means no BCG swap will take place. Am I reading it wrong?

Copy link
Member

Choose a reason for hiding this comment

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

In the about:blank popup case, the line will not return false because activeDocumentCOOP value is noopener-allow-popups and the value of responseCOOP is unsafe-none. So they don't match, per line 86864 in this PR.

The step 2 of this algorithm (if all of the following are true...) deals specifically with the popup case. You should modify the following in this step:

so that it also applies if activeDocumentCOOPValue is noopener-allow-popups.

However, this line will return false when navigating an about:blank noopener-allow-popups to another noopener-allow-popups page, meaning that the noopener-allow-popups popup will keep its opener (which is itself noopener-allow-popups). I think this would be unexpected.

<var>responseCOOPValue</var>, and <var>responseOrigin</var> is true, return false.</p></li>
<var>responseCOOPValue</var>, and <var>responseOrigin</var> is true, then return false.</p></li>

<li><p>If <var>activeDocumentCOOPValue</var> is "<code
Copy link
Member

Choose a reason for hiding this comment

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

In the about:blank popup case, the line will not return false because activeDocumentCOOP value is noopener-allow-popups and the value of responseCOOP is unsafe-none. So they don't match, per line 86864 in this PR.

The step 2 of this algorithm (if all of the following are true...) deals specifically with the popup case. You should modify the following in this step:

so that it also applies if activeDocumentCOOPValue is noopener-allow-popups.

However, this line will return false when navigating an about:blank noopener-allow-popups to another noopener-allow-popups page, meaning that the noopener-allow-popups popup will keep its opener (which is itself noopener-allow-popups). I think this would be unexpected.


<ol>
<li><p>If <var>responseCOOPValue</var> is "<code
data-x="coop-unsafe-none">unsafe-none</code>", then return false.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This means that we will not trigger a BCG switch when navigating from noopener-allow-popups to unsafe-none. This is different from the same-origin-allow-popups behavior, where this is only true for the intial navigation in a popup. I would recommend removing this and aligning on same-origin-allow-popups here.

<li><p>If <var>responseCOOPValue</var> is "<code
data-x="coop-unsafe-none">unsafe-none</code>", then return false.</p></li>

<li><p>If <var>responseCOOPValue</var> is "<code
Copy link
Member

Choose a reason for hiding this comment

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

So navigating from noopener-allow-popups to same-origin-allow-popups doesn't trigger a BCG switch, but the reverse does? I think this is likely to be confusing, especially when you factor in back navigations.

For example, consider the following scenario:

  • You're on page A with noopener-allow-popups. Page A opens popup B. Communication with the popup works.
  • Page A navigates to a same -origin same-origin-allow-popups page. Communication with the popup works.
  • The user goes back to the noopener-allow-popups page. Communication with the popup stops working. This might be unexpected to the original noopener-allow-popups page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation needs implementer interest Moving the issue forward requires implementers to express interest
Development

Successfully merging this pull request may close these issues.

Severing a document's opener relationship regardless of origin
7 participants