From 1be846680a9206249bba91d7d12cf27b0ae23d85 Mon Sep 17 00:00:00 2001 From: Tin Date: Fri, 10 Jul 2026 17:22:33 -0700 Subject: [PATCH] fix(mcp): show the upstream-mismatch warning on edit and return undefined from withoutMintedTokenCredentials so a restore never blanks a stored client --- .../mcp_tools/create_mcp_server.tsx | 15 ++++++---- .../mcp_tools/mcp_server_edit.test.tsx | 29 +++++++++++++++++++ .../components/mcp_tools/mcp_server_edit.tsx | 19 ++++++++++++ .../src/components/mcp_tools/types.test.tsx | 6 ++++ .../src/components/mcp_tools/types.tsx | 5 +++- 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx index 5172a487562..2230ad93cfa 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.tsx @@ -748,20 +748,23 @@ const CreateMCPServer: React.FC = ({ // form's post-reset state (not the pre-reset snapshot, which still holds the discarded token). // Editing the client fields is the admin managing/acknowledging the app, so it always dismisses // the "may not match upstream" warning regardless of the stale-token branch below. + // Editing the client fields is the admin managing/acknowledging the app, so it dismisses the "may + // not match upstream" warning. Otherwise a url/endpoint change while a declared app is present keeps + // the app but flags that it may not match the new upstream (the "keep + warn" behavior). This is + // independent of the held-token stale check below so it fires even without an authorize this session. if ("credentials" in changedValues) { setAppMayNotMatchUpstream(false); - } - if (isHeldOAuthTokenStale(form.getFieldsValue(true), authorizedIdentity)) { - // A url/endpoint change while a declared app is present keeps the app but flags that it may not - // match the new upstream (the "keep + warn" behavior); a client-key edit is handled above. + } else { const upstreamChanged = ["url", "spec_path", "authorization_url", "token_url", "registration_url"].some( (key) => key in changedValues, ); const hasDeclaredApp = preservedDeclaredAppCredentials(form.getFieldValue("credentials")) !== undefined; - clearHeldOAuthToken(changedValues); - if (upstreamChanged && hasDeclaredApp && !("credentials" in changedValues)) { + if (upstreamChanged && hasDeclaredApp) { setAppMayNotMatchUpstream(true); } + } + if (isHeldOAuthTokenStale(form.getFieldsValue(true), authorizedIdentity)) { + clearHeldOAuthToken(changedValues); setFormValues(form.getFieldsValue(true)); return; } 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 9cfe56f5fe2..2980753f43e 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 @@ -1519,6 +1519,35 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => { expect(payload.credentials).toEqual({ client_id: null, client_secret: null }); }); + it("warns that the saved app may not match after a URL change on a client-forwarded server", async () => { + render( + , + ); + + // No warning until the upstream changes. + 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" }, + }); + }); + + // Keep + warn parity with the create form: the stored app is kept, and the banner appears. + expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); + }); + it("forwards a newly authorized browser-held token for tool loading before the form is saved", async () => { // Regression: fetchTools keyed the browser-held decision off the saved mcpServer.auth_type, so // after switching the form to true_passthrough and authorizing, the fresh token was not sent as 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 741b23a12fa..009abe6ae14 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 @@ -79,6 +79,9 @@ const MCPServerEdit: React.FC = ({ const [searchValue, setSearchValue] = useState(""); const [aliasManuallyEdited, setAliasManuallyEdited] = useState(false); const [removeStoredApp, setRemoveStoredApp] = useState(false); + // Set when the upstream identity (url/endpoints) changed while a declared app is present, so the + // section warns that the saved app may not match the new upstream (the app is kept, not wiped). + const [appMayNotMatchUpstream, setAppMayNotMatchUpstream] = useState(false); const [allowedTools, setAllowedTools] = useState([]); const [hasToolAllowlistInteraction, setHasToolAllowlistInteraction] = useState(false); const [toolNameToDisplayName, setToolNameToDisplayName] = useState>({}); @@ -445,6 +448,21 @@ const MCPServerEdit: React.FC = ({ }; const handleFormValuesChange = (changedValues: Record) => { + // Editing the client fields dismisses the "may not match upstream" warning; otherwise a url/endpoint + // 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). + if ("credentials" in changedValues) { + setAppMayNotMatchUpstream(false); + } else { + const upstreamChanged = ["url", "spec_path", "authorization_url", "token_url", "registration_url"].some( + (key) => key in changedValues, + ); + const hasDeclaredApp = preservedDeclaredAppCredentials(form.getFieldValue("credentials")) !== undefined; + if (upstreamChanged && hasDeclaredApp) { + setAppMayNotMatchUpstream(true); + } + } if (isHeldOAuthTokenStale(form.getFieldsValue(true), authorizedIdentityRef.current)) { clearHeldOAuthToken(changedValues); } @@ -1086,6 +1104,7 @@ const MCPServerEdit: React.FC = ({ savedAuthType={mcpServer.auth_type} removeStoredApp={removeStoredApp} onRemoveStoredAppChange={setRemoveStoredApp} + appMayNotMatchUpstream={appMayNotMatchUpstream} /> )} diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx index 48f9fc2ac75..7ce803d583c 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx @@ -215,6 +215,12 @@ describe("withoutMintedTokenCredentials", () => { }; expect(withoutMintedTokenCredentials(mixed)).toEqual({ client_id: "a", client_secret: "b", scopes: ["read"] }); }); + + it("returns undefined (not {}) when only minted keys are present, so a restore never blanks the fields", () => { + expect(withoutMintedTokenCredentials({ access_token: "t", refresh_token: "r", expires_in: 3600 })).toBeUndefined(); + // A declared client is always kept, so a stored client_id can never be overwritten with empty. + expect(withoutMintedTokenCredentials({ client_id: "x", access_token: "t" })).toEqual({ client_id: "x" }); + }); }); describe("credentialAuthClass", () => { diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 29441cdb84c..3bab1cebb6e 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -128,9 +128,12 @@ export const withoutMintedTokenCredentials = ( credentials: Record | null | undefined, ): Record | undefined => { if (!credentials) return undefined; - return Object.fromEntries( + const kept = Object.fromEntries( Object.entries(credentials).filter(([key]) => !(MINTED_TOKEN_CREDENTIAL_KEYS as readonly string[]).includes(key)), ); + // Return undefined (not {}) when only minted keys were present, so a restore spreads `credentials: + // undefined` (the fields keep their placeholder / keep-existing state) rather than blanking them. + return Object.keys(kept).length > 0 ? kept : undefined; }; // The client-forwarded modes share one credential class (same declared app, same authorize relay), so