diff --git a/.changeset/thirty-hats-bathe.md b/.changeset/thirty-hats-bathe.md new file mode 100644 index 000000000000..1ec2b932af38 --- /dev/null +++ b/.changeset/thirty-hats-bathe.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Correctly merge headers from the original response when an error page is rendered diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 6abd2f734d23..b1689bac6aa3 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -386,8 +386,12 @@ export class App { return response; } - #mergeResponses(newResponse: Response, oldResponse?: Response, override?: { status: 404 | 500 }) { - if (!oldResponse) { + #mergeResponses( + newResponse: Response, + originalResponse?: Response, + override?: { status: 404 | 500 } + ) { + if (!originalResponse) { if (override !== undefined) { return new Response(newResponse.body, { status: override.status, @@ -398,21 +402,31 @@ export class App { return newResponse; } - const { statusText, headers } = oldResponse; - // If the new response did not have a meaningful status, an override may have been provided // If the original status was 200 (default), override it with the new status (probably 404 or 500) // Otherwise, the user set a specific status while rendering and we should respect that one const status = override?.status ? override.status - : oldResponse.status === 200 + : originalResponse.status === 200 ? newResponse.status - : oldResponse.status; + : originalResponse.status; + try { + // this function could throw an error... + originalResponse.headers.delete('Content-type'); + } catch {} return new Response(newResponse.body, { status, - statusText: status === 200 ? newResponse.statusText : statusText, - headers: new Headers(Array.from(headers)), + statusText: status === 200 ? newResponse.statusText : originalResponse.statusText, + // If you're looking at here for possible bugs, it means that it's not a bug. + // With the middleware, users can meddle with headers, and we should pass to the 404/500. + // If users see something weird, it's because they are setting some headers they should not. + // + // Although, we don't want it to replace the content-type, because the error page must return `text/html` + headers: new Headers([ + ...Array.from(newResponse.headers), + ...Array.from(originalResponse.headers), + ]), }); } diff --git a/packages/astro/src/vite-plugin-astro-server/request.ts b/packages/astro/src/vite-plugin-astro-server/request.ts index f0c6b3de0c50..5dd507c75b8d 100644 --- a/packages/astro/src/vite-plugin-astro-server/request.ts +++ b/packages/astro/src/vite-plugin-astro-server/request.ts @@ -1,10 +1,6 @@ import type http from 'node:http'; import type { ManifestData, SSRManifest } from '../@types/astro.js'; -import { collectErrorMetadata } from '../core/errors/dev/index.js'; -import { createSafeError } from '../core/errors/index.js'; -import { formatErrorMessage } from '../core/messages.js'; import { collapseDuplicateSlashes, removeTrailingForwardSlash } from '../core/path.js'; -import { eventError, telemetry } from '../events/index.js'; import { isServerLikeOutput } from '../prerender/utils.js'; import type { DevServerController } from './controller.js'; import { runWithErrorHandling } from './controller.js'; diff --git a/packages/astro/test/fixtures/middleware space/src/middleware.js b/packages/astro/test/fixtures/middleware space/src/middleware.js index 779bc5c93930..1404169ef96c 100644 --- a/packages/astro/test/fixtures/middleware space/src/middleware.js +++ b/packages/astro/test/fixtures/middleware space/src/middleware.js @@ -7,6 +7,12 @@ const first = defineMiddleware(async (context, next) => { return new Response('New content!!', { status: 200, }); + } else if (context.request.url.includes('/content-policy')) { + const response = await next(); + response.headers.append('X-Content-Type-Options', 'nosniff'); + response.headers.append('Content-Type', 'application/json'); + + return next(); } else if (context.request.url.includes('/broken-500')) { return new Response(null, { status: 500, @@ -19,21 +25,21 @@ const first = defineMiddleware(async (context, next) => { headers: response.headers, }); } else if (context.url.pathname === '/throw') { - throw new Error; + throw new Error(); } else if (context.url.pathname === '/clone') { const response = await next(); const newResponse = response.clone(); const /** @type {string} */ html = await newResponse.text(); const newhtml = html.replace('testing', 'it works'); return new Response(newhtml, { status: 200, headers: response.headers }); - } else if(context.url.pathname === '/return-response-cookies') { + } else if (context.url.pathname === '/return-response-cookies') { const response = await next(); - const html = await response.text(); + const html = await response.text(); - return new Response(html, { - status: 200, - headers: response.headers - }); + return new Response(html, { + status: 200, + headers: response.headers, + }); } else { if (context.url.pathname === '/') { context.cookies.set('foo', 'bar'); diff --git a/packages/astro/test/middleware.test.js b/packages/astro/test/middleware.test.js index c858f37a826e..64dc29cd9150 100644 --- a/packages/astro/test/middleware.test.js +++ b/packages/astro/test/middleware.test.js @@ -269,6 +269,16 @@ describe('Middleware API in PROD mode, SSR', () => { expect(text).to.include('

There was an error rendering the page.

'); }); + it('should correctly render the page even when custom headers are set in a middleware', async () => { + const request = new Request('http://example.com/content-policy'); + const routeData = app.match(request); + + const response = await app.render(request, { routeData }); + expect(response).to.deep.include({ status: 404 }); + expect(response.headers.get('content-type')).equal('text/html'); + }); + + // keep this last it('the integration should receive the path to the middleware', async () => { fixture = await loadFixture({ root: './fixtures/middleware space/',