diff --git a/.changeset/soft-things-deny.md b/.changeset/soft-things-deny.md new file mode 100644 index 000000000000..f9b1b397eaa4 --- /dev/null +++ b/.changeset/soft-things-deny.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a bug where the custom 404 (or 500) page was not rendered when a middleware rewrite targeted a route that returned an empty 404/500 response, and a blank page was returned instead diff --git a/packages/astro/src/core/errors/default-handler.ts b/packages/astro/src/core/errors/default-handler.ts index 84f0be42fb8b..6159c8939165 100644 --- a/packages/astro/src/core/errors/default-handler.ts +++ b/packages/astro/src/core/errors/default-handler.ts @@ -11,6 +11,7 @@ import { provideSession } from '../session/handler.js'; import { validateHost } from '../app/validate-headers.js'; import { getErrorRoutePath } from '../../i18n/error-routes.js'; import { getOutputFilename } from '../output-filename.js'; +import { REROUTABLE_STATUS_CODES } from '../constants.js'; import type { ErrorHandler } from './handler.js'; type ErrorPagePath = @@ -127,6 +128,29 @@ export class DefaultErrorHandler implements ErrorHandler { errorState, this.#pagesHandler.handle.bind(this.#pagesHandler), ); + // A middleware rewrite (`ctx.rewrite()` / `next(payload)`) issued while + // rendering the error page swaps the state's routeData away from the + // error route, so the rewrite target renders instead of 404/500.astro. + // If that hijacked render produced another empty reroutable error + // response, we'd return a blank page — retry rendering the error page + // without middleware instead (same fallback used when middleware throws). + // A rewrite that produced a real body is left untouched, so middleware + // that intentionally rewrites error renders keeps working. + if ( + skipMiddleware === false && + errorState.routeData !== errorRouteData && + response.body === null && + REROUTABLE_STATUS_CODES.includes(response.status) + ) { + return this.renderError(request, { + ...resolvedRenderOptions, + status, + error, + response: originalResponse, + skipMiddleware: true, + pathname: resolvedPathname, + }); + } const newResponse = mergeResponses(response, originalResponse); prepareResponse(newResponse, resolvedRenderOptions); return newResponse; diff --git a/packages/astro/src/core/errors/dev-handler.ts b/packages/astro/src/core/errors/dev-handler.ts index a7f97270fcdb..39827a0a1b73 100644 --- a/packages/astro/src/core/errors/dev-handler.ts +++ b/packages/astro/src/core/errors/dev-handler.ts @@ -5,6 +5,7 @@ import type { RouteData } from '../../types/public/index.js'; import { AstroMiddleware } from '../middleware/astro-middleware.js'; import { PagesHandler } from '../pages/handler.js'; import { getCustom404Route, getCustom500Route } from '../routing/helpers.js'; +import { REROUTABLE_STATUS_CODES } from '../constants.js'; import { type AstroError, isAstroError } from './index.js'; import { MiddlewareNoDataOrNextCalled, MiddlewareNotAResponse } from './errors-data.js'; import type { ErrorHandler } from './handler.js'; @@ -77,6 +78,25 @@ export class DevErrorHandler implements ErrorHandler { this.#pagesHandler.handle.bind(this.#pagesHandler), ); + // A middleware rewrite issued while rendering the error page swaps the + // state's routeData away from the error route. If that hijacked render + // produced another empty reroutable error response, retry rendering the + // error page without middleware (same fallback used when middleware throws). + if ( + skipMiddleware === false && + errorState.routeData !== routeData && + response.body === null && + REROUTABLE_STATUS_CODES.includes(response.status) + ) { + return this.renderError(request, { + ...resolvedRenderOptions, + status, + error, + skipMiddleware: true, + pathname: resolvedPathname, + }); + } + if (error) { // Log useful information that the custom 500 page may not display unlike the default error overlay app.logger.error('router', (error as AstroError).stack || (error as AstroError).message); diff --git a/packages/astro/test/units/routing/rewrite-app.test.ts b/packages/astro/test/units/routing/rewrite-app.test.ts index d9d283ca5699..6fd911238f71 100644 --- a/packages/astro/test/units/routing/rewrite-app.test.ts +++ b/packages/astro/test/units/routing/rewrite-app.test.ts @@ -628,6 +628,71 @@ describe('Rewrites via App - body-used rewrite returns 500', () => { }); }); +describe('Rewrites via App - middleware rewrite to a route returning an empty 404 (#17281)', () => { + const custom404 = createComponent(() => render`

Custom 404

`); + const empty404Page = createComponent(() => new Response(null, { status: 404 })); + + const middleware = async (context: APIContext, next: MiddlewareNext) => { + if (context.url.pathname === '/rewrite') { + return context.rewrite('/unexisting-page'); + } + return next(); + }; + + const app = createTestApp( + [ + createPage(custom404, { route: '/404', component: '404.astro' }), + createPage(empty404Page, { + route: '/[...slug]', + segments: [[spreadPart('...slug')]], + pathname: undefined, + }), + ], + { middleware: () => ({ onRequest: middleware }) }, + ); + + it('renders the custom 404 page when middleware rewrites to a route that returns an empty 404', async () => { + const res = await app.render(new Request('http://example.com/rewrite')); + assert.equal(res.status, 404); + const $ = cheerio.load(await res.text()); + assert.equal($('h1').text(), 'Custom 404'); + }); + + it('renders the custom 404 page when the 404-returning route is hit directly', async () => { + const res = await app.render(new Request('http://example.com/unexisting-page')); + assert.equal(res.status, 404); + const $ = cheerio.load(await res.text()); + assert.equal($('h1').text(), 'Custom 404'); + }); +}); + +describe('Rewrites via App - middleware rewrites to a real page after next() returns 404', () => { + const empty404Page = createComponent(() => new Response(null, { status: 404 })); + + const middleware = async (context: APIContext, next: MiddlewareNext) => { + const response = await next(); + if (response.status === 404 && context.url.pathname === '/soft-404') { + return context.rewrite('/'); + } + return response; + }; + + const app = createTestApp( + [ + createPage(indexPage, { route: '/', isIndex: true }), + createPage(empty404Page, { route: '/soft-404' }), + ], + { middleware: () => ({ onRequest: middleware }) }, + ); + + it('renders the rewrite target when middleware rewrites on a 404 response', async () => { + const res = await app.render(new Request('http://example.com/soft-404')); + assert.equal(res.status, 200); + const $ = cheerio.load(await res.text()); + assert.equal($('h1').text(), 'Index'); + }); +}); + describe('Rewrites via App - routePattern updated after sequence rewrite', () => { const patternPage = createComponent((result: any, props: any, slots: any) => { const Astro = result.createAstro(props, slots);