Further restrict CSP and fix misfiring Google Analytics violation #532
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Include a very strict CSP for all responses apart from our HTML entrypoints and playground web worker. Add some additional directives.
|
A live preview of this PR will be available at the URL(s) below. https://pr532-a3fe7c4---lit-dev-5ftespv5na-uc.a.run.app/ |
AndrewJakubowicz
approved these changes
Sep 30, 2021
| @@ -61,9 +81,14 @@ export const contentSecurityPolicyMiddleware = ( | |||
| // | |||
| // In dev mode, data: scripts are required because @web/dev-server uses them | |||
There was a problem hiding this comment.
Nit: Maybe this comment could be about L90? No action required.
Merged
aomarks
added a commit
that referenced
this pull request
Oct 1, 2021
- Handle 304 cases. The reason we were getting a lot of CSP errors since #532 was that we weren't accounting for 304 Not Modified responses. In those cases, no `Content-Type` header is set, because the browser will already know it from the most recent 200 response. Our middleware incorrectly assumed it could check `type` after awaiting the downstream Koa static middleware. We now instead set the policy based on the path of the request (anything ending in `/`), since that will work for both 200 and 304 responses. There's an interesting question about whether we *should* set a CSP at all for a 304 response. There's some discussion about that [here](w3c/webappsec-csp#161) and [here](https://bugs.chromium.org/p/chromium/issues/detail?id=174301). Chrome *does* update the CSP if it's included in a 304 response, otherwise it ignores it in favor of the CSP from the last 200 response (same with all headers, except for the ones enumerated [here](https://chromium.googlesource.com/chromium/src/net/+/refs/heads/main/http/http_response_headers.cc#81)). The advantage of including it seems to be that if we update the CSP, but the content (and hence ETag) of a file has *not* changed, then the browser would otherwise continue using the stale CSP. OTOH, if you are using a nonce based script allowlist (we use hashes instead currently), then this wouldn't really work unless you did something clever to make the ETag aware of some CSP version. - Fix policy for Google Analytics. We covered the origin needed for the initial script, but then *that* script goes on to require a connection to a different origin. - Hook up a new Google Analytics test ID that I just created, so that we will catch CSP reports related to Google Analytics during dev and in PR builds going forward. Part of #517 Part of #531
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For all requests other than our HTML entrypoints and the playground worker script, serve a super strict policy, just in case a response that shouldn't normally allow any code execution somehow actually does. Based on this and this comment.
Add some directives that I found through https://csp-evaluator.withgoogle.com/ and this comment.
Temporary fix for inline Google Analytics script which was being reported as a violation. See Improve CSP handling for google analytics script #531 for details. Will fix properly in followup.