feat(server): optional unauthenticated loopback listener for direct-spawn Codex (#1102) - #1220
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
03bc98d to
9d95125
Compare
ea8f5d0 to
89e32ae
Compare
Direct-spawn activation evidenceThe audit named this the ready gate. Here it is, from a real The last line is the claim of #1102: a real app-server, spawned from the resolved entrypoint with no token in its environment, opened On the oracle. Codex caches model lists and falls back to a bundled catalog when a refresh fails, so "did Two things writing it surfaced. Ablation. Pointing The script is committed at Marking ready for review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d7e0ca751
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| > **개정 이력.** 첫 판은 "opt-in 으로 loopback 소켓 피어를 무인증 admit" 을 | ||
| > 제안했다. 독립 감사가 P1 다섯 건으로 되돌렸고, 그중 둘이 설계를 바꿨다: | ||
| > (a) 그 스위치는 `resolveApiAuth` 를 타고 #1102 와 무관한 8개 엔드포인트까지 | ||
| > 열고, (b) 공용 리스너의 피어 주소는 최종 사용자 신원이 아니다. 아래는 |
There was a problem hiding this comment.
This new _plan document records pre-merge security audit findings, P1 severity assessments, admission-bypass reasoning, and threat/deployment paths. Because devlog/ is tracked and public, this material should remain in .tmp/ or another scratch directory until the fix and any advisory are public; only the published outcome should enter the repository.
AGENTS.md reference: AGENTS.md:L61-L68
Useful? React with 👍 / 👎.
| const loopback = config?.unauthenticatedLoopbackListener; | ||
| if (loopback?.enabled) port = loopback.port; |
There was a problem hiding this comment.
When the public listener uses a specific non-loopback hostname such as 192.168.1.10, this substitutes only the loopback port; the later setRootOpenaiBaseUrl(..., config?.hostname) still emits http://192.168.1.10:<loopback-port>/v1. Unlike wildcard hosts, providerBaseHost preserves that address, while the new socket listens only on 127.0.0.1, so directly spawned Codex processes get connection refusals and the advertised feature fails. Use 127.0.0.1 as the injected host whenever this listener is selected.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| // credential (#1102). Emitting the env header anyway would be worse than useless: the | ||
| // directly-spawned app-server this exists for has no OPENCODEX_API_AUTH_TOKEN in its | ||
| // environment, and Codex would send an empty header value. | ||
| if (config?.unauthenticatedLoopbackListener?.enabled) return false; |
There was a problem hiding this comment.
With a remote public bind and this option enabled, returning false here also changes unrelated callers in src/clients/config-export.ts: opencodeProviderOptions and proxyAdmissionHeaders stop emitting x-opencodex-api-key. Both CLI export (src/cli/export-command.ts:176) and management export (src/server/management/model-routes.ts:183-186) still point those clients at the authenticated public listener, so existing OpenCode, Hermes, and OpenClaw generated configs begin receiving 401 responses. Keep this helper describing the target/public listener, and special-case the unauthenticated listener only in Codex injection or make the target listener explicit.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| const proxyPort = (value as Record<string, unknown>).port; | ||
| if (typeof proxyPort === "number" && proxyPort === listenerPort) { | ||
| return "schema_invalid: unauthenticatedLoopbackListener.port: must differ from the proxy port"; |
There was a problem hiding this comment.
When the config omits port, the schema supplies the proxy default 10100, but this comparison only runs when the raw candidate contains a numeric port. Therefore ocx config set unauthenticatedLoopbackListener '{"enabled":true,"port":10100}' can report success for a config whose two effective ports are identical; the next startup then fails in chooseListenPort despite write-time validation promising to reject this relationship. Compare the listener port against the effective defaulted proxy port as well.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| `ocx sync` then writes `base_url = "http://127.0.0.1:10200/v1"` into the managed Codex provider block | ||
| and omits the auth header, so a directly spawned app-server works without any credential plumbing. |
There was a problem hiding this comment.
When this setting is added while the proxy is already running, startServer has already captured the old listener configuration and no socket is created on the new port, while ocx sync reloads the persisted config and immediately writes that port into Codex. Following this documented sequence therefore repoints app-servers to a refused connection until the proxy restarts. Instruct users to restart first—or rely on startup sync after restarting—instead of presenting ocx sync alone as sufficient.
AGENTS.md reference: docs-site/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| const probe = spawnSync(process.execPath, [ | ||
| "-e", | ||
| "process.stdout.write(require.resolve('@openai/codex/bin/codex.js'))", | ||
| ], { encoding: "utf8" }); | ||
| if (probe.status === 0 && probe.stdout.trim()) return probe.stdout.trim(); | ||
| const which = spawnSync("readlink", ["-f", spawnSync("which", ["codex"], { encoding: "utf8" }).stdout.trim()], { encoding: "utf8" }); | ||
| const path = which.stdout.trim(); | ||
| if (!path) throw new Error("cannot resolve @openai/codex/bin/codex.js"); |
There was a problem hiding this comment.
The fallback uses Unix-only which and readlink, so it cannot run on supported Windows hosts when the repository-local require.resolve fails, which is expected because this repository does not depend on @openai/codex. On Unix it can also return the generated OpenCodex shell shim when that shim owns PATH, after which invoking the result through node does not exercise the direct JavaScript entrypoint this probe claims to verify. Resolve the installed package through a cross-platform package/global-install or recorded-shim-backup path and explicitly reject managed shims.
AGENTS.md reference: scripts/AGENTS.md:L14-L15
Useful? React with 👍 / 👎.
| const hitResponses = seen.some(entry => entry.includes("/v1/responses")); | ||
| record( | ||
| "the app-server reached the loopback listener without a credential", | ||
| hitModels || hitResponses, | ||
| seen.slice(0, 8).join(" | ") || "no requests observed", |
There was a problem hiding this comment.
If Codex refreshes GET /v1/models successfully but turn/start is rejected locally or never dispatches a model request, hitModels makes this final check pass even though no /v1/responses request reached the listener. The script does not otherwise mark a missing or failed turn/start response as a failed step, so it can exit 0 without proving the direct-spawn model-call path that the feature exists to repair. Require hitResponses here; model discovery is already checked separately.
AGENTS.md reference: scripts/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
The design changed twice under audit. The first proposal was an opt-in switch that would have admitted loopback socket peers on a remote bind; it rode resolveApiAuth into eight endpoints unrelated to #1102, and a public listener's peer address only proves the last transport hop, which Docker Desktop, host-network containers, WSL mirrored networking and tunnels all terminate locally. The shipped design leaves public admission untouched and opens a separate 127.0.0.1-bound listener, so the kernel refuses remote connections instead of us judging addresses. Later rounds closed the implementation contracts: a fixed port (an ephemeral one would manufacture the restart breakage the issue claimed and we disproved), a per-request auth/CORS policy view narrow enough that it cannot masquerade as business config, one startup transaction across both binds, composite stop that completes cleanup AND propagates failure, and GET /v1/models on the allowlist because Codex falls back to it when no catalog is installed. The last blocker was the sharpest: the /v1/models ablation would not have gone red, because the models-manager catches refresh failures and returns its bundled list. The acceptance test now turns on a runtime-generated unique routed model that no bundled catalog can synthesize.
…pawn Codex (#1102) A `codex app-server` launched by a host that resolves the entrypoint directly never passes through the generated shim, so it never inherits OPENCODEX_API_AUTH_TOKEN. On a wildcard bind every model call then 401s at admission, before any SSE frame. The tempting fix — exempt callers whose socket peer looks like loopback — is unsound. `requestIP()` proves only the last transport hop, and Docker Desktop port forwarding, host-network containers, WSL mirrored networking and tunnel terminators all re-open remote connections locally. It would also have ridden `resolveApiAuth` into eight endpoints unrelated to this issue. Instead a second listener binds 127.0.0.1. The kernel refuses remote connections outright, so there is no address to judge, and the public listener's admission policy is byte-for-byte unchanged. The parts that are load-bearing: Auth and CORS read a `RequestPolicyView` — a Pick of hostname, corsAllowOrigins and apiKeys — chosen per request from the receiving listener. Rewriting the whole config and holding it would go stale on the next management change; adding an `allowUnauthenticated` parameter to the resolvers would put an admission bypass on the wrong side of the boundary. The narrow type also means a policy view that leaks into a routing path fails to typecheck. The bind is loopback but the boundary is not the bind alone: an attacker page can make a victim's browser connect to 127.0.0.1. The listener therefore takes the same Host/Origin branch a plain loopback bind always has. A test asserts the same hostile Origin that the loopback policy rejects would be accepted under the public policy. The port is required in config, never OS-assigned. An ephemeral port would change across restarts while running app-servers kept the old base_url — the exact symptom this issue reported for token rotation, which does not actually happen. `GET /v1/models` is on the four-route allowlist because when catalog materialization fails, `syncCodex` injects with `catalogPath: null` and Codex falls back to an online model manager that refreshes through it. Both binds are one startup transaction, and composite stop completes cleanup on both listeners while still propagating failure — swallowing it would let drainAndShutdown report success while a socket is held. Off by default. When on, every local process can spend account quota and paid provider credentials, which the startup warning and the docs say plainly. Ablation: reverting the policy view to the shared config makes 3 tests red including the hostile-Host case; dropping the auth-header rule makes 1 red; removing the port validator makes 2 red; rewriting the public bind to a literal 127.0.0.1 makes the F4 symmetry guard red. Refs #1102
…eserve its port The audit found the previous commit shipped an unauthenticated surface whose tests never started a server. Every assertion ran against pure helpers, so the listener could fail to open, bind the wrong address, lose the identity comparison against the public server, or serve routes outside its allowlist, and the suite would stay green. That is not a coverage gap; it is a missing oracle for a security boundary. Ten integration tests now speak HTTP to real listeners: the same request answered 401 on the public socket and 200 on the loopback one, a connection to the machine's non-loopback IPv4 being refused, management and dashboard and unrelated data-plane routes returning 404 while an allowlisted one returns 200, the loopback Host/Origin gate rejecting the rebinding shape the public policy would accept, both ports rebindable after stop, and a squatted loopback port rolling back the public bind. Ablation, all driven red rather than argued: binding the loopback listener to 0.0.0.0 fails 2; removing the allowlist check fails 1; collapsing the policy view to the shared config fails 3. The second finding was a real gap rather than a test one. `ocx start --port <loopback port>` would bind the public listener onto the address the loopback listener is configured for, and the loopback bind would then fail EADDRINUSE — rolling back a startup whose only problem was a config collision. `findAvailablePort` takes a `reservedPort`: an explicit preference for it is refused up front rather than retried, and ephemeral selection redraws if the OS hands it back. Two smaller corrections. `drainingResponse` and `serverBusyResponse` run before the auth checks and were still using the shared config, so a 503 on the loopback listener could echo a hostile origin; both now take the receiving listener's policy. And a non-boolean `enabled` was silently deleted by the schema's catch, leaving an operator convinced they had enabled a listener that was off — write-time now rejects it. Refs #1102
…ch the real inject wiring Two of the previous tests were watching nothing, and the audit was right about both. The allowlist check probed POST routes with GET. Chat Completions, Messages, Images, search and Live all 404 on method mismatch inside their handlers, so widening the allowlist to admit one of them would have kept the assertion green. Each route is now requested with the method its handler accepts, plus two cases proving the allowlisted paths still reject methods they do not serve. Ablation: adding /v1/messages to the allowlist now fails it. The injection test handed the loopback port straight to buildProviderTableBlock, so deleting the substitution inside injectCodexConfig changed nothing it observed. It now runs the real injector in a subprocess — CODEX_CONFIG_PATH resolves at module load, so an in-process CODEX_HOME would write somewhere else — passes the PUBLIC port the way every caller does, and reads the written config.toml. Ablation: removing the substitution fails it. Also added: POST /v1/responses and /v1/responses/compact admitted on the loopback listener and 401 on the public one, asserted as neither 401 nor 404 because "not 401" alone would survive removing the route; a Responses WebSocket handshake on both listeners; and a rollback test that binds a fixed public port and rebinds it after the throw, since throwing while leaving the public listener up is the failure the rollback exists to prevent. Two honest gaps are recorded rather than papered over. The WebSocket test cannot defend requestServer.upgrade over server.upgrade: that ablation stayed green because this Bun version accepts an upgrade issued from a sibling Bun.serve. And composite stop's failure propagation has no test, because the composite captures the underlying stop at construction and there is no seam to inject a rejection through; its sibling property, cleanup across both listeners, is covered. Port selection fixes from the same review: the ephemeral redraw is now a bounded loop behind an injectable allocator rather than unbounded async recursion, and it has tests that actually reach the redraw branch. An explicit --port collision with the reserved port is rejected before the 60-second reclaim path instead of after it, with a message naming the real cause. runAdmittedHttpTurn now takes the policy explicitly, so no CORS-emitting helper falls back to the shared config. Refs #1102
…two blind seams The shutdown orchestration held two properties that pull against each other — keep cleaning up after a failure, yet still report it — and neither was testable in place, because the composite captures the underlying stop at construction. Extracted to `runListenerShutdown`, which now has four cases including both failure shapes. Ablation: removing the per-step catch fails 2, swallowing the collected failures fails 3. Two seams have no runtime oracle on this Bun version, and both would regress silently. Swapping `requestServer.upgrade` for `server.upgrade` stays green because this Bun accepts an upgrade issued from a sibling Bun.serve; another version is not promised to, and the loopback listener would then fail to upgrade at all. And the connection-refused test degrades to a warning on a host with no external IPv4, so the 0.0.0.0 ablation would pass there. Source assertions are a weak instrument, but one aimed at a known blind spot beats a comment nobody runs — and the upgrade assertion does go red on that swap. Smaller review fixes: the WebSocket handshake helper now clears its timer and settles once, so a late timeout cannot fire into the next test; and the rollback test draws its public port with the loopback port reserved, since two back-to-back freePort() calls can return the same port and the test would squat itself. Refs #1102
The audit named this the ready gate, and it was right to. Every existing test proves a piece — admission, the route allowlist, CORS, the bind scope, the injected port — and none of them prove the thing the feature exists for: that a real `codex app-server`, spawned the way a third-party host spawns it, reaches the proxy without a credential. That seam is between two processes. Not a `bun test` file. The repository does not depend on `@openai/codex`, so a test that skips when it is absent would report green on machines that never ran it. This fails loudly and its output is the evidence. The oracle is a routed model id generated at run time. Codex caches model lists and falls back to a bundled catalog when a refresh fails, so "did model/list succeed" proves nothing — a broken path looks identical to a working one. A name no bundled catalog can contain can only have arrived through our listener. Writing it surfaced two things worth recording. `model/list` reads `model_catalog_json`; it does not call the provider's `/v1/models`, so the first version watched Codex return its five bundled ids while never touching the listener — the exact false-negative shape the unique id exists to expose, pointed at the harness instead of the feature. And a hand-written catalog fixture is a liability: Codex rejects the whole file on any schema mismatch and silently falls back, so the catalog is now built with our own serializer, which also means the script exercises the bytes `ocx sync` writes. Result on Codex 0.146.0, isolated CODEX_HOME with no models_cache.json and OPENCODEX_API_AUTH_TOKEN stripped from the child environment: 9/9, ending with POST /v1/responses observed on the loopback listener. Ablation: pointing base_url at a dead port makes the last check red, so the harness is watching the hop rather than asserting its own setup. Refs #1102
89e32ae to
fca2cd7
Compare
0d7e0ca to
912cb99
Compare
1 participant
Summary
A
codex app-serverlaunched by a host that resolves the entrypoint directly never passes through the generated shim, so it never inheritsOPENCODEX_API_AUTH_TOKEN. On a wildcard bind every model call then 401s at admission, before any stream frame.The tempting fix is to exempt callers whose socket peer looks like loopback. That is unsound:
requestIP()proves only the last transport hop, and Docker Desktop port forwarding, host-network containers, WSL mirrored networking and tunnel terminators all re-open remote connections locally. It would also have riddenresolveApiAuthinto eight endpoints unrelated to this issue.Instead a second listener binds
127.0.0.1. The kernel refuses remote connections outright, so there is no address to judge, and the public listener's admission policy is unchanged.Off by default.
unauthenticatedLoopbackListener: { enabled: true, port: N }.What is load-bearing
A per-request policy view. Auth and CORS read a
Pick<OcxConfig, "hostname" | "corsAllowOrigins" | "apiKeys">chosen from the receiving listener. Rewriting the whole config and holding it would go stale on the next management change; adding anallowUnauthenticatedparameter to the resolvers would put an admission bypass on the wrong side of the boundary. The narrow type also means a policy view leaking into a routing path fails to typecheck.The bind is not the whole boundary. An attacker page can make a victim's browser connect to
127.0.0.1, and that connection is local. The listener takes the same Host/Origin branch a plain loopback bind always has. A test asserts the same hostile Origin the loopback policy rejects would be accepted under the public policy.A fixed port. Never OS-assigned. An ephemeral port would change across restarts while running app-servers kept the old
base_url— the exact symptom this issue reported for token rotation, which investigation showed does not actually happen (writeServiceApiTokenFilemirrors an already-present env value, only from install/repair).GET /v1/modelsis on the four-route allowlist. When catalog materialization fails,syncCodexinjects withcatalogPath: nulland Codex falls back to an online model manager that refreshes through it. Returning 404 would fix the direct-spawn host while breaking its model list.One startup transaction. If the loopback bind fails, the public listener rolls back rather than being stranded — otherwise the CLI's port retry reads it as a public-port conflict and picks a different port, accumulating listeners. Composite stop completes cleanup on both listeners and propagates failure; swallowing it would let
drainAndShutdownreport success while a socket is held.Verification
bun run typecheckclean,bun run privacy:scanpassed.bun run test— 9756 pass, 8 skip, 0 fail across 607 files.0.0.0.0fails 2; allowlist check disabled fails 1; policy view collapsed to the shared config fails 3;/v1/messagesadded to the allowlist fails 1; the inject substitution removed fails 1; per-step catch removed fails 2; collected failures swallowed fails 3;requestServer.upgradeswapped forserver.upgradefails 1.Known gaps, stated rather than implied
@openai/codexis not a test dependency, so the plan calls for a non-skippable activation run against a real app-server before ready-for-review: isolatedCODEX_HOMEwith nomodels_cache.json, entrypoint resolved directly, token stripped from the child environment, a runtime-generated unique routed model asserted inmodel/list, and one turn against that model. Catalog-present and catalog-absent both.{ kind: "loopback" }log row on a real turn is not.Status
This does not close #1102. The default is off, so the reporter's repro still 401s until they adopt the option, and the question of whether it suits their deployment is still open on the issue.
Stacked on #1216.
Refs #1102