apollographql / apollo-client-nextjs

Apollo Client support for the Next.js App Router
https://www.npmjs.com/package/@apollo/experimental-nextjs-app-support
MIT License
358 stars 25 forks source link

feat: add support for passing a nonce to the rehydration script #160

Closed josh-feldman closed 4 months ago

josh-feldman commented 4 months ago

Rehydration currently does not work in an app with a CSP that disables unsafe-inline scripts.

With this change, you can now follow the NextJS guide on CSP, and in layout.tsx or page.tsx pass the nonce from headers down into the ApolloWrapper to prevent unsafe-inline CSP errors.

apollo-cla commented 4 months ago

@josh-feldman: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

phryneas commented 4 months ago

@josh-feldman thanks a ton for the PR!

I've tweaked this a bit to now accept a extraScriptProps property. I've also added a bunch of tests.

Generally, this is probably not optimal because it means passing the nonce to a client component prop - and that means that the nonce will be passed into the client, for a potential attacker to read it.

At the same time, it's probably also the best we can get at this point, since Next.js has not native way of having SSR-specific secrets.

Could you take another look if these changes still work for your use case?

josh-feldman commented 4 months ago

Generally, this is probably not optimal because it means passing the nonce to a client component prop - and that means that the nonce will be passed into the client, for a potential attacker to read it.

Thank you for the more generic solution @phryneas, this should work for us.

As far as I'm aware, the serialized props that get passed to a client component come along in the GET request for the Document itself embedded in the self.__next_f.push scripts, so it is exposed to the client at the same time the html of the document response itself comes down. At that point, the nonce would already present on the client from the server-side rendered html.

Let me know if there is a security concern I'm missing though, I'd like to better understand.

phryneas commented 4 months ago

@josh-feldman The difference is that JavaScript (not even the DevTools) can actually read the nonce attribute of a script tag - while it can be read out of the RSC payload.

Try

self.__next_f.map(x=>x[1]).filter(x => x?.includes('nonce')).map(x=>/nonce=(.*?),/.exec(x)[1])

that will extract the nonce props for you in a very unsophisticated & crude way.

That said, this is still a lot more safe than without the nonce - but it probably wouldn't help against a very targeted attack. We have a similar problem with token values if they are passed from a server component to a client component.

There is a potential mitigation against this to be found in this comment, but it's not nice either and a lot of manual work - as far as I know, nobody has made a library out of this yet.

josh-feldman commented 4 months ago

@phryneas thank you for the explanation, that makes total sense and I'd agree with your conclusion. I just wanted to make sure we weren't introducing security theater with this PR or encouraging an anti pattern. I'm happy with this PR, let me know if you need anything else from me to get it merged 👍.

As you've stated, the nonce is now exposed on the client side JavaScript and perhaps a very targeted attack could find a way around other protections. That being said, a CSP nonce is just a mechanism to prevent arbitrary JavaScript execution on any given page load. I'd imagine in many cases if an attacker has the means to execute code to retrieve the nonce from the RSC payload, then they've already bypassed most protection the nonce would provide.

I can see how providing access tokens or auth cookies to the client props (as discussed in your linked comment) could raise a larger concern due to the doors it may open for an attacker. This PR should help mitigate those types of attacks by allowing users of Apollo to set stricter script-src CSP.

Let me know if there's any gap in my understanding above, and thank you again for the updates to the PR!

phryneas commented 4 months ago

I agree that this is a low-risk problem in this specific case, but as this came up with a second use case now, I've gone and created a library for this: https://www.npmjs.com/package/ssr-only-secrets

I've also added it to the integration tests here, so these should serve as an example how to use it for now.

I'll probably get to publishing this PR tomorrow :)

phryneas commented 4 months ago

Released in https://github.com/apollographql/apollo-client-nextjs/releases/tag/v.0.6.0