diff --git a/.changeset/polite-balloons-rhyme.md b/.changeset/polite-balloons-rhyme.md new file mode 100644 index 000000000000..319f9a7467ba --- /dev/null +++ b/.changeset/polite-balloons-rhyme.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Ensures that URLs with multiple leading slashes (e.g. `//admin`) are normalized to a single slash before reaching middleware, so that pathname checks like `context.url.pathname.startsWith('/admin')` work consistently regardless of the request URL format diff --git a/packages/astro/src/core/app/base.ts b/packages/astro/src/core/app/base.ts index bb263aa19407..bc4b299b7005 100644 --- a/packages/astro/src/core/app/base.ts +++ b/packages/astro/src/core/app/base.ts @@ -1,5 +1,6 @@ import { appendForwardSlash, + collapseDuplicateLeadingSlashes, collapseDuplicateTrailingSlashes, hasFileExtension, isInternalPath, @@ -187,6 +188,10 @@ export abstract class BaseApp

{ } public removeBase(pathname: string) { + // Collapse multiple leading slashes to prevent middleware authorization bypass. + // Without this, `//admin` would be treated as starting with base `/` and sliced + // to `/admin` for routing, while middleware still sees `//admin` in the URL. + pathname = collapseDuplicateLeadingSlashes(pathname); if (pathname.startsWith(this.manifest.base)) { return pathname.slice(this.baseWithoutTrailingSlash.length + 1); } diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index ca52a80bf4b9..aaa4071b86df 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -37,6 +37,7 @@ import { getParams, getProps, type Pipeline, Slots } from './render/index.js'; import { isRoute404or500, isRouteExternalRedirect, isRouteServerIsland } from './routing/match.js'; import { copyRequest, getOriginPathname, setOriginPathname } from './routing/rewrite.js'; import { AstroSession } from './session/runtime.js'; +import { collapseDuplicateLeadingSlashes } from '@astrojs/internal-helpers/path'; import { validateAndDecodePathname } from './util/pathname.js'; /** @@ -86,6 +87,10 @@ export class RenderContext { static #createNormalizedUrl(requestUrl: string): URL { const url = new URL(requestUrl); + // Collapse multiple leading slashes so middleware sees the canonical pathname. + // Without this, a request to `//admin` would preserve `//admin` in context.url.pathname, + // bypassing middleware checks like `pathname.startsWith('/admin')`. + url.pathname = collapseDuplicateLeadingSlashes(url.pathname); try { // Decode and validate pathname to prevent multi-level encoding bypass attacks url.pathname = validateAndDecodePathname(url.pathname); diff --git a/packages/astro/test/units/app/double-slash-bypass.test.js b/packages/astro/test/units/app/double-slash-bypass.test.js new file mode 100644 index 000000000000..f5defd12b491 --- /dev/null +++ b/packages/astro/test/units/app/double-slash-bypass.test.js @@ -0,0 +1,162 @@ +// @ts-check +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 { createManifest } from './test-helpers.js'; + +/** + * Security tests for double-slash URL prefix middleware authorization bypass. + * + * Vulnerability: A normalization inconsistency between route matching and middleware + * URL construction allows bypassing middleware-based authorization by prepending an + * extra `/` to the URL path (e.g., `//admin` instead of `/admin`). + * + * - `removeBase("//admin")` strips one slash → router matches `/admin` + * - `context.url.pathname` preserves `//admin` → middleware `startsWith("/admin")` fails + * + * See: withastro/astro-security#5 + * CWE-647: Use of Non-Canonical URL Paths for Authorization Decisions + * CWE-285: Improper Authorization + */ + +const routeOptions = /** @type {Parameters[1]} */ ( + /** @type {any} */ ({ + config: { base: '/', trailingSlash: 'ignore' }, + pageExtensions: [], + }) +); + +const adminRouteData = parseRoute('admin', routeOptions, { + component: 'src/pages/admin.astro', +}); + +const dashboardRouteData = parseRoute('dashboard', routeOptions, { + component: 'src/pages/dashboard.astro', +}); + +const publicRouteData = parseRoute('index.astro', routeOptions, { + component: 'src/pages/index.astro', +}); + +const adminPage = createComponent(() => { + return render`

Admin Panel

`; +}); + +const dashboardPage = createComponent(() => { + return render`

Dashboard

`; +}); + +const publicPage = createComponent(() => { + return render`

Public

`; +}); + +const pageMap = new Map([ + [ + adminRouteData.component, + async () => ({ + page: async () => ({ + default: adminPage, + }), + }), + ], + [ + dashboardRouteData.component, + async () => ({ + page: async () => ({ + default: dashboardPage, + }), + }), + ], + [ + publicRouteData.component, + async () => ({ + page: async () => ({ + default: publicPage, + }), + }), + ], +]); + +/** + * Middleware that blocks access to /admin and /dashboard routes, + * as recommended in the official Astro authentication docs. + * @returns {() => Promise<{onRequest: import('../../../dist/types/public/common.js').MiddlewareHandler}>} + */ +function createAuthMiddleware() { + return async () => ({ + onRequest: /** @type {import('../../../dist/types/public/common.js').MiddlewareHandler} */ ( + async (context, next) => { + const protectedPaths = ['/admin', '/dashboard']; + if (protectedPaths.some((p) => context.url.pathname.startsWith(p))) { + return new Response('Forbidden', { status: 403 }); + } + return next(); + } + ), + }); +} + +/** + * @param {ReturnType} middleware + */ +function createApp(middleware) { + return new App( + createManifest({ + routes: [ + { routeData: adminRouteData }, + { routeData: dashboardRouteData }, + { routeData: publicRouteData }, + ], + pageMap, + middleware, + }), + ); +} + +describe('Security: double-slash URL prefix middleware bypass', () => { + it('middleware blocks /admin with normal request', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com/admin'); + const response = await app.render(request); + assert.equal(response.status, 403, '/admin should be blocked by middleware'); + }); + + it('middleware blocks //admin (double-slash bypass attempt)', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com//admin'); + const response = await app.render(request); + assert.equal(response.status, 403, '//admin should also be blocked by middleware'); + }); + + it('middleware blocks ///admin (triple-slash bypass attempt)', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com///admin'); + const response = await app.render(request); + assert.equal(response.status, 403, '///admin should also be blocked by middleware'); + }); + + it('middleware blocks //dashboard (double-slash on another protected route)', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com//dashboard'); + const response = await app.render(request); + assert.equal(response.status, 403, '//dashboard should also be blocked by middleware'); + }); + + it('middleware blocks //admin/ (double-slash with trailing slash)', async () => { + const app = createApp(createAuthMiddleware()); + const request = new Request('http://example.com//admin/'); + const response = await app.render(request); + assert.equal(response.status, 403, '//admin/ should also be blocked by middleware'); + }); + + 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/); + }); +}); diff --git a/packages/astro/test/units/app/test-helpers.js b/packages/astro/test/units/app/test-helpers.js index 1a7cb3c89997..9a48dd887dd7 100644 --- a/packages/astro/test/units/app/test-helpers.js +++ b/packages/astro/test/units/app/test-helpers.js @@ -1,10 +1,24 @@ // @ts-check -export function createManifest({ routes, pageMap, base = '/', trailingSlash = 'ignore' } = {}) { +/** + * @param {object} [options] + * @param {any[]} [options.routes] + * @param {Map} [options.pageMap] + * @param {string} [options.base] + * @param {string} [options.trailingSlash] + * @param {Function} [options.middleware] + */ +export function createManifest({ + routes, + pageMap, + base = '/', + trailingSlash = 'ignore', + middleware = undefined, +} = {}) { const rootDir = new URL('file:///astro-test/'); const buildDir = new URL('file:///astro-test/dist/'); - return { + return /** @type {import('../../../dist/core/app/types.js').SSRManifest} */ ({ adapterName: 'test-adapter', routes, site: undefined, @@ -16,6 +30,7 @@ export function createManifest({ routes, pageMap, base = '/', trailingSlash = 'i assetsPrefix: undefined, renderers: [], serverLike: true, + middlewareMode: /** @type {'classic'} */ ('classic'), clientDirectives: new Map(), entryModules: {}, inlinedScripts: new Map(), @@ -26,11 +41,12 @@ export function createManifest({ routes, pageMap, base = '/', trailingSlash = 'i serverIslandMappings: undefined, key: Promise.resolve(/** @type {CryptoKey} */ ({})), i18n: undefined, - middleware: undefined, + middleware, actions: undefined, sessionDriver: undefined, checkOrigin: false, allowedDomains: undefined, + actionBodySizeLimit: 0, sessionConfig: undefined, cacheDir: rootDir, srcDir: rootDir, @@ -54,7 +70,7 @@ export function createManifest({ routes, pageMap, base = '/', trailingSlash = 'i experimentalQueuedRendering: { enabled: false, }, - }; + }); } export function createRouteInfo(routeData) { diff --git a/packages/internal-helpers/src/path.ts b/packages/internal-helpers/src/path.ts index 3f551ab433ac..060c8ca4e4d5 100644 --- a/packages/internal-helpers/src/path.ts +++ b/packages/internal-helpers/src/path.ts @@ -15,6 +15,15 @@ export function prependForwardSlash(path: string) { return path[0] === '/' ? path : '/' + path; } +export const MANY_LEADING_SLASHES = /^\/{2,}/; + +export function collapseDuplicateLeadingSlashes(path: string) { + if (!path) { + return path; + } + return path.replace(MANY_LEADING_SLASHES, '/'); +} + export const MANY_TRAILING_SLASHES = /\/{2,}$/g; export function collapseDuplicateTrailingSlashes(path: string, trailingSlash: boolean) {