Repository navigation
fix(security): harden codex app-server transport (#11205 post-merge review) - #11281
Merged
Merged
Conversation
…eview) Two findings from the automated push security review on #11205: 1. HIGH (Agent/Subprocess Permission Bypass): approvalPolicy "never" + sandbox "danger-full-access" defaults, plus blanket auto-APPROVE of every server->client approval prompt, meant codex-decided host commands ran with no gate at all. Now: sandbox defaults to "workspace-write" (override via providerSpecificData.codexAppServerSandbox / OMNIROUTE_CODEX_APPSERVER_SANDBOX), and approval prompts — which gate codex's OWN command/file/permission execution, NOT the harness tool passthrough (item/tool/call) — are auto-DENIED unless the operator opts in via providerSpecificData.codexAppServerAutoApprove / OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE. 2. MEDIUM (SSRF / credential exfiltration): the /readyz health probe sent the bearer token to whatever URL a connection's providerSpecificData supplied and followed redirects. Now: env-sourced tokens only pair with env-sourced URLs or operator-local hosts (loopback/RFC1918/link-local/ULA/localhost/ single-label LAN names/*.local/*.ts.net/*.internal — literal match, no DNS), enforced inside resolveAppServerConfig so executor, gating and health probe all inherit it; and the probe uses redirect:"manual". TDD: 4 failing-then-passing tests (deny-by-default, workspace-write default, env-token→remote-psd-URL refusal incl. no-network assertion, redirect pinning) plus 6 new passing cases (local-host matrix, psd-token pairing, env/env pairing, opt-in approve). 30/30 in tests/unit/codex-app-server.test.ts. Also rebaselines two file-size entries that drifted on the release tip during the 2026-08-23 merge wave (annotated; verified pristine-tip).
…tap.testFiles Base-red drain: #11267 added tests/unit/quota-exhaustion-cutoff-opencode.test.ts (covers the mutated src/sse/services/auth.ts) without registering it, so the mutation-test-coverage gate fails on the pristine release tip.
diegosouzapw
pushed a commit
that referenced
this pull request
Aug 23, 2026
…nto v3.8.51) (#10952) Validated on the resolved merge against the current tip (527da65 + the post-#11281 rebaseline): the single conflict was a comment-only collision in providers/[id]/models/route.ts (kept the tip's #10828-ordering note). Focused suites 125/125 across all 13 touched test files (build-sqlite-stub, cc-compatible, copilot-claude-messages, copilot-gemini-route, executor-github, ghe-copilot, github-copilot-discovery-token, github-copilot-model-discovery, noauth-sibling-7620, provider-header-profiles, provider-models-config, request-log-payloads, upstream-error-passthrough), typecheck:core clean, file-size/changelog-integrity OK. Merged --admin over the inherited 2026-08-23 base-red cluster (#9985) — the reds are proven tip failures (CLI catalog cluster + @testing-library allowlist, being drained by #11280), not from this diff. Note: the rebase means several items the body listed (relay x-relay-path SSRF, /v1/search blocked-providers, #10736 rotation fence, #10903, #10865, #10899, #10916) already landed upstream and are NOT in this delta — the delta is: better-sqlite3 build guard + build heap/worker caps + telemetry-off (#10060 re-derived), credential-echo passthrough refusal + OCR/moderation redaction + call-log key redaction, Copilot CLI 1.0.81-6 wire identity + Claude→/v1/messages name-matched routing + discovery token fix, CC model_not_found 400, compat overrides for no-auth aliases (#7620-pinned). The Copilot wire-identity change is the one to watch in production. Thank you @arminanton — and the ported-author credits in the commit history (@rqzbeh, yidecode, the #10899/#10916 authors) are preserved. Your config-posture finding (REQUIRE_API_KEY default vs 0.0.0.0) is noted for a maintainer decision, as you scoped it.
Merged
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…post-merge review) (diegosouzapw#11281) TDD: 4 red-to-green tests reproducing the two review findings + 6 new cases; 30/30 in tests/unit/codex-app-server.test.ts; typecheck/eslint/env-doc-sync/file-size clean. Merged --admin over the inherited 2026-08-23 base-red cluster (diegosouzapw#9985): the remaining reds (CLI catalog/registry tests, @testing-library allowlist) are proven tip failures unrelated to this diff — shard logs show only CLI-cluster failures, and this PR itself drains the mutation-test-coverage red (stryker registration for diegosouzapw#11267's test). Behavior change for app-server deployments is documented in the PR body (sandbox default + binding refusal).
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…nto v3.8.51) (diegosouzapw#10952) Validated on the resolved merge against the current tip (585c2b7 + the post-diegosouzapw#11281 rebaseline): the single conflict was a comment-only collision in providers/[id]/models/route.ts (kept the tip's diegosouzapw#10828-ordering note). Focused suites 125/125 across all 13 touched test files (build-sqlite-stub, cc-compatible, copilot-claude-messages, copilot-gemini-route, executor-github, ghe-copilot, github-copilot-discovery-token, github-copilot-model-discovery, noauth-sibling-7620, provider-header-profiles, provider-models-config, request-log-payloads, upstream-error-passthrough), typecheck:core clean, file-size/changelog-integrity OK. Merged --admin over the inherited 2026-08-23 base-red cluster (diegosouzapw#9985) — the reds are proven tip failures (CLI catalog cluster + @testing-library allowlist, being drained by diegosouzapw#11280), not from this diff. Note: the rebase means several items the body listed (relay x-relay-path SSRF, /v1/search blocked-providers, diegosouzapw#10736 rotation fence, diegosouzapw#10903, diegosouzapw#10865, diegosouzapw#10899, diegosouzapw#10916) already landed upstream and are NOT in this delta — the delta is: better-sqlite3 build guard + build heap/worker caps + telemetry-off (diegosouzapw#10060 re-derived), credential-echo passthrough refusal + OCR/moderation redaction + call-log key redaction, Copilot CLI 1.0.81-6 wire identity + Claude→/v1/messages name-matched routing + discovery token fix, CC model_not_found 400, compat overrides for no-auth aliases (diegosouzapw#7620-pinned). The Copilot wire-identity change is the one to watch in production. Thank you @arminanton — and the ported-author credits in the commit history (@rqzbeh, yidecode, the diegosouzapw#10899/diegosouzapw#10916 authors) are preserved. Your config-posture finding (REQUIRE_API_KEY default vs 0.0.0.0) is noted for a maintainer decision, as you scoped it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the two findings from the automated push security review on the merged #11205:
1. HIGH — Agent/Subprocess Permission Bypass
sandboxdefault:danger-full-access→workspace-write(per-connection override:providerSpecificData.codexAppServerSandbox; env:OMNIROUTE_CODEX_APPSERVER_SANDBOX).item/tool/call) are now auto-DENIED by default. Opt-in auto-approval viaproviderSpecificData.codexAppServerAutoApprove/OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE(true/1/yes).approvalPolicy: "never"is kept (a router turn must never block on codex's interactive approval) — the sandbox is now the real gate.2. MEDIUM — SSRF / credential exfiltration
resolveAppServerConfig(so the executor, theisCodexAppServerRequiredgating and the health probe all inherit it): an env-sourced capability token is only sent to (a) an env-sourced URL, or (b) an operator-local host — loopback, RFC1918, link-local, IPv6 ULA/link-local,localhost, single-label LAN/hosts-file names,*.local,*.ts.net,*.internal(literal matching, no DNS resolution). A psd-sourced token may go anywhere (whoever wrote the psd already knows it)./readyzprobe now usesredirect: "manual"— the bearer token never follows a 30x.danger-full-accessdefault, setcodexAppServerSandbox: "danger-full-access"in the connection's providerSpecificData (or the env var) explicitly; if it relied on auto-approved prompts, setcodexAppServerAutoApprove: "true". And if your connection used a remote (non-LAN) app-server URL with the token in env, that pairing is now refused by design — move the token into the connection's providerSpecificData (or the URL into env).Validation (TDD, Hard Rule #18)
4 failing-then-passing tests reproducing the findings → fix → green:
sandbox: "workspace-write"resolveAppServerConfigreturns null (and the health probe performs zero network calls in that case)redirect: "manual"Plus 6 new passing cases: local-host matrix (11 URLs), psd-token pairing, env/env pairing. 30/30 in
tests/unit/codex-app-server.test.ts. Gates: typecheck:core clean, env-doc-sync ✓, file-size ✓ (two entries rebaselined for pristine-tip drift from the 2026-08-23 merge wave, annotated), eslint clean on touched files.Docs:
.env.example+docs/reference/ENVIRONMENT.mdupdated (new var + sandbox default note). Changelog fragment included.