Skip to content

fix(security): separate OAuth PKCE state from code_verifier#10699

Closed
shaun0927 wants to merge 2 commits into
NousResearch:mainfrom
shaun0927:fix/oauth-pkce-state
Closed

fix(security): separate OAuth PKCE state from code_verifier#10699
shaun0927 wants to merge 2 commits into
NousResearch:mainfrom
shaun0927:fix/oauth-pkce-state

Conversation

@shaun0927

@shaun0927 shaun0927 commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Summary

The Anthropic OAuth PKCE flow reused the code_verifier as the state parameter. This PR generates an independent cryptographic random value for state and validates it on callback.

Changes

agent/anthropic_adapter.py:

# Before (line 675)
"state": verifier,

# After
import secrets as _secrets
oauth_state = _secrets.token_urlsafe(32)
# ...
"state": oauth_state,

On callback:

# Before — state parsed but never validated
state = splits[1] if len(splits) > 1 else ""

# After — state validated against original value
received_state = splits[1] if len(splits) > 1 else ""
if received_state != oauth_state:
    logger.warning("OAuth state mismatch — possible CSRF, aborting")
    return None

Why this matters

Per RFC 6749 §10.12 and RFC 7636, state and code_verifier serve fundamentally different purposes:

  • state is an anti-CSRF token, visible in the authorization URL (browser history, referrer headers, server logs)
  • code_verifier must remain secret — known only to the client, used in the token exchange

Reusing the verifier as state leaked it via the authorization URL. Additionally, the state was never validated on callback, so CSRF protection was absent.

Files changed (1 file, +10/-3)

File Change
agent/anthropic_adapter.py Separate state generation + callback validation

Test plan

  • hermes login --anthropic OAuth flow completes successfully
  • State mismatch on callback → returns None with warning log
  • Token exchange sends correct code_verifier (unchanged)

Closes #10693

@shaun0927
fix(security): separate OAuth PKCE state from code_verifier
b2708c7 
The PKCE flow reused the code_verifier as the OAuth state parameter.
Per RFC 6749 §10.12 and RFC 7636, these serve different purposes:
state is an anti-CSRF token visible in the authorization URL; the
code_verifier must remain secret for the token exchange.

Generate an independent secrets.token_urlsafe(32) for state and
validate it on callback to provide actual CSRF protection.

Closes NousResearch#10693
@shaun0927
style: move secrets import alongside other function-level imports
8e66b3d 
Group the secrets import with time and webbrowser at the top of
run_hermes_oauth_login_pure(), matching the existing pattern.
Drop the _secrets alias — no name conflict in this scope.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround area/auth Authentication, OAuth, credential pools provider/anthropic Anthropic native Messages API labels Apr 25, 2026
@alt-glitch

alt-glitch commented Apr 25, 2026

Copy link
Copy Markdown
Collaborator

Possibly superseded by #1775 (merged) which fixed PKCE verifier leak. However, #3107 (merged) later removed the Hermes-native PKCE OAuth flow entirely. Please verify if agent/anthropic_adapter.py still contains the vulnerable code path or if this is stale. Closes #10693.

@alt-glitch alt-glitch mentioned this pull request Apr 25, 2026
Closed
@shaun0927

shaun0927 commented Apr 27, 2026

Copy link
Copy Markdown
Contributor Author

Re-checked this against current origin/main (ec671c41). The Hermes-native PKCE flow was removed in #3107 but reintroduced by ddb0490a ("hermes_pkce sync/retry parity") on Apr 10, and agent/anthropic_adapter.py on main currently still contains the verifier-as-state pattern:

  • agent/anthropic_adapter.py:888-898 — verifier, challenge = _generate_pkce() then "state": verifier
  • agent/anthropic_adapter.py:935-944 — incoming state is parsed but never validated against a stored value
  • agent/anthropic_adapter.py:946 — code_verifier is reused in the token POST as well

So the concern in #10693 isn't stale — the same code path #1775 originally fixed has come back via ddb0490a.

This branch rebases cleanly onto ec671c41 (no conflicts). The two commits on top of main after rebase:

  • fix(security): separate OAuth PKCE state from code_verifier
  • style: move secrets import alongside other function-level imports

Net diff is still scoped to agent/anthropic_adapter.py only:

  • generate oauth_state = secrets.token_urlsafe(32) independent of the verifier
  • send oauth_state as the state query param
  • compare received_state == oauth_state on callback before exchanging the code

Happy to force-push the rebase if maintainers prefer a clean linear history before review.

@teknium1 teknium1 mentioned this pull request May 16, 2026
Merged
@teknium1

teknium1 commented May 16, 2026

Copy link
Copy Markdown
Contributor

Salvaged in #26861 (merged). Your two commits were cherry-picked onto current main with your authorship preserved in git log — thanks for the catch and the clean RFC-grounded fix. Added two regression tests on top so this doesn't drift back.

For context: this was a regression of #1775 — the old run_hermes_oauth_login() was fixed there, but a later credential-pool PR (#2647, b17e5c1) added run_hermes_oauth_login_pure() and copy-pasted the pre-fix pattern. #3107 then removed the old (fixed) function, leaving only the regressed copy. Your fix on #10699 closes it again.

@teknium1 teknium1 closed this May 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area/auth Authentication, OAuth, credential pools P1 High — major feature broken, no workaround provider/anthropic Anthropic native Messages API type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OAuth PKCE code_verifier reused as state parameter — verifier leaked via authorization URL

3 participants

@shaun0927 @alt-glitch @teknium1