fix(security): separate OAuth PKCE state from code_verifier (#10693)#26861
Merged
Conversation
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 #10693
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.
…tion Two unit tests for run_hermes_oauth_login_pure(): 1. test_authorization_url_state_is_not_pkce_verifier — asserts state in the auth URL is independent from the PKCE code_verifier sent in the token exchange, and that the verifier never appears in the URL. 2. test_callback_state_mismatch_aborts — asserts the flow returns None (no token exchange) when the callback state does not match the value we generated. Negative control verified: reintroducing the b17e5c1 vulnerable pattern (state = verifier, no callback validation) makes both tests fail. Also adds AUTHOR_MAP entry for shaun0927 (contributor of the fix).
This was referenced May 16, 2026
Contributor
🔎 Lint report:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Salvage of #10699 by @shaun0927 — cherry-picked clean onto current main with contributor authorship preserved.
Summary
run_hermes_oauth_login_pure()reused the PKCEcode_verifieras the OAuthstateparameter, leaking the verifier into the authorization URL (browser history, Referer headers, auth-server logs) and removing CSRF protection on the callback. Now generates an independentsecrets.token_urlsafe(32)forstateand validates it before token exchange.Regression context
This exact bug was fixed in PR #1775 on the old
run_hermes_oauth_login(). PR #2647 (commit b17e5c1, credential pools) added a newrun_hermes_oauth_login_pure()and silently copy-pasted the pre-#1775 vulnerable pattern. PR #3107 then removed the old (fixed) function, leaving only the regressed copy.Changes
agent/anthropic_adapter.py(+10/-3) — @shaun0927's fix, cherry-pickedtests/agent/test_anthropic_oauth_pkce.py(+159) — two regression tests:test_authorization_url_state_is_not_pkce_verifier— asserts state in the auth URL is independent from the verifier sent in the token exchange, and the verifier never appears in the URLtest_callback_state_mismatch_aborts— asserts the flow returns None (no token exchange) when callback state does not matchscripts/release.py— AUTHOR_MAP entry for shaun0927Validation
test_anthropic_oauth_pkce.py,test_anthropic_adapter.py,test_anthropic_oauth_flow.py,test_auth_commands.pyCloses #10693
Supersedes #10699