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

Remove redundant header setting of "Content-Security-Policy". #75307

Closed
wants to merge 1 commit into from

Conversation

ar-work-acc
Copy link

Remove redundant header setting of "Content-Security-Policy".

Remove redundant header setting of "Content-Security-Policy".
@ijjk ijjk added the Documentation Related to Next.js' official documentation. label Jan 25, 2025
@ijjk
Copy link
Member

ijjk commented Jan 25, 2025

Allow CI Workflow Run

  • approve CI run for commit: a2fb095

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@darthmaim
Copy link
Contributor

Good catch, but this still looks wrong, because you should only set x-nonce on the request, not on the response. So instead of removing response.headers.set, the { headers: requestHeaders } should be removed from NextResponse.next (maybe someone even sets other headers on the request before the CSP part in their middleware that should be internal only and not be exposed on the response).

@ar-work-acc
Copy link
Author

Okay, thanks for your reply. I just wanted to point out the weird stuff in the documentation. You can fix this on your own, as I'll close the pull request (since more might need to be added to make the example more complete).

@darthmaim
Copy link
Contributor

I took another look, and the code is actually correct right now.

  1. This creates a new Headers instance and sets the x-nonce and Content-Security-Policy:
    const requestHeaders = new Headers(request.headers)
    requestHeaders.set('x-nonce', nonce)
    requestHeaders.set(
      'Content-Security-Policy',
      contentSecurityPolicyHeaderValue
    )
  2. This then tells next to handle the request using the created headers (notice the request: , the response will not have those headers):
    const response = NextResponse.next({
      request: {
        headers: requestHeaders,
      },
    })
  3. And finally the Content-Security-Policy header is also added to the response:
    response.headers.set(
      'Content-Security-Policy',
      contentSecurityPolicyHeaderValue
    )

So nothing has to be changed 👍

@ar-work-acc
Copy link
Author

ar-work-acc commented Jan 26, 2025

Okay, after tracing the code:

  static next(init?: MiddlewareResponseInit) {
    const headers = new Headers(init?.headers)
    headers.set('x-middleware-next', '1')

    handleMiddlewareField(init, headers)
    return new NextResponse(null, { ...init, headers })
  }
interface MiddlewareResponseInit extends globalThis.ResponseInit {
 /**
  * These fields will override the request from clients.
  */
 request?: ModifiedRequest
}
interface ModifiedRequest {
  /**
   * If this is set, the request headers will be overridden with this value.
   */
  headers?: Headers
}

It seems that it will indeed only modify request headers.

But the description in the documentation is somewhat misleading:

The next() method is useful for Middleware, as it allows you to return early and continue routing.


import { NextResponse } from 'next/server'
 
return NextResponse.next()
You can also forward headers when producing the response:


import { NextResponse } from 'next/server'
 
// Given an incoming request...
const newHeaders = new Headers(request.headers)
// Add a new header
newHeaders.set('x-version', '123')
// And produce a response with the new headers
return NextResponse.next({
  request: {
    // New request headers
    headers: newHeaders,
  },
})

// And produce a response with the new headers

This is the line that got me.

Just clarifying things. Thanks for your help and time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Related to Next.js' official documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants