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

More CSP fixes #540

Merged
merged 3 commits into from
Oct 1, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤞 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!


# 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('/')) {
Copy link
Contributor

@AndrewJakubowicz AndrewJakubowicz Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@aomarks aomarks Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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();
};
};