From 3e00a3d2c05c70ebab1a95cee31aa78a5ea64556 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 12 Dec 2024 17:00:13 +0000 Subject: [PATCH 1/5] fix: handle requests for double slash in dev --- .changeset/twenty-cherries-switch.md | 5 ++++ .../astro/src/core/routing/manifest/create.ts | 25 +++++++++++++++++++ .../src/vite-plugin-astro-server/base.ts | 6 ++++- packages/astro/test/dev-routing.test.js | 13 ++++++++++ 4 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 .changeset/twenty-cherries-switch.md diff --git a/.changeset/twenty-cherries-switch.md b/.changeset/twenty-cherries-switch.md new file mode 100644 index 000000000000..c79c53284e21 --- /dev/null +++ b/.changeset/twenty-cherries-switch.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a bug that caused the dev server to return an error if requesting "//" diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 6dbcf18aee68..1b7d30f9cd3e 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -427,6 +427,10 @@ function detectRouteCollision(a: RouteData, b: RouteData, _config: AstroConfig, // Fallbacks are always added below other routes exactly to avoid collisions. return; } + // The double-slash redirect will never collide with any other route. + if (a.route === '//' || b.route === '//') { + return; + } if ( a.route === b.route && @@ -502,6 +506,27 @@ export async function createRouteManifest( } const redirectRoutes = createRedirectRoutes(params, routeMap, logger); + if (dev) { + // Add a redirect for `//` to `/`. We only do this in dev because different platforms + // vary a lot in how or whether they handle this sort of redirect, so we leave it to the adapter + redirectRoutes.push({ + type: 'redirect', + isIndex: false, + route: '//', + pattern: /^\/+$/, + segments: [], + params: [], + component: '/', + generate: () => '//', + pathname: '//', + prerender: false, + redirect: '/', + redirectRoute: routeMap.get('/'), + fallbackRoutes: [], + distURL: [], + origin: 'project', + }); + } // we remove the file based routes that were deemed redirects const filteredFiledBasedRoutes = fileBasedRoutes.filter((fileBasedRoute) => { diff --git a/packages/astro/src/vite-plugin-astro-server/base.ts b/packages/astro/src/vite-plugin-astro-server/base.ts index 562b89ba2204..06e590cdb915 100644 --- a/packages/astro/src/vite-plugin-astro-server/base.ts +++ b/packages/astro/src/vite-plugin-astro-server/base.ts @@ -9,6 +9,8 @@ import type { Logger } from '../core/logger/core.js'; import notFoundTemplate, { subpathNotUsedTemplate } from '../template/4xx.js'; import { writeHtmlResponse } from './response.js'; +const justSlashes = /^\/{2,}$/; + export function baseMiddleware( settings: AstroSettings, logger: Logger, @@ -24,7 +26,9 @@ export function baseMiddleware( let pathname: string; try { - pathname = decodeURI(new URL(url, 'http://localhost').pathname); + // If the request is for just slashes it will break the URL constructor, so we special case it. + // We just set the pathname to "//", which will match a redirect to "/" in the dev server. + pathname = justSlashes.test(url) ? '//' : decodeURI(new URL(url, 'http://localhost').pathname); } catch (e) { /* malform uri */ return next(e); diff --git a/packages/astro/test/dev-routing.test.js b/packages/astro/test/dev-routing.test.js index c6a19fc4e440..a43561769a1a 100644 --- a/packages/astro/test/dev-routing.test.js +++ b/packages/astro/test/dev-routing.test.js @@ -48,6 +48,19 @@ describe('Development Routing', () => { assert.equal(response.status, 200); }); + it('redirects when loading double slash', async () => { + const response = await fixture.fetch('//', { redirect: 'manual' }); + assert.equal(response.status, 301); + assert.equal(response.headers.get('Location'), '/'); + }); + + it('redirects when loading multiple slashes', async () => { + const response = await fixture.fetch('/////', { redirect: 'manual' }); + assert.equal(response.status, 301); + assert.equal(response.headers.get('Location'), '/'); + }); + + it('404 when loading invalid dynamic route', async () => { const response = await fixture.fetch('/2'); assert.equal(response.status, 404); From c3d9ef12293afc4108f885828da74829b1571cf9 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Fri, 13 Dec 2024 12:13:08 +0000 Subject: [PATCH 2/5] Handle base --- packages/astro/src/core/routing/manifest/create.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 1b7d30f9cd3e..97e872d4763a 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -506,7 +506,7 @@ export async function createRouteManifest( } const redirectRoutes = createRedirectRoutes(params, routeMap, logger); - if (dev) { + if (dev && !config.base) { // Add a redirect for `//` to `/`. We only do this in dev because different platforms // vary a lot in how or whether they handle this sort of redirect, so we leave it to the adapter redirectRoutes.push({ From e427a959e1d79bcd633c414a9980ced1265b7940 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Fri, 13 Dec 2024 13:36:41 +0000 Subject: [PATCH 3/5] Oops --- packages/astro/src/core/routing/manifest/create.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 97e872d4763a..769893b87b0a 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -506,7 +506,7 @@ export async function createRouteManifest( } const redirectRoutes = createRedirectRoutes(params, routeMap, logger); - if (dev && !config.base) { + if (dev && config.base === '/') { // Add a redirect for `//` to `/`. We only do this in dev because different platforms // vary a lot in how or whether they handle this sort of redirect, so we leave it to the adapter redirectRoutes.push({ From 1cf1b0b28c40f314c7a63e8a54efe99abf42ef11 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Fri, 13 Dec 2024 14:02:31 +0000 Subject: [PATCH 4/5] Snapshots --- .../astro/test/units/integrations/api.test.js | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/packages/astro/test/units/integrations/api.test.js b/packages/astro/test/units/integrations/api.test.js index 97da20c19332..88e4fd98fbb4 100644 --- a/packages/astro/test/units/integrations/api.test.js +++ b/packages/astro/test/units/integrations/api.test.js @@ -209,6 +209,13 @@ describe('Integration API', () => { params: [], origin: 'internal', }, + { + entrypoint: '/', + isPrerendered: false, + origin: 'project', + params: [], + pattern: '//', + }, { isPrerendered: false, entrypoint: 'astro-default-404.astro', @@ -271,6 +278,13 @@ describe('Integration API', () => { params: [], origin: 'internal', }, + { + entrypoint: '/', + isPrerendered: false, + origin: 'project', + params: [], + pattern: '//', + }, { isPrerendered: false, entrypoint: 'astro-default-404.astro', @@ -333,6 +347,13 @@ describe('Integration API', () => { params: [], origin: 'internal', }, + { + entrypoint: '/', + isPrerendered: false, + origin: 'project', + params: [], + pattern: '//', + }, { isPrerendered: false, entrypoint: 'astro-default-404.astro', From 69bc98a3f358152a8dec54078302f6a19da4c41d Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 16 Jan 2025 10:18:02 +0000 Subject: [PATCH 5/5] Move redirect out of routing --- packages/astro/src/core/routing/3xx.ts | 4 +-- .../astro/src/core/routing/manifest/create.ts | 25 ------------------- .../src/vite-plugin-astro-server/base.ts | 13 +++++----- .../src/vite-plugin-astro-server/response.ts | 12 +++++++++ .../astro/test/units/integrations/api.test.js | 21 ---------------- 5 files changed, 21 insertions(+), 54 deletions(-) diff --git a/packages/astro/src/core/routing/3xx.ts b/packages/astro/src/core/routing/3xx.ts index fd4e211ed7d6..c05d7a894f7d 100644 --- a/packages/astro/src/core/routing/3xx.ts +++ b/packages/astro/src/core/routing/3xx.ts @@ -1,5 +1,5 @@ export type RedirectTemplate = { - from: string; + from?: string; location: string | URL; status: number; }; @@ -14,6 +14,6 @@ export function redirectTemplate({ status, location, from }: RedirectTemplate) { - Redirecting from ${from} to ${location} + Redirecting ${from ? `from ${from} ` : ''}to ${location} `; } diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 119ea3548355..b0495a682ad8 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -406,10 +406,6 @@ function detectRouteCollision(a: RouteData, b: RouteData, _config: AstroConfig, // Fallbacks are always added below other routes exactly to avoid collisions. return; } - // The double-slash redirect will never collide with any other route. - if (a.route === '//' || b.route === '//') { - return; - } if ( a.route === b.route && @@ -485,27 +481,6 @@ export async function createRouteManifest( } const redirectRoutes = createRedirectRoutes(params, routeMap, logger); - if (dev && config.base === '/') { - // Add a redirect for `//` to `/`. We only do this in dev because different platforms - // vary a lot in how or whether they handle this sort of redirect, so we leave it to the adapter - redirectRoutes.push({ - type: 'redirect', - isIndex: false, - route: '//', - pattern: /^\/+$/, - segments: [], - params: [], - component: '/', - generate: () => '//', - pathname: '//', - prerender: false, - redirect: '/', - redirectRoute: routeMap.get('/'), - fallbackRoutes: [], - distURL: [], - origin: 'project', - }); - } // we remove the file based routes that were deemed redirects const filteredFiledBasedRoutes = fileBasedRoutes.filter((fileBasedRoute) => { diff --git a/packages/astro/src/vite-plugin-astro-server/base.ts b/packages/astro/src/vite-plugin-astro-server/base.ts index cab575dcb713..4aa7e2a2d9b7 100644 --- a/packages/astro/src/vite-plugin-astro-server/base.ts +++ b/packages/astro/src/vite-plugin-astro-server/base.ts @@ -7,9 +7,9 @@ import { appendForwardSlash } from '@astrojs/internal-helpers/path'; import { bold } from 'kleur/colors'; import type { Logger } from '../core/logger/core.js'; import notFoundTemplate, { subpathNotUsedTemplate } from '../template/4xx.js'; -import { writeHtmlResponse } from './response.js'; +import { writeHtmlResponse, writeRedirectResponse } from './response.js'; -const justSlashes = /^\/{2,}$/; +const manySlashes = /\/{2,}$/; export function baseMiddleware( settings: AstroSettings, @@ -23,12 +23,13 @@ export function baseMiddleware( return function devBaseMiddleware(req, res, next) { const url = req.url!; - + if (manySlashes.test(url)) { + const destination = url.replace(manySlashes, '/'); + return writeRedirectResponse(res, 301, destination); + } let pathname: string; try { - // If the request is for just slashes it will break the URL constructor, so we special case it. - // We just set the pathname to "//", which will match a redirect to "/" in the dev server. - pathname = justSlashes.test(url) ? '//' : decodeURI(new URL(url, 'http://localhost').pathname); + pathname = decodeURI(new URL(url, 'http://localhost').pathname); } catch (e) { /* malformed uri */ return next(e); diff --git a/packages/astro/src/vite-plugin-astro-server/response.ts b/packages/astro/src/vite-plugin-astro-server/response.ts index ef3c8247aac9..753e77c66cb7 100644 --- a/packages/astro/src/vite-plugin-astro-server/response.ts +++ b/packages/astro/src/vite-plugin-astro-server/response.ts @@ -7,6 +7,7 @@ import { Readable } from 'node:stream'; import { getSetCookiesFromResponse } from '../core/cookies/index.js'; import { getViteErrorPayload } from '../core/errors/dev/index.js'; import notFoundTemplate from '../template/4xx.js'; +import { redirectTemplate } from '../core/routing/3xx.js'; export async function handle404Response( origin: string, @@ -53,6 +54,17 @@ export function writeHtmlResponse(res: http.ServerResponse, statusCode: number, res.end(); } +export function writeRedirectResponse(res: http.ServerResponse, statusCode: number, location: string) { + const html = redirectTemplate({ status: statusCode, location }); + res.writeHead(statusCode, { + Location: location, + 'Content-Type': 'text/html', + 'Content-Length': Buffer.byteLength(html, 'utf-8'), + }); + res.write(html); + res.end(); +} + export async function writeWebResponse(res: http.ServerResponse, webResponse: Response) { const { status, headers, body, statusText } = webResponse; diff --git a/packages/astro/test/units/integrations/api.test.js b/packages/astro/test/units/integrations/api.test.js index a362d5a1d940..59f88ed965f4 100644 --- a/packages/astro/test/units/integrations/api.test.js +++ b/packages/astro/test/units/integrations/api.test.js @@ -209,13 +209,6 @@ describe('Integration API', () => { params: [], origin: 'internal', }, - { - entrypoint: '/', - isPrerendered: false, - origin: 'project', - params: [], - pattern: '//', - }, { isPrerendered: false, entrypoint: 'astro-default-404.astro', @@ -278,13 +271,6 @@ describe('Integration API', () => { params: [], origin: 'internal', }, - { - entrypoint: '/', - isPrerendered: false, - origin: 'project', - params: [], - pattern: '//', - }, { isPrerendered: false, entrypoint: 'astro-default-404.astro', @@ -347,13 +333,6 @@ describe('Integration API', () => { params: [], origin: 'internal', }, - { - entrypoint: '/', - isPrerendered: false, - origin: 'project', - params: [], - pattern: '//', - }, { isPrerendered: false, entrypoint: 'astro-default-404.astro',