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

docs: Add docs on CSP and nonce generation #54601

Merged
merged 5 commits into from
Sep 1, 2023
Merged

docs: Add docs on CSP and nonce generation #54601

merged 5 commits into from
Sep 1, 2023

Conversation

leerob
Copy link
Member

@leerob leerob commented Aug 26, 2023

There's been some confusion on the correct way to add a nonce, so took the opportunity here to:

  • Add a new docs page for Content Security Policy
  • Explained how to generate a nonce with Middleware
  • Showed how to consume the nonce in a route with headers
  • Updated the with-strict-csp example
  • Update the nonce error message page
  • Backlinked to the new page in a few places in the docs

@ijjk ijjk added area: documentation examples Issue/PR related to examples created-by: Next.js Docs team PRs by the Docs team. labels Aug 26, 2023
Copy link

@orca-security-us orca-security-us bot left a comment

Choose a reason for hiding this comment

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

Orca Security Scan Summary

Status Check Issues by priority
Passed Passed Secrets high 0   medium 0   low 0   info 0 View in Orca

@karlhorky
Copy link
Contributor

cc @danieltott in case you have any feedback on this one

@leerob
Copy link
Member Author

leerob commented Aug 27, 2023

Thanks for all the feedback, addressed 🙏

@leerob
Copy link
Member Author

leerob commented Aug 29, 2023

Updated to mention this is for the next patch release, so should be okay to merge now.

Copy link
Contributor

@danieltott danieltott left a comment

Choose a reason for hiding this comment

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

This is going to be great to have.

Two big issues I see:

1: The approach is different for the Pages router vs the App router

The App router uses a different rendering engine than the Pages router.

Therefor it doesn't pick up nonce values from headers like the App router does - you need to add them manually in a custom _document.tsx file.

With the same middleware, the _document.tsx file would look like this:

// pages/_document.tsx

import Document, {
  Html,
  Head,
  Main,
  NextScript,
  DocumentContext,
  DocumentInitialProps,
} from 'next/document';

export default function PagesDocument(
  props: DocumentInitialProps & { nonce: string | undefined }
) {
  const { nonce } = props;

  return (
    <Html lang="en">
      <Head nonce={nonce} />
      <body data-nonce={nonce}>
        <Main />
        <NextScript nonce={nonce} />
      </body>
    </Html>
  );
}

PagesDocument.getInitialProps = async (
  ctx: DocumentContext
): Promise<DocumentInitialProps & { nonce: string | undefined }> => {
  // read nonce value from headers
  const nonce = ctx.req?.headers?.['x-nonce'] as string | undefined;

  const initialProps = await Document.getInitialProps(ctx);

  return {
    ...initialProps,
    nonce,
  };
};

If you need to reference nonce values in pages, they can be read using getServerSideProps:

// pages/some-page.tsx

import type { InferGetServerSidePropsType, GetServerSideProps } from 'next';

export default function PagesRouter({
  nonce,
}: InferGetServerSidePropsType<typeof getServerSideProps>) {
  return (
    <div>
      Hello! My <code>nonce</code> is <code>{nonce}</code>{' '}
    </div>
  );
}

export const getServerSideProps: GetServerSideProps<{
  nonce: string | undefined;
}> = async ({ req }) => {
  const nonce = req?.headers?.['x-nonce'] as string | undefined;
  return { props: { nonce } };
};

2: nonce-based CSPs only work on dynamic pages.

This needs to be at the top because it is confusing for many people. If you add that middleware file on the default nextjs build and run it, it won't work, because next optimizes for static output when it can.

On static pages, middleware doesn't even run.

So all pages need to be dynamic. Again, the approach here is different.

For the App router, easiest path is adding the dynamic route config:

export const dynamic = 'force-dynamic'

Other options can be found on the rendering docs.

For the Pages router, the only option is adding a getServerSideProps to each page, or adding getInitialProps to the _app.tsx file

@danieltott
Copy link
Contributor

You can see an example of the pages router in action here: https://nextjs-csp-report-only.vercel.app/pages-router, and source here: https://github.com/Sprokets/nextjs-csp-report-only/blob/main/pages/_document.tsx

@leerob
Copy link
Member Author

leerob commented Aug 30, 2023

We could add more guidance on pages/ in the follow up 👍 I'd like to get this through.

@danieltott
Copy link
Contributor

danieltott commented Aug 30, 2023

@leerob I would suggest at least the updates about dynamic pages. Otherwise it will just be more non-stop "csp not working" issues and confusion, since the solution outlined above will not work on a default build of next.

Edit - My apologies, I see the note "This means that you must use dynamic rendering to add nonces." I guess I'd say maybe that could be more prominent or maybe reiterated a couple times, but at least it's noted there.

@leerob
Copy link
Member Author

leerob commented Aug 30, 2023

I believe it's currently clear that nonces require dynamic pages, per the definition of doing a nonce. We must generate a unique value, so you need your application to be dynamic. If you're looking for this solution, a static site isn't the right fit.

@danieltott
Copy link
Contributor

I agree to the definition, but if you take a look at all the issues and conversations around this in the next github, you might see that it is not clear to all people, at least not immediately.

I'm not sure if the confusion is lack of understanding of how nonces are supposed to work, lack of thought about static vs dynamic pages, or a misunderstanding of middleware (one user was trying to use middleware on a statically exported site, for instance).

This is just my two cents - I've had my head in this for a while now and end up getting mentioned on a lot of these sorts of issues. Obviously y'all have a lot on your hands, and this doc is going to be a huge improvement regardless 🙌🏼

@leerob
Copy link
Member Author

leerob commented Aug 30, 2023

but if you take a look at all the issues and conversations around this in the next github, you might see that it is not clear to all people, at least not immediately.

This is because there's currently no docs, and people don't have the full picture. This will change when this is merged and I reply back on this issues.

I'm not sure if the confusion is lack of understanding of how nonces are supposed to work, lack of thought about static vs dynamic pages, or a misunderstanding of middleware (one user was trying to use middleware on a statically exported site, for instance).

This is documented here: https://nextjs.org/docs/app/building-your-application/deploying/static-exports#unsupported-features

This is just my two cents - I've had my head in this for a while now and end up getting mentioned on a lot of these sorts of issues. Obviously y'all have a lot on your hands, and this doc is going to be a huge improvement regardless 🙌🏼

I appreciate your feedback and help here!

* - favicon.ico (favicon file)
*/
{
source: '/((?!api|_next/static|_next/image|favicon.ico).*)',
Copy link

@rafael-fecha rafael-fecha Sep 6, 2023

Choose a reason for hiding this comment

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

is this also working for the root page in case the route is just '/' ? I think this will also match as except ones

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Next.js Docs team PRs by the Docs team. examples Issue/PR related to examples locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants