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

Security - Can't use CSP that blocks 'unsafe-inline'; #183

Closed
Graham42 opened this issue May 24, 2021 · 32 comments
Closed

Security - Can't use CSP that blocks 'unsafe-inline'; #183

Graham42 opened this issue May 24, 2021 · 32 comments

Comments

@Graham42
Copy link
Contributor

To improve security and mitigate cross site scripting threats, sites should set a Content Security Policy (CSP). I would like to restrict to script-src: self to block inline scripts, but Remix crashes on hydration when this is set.

Remix version: 0.17.2

Workaround: don't hydrate, server rendered pages still work 👌 🚀

@Graham42
Copy link
Contributor Author

Inspecting the source I see there's the __remixContext which looks like a good candidate for an inline json block. I wrote about an approach for Inline Data With a Content Security Policy a while ago that might be of interest. Looking back, I think I was a bit verbose writing that post, but hopefully the content can help.

@sergiodxa
Copy link
Member

I was going to suggest changing to use script[type="application/json"] If that worked with the script-src: 'self' CSP, good to know that works, maybe Remix should switch to that, also since there's only one script for the content it could use something like:

<script type="application/json" id="remix-content">data here</script>

And then query by the ID.

@ryanflorence
Copy link
Member

Hey thanks for opening the issues, we've got an internal one we've been planning on finishing up. Any opinions on nonce vs. json?

@Graham42
Copy link
Contributor Author

Graham42 commented May 24, 2021

Nonce doesn't work if you want caching as it needs to be generated for every single request, otherwise it's trivial to bypass. I have a section on "Nonce-based CSP" in the post I linked.

@ryanflorence
Copy link
Member

ryanflorence commented May 24, 2021

The thing that's been tripping me up is not the __remixContext script but the inline <script type="module"> where we import all of the bundles on the page and assign them to a module cache for hydration.

The output ends up something like this:

import * as route0 from "/build/root-G7K4STZZ.js";
import * as route1 from "/build/docs/routes/version-IGSESPQ6.js";
import * as route2 from "/build/docs/routes/index-NTPBUVHK.js";
    window.__remixRouteModules = {"root":route0,"docs/routes/version":route1,"docs/routes/index":route2};

https://github.com/remix-run/remix/blob/master/packages/remix-react/components.tsx#L567-L577

This inline script won't ever contain user data, so it should be fine, but I don't think it's compatible with the CSP we'd like people using.

@ryanflorence
Copy link
Member

ryanflorence commented May 26, 2021

I think we can get rid of this inline script.

  1. Put __remixContext into a json script tag
  2. Put the matches in the same __remixContext (they're already there I think)
  3. Move the inline module into the client entry bundle
  4. use dynamic import of the matches
  5. assign the __remixRouteModules
  6. dynamically import the app's client entry
  7. make sure all dynamically imported entry modules are modulepreloaded to avoid waterfalls

@dchristensen
Copy link

I'll add that the Scroll Restoration code also adds some additional script inline that would need to mitigated as well:
https://github.com/remix-run/remix/blob/main/packages/remix-react/scroll-restoration.tsx

@Graham42
Copy link
Contributor Author

That blog post above that I wrote is out of date, and my memory of this topic has turned a bit fuzzy, but I think there should be a way in Remix to provide all the correct hashes for inline scripts, since Remix takes care of the bundling. I'm wary of the nonce approach as I remember that has been misused by other bundlers in the past. It's at odds with the desire to cache things as much as possible since the nonce value must be a new unique random generated value for every page load.
Things have been pretty busy for me so I haven't had a chance to look at this, but I'm hoping to revisit mid December.

@renchap
Copy link

renchap commented Jan 20, 2022

Using hashes for inline scripts is probably a better idea than nonces, but this can be an issue for streaming responses as you need to send the headers before starting generating the content.

Does Remix start streaming the response before it has been fully computed?

@fnimick
Copy link

fnimick commented Mar 31, 2022

Any word on this? I'd like to set a content security policy for a new project and this is preventing us from doing so.

@ngbrown
Copy link
Contributor

ngbrown commented Jun 13, 2022

It's possible to use a content-security-policy with nonce. I could only use a nonce on the default export of root.tsx, not the CatchBoundary or ErrorBoundary exports.

export const loader: LoaderFunction = async () => {
  const cspScriptNonce = cryptoGetRandomString(33);
  const data: RootLoaderData = {
    cspScriptNonce,
  };
  return data;
};

export const unstable_shouldReload = () => false;

export default function App() {
  const { cspScriptNonce } = useLoaderData<RootLoaderData>();
  return (
    <html lang="en">
      <head>
        <Meta />
        <Links />
      </head>
      <body>
        <Outlet />
        <ScrollRestoration nonce={cspScriptNonce} />
        <Scripts nonce={cspScriptNonce} />
        <LiveReload nonce={cspScriptNonce} port={8002} />
      </body>
    </html>
  );
}

And in entry.server.tsx, I added this to handleRequest():

    const nonce =
      remixContext.appState.catchBoundaryRouteId === "root" &&
      remixContext.appState.error
        ? undefined
        : remixContext.routeData.root?.cspScriptNonce;
    responseHeaders.set(
      "Content-Security-Policy",
      getContentSecurityPolicy(nonce)
    );

Most of the complexity is in dynamically generating getContentSecurityPolicy() based on the presence of a nonce and NODE_ENV:

function getContentSecurityPolicy(nonce?: string) {
  let script_src: string;
  if (typeof nonce === "string" && nonce.length > 40) {
    script_src = `'self' 'report-sample' 'nonce-${nonce}'`;
  } else if (process.env.NODE_ENV === "development") {
    // Allow the <LiveReload /> component to load without a nonce in the error pages
    script_src = "'self' 'report-sample' 'unsafe-inline'";
  } else {
    script_src = "'self' 'report-sample'";
  }

  const connect_src =
    process.env.NODE_ENV === "development"
      ? "'self' ws://localhost:*"
      : "'self'";

  return (
    "default-src 'self'; " +
    `script-src ${script_src}; ` +
    "style-src 'self' 'report-sample'; img-src 'self' data:; font-src 'self'; " +
    `connect-src ${connect_src}; ` +
    "media-src 'self'; object-src 'none'; " +
    "prefetch-src 'self'; " +
    "child-src 'self'; " +
    "frame-src 'self'; worker-src 'self' blob:; frame-ancestors 'none'; " +
    "form-action 'self'; " +
    "block-all-mixed-content; " +
    "base-uri 'self'; manifest-src 'self'"
  );
}

Also, react-dom generates some error messages because it doesn't handle rehydration checking correctly on nonce since it can't be read as an attribute.

@davidpfahler
Copy link

@ngbrown Is there anything we can do about the "Prop nonce did not match." warning? Why is there a mismatch even? Why is the nonce empty string on the server?

@ngbrown
Copy link
Contributor

ngbrown commented Aug 12, 2022

Hi @davidpfahler, I'm sure you're discovering that CSP is a fairly complex feature. With as little information you've given it's hard to know what is going on. Maybe open a discussion for more interactive follow up?

@davidpfahler
Copy link

@ngbrown Sorry for being unclear. I was referring to your earlier comment saying

Also, react-dom generates some error messages because it doesn't handle rehydration checking correctly on nonce since it can't be read as an attribute.

I thought that what you meant by that was that there is an error in the browser console, namely:

Warning: Prop `nonce` did not match. Server: "" Client: "irn2qSvqJ-0xG9HuIos8DrJjQBtqYMk8L3PVz4qyG74muQ"

@EvgeniyBudaev
Copy link

@ngbrown Where can I get responseHeaders? Where can i see code cryptoGetRandomString?

@ngbrown
Copy link
Contributor

ngbrown commented Sep 9, 2022

responseHeaders is a parameter of handleRequest() in your entry.server.ts file. cryptoGetRandomString() is an exercise left to the reader as it differs per platform :). The 33 is just the number of bytes of randomness I wanted.

@EvgeniyBudaev
Copy link

@ngbrown I get a warning too in console Warning: Prop nonce did not match. Server: "" Client: "irn2qSvqJ-0xG9HuIos8DrJjQBtqYMk8L3PVz4qyG74muQ" I really need help on how to fix this

@ngbrown
Copy link
Contributor

ngbrown commented Sep 10, 2022

This is probably the warning from React that I was refering to:

react-dom generates some error messages because it doesn't handle rehydration checking correctly on nonce since it can't be read as an attribute.

I used patch-package to fix this, or you can just ignore it if your CSP is working correctly.

