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

Remove response check #493

Merged
merged 3 commits into from May 10, 2021
Merged

Remove response check #493

merged 3 commits into from May 10, 2021

Conversation

antosart
Copy link
Member

@antosart antosart commented May 5, 2021

This is a tentative PR to remove the CSP response check (checking the CSP of the response for a resource). This check is currently used only for blocking Workers which try to enforce sandboxing. I believe in general we do not want to consider CSP headers for a resource response, so this check is something we'd prefer not to have.

For keeping the previous behaviour for Workers, this change introduces an initialization step and check. I will upload companion PRs to enforce this check on html and ServiceWorker. I believe this check can also be reused in the future if we would like to actively do something with sandbox for Workers.

This change makes it possible to remove the response CSP list, see companion PR on fetch.

The downsides of this are that:

  • initialization now has a return value (blocked or allowed). It's not clear to me whether this is something we want to have.
  • ServiceWorker will not also have to depend on CSP.

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.

I think this is a reasonable approach. I'll wait to see the subsequent PRs before approving it, just to make sure I understand the integration points. Thanks!

index.bs Outdated Show resolved Hide resolved

Given a {{Document}} or <a for="/">global object</a> (|context|) and a <a for="/">policy</a>
(|policy|):

1. If |policy|'s <a for="policy">disposition</a> is not "`enforce`", or
|context| is not a {{Document}}, then abort this algorithm.
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 this leaves WorkletGlobalScope, which isn't handled below.

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 (although the algorithm would not do anything interesting for worklets). But I added an if.

antosart and others added 2 commits May 5, 2021 15:11
Co-authored-by: Mike West <mike@mikewest.org>
@antosart
Copy link
Member Author

antosart commented May 6, 2021

Thanks @mikewest! The subsequent PRs are ready and linked:

whatwg/fetch#1230
whatwg/html#6651
w3c/ServiceWorker#1591

@antosart antosart requested a review from mikewest May 10, 2021 09:18
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.

LGTM. Happy for you to merge this when you're ready.

@antosart antosart merged commit bf7e0bd into w3c:main May 10, 2021
@antosart antosart deleted the remove-response-csp-list branch May 10, 2021 09:27
github-actions bot added a commit that referenced this pull request May 10, 2021
SHA: bf7e0bd
Reason: push, by @antosart

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
annevk pushed a commit to whatwg/fetch that referenced this pull request May 10, 2021
Complements w3c/webappsec-csp#493. Response component of CSP is now managed at a higher level of abstraction.
domenic pushed a commit to whatwg/html that referenced this pull request May 11, 2021
This is a companion to w3c/webappsec-csp#493. That PR requires calling the check for blocking workers which try to enforce sandboxing via CSP directly from HTML.
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.

None yet

2 participants