From 57051d36d6cdf5d136fb152b3bd9e8b5502fad45 Mon Sep 17 00:00:00 2001 From: Tin Date: Fri, 10 Jul 2026 09:39:37 -0700 Subject: [PATCH] fix(mcp): keep admin-declared app credentials through OAuth invalidation for the client-forwarded modes --- ui/litellm-dashboard/eslint-metrics.json | 2 +- .../mcp_tools/create_mcp_server.test.tsx | 104 ++++++++++++++++++ .../mcp_tools/create_mcp_server.tsx | 9 ++ .../mcp_tools/mcp_server_edit.test.tsx | 54 +++++++++ .../components/mcp_tools/mcp_server_edit.tsx | 9 ++ .../src/components/mcp_tools/types.tsx | 25 +++++ 6 files changed, 202 insertions(+), 1 deletion(-) diff --git a/ui/litellm-dashboard/eslint-metrics.json b/ui/litellm-dashboard/eslint-metrics.json index 2e204c63a48..9431c278387 100644 --- a/ui/litellm-dashboard/eslint-metrics.json +++ b/ui/litellm-dashboard/eslint-metrics.json @@ -1,7 +1,7 @@ { "@typescript-eslint/no-explicit-any": 1978, "complexity": 129, - "local/no-large-inline-object-arg": 512, + "local/no-large-inline-object-arg": 514, "local/no-long-condition-chain": 233, "max-depth": 59, "no-console": 15 diff --git a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx index 0a3128c6e7f..cfd0ec1dfb4 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/create_mcp_server.test.tsx @@ -503,6 +503,110 @@ describe("CreateMCPServer", () => { }, ); + it("preserves admin-entered app credentials when the URL changes after authorize for true_passthrough", async () => { + await selectHttpTransport(); + + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "CF_Keep_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + + await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)"); + + await user.type( + screen.getByPlaceholderText("Leave blank to use dynamic client registration"), + "org-app-client-id", + ); + await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!({ access_token: "upstream-tok", token_type: "Bearer" }, undefined); + }); + + // Editing the URL after authorize invalidates the held token (identity change), but the + // declared app is config, not minted material: it must survive the invalidation instead of + // being silently reset, or the server would persist without the configured app. + await act(async () => { + fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), { + target: { value: "https://other.example.com/mcp" }, + }); + }); + + vi.mocked(networking.createMCPServer).mockResolvedValue({ + server_id: "kept-app-server", + server_name: "CF_Keep_Server", + alias: "CF_Keep_Server", + url: "https://other.example.com/mcp", + transport: "http", + auth_type: "true_passthrough", + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + }); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Add MCP Server" })); + }); + + await waitFor(() => expect(networking.createMCPServer).toHaveBeenCalledTimes(1)); + const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0]; + expect(payload.url).toBe("https://other.example.com/mcp"); + expect(payload.credentials).toEqual({ + client_id: "org-app-client-id", + client_secret: "org-app-secret", + }); + expect(JSON.stringify(payload)).not.toContain("upstream-tok"); + }); + + it("wipes oauth2-minted credentials when the auth type switches to a client-forwarded mode", async () => { + await selectHttpTransport(); + + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "Switch_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + + await selectAntOption("Authentication", "OAuth"); + + // The oauth2 onTokenReceived branch writes the fetched token AND the DCR client into + // form.credentials; both are minted for the oauth2 identity. + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!( + { access_token: "oauth2-minted-tok", refresh_token: "oauth2-minted-refresh", token_type: "Bearer" }, + { clientId: "dcr-minted-client", clientSecret: "dcr-minted-secret" }, + ); + }); + + // Switching into a client-forwarded mode changes the identity with auth_type in the changed + // values, so the preserve carve-out must NOT apply: the minted material would otherwise ride + // into a mode that now persists credentials onto the server row. + await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)"); + + vi.mocked(networking.createMCPServer).mockResolvedValue({ + server_id: "switched-server", + server_name: "Switch_Server", + alias: "Switch_Server", + url: "https://example.com/mcp", + transport: "http", + auth_type: "true_passthrough", + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + }); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Add MCP Server" })); + }); + + await waitFor(() => expect(networking.createMCPServer).toHaveBeenCalledTimes(1)); + const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0]; + expect(payload.credentials).toBeUndefined(); + expect(JSON.stringify(payload)).not.toContain("dcr-minted-client"); + expect(JSON.stringify(payload)).not.toContain("oauth2-minted-tok"); + }); + it("should not show auth value field when None auth type is selected", async () => { await selectHttpTransport(); 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 79db031008e..a526bc93546 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 @@ -18,6 +18,7 @@ import { getOAuthAuthorizationIdentity, CLEARED_ON_INVALIDATION, isHeldOAuthTokenStale, + preservedDeclaredAppCredentials, } from "./types"; import OAuthFormFields from "./OAuthFormFields"; import TruePassthroughWarning from "./TruePassthroughWarning"; @@ -248,7 +249,15 @@ const CreateMCPServer: React.FC = ({ clearTools(); resetOAuthFlow(); setAuthorizedIdentity(undefined); + const keptAppCredentials = preservedDeclaredAppCredentials( + form.getFieldValue("auth_type"), + "auth_type" in changedValues, + form.getFieldValue("credentials"), + ); form.resetFields([...CLEARED_ON_INVALIDATION]); + if (keptAppCredentials) { + form.setFieldsValue({ credentials: keptAppCredentials }); + } const preserved = Object.fromEntries( CLEARED_ON_INVALIDATION.filter((key) => key in changedValues).map((key) => [key, changedValues[key]]), ); 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 a1bad0307b0..4e0129d4ba1 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 @@ -1423,6 +1423,60 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => { }, ); + it.each([["true_passthrough"], ["oauth_delegate"]])( + "preserves admin-entered app credentials when the URL changes after authorize for the %s mode", + async (authType) => { + vi.mocked(networking.updateMCPServer).mockResolvedValue({ + ...interactiveOAuthServer, + auth_type: authType, + }); + + render( + , + ); + + const user = userEvent.setup({ delay: null }); + await user.type( + screen.getByPlaceholderText("Leave blank to use dynamic client registration"), + "org-app-client-id", + ); + await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "org-app-secret"); + + act(() => { + mockOauth.onTokenReceived?.({ access_token: "cf-tok", token_type: "bearer" }); + }); + + // The URL edit invalidates the held browser token (removeToken fires), but the declared app + // is config and must survive the invalidation into the update payload. + await act(async () => { + fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), { + target: { value: "https://other.example.com/mcp" }, + }); + }); + expect(mockRemoveToken).toHaveBeenCalledWith("oauth_server_1", "user-1"); + + await act(async () => { + fireEvent.click(screen.getAllByRole("button", { name: "Save Changes" })[0]); + }); + + await waitFor(() => expect(networking.updateMCPServer).toHaveBeenCalledTimes(1)); + const [, payload] = vi.mocked(networking.updateMCPServer).mock.calls[0]; + expect(payload.url).toBe("https://other.example.com/mcp"); + expect(payload.credentials).toMatchObject({ + client_id: "org-app-client-id", + client_secret: "org-app-secret", + }); + expect(JSON.stringify(payload)).not.toContain("cf-tok"); + }, + ); + 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 9b2c5af861f..62f9b7e6931 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 @@ -8,6 +8,7 @@ import { getOAuthAuthorizationIdentity, CLEARED_ON_INVALIDATION, isHeldOAuthTokenStale, + preservedDeclaredAppCredentials, OAUTH_FLOW, MCP_OAUTH2_FLOW_M2M, MCP_OAUTH2_FLOW_INTERACTIVE, @@ -409,7 +410,15 @@ const MCPServerEdit: React.FC = ({ } setTools([]); resetOAuthFlow(); + const keptAppCredentials = preservedDeclaredAppCredentials( + getEffectiveAuthType(), + "auth_type" in changedValues, + form.getFieldValue("credentials"), + ); form.resetFields([...CLEARED_ON_INVALIDATION]); + if (keptAppCredentials) { + form.setFieldsValue({ credentials: keptAppCredentials }); + } const preserved = Object.fromEntries( CLEARED_ON_INVALIDATION.filter((key) => key in changedValues).map((key) => [key, changedValues[key]]), ); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 3eba8b30968..e49fd5d57e4 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -96,6 +96,31 @@ export const getOAuthAuthorizationIdentity = (values: Record): // edit forms so what gets wiped cannot drift. export const CLEARED_ON_INVALIDATION = ["credentials"] as const; +// The carve-out to the wipe above for the client-forwarded token modes: their onTokenReceived branch +// never writes minted material into form.credentials, so for them the field only ever holds the +// admin-DECLARED upstream app (persisted as server config since the modes joined +// AUTH_TYPES_REQUIRING_CREDENTIALS), and an intra-mode identity change (e.g. a URL edit after +// Authorize) must not silently discard it. Two guards make the preserve safe: it never applies when +// auth_type itself changed (the previous mode's onTokenReceived may have written a fetched token or +// DCR client into the same field, and those are minted for the old mode), and it only ever keeps the +// declared-app keys, so token-shaped keys can never ride through a preserve. Shared by the create and +// edit forms so the carve-out cannot drift. +const DECLARED_APP_CREDENTIAL_KEYS = ["client_id", "client_secret"] as const; + +export const preservedDeclaredAppCredentials = ( + authType: string | null | undefined, + authTypeChanged: boolean, + credentials: Record | null | undefined, +): Record | undefined => { + if (!isClientForwardedTokenMode(authType) || authTypeChanged || !credentials) return undefined; + const kept = Object.fromEntries( + DECLARED_APP_CREDENTIAL_KEYS.filter((key) => typeof credentials[key] === "string" && credentials[key] !== "").map( + (key) => [key, credentials[key] as string], + ), + ); + return Object.keys(kept).length > 0 ? kept : undefined; +}; + // True when a token was authorized in this session (authorizedIdentity recorded at mint time) and the // form's current identity no longer matches it. Every invalidation decision in both forms goes through // this single check: onValuesChange for user edits, and an explicit recheck after any programmatic