diff --git a/.changeset/harden-merge-responses-cookies.md b/.changeset/harden-merge-responses-cookies.md new file mode 100644 index 000000000000..4b9bd521ff92 --- /dev/null +++ b/.changeset/harden-merge-responses-cookies.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes cookie handling during error page rendering to ensure cookies set by middleware are consistently included in the response diff --git a/packages/astro/src/core/app/base.ts b/packages/astro/src/core/app/base.ts index 6ec73e37363b..346468d0073f 100644 --- a/packages/astro/src/core/app/base.ts +++ b/packages/astro/src/core/app/base.ts @@ -23,7 +23,12 @@ import { REWRITE_DIRECTIVE_HEADER_KEY, ROUTE_TYPE_HEADER, } from '../constants.js'; -import { getSetCookiesFromResponse } from '../cookies/index.js'; +import { + AstroCookies, + attachCookiesToResponse, + getSetCookiesFromResponse, +} from '../cookies/index.js'; +import { getCookiesFromResponse } from '../cookies/response.js'; import { AstroError, AstroErrorData } from '../errors/index.js'; import { consoleLogDestination } from '../logger/console.js'; import { AstroIntegrationLogger, Logger } from '../logger/core.js'; @@ -695,16 +700,24 @@ export abstract class BaseApp

