More CSP fixes #540
More CSP fixes #540
Conversation
|
A live preview of this PR will be available at the URL(s) below. |
| @@ -21,6 +21,8 @@ steps: | |||
| - --cache-ttl=168h # 1 week | |||
| # Bake in this revision's corresponding playground sandbox url | |||
| - --build-arg=PLAYGROUND_SANDBOX=https://pr$_PR_NUMBER-$SHORT_SHA---lit-dev-playground-5ftespv5na-uc.a.run.app/ | |||
| # Testing Google Analytics ID | |||
| - --build-arg=GOOGLE_ANALYTICS_ID=G-PPMSZR9W18 | |||
There was a problem hiding this comment.
So I understand correctly, this new test based google analytics will make sure if there is a csp issue we'll see it loudly in dev 🤞 ?
| // 304 responses, we cover the case where the CSP has changed, but the | ||
| // file's content (and hence ETag) has not. Note this approach would not | ||
| // work if we were using nonces instead of hashes. | ||
| if (ctx.path.endsWith('/')) { |
There was a problem hiding this comment.
When manually testing and navigating to something like: /playground, I notice a 301 redirect to /playground/. Non blocking but curious about the guarantee of a trailing slash?
For example I can still navigate to: https://pr540-ed90bcb---lit-dev-5ftespv5na-uc.a.run.app/playground/index.html - and I assume in the future this would throw?
There was a problem hiding this comment.
Yep we redirect all paths like /foo to /foo/:
But nice catch that you can access the .html filenames too! We should block those. Filed #542
Switches our Content Security Policy from report-only mode to enforced mode. According to our internal dashboard, it looks like CSP violation numbers dropped very sharply on October 1, which is the day #540 landed. There do seem to be a few reports coming in as recently as October 5, but if so it is a very small number. Could be due to caching? Browser extensions injecting scripts/images etc. will also cause ongoing CSP violations, that's expected behavior. Also adds https://www.googletagmanager.com to the img-src directive, since https://developers.google.com/tag-manager/web/csp documents that this is needed, and in one page load I did actually see a violation here in local dev mode (but not consistently -- I can't reproduce it now). I guess analytics sometimes uses images for some reason. Fixes #517 Filed #550 to track the most important improvement, which we can't do until https://bugs.chromium.org/p/chromium/issues/detail?id=1253267 is fixed.
Handle 304 cases. The reason we were getting a lot of CSP errors since Further restrict CSP and fix misfiring Google Analytics violation #532 was that we weren't accounting for 304 Not Modified responses. In those cases, no
Content-Typeheader is set, because the browser will already know it from the most recent 200 response. Our middleware incorrectly assumed it could checktypeafter 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 and here. 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).
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