From 4b5d95dfd6cee5db7a0dec90cd18bc8756bee1f0 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Fri, 1 May 2026 09:42:19 -0400 Subject: [PATCH 1/2] Reject double-encoded URL paths instead of silently falling back to partial decoding --- .changeset/reject-double-encoded-paths.md | 5 + packages/astro/src/core/app/base.ts | 4 + packages/astro/src/core/render-context.ts | 13 +- packages/astro/src/core/util/pathname.ts | 17 +- .../units/app/double-encoding-bypass.test.ts | 160 ++++++++++++++++++ 5 files changed, 192 insertions(+), 7 deletions(-) create mode 100644 .changeset/reject-double-encoded-paths.md create mode 100644 packages/astro/test/units/app/double-encoding-bypass.test.ts diff --git a/.changeset/reject-double-encoded-paths.md b/.changeset/reject-double-encoded-paths.md new file mode 100644 index 000000000000..d904898ec86a --- /dev/null +++ b/.changeset/reject-double-encoded-paths.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Rejects double-encoded URL paths with a 400 response instead of silently falling back to partial decoding diff --git a/packages/astro/src/core/app/base.ts b/packages/astro/src/core/app/base.ts index a145b3c3a1de..30c93e49362e 100644 --- a/packages/astro/src/core/app/base.ts +++ b/packages/astro/src/core/app/base.ts @@ -38,6 +38,7 @@ import { routeHasHtmlExtension } from '../routing/helpers.js'; import { matchRoute } from '../routing/match.js'; import { type CacheLike, applyCacheHeaders } from '../cache/runtime/cache.js'; import { Router } from '../routing/router.js'; +import { MultiLevelEncodingError } from '../util/pathname.js'; import { type AstroSession, PERSIST_SYMBOL } from '../session/runtime.js'; import type { WaitUntilHook } from '../wait-until.js'; import type { AppPipeline } from './pipeline.js'; @@ -573,6 +574,9 @@ export abstract class BaseApp

