Integrate CORP and COEP by yutakahirano · Pull Request #1030 · whatwg/fetch

yutakahirano · 2020-06-01T09:59:34Z

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

yutakahirano · 2020-06-01T10:04:35Z

@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:

The reporting logic in the CORP check in https://wicg.github.io/cross-origin-embedder-policy/ is wrong, so I fixed the logic in this PR.
The CORP check between the original client and the SW (described in https://wicg.github.io/cross-origin-embedder-policy/#integration-sw) is included, because IIRC @annevk wanted to avoid checking CORP in places other than the fetch spec as much as possible.

domenic

I'm not feeling super-qualified to review here; hopefully @annevk can help more. But I did a first pass on some basic stuff.

domenic · 2020-06-01T21:48:25Z

fetch.bs

-<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


Probably this can become an <a>embedder policy value</a> <var>embedder policy</var> or similar since that term is now defined.

fetch.bs

domenic · 2020-06-01T21:50:31Z

fetch.bs

+    <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


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.

domenic · 2020-06-01T21:53:08Z

fetch.bs


- <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


Why change this to schemelessly same site?

I think I didn't change anything here. The current https://fetch.spec.whatwg.org/#cross-origin-resource-policy-check uses "schemelessly same site".

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...

Ah I see. @mikewest, did you change that intentionally?

fetch.bs

domenic · 2020-06-01T22:00:45Z

fetch.bs

+<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.


This will need to cross-link

fetch.bs

domenic · 2020-06-01T22:01:30Z

fetch.bs

+  <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


"report only value" should cross-link. I guess we should export that from HTML too.

domenic · 2020-06-01T22:02:49Z

fetch.bs

+ <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.


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.

domenic · 2020-06-01T22:03:24Z

fetch.bs

- <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>".


This is very different than https://wicg.github.io/cross-origin-embedder-policy/#integration-fetch. I guess that is intentional?

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.

fetch.bs

yutakahirano · 2020-06-04T14:10:48Z

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?

annevk

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.

fetch.bs

yutakahirano · 2020-06-05T10:50:35Z

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.

yutakahirano · 2020-06-09T05:20:34Z

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.

annevk

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.

fetch.bs

annevk

Thanks! I'd like @domenic to take another look.

fetch.bs

annevk · 2020-06-10T13:21:44Z

fetch.bs

+   </tbody>
+  </table>
+
+ <li><p><a href="https://w3c.github.io/reporting/#queue-report">Queue</a> <var>body</var> as


This can now be fixed.

fetch.bs

yutakahirano · 2020-06-11T08:31:18Z

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.

make deploy still shows an error when I use <a for="reporting">queue</a>. Should I wait further or is there anything I should do?

annevk · 2020-06-12T11:22:01Z

@yutakahirano I filed an issue for that. @domenic do you want to take another look?

domenic

Several TODOs around HTML cross-linking, but I'm not sure what's the latest on the sequencing of the PRs...

domenic · 2020-06-12T19:04:06Z

fetch.bs

+
+<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


These should cross-link to HTML, which won't work until the HTML PR is merged.

fetch.bs

domenic · 2020-06-12T19:05:16Z

fetch.bs

+<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.


TODO cross-link when HTML is merged

fetch.bs

yutakahirano · 2020-06-16T05:03:16Z

Several TODOs around HTML cross-linking, but I'm not sure what's the latest on the sequencing of the PRs...

This PR heavily uses concepts defined in whatwg/html#5454. whatwg/html#5454 depends on two things in this PR.

serialize a response URL for reporting.
the modified parameters for the CORP check

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.

domenic · 2020-06-22T14:50:11Z

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

jugglinmike · 2020-06-25T00:27:47Z

fetch.bs

  </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>.


@yutakahirano I'm having trouble understanding the implications of this change. Would you mind explaining this a little?

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.

Got it. Thank you!

annevk · 2020-06-25T15:55:55Z

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?

domenic · 2020-06-25T16:43:05Z

If I understand it correctly the plan...

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.)

yutakahirano

Updated. Adding links, with one behavior change (sorry I didn't notice this earlier). PTAL.

yutakahirano · 2020-06-26T06:27:01Z

fetch.bs

-  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


I changed here because the previous logic didn't work well for responses coming from the service worker.

annevk · 2020-06-26T11:00:04Z

Tentative commit message:

Integrate CORP and COEP

This adds support for cross-origin embedding policy, which primarily is a way of enforcing the Cross-Origin-Resource-Policy header to be set on responses.

See also this HTML change which links tests and the various issues and standards efforts involved here: https://github.com/whatwg/html/pull/5454.

The earlier added "serialize a request URL for reporting" has been replaced with "serialize a response URL for reporting" as centering things around responses was found more logical.

Any final feedback?

domenic · 2020-06-26T15:41:24Z

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: 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.)

yutakahirano mentioned this pull request Jun 1, 2020

Introduce COEP whatwg/html#5454

Merged

3 tasks

domenic mentioned this pull request Jun 1, 2020

Meta: make "queue a report" linkable w3c/reporting#211

Merged

domenic reviewed Jun 1, 2020

View reviewed changes

Base automatically changed from yhirano/request-url-for-reporting to master June 2, 2020 08:29

annevk reviewed Jun 3, 2020

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

yutakahirano force-pushed the yhirano/coep branch from 591cbd1 to 7019eaa Compare June 4, 2020 14:07

annevk reviewed Jun 4, 2020

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

annevk reviewed Jun 5, 2020

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

annevk reviewed Jun 9, 2020

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

fetch.bs Outdated Show resolved Hide resolved

annevk reviewed Jun 10, 2020

View reviewed changes

domenic reviewed Jun 12, 2020

View reviewed changes

yutakahirano added 2 commits June 23, 2020 22:58

Define "Request URL serialization for reporting"

f083f8c

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.

yutakahirano force-pushed the yhirano/coep branch from 9588dd4 to db9fbcd Compare June 23, 2020 13:59

jugglinmike reviewed Jun 25, 2020

View reviewed changes

fix

e8b90f8

yutakahirano added 3 commits June 26, 2020 14:05

fix

1ca20a9

fix

37b2bdb

fix

1e45ecd

yutakahirano commented Jun 26, 2020

View reviewed changes

fix

ea40eaf

annevk merged commit 9ff55e4 into master Jun 26, 2020

annevk deleted the yhirano/coep branch June 26, 2020 16:52

annevk mentioned this pull request Jul 6, 2020

Sketch out a CORP-only mode. #893

Closed

annevk mentioned this pull request Jan 28, 2021

Script-created redirects responses have no location URL #1146

Closed

annevk mentioned this pull request Jan 29, 2021

Remove location URL as field on responses #1149

Merged

3 tasks

Conversation

yutakahirano commented Jun 1, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yutakahirano commented Jun 1, 2020

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yutakahirano commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yutakahirano commented Jun 5, 2020

Uh oh!

yutakahirano commented Jun 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yutakahirano commented Jun 11, 2020

Uh oh!

annevk commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yutakahirano commented Jun 1, 2020 •

edited by pr-preview bot

Loading

yutakahirano commented Jun 4, 2020 •

edited

Loading

yutakahirano commented Jun 9, 2020 •

edited

Loading

annevk commented Jun 12, 2020 •

edited

Loading