From 60829241fd64d0317aa6a0dd8cd7a445a5287fed Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Fri, 21 Aug 2026 13:57:42 -0300 Subject: [PATCH] fix(security): close 4 STILL-REAL advisory findings (ACP RCE hardening, db-backups tier, uppercase authz bypass, spawn-veto drift) (#11028) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⭐5 — 4 achados STILL-REAL de advisories de segurança, cada um com TDD (RED→GREEN) e crédito ao reporter original: ACP RCE hardening (resolveVersionProbe), db-backups Tier-2 allowlist, uppercase authz bypass (matcher case-insensitive), spawn-veto drift (chatgpt-web-codex-doctor). typecheck/lint limpos, suíte authz/acp/cors verde. UNSTABLE é o base-red inherited #9985, já documentado no corpo da PR. --- src/lib/acp/registry.ts | 14 ++++ src/proxy.ts | 28 +++++--- src/server/authz/classify.ts | 21 ++++-- src/server/authz/routeGuard.ts | 5 ++ src/shared/constants/spawnCapablePrefixes.ts | 1 + tests/unit/acp-agents-route.test.ts | 29 ++++++++ tests/unit/acp-registry.test.ts | 35 +++++++++ tests/unit/authz/proxy-contract.test.ts | 16 +++-- tests/unit/authz/proxy-matcher-case.test.ts | 76 ++++++++++++++++++++ tests/unit/authz/routeGuard.test.ts | 31 ++++++++ 10 files changed, 234 insertions(+), 22 deletions(-) create mode 100644 tests/unit/authz/proxy-matcher-case.test.ts diff --git a/src/lib/acp/registry.ts b/src/lib/acp/registry.ts index 93315a546d..07408f6236 100644 --- a/src/lib/acp/registry.ts +++ b/src/lib/acp/registry.ts @@ -200,6 +200,14 @@ let _customAgentDefs: CustomAgentDef[] = []; const DISALLOWED_VERSION_COMMAND_CHARS = /[;&|<>`$\r\n]/; +// A version probe only ever needs a version flag. For untrusted (client-registered) +// custom agents the binary-match check alone is not enough: the caller controls both +// `binary` and `versionCommand`, so a matching interpreter with an eval-style argument +// (`node -e …`, `python -c …`, `ruby -e …`) reaches execFileSync as arbitrary code +// execution without any shell metacharacter. Restricting the args to a recognized +// version flag closes that path — see GHSA-jphr-2gw7-xrwp / GHSA-hf57-cqmx-p4gr. +const SAFE_VERSION_PROBE_ARG = /^(-v|-V|--version|-version|version|--ver)$/; + /** * Set custom agent definitions from settings. */ @@ -300,6 +308,12 @@ export function resolveVersionProbe( if (!allowed.has(normalizedCommand)) { return null; } + + // Untrusted probe: allow only a bare binary or a single recognized version + // flag, so a matching interpreter cannot smuggle an eval/exec argument. + if (args.length > 1 || (args.length === 1 && !SAFE_VERSION_PROBE_ARG.test(args[0]))) { + return null; + } } return { command, args }; diff --git a/src/proxy.ts b/src/proxy.ts index 153785efdf..b99a0789d9 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -24,6 +24,14 @@ export async function proxy(request: NextRequest) { return runAuthzPipeline(request, { enforce: true }); } +// Next compiles the middleware/proxy matcher from `regexp.source` only, dropping +// path-to-regexp's default case-insensitive flag — so a lowercase literal like +// `/v1/:path*` never matches `/V1/...`, while the rewrite matcher (flag kept) +// still routes it to the handler. That skipped the authz pipeline entirely +// (GHSA-jvqc-mp9f-q936). Expressing the case-insensitivity inside a custom +// path-to-regexp group (`([vV]1)`) survives the flag-drop because it needs no +// flag. Keep these in sync with the client-API aliases in +// next.config.mjs rewrites and src/server/authz/classify.ts. export const config = { matcher: [ "/", @@ -31,15 +39,15 @@ export const config = { "/home", "/home/:path*", "/api/:path*", - "/v1/:path*", - "/v1", - "/v1beta/:path*", - "/v1beta", - "/chat/:path*", - "/responses/:path*", - "/responses", - "/codex/:path*", - "/codex", - "/models", + "/:v1seg([vV]1)/:path*", + "/:v1seg([vV]1)", + "/:v1betaseg([vV]1[bB][eE][tT][aA])/:path*", + "/:v1betaseg([vV]1[bB][eE][tT][aA])", + "/:chatseg([cC][hH][aA][tT])/:path*", + "/:respseg([rR][eE][sS][pP][oO][nN][sS][eE][sS])/:path*", + "/:respseg([rR][eE][sS][pP][oO][nN][sS][eE][sS])", + "/:codexseg([cC][oO][dD][eE][xX])/:path*", + "/:codexseg([cC][oO][dD][eE][xX])", + "/:modelsseg([mM][oO][dD][eE][lL][sS])", ], }; diff --git a/src/server/authz/classify.ts b/src/server/authz/classify.ts index a5270860d6..bfe0f0d6f9 100644 --- a/src/server/authz/classify.ts +++ b/src/server/authz/classify.ts @@ -16,30 +16,39 @@ function normalizePathname(rawPath: string): { path: string; reason?: Classifica if (!path.startsWith("/")) path = "/" + path; if (path.length > 1 && path.endsWith("/")) path = path.slice(0, -1); - if (path === "/codex" || path.startsWith("/codex/")) { + // Client-API aliases are matched case-insensitively on the control segment. + // Next's rewrite layer accepts `/V1/...`, `/CODEX`, etc. and routes them to + // the client handler, so the classifier must recognize the same casing — + // otherwise an uppercase alias falls through to the management fallback and + // the request is treated as a different route class than it is actually + // dispatched to (GHSA-jvqc-mp9f-q936). Only the leading control segment is + // lowercased for detection; the original-case tail is preserved. + const lower = path.toLowerCase(); + + if (lower === "/codex" || lower.startsWith("/codex/")) { return { path: "/api/v1/responses", reason: "client_api_codex_alias" }; } - if (path === "/v1/v1" || path.startsWith("/v1/v1/")) { + if (lower === "/v1/v1" || lower.startsWith("/v1/v1/")) { const tail = path.slice("/v1/v1".length) || ""; return { path: "/api/v1" + tail, reason: "client_api_double_prefix" }; } - if (path === "/v1beta" || path.startsWith("/v1beta/")) { + if (lower === "/v1beta" || lower.startsWith("/v1beta/")) { const tail = path.slice("/v1beta".length) || ""; return { path: "/api/v1beta" + tail, reason: "client_api_alias" }; } - if (path === "/v1" || path.startsWith("/v1/")) { + if (lower === "/v1" || lower.startsWith("/v1/")) { const tail = path.slice("/v1".length) || ""; return { path: "/api/v1" + tail, reason: "client_api_alias" }; } for (const { alias, canonical } of CLIENT_API_ALIAS_PREFIXES) { - if (path === alias) { + if (lower === alias) { return { path: canonical, reason: "client_api_alias" }; } - if (path.startsWith(alias + "/")) { + if (lower.startsWith(alias + "/")) { return { path: canonical + path.slice(alias.length), reason: "client_api_alias" }; } } diff --git a/src/server/authz/routeGuard.ts b/src/server/authz/routeGuard.ts index 4bb072d5cb..d59e68836f 100644 --- a/src/server/authz/routeGuard.ts +++ b/src/server/authz/routeGuard.ts @@ -119,6 +119,11 @@ export const ALWAYS_PROTECTED_API_PATHS: ReadonlyArray = [ "/api/shutdown", "/api/providers/health-autopilot/actions", "/api/settings/database", + // Full-database export/import: a credential dump and an irreversible replace. + // Must stay authenticated even under requireLogin=false, for the same reason + // /api/settings/database already does. isAlwaysProtectedPath matches on a path + // boundary, so this covers export, exportAll and import. (GHSA-mghq-58h3-qcqj) + "/api/db-backups", ]; export function isLoopbackHost(hostHeader: string | null): boolean { diff --git a/src/shared/constants/spawnCapablePrefixes.ts b/src/shared/constants/spawnCapablePrefixes.ts index c23d6cc4a5..20787d7d2f 100644 --- a/src/shared/constants/spawnCapablePrefixes.ts +++ b/src/shared/constants/spawnCapablePrefixes.ts @@ -52,6 +52,7 @@ export const SPAWN_CAPABLE_PATTERNS: ReadonlyArray = [ /^\/api\/providers\/[^/]+\/login\/?$/, // pre-existing gap: in LOCAL_ONLY_API_PATTERNS today but never in a spawn-capable deny-list /^\/api\/providers\/[^/]+\/refresh-cursor\/?$/, // spawns cursor-agent via renewal.ts (Hard Rules #15 + #17) /^\/api\/providers\/cursor\/agent-availability\/?$/, // static path (no dynamic segment), but kept in this array alongside its /api/providers/ siblings rather than the flat SPAWN_CAPABLE_PREFIXES array — spawns cursor-agent status via checkCursorAgentAvailability()/getCachedCursorAgentAvailability() (Hard Rules #15 + #17) + /^\/api\/providers\/[^/]+\/chatgpt-web-codex-doctor\/?$/, // spawns via getTunnelRuntimeStatus() → spawnSync("...","runtimes status") (open-sse/executors/chatgpt-web-codex/tunnelClient.ts). Mirrors LOCAL_ONLY_API_PATTERNS in routeGuard.ts; keep the two in sync (GHSA-9q3h-mjm5-f4gj). ]; /** diff --git a/tests/unit/acp-agents-route.test.ts b/tests/unit/acp-agents-route.test.ts index 699cb607bd..2d3065a9a8 100644 --- a/tests/unit/acp-agents-route.test.ts +++ b/tests/unit/acp-agents-route.test.ts @@ -100,3 +100,32 @@ test("POST /api/acp/agents rejects unsafe version commands for authenticated ses assert.equal(response.status, 400); assert.match(body.error, /Invalid versionCommand/i); }); + +test("POST /api/acp/agents rejects an interpreter eval payload (GHSA-jphr-2gw7-xrwp)", async () => { + // Exact shape of the advisory PoC: binary + versionCommand both name `node`, + // so the binary-match check passes, but the `-e` eval argument must still be + // refused before it can reach execFileSync("node", ["-e", ...]). + process.env.JWT_SECRET = "acp-agents-jwt-secret"; + await localDb.updateSettings({ requireLogin: true, password: "hashed-password" }); + const token = await createSessionToken(); + + const response = await routeModule.POST( + makeRequest( + "POST", + { + id: "anonrce", + name: "anonrce", + binary: "node", + versionCommand: 'node -e "process.exit(1)"', + providerAlias: "anonrce", + spawnArgs: [], + protocol: "stdio", + }, + token + ) + ); + const body = (await response.json()) as { error?: string }; + + assert.equal(response.status, 400); + assert.match(body.error ?? "", /Invalid versionCommand/i); +}); diff --git a/tests/unit/acp-registry.test.ts b/tests/unit/acp-registry.test.ts index deb14d709b..ed8dff978e 100644 --- a/tests/unit/acp-registry.test.ts +++ b/tests/unit/acp-registry.test.ts @@ -32,6 +32,41 @@ test("resolveVersionProbe rejects shell metacharacters in version commands", () assert.equal(probe, null); }); +// Regression guard — GHSA-jphr-2gw7-xrwp / GHSA-hf57-cqmx-p4gr (ACP custom-agent +// RCE). A client-registered custom agent controls both `binary` and +// `versionCommand`; the binary-match check alone still admits an eval-style +// argument on a matching interpreter, which reaches execFileSync as arbitrary +// code execution (no shell metacharacter required). A version *probe* only ever +// needs a version flag, so untrusted probes must reject non-version arguments. +test("resolveVersionProbe rejects interpreter eval arguments on a matching binary", () => { + assert.equal(resolveVersionProbe("node", 'node -e "process.exit(1)"', true), null); + assert.equal(resolveVersionProbe("node", "node --eval 1", true), null); + assert.equal(resolveVersionProbe("python3", 'python3 -c "import os"', true), null); + assert.equal(resolveVersionProbe("ruby", 'ruby -e "puts 1"', true), null); + // Any extra argument beyond a single version flag is refused for a probe. + assert.equal(resolveVersionProbe("node", "node --version --eval 1", true), null); +}); + +test("resolveVersionProbe still accepts legitimate version flags for custom agents", () => { + assert.deepEqual(resolveVersionProbe("node", "node --version", true), { + command: "node", + args: ["--version"], + }); + assert.deepEqual(resolveVersionProbe("my-agent", "my-agent -v", true), { + command: "my-agent", + args: ["-v"], + }); + assert.deepEqual(resolveVersionProbe("my-agent", "my-agent version", true), { + command: "my-agent", + args: ["version"], + }); + // Bare binary with no arguments is a valid probe too. + assert.deepEqual(resolveVersionProbe("my-agent", "my-agent", true), { + command: "my-agent", + args: [], + }); +}); + test("shouldUseShellForVersionProbe preserves Windows npm wrapper detection", () => { assert.equal(shouldUseShellForVersionProbe("codex", "win32"), true); assert.equal( diff --git a/tests/unit/authz/proxy-contract.test.ts b/tests/unit/authz/proxy-contract.test.ts index 93d44b5852..438eb0f24c 100644 --- a/tests/unit/authz/proxy-contract.test.ts +++ b/tests/unit/authz/proxy-contract.test.ts @@ -55,15 +55,19 @@ test("proxy.ts delegates to runAuthzPipeline with enforce: true", () => { test("proxy.ts config.matcher covers every /api/* route plus dashboard and v1 aliases", () => { const content = fs.readFileSync("src/proxy.ts", "utf8"); // Required prefixes — drop one and the corresponding routes go unguarded. + // The client-API aliases use a case-insensitive path-to-regexp group + // (`([vV]1)`) so `/V1/...` reaches the pipeline too — see + // GHSA-jvqc-mp9f-q936 and tests/unit/authz/proxy-matcher-case.test.ts for the + // semantic (compiled-matcher) coverage assertions. const requiredMatchers = [ '"/api/:path*"', '"/dashboard/:path*"', - '"/v1/:path*"', - '"/v1beta/:path*"', - '"/chat/:path*"', - '"/responses/:path*"', - '"/codex/:path*"', - '"/models"', + '"/:v1seg([vV]1)/:path*"', + '"/:v1betaseg([vV]1[bB][eE][tT][aA])/:path*"', + '"/:chatseg([cC][hH][aA][tT])/:path*"', + '"/:respseg([rR][eE][sS][pP][oO][nN][sS][eE][sS])/:path*"', + '"/:codexseg([cC][oO][dD][eE][xX])/:path*"', + '"/:modelsseg([mM][oO][dD][eE][lL][sS])"', ]; for (const matcher of requiredMatchers) { assert.ok( diff --git a/tests/unit/authz/proxy-matcher-case.test.ts b/tests/unit/authz/proxy-matcher-case.test.ts new file mode 100644 index 0000000000..4069fd9b68 --- /dev/null +++ b/tests/unit/authz/proxy-matcher-case.test.ts @@ -0,0 +1,76 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { createRequire } from "node:module"; + +import { config } from "../../../src/proxy.ts"; +import { classifyRoute } from "../../../src/server/authz/classify.ts"; + +// Regression guard — GHSA-jvqc-mp9f-q936 (case-sensitive authz-matcher bypass). +// +// Next.js compiles the middleware/proxy matcher from `regexp.source` only, +// dropping path-to-regexp's default case-insensitive flag, so a lowercase +// literal like `/v1/:path*` does NOT match `/V1/...`. The rewrite matcher keeps +// the flag, so `/V1/chat/completions` was still rewritten to the handler while +// skipping the authz pipeline entirely — an unauthenticated inference bypass. +// +// The fix expresses the case-insensitivity inside a path-to-regexp custom group +// (`/:seg([vV]1)/:path*`), which survives the flag-drop because it needs no +// flag. This test compiles the matcher exactly the way Next does and asserts the +// uppercase / mixed-case client aliases are covered. + +const require = createRequire(import.meta.url); +const { tryToParsePath } = require("next/dist/lib/try-to-parse-path.js"); + +function compiledMatcherRegexes(): RegExp[] { + return (config.matcher as string[]).map((entry) => { + const parsed = tryToParsePath(entry); + // Mirror Next's middleware-route-matcher: source only, no flags. + return new RegExp(parsed.regexStr as string); + }); +} + +function isMatchedByProxy(path: string): boolean { + return compiledMatcherRegexes().some((re) => re.test(path)); +} + +test("proxy matcher still covers the canonical lowercase client aliases", () => { + for (const p of [ + "/v1/chat/completions", + "/v1/models", + "/v1beta/models", + "/responses", + "/codex/x", + "/models", + ]) { + assert.equal(isMatchedByProxy(p), true, `expected proxy matcher to cover ${p}`); + } +}); + +test("proxy matcher covers uppercase / mixed-case client aliases (GHSA-jvqc-mp9f-q936)", () => { + for (const p of [ + "/V1/chat/completions", + "/V1/models", + "/V1BETA/models", + "/CHAT/completions", + "/RESPONSES", + "/CODEX/x", + "/MODELS", + "/Responses/x", + "/v1BeTa/models", + ]) { + assert.equal( + isMatchedByProxy(p), + true, + `uppercase alias ${p} must reach the authz pipeline, not skip it` + ); + } +}); + +test("classifyRoute treats uppercase client aliases as CLIENT_API, not management fallback", () => { + assert.equal(classifyRoute("/V1/chat/completions", "POST").routeClass, "CLIENT_API"); + assert.equal(classifyRoute("/V1BETA/models", "GET").routeClass, "CLIENT_API"); + assert.equal(classifyRoute("/MODELS", "GET").routeClass, "CLIENT_API"); + assert.equal(classifyRoute("/CODEX", "POST").routeClass, "CLIENT_API"); + // Lowercase behavior is unchanged. + assert.equal(classifyRoute("/v1/chat/completions", "POST").routeClass, "CLIENT_API"); +}); diff --git a/tests/unit/authz/routeGuard.test.ts b/tests/unit/authz/routeGuard.test.ts index 2a4fd3799c..163f5bce41 100644 --- a/tests/unit/authz/routeGuard.test.ts +++ b/tests/unit/authz/routeGuard.test.ts @@ -6,6 +6,7 @@ import { isAlwaysProtectedPath, isLoopbackHost, } from "../../../src/server/authz/routeGuard.ts"; +import { SPAWN_CAPABLE_PATTERNS } from "../../../src/shared/constants/spawnCapablePrefixes.ts"; import { managementPolicy } from "../../../src/server/authz/policies/management.ts"; import { getMachineTokenSync } from "../../../src/lib/machineToken.ts"; import { CLI_TOKEN_HEADER } from "../../../src/server/authz/headers.ts"; @@ -50,6 +51,27 @@ test("isLocalOnlyBypassableByManageScope: non-local-only routes are not bypassab assert.equal(isLocalOnlyBypassableByManageScope("/api/settings"), false); }); +test("SPAWN_CAPABLE_PATTERNS covers every regex-tier LOCAL_ONLY spawn route (GHSA-9q3h-mjm5-f4gj)", () => { + // The manage-scope bypass veto's precise early-deny keys on + // SPAWN_CAPABLE_PATTERNS, so every LOCAL_ONLY_API_PATTERNS entry (a + // spawn-capable regex route) must have a matching pattern here — otherwise the + // two layers drift and a spawn route loses its exact early-deny. This guards + // against the chatgpt-web-codex-doctor drift and any future one. + const spawnRoutes = [ + "/api/providers/acct-1/login", + "/api/providers/acct-1/refresh-cursor", + "/api/providers/acct-1/chatgpt-web-codex-doctor", + ]; + for (const p of spawnRoutes) { + assert.equal(isLocalOnlyPath(p), true, `${p} must be LOCAL_ONLY`); + assert.equal( + SPAWN_CAPABLE_PATTERNS.some((re) => re.test(p)), + true, + `${p} is a LOCAL_ONLY spawn route but SPAWN_CAPABLE_PATTERNS does not cover it` + ); + } +}); + test("isAlwaysProtectedPath: /api/shutdown is always protected", () => { assert.equal(isAlwaysProtectedPath("/api/shutdown"), true); }); @@ -58,6 +80,15 @@ test("isAlwaysProtectedPath: /api/settings/database is always protected", () => assert.equal(isAlwaysProtectedPath("/api/settings/database"), true); }); +test("isAlwaysProtectedPath: /api/db-backups is always protected (GHSA-mghq-58h3-qcqj)", () => { + // Full-database read/replace must require auth even when requireLogin=false — + // the same Tier-2 trade-off /api/settings/database already makes. The single + // prefix entry covers export, exportAll, import and future siblings. + assert.equal(isAlwaysProtectedPath("/api/db-backups/export"), true); + assert.equal(isAlwaysProtectedPath("/api/db-backups/exportAll"), true); + assert.equal(isAlwaysProtectedPath("/api/db-backups/import"), true); +}); + test("isAlwaysProtectedPath: ordinary settings routes are not always protected", () => { assert.equal(isAlwaysProtectedPath("/api/settings"), false); assert.equal(isAlwaysProtectedPath("/api/settings/proxy"), false);