Bound OpenAI Codex auth JSON responses - #55254
Conversation
114d21b to
a3ed1a1
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Good security hardening — bounds OpenAI Codex auth JSON response reads to prevent memory exhaustion. The _read_codex_auth_json_response helper with iter_bytes and max_bytes check follows the established pattern. Clean dataclass extraction for the response type.
Security: Limits auth JSON responses to prevent oversized upstream responses.
Code Quality: Well-structured with proper error codes and AuthError wrapping.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Adds streamed bounded JSON reader for OpenAI Codex device-code auth responses. Routes CLI device-code creation, auth polling, and token exchange through the bounded reader.
- Clean streaming pattern with size enforcement
- Well-scoped: 4 files, 256 additions
Reviewed 4 files, 256 additions. Approved.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused hardening work. The device-code and dashboard premise still holds on current main: those flows eagerly use post() followed by .json() in hermes_cli/auth.py:7284-7439 and hermes_cli/web_server.py:9277-9345.
Problems
refresh_codex_oauth_pure()is another Codex OAuth token-endpoint path, but remains unbounded athermes_cli/auth.py:3450; it parses non-200 and successful JSON at:3489and:3531.- The dashboard context has changed since the PR base. Current
hermes_cli/web_server.py:9283passes start failures to_codex_device_code_start_error(), which readsresp.json()/resp.textat:9203-9226. The submitted wrapper retains neither a non-200 payload nor those response methods, so salvage must preserve the current dashboard-specific authorization guidance.
Suggested changes
- Cover the refresh-token response path with the same bounded-reader approach while retaining its 429 and structured-error behavior.
- Integrate the dashboard change with the current bounded error-detail path and retain the existing device-authorization guidance test.
Automated hermes-sweeper review.
| ) | ||
| issuer = "https://auth.openai.com" | ||
|
|
||
| # Step 1: request device code | ||
| with httpx.Client(timeout=httpx.Timeout(15.0)) as client: | ||
| resp = client.post( | ||
| resp = _post_codex_auth_json( |
There was a problem hiding this comment.
When salvaging this onto current main, preserve the non-200 response detail consumed by _codex_device_code_start_error() (hermes_cli/web_server.py:9203-9248). Current main invokes that helper for this response, while _CodexAuthJsonResponse retains only status, headers, and a 200-only payload.
a3ed1a1 to
ca6e551
Compare
Summary
Fixes #55253.
Validation
PYTHONPATH=$PWD C:\Users\Administrator\Documents\Codex\2026-06-29\hermes-main-latest-scan\.venv\Scripts\python.exe -m pytest tests\hermes_cli\test_auth_codex_provider.py tests\hermes_cli\test_web_oauth_dispatch.py -q --basetemp .pytest-tmp-codex-auth-json-web-> 61 passedPYTHONPATH=$PWD C:\Users\Administrator\Documents\Codex\2026-06-29\hermes-main-latest-scan\.venv\Scripts\python.exe -m ruff check hermes_cli\auth.py hermes_cli\web_server.py tests\hermes_cli\test_auth_codex_provider.py tests\hermes_cli\test_web_oauth_dispatch.py-> passedgit diff --check-> passedNotes
This is separate from #54750, which bounds
hermes_cli.copilot_auth. This PR covers theopenai-codexdevice-code flow in bothhermes_cli.authand the dashboard login worker inhermes_cli.web_server.