@EvgeniyBudaev
Copy link

EvgeniyBudaev commented Sep 10, 2022

To fix "Warning: Prop nonce did not match. Server: "" Client: "irn2qSvqJ-0xG9HuIos8DrJjQBtqYMk8L3PVz4qyG74muQ" ", you need to add:

	if(typeof document !== "undefined") {
		cspScriptNonce = "";
	}

Full code:

export default function App() {
	const { cspScriptNonce } = useLoaderData<RootLoaderData>();
	if(typeof document !== "undefined") {
		cspScriptNonce = "";
	}
	return (
	  <html lang="en">
		<head>
		  <Meta />
		  <Links />
		</head>
		<body>
		  <Outlet />
		  <ScrollRestoration nonce={cspScriptNonce} />
		  <Scripts nonce={cspScriptNonce} />
		  <LiveReload nonce={cspScriptNonce} port={8002} />
		</body>
	  </html>
	);
  }

@fnimick
Copy link

fnimick commented Sep 21, 2022

@EvgeniyBudaev thank you! This works great!

For anyone unclear on why this works - the nonce attribute is stripped from the script elements as soon as the page loads. Then when client hydration happens, the nonce values are re-applied, and are then updated again every time the loader is re-fetched. This shouldn't be happening anyway, and it would be great if there was added documentation in the Remix docs to ensure the nonces are only provided on the initial server-side load.

ref: https://html.spec.whatwg.org/multipage/urls-and-fetching.html#nonce-attributes : "Elements that have a nonce content attribute ensure that the cryptographic nonce is only exposed to script (and not to side-channels like CSS attribute selectors) by taking the value from the content attribute, moving it into an internal slot named [[CryptographicNonce]], exposing it to script via the HTMLOrSVGElement interface mixin, and setting the content attribute to the empty string. Unless otherwise specified, the slot's value is the empty string."

@58bits
Copy link

58bits commented Jan 2, 2023

Was also looking at this today. Not sure if this helps - but Kent Dodd's site @kentcdodds - is also using nonce values, generated in his express server, and available via loader context..... https://github.com/kentcdodds/kentcdodds.com/blob/main/app/root.tsx

@ShawLocke
Copy link

@ngbrown thanks for your comment. i am inspired by it and setting CSP for shopify app with remix successfully which is critical for shopify app review and routine evaluation.

@brophdawg11
Copy link
Contributor

I think this is no longer an issue with the nonce support? As shown on Kent's site - the nonce can be included on the <script> tags rendered via <Scripts>, <LiveReload>, <ScrollRestoration> etc., and then included in the CSP accordingly.

If this is still an issue, please feel free to re-open with a small reproduction.

@kentcdodds
Copy link
Member

This is correct. It has been fixed 👍

@brophdawg11 brophdawg11 removed their assignment Aug 9, 2023
@samcolby
Copy link

samcolby commented Sep 4, 2023

Edit: See ahuth's solution below, which is much better than mine 🤦 .

For anyone as confused by this thread as I was... At the time of writing this isn't currently working on Kent's site .

And the NonceContext approach doesn't add the nonce to the html, which I assume is why it is not working on Kent's site.

Using the other comments in this thread, you can get it work using remix 1.19.3 with all V2 flags enabled using

In root.tsx

export const loader: LoaderFunction = async () => {
  return {cspScriptNonce: crypto.randomBytes(16).toString('hex')}
}

// don't include any inline scripts with whatever you use here as the nonce wont work
export const ErrorBoundary = () => <div /> 

const App = () => {
    const data = useLoaderData<LoaderData>() 
    const cspScriptNonce = typeof document === 'undefined' ? data.cspScriptNonce : ''
    ...etc...
}

Then in handleRequest or handleBrowserRequest etc..