{ // this function could throw an error... originalResponse.headers.delete('Content-type'); } catch {} - // we use a map to remove duplicates - const mergedHeaders = new Map([ - ...Array.from(newResponseHeaders), - ...Array.from(originalResponse.headers), - ]); + // Build merged headers using append() to preserve multi-value headers (e.g. Set-Cookie). + // Headers from the original response take priority over new response headers for + // single-value headers, but we use append to avoid collapsing multi-value entries. const newHeaders = new Headers(); - for (const [name, value] of mergedHeaders) { - newHeaders.set(name, value); + const seen = new Set(); + // Add original response headers first (they take priority) + for (const [name, value] of originalResponse.headers) { + newHeaders.append(name, value); + seen.add(name.toLowerCase()); + } + // Add new response headers that weren't already set by the original response, + // but skip content-type since the error page must return text/html + for (const [name, value] of newResponseHeaders) { + if (!seen.has(name.toLowerCase())) { + newHeaders.append(name, value); + } } - return new Response(newResponse.body, { + const mergedResponse = new Response(newResponse.body, { status, statusText: status === 200 ? newResponse.statusText : originalResponse.statusText, // If you're looking at here for possible bugs, it means that it's not a bug. @@ -714,6 +727,24 @@ export abstract class BaseApp

{ // Although, we don't want it to replace the content-type, because the error page must return `text/html` headers: newHeaders, }); + + // Transfer AstroCookies from the original or new response so that + // #prepareResponse can read them when addCookieHeader is true. + const originalCookies = getCookiesFromResponse(originalResponse); + const newCookies = getCookiesFromResponse(newResponse); + if (originalCookies) { + // If both responses have cookies, merge new response cookies into original + if (newCookies) { + for (const cookieValue of AstroCookies.consume(newCookies)) { + originalResponse.headers.append('set-cookie', cookieValue); + } + } + attachCookiesToResponse(mergedResponse, originalCookies); + } else if (newCookies) { + attachCookiesToResponse(mergedResponse, newCookies); + } + + return mergedResponse; } getDefaultStatusCode(routeData: RouteData, pathname: string): number { diff --git a/packages/astro/test/units/middleware/middleware-app.test.js b/packages/astro/test/units/middleware/middleware-app.test.js index 80013bd41892..284edbc88281 100644 --- a/packages/astro/test/units/middleware/middleware-app.test.js +++ b/packages/astro/test/units/middleware/middleware-app.test.js @@ -585,6 +585,182 @@ describe('Middleware via App.render()', () => { }); }); + describe('cookies on error pages', () => { + it('should preserve cookies set by middleware when returning Response(null, { status: 404 })', async () => { + // Middleware sets a cookie and returns 404 with null body (common auth guard pattern) + const onRequest = async (ctx, next) => { + ctx.cookies.set('session', 'abc123', { path: '/' }); + if (ctx.url.pathname.startsWith('/api/guarded')) { + return new Response(null, { status: 404 }); + } + return next(); + }; + + const guardedRouteData = createRouteData({ + route: '/api/guarded/[...path]', + pathname: undefined, + segments: undefined, + }); + // Override for spread route + guardedRouteData.params = ['...path']; + guardedRouteData.pattern = /^\/api\/guarded(?:\/(.*))?$/; + guardedRouteData.pathname = undefined; + guardedRouteData.segments = [ + [{ content: 'api', dynamic: false, spread: false }], + [{ content: 'guarded', dynamic: false, spread: false }], + [{ content: '...path', dynamic: true, spread: true }], + ]; + + const pageMap = new Map([ + [ + guardedRouteData.component, + async () => ({ + page: async () => ({ + default: simplePage(), + }), + }), + ], + [ + notFoundRouteData.component, + async () => ({ page: async () => ({ default: notFoundPage }) }), + ], + ]); + const app = createAppWithMiddleware({ + onRequest, + routes: [{ routeData: guardedRouteData }, { routeData: notFoundRouteData }], + pageMap, + }); + + const response = await app.render(new Request('http://localhost/api/guarded/secret'), { + addCookieHeader: true, + }); + + assert.equal(response.status, 404); + const setCookie = response.headers.get('set-cookie'); + assert.ok(setCookie, 'Expected Set-Cookie header to be present on 404 error page response'); + assert.match(setCookie, /session=abc123/); + }); + + it('should preserve cookies set by middleware when returning Response(null, { status: 500 })', async () => { + const onRequest = async (ctx, next) => { + ctx.cookies.set('csrf', 'token456', { path: '/' }); + if (ctx.url.pathname.startsWith('/api/error')) { + return new Response(null, { status: 500 }); + } + return next(); + }; + + const errorRouteData = createRouteData({ + route: '/api/error/[...path]', + pathname: undefined, + segments: undefined, + }); + errorRouteData.params = ['...path']; + errorRouteData.pattern = /^\/api\/error(?:\/(.*))?$/; + errorRouteData.pathname = undefined; + errorRouteData.segments = [ + [{ content: 'api', dynamic: false, spread: false }], + [{ content: 'error', dynamic: false, spread: false }], + [{ content: '...path', dynamic: true, spread: true }], + ]; + + const pageMap = new Map([ + [ + errorRouteData.component, + async () => ({ + page: async () => ({ + default: simplePage(), + }), + }), + ], + [ + serverErrorRouteData.component, + async () => ({ page: async () => ({ default: serverErrorPage }) }), + ], + ]); + const app = createAppWithMiddleware({ + onRequest, + routes: [{ routeData: errorRouteData }, { routeData: serverErrorRouteData }], + pageMap, + }); + + const response = await app.render(new Request('http://localhost/api/error/test'), { + addCookieHeader: true, + }); + + assert.equal(response.status, 500); + const setCookie = response.headers.get('set-cookie'); + assert.ok(setCookie, 'Expected Set-Cookie header to be present on 500 error page response'); + assert.match(setCookie, /csrf=token456/); + }); + + it('should preserve multiple cookies from sequenced middleware during error page rerouting', async () => { + const onRequest = async (ctx, next) => { + ctx.cookies.set('session', 'abc123', { path: '/' }); + ctx.cookies.set('csrf', 'token456', { path: '/' }); + if (ctx.url.pathname.startsWith('/api/guarded')) { + ctx.cookies.set('auth_attempt', 'failed', { path: '/' }); + return new Response(null, { status: 404 }); + } + return next(); + }; + + const guardedRouteData = createRouteData({ + route: '/api/guarded/[...path]', + pathname: undefined, + segments: undefined, + }); + guardedRouteData.params = ['...path']; + guardedRouteData.pattern = /^\/api\/guarded(?:\/(.*))?$/; + guardedRouteData.pathname = undefined; + guardedRouteData.segments = [ + [{ content: 'api', dynamic: false, spread: false }], + [{ content: 'guarded', dynamic: false, spread: false }], + [{ content: '...path', dynamic: true, spread: true }], + ]; + + const pageMap = new Map([ + [ + guardedRouteData.component, + async () => ({ + page: async () => ({ + default: simplePage(), + }), + }), + ], + [ + notFoundRouteData.component, + async () => ({ page: async () => ({ default: notFoundPage }) }), + ], + ]); + const app = createAppWithMiddleware({ + onRequest, + routes: [{ routeData: guardedRouteData }, { routeData: notFoundRouteData }], + pageMap, + }); + + const response = await app.render(new Request('http://localhost/api/guarded/secret'), { + addCookieHeader: true, + }); + + assert.equal(response.status, 404); + const setCookies = response.headers.getSetCookie(); + const cookieValues = setCookies.join(', '); + assert.ok( + cookieValues.includes('session=abc123'), + 'Expected session cookie in Set-Cookie headers', + ); + assert.ok( + cookieValues.includes('csrf=token456'), + 'Expected csrf cookie in Set-Cookie headers', + ); + assert.ok( + cookieValues.includes('auth_attempt=failed'), + 'Expected auth_attempt cookie in Set-Cookie headers', + ); + }); + }); + describe('middleware with custom headers', () => { it('should correctly set custom headers in middleware', async () => { const onRequest = async (_ctx, next) => {