diff --git a/docs/architecture/AUTHZ_GUIDE.md b/docs/architecture/AUTHZ_GUIDE.md index c12efe8c4c..911b1bd72c 100644 --- a/docs/architecture/AUTHZ_GUIDE.md +++ b/docs/architecture/AUTHZ_GUIDE.md @@ -108,24 +108,48 @@ A successful policy returns `AuthSubject` with `kind ∈ { client_api_key, dashb `src/shared/constants/publicApiRoutes.ts` is the explicit allowlist: +The list is split by **shape**, and the split is load-bearing (GHSA-74g9-q8f6-793h): a prefix is +matched with `startsWith()`, so it also matches every adjacent path sharing its leading characters. +`/api/usage/om-usage` as a prefix marked `/api/usage/om-usage` PUBLIC, and Next resolves +that to `/api/usage/[connectionId]` — a handler with no auth of its own. + ```ts +// Genuine subtrees. Every entry MUST end in "/" (asserted by a unit test). PUBLIC_API_ROUTE_PREFIXES = [ + "/api/auth/oidc/", + "/api/v1/", // treated as CLIENT_API in classify, not as "no-auth public" + "/api/oauth/", + "/api/codex/connect/", + "/api/telegram/", + "/api/cursor-cli/", +]; + +// Single routes, matched EXACTLY (with or without a trailing slash). +PUBLIC_API_ROUTES_EXACT = new Set([ "/api/auth/login", "/api/auth/logout", "/api/auth/status", "/api/init", - "/api/v1/", // treated as CLIENT_API in classify, not as "no-auth public" - "/api/cloud/", "/api/sync/bundle", - "/api/oauth/", + "/api/cli/connect", + "/api/usage/om-usage", + "/api/skills/collect/chaos", +]); + +// Read-only single routes that also take the CORS origin relaxation. +PUBLIC_READONLY_CORS_API_ROUTES = [ + "/api/health/ping", + "/api/monitoring/health", + "/api/settings/require-login", ]; -PUBLIC_READONLY_API_ROUTE_PREFIXES = ["/api/monitoring/health", "/api/settings/require-login"]; +// Read-only single route WITHOUT the CORS relaxation. +PUBLIC_READONLY_API_ROUTES_EXACT = new Set(["/api/health"]); PUBLIC_READONLY_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); ``` -Read-only prefixes are public **only** for safe methods. Note: `classifyRoute()` excludes `/api/v1/*` and `/api/v1beta/*` from the PUBLIC fall-through — those are always `CLIENT_API` so the Bearer-key policy still applies. +Read-only routes are public **only** for safe methods. Note: `classifyRoute()` excludes `/api/v1/*` and `/api/v1beta/*` from the PUBLIC fall-through — those are always `CLIENT_API` so the Bearer-key policy still applies. ## Adding a New Route @@ -168,7 +192,7 @@ export async function POST(request: Request) { ### Pattern 3 — Adding to the public allowlist -Add the prefix to `PUBLIC_API_ROUTE_PREFIXES` (or `PUBLIC_READONLY_API_ROUTE_PREFIXES` for GET-only). Update unit tests at `tests/unit/public-api-routes.test.ts` and `tests/unit/authz/classify.test.ts`. +Pick the set by shape, not by convenience. One route goes in `PUBLIC_API_ROUTES_EXACT` (or `PUBLIC_READONLY_CORS_API_ROUTES` for GET-only); only a genuine subtree goes in `PUBLIC_API_ROUTE_PREFIXES`, and it **must end in `/`**. Putting a single route in the prefix list also publishes every adjacent path that shares its leading characters — including dynamic-segment siblings added later (GHSA-74g9-q8f6-793h). Update unit tests at `tests/unit/public-api-routes.test.ts`, `tests/unit/authz/public-route-exact-match.test.ts` and `tests/unit/authz/classify.test.ts`. ## Scopes diff --git a/src/server/authz/classify.ts b/src/server/authz/classify.ts index bfe0f0d6f9..6bab3a05ae 100644 --- a/src/server/authz/classify.ts +++ b/src/server/authz/classify.ts @@ -1,7 +1,6 @@ import { - PUBLIC_READONLY_API_ROUTE_PREFIXES, - PUBLIC_READONLY_METHODS, isPublicApiRoute, + isPublicReadonlyCorsRoute, } from "../../shared/constants/publicApiRoutes"; import type { ClassificationReason, RouteClassification } from "./types"; @@ -135,8 +134,9 @@ export function classifyRoute(rawPath: string, method: string = "GET"): RouteCla } function matchesReadonlyPublic(path: string, method: string): boolean { - if (!PUBLIC_READONLY_METHODS.has(String(method).toUpperCase())) return false; - return PUBLIC_READONLY_API_ROUTE_PREFIXES.some((p) => path.startsWith(p)); + // Exact match, not startsWith: a prefix here would hand the CORS origin + // relaxation to every adjacent path too (GHSA-74g9-q8f6-793h). + return isPublicReadonlyCorsRoute(path, method); } function isClassifiedAsPublic(path: string, method: string): boolean { diff --git a/src/shared/constants/publicApiRoutes.ts b/src/shared/constants/publicApiRoutes.ts index b34e130acd..753106045e 100644 --- a/src/shared/constants/publicApiRoutes.ts +++ b/src/shared/constants/publicApiRoutes.ts @@ -1,30 +1,27 @@ +// Public API surface, split by SHAPE — this file is matched two different ways +// and the distinction is load-bearing (GHSA-74g9-q8f6-793h). +// +// A prefix is matched with `startsWith()`, so it also matches every adjacent +// path that merely shares its leading characters. `/api/usage/om-usage` as a +// prefix marked `/api/usage/om-usage` PUBLIC — and Next resolves that +// to the dynamic route `/api/usage/[connectionId]`, whose handler carries no +// auth of its own because it relies on being classified MANAGEMENT. Ten other +// entries had no shadowing sibling in the route tree today, but any route added +// later under a dynamic segment adjacent to one of them would inherit the same +// bypass silently. +// +// So: PREFIXES are genuine subtrees and MUST end in "/" (asserted by +// tests/unit/authz/public-route-exact-match.test.ts); single routes live in an +// EXACT set instead. + +// Genuine subtrees. Every entry MUST end in "/". const PUBLIC_API_ROUTE_PREFIXES = [ - "/api/auth/login", - "/api/auth/logout", - "/api/auth/status", "/api/auth/oidc/", - "/api/init", "/api/v1/", - "/api/sync/bundle", "/api/oauth/", // Public, ticket-gated Codex device-flow completion (validate + persist). // The handler enforces its own single-use ticket check; no dashboard auth. "/api/codex/connect/", - // Remote-mode bootstrap: exchange the management password for a scoped CLI - // access token. The handler enforces its own password check + lockout — there - // is no token yet at this point, so it cannot require management auth. - "/api/cli/connect", - // Terminal-friendly @@om-usage equivalent for CLI clients (Claude Code/Codex). - // The handler enforces its own auth via extractUsageCommandApiKey/isValidApiKey - // and the allowUsageCommand flag — it must not be gated by management auth. - "/api/usage/om-usage", - // Chaos Mode external dispatch endpoint (POST /api/skills/collect/chaos). - // This entry only bypasses the dashboard requireLogin (cookie) gate — the - // handler enforces its own Bearer-token auth (validateApiKey + - // chaosModeEnabled check) before doing any work. See src/app/api/skills/ - // collect/chaos/route.ts. Do not widen this prefix to cover other - // /api/skills/collect/* routes without the same per-handler auth. - "/api/skills/collect/chaos", // Telegram Bot API update webhook + Mini App proxy. Telegram POSTs updates // here without any dashboard cookie/API key; the handler enforces its own // auth (503 when TELEGRAM_BOT_TOKEN is unset; 401 on invalid initData @@ -38,18 +35,45 @@ const PUBLIC_API_ROUTE_PREFIXES = [ "/api/cursor-cli/", ]; -const PUBLIC_READONLY_API_ROUTE_PREFIXES = [ +// Single routes, public by EXACT path (both spellings) — never by prefix. +const PUBLIC_API_ROUTES_EXACT = new Set([ + "/api/auth/login", + "/api/auth/logout", + "/api/auth/status", + "/api/init", + "/api/sync/bundle", + // Remote-mode bootstrap: exchange the management password for a scoped CLI + // access token. The handler enforces its own password check + lockout — there + // is no token yet at this point, so it cannot require management auth. + "/api/cli/connect", + // Terminal-friendly @@om-usage equivalent for CLI clients (Claude Code/Codex). + // The handler enforces its own auth via extractUsageCommandApiKey/isValidApiKey + // and the allowUsageCommand flag — it must not be gated by management auth. + // EXACT: the sibling `/api/usage/[connectionId]` has no auth of its own. + "/api/usage/om-usage", + // Chaos Mode external dispatch endpoint (POST /api/skills/collect/chaos). + // This entry only bypasses the dashboard requireLogin (cookie) gate — the + // handler enforces its own Bearer-token auth (validateApiKey + + // chaosModeEnabled check) before doing any work. See src/app/api/skills/ + // collect/chaos/route.ts. Do not widen it to other /api/skills/collect/* + // routes without the same per-handler auth. + "/api/skills/collect/chaos", +]); + +// Read-only single routes that ALSO take the CORS origin relaxation: they +// classify as `public_readonly_prefix`, which authz/pipeline.ts keys on. +const PUBLIC_READONLY_CORS_API_ROUTES = [ "/api/health/ping", "/api/monitoring/health", "/api/settings/require-login", ]; -// Read-only routes public by EXACT path, never by prefix. +// Read-only routes public by EXACT path, WITHOUT the CORS relaxation. // // `/api/health` has to be reachable without a key — a probe has none, and a 401 there is -// indistinguishable from a wrong key or a missing route. It cannot go in the prefix list -// above: `startsWith("/api/health")` would also expose `/api/health/degradation`, which is -// authenticated today. +// indistinguishable from a wrong key or a missing route. It stays in its own set (rather than +// joining PUBLIC_READONLY_CORS_API_ROUTES) so it keeps classifying as `public_prefix`: moving it +// would silently widen CORS on it. const PUBLIC_READONLY_API_ROUTES_EXACT = new Set(["/api/health"]); const PUBLIC_READONLY_METHODS = new Set(["GET", "HEAD", "OPTIONS"]); @@ -64,6 +88,13 @@ function pathMatchesExactRoute(pathname: string, routePath: string): boolean { return pathname === routePath || pathname === `${routePath}/`; } +function matchesAnyExactRoute(pathname: string, routes: Iterable): boolean { + for (const route of routes) { + if (pathMatchesExactRoute(pathname, route)) return true; + } + return false; +} + function isPublicCloudApiRoute(pathname: string, method: string): boolean { const normalizedMethod = String(method).toUpperCase(); return PUBLIC_CLOUD_API_ROUTES.some( @@ -82,6 +113,17 @@ const LOCAL_ONLY_OAUTH_IMPORT_ROUTES = [ "/api/oauth/raycast/auto-import", ]; +/** + * Whether the route classifies as read-only PUBLIC *with* the CORS origin + * relaxation (authz/classify.ts reason `public_readonly_prefix`). Exported as a + * predicate rather than as the raw list so a caller cannot reintroduce the + * prefix match this file exists to prevent. + */ +export function isPublicReadonlyCorsRoute(pathname: string, method = "GET"): boolean { + if (!PUBLIC_READONLY_METHODS.has(String(method).toUpperCase())) return false; + return matchesAnyExactRoute(pathname, PUBLIC_READONLY_CORS_API_ROUTES); +} + export function isPublicApiRoute(pathname: string, method = "GET"): boolean { if ( LOCAL_ONLY_OAUTH_IMPORT_ROUTES.some( @@ -95,6 +137,10 @@ export function isPublicApiRoute(pathname: string, method = "GET"): boolean { return true; } + if (matchesAnyExactRoute(pathname, PUBLIC_API_ROUTES_EXACT)) { + return true; + } + if (PUBLIC_API_ROUTE_PREFIXES.some((route) => pathname.startsWith(route))) { return true; } @@ -103,18 +149,17 @@ export function isPublicApiRoute(pathname: string, method = "GET"): boolean { return false; } - for (const route of PUBLIC_READONLY_API_ROUTES_EXACT) { - if (pathMatchesExactRoute(pathname, route)) { - return true; - } + if (matchesAnyExactRoute(pathname, PUBLIC_READONLY_API_ROUTES_EXACT)) { + return true; } - return PUBLIC_READONLY_API_ROUTE_PREFIXES.some((route) => pathname.startsWith(route)); + return isPublicReadonlyCorsRoute(pathname, method); } export { PUBLIC_API_ROUTE_PREFIXES, - PUBLIC_READONLY_API_ROUTE_PREFIXES, + PUBLIC_API_ROUTES_EXACT, + PUBLIC_READONLY_CORS_API_ROUTES, PUBLIC_READONLY_API_ROUTES_EXACT, PUBLIC_READONLY_METHODS, }; diff --git a/src/shared/utils/dashboardCsrf.ts b/src/shared/utils/dashboardCsrf.ts index 872f09644d..226f33f7f5 100644 --- a/src/shared/utils/dashboardCsrf.ts +++ b/src/shared/utils/dashboardCsrf.ts @@ -1,5 +1,5 @@ import { DASHBOARD_CSRF_HEADER } from "@/shared/constants/dashboardCsrf"; -import { PUBLIC_API_ROUTE_PREFIXES } from "@/shared/constants/publicApiRoutes"; +import { isPublicApiRoute } from "@/shared/constants/publicApiRoutes"; interface CachedDashboardCsrfToken { token: string; @@ -113,11 +113,7 @@ function isClientApiPath(pathname: string): boolean { ); } -function isPublicApiPath(pathname: string): boolean { - return PUBLIC_API_ROUTE_PREFIXES.some((prefix) => pathname.startsWith(prefix)); -} - -function shouldAttachDashboardCsrf(url: URL): boolean { +function shouldAttachDashboardCsrf(url: URL, method: string): boolean { if ( TOP_LEVEL_MANAGEMENT_PATH_PREFIXES.some( (prefix) => url.pathname === prefix || url.pathname.startsWith(prefix + "/") @@ -129,7 +125,10 @@ function shouldAttachDashboardCsrf(url: URL): boolean { return ( url.pathname.startsWith("/api/") && url.pathname !== "/api/auth/csrf" && - !isPublicApiPath(url.pathname) && + // Share the server's PUBLIC classification instead of re-scanning the + // prefix list here — a second copy is a second chance to disagree with the + // authz pipeline (GHSA-74g9-q8f6-793h). + !isPublicApiRoute(url.pathname, method) && !isClientApiPath(url.pathname) ); } @@ -150,7 +149,7 @@ function sameOriginDashboardMutation(input: RequestInfo | URL, init?: RequestIni return false; } - return url.origin === window.location.origin && shouldAttachDashboardCsrf(url); + return url.origin === window.location.origin && shouldAttachDashboardCsrf(url, method); } function mergedHeaders(input: RequestInfo | URL, init?: RequestInit): Headers { diff --git a/tests/unit/authz/public-route-exact-match.test.ts b/tests/unit/authz/public-route-exact-match.test.ts new file mode 100644 index 0000000000..63cf5a2ef8 --- /dev/null +++ b/tests/unit/authz/public-route-exact-match.test.ts @@ -0,0 +1,113 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + PUBLIC_API_ROUTE_PREFIXES, + PUBLIC_API_ROUTES_EXACT, + PUBLIC_READONLY_API_ROUTES_EXACT, + PUBLIC_READONLY_CORS_API_ROUTES, + isPublicApiRoute, +} from "../../../src/shared/constants/publicApiRoutes.ts"; +import { classifyRoute } from "../../../src/server/authz/classify.ts"; + +// GHSA-74g9-q8f6-793h — `isPublicApiRoute()` matched every entry of +// PUBLIC_API_ROUTE_PREFIXES with startsWith(), but most entries name ONE exact +// route, not a subtree. As prefixes they also marked every adjacent path +// sharing the same leading characters as PUBLIC, skipping the MANAGEMENT auth +// gate. `/api/usage/om-usage` resolves to the dynamic route +// `/api/usage/[connectionId]`, whose handler carries no auth of its own. + +test("every prefix entry is a genuine subtree (ends in a slash)", () => { + for (const prefix of PUBLIC_API_ROUTE_PREFIXES) { + assert.equal( + prefix.endsWith("/"), + true, + `${prefix} is matched with startsWith(): a prefix that does not end in "/" also ` + + `matches every adjacent path sharing its leading characters (GHSA-74g9-q8f6-793h)` + ); + } +}); + +test("exact public routes stay public in both spellings", () => { + for (const route of PUBLIC_API_ROUTES_EXACT) { + assert.equal(isPublicApiRoute(route, "POST"), true, route); + assert.equal(isPublicApiRoute(`${route}/`, "POST"), true, `${route}/`); + } + for (const route of [...PUBLIC_READONLY_API_ROUTES_EXACT, ...PUBLIC_READONLY_CORS_API_ROUTES]) { + assert.equal(isPublicApiRoute(route, "GET"), true, route); + assert.equal(isPublicApiRoute(`${route}/`, "GET"), true, `${route}/`); + } +}); + +test("sibling paths shadowed by an exact route are NOT public", () => { + const shadowed = [ + "/api/auth/login-as", + "/api/auth/logout-all", + "/api/auth/status-page", + "/api/init-db", + "/api/sync/bundle-export", + "/api/cli/connect-token", + "/api/usage/om-usage-x", + "/api/usage/om-usageZZZ", + "/api/skills/collect/chaos-report", + "/api/health/pings", + "/api/monitoring/health-detail", + "/api/settings/require-login-policy", + ]; + for (const path of shadowed) { + assert.equal(isPublicApiRoute(path, "GET"), false, `${path} (GET)`); + assert.equal(isPublicApiRoute(path, "POST"), false, `${path} (POST)`); + } +}); + +test("the reported bypass: /api/usage/om-usage classifies MANAGEMENT", () => { + // The live one — Next resolves it to /api/usage/[connectionId], a handler + // with no auth of its own that reaches fetchAndPersistProviderLimits(). + assert.equal(classifyRoute("/api/usage/om-usage-x", "GET").routeClass, "MANAGEMENT"); + assert.equal(classifyRoute("/api/usage/om-usageZZZ", "GET").routeClass, "MANAGEMENT"); + // The real CLI route keeps its PUBLIC classification (it enforces its own key). + assert.equal(classifyRoute("/api/usage/om-usage", "GET").routeClass, "PUBLIC"); + assert.equal(classifyRoute("/api/usage/om-usage/", "GET").routeClass, "PUBLIC"); +}); + +test("genuine subtrees stay public all the way down", () => { + assert.equal(isPublicApiRoute("/api/v1/chat/completions", "POST"), true); + assert.equal(isPublicApiRoute("/api/oauth/cursor/callback", "GET"), true); + assert.equal(isPublicApiRoute("/api/auth/oidc/callback", "GET"), true); + assert.equal(isPublicApiRoute("/api/codex/connect/complete", "POST"), true); + assert.equal(isPublicApiRoute("/api/telegram/update", "POST"), true); + assert.equal(isPublicApiRoute("/api/cursor-cli/auth/exchange_user_api_key", "POST"), true); +}); + +test("read-only method gate is unchanged", () => { + for (const route of [...PUBLIC_READONLY_API_ROUTES_EXACT, ...PUBLIC_READONLY_CORS_API_ROUTES]) { + assert.equal(isPublicApiRoute(route, "GET"), true, `${route} GET`); + assert.equal(isPublicApiRoute(route, "HEAD"), true, `${route} HEAD`); + assert.equal(isPublicApiRoute(route, "OPTIONS"), true, `${route} OPTIONS`); + assert.equal(isPublicApiRoute(route, "POST"), false, `${route} POST`); + assert.equal(isPublicApiRoute(route, "DELETE"), false, `${route} DELETE`); + } +}); + +test("CORS relaxation reason set is unchanged", () => { + // pipeline.ts keys its CORS origin relaxation off `public_readonly_prefix`. + for (const route of PUBLIC_READONLY_CORS_API_ROUTES) { + assert.equal(classifyRoute(route, "GET").reason, "public_readonly_prefix", route); + } + // /api/health deliberately stays `public_prefix` — folding it into the + // read-only set would silently widen CORS on it. + assert.equal(classifyRoute("/api/health", "GET").reason, "public_prefix"); + // ...and a shadowed sibling must not inherit the relaxation either. + assert.equal(classifyRoute("/api/monitoring/health-detail", "GET").routeClass, "MANAGEMENT"); +}); + +test("LOCAL_ONLY oauth auto-import exclusions still win over the /api/oauth/ subtree", () => { + for (const route of [ + "/api/oauth/cursor/auto-import", + "/api/oauth/kiro/auto-import", + "/api/oauth/raycast/auto-import", + ]) { + assert.equal(isPublicApiRoute(route, "POST"), false, route); + assert.equal(classifyRoute(route, "POST").routeClass, "MANAGEMENT", route); + } +});