Add a default content security policy to the skeleton template #1235

blittle · 2023-08-16T21:42:27Z

WHY are these changes introduced?

Hydrogen provides no default content security policy (CSP) implementation at the moment. This means that any site created with the default templates is subject to cross-site-scripting and embedded content vulnerabilities. This adds the following primitives and updates the templates:

createContentSecurityPolicy - returns a provider, nonce, and header all at once. I think making the provider not directly exposed is better, because it should only ever be rendered inside entry.server.jsx, right where the CSP is created. In fact I could create a warning if we detect createContentSecurityPolicy is called from the browser.
useNonce - This is how you get access to the nonce inside actual react components.
<Script> - The same as a normal <script> except the nonce is automatically added

templates/skeleton/app/entry.server.tsx

templates/skeleton/server.ts

blittle · 2023-08-16T21:52:26Z

@davidhousedev @duncan-fairley I'm curious if you all use a CSP in your sites? If so, do you use nonces and how have you set them up with Remix?

wizardlyhel · 2023-08-16T23:21:50Z

I don't think there will be hydration issue if we set client with nounce="" - https://github.com/kentcdodds/nonce-hydration-issues

I feel we can abstract some Hydrogen components:

<ScriptCSP> - renders script tag with nounce
<CSPProvider> - nounce provider
whatever element needs to add a nounce

While the codes in server.ts seems dynamic, at the very least, I think we can provide:

const {requestWithNounce, nounce} = setNounce(request) - sets the nounce on request for SSR

createCSPHeader(nounce) - create the Content-Security-Policy header with a recommended default and allow for optional override on any directives .. like

createCSPHeader(nounce, {
  'default-src': `'self' cdn.shopify.com checkout.hydrogen.shop`
})

Since graphqli route is a virtual route, can we just define a separate CSP header so that it is valid? It's only gonna exist in dev mode anyways.

Our template uses some inline styles, so I laxed the default policy to allow "unsafe-inline" styles. Is that okay?

Feels like lazy on our part if we allow it. If we got lazy, people who clone with our template will also be lazy. (edit) This is bad. unsafe-inline also allows inline scripts to execute.

Questions:

How would nounce work for dynamic scripts on sub navigation? For example, I have a instagram feed that only show up on about page. I don't want this script to load on home page and only loads it when user navigates to about page.

Would the loader requests contain the nounce? does this validates the CSP? Should each loader request creates a new nounce? but that won't match the main request nounce since it is a single page app.

Reading a couple resources, I think we definitely should have a recommended setting with clear instructions on what it allows. CSP in SPA seems like a bit more work is involved.

https://auth0.com/blog/deploying-csp-in-spa/

davidhousedev · 2023-08-17T10:38:58Z

@blittle unfortunately this isn't something we've had a chance to look into yet. I'm definitely curious to see what you all recommend!

wizardlyhel · 2023-08-17T15:31:35Z

One more thing we need to look into is that if there are any missing supports that we need from Remix. We would need to add support for nounce from entry.server as well with renderToReadableStream facebook/react#26738

blittle · 2023-08-17T20:44:54Z

@wizardlyhel I think this actually can be much simpler, I think all of the logic in server.ts can be avoided, and instead everything just inside entry.server.tsx. Thats where the nonce can be generated, added to a provider, and the CSP header can be added to the return Response object.

blittle · 2023-08-17T21:48:21Z

@wizardlyhel I updated the PR to package the following functionality into Hydrogen:

generateNonce() - A utility for generating a script nonce.
<CSPProvider nonce={...} /> - A provider component that just is rendered inside entry.server.tsx
createCSPHeader(nonce, {...}) - A utility for creating the CSP header, it relies on https://www.npmjs.com/package/content-security-policy-builder, which is 5kb and used by HelmetJS
<Script /> component that automatically adds a nonce
useNonce() hook that must be called from within root.tsx to pass the nonce to the Remix <Scripts /> component.

I think this works, but I wish it all could somehow be wrapped up better in a way where we don't need to expose so many individual exports. But it's tricky to do while both React components and non-react functionality all access to the nonce.

blittle · 2023-08-17T22:01:13Z

@juanpprieto at the moment the default CSP allows inline styles, this is because the skeleton route uses inline styles for active elements. I assume this is necessary for now.

wizardlyhel

Nice~ This is much cleaner

wizardlyhel · 2023-08-17T22:00:23Z

packages/hydrogen/src/csp/csp.ts

+function toHexString(byteArray: Uint8Array) {
+  return Array.from(byteArray, function (byte) {
+    return ('0' + (byte & 0xff).toString(16)).slice(-2);
+  }).join('');
+}


Can we reuse buildUUID function from hydrogen-react?

It isn't a UUID though, at least all nonce's I've seen are not.

wizardlyhel · 2023-08-17T22:02:26Z

packages/hydrogen/src/csp/Script.tsx

+export const Script = forwardRef<HTMLScriptElement, ScriptProps>(
+  (props, ref) => {
+    const nonce = useNonce();
+    return <script {...props} nonce={nonce} ref={ref} />;
+  },
+);