{ timeStart, }); } catch (err: any) { + if (err instanceof MultiLevelEncodingError) { + return new Response('Bad Request', { status: 400 }); + } this.logger.error(null, err.stack || err.message || String(err)); return this.renderError(request, { ...resolvedRenderOptions, diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index 17b6276d54f3..caaac3351d2f 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -42,7 +42,7 @@ import { NoopAstroCache, DisabledAstroCache } from './cache/runtime/noop.js'; import { compileCacheRoutes, matchCacheRoute } from './cache/runtime/route-matching.js'; import { AstroSession } from './session/runtime.js'; import { collapseDuplicateSlashes } from '@astrojs/internal-helpers/path'; -import { validateAndDecodePathname } from './util/pathname.js'; +import { MultiLevelEncodingError, validateAndDecodePathname } from './util/pathname.js'; /** * Each request is rendered using a `RenderContext`. @@ -136,10 +136,13 @@ export class RenderContext { try { // Decode and validate pathname to prevent multi-level encoding bypass attacks url.pathname = validateAndDecodePathname(url.pathname); - } catch { - // If validation fails, return URL with pathname as-is - // This will be caught elsewhere in the request handling pipeline - // For now, just decode without validation to maintain compatibility + } catch (e) { + // Multi-level encoding (e.g., %2561 → %61) must be rejected, not silently decoded. + // Let this error propagate so the caller can return a 400 response. + if (e instanceof MultiLevelEncodingError) { + throw e; + } + // For other decoding failures (truly malformed URLs), fall back gracefully. try { url.pathname = decodeURI(url.pathname); } catch { diff --git a/packages/astro/src/core/util/pathname.ts b/packages/astro/src/core/util/pathname.ts index ed79de2c1a9a..f8ebf98ffee7 100644 --- a/packages/astro/src/core/util/pathname.ts +++ b/packages/astro/src/core/util/pathname.ts @@ -1,3 +1,15 @@ +/** + * Error thrown when multi-level URL encoding is detected in a pathname. + * This is a distinct error type so callers can handle it specifically + * (e.g., returning a 400 response) rather than falling back to partial decoding. + */ +export class MultiLevelEncodingError extends Error { + constructor() { + super('Multi-level URL encoding is not allowed'); + this.name = 'MultiLevelEncodingError'; + } +} + /** * Validates that a pathname is not multi-level encoded. * Detects if a pathname contains encoding that was encoded again (e.g., %2561dmin where %25 decodes to %). @@ -5,7 +17,8 @@ * * @param pathname - The pathname to validate * @returns The decoded pathname if valid - * @throws Error if multi-level encoding is detected + * @throws MultiLevelEncodingError if multi-level encoding is detected + * @throws Error if the pathname contains invalid URL encoding */ export function validateAndDecodePathname(pathname: string): string { let decoded: string; @@ -24,7 +37,7 @@ export function validateAndDecodePathname(pathname: string): string { const decodedStillHasEncoding = /%[0-9a-fA-F]{2}/.test(decoded); if (hasDecoding && decodedStillHasEncoding) { - throw new Error('Multi-level URL encoding is not allowed'); + throw new MultiLevelEncodingError(); } return decoded; diff --git a/packages/astro/test/units/app/double-encoding-bypass.test.ts b/packages/astro/test/units/app/double-encoding-bypass.test.ts new file mode 100644 index 000000000000..ab61e300f5f3 --- /dev/null +++ b/packages/astro/test/units/app/double-encoding-bypass.test.ts @@ -0,0 +1,160 @@ +import assert from 'node:assert/strict'; +import { describe, it } from 'node:test'; +import { App } from '../../../dist/core/app/app.js'; +import { parseRoute } from '../../../dist/core/routing/parse-route.js'; +import { createComponent, render } from '../../../dist/runtime/server/index.js'; +import type { MiddlewareHandler } from '../../../dist/types/public/common.js'; +import { createManifest, createRouteInfo } from './test-helpers.ts'; + +/** + * Tests that double-URL-encoded paths do not bypass middleware authorization. + * + * When a path like /api/%2561dmin/users is received, validateAndDecodePathname + * detects the multi-level encoding and must reject the request rather than + * silently falling back to a single decodeURI() that leaves middleware + * seeing a half-decoded pathname (/api/%61dmin/users) that doesn't match + * its authorization checks. + */ + +const routeOptions: Parameters[1] = { + config: { base: '/', trailingSlash: 'ignore' }, + pageExtensions: [], +} as any; + +// Catch-all API endpoint: /api/[...path] +// Represents a typical BFF or API proxy behind middleware auth. +const apiCatchAllRouteData = parseRoute('api/[...path].ts', routeOptions, { + component: 'src/pages/api/[...path].ts', + type: 'endpoint', +}); + +const publicRouteData = parseRoute('index.astro', routeOptions, { + component: 'src/pages/index.astro', +}); + +const publicPage = createComponent((_result: any, _props: any, _slots: any) => { + return render`

Public

`; +}); + +const pageMap = new Map([ + [ + apiCatchAllRouteData.component, + async () => ({ + page: async () => ({ + GET: async ({ params, url }: { params: Record; url: URL }) => + new Response( + JSON.stringify({ + path: params.path, + url_pathname: url.pathname, + }), + { headers: { 'Content-Type': 'application/json' } }, + ), + }), + }), + ], + [ + publicRouteData.component, + async () => ({ + page: async () => ({ + default: publicPage, + }), + }), + ], +]); + +/** + * Middleware that blocks access to /api/admin, as documented in Astro's + * middleware guide for path-based authorization. + */ +function createAuthMiddleware() { + return (async () => ({ + onRequest: (async (context, next) => { + if (context.url.pathname.startsWith('/api/admin')) { + return new Response(JSON.stringify({ error: 'Unauthorized' }), { + status: 401, + headers: { 'Content-Type': 'application/json' }, + }); + } + return next(); + }) satisfies MiddlewareHandler, + })) as () => Promise<{ onRequest: MiddlewareHandler }>; +} + +function createApp(middleware: ReturnType) { + return new App( + createManifest({ + routes: [createRouteInfo(apiCatchAllRouteData), createRouteInfo(publicRouteData)], + pageMap: pageMap as any, + middleware: middleware as any, + }) as any, + ); +} + +describe('URL normalization: double-encoding middleware bypass', () => { + it('middleware blocks /api/admin/users', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/api/admin/users'); + const response = await app.render(request); + assert.equal(response.status, 401, '/api/admin/users should be blocked by middleware'); + }); + + it('middleware blocks single-encoded /api/%61dmin/users', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/api/%61dmin/users'); + const response = await app.render(request); + assert.equal( + response.status, + 401, + '/api/%61dmin/users should be blocked (single encoding decodes to /api/admin/users)', + ); + }); + + it('rejects double-encoded /api/%2561dmin/users with 400', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/api/%2561dmin/users'); + const response = await app.render(request); + assert.equal( + response.status, + 400, + '/api/%2561dmin/users should be rejected as a bad request, not served', + ); + }); + + it('rejects double-encoded paths with multiple encoded segments', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/api/%2561dmin/%75sers'); + const response = await app.render(request); + assert.equal( + response.status, + 400, + '/api/%2561dmin/%75sers should be rejected as a bad request', + ); + }); + + it('public route is still accessible', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/'); + const response = await app.render(request); + assert.equal(response.status, 200, '/ should be accessible'); + const html = await response.text(); + assert.match(html, /Public/); + }); + + it('non-protected API routes are still accessible', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/api/public/data'); + const response = await app.render(request); + assert.equal(response.status, 200, '/api/public/data should be accessible'); + const body = await response.json(); + assert.equal(body.path, 'public/data'); + }); + + it('single-encoded non-admin API routes still work', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/api/us%65rs/list'); + const response = await app.render(request); + assert.equal(response.status, 200, '/api/us%65rs/list should be accessible'); + const body = await response.json(); + assert.equal(body.path, 'users/list'); + }); +}); From f2914ee37fef03b72e7288f98eef726a5bcf0742 Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Fri, 1 May 2026 15:34:18 -0400 Subject: [PATCH 2/2] Fix double-encoding tests to expect 400 instead of 404 --- packages/astro/test/middleware.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/astro/test/middleware.test.ts b/packages/astro/test/middleware.test.ts index a2d7bc2177e0..0b2012142277 100644 --- a/packages/astro/test/middleware.test.ts +++ b/packages/astro/test/middleware.test.ts @@ -22,14 +22,14 @@ describe('Middleware in DEV mode', () => { }); describe('Path encoding in middleware', () => { - it('should reject double-encoded paths with 404', async () => { + it('should reject double-encoded paths with 400', async () => { const res = await fixture.fetch('/%2561dmin', { redirect: 'manual' }); - assert.equal(res.status, 404); + assert.equal(res.status, 400); }); - it('should reject triple-encoded paths with 404', async () => { + it('should reject triple-encoded paths with 400', async () => { const res = await fixture.fetch('/%252561dmin', { redirect: 'manual' }); - assert.equal(res.status, 404); + assert.equal(res.status, 400); }); });