Conversation
e3eef10 to
8c44278
Compare
f33e1d1 to
02a3cc4
Compare
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review (GitHub will not permit this identity to set REQUEST_CHANGES without explicit repository review access).
I re-reviewed this as the direct replacement for #91199, including the exact transport/auth hardening rather than inheriting the older verdict. The new downstream-authority half fixes the stop-the-line issue I raised on #91199 for POSIX: /health and /v1/* authenticate before pool inspection or credential resolution, the comparison is constant-time, the client bearer is stripped rather than forwarded, Codex-owned identity headers override client spellings, the upstream is pinned exactly, raw query/body/SSE handling is preserved, and the cancellation/prepare/stream cleanup tests cover the right failure directions.
There is still one cross-platform authority blocker on exact head 02a3cc41376720234605424f1c4dbf749d25d58c:
Windows does not enforce the owner-only token-file boundary
_read_client_auth_token() makes the token file an authorization credential specifically because loopback is only machine-local on a multi-user host. But the ownership/permission checks are entirely inside if os.name != "nt".
On Windows the function currently proves only:
- the path was a regular file at
lstat; - it did not resolve through an observed symlink;
- the opened file has the same
(st_dev, st_ino); - the contents are bounded/nonempty/single-value.
It never proves who owns the file or who can read it. A perfectly ordinary regular file under a shared location such as C:\Users\Public\codex-proxy.token, or any token file whose inherited DACL grants BUILTIN\Users / Everyone read access, is accepted. Another local OS user can then read the bearer, connect to the loopback listener, and spend the profile owner's ChatGPT/Codex subscription. That recreates the exact confused-deputy boundary this replacement exists to close, just on Windows instead of POSIX.
The tests encode the gap: the 0644 -> reject / 0600 -> accept assertion is skipped on Windows, and there is no Windows DACL/owner witness. The docs call the bearer “owner-only” generally while only explaining POSIX mode enforcement.
Please make the authority invariant cross-platform before landing. Any of these shapes is defensible:
- inspect the Windows file owner/DACL and reject token files readable by unrelated principals (at minimum broad
Users/Everyoneread); or - provision/validate a token in an OS-owned private location with equivalent DACL proof; or
- explicitly fail closed / mark this Codex proxy unsupported on Windows until that proof exists.
Add a real Windows regression where a broadly-readable regular token file is refused and a correctly private token file is accepted. The load path must still retain the current symlink/swap/bounds checks.
Repository topology / credit
The consolidation direction is right and contributor preservation is real: the first commit feb202d23844e32a5cd4ddf06d45d7a6451c66b5 retains Chris Munn's author identity from #91199, and the hardening commit credits Dillon Townsel from #62510. Please also make the older authority lineage explicit when retiring competing delivery objects:
- #62297 by
@darkyy92is the closest earlier native-Responses + owner-only per-client-key implementation for multi-user hosts; superseded as delivery, but directly relevant authority provenance. - #62510 by
@dtownselis downstream-bearer precedent with older token-file/refresh defects; its useful auth shape is carried here and credit is already preserved in the hardening commit. - #92409 by
@phucnguyenquangis overlapping Codex-proxy/inbound-auth work and should not remain a second delivery owner if this lands. - #54877 by
@blazing-mjis older shared-pool/Cloudflare-header work with a Chat-Completions translation choice; adjacent/superseded rather than a byte-level duplicate. - #91199 remains the direct feature origin and should close as superseded by this combined replacement once the hardened object lands.
Exact-object evidence
Current upstream main is 4553e71993dbeb21449f1ef4d6fad069adb20915; the actual merge base is 32a8a7031e8248b05db8142b0d7349616fda71fa, so this head is 2 commits ahead / 3 behind. Those three main commits only touch update/update-receipt tests and do not overlap this proxy diff, but rebase before final acceptance.
Exact-head Docker 32624589313 and Nix 32624589264 are green. CI 32624589728 is cancelled, not green: Detect affected areas was cancelled and the Python, OS-specific, lint, supply-chain, docs, and other substantive lanes were consequently skipped. The aggregate All required checks pass job succeeding after that cancellation is not evidence that those skipped lanes ran. After the Windows authority repair/rebase, require fresh exact-head CI in addition to Docker/Nix.
Once the Windows token-file authority is real and the resulting exact object has complete hosted execution, the rest of this combined replacement looks like the correct canonical Codex proxy shape.
02a3cc4 to
59124ba
Compare
|
Addressed the Windows authority blocker in final rebased head |
59124ba to
bfba30a
Compare
Require an owner-only regular token file before loopback clients can spend the profile-owned Codex subscription. Enforce the same boundary in CLI and programmatic server startup, compare bearer credentials in constant time, and prove unauthorized callers never resolve or contact the upstream. Co-authored-by: Dillon Townsel <dillontownsel@gmail.com>
bfba30a to
57e87dc
Compare
Verify the opened token-file descriptor is owned by the current user or SYSTEM and reject null or permissive DACLs. Add native-Windows regressions for private and Everyone-readable files while preserving the existing symlink, file-swap, and content-bound checks.
57e87dc to
326eadd
Compare
Summary
Direct combined replacement for #91199. The first commit preserves Chris Munn’s original Codex OAuth adapter authorship; the following commits add cross-platform downstream-authority hardening.
/healthand/v1/*before any credential-pool lookup or upstream request0600on POSIXContributor lineage
@BELGARATHbb) is the direct feature origin; his commit remains first with original authorship.@darkyy92is the closest earlier native-Responses and owner-only per-client-authority precedent.@dtownsel) provided downstream-bearer precedent; his contribution is credited in the hardening commit.@phucnguyenquangoverlaps the Codex-proxy/inbound-auth delivery surface.@blazing-mjis adjacent shared-pool/Cloudflare-header work with a different Chat-Completions translation design.This PR is intended as the single hardened delivery object; maintainers can retire the overlapping PRs if this version is accepted.
Security boundary
Loopback limits network reachability, but it does not prove caller authority on a multi-user host. Missing or incorrect downstream credentials return
401 proxy_auth_failedbefore Hermes reads pool availability, resolves an OAuth bearer, or contacts OpenAI.On Windows, the already-open file descriptor is inspected with pywin32. Only the current user and SYSTEM may own or receive allowed access. This preserves the existing symlink, swap, regular-file, and content-bound checks while closing the cross-user confused-deputy path.
Verification
Fresh on the rebased branch:
scripts/run_tests.sh tests/hermes_cli/test_proxy.py tests/hermes_cli/test_proxy_codex.py— 34 passed, 2 native-Windows tests skipped locally and selected for the Windows CI lanegit diff --check— passed/health— HTTP 401proxy_auth_failed/health— HTTP 200, upstream authenticated/v1/responses— HTTP 200,response.completed, exact marker returnedHosted exact-head CI status is recorded in the PR checks.
No credential value, credential file, or production-service configuration is included.