fix: merge headers from the original response in error pages (#9433)

* fix: merge headers from the original response in error pages

* revert local change

* change test ordering

* apply feedback

.changeset/thirty-hats-bathe.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Correctly merge headers from the original response when an error page is rendered

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),
+			]),
 		});
 	}
 

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';

packages/astro/test/fixtures/middleware space/src/middleware.js

@@ -7,6 +7,12 @@ const first = defineMiddleware(async (context, next) => {
 		return new Response('<span>New content!!</span>', {
 			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');

packages/astro/test/middleware.test.js

@@ -269,6 +269,16 @@ describe('Middleware API in PROD mode, SSR', () => {
 		expect(text).to.include('<h1>There was an error rendering the page.</h1>');
 	});
 
+	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/',