fix(mcp): forward short OAuth state upstream, keep session in a cookie - #32146
Conversation
Greptile SummaryFixes LIT-4197: strict upstream IdPs were rejecting LiteLLM's MCP OAuth proxy because the
Confidence Score: 5/5Safe to merge — the change is isolated to the MCP OAuth proxy authorize/callback path, is backward-compatible for in-flight flows, and is covered by new mock tests that verify both the cookie round trip and error-path cookie cleanup. The relay-state design is stateless and correct across replicas. Cookie attributes (HttpOnly, SameSite=lax, Max-Age=600, path/secure derived consistently for both set and delete) are appropriate for the cross-IdP-redirect use case. The fallback path for legacy encrypted states is safe. The one remaining gap — the one-time cookie is not cleared when an HTTPException is re-raised on the success path — is a minor UX leak (orphaned cookie expires in 600 s) that was already identified in prior review threads. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Core change: introduces cookie-based relay state for OAuth authorize/callback. Cookie helpers are correct (path/secure/httponly/samesite handled consistently between set and delete). Backward-compat fallback in _resolve_encoded_oauth_state works. Cookie is cleared on all callback return paths except the re-raised HTTPException (minor UX gap, previously flagged). |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Adds req.cookies={} to the shared mock fixture and two new async tests covering the full authorize→callback cookie round trip and the IdP error + cookie-cleanup path. No real network calls; all HTTP is mocked. Existing tests are unmodified. |
Reviews (5): Last reviewed commit: "test(mcp): cover /callback error path co..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This comment was marked as outdated.
This comment was marked as outdated.
71510f9 to
6ce6fd9
Compare
|
Addressed the review feedback in the latest commit The The one-time cookie is now also cleared on the The regression test now asserts the callback response expires the cookie ( |
|
@greptileai review |
Some upstream authorization servers reject the OAuth authorize request with "state parameter too long" because LiteLLM replaced the client's short state with its own long encrypted session blob (base_url, original state, PKCE, client redirect_uri) and sent that upstream as state. Forward a short random handle as the upstream state instead, and carry the encrypted session in a per-flow HttpOnly, SameSite=lax cookie bound to that handle. The browser replays the cookie on /callback, so the session is recovered without any server-side store and the client still gets its own original state back. /callback falls back to decoding state directly when no cookie is present, so flows in flight across a deploy keep working. Resolves LIT-4197
6ce6fd9 to
5c65f1c
Compare
The happy-path regression test already asserts the short-handle -> cookie round trip. Add a focused test for the IdP-error branch of /callback: it must recover the client's original state from the per-flow cookie (not the short handle), propagate the error to the client's redirect_uri, and expire the one-time cookie. Fails if the error path stops reading or clearing the cookie.
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 444446e. Configure here.
| return RedirectResponse(url=complete_returned_url, status_code=302) | ||
| response = RedirectResponse(url=complete_returned_url, status_code=302) | ||
| _clear_oauth_state_cookie(response, request, state) | ||
| return response |
There was a problem hiding this comment.
HTTPException skips cookie cleanup
Medium Severity
On the successful /callback path, when _get_validated_client_redirect_uri raises HTTPException, the handler re-raises without calling _clear_oauth_state_cookie. Other failure branches in the same handler clear the one-time mcp_oauth_state_* cookie, so the encrypted OAuth session can remain in the browser for the full Max-Age.
Reviewed by Cursor Bugbot for commit 444446e. Configure here.
There was a problem hiding this comment.
Accurate read of that branch, and it is intentional; the exposure is low enough that reworking it would cost more than it saves
The only thing that raises HTTPException inside that try is _get_validated_client_redirect_uri, so the cookie survives only when the decoded client_redirect_uri fails the sink-side VERIA-57 trust check. The re-raise is deliberate; it surfaces that as a 400 rather than the generic authentication-incomplete fallback, and it is pinned by the existing VERIA-57 regression tests, which assert pytest.raises(HTTPException) with status_code == 400. Converting the branch to a returned response so it can clear the cookie would break that contract and route around the proxy's HTTPException handler
The surviving cookie is also inert. It carries the encrypted {original_state, client_redirect_uri, code_challenge, ...} blob with no tokens and no authorization code; it is HttpOnly and Secure, and it expires within its 600s Max-Age. Any replay re-runs the same validation and re-fails with the same 400, and the code is only appended to the redirect after validation passes, so a surviving cookie cannot leak it to the untrusted URI. A retry mints a fresh handle and cookie and orphans the stale one, and for the loopback native-client flows this targets the branch never fires at all, since loopback validates identically at /authorize and /callback; it needs a same-origin UI redirect plus an origin shift between the two requests
Leaving it as intentional on that basis. If strict parity across every branch is wanted later, the safe way is to attach the delete-cookie to the raised exception's headers so the 400 contract and the handler both stay intact, rather than returning a response


Relevant issues
Linear ticket
Resolves LIT-4197
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewScreenshots / Proof of Fix
Live proxy on
localhost:4000with oneauth_type: oauth2MCP server whoseauthorization_urlpoints at an upstream IdP. The request below is the exact interactive/authorizean MCP client (mcp-inspector, Claude Code, Cursor, the LiteLLM UI connect button) makes, with a native loopbackredirect_uri.Before the fix, LiteLLM forwards its own long encrypted session as the upstream
state:After the fix, the same request forwards a short handle upstream and moves the encrypted session into a per-flow HttpOnly cookie:
The IdP redirects back to
/callbackwith the handle; the browser replays the cookie, so LiteLLM recovers the session and returns the client's own originalstateplus the code, then clears the one-time cookie:Flows started before the change (or in flight across a rolling deploy) still carry the encrypted blob directly in
statewith no cookie;/callbackfalls back to decoding it, so they keep working:Type
🐛 Bug Fix
Changes
When LiteLLM proxies an interactive
authorization_codeOAuth flow to an upstream MCP authorization server, it needs to remember two things for/callback: the client's originalstate(to echo back so the client's CSRF check passes) and the client'sredirect_uri(to know where to send the browser). The upstream only reflects the onestatevalue it is given, so the old code packed the whole session (base_url, original state, PKCE fields, client redirect_uri) into an encrypted blob and sent that blob upstream asstate. That blob runs a few hundred characters, and some authorization servers reject an over-longstate, which is the "state parameter too long" failureauthorize_with_servernow forwards a short random handle as the upstreamstateand stores the existing encrypted session in a per-flow HttpOnly,SameSite=laxcookie keyed by that handle. The browser carries the cookie across the upstream round trip, so/callbackrecovers the session with no server-side store, which keeps the flow correct across proxy replicas the same way the stateless-state design did. The handle is server-generated rather than the client's ownstateso it is unique per flow (no cross-flow cookie collisions) and unguessable, and the client still receives its own originalstateback at itsredirect_uribecause/callbackrestores it from the cookie/callbackreads the session from the cookie when present and falls back to decodingstatedirectly otherwise, so states minted before this change keep working. PKCE is unchanged;code_challengeandcode_verifierare still forwarded to the upstream, and they were never read back on/callback, so nothing depends on them surviving insidestateThe change is confined to
authorize_with_serverand the shared/callback, so it covers both the discoverable/authorize(used by MCP clients) and the UI/server/oauth/{server_id}/authorizeflow. Pass-through servers (auth_typenone, upstream-delegated discovery),client_credentials, static-header auth, and the BYOK authorization-server endpoints do not route through these functions and are untouchedNote
Medium Risk
Touches OAuth authorize/callback and redirect/cookie handling on a security-sensitive path; behavior is backward-compatible but cookie loss or cross-site edge cases could break interactive MCP login.
Overview
Fixes LIT-4197: strict upstream IdPs were rejecting LiteLLM’s MCP OAuth proxy because the upstream
statecarried a long encrypted session blob./authorize(authorize_with_server) now sends a short random handle as upstreamstateand stores the same encrypted session in a per-flow HttpOnly, SameSite=lax cookie (mcp_oauth_state_<handle>, 10‑minute TTL). The browser carries that cookie through the IdP redirect, so no server-side session store is needed and multi-replica proxies still work./callbackresolves the session via_resolve_encoded_oauth_state(cookie when present, otherwise the legacy value instatefor in-flight or pre-deploy flows), then clears the one-time cookie on success, IdP error propagation, and failure paths.Tests cover the full authorize→callback round trip, IdP error handling with cookie cleanup, and mock request cookies.
Reviewed by Cursor Bugbot for commit 444446e. Bugbot is set up for automated code reviews on this repo. Configure here.