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

Missing nonce attribute in preinitialized scripts causing CSP violations in app router #54055

Closed
1 task done
rinvii opened this issue Aug 15, 2023 · 6 comments · Fixed by #54059
Closed
1 task done
Labels
bug Issue was opened via the bug report template. locked Runtime Related to Node.js or Edge Runtime with Next.js.

Comments

@rinvii
Copy link

rinvii commented Aug 15, 2023

Verify canary release

  • I verified that the issue exists in the latest Next.js canary release

Provide environment information

Operating System:
      Platform: darwin
      Arch: x64
      Version: Darwin Kernel Version 19.6.0: Tue Jun 21 21:18:39 PDT 2022; root:xnu-6153.141.66~1/RELEASE_X86_64
    Binaries:
      Node: 20.3.0
      npm: 9.6.7
      Yarn: 3.6.0
      pnpm: 8.6.10
    Relevant Packages:
      next: 13.4.16-canary.1
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0
      typescript: 5.1.3
    Next.js Config:
      output: N/A

Which area(s) of Next.js are affected? (leave empty if unsure)

App Router, Middleware / Edge (API routes, runtime)

Link to the code that reproduces this issue or a replay of the bug

https://github.com/rinvii/missing-nonce

To Reproduce

Set up a basic Next.js 13.4 App Router project.

  1. Create a middleware.ts file in the root dir
import { NextResponse } from "next/server";
import type { NextRequest } from "next/server";

// adding CSP to middleware example from https://nextjs.org/docs/app/building-your-application/routing/middleware#example

// This function can be marked `async` if using `await` inside
export function middleware(request: NextRequest) {
  const requestHeaders = new Headers(request.headers);
  let cspValue =
    "default-src 'self'; " +
    "frame-ancestors 'none'; " +
    "form-action 'self'; " +
    "base-uri 'self'; " +
    "object-src 'none'; " +
    "style-src 'self' 'unsafe-inline'; " +
    `script-src 'self' 'unsafe-inline' https: http:${
      process.env.NODE_ENV === "development" ? " 'unsafe-eval'" : ""
    }`;
  const nonce = crypto.randomUUID();

  cspValue += ` 'nonce-${nonce}' 'strict-dynamic'`;

  requestHeaders.set("content-security-policy", cspValue);

  const response = NextResponse.next({
    request: {
      headers: requestHeaders,
    },
  });

  response.headers.set("content-security-policy", cspValue);

  return response;
}

// See "Matching Paths" below to learn more
export const config = {
  matcher: "/:path*",
};
  1. npm run dev
  2. Observe CSP violations in the browser console log

Describe the Bug

When implementing a strict Content Security Policy via middleware as described in #43743 (comment), some script tags are missing a nonce value inside the head element.

I believe this bug was introduced in the PR #53705 where all but one of the required scripts for every page load are now currently preinitialized.

This seemed to have been resolved by the issues #43743 and #7486, but somehow reintroduced?

I noticed this when I was upgrading from 13.4.13 to 13.4.16. I managed to pinpoint this to 13.4.14-canary.2.

This happens both in dev and prod.

Expected Behavior

Inline scripts injected into the page should have a nonce attribute when using nonce for the CSP script-src directive.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

@rinvii rinvii added the bug Issue was opened via the bug report template. label Aug 15, 2023
@github-actions github-actions bot added the Runtime Related to Node.js or Edge Runtime with Next.js. label Aug 15, 2023
@karlhorky
Copy link
Contributor

karlhorky commented Aug 17, 2023

cc @gnoff @danieltott @timneutkens seems like CSP nonces are currently not working with Next.js in versions newer than [email protected] - can confirm in our deployed application too

@danieltott

This comment was marked as spam.

@danieltott
Copy link
Contributor

@rinvii can confirm this as well.

Although for the example app, I would add export const dynamic = "force-dynamic"; to the page.tsx file. Without that, it will create a static page and middleware won't run at all.

@skullface
Copy link

Related: #53928

@kodiakhq kodiakhq bot closed this as completed in #54059 Aug 20, 2023
kodiakhq bot pushed a commit that referenced this issue Aug 20, 2023
Fixes #54055.

A bug recently introduced in #53705 made it so that we were now preinitializing some of our scripts slightly better, but in doing so, we failed to pass in a nonce. This broke nonce-based CSP usage. The fix was to add the `nonce` to our `ReactDOM.preinit` calls.

Manual testing shows that this change fixes the error and the nonce is now passed in as expected.


Co-authored-by: Dan Ott <[email protected]>
@karlhorky
Copy link
Contributor

karlhorky commented Aug 23, 2023

Thanks for the #54059 PR, review, merge and followup tests @nbhargava @danieltott @shuding @gnoff 🙌

This was released in [email protected] and I can confirm that this fixes CSP nonces with the App Router and Middleware (I removed our patch and our Playwright tests are no longer failing).

@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2023

This closed issue has been automatically locked because it had no new activity for 2 weeks. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@github-actions github-actions bot added the locked label Sep 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template. locked Runtime Related to Node.js or Edge Runtime with Next.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants