Normalize request paths before route matching (#17772)

* Use normalized paths for runtime route selection

* Normalize paths before route matching

Author: matthewp

.changeset/eight-actors-marry.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes route selection for normalized request paths in adapter and development request handling

packages/astro/src/core/app/base.ts

@@ -23,6 +23,7 @@ import { handleRequest } from '../routing/handler.js';
 import { getDefaultStatusCode } from '../routing/helpers.js';
 import { matchRequest } from '../routing/match-request.js';
 import { getRouteTable, matchRoute, updateRouteTable } from '../routing/route-table.js';
+import { validateAndDecodePathname } from '../util/pathname.js';
 import { setRenderOptions } from './render-options.js';
 import type { WaitUntilHook } from '../wait-until.js';
 import type { SSRManifest } from './types.js';
@@ -285,22 +286,22 @@ export abstract class BaseApp {
 	}
 
 	/**
-	 * Decodes a pathname with `decodeURI`, falling back to the raw pathname when it
-	 * contains an invalid percent-sequence (e.g. `%C0%AF`, an overlong-UTF-8 encoding of
-	 * `/` commonly sent by path-traversal scanners). A raw `decodeURI()` would throw
-	 * `URIError: URI malformed`, and because `match()` runs before `render()` that error
-	 * escapes the adapter's request handler as an uncaught exception (HTTP 500) that user
-	 * middleware can't catch.
+	 * Fully decodes a pathname, falling back to a single decode and then the raw pathname
+	 * when validation fails. Adapter matching runs before `render()`, so it must not throw
+	 * for request input that render-time validation handles.
 	 */
-	private safeDecodeURI(pathname: string): string {
+	private safeDecodePathname(pathname: string): string {
 		try {
-			return decodeURI(pathname);
+			return validateAndDecodePathname(pathname);
 		} catch (e: any) {
-			// Malformed request paths are expected client input (commonly from automated
-			// scanners) rather than a server fault, and this runs per-request on the hot
-			// path. Log at `debug` so it stays diagnosable without flooding error logs.
+			// Path decoding failures are request input rather than a server fault. Log at
+			// `debug` so they stay diagnosable without flooding error logs.
 			this.adapterLogger.debug(e.toString());
-			return pathname;
+			try {
+				return decodeURI(pathname);
+			} catch {
+				return pathname;
+			}
 		}
 	}
 
@@ -311,7 +312,7 @@ export abstract class BaseApp {
 	public getPathnameFromRequest(request: Request): string {
 		const url = new URL(request.url);
 		const pathname = prependForwardSlash(this.removeBase(url.pathname));
-		return this.safeDecodeURI(pathname);
+		return this.safeDecodePathname(pathname);
 	}
 
 	/**
@@ -398,7 +399,7 @@ export abstract class BaseApp {
 		if (!routeData) {
 			const domainPathname = this.computePathnameFromDomain(request);
 			if (domainPathname) {
-				routeData = matchRoute(this.manifest, this.safeDecodeURI(domainPathname));
+				routeData = matchRoute(this.manifest, this.safeDecodePathname(domainPathname));
 			}
 		}
 		const resolvedOptions: ResolvedRenderOptions = {

packages/astro/src/core/routing/match-request.ts

@@ -4,29 +4,29 @@ import type { SSRManifest } from '../app/types.js';
 import { computePathnameFromDomain } from '../i18n/domain.js';
 import { AstroIntegrationLogger } from '../logger/core.js';
 import { getLogger } from '../logger/manifest-logger.js';
+import { validateAndDecodePathname } from '../util/pathname.js';
 import { matchAllRoutes, matchRoute } from './route-table.js';
 
 /**
- * Decodes a pathname with `decodeURI`, falling back to the raw pathname when it
- * contains an invalid percent-sequence (e.g. `%C0%AF`, an overlong-UTF-8 encoding of
- * `/` commonly sent by path-traversal scanners). A raw `decodeURI()` would throw
- * `URIError: URI malformed`, and because `match()` runs before `render()` that error
- * escapes the adapter's request handler as an uncaught exception (HTTP 500) that user
- * middleware can't catch.
+ * Fully decodes a pathname, falling back to a single decode and then the raw pathname
+ * when validation fails. Matching runs before `render()`, so it must not throw for
+ * request input that render-time validation handles.
  */
-function safeDecodeURI(manifest: SSRManifest, pathname: string): string {
+function safeDecodePathname(manifest: SSRManifest, pathname: string): string {
 	try {
-		return decodeURI(pathname);
+		return validateAndDecodePathname(pathname);
 	} catch (e: any) {
-		// Malformed request paths are expected client input (commonly from automated
-		// scanners) rather than a server fault, and this runs per-request on the hot
-		// path. Log at `debug` so it stays diagnosable without flooding error logs.
-		// Allocated lazily — only on the malformed branch — with the same options
-		// and label as the facade's `adapterLogger`.
+		// Path decoding failures are request input rather than a server fault. Log at
+		// `debug` so they stay diagnosable without flooding error logs. The logger is
+		// allocated lazily with the same options and label as the facade's `adapterLogger`.
 		new AstroIntegrationLogger(getLogger(manifest).options, manifest.adapterName).debug(
 			e.toString(),
 		);
-		return pathname;
+		try {
+			return decodeURI(pathname);
+		} catch {
+			return pathname;
+		}
 	}
 }
 
@@ -56,7 +56,8 @@ export function matchRequest(
 	if (!pathname) {
 		pathname = prependForwardSlash(stripRequestBase(url.pathname, manifest.base));
 	}
-	const routeData = matchRoute(manifest, safeDecodeURI(manifest, pathname));
+	pathname = safeDecodePathname(manifest, pathname);
+	const routeData = matchRoute(manifest, pathname);
 	if (!routeData) return undefined;
 	if (allowPrerenderedRoutes) {
 		return routeData;
@@ -68,7 +69,7 @@ export function matchRequest(
 	// the same pattern should handle all other URLs.
 	if (routeData.prerender) {
 		if (routeData.params.length > 0) {
-			const allMatches = matchAllRoutes(manifest, safeDecodeURI(manifest, pathname));
+			const allMatches = matchAllRoutes(manifest, pathname);
 			return allMatches.find((r) => !r.prerender);
 		}
 		return undefined;

packages/astro/src/vite-plugin-app/handle-request.ts

@@ -8,6 +8,7 @@ import { createSafeError } from '../core/errors/index.js';
 import { setLogger } from '../core/logger/manifest-logger.js';
 import type { ModuleLoader } from '../core/module-loader/index.js';
 import { createRequest } from '../core/request.js';
+import { validateAndDecodePathname } from '../core/util/pathname.js';
 import { SERIALIZED_MANIFEST_ID } from '../manifest/serialized.js';
 import type { AstroSettings } from '../types/astro.js';
 import type { SSRManifest } from '../types/public/index.js';
@@ -114,8 +115,12 @@ export async function handleDevRequest(
 		pathname = '';
 	} else {
 		// We already have a middleware that checks if there's an incoming URL that has invalid URI, so it's safe
-		// to not handle the error: packages/astro/src/vite-plugin-astro-server/base.ts
-		pathname = decodeURI(url.pathname);
+		// to only handle paths that exceed the supported decoding depth here.
+		try {
+			pathname = validateAndDecodePathname(url.pathname);
+		} catch {
+			pathname = decodeURI(url.pathname);
+		}
 	}
 
 	// Add config.base back to url before passing it to SSR

packages/astro/test/units/app/double-encoding-bypass.test.ts

@@ -26,6 +26,11 @@ const apiCatchAllRouteData = parseRoute('api/[...path].ts', routeOptions, {
 	type: 'endpoint',
 });
 
+const adminRouteData = parseRoute('api/admin/users.ts', routeOptions, {
+	component: 'src/pages/api/admin/users.ts',
+	type: 'endpoint',
+});
+
 const publicRouteData = parseRoute('index.astro', routeOptions, {
 	component: 'src/pages/index.astro',
 });
@@ -50,6 +55,14 @@ const pageMap = new Map<string, any>([
 			}),
 		}),
 	],
+	[
+		adminRouteData.component,
+		async () => ({
+			page: async () => ({
+				GET: async () => new Response('admin'),
+			}),
+		}),
+	],
 	[
 		publicRouteData.component,
 		async () => ({
@@ -98,17 +111,39 @@ function createRewriteAuthMiddleware() {
 	})) as () => Promise<{ onRequest: MiddlewareHandler }>;
 }
 
+function createPassthroughMiddleware() {
+	return (async () => ({
+		onRequest: (async (_context, next) => next()) satisfies MiddlewareHandler,
+	})) as () => Promise<{ onRequest: MiddlewareHandler }>;
+}
+
 function createApp(middleware: ReturnType<typeof createAuthMiddleware>) {
 	return new App(
 		createManifest({
-			routes: [createRouteInfo(apiCatchAllRouteData), createRouteInfo(publicRouteData)],
+			routes: [
+				createRouteInfo(adminRouteData),
+				createRouteInfo(apiCatchAllRouteData),
+				createRouteInfo(publicRouteData),
+			],
 			pageMap: pageMap as any,
 			middleware: middleware as any,
 		}) as any,
 	);
 }
 
 describe('URL normalization: double-encoding middleware bypass', () => {
+	it('normalizes the request pathname before adapter route matching', async () => {
+		const app = createApp(createPassthroughMiddleware());
+		const request = new Request('http://example.com/api/%2561dmin/users');
+		const routeData = app.match(request);
+
+		assert.equal(app.getPathnameFromRequest(request), '/api/admin/users');
+		assert.equal(routeData?.route, '/api/admin/users');
+		const response = await app.render(request, { routeData });
+		assert.equal(response.status, 200);
+		assert.equal(await response.text(), 'admin');
+	});
+
 	it('middleware blocks /api/admin/users', async () => {
 		const app = createApp(createAuthMiddleware());
 		const request = new Request('http://example.com/api/admin/users');