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

Response headers don't stick when set across multiple chained middlewares #128

Open
derekaug opened this issue Dec 14, 2024 · 8 comments
Open
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@derekaug
Copy link

Describe the bug
When setting response headers in one middleware and forwarding the response, the response headers are not being applied further down the chain. The only headers "that stick" are headers set within a middleware that you return a NextResponse on.

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://stackblitz.com/edit/stackblitz-starters-q52fskvz?file=middleware.ts
  2. Wait for dependencies to install and dev server start.
  3. Wait for the page to load
  4. See logs in console that show the logs from each middleware after setting a header in each but not showing the headers from the previously executed middleware
headers set applied in first {/*any}} [ 'x-first: first test', 'x-middleware-next: 1' ]
headers set applied in second {/*any}} [ 'x-middleware-next: 1', 'x-second: second test' ]
headers set applied in after {/*any}} [ 'x-after: after test', 'x-middleware-next: 1' ]

Expected behavior
I expect each middleware to get the same response and have each of their headers applied to it, so a log output like:

headers set applied in first {/*any}} [ 'x-first: first test', 'x-middleware-next: 1' ]
headers set applied in second {/*any}} [ 'x-first: first test', 'x-middleware-next: 1', 'x-second: second test' ]
headers set applied in after {/*any}} [ 'x-after: after test', 'x-first: first test', 'x-middleware-next: 1', 'x-second: second test']

Screenshots
If applicable, add screenshots to help explain your problem.

Screenshot 2024-12-14 at 5 39 40 AM

Additional context
Add any other context about the problem here.

This was working in versions 1.2.* at least in the case that I grabbed the response from the arguments passed in and returned NextResponse.next(). Middlewares further down the chain would have x-some-header set and applied in their responses (this was in a before middleware).

https://stackblitz.com/edit/stackblitz-starters-z96vrexv?file=middleware.ts

headers set applied in first {/*any}} [ 'x-first: first test', 'x-middleware-next: 1' ]
headers set applied in second {/*any}} [
  'x-first: first test',
  'x-middleware-next: 1',
  'x-second: second test'
]
headers set applied in after {/*any}} [
  'x-after: after test',
  'x-first: first test',
  'x-middleware-next: 1',
  'x-second: second test'
]
export async function headersMiddleware({
  request,
  response,
}: MiddlewareFunctionProps) {
  // set any response headers
  response.headers.set(
    'x-some-header',
    'header'
  )

  return NextResponse.next()
}

Screenshot 2024-12-14 at 5 43 52 AM

Copy link

linear bot commented Dec 14, 2024

@z4nr34l
Copy link
Owner

z4nr34l commented Dec 16, 2024

I've made that intentionally. There were 2 major issues with prev solution:

  1. Security - those headers were leaking security or internal headers from libs/hostings.
  2. React Actions - headers passage were breaking react actions - specifically data or component returned by them in prod envs.

If you need to pass data across chain you should use context then.

Current solution is way more stable and secure.

I'm closing as it's not really an issue - but feel free to respond or reopen!

All best!

@z4nr34l z4nr34l closed this as completed Dec 16, 2024
@atothewest
Copy link

Out of curiosity, what were the bugs you were encountering with Actions that made this fix necessary?

Currently, there doesn't seem to be any way to cleanly have multiple middlewares all contribute to the response headers. The code looks like it grabs any headers which were added to the response each middleware and collects them in a variable called finalHeaders. Which is fine, but this variable is then used to populate the request headers of the ultimate response:

return NextResponse.next({
request: {
headers: finalHeaders,
},
});
};

Setting response headers, for example as required when implementing a CSP via middleware per Next's docs, must be performed in the final middleware in the chain or else they are lost.

Furthermore, in order to set response headers at all, the middleware must return a response, it cannot be forwarded. Returning a response from any middleware halts execution of any subsequent middlewares, so the end result seems to be that the only real solution would be to have a dedicated middleware for composing a final response, which must be the final middleware to execute, and pulls together different headers using context.

That's not necessarily a problem, but I don't think much of the above is made particularly clear in the docs currently. I'm happy to help out on either updating the docs or trying to implement a solution that allows response header composition without triggering either of the two problems mentioned here.

Thanks for your work on this package, which helps solve a glaring omission from the core Next middleware implementation.

@derekaug
Copy link
Author

I have two issues with this decision that led me to essentially making my own solution for this.

  1. This was a breaking change in your library's API and it wasn't really documented anywhere and came through on a minor patch bump.

  2. If I wanted to set a response header in one middleware and then have it apply generally, I'd have to make some drastic changes to either funnel all my middleware responses through one "final" middleware OR have every middleware in my app that returns a response run a function that checks for some context and adds them to the response before returning. Either work I guess but feel rather clunky, imo.

Anyway, agreed with @atothewest thanks for this lib. It was nice while it worked for us and at least inspired us to rethink how to do middleware in Next.js v13+ because the out of the box is definitely lacking.

@z4nr34l
Copy link
Owner

z4nr34l commented Feb 12, 2025

I'm preparing major update which will improve headers handling and include revert to 6.1 version of path-to-regexp dep for more usability and to match next.js native dep version.

Thanks for feedback!

@z4nr34l z4nr34l reopened this Feb 13, 2025
@z4nr34l
Copy link
Owner

z4nr34l commented Feb 13, 2025

Would be solved in v2 (#140)

@z4nr34l
Copy link
Owner

z4nr34l commented Feb 14, 2025

Hello @derekaug, @atothewest - could I ask you to check the current WIP v2 version in #140?

bun add https://pkg.pr.new/z4nr34l/nemo/@rescale/nemo@140

Feedback on that stage would be helpful and will allow me to prepare better docs or event change direction of that solution.

Thanks in advance.

@z4nr34l z4nr34l self-assigned this Feb 14, 2025
@z4nr34l z4nr34l added bug Something isn't working enhancement New feature or request labels Feb 14, 2025
@atothewest
Copy link

Thanks for the update @z4nr34l I will check it out as soon as I can this week 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants