diff --git a/.env.example b/.env.example index e50780dfc4..7d92b7cad5 100644 --- a/.env.example +++ b/.env.example @@ -1324,8 +1324,14 @@ CURSOR_USER_AGENT="Cursor/3.4" # Approval policy passed to the app-server turn (e.g. never, on-request). # OMNIROUTE_CODEX_APPSERVER_APPROVAL=never # Sandbox policy passed to the app-server turn (e.g. read-only, -# workspace-write, danger-full-access). +# workspace-write, danger-full-access). When unset, the executor defaults to +# "workspace-write" (hardened; used to be "danger-full-access"). # OMNIROUTE_CODEX_APPSERVER_SANDBOX=read-only +# Auto-approve the app-server's own approval prompts (command/file/permission +# execution on the host). Defaults to OFF — prompts are auto-denied. Set to +# true/1/yes only when you trust the deployment to run codex-decided host +# commands. Per-connection override: providerSpecificData.codexAppServerAutoApprove. +# OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE=false # ═══════════════════════════════════════════════════════════════════════════════ # 13. CLI FINGERPRINT COMPATIBILITY (Anti-Detection) diff --git a/changelog.d/fixes/codex-appserver-hardening.md b/changelog.d/fixes/codex-appserver-hardening.md new file mode 100644 index 0000000000..b73bc3fc20 --- /dev/null +++ b/changelog.d/fixes/codex-appserver-hardening.md @@ -0,0 +1 @@ +- Hardened the Codex app-server transport after the post-merge security review of #11205: approval prompts from the app-server (its own command/file/permission execution — not the harness tool passthrough) are now auto-denied by default, with opt-in auto-approval via `providerSpecificData.codexAppServerAutoApprove` / `OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE`; the default codex sandbox changed from `danger-full-access` to `workspace-write` (override per connection or env); env-sourced capability tokens are now only sent to env-sourced URLs or operator-local hosts (loopback/RFC1918/link-local/ULA/localhost/single-label LAN names/*.local/*.ts.net/*.internal), so a connection's providerSpecificData URL can no longer exfiltrate the operator's env token; and the `/readyz` health probe no longer follows redirects while carrying the bearer token. diff --git a/config/quality/file-size-baseline.json b/config/quality/file-size-baseline.json index 70e2123990..4515d85e25 100644 --- a/config/quality/file-size-baseline.json +++ b/config/quality/file-size-baseline.json @@ -433,7 +433,7 @@ "src/shared/components/analytics/charts.tsx": 1346, "src/shared/services/cliRuntime.ts": 1459, "src/sse/handlers/chat.ts": 2493, - "src/sse/services/auth.ts": 3337, + "src/sse/services/auth.ts": 3344, "_rebaseline_2026_08_23_11186_synced_inventory_routing": "PR #11186 (pacocartones) own growth: src/sse/services/auth.ts 3260->3337 (+77, loadAdvertisedModelsForSelfHostedConnections + the modelNotAdvertised candidate-filter predicate — pins chat routing to the connection whose synced inventory actually advertises the model, fixing spurious model-not-found on multi-host self-hosted setups; at the existing credential-selection chokepoint, not extractable without splitting the selection flow). Covered by tests/unit/chat-routing-synced-inventory-11089.test.ts. Owner pre-authorized baseline bumps 2026-08-22.", "tests/unit/account-fallback-service.test.ts": 2044, "tests/unit/provider-validation-specialty.test.ts": 3880, @@ -464,14 +464,15 @@ "src/app/(dashboard)/dashboard/providers/[id]/providerPageHelpers.ts": 1014, "open-sse/config/imageRegistry.ts": 1034, "src/sse/handlers/chatHelpers.ts": 1019, - "src/shared/middleware/chatBodyAdmission.ts": 1009, + "src/shared/middleware/chatBodyAdmission.ts": 1118, "_rebaseline_2026_08_22_11020_sigterm_drain": "PR #11020 (RaviTharuma) own growth: chatBodyAdmission.ts 1005->1009 (+4, heavyweight admission leases now increment the SIGTERM drain counter and releaseChatAdmissionWhenDone holds it for the SSE lifetime — closes #11015; +4 are the lease/drain wiring lines at the existing admission chokepoint). Covered by tests/unit/chat-body-admission.test.ts heavyweight-lease cases. Owner pre-authorized baseline bumps 2026-08-22.", "_rebaseline_2026_08_20_10668_tabitoken_gateway": "#10668 (yawar-aquil) own catalog growth: src/shared/constants/providers/apikey/gateways.ts 1268->1283 (+15, entirely this PR diff -- one new tabitoken gateway entry, data lines only; base moved from 1255 to 1268 via other merges since the PR forked). Not combination drift: reproducible on the PR branch alone, so the WS5.5 release-captain rule does not apply. Extraction is not available -- the file is pure data (own header: \"Pure data; merged by apikey/index.ts via spread\") and already split into 6 family files under apikey/. Same precedent as _rebaseline_2026_08_14_imagetotext_servicekinds (#10275/#10291, gateways.ts 1250->1255, data lines only) and _rebaseline_2026_08_11_v3850_merge_storm_provider_registry (owner-authorized for this same file).", "open-sse/executors/commandCode.ts": 1059, "_rebaseline_2026_08_21_10859_vision_bridge_catalog": "#10859 own growth (Vision Bridge fixes #10808/#10809): src/lib/modelCapabilities.ts 1006->1016 (+10, cmd/gpt-5.3-codex* text-only capability resolution) and open-sse/executors/commandCode.ts 988->1023 (+35, Command Code wire-model normalization for bare ids + reasoning field fallback for opencode-routed gateways). Cohesive bug fixes at the existing capability-resolution / executor chokepoints; not extractable mid-fix. Covered by tests/unit/model-capabilities-command-code-codex-textonly-10703.test.ts, tests/unit/command-code-vision.test.ts, tests/unit/opencode-mimo-reasoning-details-nonstream.test.ts. Pushed directly to release (own-session miss: the original rebaseline was made in a throwaway validation worktree and never landed on the PR branch or the release before merge).", "_rebaseline_2026_08_21_10907_sticky_pin_clear": "#10907 own growth: open-sse/executors/commandCode.ts 1023->1038 (+15, effort-suffix sanitization threading for the sticky-pin-clear fix). Cohesive change at the existing executor chokepoint. Covered by tests/unit/command-code-executor.test.ts.", "_rebaseline_2026_08_21_10986_reasoning_only_content": "#10986 own growth: open-sse/executors/commandCode.ts 1038->1059 (+21, reasoning-only content fallback — when upstream emits only reasoning-delta events and never a text-delta, surface the reasoning text as message.content in createJsonResponse and emit a synthetic content delta in createStreamResponse). Cohesive bug fix at the existing executor chokepoint (mirrors precedent style of #10907/#10859). Covered by tests/unit/command-code-executor.test.ts (2 new cases: non-stream + streaming).", - "_rebaseline_2026_08_21_11069_m365_har_import": "#11069 own growth: AddApiKeyModal.tsx 1073->1080 (+7 = Import .har file button for the copilot-m365-web credential modal — M365 is the only provider whose credential (access_token+chathubPath) must be extracted from a DevTools HAR WebSocket URL, added as a new modal affordance). Cohesive UI at the existing modal chokepoint; not extractable. Covered by tests/unit/m365-har-import*.test.ts." + "_rebaseline_2026_08_21_11069_m365_har_import": "#11069 own growth: AddApiKeyModal.tsx 1073->1080 (+7 = Import .har file button for the copilot-m365-web credential modal — M365 is the only provider whose credential (access_token+chathubPath) must be extracted from a DevTools HAR WebSocket URL, added as a new modal affordance). Cohesive UI at the existing modal chokepoint; not extractable. Covered by tests/unit/m365-har-import*.test.ts.", + "_rebaseline_2026_08_23_tip_drift_post_batch0823": "Tip drift after the 2026-08-23 merge wave: chatBodyAdmission.ts 1009->1118 (+109, gate count incl. +1) and auth.ts 3337->3344 (+7), both grown by merges already on origin/release/v3.8.50 (verified identical on the pristine tip) — not by the codex-appserver-hardening PR that carries this bump. Owner pre-authorized baseline bumps 2026-08-22." }, "_rebaseline_base_2026_08_10_proxyfetch": "Base-red fix (green-prs sweep, issue #9985): open-sse/utils/proxyFetch.ts 1207 > cap 1000 — new proxied-TLS fetch helper introduced by the Fal reference-image work. Owner-authorized quick rebaseline to green; structural slim tracked for v3.9.0.", "_rebaseline_2026_07_27_v3849_train2": "Merge-train 2 (7 PRs) — owner-approved 2026-07-27. Single entry: chatCore.ts 4955->5006 (#8595, Responses multi-turn image compaction before the context hard-reject). Genuine irreducible growth at the existing compaction chokepoint in handleChatCore — the PR adds a last-resort retry against the concrete budget plus the estimateFinalInputTokens helper, both wired at the pre-existing call site rather than a new branch. Covered by tests/unit/8560-responses-image-compaction.test.ts (4 tests).", diff --git a/docs/reference/ENVIRONMENT.md b/docs/reference/ENVIRONMENT.md index 11c27a7ee2..2681ba5478 100644 --- a/docs/reference/ENVIRONMENT.md +++ b/docs/reference/ENVIRONMENT.md @@ -742,7 +742,8 @@ REQUEST_TIMEOUT_MS (global override) | `OMNIROUTE_CODEX_APPSERVER_WS_TOKEN_FILE` | _(unset)_ | Path to a file holding the app-server capability token (from `codex app-server --ws-token-file`). Used when `OMNIROUTE_CODEX_APPSERVER_WS_TOKEN` is unset. Per-connection override: `providerSpecificData.codexAppServerTokenFile`. | | `OMNIROUTE_CODEX_APPSERVER_CWD` | `/tmp` | Working directory the app-server turn runs in. Per-connection override: `providerSpecificData.codexAppServerCwd`. | | `OMNIROUTE_CODEX_APPSERVER_APPROVAL` | _(unset)_ | Approval policy passed to the app-server turn (e.g. `never`, `on-request`). Per-connection override: `providerSpecificData.codexAppServerApprovalPolicy`. | -| `OMNIROUTE_CODEX_APPSERVER_SANDBOX` | _(unset)_ | Sandbox policy passed to the app-server turn (e.g. `read-only`, `workspace-write`, `danger-full-access`). Per-connection override: `providerSpecificData.codexAppServerSandbox`. | +| `OMNIROUTE_CODEX_APPSERVER_SANDBOX` | _(unset)_ | Sandbox policy passed to the app-server turn (e.g. `read-only`, `workspace-write`, `danger-full-access`). When unset the executor defaults to `workspace-write` (hardened; previously `danger-full-access`). Per-connection override: `providerSpecificData.codexAppServerSandbox`. | +| `OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE` | `false` | Auto-approve the app-server's own approval prompts (command/file/permission execution on the host). Off by default — prompts are auto-denied; harness tool calls are unaffected (they travel the separate `item/tool/call` passthrough). Accepts `true`/`1`/`yes`. Per-connection override: `providerSpecificData.codexAppServerAutoApprove`. | | `FETCH_HEADERS_TIMEOUT_MS` | = `FETCH_TIMEOUT_MS` | Time to receive response headers. | | `OMNIROUTE_DIRECT_HEADERS_TIMEOUT_MS` | `30000` (30s) | Maximum response-start wait (ms) for each direct no-proxy attempt. A timeout retries once on a fresh socket; set `0` to disable the bound and retain the previous behavior. | | `FETCH_BODY_TIMEOUT_MS` | = `FETCH_TIMEOUT_MS` | Time to receive the full response body. | diff --git a/open-sse/executors/codex-app-server.ts b/open-sse/executors/codex-app-server.ts index 2c62390f28..abd7ef1fd8 100644 --- a/open-sse/executors/codex-app-server.ts +++ b/open-sse/executors/codex-app-server.ts @@ -11,7 +11,7 @@ import { CodexAppServerClient, type CodexAppServerClientOptions, } from "./codex/appServerClient.ts"; -import { resolveAppServerConfig, type CodexAppServerConfig } from "./codex/appServerConfig.ts"; +import { resolveAppServerConfig, resolveThreadStartPolicy, type CodexAppServerConfig } from "./codex/appServerConfig.ts"; import { translateNotification, translateToolCall, @@ -232,13 +232,20 @@ export class CodexAppServerExecutor extends BaseExecutor { "codex_app_server_unconfigured" ); } + // Turn policy (hardened after the #11205 security review): approvalPolicy + // "never", sandbox "workspace-write", autoApprove off unless the operator + // opted in — see resolveThreadStartPolicy. + const policy = resolveThreadStartPolicy(config, psd); const promptText = extractPromptText(input.body); const effort = extractEffort(input.body); const toolMaps = buildAppServerToolMaps(input.body); const hasTools = toolMaps.specs.length > 0; const events = new AsyncEventQueue(); - const client = new CodexAppServerClient(this.clientOptions); + const client = new CodexAppServerClient({ + ...this.clientOptions, + autoApproveApprovals: policy.autoApprove, + }); const run = async () => { let terminated = false; @@ -284,17 +291,16 @@ export class CodexAppServerExecutor extends BaseExecutor { cwd: config.cwd, // OmniRoute is a router: the HARNESS that consumes OmniRoute owns tool // execution and policy. codex must therefore NEVER block a turn waiting - // on its own interactive approval, and its own sandbox must not gate the - // model — the harness decides what actually runs. So we pair - // approvalPolicy:"never" (non-interactive; codex never prompts) with - // sandbox:"danger-full-access" (codex's own sandbox imposes no - // restriction), mirroring codexInstructions.ts:50 ("never + - // danger-full-access = take advantage of it"). Any server→client - // approval request that still arrives is auto-APPROVED by the client - // (see CodexAppServerClient), never denied — denial would sabotage the - // harness's tool calls. Callers can override both via providerSpecificData. - approvalPolicy: config.approvalPolicy ?? "never", - sandbox: config.sandbox ?? "danger-full-access", + // on its own interactive approval (approvalPolicy "never"). Its own + // sandbox defaults to "workspace-write" (hardened after the #11205 + // security review; WAS "danger-full-access") so codex-decided host + // commands are confined to the turn's cwd tree — widen only via an + // explicit operator override. Server→client approval prompts (codex's + // own command/file/permission requests, NOT the harness tool + // passthrough) are auto-DENIED by the client unless the operator opted + // into auto-approval (see CodexAppServerClient). + approvalPolicy: policy.approvalPolicy, + sandbox: policy.sandbox, // INBOUND harness tools → codex. The client tells the app-server which // function tools are available for the thread via the `dynamicTools` // field on thread/start (a DynamicToolSpec[] under the experimental API, diff --git a/open-sse/executors/codex/appServerClient.ts b/open-sse/executors/codex/appServerClient.ts index eaa761fb27..e0fd44ae34 100644 --- a/open-sse/executors/codex/appServerClient.ts +++ b/open-sse/executors/codex/appServerClient.ts @@ -12,9 +12,10 @@ * command / patch / permission. OmniRoute is a ROUTER — the harness that consumes * it owns tool execution and policy — so codex must never stall a turn on its own * interactive approval. Every inbound ServerRequest is always answered: approval - * prompts are auto-APPROVED (so the model's agentic tool calls proceed; the harness - * decides what really runs), and anything else we can't service gets a JSON-RPC - * error so the id is always settled and the turn never hangs. + * prompts are auto-DENIED by default (they gate codex's OWN host execution, not + * the harness's tools; auto-approval is an explicit operator opt-in — hardening + * after the #11205 security review), and anything else we can't service gets a + * JSON-RPC error so the id is always settled and the turn never hangs. */ // wreq-js WebSocket surface (mirrors the private type in codex.ts:71-77). @@ -37,7 +38,12 @@ interface PendingReq { } // The set of ServerRequest methods that are approval prompts (see PROTOCOL-DIGEST -// "Server -> client REQUESTS"). All of these get an auto-denial decision. +// "Server -> client REQUESTS"). All of these get an auto-DENIAL decision unless +// the operator explicitly opted into auto-approval (hardening after the #11205 +// security review): these prompts gate codex's OWN command/file/permission +// execution on the host, NOT the harness's dynamic tools (those travel the +// separate item/tool/call passthrough), so denying by default never sabotages +// harness tool calls — it closes a prompt-injection → host-execution path. const APPROVAL_REQUEST_METHODS = new Set([ "item/commandExecution/requestApproval", "item/fileChange/requestApproval", @@ -47,12 +53,20 @@ const APPROVAL_REQUEST_METHODS = new Set([ ]); const ROUTER_APPROVAL_NOTE = "router: harness-controlled execution"; +const ROUTER_DENIAL_NOTE = + "router: denied by default (set codexAppServerAutoApprove to opt in)"; export interface CodexAppServerClientOptions { /** Transport factory. Defaults to the shared wreq-js websocket() when omitted. */ websocketFn?: CodexAppServerWebsocketFn | null; /** Default per-request timeout (ms). */ defaultTimeoutMs?: number; + /** + * Auto-APPROVE codex's own approval prompts (command/file/permission). + * Defaults to FALSE — prompts are auto-denied. Enable only when the operator + * trusts the app-server deployment to run codex-decided host commands. + */ + autoApproveApprovals?: boolean; } /** @@ -89,11 +103,13 @@ export class CodexAppServerClient { private toolCallHandler: CodexAppServerToolCallHandler | null = null; private readonly websocketFn: CodexAppServerWebsocketFn | null; private readonly defaultTimeoutMs: number; + private readonly autoApproveApprovals: boolean; private closed = false; constructor(options: CodexAppServerClientOptions = {}) { this.websocketFn = options.websocketFn ?? null; this.defaultTimeoutMs = options.defaultTimeoutMs ?? 120_000; + this.autoApproveApprovals = options.autoApproveApprovals === true; } /** @@ -232,22 +248,27 @@ export class CodexAppServerClient { } /** - * Always answer an inbound ServerRequest so its id is settled. Approval prompts - * are auto-APPROVED (OmniRoute is a router; the harness that consumes it owns - * execution policy, so codex's own approval must not block the turn). Anything - * we cannot service gets a JSON-RPC error so the id is still settled. + * Always answer an inbound ServerRequest so its id is settled. Approval + * prompts are auto-DENIED unless the operator opted into auto-approval + * (hardening after the #11205 security review): they gate codex's OWN host + * command/file execution, not the harness's tools. Anything we cannot + * service gets a JSON-RPC error so the id is still settled. */ private answerServerRequest(id: number, method: string): void { if (!this.ws || this.closed) return; if (APPROVAL_REQUEST_METHODS.has(method)) { - // ReviewDecision "approved" — let the model's agentic action proceed. The - // harness downstream of OmniRoute is the real gate. Note the note field is - // advisory; the decision string is what codex acts on. + // ReviewDecision — "denied" by default; "approved" only with the explicit + // operator opt-in. The note field is advisory; the decision string is + // what codex acts on. + const approved = this.autoApproveApprovals; this.ws.send( JSON.stringify({ jsonrpc: "2.0", id, - result: { decision: "approved", note: ROUTER_APPROVAL_NOTE }, + result: { + decision: approved ? "approved" : "denied", + note: approved ? ROUTER_APPROVAL_NOTE : ROUTER_DENIAL_NOTE, + }, }) ); return; diff --git a/open-sse/executors/codex/appServerConfig.ts b/open-sse/executors/codex/appServerConfig.ts index ebf089a79f..772326dab7 100644 --- a/open-sse/executors/codex/appServerConfig.ts +++ b/open-sse/executors/codex/appServerConfig.ts @@ -22,15 +22,19 @@ export interface CodexAppServerConfig { */ approvalPolicy?: string; /** - * Optional codex sandbox override (SandboxMode). Defaults to "danger-full-access" - * in the executor so codex's own sandbox does not gate the model; the harness is - * the real gate. Callers may tighten this per request via providerSpecificData. + * Optional codex sandbox override (SandboxMode). Defaults to "workspace-write" + * in the executor (hardened after the #11205 security review; WAS + * "danger-full-access") so codex's own command/file execution is confined to + * the turn's cwd tree. Widen per connection via providerSpecificData or env. */ sandbox?: string; } type ProviderSpecificData = Record | null | undefined; +/** Where a resolved value came from — the SSRF binding below keys off this. */ +type ConfigSource = "psd" | "env"; + function firstString(...values: unknown[]): string | null { for (const value of values) { if (typeof value === "string" && value.trim().length > 0) return value.trim(); @@ -38,23 +42,47 @@ function firstString(...values: unknown[]): string | null { return null; } +function firstStringWithSource( + psdValue: unknown, + envValue: unknown +): { value: string; source: ConfigSource } | null { + if (typeof psdValue === "string" && psdValue.trim().length > 0) { + return { value: psdValue.trim(), source: "psd" }; + } + if (typeof envValue === "string" && envValue.trim().length > 0) { + return { value: envValue.trim(), source: "env" }; + } + return null; +} + /** * Read the capability token, preferring an inline token, then a token FILE path. * The token file (produced by `codex app-server --ws-token-file `) holds the - * same hex string that is presented as the bearer token. + * same hex string that is presented as the bearer token. The source of the value + * (psd vs env) is tracked for the credential/URL binding rule. */ -function resolveToken(psd: ProviderSpecificData): string | null { - const inline = firstString( - psd?.codexAppServerToken, - process.env.OMNIROUTE_CODEX_APPSERVER_WS_TOKEN - ); - if (inline) return inline; +function resolveTokenWithSource( + psd: ProviderSpecificData +): { value: string; source: ConfigSource } | null { + const inlinePsd = firstString(psd?.codexAppServerToken); + if (inlinePsd) return { value: inlinePsd, source: "psd" }; + const inlineEnv = firstString(process.env.OMNIROUTE_CODEX_APPSERVER_WS_TOKEN); + if (inlineEnv) return { value: inlineEnv, source: "env" }; - const tokenFile = firstString( - psd?.codexAppServerTokenFile, - process.env.OMNIROUTE_CODEX_APPSERVER_WS_TOKEN_FILE - ); - if (!tokenFile) return null; + const filePsd = firstString(psd?.codexAppServerTokenFile); + if (filePsd) { + const contents = readTokenFile(filePsd); + if (contents) return { value: contents, source: "psd" }; + } + const fileEnv = firstString(process.env.OMNIROUTE_CODEX_APPSERVER_WS_TOKEN_FILE); + if (fileEnv) { + const contents = readTokenFile(fileEnv); + if (contents) return { value: contents, source: "env" }; + } + return null; +} + +function readTokenFile(tokenFile: string): string | null { try { const contents = readFileSync(tokenFile, "utf8").trim(); return contents.length > 0 ? contents : null; @@ -67,18 +95,80 @@ function isWebSocketUrl(url: string): boolean { return url.startsWith("ws://") || url.startsWith("wss://"); } +function urlHostname(url: string): string | null { + try { + return new URL(url).hostname || null; + } catch { + return null; + } +} + +/** + * Is this hostname inside the operator's own network? Used by the + * credential/URL binding rule: an ENV-sourced capability token (the operator's + * shared secret, not visible to whoever wrote a connection's + * providerSpecificData) may only be sent to env-configured URLs or to + * operator-local hosts. Literal addresses only — no DNS resolution, so a + * public hostname can never smuggle an env token out via DNS. Single-label + * names (`ts-egress`) resolve via the operator's own hosts/mDNS and count as + * local; dotted names must carry a known-local suffix. + */ +export function isLocalAppServerHost(hostname: string): boolean { + const h = hostname + .trim() + .toLowerCase() + .replace(/^\[|\]$/g, ""); + if (!h) return false; + if (h === "localhost" || h.endsWith(".localhost")) return true; + if (h.endsWith(".local") || h.endsWith(".ts.net") || h.endsWith(".internal")) return true; + if (h.includes(":")) { + // IPv6: loopback, ULA (fc00::/7), link-local (fe80::/10) + if (h === "::1") return true; + return /^f[cd]/.test(h) || /^fe[89ab]/.test(h); + } + const m = /^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/.exec(h); + if (m) { + const a = Number(m[1]); + const b = Number(m[2]); + if (a === 10 || a === 127) return true; + if (a === 172 && b >= 16 && b <= 31) return true; + if (a === 192 && b === 168) return true; + if (a === 169 && b === 254) return true; + return false; + } + // single-label hostname (no dots): LAN/hosts-file/mDNS name + if (!h.includes(".")) return true; + return false; +} + /** * Resolve the app-server connection config from providerSpecificData with env * fallbacks. Returns `null` when not fully configured (URL + token both required) * so the gating predicate `isCodexAppServerRequired` stays false and Codex falls * back to its other transports. + * + * CREDENTIAL/URL BINDING (hardening after the #11205 security review): an + * env-sourced token is the operator's shared secret. It is only ever paired + * with (a) an env-sourced URL, or (b) an operator-local host + * (isLocalAppServerHost). A providerSpecificData URL pointing at an outside + * host combined with an env token is refused (returns null) — otherwise anyone + * able to write a connection could exfiltrate the env credential. A + * psd-sourced token may go anywhere: whoever wrote the psd already knows it. */ export function resolveAppServerConfig(psd: ProviderSpecificData): CodexAppServerConfig | null { - const url = firstString(psd?.codexAppServerUrl, process.env.OMNIROUTE_CODEX_APPSERVER_WS); - if (!url || !isWebSocketUrl(url)) return null; + const urlRes = firstStringWithSource(psd?.codexAppServerUrl, process.env.OMNIROUTE_CODEX_APPSERVER_WS); + if (!urlRes || !isWebSocketUrl(urlRes.value)) return null; - const token = resolveToken(psd); - if (!token) return null; + const tokenRes = resolveTokenWithSource(psd); + if (!tokenRes) return null; + + if (tokenRes.source === "env" && urlRes.source === "psd") { + const host = urlHostname(urlRes.value); + if (!host || !isLocalAppServerHost(host)) return null; + } + + const url = urlRes.value; + const token = tokenRes.value; const cwd = firstString(psd?.codexAppServerCwd, process.env.OMNIROUTE_CODEX_APPSERVER_CWD) ?? "/tmp"; @@ -92,3 +182,35 @@ export function resolveAppServerConfig(psd: ProviderSpecificData): CodexAppServe return { url, token, cwd, ...(approvalPolicy ? { approvalPolicy } : {}), ...(sandbox ? { sandbox } : {}) }; } + +/** + * The turn/start policy triple for a resolved config (hardening after the + * #11205 security review): + * - approvalPolicy defaults to "never": codex must not block a router turn on + * its own interactive approval (unchanged). + * - sandbox defaults to "workspace-write" (WAS "danger-full-access"): codex's + * own command/file execution is confined to the turn's cwd tree unless the + * operator explicitly widens it (providerSpecificData.codexAppServerSandbox / + * OMNIROUTE_CODEX_APPSERVER_SANDBOX). With "never" + a permissive sandbox, + * codex would run model-decided commands on the host with no gate at all. + * - autoApprove defaults to false: server→client approval prompts are answered + * "denied" unless the operator opts in via + * providerSpecificData.codexAppServerAutoApprove ("true"/"1"/"yes") or + * OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE. Harness tool calls are unaffected — + * they travel the separate item/tool/call passthrough. + */ +export function resolveThreadStartPolicy( + config: CodexAppServerConfig, + psd: ProviderSpecificData +): { approvalPolicy: string; sandbox: string; autoApprove: boolean } { + const raw = firstString( + psd?.codexAppServerAutoApprove, + process.env.OMNIROUTE_CODEX_APPSERVER_AUTO_APPROVE + ); + const autoApprove = raw === "true" || raw === "1" || raw === "yes"; + return { + approvalPolicy: config.approvalPolicy ?? "never", + sandbox: config.sandbox ?? "workspace-write", + autoApprove, + }; +} diff --git a/src/app/api/providers/[id]/test/codexAppServerHealth.ts b/src/app/api/providers/[id]/test/codexAppServerHealth.ts index ff825bf9fb..8476d9e832 100644 --- a/src/app/api/providers/[id]/test/codexAppServerHealth.ts +++ b/src/app/api/providers/[id]/test/codexAppServerHealth.ts @@ -61,7 +61,11 @@ export async function testCodexAppServerConnection( ); const config = resolveAppServerConfig(psd); if (!config) { - const error = "Codex app-server transport is not configured (missing url or token)"; + // Also reached when the credential/URL binding refused (env token + remote + // psd URL) — the resolve deliberately returns null there so the token can + // never leave the operator's network (see appServerConfig.ts). + const error = + "Codex app-server transport is not configured (missing url/token, or the env-token/remote-URL binding was refused)"; return { valid: false, error, @@ -81,6 +85,10 @@ export async function testCodexAppServerConnection( method: "GET", headers: { Authorization: `Bearer ${config.token}` }, signal: controller.signal, + // Never follow redirects carrying the bearer token (SSRF hardening after + // the #11205 security review): a 30x to an outside host would exfiltrate + // the capability token. A redirect response is simply "not ready". + redirect: "manual", }); if (res.status !== 200) { const error = `Codex app-server not ready (${readyzUrl} → HTTP ${res.status})`; diff --git a/stryker.conf.json b/stryker.conf.json index 33428a46da..f9317d95a0 100644 --- a/stryker.conf.json +++ b/stryker.conf.json @@ -89,6 +89,7 @@ "tests/unit/auth-terminal-status.test.ts", "tests/unit/authz/discovery-routes-local-only.test.ts", "tests/unit/authz/oauth-autoimport-local-only.test.ts", + "tests/unit/quota-exhaustion-cutoff-opencode.test.ts", "tests/unit/authz/route-guard-local-prefix.test.ts", "tests/unit/authz/route-guard-skills-collect.test.ts", "tests/unit/authz/route-guard-version-get-exemption.test.ts", diff --git a/tests/unit/codex-app-server.test.ts b/tests/unit/codex-app-server.test.ts index 28445baad2..3adf4980b5 100644 --- a/tests/unit/codex-app-server.test.ts +++ b/tests/unit/codex-app-server.test.ts @@ -213,9 +213,9 @@ test("mapUsage: converts snake_case token counts", () => { assert.equal(mapUsage(undefined), undefined); }); -// ── Client: stall-guard auto-approval ─────────────────────────────────────── +// ── Client: approval stall-guard (deny-by-default, opt-in approve) ───────── -test("CodexAppServerClient: server approval request is auto-approved", async () => { +test("CodexAppServerClient: server approval request is auto-DENIED by default (#11205 hardening)", async () => { const ctrl = makeFakeSocket(); const { fn } = fakeTransport(ctrl); const client = new CodexAppServerClient({ websocketFn: fn }); @@ -234,12 +234,30 @@ test("CodexAppServerClient: server approval request is auto-approved", async () const reply = ctrl.sent.find((f) => f.id === 99); assert.ok(reply, "client must reply to the server approval request"); - // OmniRoute is a router: approvals are auto-APPROVED so the model's agentic - // tool calls proceed; the harness downstream is the real execution gate. - assert.equal( - (reply!.result as Record).decision, - "approved" - ); + // Security contract (post-#11205 review): codex's OWN command/file/permission + // executions are denied by default — approval prompts are NOT the harness + // tool-call passthrough (that path is item/tool/call, handled separately), so + // denying never sabotages harness tools. Blanket auto-approve + a permissive + // sandbox is a confused-deputy for prompt-injected turns. + assert.equal((reply!.result as Record).decision, "denied"); +}); + +test("CodexAppServerClient: approval request is auto-approved only with explicit opt-in", async () => { + const ctrl = makeFakeSocket(); + const { fn } = fakeTransport(ctrl); + const client = new CodexAppServerClient({ websocketFn: fn, autoApproveApprovals: true }); + await client.connect("ws://x", "tok"); + + ctrl.emit({ + jsonrpc: "2.0", + id: 100, + method: "item/fileChange/requestApproval", + params: { changes: [] }, + }); + + const reply = ctrl.sent.find((f) => f.id === 100); + assert.ok(reply, "client must reply to the server approval request"); + assert.equal((reply!.result as Record).decision, "approved"); }); test("CodexAppServerClient: non-approval server request gets a JSON-RPC error", async () => { @@ -332,11 +350,13 @@ test("CodexAppServerExecutor: streaming turn emits initialize → thread/start assert.deepEqual(lifecycle, ["initialize", "thread/start", "turn/start"]); // thread/start carried the router defaults: approvalPolicy:"never" (codex - // never blocks on its own approval) + sandbox:"danger-full-access" (codex's - // own sandbox does not gate the model; the harness is the real execution gate). + // never blocks on its own approval) + sandbox:"workspace-write" — hardened + // default post-#11205 security review (was "danger-full-access"): codex's own + // sandbox now confines writes to the turn's cwd tree unless the operator + // explicitly opts back into a wider sandbox via providerSpecificData/env. const threadStart = sent.find((f) => f.method === "thread/start"); assert.equal((threadStart!.params as Record).approvalPolicy, "never"); - assert.equal((threadStart!.params as Record).sandbox, "danger-full-access"); + assert.equal((threadStart!.params as Record).sandbox, "workspace-write"); // turn/start carried the text input with text_elements:[] const turnStart = sent.find((f) => f.method === "turn/start"); @@ -700,3 +720,173 @@ test("probeCodexAppServerAuth: no transport → unknown (does not throw)", async assert.equal(status.state, "unknown"); }); + +// ── Security hardening (#11205 post-merge review) ─────────────────────────── +// Two findings from the automated push review on the original #11205 merge: +// (1) the readyz health probe sent the bearer token to any URL a connection +// config pointed at, following redirects (SSRF / credential exfil); +// (2) env-sourced credentials were happily paired with a +// providerSpecificData-sourced URL, so anyone able to write a connection +// could harvest the operator's env token. +// The binding rule: env-sourced tokens are only sent to env-sourced URLs or to +// operator-local hosts (loopback / RFC1918 / link-local / ULA / localhost / +// single-label LAN names / *.local / *.ts.net / *.internal). A psd-sourced +// token may go anywhere — whoever wrote the psd already knows it. + +function withEnv(vars: Record, fn: () => T): T { + const prev: Record = {}; + for (const k of Object.keys(vars)) { + prev[k] = process.env[k]; + if (vars[k] === undefined) delete process.env[k]; + else process.env[k] = vars[k]; + } + try { + return fn(); + } finally { + for (const k of Object.keys(vars)) { + if (prev[k] === undefined) delete process.env[k]; + else process.env[k] = prev[k]; + } + } +} + +const BINDING_ENV_KEYS = { + OMNIROUTE_CODEX_APPSERVER_WS: undefined, + OMNIROUTE_CODEX_APPSERVER_WS_TOKEN: "env-token-hex", + OMNIROUTE_CODEX_APPSERVER_WS_TOKEN_FILE: undefined, +} as const; + +test("resolveAppServerConfig: refuses env token → remote psd URL (SSRF binding)", () => { + withEnv({ ...BINDING_ENV_KEYS }, () => { + // attacker/lower-priv connection config points the URL at an outside host; + // the env token must NOT be attached → unconfigured (null), feature off. + assert.equal( + resolveAppServerConfig({ + codexTransport: "app-server", + codexAppServerUrl: "wss://evil.example.com:8443", + }), + null + ); + // dotted hostnames are not local even when they look benign + assert.equal( + resolveAppServerConfig({ + codexTransport: "app-server", + codexAppServerUrl: "ws://appserver.evil-corp.io:1456", + }), + null + ); + }); +}); + +test("resolveAppServerConfig: env token allowed to operator-local psd URLs", () => { + withEnv({ ...BINDING_ENV_KEYS }, () => { + const localUrls = [ + "ws://127.0.0.1:1456", + "ws://localhost:1456", + "ws://[::1]:1456", + "ws://10.0.0.5:1456", + "ws://172.16.3.4:1456", + "ws://192.168.0.15:1456", + "ws://169.254.1.1:1456", + "ws://ts-egress:1456", // single-label LAN/hosts-file name + "ws://codex.local:1456", + "ws://node1.ts.net:1456", + "ws://sidecar.internal:1456", + ]; + for (const url of localUrls) { + const cfg = resolveAppServerConfig({ codexTransport: "app-server", codexAppServerUrl: url }); + assert.ok(cfg, `expected env token to bind to local URL ${url}`); + assert.equal(cfg!.token, "env-token-hex"); + } + }); +}); + +test("resolveAppServerConfig: psd-sourced token may pair with any psd URL", () => { + withEnv( + { + OMNIROUTE_CODEX_APPSERVER_WS: undefined, + OMNIROUTE_CODEX_APPSERVER_WS_TOKEN: undefined, + OMNIROUTE_CODEX_APPSERVER_WS_TOKEN_FILE: undefined, + }, + () => { + const cfg = resolveAppServerConfig({ + codexTransport: "app-server", + codexAppServerUrl: "wss://codex.remote.example.com:443", + codexAppServerToken: "psd-token", + }); + assert.ok(cfg, "psd token + psd URL is self-consistent, allowed"); + assert.equal(cfg!.token, "psd-token"); + } + ); +}); + +test("resolveAppServerConfig: env URL + env token pairs regardless of host", () => { + withEnv( + { + OMNIROUTE_CODEX_APPSERVER_WS: "wss://codex-remote.example.com:8443", + OMNIROUTE_CODEX_APPSERVER_WS_TOKEN: "env-token-hex", + OMNIROUTE_CODEX_APPSERVER_WS_TOKEN_FILE: undefined, + }, + () => { + const cfg = resolveAppServerConfig({ codexTransport: "app-server" }); + assert.ok(cfg, "operator's own env pair is self-consistent, allowed"); + assert.equal(cfg!.url, "wss://codex-remote.example.com:8443"); + } + ); +}); + +// ── Health probe: redirect pinning + binding inheritance ──────────────────── + +test("testCodexAppServerConnection: readyz probe pins redirects (no token leak via 30x)", async () => { + const { testCodexAppServerConnection } = await import( + "../../src/app/api/providers/[id]/test/codexAppServerHealth.ts" + ); + const originalFetch = globalThis.fetch; + const seen: Array<{ url: string; init?: RequestInit }> = []; + globalThis.fetch = (async (url: unknown, init?: RequestInit) => { + seen.push({ url: String(url), init }); + return new Response("not ready", { status: 503 }); + }) as typeof fetch; + try { + const result = await testCodexAppServerConnection({ + provider: "codex-app-server", + providerSpecificData: { + codexAppServerUrl: "ws://127.0.0.1:1456", + codexAppServerToken: "deadbeef", + }, + }); + assert.ok(result, "app-server provider must take the readyz path"); + assert.equal(result!.valid, false); + assert.equal(seen.length, 1); + assert.equal(seen[0].url, "http://127.0.0.1:1456/readyz"); + assert.equal(seen[0].init?.redirect, "manual", "bearer token must never follow a redirect"); + } finally { + globalThis.fetch = originalFetch; + } +}); + +test("testCodexAppServerConnection: env token + remote psd URL reports unconfigured, no network", async () => { + const { testCodexAppServerConnection } = await import( + "../../src/app/api/providers/[id]/test/codexAppServerHealth.ts" + ); + const originalFetch = globalThis.fetch; + let fetched = false; + globalThis.fetch = (async () => { + fetched = true; + return new Response("ok", { status: 200 }); + }) as typeof fetch; + try { + await withEnv({ ...BINDING_ENV_KEYS }, async () => { + const result = await testCodexAppServerConnection({ + provider: "codex-app-server", + providerSpecificData: { codexAppServerUrl: "wss://evil.example.com:8443" }, + }); + assert.ok(result); + assert.equal(result!.valid, false); + assert.match(String((result!.diagnosis as { code?: string })?.code), /app_server_unconfigured/); + }); + assert.equal(fetched, false, "binding refusal must happen before any network call"); + } finally { + globalThis.fetch = originalFetch; + } +});