fix(mcp): warn on upstream change when the stored OAuth app is redacted from the edit form - #32922
Conversation
…ed from the edit form The edit form's "app may not match upstream" warning only fired when a client was sitting in the form, so a stored app (redacted to null by the GET) never triggered it: an admin could repoint a client-forwarded server at a different upstream and silently keep an app registered for the old one. The backend now stamps a non-secret has_configured_client boolean on redacted responses (derived from the credentials blob at redaction time, or from the registry's decrypted client_id on the list path, whose table objects never carry the blob). The non-admin and virtual-key sanitizers null it back out; only the admin edit form needs it. The edit form fires the warning from the flag when the credential class is unchanged (a cross-class switch replaces the stored app, so nothing kept can mismatch), and the banner hides while the remove-app checkbox is checked since removal writes an explicit-null credential
Greptile SummaryThis PR adds a backend
Confidence Score: 4/5Safe to merge; the backend change is additive (a new response-only field never written to the DB) and the frontend change adds a warning path without altering any save logic. The implementation is well-structured and thoroughly tested across both the registry list path and the single-fetch DB path. The only issue found is a minor copy inconsistency in the warning banner on the create form. PassthroughAuthorizeSection.tsx — the revised warning text references 'remove the app' which is only actionable on the edit form.
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Stamps has_configured_client from the credentials blob before redacting; preserves a truthy flag from the build step for blob-free registry objects; correctly nulls it in both sanitizers. |
| ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx | Adds hasStoredKeptApp check using has_configured_client and credential-class comparison, correctly gating the URL-change warning for stored-but-redacted OAuth apps. |
| ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx | Warning suppressed while removeStoredApp is checked; copy updated but references 'remove the app' even on the create form where no remove-app checkbox exists. |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Stamps has_configured_client=bool(server.client_id) in _build_mcp_server_table for the registry list path. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Thorough unit tests covering all stamping/preserve/sanitize paths and an end-to-end fetch_mcp_server test. |
| litellm/models/mcp_server.py | Adds response-only has_configured_client: Optional[bool] field; never persisted, correctly defaulted to None. |
| ui/litellm-dashboard/src/components/mcp_tools/types.tsx | Adds `has_configured_client?: boolean |
Reviews (1): Last reviewed commit: "fix(mcp): warn on upstream change when t..." | Re-trigger Greptile
| You changed the upstream URL or endpoints; the OAuth app configured for this server was registered for the | ||
| previous upstream and may not be valid. Enter a client ID registered for the new upstream, or remove the app | ||
| to use dynamic client registration. |
There was a problem hiding this comment.
The revised warning text says "remove the app to use dynamic client registration", but on the create form
PassthroughAuthorizeSection is rendered with isEditing=false, so the remove-app checkbox is never shown. A user on the create flow who changes the upstream URL after typing a client ID will see the warning but find no checkbox to act on the "remove" instruction — the only way to dismiss it is to clear the client ID field themselves.
| You changed the upstream URL or endpoints; the OAuth app configured for this server was registered for the | |
| previous upstream and may not be valid. Enter a client ID registered for the new upstream, or remove the app | |
| to use dynamic client registration. | |
| You changed the upstream URL or endpoints; the OAuth app configured for this server was registered for the | |
| previous upstream and may not be valid. Enter a client ID registered for the new upstream | |
| {isEditing ? ", or remove the app to use dynamic client registration" : ", or clear the client ID to use dynamic client registration"}. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Relevant issues
Follow-up to #32752, which noted this gap: the "app may not match upstream" warning fires on a URL change only when a client is sitting in the form, so it works on create (the admin just typed the app) but stays silent on edit for a stored app, because the GET redacts
credentialsto null and blank fields mean keep-existing. An admin could repoint a client-forwarded server at a different upstream and silently keep an OAuth app registered for the old one. The form cannot fix this alone since it never sees the stored client; it needs a non-secret "a client exists" bit from the backend, which is a response-shape change and therefore its own PRLinear ticket
Resolves LIT-4682
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
UI flow to capture (before = 0c23c40, the #32752 merge on staging; after = 559fa91):
python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml --detailed_debug --reload --use_v2_migration_resolverandnpm run devinui/litellm-dashboard(port 3000)Backend flag, observable end to end on the same server:
returns
credentials: nullwithhas_configured_client: truefor the server above, andfalsefor servers without a stored client. Non-admin and virtual-key views returnhas_configured_client: nullType
🐛 Bug Fix
Changes
Backend:
LiteLLM_MCPServerTablegains a response-onlyhas_configured_clientboolean._redact_mcp_credentialsstamps it from the credentials blob it is about to remove;_build_mcp_server_tablestamps it from the registry's decryptedclient_id, because the list path (which feeds the dashboard grid and the edit form) builds table objects that never carry the blob, and the redactor preserves that stamp rather than resetting it. The non-admin and virtual-key sanitizers null the flag back out since only the admin edit form needs it. Nothing writes the field to the DB; create and update payloads go through the request models, which do not have itFrontend: the edit form's URL-change check now also fires when
has_configured_clientis true, gated on the credential class being unchanged (a cross-class auth switch replaces the stored app on save, so there is nothing kept to mismatch). The banner hides while the remove-app checkbox is checked, since removal writes an explicit-null credential, and reappears if it is unchecked. The warning copy now says "the OAuth app configured for this server" instead of "entered here", which was wrong for the stored case where the fields are blankTests: backend unit tests for the redactor stamp (blob present, absent, empty client_id, preserve-build-stamp), the sanitizer nulling, the table builder stamp, and an endpoint-level test through
fetch_mcp_server; frontend tests for the redacted-stored-app warning firing on URL change, the remove-checkbox hide/restore, the cross-class no-warn case, and the banner suppression prop