From 559fa91304418ded08628fcbf7544665c1a083a7 Mon Sep 17 00:00:00 2001 From: Tin Date: Sat, 11 Jul 2026 10:56:15 -0700 Subject: [PATCH] fix(mcp): warn on upstream change when the stored OAuth app is redacted 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 --- litellm/models/mcp_server.py | 6 + .../mcp_server/mcp_server_manager.py | 1 + .../mcp_management_endpoints.py | 23 +++- .../mcp_server/test_mcp_server_manager.py | 41 +++++++ .../test_mcp_management_endpoints.py | 115 ++++++++++++++++++ .../PassthroughAuthorizeSection.test.tsx | 17 +++ .../mcp_tools/PassthroughAuthorizeSection.tsx | 7 +- .../mcp_tools/create_mcp_server.test.tsx | 4 +- .../mcp_tools/mcp_server_edit.test.tsx | 97 +++++++++++++++ .../components/mcp_tools/mcp_server_edit.tsx | 12 +- .../src/components/mcp_tools/types.tsx | 6 + 11 files changed, 323 insertions(+), 6 deletions(-) diff --git a/litellm/models/mcp_server.py b/litellm/models/mcp_server.py index af2efa822b0..c2f20ac55da 100644 --- a/litellm/models/mcp_server.py +++ b/litellm/models/mcp_server.py @@ -101,6 +101,12 @@ class LiteLLM_MCPServerTable(LiteLLMPydanticObjectBase): byok_description: List[str] = Field(default_factory=list) byok_api_key_help_url: Optional[str] = None has_user_credential: Optional[bool] = None + has_configured_client: Optional[bool] = Field( + default=None, + description=( + "Response-only indicator that the stored (redacted) credentials include an OAuth client_id; never persisted" + ), + ) source_url: Optional[str] = None timeout: Optional[float] = None max_concurrent_requests: Optional[int] = None diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 1d681b43b9e..eb56bc3858e 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -4988,6 +4988,7 @@ class MCPServerManager: is_byok=server.is_byok, byok_description=server.byok_description, byok_api_key_help_url=server.byok_api_key_help_url, + has_configured_client=bool(server.client_id), source_url=server.source_url, instructions=server.instructions, timeout=server.timeout, diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index cab2a51a8ca..4c09f8038df 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -473,13 +473,31 @@ if MCP_AVAILABLE: def _redact_mcp_credentials( mcp_server: LiteLLM_MCPServerTable, ) -> LiteLLM_MCPServerTable: - """Return a copy of the MCP server object with credentials removed.""" + """Return a copy of the MCP server object with credentials removed. + + Stamps ``has_configured_client`` before redacting so the admin edit form + can tell that a stored OAuth app exists without ever seeing its value + (the URL-change "app may not match upstream" warning needs exactly this + bit; the stored ``client_id`` itself is encrypted and never returned). + Derives from the credentials blob when the object carries one (DB reads), + otherwise preserves a truthy flag already stamped upstream + (``_build_mcp_server_table`` on the registry list path, whose tables + never include the blob). + """ try: redacted_server = mcp_server.model_copy(deep=True) except AttributeError: redacted_server = mcp_server.copy(deep=True) # type: ignore[attr-defined] + stored_credentials = getattr(mcp_server, "credentials", None) + stored_client_id = stored_credentials.get("client_id") if isinstance(stored_credentials, dict) else None + setattr( + redacted_server, + "has_configured_client", + bool(stored_client_id or getattr(mcp_server, "has_configured_client", None)), + ) + if hasattr(redacted_server, "credentials"): setattr(redacted_server, "credentials", None) @@ -548,6 +566,8 @@ if MCP_AVAILABLE: # admin configured. Non-admins get the per-user vars they must fill in # from the dedicated /user-env-vars/status endpoint instead. sanitized.env_vars = None + # Only the admin edit form needs the stored-app indicator. + sanitized.has_configured_client = None return sanitized def _sanitize_mcp_server_list_for_non_admin( @@ -591,6 +611,7 @@ if MCP_AVAILABLE: sanitized.health_check_error = None sanitized.last_health_check = None + sanitized.has_configured_client = None sanitized.created_by = None sanitized.updated_by = None diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index 7c55bd4560f..96a5a273149 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -7335,3 +7335,44 @@ def test_build_mcp_server_table_carries_null_oauth2_flow(): table = manager._build_mcp_server_table(server) assert table.oauth2_flow is None + + +def test_build_mcp_server_table_stamps_has_configured_client(): + """The list endpoint serves registry servers through this conversion WITHOUT the + credentials blob, so the redaction layer cannot see the stored client there. The + build must stamp has_configured_client from the registry's decrypted client_id or + the edit form never learns a saved OAuth app exists (its URL-change "app may not + match upstream" warning would stay silent for stored apps).""" + manager = MCPServerManager() + server = MCPServer( + server_id="stored-app-server", + name="stored_app_server", + server_name="stored_app_server", + alias="stored_app_server", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.true_passthrough, + client_id="org-slack-app-client-id", + client_secret="org-slack-app-secret", + ) + + table = manager._build_mcp_server_table(server) + + assert table.has_configured_client is True + + +def test_build_mcp_server_table_has_configured_client_false_without_client(): + manager = MCPServerManager() + server = MCPServer( + server_id="no-app-server", + name="no_app_server", + server_name="no_app_server", + alias="no_app_server", + url="https://up.example.com/mcp", + transport=MCPTransport.http, + auth_type=MCPAuth.true_passthrough, + ) + + table = manager._build_mcp_server_table(server) + + assert table.has_configured_client is False diff --git a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py index a669a277d2b..062a1c5e049 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py @@ -5376,3 +5376,118 @@ async def test_edit_mcp_server_snapshot_failure_skips_purge_but_edit_succeeds(): assert result.server_id == server_id mock_purge.assert_not_awaited() + + +def test_redact_stamps_has_configured_client_from_stored_blob(): + """The GET redacts credentials to null, so the edit form cannot see a stored OAuth + app; has_configured_client is the non-secret existence bit the URL-change "app may + not match upstream" warning keys on. Redaction must stamp it from the blob it is + about to remove, and must not leak or mutate the blob itself.""" + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + _redact_mcp_credentials, + ) + + server = generate_mock_mcp_server_db_record() + server.credentials = {"client_id": "encrypted-client", "client_secret": "encrypted-secret"} + + redacted = _redact_mcp_credentials(server) + + assert redacted.has_configured_client is True + assert redacted.credentials is None + assert server.credentials == {"client_id": "encrypted-client", "client_secret": "encrypted-secret"} + + +@pytest.mark.parametrize( + "credentials", + [None, {"auth_value": "top-secret"}, {"client_id": ""}], + ids=["no-blob", "no-client-in-blob", "empty-client-id"], +) +def test_redact_stamps_has_configured_client_false_without_stored_client(credentials): + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + _redact_mcp_credentials, + ) + + server = generate_mock_mcp_server_db_record() + server.credentials = credentials + + redacted = _redact_mcp_credentials(server) + + assert redacted.has_configured_client is False + assert redacted.credentials is None + + +def test_redact_preserves_build_time_has_configured_client(): + """The list endpoint serves registry servers whose table objects never carry the + credentials blob; _build_mcp_server_table stamps the flag instead. Redaction must + preserve that stamp rather than resetting it to False for lack of a blob.""" + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + _redact_mcp_credentials, + ) + + server = generate_mock_mcp_server_db_record() + server.credentials = None + server.has_configured_client = True + + redacted = _redact_mcp_credentials(server) + + assert redacted.has_configured_client is True + + +def test_sanitized_views_drop_has_configured_client(): + """Only the admin edit form needs the stored-app indicator; the non-admin and + virtual-key discovery views must not reveal whether an OAuth app is configured.""" + import litellm.proxy.management_endpoints.mcp_management_endpoints as mgmt + + server = generate_mock_mcp_server_db_record() + server.credentials = {"client_id": "encrypted-client"} + + assert mgmt._sanitize_mcp_server_for_non_admin(server).has_configured_client is None + assert mgmt._sanitize_mcp_server_for_virtual_key(server).has_configured_client is None + + +@pytest.mark.asyncio +async def test_fetch_single_mcp_server_returns_has_configured_client(): + """End to end through GET /v1/mcp/server/{id}: a stored client surfaces only as + has_configured_client=True while the credentials stay redacted.""" + mock_server = generate_mock_mcp_server_db_record(server_id="server-1", alias="Server 1") + mock_server.credentials = {"client_id": "encrypted-client", "client_secret": "encrypted-secret"} + + mock_prisma_client = MagicMock() + + mock_health_result = generate_mock_mcp_server_db_record(server_id="server-1", alias="Server 1") + mock_health_result.status = "healthy" + mock_health_result.last_health_check = datetime.now() + mock_health_result.health_check_error = None + + mock_user_auth = generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN) + + with ( + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.get_prisma_client_or_throw", + return_value=mock_prisma_client, + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.get_mcp_server", + AsyncMock(return_value=mock_server), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints.global_mcp_server_manager.health_check_server", + AsyncMock(return_value=mock_health_result), + ), + patch( + "litellm.proxy.management_endpoints.mcp_management_endpoints._user_has_admin_view", + return_value=True, + ), + ): + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + fetch_mcp_server, + ) + + result = await fetch_mcp_server( + request=_make_mock_request(), + server_id="server-1", + user_api_key_dict=mock_user_auth, + ) + + assert result.has_configured_client is True + assert result.credentials is None diff --git a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx index 0a09ef3f856..b5503695ad3 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx @@ -51,4 +51,21 @@ describe("PassthroughAuthorizeSection credential-class-aware copy", () => { ); expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); }); + + it("hides the keep+warn banner while the remove-stored-app checkbox is checked", () => { + render( + + {}} + /> + , + ); + expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument(); + }); }); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx index 0ed4ee555d1..14bcdef234f 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx @@ -75,10 +75,11 @@ export default function PassthroughAuthorizeSection({ and is never saved to LiteLLM. An OAuth app configured below IS saved with the server, so internal users who authorize from the Tools page go through it.

- {appMayNotMatchUpstream && ( + {appMayNotMatchUpstream && !removeStoredApp && (

- You changed the upstream URL or endpoints; the OAuth app entered here was registered for the previous upstream - and may not be valid. Update the client ID, or clear it 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, or remove the app + to use dynamic client registration.

)} { }); // Keep + warn: the app stays in the field, and a non-blocking warning appears. - expect(screen.getByText(/OAuth app entered here was registered for the previous upstream/)).toBeInTheDocument(); + expect( + screen.getByText(/OAuth app configured for this server was registered for the previous upstream/), + ).toBeInTheDocument(); }); it("keeps client_secret when only client_id is edited after a client-forwarded authorize", async () => { diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx index 5b8d0aac8c0..324c2fb4ef9 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.test.tsx @@ -1549,6 +1549,103 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => { expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); }); + it("warns after a URL change when the stored app is redacted (has_configured_client, blank fields)", async () => { + // The real GET redacts credentials to null, so the form holds no client even though the server + // has a saved app. has_configured_client is the backend's non-secret "a client exists" bit; the + // warning must fire from it, otherwise keep-existing silently keeps an app registered for the + // old upstream and the admin is never told. + render( + , + ); + + expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument(); + + await act(async () => { + fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), { + target: { value: "https://different.example.com/mcp" }, + }); + }); + + expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); + }); + + it("hides the stored-app warning while the remove checkbox is checked and restores it on uncheck", async () => { + // Removal writes an explicit-null credential on save, so nothing kept can mismatch; unchecking + // returns to keep-existing, where the mismatch concern is live again. + render( + , + ); + + await act(async () => { + fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), { + target: { value: "https://different.example.com/mcp" }, + }); + }); + expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); + + const removeCheckbox = screen.getByRole("checkbox", { name: /Remove the saved OAuth app on save/ }); + fireEvent.click(removeCheckbox); + expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument(); + + fireEvent.click(removeCheckbox); + expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); + }); + + it("does not warn from has_configured_client after a cross-class auth switch", async () => { + // Saved oauth2 server with a stored client (e.g. a persisted DCR app). Switching to + // true_passthrough is a cross-class change: blanks mean "no app" and the stored app is replaced + // on save, so a URL change has nothing kept to warn about. + render( + , + ); + + await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)"); + + await act(async () => { + fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), { + target: { value: "https://different.example.com/mcp" }, + }); + }); + + expect(screen.queryByText(/registered for the previous upstream/)).not.toBeInTheDocument(); + }); + it("preserves a stored client_id on OAuth-resume restore even when the saved snapshot is token-only", async () => { // Post-redirect restore: the sessionStorage snapshot carries only a minted token (no client keys), // while the loaded server has a stored client_id. The restore must merge the server's declared app diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx index 6709eb02c65..f791a006bad 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_edit.tsx @@ -4,6 +4,7 @@ import { InfoCircleOutlined } from "@ant-design/icons"; import { Button, TabGroup, TabList, Tab, TabPanels, TabPanel } from "@tremor/react"; import { AUTH_TYPE, + credentialAuthClass, isClientForwardedTokenMode, getOAuthAuthorizationIdentity, CLEARED_ON_INVALIDATION, @@ -463,6 +464,10 @@ const MCPServerEdit: React.FC = ({ // change while a declared app is present keeps the app but flags that it may not match the new // upstream (the "keep + warn" behavior). Mirrors the create form; independent of the held-token // stale check so it fires even without an authorize this session (the stored app is for the old url). + // A stored app counts too: the GET redacts credentials to null, so blank fields (keep-existing) can + // still hide a saved client. has_configured_client is the backend's non-secret "a client exists" + // bit; it only warns while the credential class is unchanged, because a cross-class switch replaces + // the stored app on save (blanks then mean "no app"), so there is nothing kept to mismatch. if ("credentials" in changedValues) { setAppMayNotMatchUpstream(false); } else { @@ -470,7 +475,12 @@ const MCPServerEdit: React.FC = ({ (key) => key in changedValues, ); const hasDeclaredApp = preservedDeclaredAppCredentials(form.getFieldValue("credentials")) !== undefined; - if (upstreamChanged && hasDeclaredApp) { + const formAuthType = form.getFieldValue("auth_type") as string | undefined; + const hasStoredKeptApp = + mcpServer.has_configured_client === true && + isClientForwardedTokenMode(formAuthType) && + credentialAuthClass(mcpServer.auth_type) === credentialAuthClass(formAuthType); + if (upstreamChanged && (hasDeclaredApp || hasStoredKeptApp)) { setAppMayNotMatchUpstream(true); } } diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 04766aad7b4..154325dde1d 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -371,6 +371,12 @@ export interface MCPServer { max_concurrent_requests?: number | null; /** Redacted to null in server responses; present when constructing a server locally. */ credentials?: Record | null; + /** + * Response-only: true when the stored (redacted) credentials include an OAuth client_id. Lets the + * edit form know a saved app exists without ever seeing its value, so the "app may not match + * upstream" warning can fire on a URL change even though `credentials` arrives null. + */ + has_configured_client?: boolean | null; /** Stdio-only fields (present when transport === 'stdio') */ command?: string | null;