Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/nine-colts-add.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'astro': patch
'@astrojs/internal-helpers': patch
---

Fixes base path stripping to respect path-segment boundaries. With a configured `base` such as `/docs`, a request like `/docs-archive/page` is no longer treated as being under the base, so routing and `context.url.pathname` now agree on the same pathname.
11 changes: 2 additions & 9 deletions packages/astro/src/core/app/base.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
collapseDuplicateLeadingSlashes,
prependForwardSlash,
removeTrailingForwardSlash,
stripRequestBase,
} from '@astrojs/internal-helpers/path';
import { matchPattern } from '@astrojs/internal-helpers/remote';
import { computePathnameFromDomain } from '../i18n/domain.js';
Expand Down Expand Up @@ -281,14 +281,7 @@ 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);
}
return pathname;
return stripRequestBase(pathname, this.manifest.base);
}

/**
Expand Down
17 changes: 5 additions & 12 deletions packages/astro/src/core/fetch/fetch-state.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import colors from 'piccolore';
import {
collapseDuplicateLeadingSlashes,
collapseDuplicateSlashes,
prependForwardSlash,
removeTrailingForwardSlash,
stripRequestBase,
} from '@astrojs/internal-helpers/path';
import { deserializeActionResult } from '../../actions/runtime/client.js';
import { createCallAction, createGetActionResult, hasActionPayload } from '../../actions/utils.js';
Expand Down Expand Up @@ -1016,18 +1015,12 @@ export class FetchState implements AstroFetchState {
* Strips the manifest's base from a normalized request pathname and prepends
* a forward slash.
*
* Mirrors `BaseApp.removeBase`, including the
* `collapseDuplicateLeadingSlashes` fix that prevents middleware
* authorization bypass when the URL starts with `//`.
* Mirrors `BaseApp.removeBase`: the router matches against this stripped path
* while middleware reads the un-stripped `context.url.pathname`, so both must
* strip the base identically.
*/
#computePathname(normalizedPathname: string): string {
let pathname = collapseDuplicateLeadingSlashes(normalizedPathname);
const base = this.manifest.base;
if (pathname.startsWith(base)) {
const baseWithoutTrailingSlash = removeTrailingForwardSlash(base);
pathname = pathname.slice(baseWithoutTrailingSlash.length + 1);
}
return prependForwardSlash(pathname);
return prependForwardSlash(stripRequestBase(normalizedPathname, this.manifest.base));
}

/**
Expand Down
19 changes: 3 additions & 16 deletions packages/astro/src/core/i18n/domain.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import {
appendForwardSlash,
collapseDuplicateLeadingSlashes,
joinPaths,
prependForwardSlash,
removeTrailingForwardSlash,
stripRequestBase,
} from '@astrojs/internal-helpers/path';
import { normalizeTheLocale } from '../../i18n/path.js';
import type { SSRManifest } from '../app/types.js';
Expand Down Expand Up @@ -72,13 +72,13 @@ export function computePathnameFromDomain(
}

if (locale) {
const requestPathname = pathnameFromRequest ?? removeBase(url.pathname, base);
const requestPathname = pathnameFromRequest ?? stripRequestBase(url.pathname, base);
pathname = prependForwardSlash(joinPaths(normalizeTheLocale(locale), requestPathname));
if (trailingSlash === 'always') {
pathname = appendForwardSlash(pathname);
} else if (trailingSlash === 'never') {
pathname = removeTrailingForwardSlash(pathname);
} else if (requestPathname.endsWith('/')) {
} else if (url.pathname.endsWith('/')) {
// trailingSlash === 'ignore': preserve the original trailing slash
pathname = appendForwardSlash(pathname);
}
Expand All @@ -94,16 +94,3 @@ export function computePathnameFromDomain(
}
return pathname;
}

/**
* Mirror of `BaseApp.removeBase`, including the
* `collapseDuplicateLeadingSlashes` fix that prevents middleware
* authorization bypass when the URL starts with `//`.
*/
function removeBase(pathname: string, base: string): string {
pathname = collapseDuplicateLeadingSlashes(pathname);
if (pathname.startsWith(base)) {
return pathname.slice(removeTrailingForwardSlash(base).length + 1);
}
return pathname;
}
126 changes: 126 additions & 0 deletions packages/astro/test/units/app/base-prefix-boundary.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import assert from 'node:assert/strict';
import { describe, it } from 'node:test';
import type { MiddlewareHandler } from '../../../dist/types/public/common.js';
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, createRouteInfo } from './test-helpers.ts';

