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

Adapt to the policy container #482

Merged
merged 3 commits into from
May 5, 2021

Conversation

antosart
Copy link
Member

This is a companion PR of a PR onto the html spec adding a policy container and storing CSP lists inside the policy container. The PR onto the html allows to remove the policy inheritance parts from the CSP spec, since inheritance is defined for the policy container directly.

@antosart antosart mentioned this pull request Mar 17, 2021
3 tasks
Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable to me, and it's a nice simplification. Let's wait until the HTML patch is further along before digging in here to any more depth.

Copy link
Member

@letitz letitz left a comment

Choose a reason for hiding this comment

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

Drive-by review just because I'm curious.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

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

Quick pass, this is looking good.

@@ -994,25 +1017,9 @@ spec: INFRA; urlPrefix: https://infra.spec.whatwg.org/
populates its <a for="response">CSP list</a> accordingly:

<ol class="algorithm">
1. Set |response|'s [=response/CSP list=] to the empty list.
Copy link
Member

Choose a reason for hiding this comment

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

I think we decided in the HTML PR that Fetch can drop a response's CSP list as part of this set of changes. Are you going to put up another CL removing that dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. That needs some more work, since response's CSP list is used by the CSP spec for the navigational response checks (frame-ancestors). For now, I just made the html spec not rely on them. I would refactor the CSP spec not to rely on them and then remove the reference from Fetch in later steps, if you agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I was just looking into this, and there is still one dependency for getting rid of response's CSP list, which is this sandbox check for workers https://w3c.github.io/webappsec-csp/#sandbox-response

I'll have to think how to get rid of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the mean time, I did fix the navigational response check (i.e. frame-ancestors) to use the policy container. Since it is a small thing, I put it together inside this change.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

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

Thanks! I'm happy with where this has ended up.

index.bs Show resolved Hide resolved
annevk pushed a commit to whatwg/html that referenced this pull request May 3, 2021
This change adds the policy container concept. See also https://github.com/antosart/policy-container-explained. A policy container serves as collection of policies to be applied to a document, WorkerGlobalScope, or WorkletGlobalScope. Its purpose is to simplify how policies are initialized and inherited.

Policies are populated by parsing headers and/or meta elements. A policy container can be cloned, hence supporting inheritance of policies. Initially a policy container only contains a CSP list.

This is not meant to be a behavioral change, but rather a refactoring. Small behavioral changes introduced by this change (for example storing and reloading policies from history) address what are usually considered to be bugs in the standard/implementation (which often turn out to be security vulnerabilities).

CSP PR: w3c/webappsec-csp#482.

Service Worker PR: w3c/ServiceWorker#1588.

Helps with #4926.
@antosart antosart force-pushed the switch-to-policy-container branch from f2962f6 to 0d7956f Compare May 4, 2021 09:16
@antosart
Copy link
Member Author

antosart commented May 4, 2021

I fixed the references now that the PR on html has been merged.

index.bs Outdated Show resolved Hide resolved
@antosart antosart force-pushed the switch-to-policy-container branch from 0d7956f to 956f5b1 Compare May 4, 2021 10:50
@antosart antosart merged commit 58691e0 into w3c:main May 5, 2021
@antosart antosart deleted the switch-to-policy-container branch May 5, 2021 11:33
github-actions bot added a commit that referenced this pull request May 5, 2021
SHA: 58691e0
Reason: push, by @antosart

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
domenic pushed a commit to whatwg/html that referenced this pull request May 11, 2021
Together with a companion change to CSP (w3c/webappsec-csp#482), this enables checking policies consistently during the navigation, fixing whatwg/fetch#832 for navigation requests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants