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

Add Content Security Policy check on request #367

Merged
merged 1 commit into from
Nov 10, 2021
Merged

Add Content Security Policy check on request #367

merged 1 commit into from
Nov 10, 2021

Conversation

yutakahirano
Copy link
Contributor

@yutakahirano yutakahirano commented Oct 7, 2021

We only need the pre-request check. We don't need the post-request
check given there is no redirects on WebTransport.

This is for #59.


Preview | Diff

@yutakahirano
Copy link
Contributor Author

@vasilvv @mikewest @annevk could you take a look at this change?

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 reasonable in the short term, but we should give you a cleaner hook for this: you shouldn't have to create an intermediate request object.

The behavior it specifies, however, would be equivalent to this: the empty destination will result in a connect-src (or default-src) check against the URL provided, using the policies specified on the document from which the connection is being established.

Do we have WPT covering this case?

@yutakahirano
Copy link
Contributor Author

This looks reasonable in the short term, but we should give you a cleaner hook for this: you shouldn't have to create an intermediate request object.

The behavior it specifies, however, would be equivalent to this: the empty destination will result in a connect-src (or default-src) check against the URL provided, using the policies specified on the document from which the connection is being established.

Do we have WPT covering this case?

There are csp-pass.https.window.js and csp-fail.https.window.js.

@annevk
Copy link
Member

annevk commented Oct 7, 2021

I agree with @mikewest. Perhaps @antosart is willing to help with providing a lower-level hook in CSP for WebTransport.

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

antosart commented Oct 8, 2021

I agree we should provide a better hook in CSP. I will think about it but I am not totally sure how to integrate it cleanly.

Nit: Notice that [=should request be blocked by Content Security Policy?=] does not check report-only policies, which is something we probably want to do in this case.

@yutakahirano
Copy link
Contributor Author

Nit: Notice that [=should request be blocked by Content Security Policy?=] does not check report-only policies, which is something we probably want to do in this case.

Done.

We only need the pre-request check. We don't need the post-request
check given there is no redirects on WebTransport.
@yutakahirano
Copy link
Contributor Author

@martinthomson @vasilvv @jan-ivar can you take a look again?

@jan-ivar jan-ivar merged commit 94d996e into main Nov 10, 2021
github-actions bot added a commit that referenced this pull request Nov 10, 2021
SHA: 94d996e
Reason: push, by @jan-ivar

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yutakahirano yutakahirano deleted the yhirano/csp branch November 10, 2021 05:50
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.

6 participants