fix(i18n): prevent overwriting 404.astro (#10281)

* fix(i18n): prevent overwriting 404.astro

* add changeset

* add tests

* adjust unit test

.changeset/curvy-trees-compare.md

@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Fixes an issue where `404.astro` was ignored with `i18n` routing enabled.

packages/astro/src/core/app/index.ts

@@ -320,6 +320,7 @@ export class App {
 			});
 		}
 
+		// We remove internally-used header before we send the response to the user agent.
 		if (response.headers.has(REROUTE_DIRECTIVE_HEADER)) {
 			response.headers.delete(REROUTE_DIRECTIVE_HEADER);
 		}

packages/astro/src/core/render-context.ts

@@ -13,6 +13,7 @@ import { renderEndpoint } from '../runtime/server/endpoint.js';
 import { renderPage } from '../runtime/server/index.js';
 import {
 	ASTRO_VERSION,
+	REROUTE_DIRECTIVE_HEADER,
 	ROUTE_TYPE_HEADER,
 	clientAddressSymbol,
 	clientLocalsSymbol,
@@ -102,7 +103,12 @@ export class RenderContext {
 									streaming,
 									routeData
 								);
+								// Signal to the i18n middleware to maybe act on this response
 								response.headers.set(ROUTE_TYPE_HEADER, 'page');
+								// Signal to the error-page-rerouting infra to let this response pass through to avoid loops
+								if (routeData.route === "/404" || routeData.route === "/500") {
+									response.headers.set(REROUTE_DIRECTIVE_HEADER, "no")
+								}
 								return response;
 							}
 						: type === 'fallback'

packages/astro/src/i18n/middleware.ts

@@ -2,7 +2,7 @@ import { appendForwardSlash, joinPaths } from '@astrojs/internal-helpers/path';
 import type { APIContext, Locales, MiddlewareHandler, SSRManifest } from '../@types/astro.js';
 import type { SSRManifestI18n } from '../core/app/types.js';
 import { shouldAppendForwardSlash } from '../core/build/util.js';
-import { ROUTE_TYPE_HEADER } from '../core/constants.js';
+import { REROUTE_DIRECTIVE_HEADER, ROUTE_TYPE_HEADER } from '../core/constants.js';
 import { getPathByLocale, normalizeTheLocale } from './index.js';
 
 // Checks if the pathname has any locale, exception for the defaultLocale, which is ignored on purpose.
@@ -44,12 +44,9 @@ export function createI18nMiddleware(
 			}
 		}
 
-		// Astro can't know where the default locale is supposed to be, so it returns a 404 with no content.
+		// Astro can't know where the default locale is supposed to be, so it returns a 404.
 		else if (!pathnameHasLocale(url.pathname, i18n.locales)) {
-			return new Response(null, {
-				status: 404,
-				headers: response.headers,
-			});
+			return notFound(response);
 		}
 
 		return undefined;