Do we need to make this export a different name than Script to avoid import confusion?

What kind of import confusion? I assume if we later have other things we do for 3p scripts, we wouldn't want multiple Script components.

Along with that, perhaps we make the CSPProvider a more generic HydrogenProvider, so down the road we can add more logic to a single provider, instead of exposing many?

If we add, for example, integration with partytown or something, would we add it to this same component?

Also, +1 to have a generic provider for Hydrogen instead of CSP-specific.

frandiox

Thanks for pushing this Bret!

A couple of things:

We should also add the nonce to the scripts in the ErrorBoundary (including the ScrollRestoration like I did in the suggestion comment below).
MiniOxygen is adding a script for live reload and that one doesn't include the nonce, so refreshing on changes breaks. I'll try to fix this and release a new version of MiniOxygen. ==> https://github.com/Shopify/mini-oxygen/pull/522

packages/hydrogen/src/csp/csp.ts

templates/skeleton/app/utils.ts

packages/hydrogen/src/csp/csp.ts

frandiox · 2023-08-18T00:39:47Z

packages/hydrogen/src/csp/Script.tsx

+export const Script = forwardRef<HTMLScriptElement, ScriptProps>(
+  (props, ref) => {
+    const nonce = useNonce();
+    return <script {...props} nonce={nonce} ref={ref} />;
+  },
+);


If we add, for example, integration with partytown or something, would we add it to this same component?

Also, +1 to have a generic provider for Hydrogen instead of CSP-specific.

templates/skeleton/app/root.tsx

.changeset/two-carrots-relax.md

blittle · 2023-08-18T16:06:05Z

@frandiox I really like the simplicity of having all the logic only within entry.server.tsx (adding the provider, getting the nonce, and setting the CSP header), the downside is that only React rendered routes get the CSP. So a static HTML file, like /graphiql does not get the CSP. That's convenient because we don't need to add logic for the /graphiql script, but at the same time, maybe we should be sending the CSP header for all responses? Part of me says no, because it is a decent sized header, and I don't think it should be sent for all loader and action requests. Thoughts?

frandiox · 2023-08-21T13:39:36Z

So a static HTML file, like /graphiql does not get the CSP. That's convenient because we don't need to add logic for the /graphiql script, but at the same time, maybe we should be sending the CSP header for all responses? Part of me says no, because it is a decent sized header, and I don't think it should be sent for all loader and action requests. Thoughts?

GraphiQL is also only shown in development so we don't need to worry about that one.
For other ""static"" HTML files (returning HTML from a loader), we should just mention in the docs that if you opt-out of React rendering, you should handle CSP manually as well (in scripts and also in the headers in the loader response). This situation should be rather rare, I think.

That leaves us with API responses... I'm not an expert on this topic but I think API responses wouldn't benefit from most of the uses of a CSP. There might be some minor benefits but perhaps something we can revisit when it's simpler to set it up in Remix? Right now we'd need to also modify server.ts for that and I'm not sure if the extra complexity for the users is worth it.

packages/hydrogen/src/csp/csp.ts

.changeset/two-carrots-relax.md

wizardlyhel · 2023-08-28T18:45:40Z

We should add the monorail connect-src as part of default ... or add as part of the instructions for Shopify Analytics

frandiox

I like how this has evolved 👍

frandiox · 2023-08-30T00:08:51Z

packages/hydrogen/src/csp/csp.ts

+    styleSrc: ["'self'", "'unsafe-inline'", 'https://cdn.shopify.com'],
+    connectSrc: ['self', 'https://monorail-edge.shopifysvc.com'],


These might be overwritten by mistake when adding more domains. Should we have a different strategy to merge directives so that we always include Shopify defaults?

Yeah I went back and forth on that. I decided to keep make sure the nonce is always merged in, because that's the main value prop we are providing. So if someone doesn't want the nonce, they should just do their own CSP manually. But if we automatically merge domains as well? If they are deploying off oxygen, there's a high chance that they wouldn't want the shopify domains at all.

blittle commented Aug 16, 2023

View reviewed changes

templates/skeleton/app/entry.server.tsx Outdated Show resolved Hide resolved

blittle commented Aug 16, 2023

View reviewed changes

templates/skeleton/server.ts Outdated Show resolved Hide resolved

blittle commented Aug 16, 2023

View reviewed changes

templates/skeleton/server.ts Outdated Show resolved Hide resolved

github-actions bot deployed to preview August 17, 2023 21:42 View deployment

wizardlyhel reviewed Aug 17, 2023

View reviewed changes

frandiox reviewed Aug 18, 2023

View reviewed changes

juanpprieto reviewed Aug 18, 2023

View reviewed changes

.changeset/two-carrots-relax.md Show resolved Hide resolved

blittle force-pushed the bl-csp branch from 8066aa5 to 3854c4b Compare August 18, 2023 15:03

github-actions bot deployed to preview August 18, 2023 15:05 View deployment

