fix(authz): hard-gate every credential export and CLI-config write (GHSA-5926-2w35-7h4q) - #12600
Conversation
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
…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).
/sweep-reds round 6 — FQG diagnosis (no code change)
FQG job: https://github.com/diegosouzapw/OmniRoute/actions/runs/33753300941/job/100642036504 Reproduced on a clean tip checkout ( Cause: Own complexity / file-size / mutation: not in the failed-gate list (FQG failed only Not merging origin/release into this PR (base-red #12581 OPEN). Holding until a base-reds PR retargets the allowlist line-key (302 → 313). |
#12350 fixed the LOCAL_ONLY half of the checker (prefixes + patterns + imported consts). isAlwaysProtectedPath() is two-armed the same way: ALWAYS_PROTECTED_API_PATHS.some(...) || ALWAYS_PROTECTED_API_PATTERNS.some(...) but the checker still read only the path array, so the four credential routes gated by the GHSA-5926-2w35-7h4q pattern (#12600) — /api/providers/{id}/{claude,codex}-auth/{export,apply-local} — reported as 'has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS', asking for the removal of a CORRECT annotation on a credential-export route. Verified with the real predicate: all four isAlwaysProtectedPath() → true; control /api/providers/{id}/models → false. tests/unit/openapi-security-tiers.test.ts already checks BOTH arrays (#12600 updated the test but not the gate script) and stays green — this commit makes the gate agree with the test and with the runtime. Also carries the file-size rebaseline for four caps grown by merged PRs (chat.ts +10 from #12427/#12503; stream.ts / accountFallback.ts / codex.ts +17 from #12179), rationale recorded in the baseline file. Refs #12581
….testFiles tests/unit/reset-aware-request-scope-12600.test.ts imports open-sse/services/combo/quotaScoring.ts, so the mutation-test-coverage gate (--strict) requires it in stryker.conf.json tap.testFiles so its mutant kills count. Pre-existing drift from diegosouzapw#12600, inherited from release/v3.8.51; it fails the Fast Quality Gates gate on every PR.
…too (+ file-size rebaseline) (#12605) * fix(ci): mirror isLocalOnlyPath in the security-tier gate and rebaseline four merged-growth file caps Two base-reds on release/v3.8.51 (#12581), both drained at the source. 1) check:openapi-security-tiers reported six CORRECTLY annotated routes as unprotected and demanded the removal of their x-loopback-only annotation — pushing the fix in the unsafe direction. The gate re-reads routeGuard.ts as text (it cannot import the module: routeGuard pulls the server runtime and the gate runs on plain node), but it only read the FIRST half of isLocalOnlyPath(): LOCAL_ONLY_API_PREFIXES.some(...) || LOCAL_ONLY_API_PATTERNS.some(...) so every route gated by a regex (/api/providers/volcengine-plan/connect/*) or by an imported constant (VNC_ROUTE_PREFIX, which the text parse turned into the literal string "VNC_ROUTE_PREFIX") looked open. Proven with isLocalOnlyPath() at runtime: all six return true; the control /api/providers/{id}/refresh stays false. New scripts/check/routeGuardConstants.mjs reads BOTH arrays, resolves imported identifiers by following the import, and THROWS on an unresolvable token instead of silently degrading it into a literal. Its array scanner is hand-rolled because regex literals carry the brackets and commas a \[([^\]]+)\] capture plus a naive comma split break on ([^/] and {1,3}). The reverse pass (missing-annotation warnings) now uses the same predicate. 2) check:file-size: four frozen files grew past their cap through merged PRs — chat.ts +10 (#12427/#12503 video-transcript redaction, derived from the post-guardrail payload at the single dispatch point) and stream.ts / accountFallback.ts / codex.ts +17 total (#12179 hot-path regex hoisting, bounded caches, quadratic-buffering fix). All cohesive at existing chokepoints; rebaselined with the rationale recorded in the baseline file. Refs #12581 * fix(ci): security-tier gate must honor ALWAYS_PROTECTED_API_PATTERNS too #12350 fixed the LOCAL_ONLY half of the checker (prefixes + patterns + imported consts). isAlwaysProtectedPath() is two-armed the same way: ALWAYS_PROTECTED_API_PATHS.some(...) || ALWAYS_PROTECTED_API_PATTERNS.some(...) but the checker still read only the path array, so the four credential routes gated by the GHSA-5926-2w35-7h4q pattern (#12600) — /api/providers/{id}/{claude,codex}-auth/{export,apply-local} — reported as 'has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS', asking for the removal of a CORRECT annotation on a credential-export route. Verified with the real predicate: all four isAlwaysProtectedPath() → true; control /api/providers/{id}/models → false. tests/unit/openapi-security-tiers.test.ts already checks BOTH arrays (#12600 updated the test but not the gate script) and stays green — this commit makes the gate agree with the test and with the runtime. Also carries the file-size rebaseline for four caps grown by merged PRs (chat.ts +10 from #12427/#12503; stream.ts / accountFallback.ts / codex.ts +17 from #12179), rationale recorded in the baseline file. Refs #12581 * fix(ci): re-anchor the zcodeProtocol public-creds allowlist entry (302 -> 313) The check:public-creds allowlist pins each frozen literal by FILE:LINE, so #12179 (hot-path regex hoisting in the same file) shifted the ZCode handshake id from L302 to L313 and broke the gate twice over: the old entry went stale ('a violação foi corrigida; REMOVA a entrada') while the literal itself, now at L313, was no longer covered. The literal is unchanged and still not a credential: `omniroute-${process.pid}` is a per-process handshake id for the local ZCode app-server, already audited and frozen with that justification. Only the anchor moves. Refs #12581 * test(ci): re-anchor the ZCode allowlist test to L313 alongside the gate entry The allowlist key is file:LINE:value, so the synthetic source in this test pads to the exact line the entry pins. Re-anchoring the entry 302 -> 313 (previous commit) without moving the padding left the test asserting the old line — caught by Unit Tests fast-path (4/4) on #12605. Both halves now sit at 313, and the test still proves the allowlist does NOT weaken detection: swapping the value for 'upstream-client-' is still flagged. Refs #12581 * docs(ci): changelog fragment for #12605 * chore(ci): trim #12605 to the one fix the base still needs The base drained fast while this PR was open. Re-verified on 008da6d and dropped everything already covered there: - check-public-creds.mjs: the base already re-anchors the ZCode entry to L313 (my commit only added a comment on top) -> reverted to the base version. - file-size-baseline.json: the base rebaselined chat.ts/codex.ts/ accountFallback.ts to HIGHER caps than mine, and stream.ts measures 3064 against the base cap of 3072 — my 3078 bump would have loosened a cap for no reason -> reverted to the base version. What the base still does NOT have, verified on its current tip: node scripts/check/check-openapi-security-tiers.mjs -> EXIT=1, 4 mismatches so the ALWAYS_PROTECTED_API_PATTERNS half stays, plus its changelog entry. Refs #12581
…ource text The diegosouzapw#12600 guard read combo.ts as text and asserted `getQuotaFetchScope(` appeared in it. That was always a proxy for "the builder scopes the quota fetch per model family", and it broke the moment the builder moved — a regex over a file path cannot survive a refactor. It now drives buildAutoCandidates with a spy quota fetcher and caching off: two model families on one Antigravity account produce two quota fetches, two models of one family produce one. That is the behaviour diegosouzapw#12600 shipped (a Claude-empty account must not hide its Gemini window), and it holds wherever the builder lives next. The companion assertion that quotaStrategies.ts uses the shared helper and does not redefine it stays as a source check: there is no observable behaviour that distinguishes one definition from a duplicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…HSA-5926-2w35-7h4q) (diegosouzapw#12600) * 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 diegosouzapw#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. diegosouzapw#11438).
…too (+ file-size rebaseline) (diegosouzapw#12605) * fix(ci): mirror isLocalOnlyPath in the security-tier gate and rebaseline four merged-growth file caps Two base-reds on release/v3.8.51 (diegosouzapw#12581), both drained at the source. 1) check:openapi-security-tiers reported six CORRECTLY annotated routes as unprotected and demanded the removal of their x-loopback-only annotation — pushing the fix in the unsafe direction. The gate re-reads routeGuard.ts as text (it cannot import the module: routeGuard pulls the server runtime and the gate runs on plain node), but it only read the FIRST half of isLocalOnlyPath(): LOCAL_ONLY_API_PREFIXES.some(...) || LOCAL_ONLY_API_PATTERNS.some(...) so every route gated by a regex (/api/providers/volcengine-plan/connect/*) or by an imported constant (VNC_ROUTE_PREFIX, which the text parse turned into the literal string "VNC_ROUTE_PREFIX") looked open. Proven with isLocalOnlyPath() at runtime: all six return true; the control /api/providers/{id}/refresh stays false. New scripts/check/routeGuardConstants.mjs reads BOTH arrays, resolves imported identifiers by following the import, and THROWS on an unresolvable token instead of silently degrading it into a literal. Its array scanner is hand-rolled because regex literals carry the brackets and commas a \[([^\]]+)\] capture plus a naive comma split break on ([^/] and {1,3}). The reverse pass (missing-annotation warnings) now uses the same predicate. 2) check:file-size: four frozen files grew past their cap through merged PRs — chat.ts +10 (diegosouzapw#12427/diegosouzapw#12503 video-transcript redaction, derived from the post-guardrail payload at the single dispatch point) and stream.ts / accountFallback.ts / codex.ts +17 total (diegosouzapw#12179 hot-path regex hoisting, bounded caches, quadratic-buffering fix). All cohesive at existing chokepoints; rebaselined with the rationale recorded in the baseline file. Refs diegosouzapw#12581 * fix(ci): security-tier gate must honor ALWAYS_PROTECTED_API_PATTERNS too diegosouzapw#12350 fixed the LOCAL_ONLY half of the checker (prefixes + patterns + imported consts). isAlwaysProtectedPath() is two-armed the same way: ALWAYS_PROTECTED_API_PATHS.some(...) || ALWAYS_PROTECTED_API_PATTERNS.some(...) but the checker still read only the path array, so the four credential routes gated by the GHSA-5926-2w35-7h4q pattern (diegosouzapw#12600) — /api/providers/{id}/{claude,codex}-auth/{export,apply-local} — reported as 'has x-always-protected but is NOT in ALWAYS_PROTECTED_API_PATHS', asking for the removal of a CORRECT annotation on a credential-export route. Verified with the real predicate: all four isAlwaysProtectedPath() → true; control /api/providers/{id}/models → false. tests/unit/openapi-security-tiers.test.ts already checks BOTH arrays (diegosouzapw#12600 updated the test but not the gate script) and stays green — this commit makes the gate agree with the test and with the runtime. Also carries the file-size rebaseline for four caps grown by merged PRs (chat.ts +10 from diegosouzapw#12427/diegosouzapw#12503; stream.ts / accountFallback.ts / codex.ts +17 from diegosouzapw#12179), rationale recorded in the baseline file. Refs diegosouzapw#12581 * fix(ci): re-anchor the zcodeProtocol public-creds allowlist entry (302 -> 313) The check:public-creds allowlist pins each frozen literal by FILE:LINE, so diegosouzapw#12179 (hot-path regex hoisting in the same file) shifted the ZCode handshake id from L302 to L313 and broke the gate twice over: the old entry went stale ('a violação foi corrigida; REMOVA a entrada') while the literal itself, now at L313, was no longer covered. The literal is unchanged and still not a credential: `omniroute-${process.pid}` is a per-process handshake id for the local ZCode app-server, already audited and frozen with that justification. Only the anchor moves. Refs diegosouzapw#12581 * test(ci): re-anchor the ZCode allowlist test to L313 alongside the gate entry The allowlist key is file:LINE:value, so the synthetic source in this test pads to the exact line the entry pins. Re-anchoring the entry 302 -> 313 (previous commit) without moving the padding left the test asserting the old line — caught by Unit Tests fast-path (4/4) on diegosouzapw#12605. Both halves now sit at 313, and the test still proves the allowlist does NOT weaken detection: swapping the value for 'upstream-client-' is still flagged. Refs diegosouzapw#12581 * docs(ci): changelog fragment for diegosouzapw#12605 * chore(ci): trim diegosouzapw#12605 to the one fix the base still needs The base drained fast while this PR was open. Re-verified on 9960e46 and dropped everything already covered there: - check-public-creds.mjs: the base already re-anchors the ZCode entry to L313 (my commit only added a comment on top) -> reverted to the base version. - file-size-baseline.json: the base rebaselined chat.ts/codex.ts/ accountFallback.ts to HIGHER caps than mine, and stream.ts measures 3064 against the base cap of 3072 — my 3078 bump would have loosened a cap for no reason -> reverted to the base version. What the base still does NOT have, verified on its current tip: node scripts/check/check-openapi-security-tiers.mjs -> EXIT=1, 4 mismatches so the ALWAYS_PROTECTED_API_PATTERNS half stays, plus its changelog entry. Refs diegosouzapw#12581
Fixes GHSA-5926-2w35-7h4q (HIGH), reported by @skeletonsec. Targets
release/v3.8.51.The report, confirmed on the tip
POST /api/providers/{id}/claude-auth/exportand.../codex-auth/exportgate onrequireManagementAuth(request)with noalwaysRequireAuth, and neither path was inALWAYS_PROTECTED_API_PATHS. UnderrequireLogin=false— the local-first default — both fail open: anyone who knows a connection id downloads the operator's raw Claude/Codex OAuthaccess_token/refresh_token, plus the Codexid_token.The report is exactly right, including the part that stings: this is the third recurrence of one class. GHSA-mghq-58h3-qcqj added
/api/db-backups; GHSA-v7g9-7f55-5g46 added the/api/settings/*-jsonsiblings mghq had missed; these two are the siblings both missed.So the fix is against the class, not the two routes
Sweeping every route that hands out stored credentials, dumps captured traffic, or writes the operator's CLI config turned up four more sitting on the fail-open tier:
requireLogin=falseGET /api/logs/exportcall_logs(prompts and responses) andproxy_logs, up to 168h/api/cli-tools/codex-profilesauth.json+config.tomlinto the host's Codex CLI config{claude,codex}-auth/apply-local~/.codex/auth.json/ the Claude equivalentproviders/agy-auth/apply-local~/.gemini/antigravity-cli/antigravity-oauth-tokenThe
codex-profilesone is arguably worse than what was reported, because it is a write. Its only guard isensureCliConfigWriteAllowed()called with notargetPath, which reduces toCLI_ALLOW_CONFIG_WRITES— defaulttrue. Paired with the sibling POST that stores an arbitrary profile, the chain is: POST a profile holding the attacker'sauth.json, PUT it, and the operator's Codex CLI now runs on attacker credentials — or, viaconfig.toml, against an attacker-controlled base URL, which puts every subsequent prompt through them.The traffic-inspector HAR exports were already covered by
LOCAL_ONLY— verified, not assumed.Implementation
Routes with a dynamic segment cannot be expressed in the exact/prefix list: a
/api/providers/prefix would hard-gate the entire provider surface and break every keyless install. So this addsALWAYS_PROTECTED_API_PATTERNS, mirroring theLOCAL_ONLY_API_PATTERNSthat already exists for the same reason, andisAlwaysProtectedPathconsults both.Two deliberate calls, both flagged because they are judgement rather than mechanics:
apply-localgets ALWAYS_PROTECTED, not LOCAL_ONLY. It closes the anonymous hole without breaking an operator driving the dashboard through a tunnel. LOCAL_ONLY would be defensible on "it writes to the host's home dir", but it changes a working flow for a legitimate setup.{ alwaysRequireAuth: true }at the handlers, even though the report suggests it. Tier 2 is the architecture's designated mechanism and the guard runs before the handler, so it would be redundant — and a second copy of the same decision inside each route is exactly the kind of duplicate that drifts (cf. thedashboardCsrfprefix scan that had to be unified in fix(authz): match exact public routes exactly, not as prefixes (GHSA-74g9-q8f6-793h) #11417). If you would rather have belt-and-braces here, say so and I will add it.Validation
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, so recurrence #4 fails the suite instead of arriving as an advisory. Includes the negative cases that matter:/api/providers,/api/providers/{id},/api/providers/{id}/models,/api/providers/{id}/claude-authmust stay on MANAGEMENT — hard-gating them would break keyless local-first installs[^/]+and fall back to the fail-open tierRuns, hermetic (
env -u OMNIROUTE_API_KEY):tests/unit/authz/**+openapi-security-tiers+check-route-guard-membership+public-api-routes— 298/298typecheck:core— exit 0, no outputcheck:docs-sync,check:tracked-artifacts— PASSprettier --checkclean;eslint: the 8 errors inopenapi-security-tiers.test.tsare the pre-existingno-explicit-anyset already frozen ineslint-suppressions.json(count: 8, unchanged — this diff adds and removes noany)docs/openapi.yamlmarks the seven newly-gated operationsx-always-protected, andopenapi-security-tiers.test.tsnow resolves{param}placeholders so it can validate pattern entries — aligned to the new contract, not relaxed.