From 86506775c9506754c426e7ac15cfdc464358bff3 Mon Sep 17 00:00:00 2001 From: Tin Date: Fri, 10 Jul 2026 18:06:54 -0700 Subject: [PATCH] fix(mcp): preserve a stored client on OAuth-resume restore, add the edit upstream-mismatch warning, and type the credentials field --- .../mcp_tools/create_mcp_server.tsx | 8 ++--- .../mcp_tools/mcp_server_edit.test.tsx | 36 ++++++++++++++++++- .../components/mcp_tools/mcp_server_edit.tsx | 17 ++++++--- .../src/components/mcp_tools/types.tsx | 2 ++ 4 files changed, 52 insertions(+), 11 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 2230ad93cfa..25c712e5b01 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 @@ -322,13 +322,11 @@ const CreateMCPServer: React.FC = ({ setTransportType(restoredTransport); } if (parsed.formValues) { - // Strip minted token material from the restored credentials so a stale token never rehydrates - // into the form store (defense in depth for pre-fix snapshots); the declared app is kept. + // Assign the cleaned credentials (strip minted token material so a stale token never rehydrates); + // the declared app the admin typed is kept. Create has no server-side stored app to merge. const restoredValues = { ...parsed.formValues, - ...(parsed.formValues.credentials - ? { credentials: withoutMintedTokenCredentials(parsed.formValues.credentials) } - : {}), + credentials: withoutMintedTokenCredentials(parsed.formValues.credentials), }; setPendingRestoredValues({ values: restoredValues, transport: restoredTransport }); } 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 2980753f43e..758d8a80251 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 @@ -2,7 +2,8 @@ import React from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { render, screen, waitFor, fireEvent, act } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; -import MCPServerEdit from "./mcp_server_edit"; +import MCPServerEdit, { EDIT_OAUTH_UI_STATE_KEY } from "./mcp_server_edit"; +import { setSecureItem } from "@/utils/secureStorage"; import * as networking from "../networking"; import NotificationsManager from "../molecules/notifications_manager"; import { selectAntOption } from "./testUtils"; @@ -1548,6 +1549,39 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => { expect(screen.getByText(/registered for the previous upstream/)).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 + // under the snapshot before stripping tokens, so the stored client_id is never cleared to blank. + setSecureItem( + EDIT_OAUTH_UI_STATE_KEY, + JSON.stringify({ + serverId: "oauth_server_1", + formValues: { auth_type: "true_passthrough", credentials: { access_token: "leftover-token" } }, + }), + ); + + render( + , + ); + + const clientIdField = await screen.findByPlaceholderText("Leave blank to keep the currently saved app (if any)"); + await waitFor(() => expect((clientIdField as HTMLInputElement).value).toBe("stored-client")); + // The leftover minted token must not have rehydrated anywhere. + expect(document.body.innerHTML).not.toContain("leftover-token"); + }); + 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 009abe6ae14..e8af0b3be37 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 @@ -309,6 +309,7 @@ const MCPServerEdit: React.FC = ({ } syncedServerIdRef.current = mcpServer.server_id; form.setFieldsValue(initialValues); + setAppMayNotMatchUpstream(false); }, [mcpServer.server_id, initialValues, form]); // Initialize cost config from existing server data @@ -346,14 +347,19 @@ const MCPServerEdit: React.FC = ({ return; } if (parsed.formValues) { - // Strip minted token material from restored credentials so a stale token never rehydrates into - // the form store (defense in depth for pre-fix snapshots); the declared app is kept. + // Rebuild credentials from the declared app in EITHER the loaded server or the saved snapshot, + // then strip minted token material. Merging the two (server under snapshot) before stripping is + // what guarantees a token-only snapshot never clears a stored client_id/client_secret: the + // server's declared app survives and only the token keys drop. Assigning the cleaned result (not + // spreading the raw snapshot) also ensures a stale token can never rehydrate into the form. + const restoredCredentials = withoutMintedTokenCredentials({ + ...(mcpServer.credentials ?? {}), + ...((parsed.formValues.credentials as Record | undefined) ?? {}), + }); const restoredValues = { ...mcpServer, ...parsed.formValues, - ...(parsed.formValues.credentials - ? { credentials: withoutMintedTokenCredentials(parsed.formValues.credentials) } - : {}), + credentials: restoredCredentials, }; setPendingRestoredValues(restoredValues); } @@ -955,6 +961,7 @@ const MCPServerEdit: React.FC = ({ } NotificationsManager.success("MCP Server updated successfully"); + setAppMayNotMatchUpstream(false); onSuccess(updated); } catch (error: any) { NotificationsManager.fromBackend("Failed to update MCP Server" + (error?.message ? `: ${error.message}` : "")); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 3bab1cebb6e..0aceff87143 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -368,6 +368,8 @@ export interface MCPServer { delegate_auth_to_upstream?: boolean; oauth_passthrough?: boolean; max_concurrent_requests?: number | null; + /** Redacted to null in server responses; present when constructing a server locally. */ + credentials?: Record | null; /** Stdio-only fields (present when transport === 'stdio') */ command?: string | null;