github-actions bot deployed to preview August 18, 2023 16:04 View deployment

github-actions bot deployed to preview August 18, 2023 16:10 View deployment

blittle marked this pull request as ready for review August 18, 2023 16:28

frandiox mentioned this pull request Aug 21, 2023

Update MiniOxygen to support nonce in autoreload script #1251

Merged

github-actions bot deployed to preview August 21, 2023 17:15 View deployment

github-actions bot deployed to preview August 22, 2023 22:14 View deployment

frandiox reviewed Aug 23, 2023

View reviewed changes

packages/hydrogen/src/csp/csp.ts Outdated Show resolved Hide resolved

frandiox reviewed Aug 23, 2023

View reviewed changes

packages/hydrogen/src/csp/csp.ts Show resolved Hide resolved

github-actions bot deployed to preview August 23, 2023 19:03 View deployment

github-actions bot deployed to preview August 23, 2023 19:13 View deployment

github-actions bot deployed to preview August 23, 2023 19:49 View deployment

github-actions bot deployed to preview August 23, 2023 20:01 View deployment

github-actions bot deployed to preview August 23, 2023 20:39 View deployment

blittle commented Aug 23, 2023

View reviewed changes

.changeset/two-carrots-relax.md Outdated Show resolved Hide resolved

github-actions bot deployed to preview August 23, 2023 20:40 View deployment

github-actions bot deployed to preview August 23, 2023 20:41 View deployment

blittle added 13 commits August 29, 2023 15:35

Add a default content security policy to the skeleton template

2944959

Update

77859e4

Fix CSP errors and introduce HydrogenServerProvider

5e98271

Add nonce to error boundary

7150190

Fix csp to work without a nonce

35c0cf6

Add CSP tests

16476cc

Add docs and rework

06d2367

More docs fixes

b0d7340

Updates

83ed363

Add comment

f061104

Update all templates and examples

eb24b27

Delete weak-drinks-refuse.md

015f42e

Update .changeset/two-carrots-relax.md

27ffc91

blittle force-pushed the bl-csp branch from 20be551 to 27ffc91 Compare August 29, 2023 19:36

github-actions bot deployed to preview August 29, 2023 19:38 View deployment

Add monorail to the default CSP

d8c2bd4

github-actions bot deployed to preview August 29, 2023 19:43 View deployment

Fix tests

b8397ab

github-actions bot deployed to preview August 29, 2023 19:54 View deployment

frandiox approved these changes Aug 30, 2023

View reviewed changes

Add a link to the guide in the release notes

b807004

github-actions bot deployed to preview August 30, 2023 18:25 View deployment

blittle merged commit 1f8acd7 into main Aug 30, 2023

blittle deleted the bl-csp branch August 30, 2023 18:32

shopify-github-actions-access bot mentioned this pull request Aug 30, 2023

[ci] release 2023-07 #1263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a default content security policy to the skeleton template #1235

Add a default content security policy to the skeleton template #1235

blittle commented Aug 16, 2023 •

edited

Loading

blittle commented Aug 16, 2023

wizardlyhel commented Aug 16, 2023 •

edited

Loading

davidhousedev commented Aug 17, 2023

wizardlyhel commented Aug 17, 2023

blittle commented Aug 17, 2023

blittle commented Aug 17, 2023

blittle commented Aug 17, 2023

wizardlyhel left a comment

wizardlyhel Aug 17, 2023

blittle Aug 17, 2023

wizardlyhel Aug 17, 2023

blittle Aug 17, 2023

blittle Aug 17, 2023

frandiox Aug 18, 2023

frandiox left a comment •

edited

Loading

frandiox Aug 18, 2023

blittle commented Aug 18, 2023

frandiox commented Aug 21, 2023

wizardlyhel commented Aug 28, 2023 •

edited

Loading

frandiox left a comment

frandiox Aug 30, 2023

blittle Aug 30, 2023

		styleSrc: ["'self'", "'unsafe-inline'", 'https://cdn.shopify.com'],
		connectSrc: ['self', 'https://monorail-edge.shopifysvc.com'],

Add a default content security policy to the skeleton template #1235

Add a default content security policy to the skeleton template #1235

Conversation

blittle commented Aug 16, 2023 • edited Loading

WHY are these changes introduced?

blittle commented Aug 16, 2023

wizardlyhel commented Aug 16, 2023 • edited Loading

davidhousedev commented Aug 17, 2023

wizardlyhel commented Aug 17, 2023

blittle commented Aug 17, 2023

blittle commented Aug 17, 2023

blittle commented Aug 17, 2023

wizardlyhel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frandiox left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blittle commented Aug 18, 2023

frandiox commented Aug 21, 2023

wizardlyhel commented Aug 28, 2023 • edited Loading

frandiox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blittle commented Aug 16, 2023 •

edited

Loading

wizardlyhel commented Aug 16, 2023 •

edited

Loading

frandiox left a comment •

edited

Loading

wizardlyhel commented Aug 28, 2023 •

edited

Loading