feat(proxy): add OpenAI Codex subscription adapter - #92409
phucnguyenquang wants to merge 1 commit into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
I reviewed exact head ec8e722a84525a4711f64647fed7d6206100a45e against current main / PR base 0cde4dd93aa794c65fee6cc85b0b5e4eee77e8e2. The raw Responses/health split is a useful direction, and the new inbound-key option and provider-header overlay are meaningful pieces. I do not think this head is safe to merge yet because two credential-authority boundaries remain open.
Blocker 1 — Codex subscription spend is still exposable by a legal CLI configuration. run_server() accepts arbitrary --host, and create_app() deliberately treats inbound_bearer_key=None as authorized. The docs then retain the literal LAN recipe hermes proxy start --host 0.0.0.0 --port 8645, merely warning that --auth-key-env should be used. For openai-codex, that means a perfectly accepted command publishes an unauthenticated listener that attaches the user's ChatGPT OAuth bearer to every allowed request. This is not equivalent to an ordinary unauthenticated local proxy: the listener is spending a subscription credential the remote caller never possesses.
The authority needs to be enforced in code, not left to operator memory. The narrow compatible rule is: openai-codex may bind loopback without an inbound key, but any non-loopback bind must fail closed unless an inbound bearer is configured (or make Codex loopback-only, as the older competing implementation does). Add a production-path regression for Codex + 0.0.0.0 with no inbound key, plus the explicitly authorized case if non-loopback-with-key is intended.
Blocker 2 — ChatGPT-Account-ID is still client-spoofable when the OAuth token does not carry that claim. _filter_request_headers() strips hop-by-hop headers and Authorization, but it leaves ChatGPT-Account-ID, originator, and User-Agent. _send_upstream() then overlays only the keys returned in active_cred.headers. _codex_cloudflare_headers() always replaces originator/User-Agent, but it intentionally omits ChatGPT-Account-ID when the JWT is malformed or lacks chatgpt_account_id. In that valid code path, an inbound ChatGPT-Account-ID: attacker-chosen-account survives unchanged and is forwarded next to Hermes's real OAuth bearer. The new test proves originator replacement but its fake credential headers do not include or exercise the account-ID omission case.
Adapter-owned provider identity must be stripped case-insensitively from inbound headers before trusted headers are synthesized, regardless of whether a particular credential produces every owned header. Please add a regression using a bearer with no account-id claim plus a spoofed inbound ChatGPT-Account-ID, and assert the spoof is absent upstream.
There is important existing topology here rather than a blank-slate adapter. Closed/unmerged #89530 by @OpenCnid is earlier provenance for the narrow Codex OAuth broker and already called out POST-only routing, value-free auth failures, and retry only when the credential changes. Open/non-mergeable #91199 by @BELGARATHbb is a competing implementation of this same openai-codex proxy surface, not a complementary PR: it carries explicit owned-header stripping, trusted-Codex-upstream enforcement, loopback-only binding, refresh coalescing/429 rotation, raw-query preservation, and streaming/session cleanup. @phucnguyenquang's smaller current-main implementation should keep its credit, but these branches should converge rather than merge independently.
Separately, open #85017 by @briandevans is complementary, not duplicate: it owns the existing proxy event-loop defect where synchronous is_authenticated(), get_credential(), and get_retry_credential() can block all concurrent streams. This adapter invokes those same synchronous Codex auth paths, so whichever proxy branch becomes canonical should compose that off-loop authority rather than fork another solution.
Exact-head hosted CI has not executed: CI 32588789244, Docker 32588788967, and Nix 32588789002 are all action_required. The PR's reported focused local tests are useful evidence, but they do not cover either failure sequence above.
Reviewed What's good
Suggestions
Solid, security-conscious adapter — #1 is the only thing I'd change before merge. |
Summary
openai-codexsubscription-proxy adapter backed by Hermes-managed Codex OAuth/v1/responsesrequests with the provider-required Codex headers and one 401 credential refreshWhy
The regular Hermes API server adds the agent prompt, tools, memory, and loop. Raw narrative consumers need the subscription authentication without that large agent context. Keeping this in
hermes proxyalso avoids downstream patches being overwritten on every Hermes update.Verification
scripts/run_tests.sh tests/hermes_cli/test_proxy_openai_codex.py(4 passed)scripts/run_tests.sh tests/hermes_cli/test_proxy.py(4 passed)scripts/run_tests.sh tests/hermes_cli/test_subcommands_profile_gateway.py(2 passed)ruff check hermes_cli/proxy hermes_cli/subcommands/gateway.py tests/hermes_cli/test_proxy_openai_codex.pygit diff --check