From b71bd10989c0070847cecb101afb8278d5ef7091 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Tue, 4 Feb 2025 09:44:01 +0000 Subject: [PATCH] fix: correct handling of collapsing slashes (#13130) --- .changeset/afraid-kangaroos-hope.md | 5 +++++ .changeset/pink-apes-invite.md | 5 +++++ .../trailing-slash.ts | 15 +++++++------ packages/astro/test/dev-routing.test.js | 16 ++++++++++++++ ...ng-slash.js => ssr-trailing-slash.test.js} | 22 +++++++++++++++++++ packages/internal-helpers/src/path.ts | 2 +- pnpm-lock.yaml | 1 - 7 files changed, 57 insertions(+), 9 deletions(-) create mode 100644 .changeset/afraid-kangaroos-hope.md create mode 100644 .changeset/pink-apes-invite.md rename packages/astro/test/{ssr-trailing-slash.js => ssr-trailing-slash.test.js} (90%) diff --git a/.changeset/afraid-kangaroos-hope.md b/.changeset/afraid-kangaroos-hope.md new file mode 100644 index 000000000000..0c137293cb41 --- /dev/null +++ b/.changeset/afraid-kangaroos-hope.md @@ -0,0 +1,5 @@ +--- +'@astrojs/internal-helpers': patch +--- + +Fixes a bug that meant that internal as well as trailing duplicate slashes were collapsed diff --git a/.changeset/pink-apes-invite.md b/.changeset/pink-apes-invite.md new file mode 100644 index 000000000000..f2c88fd163bc --- /dev/null +++ b/.changeset/pink-apes-invite.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a bug that caused duplicate slashes inside query params to be collapsed diff --git a/packages/astro/src/vite-plugin-astro-server/trailing-slash.ts b/packages/astro/src/vite-plugin-astro-server/trailing-slash.ts index 2588d4c497e1..00f71b784a57 100644 --- a/packages/astro/src/vite-plugin-astro-server/trailing-slash.ts +++ b/packages/astro/src/vite-plugin-astro-server/trailing-slash.ts @@ -9,15 +9,10 @@ export function trailingSlashMiddleware(settings: AstroSettings): vite.Connect.N const { trailingSlash } = settings.config; return function devTrailingSlash(req, res, next) { - const url = req.url!; - - const destination = collapseDuplicateTrailingSlashes(url, true); - if (url && destination !== url) { - return writeRedirectResponse(res, 301, destination); - } + const url = new URL(`http://localhost${req.url}`); let pathname: string; try { - pathname = decodeURI(new URL(url, 'http://localhost').pathname); + pathname = decodeURI(url.pathname); } catch (e) { /* malformed uri */ return next(e); @@ -25,6 +20,12 @@ export function trailingSlashMiddleware(settings: AstroSettings): vite.Connect.N if (pathname.startsWith('/_') || pathname.startsWith('/@')) { return next(); } + + const destination = collapseDuplicateTrailingSlashes(pathname, true); + if (pathname && destination !== pathname) { + return writeRedirectResponse(res, 301, `${destination}${url.search}`); + } + if ( (trailingSlash === 'never' && pathname.endsWith('/') && pathname !== '/') || (trailingSlash === 'always' && !pathname.endsWith('/') && !hasFileExtension(pathname)) diff --git a/packages/astro/test/dev-routing.test.js b/packages/astro/test/dev-routing.test.js index 2f4b84bc5284..1f5ff26c68f4 100644 --- a/packages/astro/test/dev-routing.test.js +++ b/packages/astro/test/dev-routing.test.js @@ -60,6 +60,22 @@ describe('Development Routing', () => { assert.equal(response.headers.get('Location'), '/'); }); + it('does not redirect multiple internal slashes', async () => { + const response = await fixture.fetch('/another///here', { redirect: 'manual' }); + assert.equal(response.status, 404); + }); + + it('does not redirect slashes on query params', async () => { + const response = await fixture.fetch('/another?foo=bar///', { redirect: 'manual' }); + assert.equal(response.status, 200); + }); + + it('does redirect multiple trailing slashes with query params', async () => { + const response = await fixture.fetch('/another///?foo=bar///', { redirect: 'manual' }); + assert.equal(response.status, 301); + assert.equal(response.headers.get('Location'), '/another/?foo=bar///'); + }); + it('404 when loading invalid dynamic route', async () => { const response = await fixture.fetch('/2'); assert.equal(response.status, 404); diff --git a/packages/astro/test/ssr-trailing-slash.js b/packages/astro/test/ssr-trailing-slash.test.js similarity index 90% rename from packages/astro/test/ssr-trailing-slash.js rename to packages/astro/test/ssr-trailing-slash.test.js index b34430a8e88e..d512aa6099df 100644 --- a/packages/astro/test/ssr-trailing-slash.js +++ b/packages/astro/test/ssr-trailing-slash.test.js @@ -33,6 +33,28 @@ describe('Redirecting trailing slashes in SSR', () => { assert.equal(response.headers.get('Location'), '/another/'); }); + it('Redirects to collapse multiple trailing slashes with query param', async () => { + const app = await fixture.loadTestAdapterApp(); + const request = new Request('http://example.com/another///?hello=world'); + const response = await app.render(request); + assert.equal(response.status, 301); + assert.equal(response.headers.get('Location'), '/another/?hello=world'); + }); + + it('Does not redirect to collapse multiple internal slashes', async () => { + const app = await fixture.loadTestAdapterApp(); + const request = new Request('http://example.com/another///path/'); + const response = await app.render(request); + assert.equal(response.status, 404); + }); + + it('Does not redirect trailing slashes on query params', async () => { + const app = await fixture.loadTestAdapterApp(); + const request = new Request('http://example.com/another/?hello=world///'); + const response = await app.render(request); + assert.equal(response.status, 200); + }); + it('Does not redirect when trailing slash is present', async () => { const app = await fixture.loadTestAdapterApp(); const request = new Request('http://example.com/another/'); diff --git a/packages/internal-helpers/src/path.ts b/packages/internal-helpers/src/path.ts index 158cb58c7807..c7dda9d89017 100644 --- a/packages/internal-helpers/src/path.ts +++ b/packages/internal-helpers/src/path.ts @@ -19,7 +19,7 @@ export function collapseDuplicateSlashes(path: string) { return path.replace(/(?