fix(mcp): stop persisting the DCR client onto true_passthrough and oauth_delegate server rows - #32735
Conversation
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge. The change is a two-line early return in an isolated helper; it cannot affect the OAuth2 persist path, and the discriminator test guarantees persistence still works for oauth2 servers. The production change is minimal and correctly scoped — one early return inside No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Adds early-return guard in _persist_dcr_client_registration for true_passthrough/oauth_delegate auth types, updates _raise_if_not_oauth2 docstring to document where the invariant is now enforced. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Adds parametrized regression tests for both passthrough modes asserting no DB write occurs, a discriminator test verifying oauth2 still persists, and a server-id isolation test verifying DCR clients are never adopted from sibling rows. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_resolver.py | Adds test verifying both resolve_credentials and has_user_token are keyed by server_id rather than upstream URL. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_oauth2_token_cache.py | Adds test verifying the M2M token cache is keyed by server_id so two servers with identical client_credentials config but different IDs each mint their own token. |
Reviews (3): Last reviewed commit: "test(mcp): pin credential isolation acro..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…uth_delegate server rows
1e0cee5 to
a786ba9
Compare
|
bugbot run |
|
@greptileai rereview |
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 a786ba9. Configure here.
Relevant issues
Linear 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)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Live proxy on localhost:4000 backed by real Postgres, running real Dynamic Client Registration against Linear's authorization server (https://mcp.linear.app); no mocks.
Before (bug present), captured at base commit 2a12707. Create a true_passthrough server, then run the admin Authorize register call against it:
After (fix applied), captured at 35d7231. Identical flow against a fresh true_passthrough server:
Control (fix applied, same commit 35d7231). A genuine oauth2 server still persists, so silent refresh keeps working for the interactive mode:
Type
🐛 Bug Fix
Changes
The admin dashboard's browser-only Authorize flow calls
POST /server/oauth/{server_id}/register, which forwardspersist_credentials=_user_is_full_admin(...)intoregister_client_with_server._persist_dcr_client_registrationhad no auth_type gate, so for a full admin it wrote the upstream-minted DCRclient_idonto the server row and stampedoauth2_flow="authorization_code"(withupdated_by="mcp_oauth_dcr") even fortrue_passthroughandoauth_delegateservers. Those modes promise the gateway holds no server-side OAuth state (the caller owns the upstream token;_raise_if_not_oauth2's docstring documents exactly this), so the stamp corrupts the row: a pass-through server acquires a gateway-held OAuth client identity, reuses it across later authorizes, and reads as half-oauth2 to anything that inspectsoauth2_flowThe fix is an early return in
_persist_dcr_client_registrationforis_true_passthrough/is_oauth_delegate, returning a new"skipped"result. The registration response is still relayed to the browser, so the Authorize UX is unchanged; the gateway just no longer records the client. Placing the gate inside the persist function (rather than at the management call site) covers any future caller that passespersist_credentials=True. The_raise_if_not_oauth2docstring is updated to state where the invariant is enforcedRegression tests mirror the existing
test_token_exchange_does_not_persist_for_client_forwarded_modespattern: a parametrized pair asserting noupdate_mcp_servercall fortrue_passthrough/oauth_delegate(both fail without the fix), plus an oauth2 discriminator asserting persistence still happens so the negative tests cannot pass vacuouslyA second commit (1e0cee5) pins the broader contract this bug sat inside: credentials bind to the server entry they were established for and never travel between server_ids that share an upstream URL. Three seams get an isolation test, one per credential kind: the v2 resolver's authorization_code arm plus its has_user_token discovery check (a token stored for server A resolves for A and is unauthorized for same-URL server B), the DCR persist/reuse path (a fresh server mints and persists its own client even when a same-URL sibling row already holds one), and the M2M client_credentials cache (identical config under two server_ids yields two tokens, two IdP POSTs). Each was mutation-checked by temporarily re-keying the corresponding lookup to the URL; every mutation fails exactly its test
Note
Medium Risk
Touches MCP OAuth registration persistence and documents credential scoping; behavior change is narrow but affects how pass-through server rows are interpreted and how admin Authorize interacts with DCR.
Overview
Stops the admin Authorize DCR path from writing OAuth client state onto
true_passthrough/oauth_delegateserver rows._persist_dcr_client_registrationnow returns"skipped"for those modes, sopersist_credentials=Trueno longer stampsoauth2_flowandclient_idon rows that are supposed to keep no gateway-held OAuth identity. The upstream registration response is still returned to the browser unchanged.Adds regression coverage that pass-through modes never call
update_mcp_server, that realoauth2servers still persist, and that DCR mint/reuse stays keyed byserver_idwhen two servers share the same upstream URL.Pins isolation by
server_id(not URL) with new tests: per-user authorization_code tokens andhas_user_token, DCR persist on the correct row, and M2Mclient_credentialstoken cache entries do not leak across distinctserver_ids with identical config.Reviewed by Cursor Bugbot for commit a786ba9. Bugbot is set up for automated code reviews on this repo. Configure here.