Fix duplicate slash handling (#7935)

* fix(#7806): collapse duplicate slashes * refactor: handle request.url with duplicate slashes * chore: improve duplicate slash test * fix: only collapse duplicate slashes once * chore: appease TS
withastro · Aug 3, 2023 · 6035bb3 · 6035bb3
1 parent 705432f
commit 6035bb3
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 6 deletions.
diff --git a/.changeset/clean-tools-yawn.md b/.changeset/clean-tools-yawn.md
@@ -0,0 +1,5 @@
+---
+'@astrojs/internal-helpers': patch
+---
+
+Add `collapseDuplicateSlashes` helper
diff --git a/.changeset/flat-toes-fold.md b/.changeset/flat-toes-fold.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Properly handle routing when multiple slashes are present in the request by collapsing them to a single `/`
diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts
@@ -10,7 +10,7 @@ import type { SinglePageBuiltModule } from '../build/types';
 import { attachToResponse, getSetCookiesFromResponse } from '../cookies/index.js';
 import { consoleLogDestination } from '../logger/console.js';
 import { error, type LogOptions } from '../logger/core.js';
-import { prependForwardSlash, removeTrailingForwardSlash } from '../path.js';
+import { prependForwardSlash, removeTrailingForwardSlash, collapseDuplicateSlashes } from '../path.js';
 import { RedirectSinglePageBuiltModule } from '../redirects/index.js';
 import { isResponse } from '../render/core.js';
 import {
@@ -126,13 +126,17 @@ export class App {
 		const url = new URL(request.url);
 		// ignore requests matching public assets
 		if (this.#manifest.assets.has(url.pathname)) return undefined;
-		let pathname = prependForwardSlash(this.removeBase(url.pathname));
-		let routeData = matchRoute(pathname, this.#manifestData);
+		const pathname = prependForwardSlash(this.removeBase(url.pathname));
+		const routeData = matchRoute(pathname, this.#manifestData);
 		// missing routes fall-through, prerendered are handled by static layer
 		if (!routeData || routeData.prerender) return undefined;
 		return routeData;
 	}
 	async render(request: Request, routeData?: RouteData, locals?: object): Promise<Response> {
+		// Handle requests with duplicate slashes gracefully by cloning with a cleaned-up request URL
+		if (request.url !== collapseDuplicateSlashes(request.url)) {
+			request = new Request(collapseDuplicateSlashes(request.url), request);
+		}
 		if (!routeData) {
 			routeData = this.match(request);
 		}

diff --git a/packages/astro/src/vite-plugin-astro-server/request.ts b/packages/astro/src/vite-plugin-astro-server/request.ts
@@ -7,7 +7,7 @@ import { collectErrorMetadata } from '../core/errors/dev/index.js';
 import { createSafeError } from '../core/errors/index.js';
 import { error } from '../core/logger/core.js';
 import * as msg from '../core/messages.js';
-import { removeTrailingForwardSlash } from '../core/path.js';
+import { removeTrailingForwardSlash, collapseDuplicateSlashes } from '../core/path.js';
 import { eventError, telemetry } from '../events/index.js';
 import { isServerLikeOutput } from '../prerender/utils.js';
 import { runWithErrorHandling } from './controller.js';
@@ -37,7 +37,7 @@ export async function handleRequest({
 	const origin = `${moduleLoader.isHttps() ? 'https' : 'http'}://${incomingRequest.headers.host}`;
 	const buildingToSSR = isServerLikeOutput(config);
 
-	const url = new URL(origin + incomingRequest.url);
+	const url = new URL(collapseDuplicateSlashes(origin + incomingRequest.url));
 	let pathname: string;
 	if (config.trailingSlash === 'never' && !incomingRequest.url) {
 		pathname = '';

diff --git a/packages/astro/test/fixtures/ssr-request/src/pages/request.astro b/packages/astro/test/fixtures/ssr-request/src/pages/request.astro
@@ -1,5 +1,7 @@
 ---
 const origin = Astro.url.origin;
+const requestPathname = new URL(Astro.request.url).pathname;
+const pathname = Astro.url.pathname;
 ---
 
 <html>
@@ -15,6 +17,8 @@ const origin = Astro.url.origin;
 	</script>
 </head>
 <body>
-<h1 id="origin">{origin}</h1>
+<p id="origin">{origin}</p>
+<p id="pathname">{pathname}</p>
+<p id="request-pathname">{requestPathname}</p>
 </body>
 </html>
diff --git a/packages/astro/test/ssr-request.test.js b/packages/astro/test/ssr-request.test.js
@@ -42,6 +42,18 @@ describe('Using Astro.request in SSR', () => {
 		expect($('#origin').text()).to.equal('http://example.com');
 	});
 
+	it('Duplicate slashes are collapsed', async () => {
+		const app = await fixture.loadTestAdapterApp();
+		const request = new Request('http://example.com/subpath////request/////');
+		const response = await app.render(request);
+		expect(response.status).to.equal(200);
+		const html = await response.text();
+		const $ = cheerioLoad(html);
+		expect($('#origin').text()).to.equal('http://example.com');
+		expect($('#pathname').text()).to.equal('/subpath/request/');
+		expect($('#request-pathname').text()).to.equal('/subpath/request/');
+	});
+
 	it('public file is copied over', async () => {
 		const json = await fixture.readFile('/client/cars.json');
 		expect(json).to.not.be.undefined;

diff --git a/packages/internal-helpers/src/path.ts b/packages/internal-helpers/src/path.ts
@@ -15,6 +15,10 @@ export function prependForwardSlash(path: string) {
 	return path[0] === '/' ? path : '/' + path;
 }
 
+export function collapseDuplicateSlashes(path: string) {
+	return path.replace(/(?<!:)\/\/+/g, '/');
+}
+
 export function removeTrailingForwardSlash(path: string) {
 	return path.endsWith('/') ? path.slice(0, path.length - 1) : path;
 }