Skip to content

fix(copilot): bound auth JSON response reads - #54750

Open
ooiuuii wants to merge 1 commit into
NousResearch:mainfrom
ooiuuii:fix/copilot-auth-response-cap
Open

fix(copilot): bound auth JSON response reads#54750
ooiuuii wants to merge 1 commit into
NousResearch:mainfrom
ooiuuii:fix/copilot-auth-response-cap

Conversation

@ooiuuii

@ooiuuii ooiuuii commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #54749.

This bounds Copilot auth JSON response reads in hermes_cli.copilot_auth:

  • device-code creation responses now use a limited JSON reader
  • device-code polling responses now use the same limited reader
  • Copilot token exchange responses now use the same limited reader
  • oversized JSON responses fail with a clear diagnostic instead of buffering the whole body

The cap is 1 MiB, which is far above the expected size of these OAuth/token JSON payloads while still protecting the process from malformed upstream or proxy responses.

Testing

  • C:\Users\Administrator\Documents\Codex\2026-06-29\hermes-openclaw-pattern-scan\.venv\Scripts\python.exe -m pytest tests\hermes_cli\test_copilot_auth.py tests\hermes_cli\test_copilot_token_exchange.py -q --basetemp .pytest-tmp-copilot-auth (39 passed)
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools provider/copilot GitHub Copilot (ACP + Chat) P3 Low — cosmetic, nice to have labels Jun 29, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: LGTM

Security hardening: bounds Copilot OAuth response reads to 16 KB. Applied to device code initiation, polling, and token exchange endpoints.

Looks Good

  • Consistent pattern with other response-bounding PRs
  • Tests cover both device code and token exchange rejection paths
  • Falls back properly when test doubles don't expose read(limit)

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: LGTM

Clean fix to bound Copilot auth response reads. The implementation is well-structured with a reusable helper function and comprehensive tests for multiple paths.

Looks Good

  • Clean helper function with descriptive label parameter
  • Good test coverage for device code and token exchange paths
  • Appropriate 1 MB limit for auth responses
  • Follows established pattern from other response-bound fixes

Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused hardening work. The premise is still present on current main: hermes_cli/copilot_auth.py:202, :248, and :347 each call unbounded resp.read().

Problems

  • In the PR, the polling call at hermes_cli/copilot_auth.py:253 uses the capped helper, but its ValueError is swallowed by the broad except Exception at :254-256. An oversized polling response is reduced to . retries and eventually a timeout, rather than the clear oversize diagnostic provided by the helper.

Suggested changes

  • Catch the helper's ValueError separately in the polling loop, print the diagnostic, and return None; preserve retries for transient transport failures.
  • Cover that branch with a valid initial device-code response followed by an oversized polling response.

Automated hermes-sweeper review.

@@ -239,7 +250,7 @@ def copilot_device_code_login(

try:
with urllib.request.urlopen(poll_req, timeout=10) as resp:
result = json.loads(resp.read().decode())
result = _read_copilot_json_response(resp, label="device code poll")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The surrounding except Exception catches this helper's oversize ValueError and turns it into another progress dot. Please handle that error separately so an oversized polling response fails with the clear diagnostic promised by this PR, while retaining retries for transport failures.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@ooiuuii
ooiuuii force-pushed the fix/copilot-auth-response-cap branch from 5cfbdc7 to a547699 Compare August 12, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have provider/copilot GitHub Copilot (ACP + Chat) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Copilot auth helpers read JSON response bodies without a cap

4 participants