fix(mcp): merge credentials within the client-forwarded class on an auth-type switch - #32815
Conversation
Greptile SummaryThis PR fixes
Confidence Score: 5/5The change is safe to merge: the credential-class predicate is straightforward, the Json(None) translation is isolated to the write edge, and all new code paths have dedicated mock tests. The _credential_auth_class helper is simple and correct — None maps to None (not client_forwarded), so a legacy NULL-auth_type row correctly counts as a cross-class change. The _drop_stale_minted_on_client_rotation guard fires on any client_id/client_secret update, which is the right trigger for invalidating minted tokens. dcr_bridge remains in _AUTH_FLOW_SCOPED_FIELDS and the two new dcr_bridge tests confirm both the clear-on-cross-class and preserve-on-within-class paths. The Json(None) edge translation fixes a real prisma rejection without altering merge logic. No existing assertions were weakened. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/db.py | Introduces _credential_auth_class to collapse true_passthrough/oauth_delegate to one client_forwarded class, _drop_stale_minted_on_client_rotation to strip old tokens on client rotation, and Json(None) translation at the prisma edge; all three gates in update_mcp_server now key off the class rather than the raw auth_type string. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_partial_update.py | Adds six new tests covering within-class switch without credentials (keeps app+endpoints), within-class partial credential merge, NULL-to-client-forwarded cleared as Json(None), client rotation stripping minted tokens, dcr_bridge cleared on cross-class switch, and dcr_bridge preserved on within-class switch; existing assertion updated to accept Json(None) sentinel. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_sigv4_auth.py | Cosmetic reformatting only (line-length unwrapping) plus one updated assertion in TestAuthTypeSwitchClearsCredentials that accepts Json(None) alongside bare None to match the corrected prisma-write path. |
Reviews (5): Last reviewed commit: "fix(mcp): merge credentials within the c..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
fbc656d to
e5020da
Compare
|
Good catch, and it was a real regression from how I split this PR out of a larger branch. The larger branch was based on staging before #32747 landed dcr_bridge in _AUTH_FLOW_SCOPED_FIELDS; when I extracted the backend into this PR I checked the file out wholesale onto current staging, which silently reverted that addition. Fixed in e5020da: dcr_bridge is back in _AUTH_FLOW_SCOPED_FIELDS (and in the test's stale-field lists), so a cross-class switch out of a client-forwarded mode still clears it. The interaction with the new credential class is the intended one: a cross-class switch (e.g. true_passthrough -> api_key, or oauth_delegate -> oauth2) sets auth_type_changed=True, runs the flow-scoped sweep, and nulls dcr_bridge unless the caller set it explicitly; a within-class switch (true_passthrough <-> oauth_delegate) is not a credential-class change, so the sweep does not run and dcr_bridge stays, which is correct since both modes use the DCR bridge relay. Added two tests to pin exactly this: test_cf_to_non_cf_switch_clears_dcr_bridge and test_cf_pair_switch_does_not_clear_dcr_bridge, and the pre-existing test_auth_type_switch_to_client_forwarded_keeps_explicit_dcr_bridge still passes unchanged. |
e5020da to
7d64f9d
Compare
|
Pushed 7d64f9d: fixed a second test that asserted the old raw-None cleared-credentials contract (test_mcp_sigv4_auth.py TestAuthTypeSwitchClearsCredentials); it now accepts the Json(None) SQL-null form the prisma fix writes. This was the proxy-infra failure. @greptileai |
|
@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 7d64f9d. Configure here.
|
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 7d64f9d. Configure here.
Relevant issues
Backend half of the MCP configured-OAuth-app work; the dashboard half is in the companion PR. Merge this first: the dashboard's keep-existing copy for a true_passthrough <-> oauth_delegate switch is only accurate once this merge behavior is in place
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. Captured at 69f0a6d (base) versus this branch.
An MCP server's OAuth app (client_id / client_secret) is a property of the upstream, not of the gateway's handling mode, so true_passthrough and oauth_delegate share one stored-credential shape and a switch between them must keep the app. Before this change update_mcp_server treated any auth_type change as a full credential replace, so the switch silently wiped the stored app:
A cross-class switch still replaces, correctly:
That HTTP 202 on the cross-class switch is also a fix: a live run surfaced that clearing the credentials wrote a raw Python None to the Json? column, which prisma-python rejects with "value is required but not set" (a 500). The clear path now writes Json(None), which stores SQL null and reads back as None.
Type
🐛 Bug Fix
Changes
update_mcp_server decided credential handling from a raw auth_type inequality, so it treated true_passthrough and oauth_delegate as different and ran the full replace path (null the credentials column, null the auth-flow-scoped endpoint columns, skip the per-key merge) on a switch between them. The edit form cannot repair this because the server GET redacts secrets, so it never resends the stored app; the server silently reverted to dynamic client registration and dead-ended on upstreams that lack it
The fix introduces a credential class: _credential_auth_class collapses the two client-forwarded modes to one class and leaves every other auth_type as its own. One predicate now drives all three gates, so a within-class switch keeps the endpoint columns and takes the per-key merge (preserving the stored app, and preserving a stored client_secret when only client_id is re-sent), while a cross-class switch still replaces. A NULL existing auth_type now counts as a cross-class change so a legacy row's stale blob is cleared, and a client rotation drops stale minted token keys the update did not itself set. Separately, the clear path translates its None sentinel to Json(None) at the update call so prisma-python writes SQL null instead of raising
Tests extend the mapped test_mcp_partial_update.py: a within-class switch with no credentials keeps the app and the endpoint columns; a within-class switch with a partial credential merges rather than replaces; a NULL-to-client-forwarded switch clears the blob and is written as Json(None); and a client rotation strips stale minted token keys. Each fails if the predicate reverts to the raw inequality or the minted-strip is dropped
Note
Medium Risk
Changes OAuth credential merge/clear rules on MCP admin updates, which affects stored secrets and token material, but behavior is narrowly scoped to
update_mcp_serverwith extensive new tests.Overview
MCP server updates now treat
true_passthroughandoauth_delegateas one client-forwarded credential class, so switching between those modes keeps the stored OAuth app, endpoint columns, and per-key credential merge instead of wiping secrets the UI cannot resend.Cross-class
auth_typechanges still clear flow-scoped fields and replace credentials (including legacy rows withNULLauth_type). Partial merges also drop stale minted tokens (access_token,refresh_token,expires_in) whenclient_idorclient_secretrotates without those keys in the update. Clearing credentials now persists asJson(None)so Prisma accepts SQL null instead of erroring on a bareNone.Tests in
test_mcp_partial_update.pycover within-class switches, NULL-to-CF clears, client rotation, anddcr_bridgebehavior; SigV4 tests only adjust assertions/formatting.Reviewed by Cursor Bugbot for commit 7d64f9d. Bugbot is set up for automated code reviews on this repo. Configure here.