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

Introduce COEP #5454

Merged
merged 2 commits into from
Jun 25, 2020
Merged

Introduce COEP #5454

merged 2 commits into from
Jun 25, 2020

Conversation

yutakahirano
Copy link
Member

@yutakahirano yutakahirano commented Apr 13, 2020

Merging https://github.com/WICG/cross-origin-embedder-policy to the HTML spec.

  • HTML (this PR)
    • Define "embedder policy", document's embedder policy,
      WorkerGlobalScope's embedder policy, and environment settings
      object's embedder policy.
    • Specify how to parse a cross-origin-embedder-policy HTTP header.
    • Specify how to set embedder policies for documents and workers (except
      for service workers).
  • fetch: Integrate CORP and COEP fetch#1030
    • Define response URL serialization for reporting.
    • Modify the cross-origin resource policy check.
    • Run a cross-origin resource policy check for a response coming from
      a service worker.
  • service worker: Introduce Cross-Origin Embedder Policy w3c/ServiceWorker#1516
    • Define ServiceWorkerGlobalScope's embedder policy.
    • Specify how to set embedder policy for service workers.
    • Add a cross-origin resource policy check call in the Cache.matchAll
      algorithm.
  • worklet: Define COEP for worklet environment settings object w3c/css-houdini-drafts#992
    • Define WorkletWorkerGlobalScope's embedder policy.

This fixes #5368, #5634, whatwg/fetch#985, and w3c/ServiceWorker#1490.

This doesn't fix the following issues:


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


/browsers.html ( diff )
/browsing-the-web.html ( diff )
/dom.html ( diff )
/history.html ( diff )
/iana.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/offline.html ( diff )
/origin.html ( diff )
/references.html ( diff )
/webappapis.html ( diff )
/window-object.html ( diff )
/workers.html ( diff )

@yutakahirano yutakahirano changed the title [NOT REDAY FOR REVIEW YET] Introduce COEP Introduce COEP Apr 13, 2020
@yutakahirano yutakahirano marked this pull request as ready for review April 13, 2020 12:54
@yutakahirano
Copy link
Member Author

@annevk @mikewest PTAL.

I'm planning to send multiple smaller PRs rather than one gigantic PR. If you prefer otherwise, please let me know.

@annevk
Copy link
Member

annevk commented Apr 14, 2020

@domenic should weigh in I suspect, but in general we want commits to be able to stand on their own. Perhaps things can be split where infrastructure and requirements land after each other, but the moment a following commit is needed for a prior commit to make sense there's a problem of sorts.

@domenic
Copy link
Member

domenic commented Apr 14, 2020

Also note that every time you push a commit (or collection of commits) it sends an email to the 391 watchers of the repo. Not a big deal, but perhaps consider working on things locally until they're ready, instead of continually pushing "fix" commits.

@yutakahirano
Copy link
Member Author

Also note that every time you push a commit (or collection of commits) it sends an email to the 391 watchers of the repo. Not a big deal, but perhaps consider working on things locally until they're ready, instead of continually pushing "fix" commits.

This PR was a "draft PR" until I uploaded the last commit. Is notification sent for draft PRs?

@domenic
Copy link
Member

domenic commented Apr 14, 2020

Yep! All that does is prevent merging.

@yutakahirano
Copy link
Member Author

Oh I didn't know that. Sorry, and thank you.

@domenic
Copy link
Member

domenic commented May 19, 2020

What is our current status here? @yutakahirano, would you like review on what you've done so far, or were you planning on adding the rest into this PR? I tend to agree with @annevk that we should only merge when everything is present, but we could do some early review.

@yutakahirano
Copy link
Member Author

As you prefer a big one change I'll do that. I have some time this and next weeks so hopefully I will be able to upload something ready for review.

@yutakahirano
Copy link
Member Author

yutakahirano commented May 25, 2020

I think this is now ready for review. Some checks were not successful but the error message was "/bin/sh: 1: /whatwg/wattsi/build.sh: not found" so think it is an infra issue.

I simplified and inlined Initializing a global object’s Embedder policy.