@@ -66,10 +63,7 @@ export function createI18nMiddleware(
 		if (pathnameContainsDefaultLocale) {
 			const newLocation = url.pathname.replace(`/${i18n.defaultLocale}`, '');
 			response.headers.set('Location', newLocation);
-			return new Response(null, {
-				status: 404,
-				headers: response.headers,
-			});
+			return notFound(response);
 		}
 
 		return undefined;
@@ -88,10 +82,7 @@ export function createI18nMiddleware(
 		// - the URL doesn't contain a locale
 		const isRoot = url.pathname === base + '/' || url.pathname === base;
 		if (!(isRoot || pathnameHasLocale(url.pathname, i18n.locales))) {
-			return new Response(null, {
-				status: 404,
-				headers: response.headers,
-			});
+			return notFound(response);
 		}
 
 		return undefined;
@@ -201,6 +192,19 @@ export function createI18nMiddleware(
 	};
 }
 
+/**
+ * The i18n returns empty 404 responses in certain cases.
+ * Error-page-rerouting infra will attempt to render the 404.astro page, causing the middleware to run a second time.
+ * To avoid loops and overwriting the contents of `404.astro`, we allow error pages to pass through.
+ */
+function notFound(response: Response) {
+	if (response.headers.get(REROUTE_DIRECTIVE_HEADER) === "no") return response;
+	return new Response(null, {
+		status: 404,
+		headers: response.headers,
+	});
+}
+
 /**
  * Checks if the current locale doesn't belong to a configured domain
  * @param i18n

packages/astro/src/vite-plugin-astro-server/route.ts

@@ -274,7 +274,7 @@ export async function handleRoute({
 		response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no'
 	) {
 		const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline);
-		if (options && fourOhFourRoute?.route !== options.route)
+		if (options)
 			return handleRoute({
 				...options,
 				matchedRoute: fourOhFourRoute,
@@ -288,6 +288,12 @@ export async function handleRoute({
 				incomingResponse,
 			});
 	}
+
+	// We remove the internally-used header before we send the response to the user agent.
+	if (response.headers.has(REROUTE_DIRECTIVE_HEADER)) {
+		response.headers.delete(REROUTE_DIRECTIVE_HEADER);
+	}
+
 	if (route.type === 'endpoint') {
 		await writeWebResponse(incomingResponse, response);
 		return;

packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/404.astro

@@ -0,0 +1,4 @@
+<html>
+    <head><title>Not Found</title></head>
+    <body>Can't find the page youre looking for.</body>
+</html>

packages/astro/test/i18n-routing.test.js

@@ -353,6 +353,13 @@ describe('[DEV] i18n routing', () => {
 			assert.equal(response.status, 200);
 			assert.equal((await response.text()).includes('I am index'), true);
 		});
+
+		it('can render the 404.astro route on unmatched requests', async () => {
+			const response = await fixture.fetch('/xyz');
+			assert.equal(response.status, 404);
+			const text = await response.text();
+			assert.equal(text.includes("Can't find the page youre looking for."), true);
+		});
 	});
 
 	describe('i18n routing with routing strategy [pathname-prefix-always]', () => {
@@ -1338,6 +1345,15 @@ describe('[SSR] i18n routing', () => {
 			assert.equal(response.status, 200);
 			assert.equal((await response.text()).includes('I am index'), true);
 		});
+
+		
+		it('can render the 404.astro route on unmatched requests', async () => {
+			const request = new Request('http://example.com/xyz');
+			const response = await app.render(request);
+			assert.equal(response.status, 404);
+			const text = await response.text();
+			assert.equal(text.includes("Can't find the page youre looking for."), true);
+		});
 	});
 
 	describe('i18n routing with routing strategy [pathname-prefix-always]', () => {

packages/astro/test/units/routing/endpoints.test.js

@@ -62,11 +62,10 @@ describe('endpoints', () => {
 		await done;
 		const headers = res.getHeaders();
 		assert.equal(headers['location'], 'https://example.com/destination');
-		assert.equal(headers['x-astro-reroute'], undefined);
 		assert.equal(res.statusCode, 307);
 	});
 
-	it('should append reroute header for HTTP status 404', async () => {
+	it('should remove internally-used for HTTP status 404', async () => {
 		const { req, res, done } = createRequestAndResponse({
 			method: 'GET',
 			url: '/not-found',
@@ -74,11 +73,11 @@ describe('endpoints', () => {
 		container.handle(req, res);
 		await done;
 		const headers = res.getHeaders();
-		assert.equal(headers['x-astro-reroute'], 'no');
+		assert.equal(headers['x-astro-reroute'], undefined);
 		assert.equal(res.statusCode, 404);
 	});
 
-	it('should append reroute header for HTTP status 500', async () => {
+	it('should remove internally-used header for HTTP status 500', async () => {
 		const { req, res, done } = createRequestAndResponse({
 			method: 'GET',
 			url: '/internal-error',
@@ -86,7 +85,7 @@ describe('endpoints', () => {
 		container.handle(req, res);
 		await done;
 		const headers = res.getHeaders();
-		assert.equal(headers['x-astro-reroute'], 'no');
+		assert.equal(headers['x-astro-reroute'], undefined);
 		assert.equal(res.statusCode, 500);
 	});
 });