const cspScriptNonce = remixContext.staticHandlerContext.loaderData.root?.cspScriptNonce
// this needs to handle cspScriptNonce=undefined for ErrorBoundaries
applySecurityHeaders(responseHeaders, cspScriptNonce). ```

Love to know if there is a better way of doing this without using the loader though.
Or ideally, all inline scripts can be removed so we don't have to worry about this at all 🤞 .

@ahuth
Copy link
Contributor

ahuth commented Sep 4, 2023

Hmm, seems to be working for me with v1.19.3

@samcolby
Copy link

samcolby commented Sep 5, 2023

Aha! Thank you @ahuth that's much easier to follow!

@Haraldson
Copy link

Haraldson commented Oct 9, 2023

Edit: Turning this into a mini-tutorial as I figured out my derp: I mounted the <NonceProvider> with nonce={nonce} instead of value={nonce}.

In addition, the way Remix adds dynamic scripts requires 'script-src-elem' to be defined in addition to 'script-src'.


@samcolby @ahuth How are you piecing this together? Adding the nonce itself doesn’t seem to be enough. Is there a guide somewhere?

I’ve added …

server.mjs

import express from 'express'
import helmet from 'helmet'

const server = express()

server.use((request, response, next) => {
    response.locals.cspNonce = crypto.randomBytes(32).toString('base64')
    next()
})

// ...

server.use(helmet({
    contentSecurityPolicy: {
        directives: {
            'script-src': [
                "'strict-dynamic'",
                (request, response) => `'nonce-${response.locals.cspNonce}'`
            ],
            'script-src-elem': [
                "'strict-dynamic'",
                (request, response) => `'nonce-${response.locals.cspNonce}'`
            ]
        }
    }
}))

// ...

server.all('*', createRequestHandler({
    build,

    getLoadContext: (request, response) => ({
        cspNonce: response.locals.cspNonce
    })
}))

providers/nonce.js

import { createContext, useContext } from 'react'
const NonceContext = createContext('')
export const useNonce = () => useContext(NonceContext)
export default NonceContext.Provider

entry.server.js (both bot and browser handlers)

import NonceProvider from '@providers/nonce'

// ...

return new Promise((resolve, reject) => {
    let shellRendered = false

    const nonce = loadContext?.cspNonce

    const {
        pipe,
        abort
    } = renderToPipeableStream(
        <NonceProvider value={nonce}>
            <RemixServer
                context={remixContext}
                url={request.url}
                abortDelay={ABORT_DELAY} />
        </NonceProvider>,
        {
            nonce, // <-- Unsure about this one
            
            // ...

root.js

import { useNonce } from '@providers/nonce'

// ...

const App = () => {
    const nonce = useNonce()

    return (
        <html>
            <head>
                <Meta />
                <Links />
            </head>
            <body>
                <Outlet />
                <Scripts nonce={nonce} />
                <ScrollRestoration nonce={nonce} />
                <LiveReload nonce={nonce} />
            </body>
        </html>
    )
}

… and still CSP is whining in my console.

Did I forget anything obvious?

@cskeppstedt
Copy link

cskeppstedt commented Oct 30, 2023

getLoadContext

I'm trying to implement something similar, but I'm stuck on getting the object with {cspNonce: '...'} from getLoadContext() passed to handleRequest() in entry.server.tsx – the last argument (loadContext) is always undefined.

I can see the console.log working in my express server:

app.all(
  '/admin/*',
  createRequestHandler({
    build,
    getLoadContext: (request, response) => {
      console.log('getLoadContext: hello', response.locals); // <-- this works: getLoadContext: hello {cspNonce: 'foobar123....'}
      return { cspNonce: response.locals.cspNonce };
    },
  }),
);

but the loadContext argument is always undefined:

export default function handleRequest(
  request: Request,
  responseStatusCode: number,
  responseHeaders: Headers,
  remixContext: EntryContext,
  loadContext?: AppLoadContext,
) {
  console.log(loadContext) // <-- always undefined
  return ...
}

I also tried to look for the cspNonce value inside remixConfig but could not find it anywhere (some code example suggested remixContext.staticHandlerContext.loaderData.root.cspNonce, but I also console logged all arguments and it was nowhere to be found).

I'm on remix v1.19.3 (latest 1.x release).

@Haraldson
Copy link

@cskeppstedt I’m pretty sure the fifth argument wasn‘t added until pretty recently, and only in 2.x.

@outofthisworld
Copy link

outofthisworld commented Dec 17, 2023

How come <Links/> does not support nonce?

@brophdawg11
Copy link
Contributor

Would you mind starting a new issue for Links nonce support @outofthisworld? From a quick glance it does seem that we should support a nonce to be applied to link requests: https://html.spec.whatwg.org/#fetching-and-processing-a-resource-from-a-link-element

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Closed
Development

No branches or pull requests