/**
* Tests that base stripping respects path-segment boundaries.
*
* With `base: '/app'`, only `/app` and paths under `/app/` belong to the base.
* Stripping the base with a plain prefix check treats `/appX/admin` as under the
* base and slices it to `/admin`, so the router matches `/admin` while
* `context.url.pathname` still reads `/appX/admin`. Middleware keyed on the
* base-prefixed pathname then sees a path it does not recognize.
*/

const routeOptions: Parameters<typeof parseRoute>[1] = {
config: { base: '/app', trailingSlash: 'ignore' },
pageExtensions: [],
} as any;

const adminRouteData = parseRoute('admin', routeOptions, {
component: 'src/pages/admin.astro',
});

const publicRouteData = parseRoute('index.astro', routeOptions, {
component: 'src/pages/index.astro',
});

const adminPage = createComponent((_result: any, _props: any, _slots: any) => {
return render`<h1>Admin Panel</h1>`;
});

const publicPage = createComponent((_result: any, _props: any, _slots: any) => {
return render`<h1>Public</h1>`;
});

const pageMap = new Map([
[
adminRouteData.component,
async () => ({
page: async () => ({
default: adminPage,
}),
}),
],
[
publicRouteData.component,
async () => ({
page: async () => ({
default: publicPage,
}),
}),
],
]);

/**
* Middleware that blocks access to the base-prefixed `/app/admin` route,
* as recommended in the official Astro authentication docs.
*/
function createAuthMiddleware() {
return (async () => ({
onRequest: (async (context, next) => {
const pathname = context.url.pathname;
if (pathname === '/app/admin' || pathname.startsWith('/app/admin/')) {
return new Response('Forbidden', { status: 403 });
}
return next();
}) satisfies MiddlewareHandler,
})) as () => Promise<{ onRequest: MiddlewareHandler }>;
}

function createApp(middleware: ReturnType<typeof createAuthMiddleware>) {
return new App(
createManifest({
base: '/app',
routes: [createRouteInfo(adminRouteData), createRouteInfo(publicRouteData)],
pageMap: pageMap as any,
Comment thread
matthewp marked this conversation as resolved.
Resolved
middleware: middleware as any,
}) as any,
);
}

describe('base stripping respects path-segment boundaries', () => {
it('middleware blocks /app/admin with normal request', async () => {
const app = createApp(createAuthMiddleware());
const request = new Request('http://example.com/app/admin');
const response = await app.render(request);
assert.equal(response.status, 403, '/app/admin should be blocked by middleware');
});

it('middleware blocks /appX/admin (single-character prefix extension)', async () => {
const app = createApp(createAuthMiddleware());
const request = new Request('http://example.com/appX/admin');
const response = await app.render(request);
assert.notEqual(
response.status,
200,
'/appX/admin should not resolve to the protected /admin route',
);
});

it('middleware blocks /app2/admin (digit prefix extension)', async () => {
const app = createApp(createAuthMiddleware());
const request = new Request('http://example.com/app2/admin');
const response = await app.render(request);
assert.notEqual(
response.status,
200,
'/app2/admin should not resolve to the protected /admin route',
);
});

it('middleware blocks /app-/admin (punctuation prefix extension)', async () => {
const app = createApp(createAuthMiddleware());
const request = new Request('http://example.com/app-/admin');
const response = await app.render(request);
assert.notEqual(
response.status,
200,
'/app-/admin should not resolve to the protected /admin route',
);
});
});
22 changes: 22 additions & 0 deletions packages/internal-helpers/src/path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,28 @@ export function removeBase(path: string, base: string) {
return path;
}

/**
* Strips a configured request `base` from a pathname, only when the pathname is
* the base itself or lies under it at a path-segment boundary. `stripRequestBase('/appX/admin', '/app')`
* leaves the pathname untouched, whereas `stripRequestBase('/app/admin', '/app')` returns `/admin`.
*
* Leading slashes are collapsed first so `//admin` cannot pose as being under a
* `/` base. Route matching and `context.url.pathname` derive from separate
* copies of the request path, so a mismatch here lets a request reach a route
* whose base-prefixed pathname middleware never observes.
*/
export function stripRequestBase(pathname: string, base: string): string {
pathname = collapseDuplicateLeadingSlashes(pathname);
const baseWithoutTrailingSlash = removeTrailingForwardSlash(base);
if (pathname === baseWithoutTrailingSlash) {
return '/';
}
if (pathname.startsWith(baseWithoutTrailingSlash + '/')) {
return pathname.slice(baseWithoutTrailingSlash.length);
}
return pathname;
}

const WITH_FILE_EXT = /\/[^/]+\.\w+$/;

export function hasFileExtension(path: string) {
Expand Down
Loading