From 49c4a620ca651227b97029852df31d8ab2db1595 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Thu, 3 Sep 2026 09:19:44 -0300 Subject: [PATCH] fix(authz): hard-gate every credential export and CLI-config write (GHSA-5926-2w35-7h4q) (#12600) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(authz): hard-gate every credential export and CLI-config write GHSA-5926-2w35-7h4q: `POST /api/providers/{id}/claude-auth/export` and `.../codex-auth/export` gate on `requireManagementAuth(request)` with no `alwaysRequireAuth`, and neither path was in ALWAYS_PROTECTED_API_PATHS. Under `requireLogin=false` — the local-first default — both fail open, so anyone who knows a connection id downloads the operator's raw Claude/Codex OAuth access_token / refresh_token (plus the Codex id_token). This is the third recurrence of one class. GHSA-mghq-58h3-qcqj added /api/db-backups; GHSA-v7g9-7f55-5g46 added the /api/settings/*-json siblings mghq had missed; these two are the siblings both missed. So the fix is written against the class, not the two reported routes. Sweeping every route that hands out stored credentials, dumps captured traffic, or writes the operator's CLI config turned up four more on the fail-open tier: - GET /api/logs/export — dumps call_logs (prompts and responses) and proxy_logs for up to 168h. - /api/cli-tools/codex-profiles — GET leaks the operator's account label; PUT writes attacker-supplied auth.json and config.toml straight into the host's Codex CLI config. Its only guard is ensureCliConfigWriteAllowed() with no targetPath, which checks CLI_ALLOW_CONFIG_WRITES — default true. Paired with the POST that stores an arbitrary profile, that is: save a profile holding the attacker's auth.json, apply it, and the operator's CLI now runs on attacker credentials (or, via config.toml, an attacker base URL). - {claude,codex}-auth/apply-local and providers/agy-auth/apply-local — write a stored credential into ~/.codex/auth.json and ~/.gemini/antigravity-cli/antigravity-oauth-token. The traffic-inspector HAR exports were already covered by LOCAL_ONLY. Routes with a dynamic segment cannot be expressed in the exact/prefix list — a `/api/providers/` prefix would hard-gate the whole provider surface and break every keyless install — so this adds ALWAYS_PROTECTED_API_PATTERNS, mirroring the existing LOCAL_ONLY_API_PATTERNS, and `isAlwaysProtectedPath` consults both. The apply-local routes get ALWAYS_PROTECTED rather than LOCAL_ONLY on purpose: it closes the anonymous hole without breaking an operator driving the dashboard through a tunnel. Deliberately NOT adding `{ alwaysRequireAuth: true }` at the handlers. Tier 2 is the architecture's designated mechanism and the guard runs before the handler; a second copy of the same decision inside each route is exactly the kind of duplicate that drifts out of sync (cf. the dashboardCsrf prefix scan that had to be unified in #11417). tests/unit/authz/credential-export-always-protected.test.ts — 5 tests, red before the fix. Written as an inventory of the whole class rather than two more assertions, plus negative cases: the neighbouring provider routes must stay on MANAGEMENT, and a connection id containing a slash must not slip past `[^/]+`. openapi.yaml marks the seven newly-gated operations `x-always-protected`, and openapi-security-tiers.test.ts now resolves `{param}` placeholders so it can validate the pattern entries too. Reported by @skeletonsec. Closes GHSA-5926-2w35-7h4q * chore(quality): register the credential-export authz test in stryker tap.testFiles The new tests/unit/authz/credential-export-always-protected.test.ts covers src/server/authz/routeGuard.ts, so check:mutation-test-coverage --strict fails until it is listed — its mutant kills would not count otherwise. Inserted in place (no re-serialization: a JSON round-trip on this file reorders ~10 curated entries that are already out of alphabetical order, cf. #11438). --- docs/openapi.yaml | 10 ++ src/server/authz/routeGuard.ts | 41 +++++- stryker.conf.json | 1 + ...credential-export-always-protected.test.ts | 132 ++++++++++++++++++ tests/unit/openapi-security-tiers.test.ts | 26 +++- 5 files changed, 203 insertions(+), 7 deletions(-) create mode 100644 tests/unit/authz/credential-export-always-protected.test.ts diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 2f83d88080..16a39eef8d 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -2297,6 +2297,7 @@ paths: post: tags: [Providers] summary: Auto-detect and import the local Antigravity CLI (agy) login from disk + x-always-protected: true responses: "200": description: Created or updated provider connection @@ -4050,12 +4051,14 @@ paths: get: tags: [CLI Tools] summary: Get Codex profiles + x-always-protected: true responses: "200": description: Codex profile list post: tags: [CLI Tools] summary: Create Codex profile + x-always-protected: true requestBody: required: true content: @@ -4068,6 +4071,7 @@ paths: put: tags: [CLI Tools] summary: Update Codex profile + x-always-protected: true requestBody: required: true content: @@ -4080,6 +4084,7 @@ paths: delete: tags: [CLI Tools] summary: Delete Codex profile + x-always-protected: true responses: "200": description: Profile deleted @@ -9751,6 +9756,7 @@ paths: tags: - Logs summary: "GET logs › export" + x-always-protected: true responses: "200": description: OK @@ -10230,6 +10236,7 @@ paths: tags: - Providers summary: "POST providers › › claude auth › apply local" + x-always-protected: true responses: "200": description: OK @@ -10238,6 +10245,7 @@ paths: tags: - Providers summary: "POST providers › › claude auth › export" + x-always-protected: true responses: "200": description: OK @@ -10246,6 +10254,7 @@ paths: tags: - Providers summary: "POST providers › › codex auth › apply local" + x-always-protected: true responses: "200": description: OK @@ -10254,6 +10263,7 @@ paths: tags: - Providers summary: "POST providers › › codex auth › export" + x-always-protected: true responses: "200": description: OK diff --git a/src/server/authz/routeGuard.ts b/src/server/authz/routeGuard.ts index 60a8d8e15a..b987f4fe49 100644 --- a/src/server/authz/routeGuard.ts +++ b/src/server/authz/routeGuard.ts @@ -140,6 +140,42 @@ export const ALWAYS_PROTECTED_API_PATHS: ReadonlyArray = [ // which is false under requireLogin=false. (GHSA-v7g9-7f55-5g46) "/api/settings/export-json", "/api/settings/import-json", + // Bulk log export: call_logs carries prompts and responses, proxy_logs carries + // client/public IPs, and the handler only calls requireManagementAuth() with no + // alwaysRequireAuth. Found sweeping the GHSA-5926-2w35-7h4q class. + "/api/logs/export", + // Codex CLI profile store. GET leaks the operator's account label; PUT writes + // attacker-supplied auth.json + config.toml straight into the operator's Codex + // CLI config (ensureCliConfigWriteAllowed() only checks CLI_ALLOW_CONFIG_WRITES, + // which defaults to true), so a POST+PUT pair repoints the CLI at attacker + // credentials or an attacker base URL. Found sweeping the same class. + "/api/cli-tools/codex-profiles", + // Writes into ~/.gemini/antigravity-cli/antigravity-oauth-token. Same family + // as the {claude,codex}-auth/apply-local pattern below; a plain path because + // it carries no dynamic segment. + "/api/providers/agy-auth/apply-local", +]; + +/** + * ALWAYS_PROTECTED routes whose path carries a dynamic segment, so the plain + * exact/prefix list above cannot express them: a `/api/providers/` prefix would + * hard-gate the entire provider surface and break every keyless local-first + * install. Mirrors LOCAL_ONLY_API_PATTERNS. + * + * The Claude/Codex OAuth export routes return the connection's raw + * access_token / refresh_token (and the Codex id_token) and gate only on + * `requireManagementAuth(request)` with no `alwaysRequireAuth`, which fails open + * under requireLogin=false (GHSA-5926-2w35-7h4q). They are the siblings that + * both GHSA-mghq-58h3-qcqj and GHSA-v7g9-7f55-5g46 missed. + */ +export const ALWAYS_PROTECTED_API_PATTERNS: ReadonlyArray = [ + // `export` hands the caller the raw token; `apply-local` writes it into the + // host's CLI config (~/.codex/auth.json and the Claude equivalent). The second + // does not disclose the credential, but "anonymous" is still the wrong + // audience for it. ALWAYS_PROTECTED rather than LOCAL_ONLY on purpose: it + // closes the anonymous hole without breaking an operator driving the dashboard + // through a tunnel. + /^\/api\/providers\/[^/]+\/(claude|codex)-auth\/(export|apply-local)\/?$/, ]; export function isLoopbackHost(hostHeader: string | null): boolean { @@ -295,5 +331,8 @@ export function isLocalOnlyBypassableByManageScope(path: string): boolean { } export function isAlwaysProtectedPath(path: string): boolean { - return ALWAYS_PROTECTED_API_PATHS.some((p) => path === p || path.startsWith(p)); + return ( + ALWAYS_PROTECTED_API_PATHS.some((p) => path === p || path.startsWith(p)) || + ALWAYS_PROTECTED_API_PATTERNS.some((re) => re.test(path)) + ); } diff --git a/stryker.conf.json b/stryker.conf.json index d13bf2c1d4..23dd4fb2d5 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -90,6 +90,7 @@ "tests/unit/auth-opencode-zen-noauth-fallback.test.ts", "tests/unit/auth-passthrough-per-model-402-12242.test.ts", "tests/unit/auth-terminal-status.test.ts", + "tests/unit/authz/credential-export-always-protected.test.ts", "tests/unit/authz/discovery-routes-local-only.test.ts", "tests/unit/authz/oauth-autoimport-local-only.test.ts", "tests/unit/authz/route-guard-local-prefix.test.ts", diff --git a/tests/unit/authz/credential-export-always-protected.test.ts b/tests/unit/authz/credential-export-always-protected.test.ts new file mode 100644 index 0000000000..c297bad003 --- /dev/null +++ b/tests/unit/authz/credential-export-always-protected.test.ts @@ -0,0 +1,132 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { + isAlwaysProtectedPath, + isLocalOnlyPath, + ALWAYS_PROTECTED_API_PATHS, +} from "../../../src/server/authz/routeGuard.ts"; + +// GHSA-5926-2w35-7h4q — the Claude/Codex OAuth export routes gate on +// `requireManagementAuth(request)` with no `alwaysRequireAuth`, which fails open +// under requireLogin=false, and neither path was in ALWAYS_PROTECTED_API_PATHS. +// An unauthenticated caller who knows a connection id could download the +// operator's raw access_token / refresh_token / id_token. +// +// This is the THIRD recurrence of one class: GHSA-mghq-58h3-qcqj added +// /api/db-backups, GHSA-v7g9-7f55-5g46 added the /api/settings/*-json siblings +// it had missed, and this one is the siblings BOTH missed. So the test is +// written as an inventory of the whole class rather than two more assertions: +// a route that hands out stored credentials, dumps captured traffic, or writes +// the operator's CLI config must be hard-gated (ALWAYS_PROTECTED or +// LOCAL_ONLY), never left on the fail-open MANAGEMENT tier. + +const HARD_GATED_INVENTORY: ReadonlyArray<{ path: string; why: string }> = [ + // ── Reported in GHSA-5926-2w35-7h4q ────────────────────────────────────── + { + path: "/api/providers/6f3c1b7e-0000-4000-8000-000000000000/claude-auth/export", + why: "returns the connection's raw Claude OAuth access_token/refresh_token", + }, + { + path: "/api/providers/6f3c1b7e-0000-4000-8000-000000000000/codex-auth/export", + why: "returns the connection's raw Codex access_token/refresh_token/id_token", + }, + // ── Found sweeping the class while fixing the above ────────────────────── + { + path: "/api/logs/export", + why: "dumps call_logs (prompts and responses) and proxy_logs for up to 168h", + }, + { + path: "/api/cli-tools/codex-profiles", + why: "PUT writes attacker-supplied auth.json and config.toml into the operator's Codex CLI config", + }, + // ── Same family: WRITE the operator's credentials into host CLI files ─── + // These do not hand the credential to the caller, so they are a step below + // the export routes — but anonymous is still the wrong audience for "write + // this connection's token into ~/.codex/auth.json". ALWAYS_PROTECTED rather + // than LOCAL_ONLY on purpose: it closes the anonymous hole without breaking + // an operator driving the dashboard through a tunnel. + { + path: "/api/providers/6f3c1b7e-0000-4000-8000-000000000000/codex-auth/apply-local", + why: "writes the connection's credential into the host's ~/.codex/auth.json", + }, + { + path: "/api/providers/6f3c1b7e-0000-4000-8000-000000000000/claude-auth/apply-local", + why: "writes the connection's credential into the host's Claude CLI config", + }, + { + path: "/api/providers/agy-auth/apply-local", + why: "writes into ~/.gemini/antigravity-cli/antigravity-oauth-token", + }, + // ── Already fixed; pinned so a refactor cannot silently drop them ──────── + { path: "/api/db-backups/export", why: "GHSA-mghq-58h3-qcqj" }, + { path: "/api/db-backups/exportAll", why: "GHSA-mghq-58h3-qcqj" }, + { path: "/api/settings/export-json", why: "GHSA-v7g9-7f55-5g46" }, + { path: "/api/settings/import-json", why: "GHSA-v7g9-7f55-5g46" }, + { path: "/api/settings/database", why: "irreversible database replace" }, + { path: "/api/shutdown", why: "stops the server" }, + // ── Hard-gated by the LOCAL_ONLY tier instead ──────────────────────────── + { + path: "/api/tools/traffic-inspector/export.har", + why: "captured traffic can contain Authorization headers (LOCAL_ONLY)", + }, + { + path: "/api/tools/traffic-inspector/sessions/abc/export.har", + why: "same, per session (LOCAL_ONLY)", + }, +]; + +test("every credential/traffic export and CLI-config write is hard-gated", () => { + for (const { path, why } of HARD_GATED_INVENTORY) { + const gated = isAlwaysProtectedPath(path) || isLocalOnlyPath(path); + assert.ok( + gated, + `${path} is on the fail-open MANAGEMENT tier — anonymous under requireLogin=false. ${why}` + ); + } +}); + +test("the trailing-slash spelling is gated too", () => { + for (const path of [ + "/api/providers/abc/claude-auth/export/", + "/api/providers/abc/codex-auth/export/", + "/api/logs/export/", + "/api/cli-tools/codex-profiles/", + ]) { + assert.ok(isAlwaysProtectedPath(path) || isLocalOnlyPath(path), path); + } +}); + +test("the new patterns do not over-protect their neighbours", () => { + // The dynamic-segment entries must not swallow the rest of /api/providers/, + // which is ordinary MANAGEMENT and has to keep working under requireLogin=false. + for (const path of [ + "/api/providers", + "/api/providers/abc", + "/api/providers/abc/models", + "/api/providers/abc/claude-auth", + "/api/providers/abc/codex-auth", + "/api/providers/abc/claude-auth/apply", + "/api/providers/agy-auth", + "/api/logs", + "/api/cli-tools", + ]) { + assert.equal( + isAlwaysProtectedPath(path), + false, + `${path} must stay on the MANAGEMENT tier — hard-gating it breaks keyless local-first installs` + ); + } +}); + +test("a connection id cannot escape the pattern with a slash", () => { + // `[^/]+` is deliberate: a traversal-ish id must not match and silently drop + // back to the fail-open tier by looking like a different route. + assert.equal(isAlwaysProtectedPath("/api/providers/a/b/claude-auth/export"), false); +}); + +test("the plain-path allowlist keeps its existing entries", () => { + for (const p of ["/api/shutdown", "/api/settings/database", "/api/db-backups"]) { + assert.ok(ALWAYS_PROTECTED_API_PATHS.includes(p), p); + } +}); diff --git a/tests/unit/openapi-security-tiers.test.ts b/tests/unit/openapi-security-tiers.test.ts index 8203ab6f2d..d1380a5c08 100644 --- a/tests/unit/openapi-security-tiers.test.ts +++ b/tests/unit/openapi-security-tiers.test.ts @@ -7,8 +7,12 @@ import * as yaml from "js-yaml"; const ROOT = process.cwd(); const OPENAPI_PATH = path.join(ROOT, "docs", "openapi.yaml"); -const { LOCAL_ONLY_API_PREFIXES, LOCAL_ONLY_API_PATTERNS, ALWAYS_PROTECTED_API_PATHS } = - await import("../../src/server/authz/routeGuard.ts"); +const { + LOCAL_ONLY_API_PREFIXES, + LOCAL_ONLY_API_PATTERNS, + ALWAYS_PROTECTED_API_PATHS, + ALWAYS_PROTECTED_API_PATTERNS, +} = await import("../../src/server/authz/routeGuard.ts"); const raw: any = yaml.load(fs.readFileSync(OPENAPI_PATH, "utf-8")); const paths: Record = raw.paths || {}; @@ -131,12 +135,22 @@ test("every x-always-protected path matches ALWAYS_PROTECTED_API_PATHS in routeG for (const [method, spec] of Object.entries(methods as Record)) { if (!["get", "post", "put", "patch", "delete"].includes(method)) continue; if (spec?.["x-always-protected"] !== true) continue; - const matchesPath = (ALWAYS_PROTECTED_API_PATHS as ReadonlyArray).some( - (p: string) => pathStr === p || pathStr.startsWith(`${p}/`) - ); + // Routes with a dynamic segment cannot be expressed in the plain + // exact/prefix list, so routeGuard also carries ALWAYS_PROTECTED_API_PATTERNS + // (GHSA-5926-2w35-7h4q). Substitute a concrete value for the OpenAPI + // `{param}` placeholders before testing those. + const concretePath = pathStr.replace(/\{[^}]+\}/g, "sample-id"); + const matchesPath = + (ALWAYS_PROTECTED_API_PATHS as ReadonlyArray).some( + (p: string) => pathStr === p || pathStr.startsWith(`${p}/`) + ) || + (ALWAYS_PROTECTED_API_PATTERNS as ReadonlyArray).some((re) => + re.test(concretePath) + ); assert.ok( matchesPath, - `YAML path "${pathStr}" ${method.toUpperCase()} has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS. ` + + `YAML path "${pathStr}" ${method.toUpperCase()} has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS ` + + `nor matched by ALWAYS_PROTECTED_API_PATTERNS. ` + `Entries: ${(ALWAYS_PROTECTED_API_PATHS as ReadonlyArray).join(", ")}` ); }