Skip to content

Commit

Permalink
More CSP fixes (#540)
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
aomarks authored Oct 1, 2021
1 parent 4890385 commit 03b2e65
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 10 deletions.
2 changes: 2 additions & 0 deletions cloudbuild-pr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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

# Create a new Cloud Run revision for the main site.
#
Expand Down
2 changes: 1 addition & 1 deletion packages/lit-dev-content/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"build:samples": "rm -rf samples/js && node ../lit-dev-tools-esm/lib/generate-js-samples.js",
"build:samples:watch": "npm run build:samples -- --watch",
"fonts:manrope": "rm -rf site/fonts temp && mkdir -p site/fonts && git clone https://github.com/sharanda/manrope.git temp/manrope && cd temp/manrope && git checkout 9ffbc349f4659065b62f780fe6e9d5a93518bd95 && cp fonts/web/*.woff2 ../../site/fonts/ && cd ../.. && rm -rf temp",
"dev:build:site": "rm -rf _dev && ELEVENTY_ENV=dev PLAYGROUND_SANDBOX=http://localhost:5416/ GITHUB_CLIENT_ID=FAKE_CLIENT_ID GITHUB_AUTHORIZE_URL=http://localhost:5417/login/oauth/authorize eleventy",
"dev:build:site": "rm -rf _dev && ELEVENTY_ENV=dev PLAYGROUND_SANDBOX=http://localhost:5416/ GITHUB_CLIENT_ID=FAKE_CLIENT_ID GITHUB_AUTHORIZE_URL=http://localhost:5417/login/oauth/authorize GOOGLE_ANALYTICS_ID=G-PPMSZR9W18 eleventy",
"dev:build:site:watch": "npm run dev:build:site -- --watch",
"dev:serve": "node ../lit-dev-tools-esm/lib/dev-server.js --open",
"format": "../../node_modules/.bin/prettier \"**/*.{ts,js,json,html,css,md}\" --write"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,12 @@ export interface ContentSecurityPolicyMiddlewareOptions {
const CSP_REPORT_URI = 'https://csp.withgoogle.com/csp/lit-dev';

/**
* TODO(aomarks) Generate this automatically. See
* TODO(aomarks) Generate these automatically. See
* https://github.com/lit/lit.dev/issues/531.
*/
const GOOGLE_ANALYTICS_INLINE_SCRIPT_HASH = `'sha256-bG+QS/Ob2lFyxJ7r7PCtj/a8YofLHFx4t55RzjR1znI='`;
const GOOGLE_ANALYTICS_INLINE_SCRIPT_HASH =
`'sha256-bG+QS/Ob2lFyxJ7r7PCtj/a8YofLHFx4t55RzjR1znI='` + // With production GA ID.
` 'sha256-RzTTI/28QrruyqG1AYHiMuUgzLJnScrkQZ+k4vM54sc='`; // With testing GA ID.

/**
* Creates a Koa middleware that sets the lit.dev Content Security Policy (CSP)
Expand Down Expand Up @@ -71,7 +73,7 @@ export const contentSecurityPolicyMiddleware = (
].join('; ');

// Policy for the main HTML entrypoints (homepage, docs, playground, etc.)
const htmlCsp = makePolicy(
const entrypointsCsp = makePolicy(
// TODO(aomarks) We should also enable trusted types, but that will require
// a policy in playground-elements for creating the worker, and a policy
// es-module-lexer for doing an eval (see next comment for more on that).
Expand All @@ -80,7 +82,7 @@ export const contentSecurityPolicyMiddleware = (
// TODO(aomarks) Remove unsafe-eval when https://crbug.com/1253267 is fixed.
// See comment below about playgroundWorkerCsp.
`'unsafe-eval'`,
`https://www.googletagmanager.com/gtag/js`,
`https://www.googletagmanager.com/`,
GOOGLE_ANALYTICS_INLINE_SCRIPT_HASH,
...(opts.inlineScriptHashes?.map((hash) => `'${hash}'`) ?? []),
// In dev mode, data: scripts are required because @web/dev-server uses them
Expand All @@ -93,7 +95,12 @@ export const contentSecurityPolicyMiddleware = (
//
// In dev mode, ws: connections are required because @web/dev-server uses
// them for automatic reloads.
`connect-src 'self' https://unpkg.com/${opts.devMode ? ` ws:` : ''}`,
`connect-src ${[
`'self'`,
'https://unpkg.com/',
'https://www.google-analytics.com/',
...(opts.devMode ? [` ws:`] : []),
].join(' ')}`,

// Playground previews and embedded YouTube videos.
`frame-src ${opts.playgroundPreviewOrigin} https://www.youtube-nocookie.com/`,
Expand Down Expand Up @@ -176,11 +183,19 @@ export const contentSecurityPolicyMiddleware = (
const strictFallbackCsp = makePolicy(`default-src 'none'`);

return async (ctx, next) => {
await next();

let policy: string;
if (ctx.response.type === 'text/html') {
policy = htmlCsp;
// Note we can't rely on ctx.type being set by the downstream middleware,
// because for a 304 Not Modified response, the Content-Type header will not
// be set.
//
// Also note that we don't necessarily need to set a CSP on 304 responses at
// all, because the browser will use the one from the previous 200 response
// if missing (as it does for all headers). However, by including a CSP on
// 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('/')) {
policy = entrypointsCsp;
} else if (ctx.path.endsWith('/playground-typescript-worker.js')) {
policy = playgroundWorkerCsp;
} else {
Expand All @@ -189,5 +204,6 @@ export const contentSecurityPolicyMiddleware = (
// TODO(aomarks) Remove -Report-Only suffix when we are confident the
// policy is working.
ctx.set('Content-Security-Policy-Report-Only', policy);
return next();
};
};

0 comments on commit 03b2e65

Please sign in to comment.