fix(xai-oauth): echo code_challenge in token POST so PKCE exchange succeeds (#26990) - #26999
Closed
xxxigm wants to merge 2 commits into
Closed
fix(xai-oauth): echo code_challenge in token POST so PKCE exchange succeeds (#26990)#26999xxxigm wants to merge 2 commits into
xxxigm wants to merge 2 commits into
Conversation
…cceeds xAI's OAuth implementation at ``auth.x.ai`` validates the PKCE ``code_challenge`` at the **token** endpoint, not just at the authorize step. When Hermes sends the standards-compliant token POST with ``code_verifier`` alone — exactly what RFC 7636 §4.5 prescribes — xAI rejects the exchange with ``code_challenge is required`` and the user is stuck with no working OAuth login. The fix: * Extract the token POST into ``_xai_oauth_exchange_code_for_tokens`` so the wire format is unit-testable in isolation. * Send the original ``code_challenge`` and ``code_challenge_method`` in the form body alongside ``code_verifier``. Strict RFC-compliant servers ignore the extras at the token endpoint, and xAI's permissive implementation accepts the exchange. This is the standard "defensive echo" workaround used by every OAuth client that targets a server with this quirk. * Refuse to fire the POST when ``code_verifier`` is empty — leaking the authorization code to a server that can't redeem it is worse than failing locally with an actionable error. The new error code is ``xai_pkce_verifier_missing`` and the message points at this issue for context. * Surface the HTTP status code prominently in the 4xx error message (``xAI token exchange failed (HTTP 400). Response: …``) so users and maintainers can tell a 400 (bad request / PKCE problem) from a 403 (tier denied, see NousResearch#26847) at a glance instead of parsing the JSON body by eye. Closes NousResearch#26990
14 focused tests on the extracted helper ``_xai_oauth_exchange_code_for_tokens`` cover: Core contract: * ``code_verifier`` is on the wire (RFC 7636 §4.5). * ``code_challenge`` + ``code_challenge_method=S256`` are echoed (the NousResearch#26990 defense-in-depth that makes xAI's token endpoint stop rejecting valid exchanges). * ``grant_type=authorization_code``, ``code``, ``redirect_uri``, and ``client_id`` are all locked. * Content-Type is ``application/x-www-form-urlencoded`` (xAI rejects ``application/json`` on this endpoint). * The supplied ``token_endpoint`` URL is used verbatim — no hard-coded constant sneaks in via a future refactor. * ``timeout_seconds`` is forwarded; floored at 20s. Sanity guard: * Empty ``code_verifier`` raises ``xai_pkce_verifier_missing`` with a link to NousResearch#26990 — and NOTHING is sent. Leaking the auth code to a server that can't redeem it is the wrong failure mode. * Empty ``code_challenge`` omits only the defensive echo; the standards-compliant ``code_verifier`` request still goes out so RFC-compliant servers keep working. Error surfacing: * Non-200 responses include both ``HTTP <status>`` and the body verbatim — disambiguates 400 (PKCE / bad request) from 403 (tier denied, see NousResearch#26847). * Transport errors are wrapped as ``AuthError`` with the ``xai_token_exchange_failed`` code, so the surrounding ``format_auth_error`` UI mapping still fires. * Non-dict JSON payloads raise ``xai_token_exchange_invalid``. * 200 happy path returns the parsed payload dict verbatim. End-to-end wire-format guard: * A real ``httpx.Client`` with a stub transport captures the bytes on the wire and asserts every PKCE field round-trips through ``urlencode``. Catches a future refactor that swaps ``data=`` for ``json=`` (which xAI would silently reject).
Contributor
|
Salvaged into main via #27560 — your commits were cherry-picked onto current main with your authorship preserved in git log. The defensive |
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
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.
What does this PR do?
Fixes
hermes auth add xai-oauthgetting stuck at the token-exchange step withcode_challenge is required.Issue #26990 reported a P1 regression where the loopback OAuth flow succeeds at the browser-side authorize step (callback comes back with a valid
code), but the follow-up POST to xAI's token endpoint fails withcode_challenge is required— even though Hermes was already sending a validcode_verifierexactly as RFC 7636 §4.5 prescribes.Root cause — xAI's OAuth implementation at
auth.x.aire-validates the PKCEcode_challengeat the token endpoint, not just at the authorize step. A standards-compliant client that sends onlycode_verifier(the canonical RFC 7636 token-step input) is rejected. This is a known quirk of permissive OAuth implementations: while RFC 7636 doesn't require the client to echo the challenge at the token endpoint, it also doesn't forbid it, and many home-rolled servers cross-validate.Fix — extract the token POST into a dedicated, unit-testable
_xai_oauth_exchange_code_for_tokenshelper and:code_verifieras before (unchanged contract).code_challengeandcode_challenge_method=S256in the form body. Strict RFC-compliant servers ignore extras at the token endpoint; xAI's permissive server accepts the exchange. This is the standard defensive-echo workaround used by every OAuth client targeting a server with this quirk.code_verifieris empty — leaking an auth code to a server that can't redeem it is worse than failing locally with an actionable error (new code:xai_pkce_verifier_missing, message points at xai-oauth token exchange fails: code_challenge is required (PKCE code_verifier missing) #26990).xAI token exchange failed (HTTP 400). Response: …) so users / maintainers can tell a 400 (PKCE / bad request) from a 403 (tier denied, see [Bug]: xAI OAuth (xai-oauth) returns HTTP 403 for standard SuperGrok subscribers — backend enforcing Heavy-only despite docs claiming all tiers #26847) at a glance instead of parsing the JSON body by eye.Related Issue
Closes #26990
Type of Change
Changes Made
hermes_cli/auth.py— extract token POST into_xai_oauth_exchange_code_for_tokens(...); echocode_challenge+code_challenge_method=S256alongsidecode_verifier; refuse to POST with emptycode_verifier(raisesxai_pkce_verifier_missing); embed HTTP status code in 4xx error messages._xai_oauth_loopback_loginnow delegates to the helper — 40 lines of inline httpx replaced with one call.tests/hermes_cli/test_xai_oauth_pkce_token_exchange.py— 14 new regression tests covering:code_verifieris on the wire (RFC 7636 §4.5).code_challenge+code_challenge_method=S256are echoed (the xai-oauth token exchange fails: code_challenge is required (PKCE code_verifier missing) #26990 defense-in-depth).grant_type,code,redirect_uri,client_id.token_endpointused verbatim (no hard-coded constant).timeout_secondsforwarded; floored at 20s.code_verifierraises locally without sending — and NOTHING is POSTed.code_challengekeeps the standards-compliant request flowing (defensive echo is opt-in).HTTP <status>and the response body.AuthErrorwith the right code.xai_token_exchange_invalid.httpx.Client+ stub transport that captures the bytes and parses them — catches a future refactor that swapsdata=forjson=(which xAI would silently reject).How to Test
Why "defensive echo" and not "switch to a different flow"?
code_verifierat the token endpoint — so the pre-fix code was technically correct.code_challengeat the token endpoint is harmless for strict servers (they MUST ignore unknown parameters per RFC 6749 §3.2) and necessary for xAI.