-
Notifications
You must be signed in to change notification settings - Fork 87
fix: ensure internal x-middleware-set-cookie
header is not passed on to lambda
#2891
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
base: main
Are you sure you want to change the base?
Conversation
…etting cookies in middleware
x-middleware-set-cookie
header is not passed on to lambda
📊 Package size report 0.01%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
}, | ||
{ | ||
"file": "test/e2e/app-dir/app-middleware/app-middleware.test.ts", | ||
"reason": "Relies on access to environment variables set on the edge", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -67,6 +67,10 @@ export function mergeMiddlewareCookies(middlewareResponse: Response, lambdaReque | |||
const middlewareCookies = middlewareResponse.headers.get('x-middleware-set-cookie') | |||
|
|||
if (middlewareCookies) { | |||
// Next expects internal headers to be omitted when cookies are set by the middleware | |||
// See: https://github.com/vercel/next.js/blob/005db43079c7b59fd8c2594e8362761dc4cb3211/test/e2e/app-dir/app-middleware/app-middleware.test.ts#L197-L207 | |||
middlewareResponse.headers.delete('x-middleware-set-cookie') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does final response reaching client (browser/curl etc) would have actual set-cookie
with this removal? from what I can see mergeMiddlewareCookies
only sets cookie
header:
opennextjs-netlify/edge-runtime/lib/response.ts
Lines 245 to 248 in 7795157
const newRequestCookies = mergeMiddlewareCookies(edgeResponse, newRequest) | |
if (newRequestCookies) { | |
newRequest.headers.set('Cookie', newRequestCookies) | |
} |
so I wonder if this change as-is wouldn't result in clients no longer actually receiving set-cookie
headers?
Description
Middleware cookies depend on the
x-middleware-set-cookie header
to work, however Next does not expect this header to be passed on after the middleware. This change ensures the value is not set when sending the request to the lambda.Tests
Focus is on
should omit internal headers for middleware cookies
testRelevant links (GitHub issues, etc.) or a picture of cute animal
Fixes FRB-1774