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

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

Merged
merged 5 commits into from
Jan 5, 2024

Conversation

josh-feldman
Copy link
Contributor

@josh-feldman josh-feldman commented Dec 27, 2023

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.

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](https://nextjs.org/docs/app/building-your-application/configuring/content-security-policy) on CSP,
and in `layout.tsx` pass the `nonce` from headers down into the `ApolloWrapper` to prevent `unsafe-inline` CSP errors.
@josh-feldman josh-feldman requested a review from a team as a code owner December 27, 2023 23:19
@apollo-cla
Copy link

@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
Copy link
Member

phryneas commented Jan 3, 2024

@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
Copy link
Contributor Author

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
Copy link
Member

phryneas commented Jan 4, 2024

@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
Copy link
Contributor Author

@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
Copy link
Member

phryneas commented Jan 4, 2024

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 phryneas merged commit e466b37 into apollographql:main Jan 5, 2024
7 checks passed
@phryneas
Copy link
Member

phryneas commented Jan 5, 2024

@josh-feldman josh-feldman deleted the feat/support-csp-nonce branch March 7, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants