fix: correct handling of collapsing slashes (#13130)

Author: ascorbic

.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

.changeset/pink-apes-invite.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes a bug that caused duplicate slashes inside query params to be collapsed

packages/astro/src/vite-plugin-astro-server/trailing-slash.ts

@@ -9,22 +9,23 @@ 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);
 		}
 		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))

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);

packages/astro/test/ssr-trailing-slash.js …es/astro/test/ssr-trailing-slash.test.jspackages/astro/test/ssr-trailing-slash.js renamed to packages/astro/test/ssr-trailing-slash.test.js packages/astro/test/ssr-trailing-slash.js renamed to 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/');

packages/internal-helpers/src/path.ts

@@ -19,7 +19,7 @@ export function collapseDuplicateSlashes(path: string) {
 	return path.replace(/(?<!:)\/{2,}/g, '/');
 }
 
-export const MANY_TRAILING_SLASHES = /\/{2,}/g;
+export const MANY_TRAILING_SLASHES = /\/{2,}$/g;
 
 export function collapseDuplicateTrailingSlashes(path: string, trailingSlash: boolean) {
 	if (!path) {