fix(routing): multiple decoding (#12927)

.changeset/afraid-peas-smash.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes a bug where Astro attempted to decode a request URL multiple times, resulting in an unexpected behaviour when decoding the character `%`

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

@@ -149,10 +149,22 @@ export class App {
 		return pathname;
 	}
 
+	/**
+	 * It removes the base from the request URL, prepends it with a forward slash and attempts to decoded it.
+	 *
+	 * If the decoding fails, it logs the error and return the pathname as is.
+	 * @param request
+	 * @private
+	 */
 	#getPathnameFromRequest(request: Request): string {
 		const url = new URL(request.url);
 		const pathname = prependForwardSlash(this.removeBase(url.pathname));
-		return pathname;
+		try {
+			return decodeURI(pathname);
+		} catch (e: any) {
+			this.getAdapterLogger().error(e.toString());
+			return pathname;
+		}
 	}
 
 	match(request: Request): RouteData | undefined {

packages/astro/src/core/config/schema.ts

@@ -1,3 +1,6 @@
+import type { OutgoingHttpHeaders } from 'node:http';
+import path from 'node:path';
+import { fileURLToPath, pathToFileURL } from 'node:url';
 import type {
 	ShikiConfig,
 	RehypePlugin as _RehypePlugin,
@@ -6,10 +9,6 @@ import type {
 } from '@astrojs/markdown-remark';
 import { markdownConfigDefaults } from '@astrojs/markdown-remark';
 import { type BuiltinTheme, bundledThemes } from 'shiki';
-
-import type { OutgoingHttpHeaders } from 'node:http';
-import path from 'node:path';
-import { fileURLToPath, pathToFileURL } from 'node:url';
 import { z } from 'zod';
 import type { SvgRenderMode } from '../../assets/utils/svg.js';
 import { EnvSchema } from '../../env/schema.js';

packages/astro/src/core/middleware/sequence.ts

@@ -62,7 +62,11 @@ export function sequence(...handlers: MiddlewareHandler[]): MiddlewareHandler {
 						) {
 							throw new AstroError({
 								...ForbiddenRewrite,
-								message: ForbiddenRewrite.message(pathname, pathname, routeData.component),
+								message: ForbiddenRewrite.message(
+									handleContext.url.pathname,
+									pathname,
+									routeData.component,
+								),
 								hint: ForbiddenRewrite.hint(routeData.component),
 							});
 						}

packages/astro/src/core/render-context.ts

@@ -45,6 +45,7 @@ export class RenderContext {
 		readonly pipeline: Pipeline,
 		public locals: App.Locals,
 		readonly middleware: MiddlewareHandler,
+		// It must be a DECODED pathname
 		public pathname: string,
 		public request: Request,
 		public routeData: RouteData,
@@ -90,7 +91,7 @@ export class RenderContext {
 			pipeline,
 			locals,
 			sequence(...pipeline.internalMiddleware, middleware ?? pipelineMiddleware),
-			decodeURI(pathname),
+			pathname,
 			request,
 			routeData,
 			status,
@@ -102,7 +103,6 @@ export class RenderContext {
 			partial,
 		);
 	}
-
 	/**
 	 * The main function of the RenderContext.
 	 *

packages/astro/src/core/render/params-and-props.ts

@@ -50,7 +50,7 @@ export async function getProps(opts: GetParamsAndPropsOptions): Promise<Props> {
 	// The pathname used here comes from the server, which already encoded.
 	// Since we decided to not mess up with encoding anymore, we need to decode them back so the parameters can match
 	// the ones expected from the users
-	const params = getParams(route, decodeURI(pathname));
+	const params = getParams(route, pathname);
 	const matchedStaticPath = findPathItemByKey(staticPaths, params, route, logger);
 	if (!matchedStaticPath && (serverLike ? route.prerender : true)) {
 		throw new AstroError({

packages/astro/src/vite-plugin-astro-server/base.ts

@@ -26,7 +26,7 @@ export function baseMiddleware(
 		try {
 			pathname = decodeURI(new URL(url, 'http://localhost').pathname);
 		} catch (e) {
-			/* malform uri */
+			/* malformed uri */
 			return next(e);
 		}
 

packages/astro/src/vite-plugin-astro-server/request.ts

@@ -34,7 +34,9 @@ export async function handleRequest({
 	if (config.trailingSlash === 'never' && !incomingRequest.url) {
 		pathname = '';
 	} else {
-		pathname = url.pathname;
+		// 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);
 	}
 
 	// Add config.base back to url before passing it to SSR

packages/astro/test/fixtures/ssr-params/src/pages/[category].astro

@@ -5,6 +5,7 @@ export function getStaticPaths() {
 		{ params: { category: "%23something" } },
 		{ params: { category: "%2Fsomething" } },
 		{ params: { category: "%3Fsomething" } },
+		{ params: { category: "%25something" } },
 		{ params: { category: "[page]" } },
 		{ params: { category: "你好" } },
 	]

packages/astro/test/fixtures/ssr-params/src/pages/東西/[group].astro

@@ -1,10 +1,10 @@
 ---
 export function getStaticPaths() {
 	return [
-		{ params: { category: "food" } },
+		{ params: { group: "food" } },
 	]
 }
-const { category } = Astro.params
+const { group } = Astro.params
 ---
 <html>
 	<head>
@@ -12,6 +12,6 @@ const { category } = Astro.params
 	</head>
 	<body>
 		<h1>Testing</h1>
-		<h2 class="category">{ category }</h2>
+		<h2 class="category">{ group }</h2>
 	</body>
 </html>

packages/astro/test/params.test.js

@@ -148,4 +148,10 @@ describe('Astro.params in static mode', () => {
 		const $ = cheerio.load(html);
 		assert.equal($('.category').text(), '%3Fsomething');
 	});
+
+	it("It doesn't encode/decode URI characters such as %25 (%)", async () => {
+		const html = await fixture.readFile(encodeURI('/%25something/index.html'));
+		const $ = cheerio.load(html);
+		assert.equal($('.category').text(), '%25something');
+	});
 });