Skip to content

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

Merged
merged 9 commits into from
Apr 18, 2025
4 changes: 3 additions & 1 deletion .github/workflows/deno-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ jobs:
- name: Setup Deno
uses: denoland/setup-deno@v1
with:
deno-version: vx.x.x
deno-version: v1.x.x
- name: Vendor Deno modules
run: deno vendor edge-runtime/vendor.ts --output=edge-runtime/vendor --force
- name: Test
run: deno test -A edge-runtime/
92 changes: 92 additions & 0 deletions edge-runtime/lib/middleware.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { assertEquals } from 'https://deno.land/[email protected]/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')
})
})
21 changes: 21 additions & 0 deletions edge-runtime/lib/middleware.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { Context } from '@netlify/edge-functions'

import type { ElementHandlers } from '../vendor/deno.land/x/[email protected]/src/index.ts'
import { getCookies } from '../vendor/deno.land/[email protected]/http/cookie.ts'

type NextDataTransform = <T>(data: T) => T

Expand Down Expand Up @@ -58,3 +59,23 @@ export const addMiddlewareHeaders = async (
})
return response
}

// This serves the same purpose as the mergeMiddlewareCookies in Next.js but has been customized to our domain
// See: https://github.com/vercel/next.js/blob/6e4495f8430eab33b12cd11dffdd8e27eee6e0cf/packages/next/src/server/async-storage/request-store.ts#L78-L105
export function mergeMiddlewareCookies(middlewareResponse: Response, lambdaRequest: Request) {
let mergedCookies = getCookies(lambdaRequest.headers)
const middlewareCookies = middlewareResponse.headers.get('x-middleware-set-cookie')
const regex = new RegExp(/,(?!\s)/) // commas that are not followed by whitespace

if (middlewareCookies) {
middlewareCookies.split(regex).forEach((entry) => {
const [cookie] = entry.split(';')
const [name, value] = cookie.split('=')
mergedCookies[name] = value
})
}

return Object.entries(mergedCookies)
.map((kv) => kv.join('='))
.join('; ')
}
50 changes: 32 additions & 18 deletions edge-runtime/lib/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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) {
Expand All @@ -178,7 +184,7 @@ export const buildResponse = async ({
})
}

return addMiddlewareHeaders(fetch(proxyRequest, { redirect: 'manual' }), res)
return addMiddlewareHeaders(fetch(proxyRequest, { redirect: 'manual' }), edgeResponse)
}

if (isDataReq) {
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

@mrstork mrstork Apr 17, 2025

Choose a reason for hiding this comment

The 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

const nextRes = NextResponse.rewrite(url)
nextRes.cookies.set("nextjs-cookie", "rewrite");
return nextRes;

}

if (redirect) {
Expand All @@ -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
}

/**
Expand Down
Loading