From 7e9c4a134c6ea7c8b92ea00038c0845b58c02bc5 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Wed, 17 Jul 2024 17:01:07 +0100 Subject: [PATCH] fix: avoid response rewrite inside the dev server (#11477) * fix: avoid response rewrite inside the dev server * breakdown logic of reroute and rewrite --- .changeset/chilled-impalas-dance.md | 5 ++++ packages/astro/src/core/constants.ts | 8 ++++++ packages/astro/src/core/render-context.ts | 15 ++++++---- .../src/vite-plugin-astro-server/route.ts | 18 +++++++++++- packages/astro/test/rewrite.test.js | 28 +++++++++++++++++++ 5 files changed, 68 insertions(+), 6 deletions(-) create mode 100644 .changeset/chilled-impalas-dance.md diff --git a/.changeset/chilled-impalas-dance.md b/.changeset/chilled-impalas-dance.md new file mode 100644 index 000000000000..51d6708011fd --- /dev/null +++ b/.changeset/chilled-impalas-dance.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes an issue where the development server was emitting a 404 status code when the user uses a rewrite that emits a 200 status code. diff --git a/packages/astro/src/core/constants.ts b/packages/astro/src/core/constants.ts index 0930ea6f0f7d..8e9f5ac74e0a 100644 --- a/packages/astro/src/core/constants.ts +++ b/packages/astro/src/core/constants.ts @@ -22,6 +22,14 @@ export const ASTRO_VERSION = process.env.PACKAGE_VERSION ?? 'development'; */ export const REROUTE_DIRECTIVE_HEADER = 'X-Astro-Reroute'; +/** + * Header and value that are attached to a Response object when a **user rewrite** occurs. + * + * This metadata is used to determine the origin of a Response. If a rewrite has occurred, it should be prioritised over other logic. + */ +export const REWRITE_DIRECTIVE_HEADER_KEY = 'X-Astro-Rewrite'; +export const REWRITE_DIRECTIVE_HEADER_VALUE = 'yes'; + /** * The name for the header used to help i18n middleware, which only needs to act on "page" and "fallback" route types. */ diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index 910ee679e274..722ce82c6220 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -25,6 +25,8 @@ import { clientAddressSymbol, clientLocalsSymbol, responseSentSymbol, + REWRITE_DIRECTIVE_HEADER_KEY, + REWRITE_DIRECTIVE_HEADER_VALUE, } from './constants.js'; import { AstroCookies, attachCookiesToResponse } from './cookies/index.js'; import { getCookiesFromResponse } from './cookies/response.js'; @@ -184,13 +186,12 @@ export class RenderContext { // 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 ( - this.routeData.route === '/404' || - this.routeData.route === '/500' || - this.isRewriting - ) { + if (this.routeData.route === '/404' || this.routeData.route === '/500') { response.headers.set(REROUTE_DIRECTIVE_HEADER, 'no'); } + if (this.isRewriting) { + response.headers.set(REWRITE_DIRECTIVE_HEADER_KEY, REWRITE_DIRECTIVE_HEADER_VALUE); + } break; } case 'fallback': { @@ -381,6 +382,7 @@ export class RenderContext { } #astroPagePartial?: Omit; + /** * The Astro global is sourced in 3 different phases: * - **Static**: `.generator` and `.glob` is printed by the compiler, instantiated once per process per astro file @@ -512,6 +514,7 @@ export class RenderContext { * So, it is computed and saved here on creation of the first APIContext and reused for later ones. */ #currentLocale: APIContext['currentLocale']; + computeCurrentLocale() { const { url, @@ -537,6 +540,7 @@ export class RenderContext { } #preferredLocale: APIContext['preferredLocale']; + computePreferredLocale() { const { pipeline: { i18n }, @@ -547,6 +551,7 @@ export class RenderContext { } #preferredLocaleList: APIContext['preferredLocaleList']; + computePreferredLocaleList() { const { pipeline: { i18n }, diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 64caf04f0940..39aa78394b38 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -4,6 +4,7 @@ import { DEFAULT_404_COMPONENT, REROUTE_DIRECTIVE_HEADER, clientLocalsSymbol, + REWRITE_DIRECTIVE_HEADER_KEY, } from '../core/constants.js'; import { AstroErrorData, isAstroError } from '../core/errors/index.js'; import { req } from '../core/messages.js'; @@ -218,8 +219,12 @@ export async function handleRoute({ }); let response; + let isReroute = false; + let isRewrite = false; try { response = await renderContext.render(mod); + isReroute = response.headers.has(REROUTE_DIRECTIVE_HEADER); + isRewrite = response.headers.has(REWRITE_DIRECTIVE_HEADER_KEY); } catch (err: any) { const custom500 = getCustom500Route(manifestData); if (!custom500) { @@ -264,7 +269,10 @@ export async function handleRoute({ } // We remove the internally-used header before we send the response to the user agent. - if (response.headers.has(REROUTE_DIRECTIVE_HEADER)) { + if (isReroute) { + response.headers.delete(REROUTE_DIRECTIVE_HEADER); + } + if (isRewrite) { response.headers.delete(REROUTE_DIRECTIVE_HEADER); } @@ -272,6 +280,14 @@ export async function handleRoute({ await writeWebResponse(incomingResponse, response); return; } + + // This check is important in case of rewrites. + // A route can start with a 404 code, then the rewrite kicks in and can return a 200 status code + if (isRewrite) { + await writeSSRResult(request, response, incomingResponse); + return; + } + // We are in a recursion, and it's possible that this function is called itself with a status code // By default, the status code passed via parameters is computed by the matched route. // diff --git a/packages/astro/test/rewrite.test.js b/packages/astro/test/rewrite.test.js index d14fb069a7c7..7839e7d3350b 100644 --- a/packages/astro/test/rewrite.test.js +++ b/packages/astro/test/rewrite.test.js @@ -520,6 +520,34 @@ describe('Runtime error in SSR, custom 500', () => { }); }); +describe('Runtime error in dev, custom 500', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + let devServer; + + before(async () => { + fixture = await loadFixture({ + root: './fixtures/rewrite-i18n-manual-routing/', + }); + + devServer = await fixture.startDevServer(); + }); + + after(async () => { + await devServer.stop(); + }); + + it('should return a status 200 when rewriting from the middleware to the homepage', async () => { + const response = await fixture.fetch('/reroute'); + assert.equal(response.status, 200); + const html = await response.text(); + + const $ = cheerioLoad(html); + + assert.equal($('h1').text(), 'Expected http status of index page is 200'); + }); +}); + describe('Runtime error in SSR, custom 500', () => { /** @type {import('./test-utils').Fixture} */ let fixture;