-
Notifications
You must be signed in to change notification settings - Fork 88
fix: cookies set in middleware accessible during the same request #2847
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
Changes from 5 commits
c8379d6
f85b1e1
ce05e85
bc25218
a8e1f63
a140dc8
5d7b2d9
4a900a7
cc5b3b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
import { assertEquals } from 'https://deno.land/std@0.175.0/testing/asserts.ts' | ||
import { mergeMiddlewareCookies } from './middleware.ts' | ||
|
||
const MIDDLEWARE_HEADER = 'x-middleware-set-cookie' | ||
|
||
Deno.test('mergeMiddlewareCookies', async (t) => { | ||
await t.step('should handle empty cookies', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, '') | ||
}) | ||
|
||
await t.step('should return request cookies when there are no middleware headers', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
request.headers.set('Cookie', 'oatmeal=raisin') | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'oatmeal=raisin') | ||
}) | ||
|
||
await t.step('should not require cookies in request to be set', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
response.headers.set(MIDDLEWARE_HEADER, 'peanut=butter; Path=/') | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'peanut=butter') | ||
}) | ||
|
||
await t.step('should merge request and middleware cookies', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
request.headers.set('Cookie', 'oatmeal=raisin') | ||
response.headers.set(MIDDLEWARE_HEADER, 'peanut=butter; Path=/') | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'oatmeal=raisin; peanut=butter') | ||
}) | ||
|
||
await t.step('should overwrite request cookies with latest values', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
request.headers.set('Cookie', 'oatmeal=chocolate') | ||
response.headers.set(MIDDLEWARE_HEADER, 'oatmeal=raisin; Path=/') | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'oatmeal=raisin') | ||
}) | ||
|
||
await t.step('should not decode middleware cookie values', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
response.headers.set(MIDDLEWARE_HEADER, 'greeting=Hello%20from%20the%20cookie; Path=/') | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'greeting=Hello%20from%20the%20cookie') | ||
}) | ||
|
||
await t.step('should support multiple cookies being set in middleware', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
response.headers.set( | ||
MIDDLEWARE_HEADER, | ||
'oatmeal=raisin; Path=/,peanut=butter; Path=/,chocolate=chip; Path=/', | ||
) | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'oatmeal=raisin; peanut=butter; chocolate=chip') | ||
}) | ||
|
||
await t.step('should ignore comma in middleware cookie expiry', async () => { | ||
const request = new Request('https://www.test-url.com') | ||
const response = new Response() | ||
|
||
response.headers.set( | ||
MIDDLEWARE_HEADER, | ||
'oatmeal=raisin; Path=/; Expires=Wed, 23 Apr 2025 13:37:43 GMT; Max-Age=604800', | ||
) | ||
|
||
const result = mergeMiddlewareCookies(response, request) | ||
assertEquals(result, 'oatmeal=raisin') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,12 @@ import { | |
|
||
import { updateModifiedHeaders } from './headers.ts' | ||
import type { StructuredLogger } from './logging.ts' | ||
import { addMiddlewareHeaders, isMiddlewareRequest, isMiddlewareResponse } from './middleware.ts' | ||
import { | ||
addMiddlewareHeaders, | ||
isMiddlewareRequest, | ||
isMiddlewareResponse, | ||
mergeMiddlewareCookies, | ||
} from './middleware.ts' | ||
import { RequestData } from './next-request.ts' | ||
import { | ||
addBasePath, | ||
|
@@ -116,12 +121,13 @@ export const buildResponse = async ({ | |
} | ||
return rewriter.transform(response.originResponse) | ||
} | ||
const res = new Response(result.response.body, result.response) | ||
|
||
const edgeResponse = new Response(result.response.body, result.response) | ||
request.headers.set('x-nf-next-middleware', 'skip') | ||
|
||
let rewrite = res.headers.get('x-middleware-rewrite') | ||
let redirect = res.headers.get('location') | ||
let nextRedirect = res.headers.get('x-nextjs-redirect') | ||
let rewrite = edgeResponse.headers.get('x-middleware-rewrite') | ||
let redirect = edgeResponse.headers.get('location') | ||
let nextRedirect = edgeResponse.headers.get('x-nextjs-redirect') | ||
|
||
// Data requests (i.e. requests for /_next/data ) need special handling | ||
const isDataReq = request.headers.has('x-nextjs-data') | ||
|
@@ -152,7 +158,7 @@ export const buildResponse = async ({ | |
// Data requests might be rewritten to an external URL | ||
// This header tells the client router the redirect target, and if it's external then it will do a full navigation | ||
|
||
res.headers.set('x-nextjs-rewrite', relativeUrl) | ||
edgeResponse.headers.set('x-nextjs-rewrite', relativeUrl) | ||
} | ||
|
||
if (rewriteUrl.origin !== baseUrl.origin) { | ||
|
@@ -178,7 +184,7 @@ export const buildResponse = async ({ | |
}) | ||
} | ||
|
||
return addMiddlewareHeaders(fetch(proxyRequest, { redirect: 'manual' }), res) | ||
return addMiddlewareHeaders(fetch(proxyRequest, { redirect: 'manual' }), edgeResponse) | ||
} | ||
|
||
if (isDataReq) { | ||
|
@@ -197,9 +203,9 @@ export const buildResponse = async ({ | |
logger.withFields({ rewrite_url: rewrite }).debug('Rewrite url is same as original url') | ||
return | ||
} | ||
res.headers.set('x-middleware-rewrite', relativeUrl) | ||
edgeResponse.headers.set('x-middleware-rewrite', relativeUrl) | ||
request.headers.set('x-middleware-rewrite', target) | ||
return addMiddlewareHeaders(context.rewrite(target), res) | ||
return addMiddlewareHeaders(context.rewrite(target), edgeResponse) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure, but possibly here we would also need to handle middleware cookies as well? next.js test fixture doesn't seem to have case for setting cookies for rewrites, so maybe we should not touch it in this PR - just flagging it (so this comment act as potential paper trail in case there was some future report about cookies not being handled for rewrites) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recreated the bug quite easily with the following middleware, so decided to fix in this PR as well
|
||
} | ||
|
||
if (redirect) { | ||
|
@@ -208,27 +214,35 @@ export const buildResponse = async ({ | |
logger.withFields({ redirect_url: redirect }).debug('Redirect url is same as original url') | ||
return | ||
} | ||
res.headers.set('location', redirect) | ||
edgeResponse.headers.set('location', redirect) | ||
} | ||
|
||
// Data requests shouldn't automatically redirect in the browser (they might be HTML pages): they're handled by the router | ||
if (redirect && isDataReq) { | ||
res.headers.delete('location') | ||
res.headers.set('x-nextjs-redirect', relativizeURL(redirect, request.url)) | ||
edgeResponse.headers.delete('location') | ||
edgeResponse.headers.set('x-nextjs-redirect', relativizeURL(redirect, request.url)) | ||
} | ||
|
||
nextRedirect = res.headers.get('x-nextjs-redirect') | ||
nextRedirect = edgeResponse.headers.get('x-nextjs-redirect') | ||
|
||
if (nextRedirect && isDataReq) { | ||
res.headers.set('x-nextjs-redirect', normalizeDataUrl(nextRedirect)) | ||
edgeResponse.headers.set('x-nextjs-redirect', normalizeDataUrl(nextRedirect)) | ||
} | ||
|
||
if (res.headers.get('x-middleware-next') === '1') { | ||
res.headers.delete('x-middleware-next') | ||
return addMiddlewareHeaders(context.next(), res) | ||
if (edgeResponse.headers.get('x-middleware-next') === '1') { | ||
edgeResponse.headers.delete('x-middleware-next') | ||
|
||
// coookies set in middleware need to be available during the lambda request | ||
const newRequest = new Request(request) | ||
const newRequestCookies = mergeMiddlewareCookies(edgeResponse, newRequest) | ||
if (newRequestCookies) { | ||
newRequest.headers.set('Cookie', newRequestCookies) | ||
} | ||
|
||
return addMiddlewareHeaders(context.next(newRequest), edgeResponse) | ||
} | ||
|
||
return res | ||
return edgeResponse | ||
} | ||
|
||
/** | ||
|
Uh oh!
There was an error while loading. Please reload this page.