fix(mcp): re-register DCR client when proxy origin no longer matches its registered redirect_uri - #32527
Conversation
Greptile SummaryThis PR fixes MCP OAuth DCR clients being permanently stranded when the proxy's public origin changes: the reused client stays bound to the old
Confidence Score: 5/5Safe to merge; the change is scoped to the admin DCR persist path with deliberate backward-compatible grandfathering for pre-existing rows. The stale-redirect detection logic is correct and well-guarded: missing redirect_uris in the stored blob is treated as a match, so upgrading never re-mints client IDs for existing installs. Explicit None writes for client_secret and token_endpoint_auth_method correctly override old values through the DB merge. The non-admin path is unchanged. Four regression tests pin each contract point. No data-loss or auth-boundary issues found. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Adds stale-redirect detection (_redirect_uri_not_registered, _persisted_dcr_redirect_uri_is_stale), threads current_redirect_uri through reuse checks and persist, records redirect_uris with explicit None-clearing for client_secret/token_endpoint_auth_method. Logic is correct; DB merge semantics verified to handle explicit None values properly. |
| litellm/types/mcp.py | Adds redirect_uris: Optional[List[str]] field to MCPCredentials TypedDict with clear documentation; correctly marked optional and not a secret. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Adds four well-targeted regression tests covering all four contract points: positive mismatch re-registers, missing recording grandfathered, matching recording reuses, non-admin path reuses on mismatch. Also pins redirect_uris assertion on the existing persist test. |
Reviews (3): Last reviewed commit: "fix(mcp): emit one operator warning per ..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@greptileai addressed the duplicate-warning concern: the reuse-skip log in _reuse_persisted_dcr_client_if_available is now debug level, so a re-registration event emits exactly one operator warning (the staleness probe's message naming both bindings and the re-auth impact) |
|
Confirmed — the implementation matches what you described. In the re-registration path, The debug message in |
|
@greptileai rereview |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1afc1c7. Configure here.
…its registered redirect_uri A dynamically registered (RFC 7591) OAuth client persisted onto the MCP server row is bound to the redirect_uri it was first registered with, but that binding was never recorded. After the proxy's public origin changed, every authorize paired the reused client with the new callback and the IdP rejected it permanently. The DCR persist now records redirect_uris alongside the client identity. The admin register path treats a positive mismatch between the recording and the current callback as stale and re-registers a replacement client; rows without a recording (pre-existing installs and admin-configured clients) are grandfathered so upgrades never re-mint client_ids or orphan refresh tokens. The persist also writes client_secret and token_endpoint_auth_method explicitly as None when absent so the credential blob merge cannot pair a re-registered public client with the previous client's secret. Public register routes and non-admin callers keep existing behavior. Closes #32473
The stale-redirect path logged three warnings for a single re-registration: the staleness probe plus the reuse skip in both register_client_with_server and the persist race guard. The reuse-skip message is a mechanical consequence of the probe's decision, so it now logs at debug; the actionable warning that names both bindings and the re-authentication impact is emitted once by _persisted_dcr_redirect_uri_is_stale
1afc1c7 to
2254ffb
Compare
|
@greptileai rereview |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 2254ffb. Configure here.
Behavior after this PR
1. Existing MCP server (DCR client persisted before this PR, so the row has no recorded
redirect_uris)2. Freshly created server (first connect happens on this build)
redirect_uristhe client was registered with3. Recorded server after the proxy origin changes (the case this PR fixes)
{origin}/callback, registers a replacement client upstream and persists it, logging a warning that users of this server must re-authenticate once4. Admin-configured
client_id, and non-admin or public register callersRollout ordering and recovery
This PR and #32921 fix two different triggers of the same IdP-side rejection. This PR covers the callback the proxy sends changing out from under a registered client: renaming the host, moving TLS termination, or setting
PROXY_BASE_URLfor the first time. #32921 covers the proxy sending a non-canonicalhttps://host:443/callbackthat literal-matching IdPs reject against a canonically registered URILand this PR before #32921. To a literal-matching IdP, the port strip in #32921 is itself an origin change: every DCR client registered under the
:443form stops matching the callback the proxy sends afterwards. With this PR in first, rows registered from then on carry a recording, so that transition reads as a positive mismatch and heals on the next admin register. In the reverse order those clients would strand with no recording and no self-heal pathTwo populations still need one manual admin step: rows stranded before this PR shipped, and unrecorded rows that cross the #32921 transition on deployments whose ingress sends
X-Forwarded-Port: 443withoutPROXY_BASE_URLset. Neither has a recording, so the mismatch check deliberately stays silent for them. The admin can delete and recreate the server, or more gently edit it, switch the auth type away from oauth2, save, then switch back and save again;update_mcp_serverreplaces the credentials blob whenever the auth type changes, so the stale client_id is dropped while the server_id, access groups and remaining settings survive. The next admin register then mints a fresh client under the current origin and records the binding. Users of that server re-authenticate once either way, since their refresh tokens were bound to the replaced clientSetting
PROXY_BASE_URLstays the recommended configuration: it pins the resolved origin, which both avoids the:443reconstruction that #32921 patches and prevents per-request origin variance from tripping the mismatch check after this PRRelevant issues
Closes #32473 (reported by @katzdave)
Related: #32921 (non-canonical
:443inredirect_uri); land this PR first, see the rollout section aboveLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Rig: a strict stub IdP plus MCP resource server on localhost:9500 (RFC 9728/8414 discovery, RFC 7591
/registerthat records each client'sredirect_uris,/authorizethat returns the standard 400 on a redirect mismatch exactly like a strict production IdP) and the proxy on localhost:4100 backed by Postgres. The origin change is simulated by restarting the proxy with a differentPROXY_BASE_URL, which is the production trigger described in the issueBefore the fix: the server is permanently stranded
Proxy started with
PROXY_BASE_URL=http://localhost:4100, MCP server created pointing at the stub, then the admin register endpoint mints and persists the DCR clientAuthorize works at this origin (the IdP 302s back with a code). Now the proxy is restarted with
PROXY_BASE_URL=https://litellm.example.com. Register becomes a silent no-op (the persisted client short-circuits it, nothing reaches the IdP) and authorize builds the poison pair of old client and new callbackThe stub logs
AUTHORIZE REJECTED client-1: redirect_uri='https://litellm.example.com/callback' not in registered ['http://localhost:4100/callback']. Every subsequent connect fails the same way and nothing ever re-registersAfter the fix: grandfathering holds for pre-existing rows
On the fixed build, the same register call against the row persisted by the old code (which has no
redirect_urisrecording) still reuses the existing client and sends nothing to the IdP, so upgrading does not re-mint client_ids or orphan refresh tokens for existing installsAfter the fix: a recorded row self-heals on origin change
Server recreated on the fixed build at origin A; the persisted blob now records the binding (client_id stays encrypted, the recording is not a secret)
Proxy restarted with
PROXY_BASE_URL=https://litellm.example.com, then the same admin register call detects the positive mismatch and re-registers instead of reusingThe DB blob after healing records the new binding (
redirect_uris: ['https://litellm.example.com/callback']) and the proxy logs an operator-visible warning: "persisted DCR client for server_id=stub-dcr-gh32473 is registered with redirect_uris=['http://localhost:4100/callback'] which do not include the current callback https://litellm.example.com/callback (proxy origin changed); registering a replacement client. Users previously signed in to this server will need to re-authenticate."Type
🐛 Bug Fix
Changes
Root cause: the DCR persist introduced in #31912 stores
client_id,client_secretandtoken_endpoint_auth_methodon the server row but discards theredirect_uristhe client was registered with. Every later connect short-circuits registration (either on the hydratedclient_idor through_reuse_persisted_dcr_client_if_available), whileauthorize_with_serverre-derives{current_origin}/callbackon every request. Once the resolved origin changes, the IdP rejects the mismatched pair forever; the rejection happens on the IdP side so nothing surfaces in proxy logs, and the only recovery was deleting and recreating the serverThe fix records
redirect_urisin the persisted credentials at DCR time and treats a positive mismatch with the current callback as a stale client on the admin register path (persist_credentials=True), falling through to a fresh registration that overwrites the persisted identity. A missing recording is treated as a match, which grandfathers both rows persisted before this field existed and admin-configured clients; this is deliberate, since re-minting a client_id orphans every user's refresh tokens for that server (the invarianttest_register_client_reuses_existing_client_id_without_re_dcrguards). Public register routes and non-admin callers keep today's behavior exactly, so recovery is an admin re-running the connect flow rather than any caller being able to churn upstream clientsBecause
update_mcp_servermerges credential blobs, the persist now writesclient_secretandtoken_endpoint_auth_methodexplicitly asNonewhen the registration response omits them; otherwise a re-registered public client would inherit the previous client's encrypted secret and auth method through the merge.redirect_urisrecords the callback the gateway sent rather than the IdP's echo so a normalizing IdP cannot cause a re-register loop. When re-registration happens the proxy logs a warning naming both bindings, which also covers the issue's request to surface the stranded stateRegression tests cover the four contract points: positive mismatch re-registers and persists the replacement (fails on unfixed code), a missing recording is grandfathered, a matching recording keeps the client, and the non-admin path reuses even on mismatch. The existing persist test now also pins the recorded
redirect_urisNote
Medium Risk
Touches MCP OAuth DCR persistence and client reuse; wrong mismatch logic could re-mint client_ids and force re-auth, but grandfathering and scoped admin-only re-register limit blast radius.
Overview
Fixes MCP OAuth servers getting permanently stranded after the proxy’s public origin changes: persisted DCR clients were reused while authorize always used the new
{origin}/callback, so strict IdPs rejected the pair with invalidredirect_uri.Persisted credentials now store
redirect_urisat registration time (MCPCredentials/_PersistedDcrCredentials). On the admin register path (persist_credentials=True), a recorded binding that does not include the current callback is treated as stale: registration is not short-circuited on the in-memoryclient_id, reuse is skipped, upstream DCR runs again, and the row is overwritten with the new client plus explicitNoneforclient_secret/token_endpoint_auth_methodso merged credential blobs do not leak the old secret.Missing
redirect_uris(pre-upgrade rows and admin-configured clients) is grandfathered—no automatic re-mint. Public / non-persisting register callers still reuse persisted clients even on mismatch, avoiding orphan upstream clients without persistence.Operator warning when re-registration happens; tests cover mismatch re-register, grandfathering, match reuse, and non-admin behavior.
Reviewed by Cursor Bugbot for commit 2254ffb. Bugbot is set up for automated code reviews on this repo. Configure here.