-
Notifications
You must be signed in to change notification settings - Fork 325
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
Integrate CORP and COEP #1030
Integrate CORP and COEP #1030
Conversation
@domenic: PTAL I cannot make COEP related links, which I guess blocked by whatwg/html#5454. Please let me know if I'm wrong. Note:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not feeling super-qualified to review here; hopefully @annevk can help more. But I did a first pass on some basic stuff.
fetch.bs
Outdated
<p>To perform a <dfn>cross-origin resource policy check</dfn>, given a <var>request</var> and | ||
<var>response</var>, run these steps:</p> | ||
<p>To perform a <dfn>cross-origin resource policy internal check</dfn>, given a string | ||
<var>embedder policy value</var>, a <a for=/>request</a> <var>request</var> and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this can become an <a>embedder policy value</a> <var>embedder policy</var>
or similar since that term is now defined.
fetch.bs
Outdated
<p class=XXX>Fix this assertion when | ||
<a href="https://github.com/whatwg/fetch/pull/948">#948</a> is merged. | ||
|
||
<li><p>If <var>embedder policy value</var> is "<code>unsafe-none</code>", then return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would become <code><a for="embedder policy value">unsafe-none</a></code>
after the HTML PR is ready. Same for other references to unsafe-none and require-corp.
fetch.bs
Outdated
|
||
<li><p>Return <b>allowed</b>. | ||
<ul class=brief> | ||
<li><p><var>request</var>'s <a for=request>origin</a> is <a>schemelessly same site</a> with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this to schemelessly same site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I didn't change anything here. The current https://fetch.spec.whatwg.org/#cross-origin-resource-policy-check uses "schemelessly same site".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant compared to https://wicg.github.io/cross-origin-embedder-policy/#abstract-opdef-cross-origin-resource-policy-internal-check. I think the change from Fetch -> COEP spec is intentional, but I am unsure if it is tested...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. @mikewest, did you change that intentionally?
fetch.bs
Outdated
<var>request</var> and a <a for=/>response</a> <var>response</var>, run these steps:</p> | ||
<ol> | ||
<li><p>Let <var>embedder policy</var> be <var>request</var>'s <a for=request>client</a>'s | ||
embedder policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to cross-link
fetch.bs
Outdated
<p class="note no-backref">This is to queue only COEP related violation reports. | ||
|
||
<li><p>If the <a>cross-origin resource policy internal check</a> with <var>embedder policy</var>'s | ||
report only value, <var>request</var> and <var>response</var> returns <b>blocked</b>, then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"report only value" should cross-link. I guess we should export that from HTML too.
fetch.bs
Outdated
<li><p>If the <a>cross-origin resource policy internal check</a> with <var>embedder policy</var>'s | ||
report only value, <var>request</var> and <var>response</var> returns <b>blocked</b>, then | ||
<a>queue a cross-origin embedder policy CORP violation report</a> with | ||
<var>request</var> and <var>embedder policy</var>'s report only reporting endpoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for "report only reporting endpoint".
Hmm, could we make this algorithm that an "embedder policy" and a mode or something, which is "normal" or "report only"? So it doesn't have to look into the internals of the "embedder policy" struct as much? I dunno, maybe it's fine to have these algorithms so tightly coupled. But it is strange to me that they are split across different specs if so.
fetch.bs
Outdated
<li><p>If <var>request</var>'s <a for=request>mode</a> is not "<code>no-cors</code>", then return | ||
<b>allowed</b>. | ||
<li><p>Assert: <var>request</var>'s <a for=request>mode</a> is "<code>navigate</code>" or | ||
"<code>no-cors</code>". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very different than https://wicg.github.io/cross-origin-embedder-policy/#integration-fetch. I guess that is intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please see #985. The responsibility to handle requests with modes other than "no-cors" has not been clear up until now. This change clarifies that, stating that it is caller's responsibility.
591cbd1
to
7019eaa
Compare
Hi Anne, I changed the CORP check, and it now uses response's URL list instead of request's URL list. Can you take a look again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks so much better, thank you! I left a number of inline nits.
Not needed for this PR, but what do you think about moving the URL list copying and CORP check into the scheme fetch algorithm? We might not even need as much URL list copying for the other schemes as redirecting to them is forbidden.
I think having the CORP check (only) in the scheme fetch is not enough because of redirects. |
Anne, are you fine with this change except for missing links for terms defined in the HTML spec? If so I'll fix the HTML and Service Worker PRs accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this looks good to me. Might put some time into figuring out if we can refactor this further, but I think what's important is that now the correct things are compared in the various security checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'd like @domenic to take another look.
fetch.bs
Outdated
</tbody> | ||
</table> | ||
|
||
<li><p><a href="https://w3c.github.io/reporting/#queue-report">Queue</a> <var>body</var> as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can now be fixed.
Updated: Other than addressing comments I started using the "coep" report type (without link, as it is defined in whatwg/html#5454) to address whatwg/html#5634.
|
@yutakahirano I filed an issue for that. @domenic do you want to take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several TODOs around HTML cross-linking, but I'm not sure what's the latest on the sequencing of the PRs...
fetch.bs
Outdated
|
||
<ol> | ||
<li><p>Let <var>endpoint</var> be <var>settingsObject</var>'s embedder policy's | ||
report only reporting endpoint if the <var>reportOnly</var> is true and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should cross-link to HTML, which won't work until the HTML PR is merged.
fetch.bs
Outdated
<ol> | ||
<li><p>Set <var>forNavigation</var> to false if it is not given. | ||
|
||
<li><p>Let <var>embedderPolicy</var> be <var>settingsObject</var>'s embedder policy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO cross-link when HTML is merged
This PR heavily uses concepts defined in whatwg/html#5454. whatwg/html#5454 depends on two things in this PR.
It is possible to split this change to break the circular dependency, but I think it's just OK to land the HTML PR first because it seems OK to have a missing link there in terms of building. |
GitHub says this has conflicts with master, so is probably a good idea to rebase or merge it to make landing it easier. |
This is a preliminary change for COEP merging to HTML and fetch specs. We will use the serialization multiple times both in the HTML spec and the fetch spec, so defining the operation here will be benefitial.
# This is the 1st commit message: # This is a combination of 23 commits. # This is the 1st commit message: Integrate CORP and COEP This is part of the introduction of COEP (whatwg/html#5454). The CORP check now takes COEP into account. Also, responses coming from service workers are checked. # This is the commit message #2: Update fetch.bs Co-authored-by: Domenic Denicola <d@domenic.me> # This is the commit message #3: Update fetch.bs Co-authored-by: Domenic Denicola <d@domenic.me> # This is the commit message #4: fix # This is the commit message #5: fix # This is the commit message #6: fix # This is the commit message #7: fix # This is the commit message #8: fix # This is the commit message #9: fix # This is the commit message #10: fix # This is the commit message #11: fix # This is the commit message #12: fix # This is the commit message #13: fix # This is the commit message #14: fix # This is the commit message #15: fix # This is the commit message #16: fix # This is the commit message #17: fix # This is the commit message #18: Update fetch.bs Co-authored-by: Anne van Kesteren <annevk@annevk.nl> # This is the commit message #19: Update fetch.bs Co-authored-by: Anne van Kesteren <annevk@annevk.nl> # This is the commit message #20: fix # This is the commit message #21: fix # This is the commit message #22: fix # This is the commit message #23: fix # This is the commit message #2: fix
9588dd4
to
db9fbcd
Compare
@@ -4513,13 +4614,12 @@ Range Requests</cite>. [[HTTP-RANGE]] However, this is not widely supported by b | |||
</ol> | |||
</ol> | |||
|
|||
<li><p>Set <var>response</var>'s <a for=response>URL list</a> to a <a for=list>clone</a> of | |||
<var>httpRequest</var>'s <a for=request>URL list</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yutakahirano I'm having trouble understanding the implications of this change. Would you mind explaining this a little?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes a parameter of the CORP check. Currently it takes a request, but with this change it will take a response. This specific change is to set the correct URL list to the response so that the CORP check keeps working correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thank you!
This looks good to me (and Domenic I think and presumably Yutaka), it has implementer support, and it has tests. Both Chrome and Firefox have implementation bugs, including for reporting. Should we file a general one on Safari for COOP and COEP? Commit message seems reasonable (mostly deferring to HTML for information). We should probably mention that this changes "serialize a request URL for reporting". If I understand it correctly the plan would be to merge HTML first, then wait a day, address the remaining integration issues as a result of Shepherd having new terms, and then merge this? |
Yep! |
Merges https://github.com/WICG/cross-origin-embedder-policy into HTML. Associated PRs: * whatwg/fetch#1030 * w3c/ServiceWorker#1516 * w3c/css-houdini-drafts#992 Fixes #5368, fixes #5634, fixes whatwg/fetch#985, and fixes w3c/ServiceWorker#1490. Follow-up: #4916, #4919, #4930 #5223, and #5391. (As well as defining cross-origin isolated, per #4732.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Adding links, with one behavior change (sorry I didn't notice this earlier). PTAL.
and the <a>cross-origin resource policy check</a> with <var>request</var>'s | ||
<a for=request>origin</a>, <var>request</var>'s <a for=request>client</a>, and | ||
<var>actualResponse</var> returns <b>blocked</b>, then return a <a>network error</a>. | ||
<p>If either <var>request</var>'s <a for=request>response tainting</a> or <var>response</var>'s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed here because the previous logic didn't work well for responses coming from the service worker.
Tentative commit message:
Any final feedback? |
LGTM! |
Merges https://github.com/WICG/cross-origin-embedder-policy into HTML. Associated PRs: * whatwg/fetch#1030 * w3c/ServiceWorker#1516 * w3c/css-houdini-drafts#992 Fixes whatwg#5368, fixes whatwg#5634, fixes whatwg/fetch#985, and fixes w3c/ServiceWorker#1490. Follow-up: whatwg#4916, whatwg#4919, whatwg#4930 whatwg#5223, and whatwg#5391. (As well as defining cross-origin isolated, per whatwg#4732.)
It does not need to be stored on a response and therefore resulted in confusion. Also clarify that synthetic responses need to have an absolute URL in the Location header field value (Response.redirect() does this automatically). Corresponding HTML PR: TODO. Tests: TODO. Closes #631, closes #633, closes #958, and closes #1146. (Some of these can be closed due to #1030 making response's URL no longer null for network responses.)
It does not need to be stored on a response and therefore resulted in confusion. Also clarify that synthetic responses need to have an absolute URL in the Location header field value (Response.redirect() does this automatically). Corresponding HTML PR: TODO. Tests: TODO. Closes #631, closes #633, closes #958, and closes #1146. (Some of these can be closed due to #1030 making response's URL no longer null for network responses.)
It does not need to be stored on a response and therefore resulted in confusion. Also clarify that synthetic responses need to have an absolute URL in the Location header field value (Response.redirect() does this automatically). Corresponding HTML PR: whatwg/html#6340. Tests: https://chromium-review.googlesource.com/c/chromium/src/+/2665871. Closes #631, closes #633, closes #958, closes #1146, and closes web-platform-tests/wpt#10449. (Some of these can be closed due to #1030 making response's URL no longer null for network responses.)
This is part of the introduction of COEP
(whatwg/html#5454). The CORP check now takes
COEP into account. Also, responses coming from service workers
are checked.
Preview | Diff