I put helper algorithms (e.g., "queue a Cross-Origin Embedder Policy violation on worker initialization") closer to their callers. If you think it's better to group them into one place, I'll do that.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Great work! Mostly typographical issues. I'm happy for you to do one pass and then I or another editor can do any final touchups.

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@yutakahirano
Copy link
Member Author

I believe I've addressed the comments.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

OK, I pushed some editorial tweaks and also added the full header definition. This LGTM.

However, it seems like we should hold off merging until some of the following are done:

Does that seem right?

@domenic domenic added addition/proposal New features or enhancements topic: cross-origin-embedder-policy Issues and ideas around the new "require CORP for subresource requests and frames and etc" proposal. do not merge yet Pull request must not be merged per rationale in comment labels May 29, 2020
@domenic
Copy link
Member

domenic commented May 29, 2020

Are we planning to restrict COEP to secure contexts? I don't see that here. #4930 indicates that it might be the plan though. If so, this is probably blocked by #5558...

@yutakahirano
Copy link
Member Author

Thank you!

https://wicg.github.io/cross-origin-embedder-policy/#integration-fetch is merged into Fetch (or maybe we merge both PRs at the same time?)

Is it possible to have a reference from fetch to the HTML without this change (e.g., <a for="embedder policy">reporting value</a>? I failed to build the fetch spec locally...

@yutakahirano
Copy link
Member Author

I plan to introduce the secure-context restriction (#4930) and more reporting properties (#5391) after this is merged.

yutakahirano added a commit to whatwg/fetch that referenced this pull request Jun 1, 2020
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.
@domenic
Copy link
Member

domenic commented Jun 1, 2020

Is it possible to have a reference from fetch to the HTML without this change (e.g., <a for="embedder policy">reporting value</a>? I failed to build the fetch spec locally...

It is not; we'd need to merge HTML first then give things ~24 hours to propagate to the cross-linking databases. So I guess we should merge the HTML PR first. Does the Fetch PR build entirely on top of the HTML PR, or would it be better to land them both as close together as possible?

What is the complete list of terms you'll want to reference from Fetch? You need to manually mark them up using data-export="" and data-dfn-for="...", which is a bit fiddly. I can help if you'd like.

I plan to introduce the secure-context restriction (#4930) and more reporting properties (#5391) after this is merged.

I am a bit worried about not introducing the secure context restriction in the initial PR. But maybe it is fine. @annevk any thoughts?

yutakahirano added a commit to whatwg/fetch that referenced this pull request Jun 4, 2020
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.
@yutakahirano
Copy link
Member Author

Updated. Here is the change log from the last round:

PTAL again.

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.

I'm a bit confused now as I see this now includes reporting for navigation as well.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated
<dt>"<dfn data-x="coep-unsafe-none" data-export="" data-dfn-for="embedder policy value"><code
data-x="">unsafe-none</code>"</dfn></dt>
<dd><p>This is the default value. When this value is used, cross-origin resources can be fetched
without giving explicit permission through the <span>CORS protocol</span> or the
Copy link
Member

Choose a reason for hiding this comment

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

The CORS protocol is not at all involved, right? It seems this is old text that didn't account for the switch to CORP.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I want to say here is, with cross-origin-embedder-policy: require-corp, all cross-origin requests should be verified by CORS or CORP.

Copy link
Member

Choose a reason for hiding this comment

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

But that's not what it's saying, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

This sentence is about "unsafe-none", so cross-origin resources that are not verified by CORS or CORP can be fetched... Does this answer your question?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@yutakahirano
Copy link
Member Author

Defined the "coep" report type, to address #5634. cc: @jugglinmike

I'm a bit confused now as I see this now includes reporting for navigation as well.

Anne, I don't understand this comment. Could you rephrase it?

@annevk
Copy link
Member

annevk commented Jun 12, 2020

From a clarity perspective, I'd like OP to be updated and indicate which of topic: cross-origin-embedder-policy Issues and ideas around the new "require CORP for subresource requests and frames and etc" proposal. is being fixed here and which will remain. It would also be good to have a tentative commit message there briefly explaining how this is split across standards.

My question with respect reporting is that Fetch also includes COEP reporting for navigation, so I'm wondering why there's an algorithm here as well and how they differ.

@yutakahirano
Copy link
Member Author

Updated the PR description.

My question with respect reporting is that Fetch also includes COEP reporting for navigation, so I'm wondering why there's an algorithm here as well and how they differ.

There are two checks in the navigation process. One is the CORP check for the navigation and that will be covered by the fetch spec (the call site will exist in the HTML spec). The other is checking the parent COEP and the child COEP. They are checking different things each other, and violation reports are made independently.

@yutakahirano
Copy link
Member Author

Anne, have your comments and questions been addressed?

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I did a final pass and pushed some very minor nit fixes. This looks good to me, although I guess we should try to land this around the same time as whatwg/fetch#1030 because of the modified CORP check.

@yutakahirano
Copy link
Member Author

Thank you! Do you think it's good to start adding links whatwg/fetch#1030 before landing this? I'm asking because my ability to find and fix problems hugely rely on the build and preview tools.

@domenic
Copy link
Member

domenic commented Jun 22, 2020

Probably it's best to wait until the review is done there, so that you can add the links in a single commit and not have to go back and change anything else.

yutakahirano added a commit to whatwg/fetch that referenced this pull request Jun 23, 2020
# 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
@domenic
Copy link
Member

domenic commented Jun 24, 2020

I rebased this on master (where COOP was merged), and fixed several merge conflicts (both literal, and more indirect ones, e.g. the COOP PR referenced https://wicg.github.io/cross-origin-embedder-policy/ and I updated those sections to point to the appropriate definitions here instead). I squashed all that work into a single commit to make rebasing easier.

I also pushed an additional commit that splits the new section into a couple subsections, to be symmetrical with the COOP section.

So this looks good to go from my end. I'd like @annevk to coordinate merging this and reviewing/merging whatwg/fetch#1030. (As mentioned above, the plan is to merge this first and then update the Fetch PR with cross-links and merge it once the linking databases populate. But we probably shouldn't merge this until the Fetch PR is approved-modulo-crosslinks.)

Note that @yutakahirano's OP has a great amount of detail which will make it easy to write a useful commit message when we merge this.

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.

Thanks @yutakahirano for all your work on this! Let's land it.

@annevk
Copy link
Member

annevk commented Jun 25, 2020

Tentative commit message:

Add cross-origin embedder policy

Merges https://github.com/WICG/cross-origin-embedder-policy into HTML.

Associated PRs:

* https://github.com/whatwg/fetch/pull/1030
* https://github.com/w3c/ServiceWorker/pull/1516
* https://github.com/w3c/css-houdini-drafts/pull/992

Fixes #5368, fixes #5634, fixes https://github.com/whatwg/fetch/issues/985, and fixes https://github.com/w3c/ServiceWorker/issues/1490.

Follow-up: #4916, #4919, #4930 #5223, and #5391. (As well as defining cross-origin isolated as per #4732.)

@domenic domenic merged commit 70b8bb4 into whatwg:master Jun 25, 2020
@yutakahirano yutakahirano deleted the yhirano/coep-def branch June 26, 2020 04:26
@yutakahirano
Copy link
Member Author

Thank you!

annevk pushed a commit to whatwg/fetch that referenced this pull request Jun 26, 2020
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: whatwg/html#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.
jakearchibald pushed a commit to w3c/ServiceWorker that referenced this pull request Jul 8, 2020
* Introduce Cross-Origin Embedder Policy

This is part of whatwg/html#5454.

 - Define embedder policy in environment settings object for service
   workers.
 - Add the CORP check in #dom-cache-matchall.

* fix

* fix

* fix

* fix

* fix

* fix
@yutakahirano yutakahirano mentioned this pull request Aug 20, 2020
3 tasks
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 do not merge yet Pull request must not be merged per rationale in comment topic: cross-origin-embedder-policy Issues and ideas around the new "require CORP for subresource requests and frames and etc" proposal.
Development

Successfully merging this pull request may close these issues.

Introduce COEP to html and fetch specs
3 participants