diff --git a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx new file mode 100644 index 000000000000..0a09ef3f856c --- /dev/null +++ b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.test.tsx @@ -0,0 +1,54 @@ +import React from "react"; +import { describe, it, expect } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { Form } from "antd"; +import PassthroughAuthorizeSection from "./PassthroughAuthorizeSection"; + +const WithForm: React.FC<{ children: React.ReactNode }> = ({ children }) => { + const [form] = Form.useForm(); + return
{children}
; +}; + +const noopFlow = { startOAuthFlow: () => {}, status: "idle", error: null, tokenResponse: null }; + +describe("PassthroughAuthorizeSection credential-class-aware copy", () => { + it("shows keep-existing copy when the credential class is unchanged (true_passthrough <-> oauth_delegate)", () => { + render( + + + , + ); + expect(screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)")).toBeInTheDocument(); + expect(screen.getByPlaceholderText("Leave blank to keep the currently saved secret (if any)")).toBeInTheDocument(); + }); + + it("shows the discard warning copy when switching from a different class (oauth2 -> true_passthrough)", () => { + render( + + + , + ); + expect(screen.getByPlaceholderText("Leave blank to use dynamic client registration")).toBeInTheDocument(); + expect(screen.getByPlaceholderText("Leave blank for public clients / PKCE")).toBeInTheDocument(); + expect(screen.getByText(/Switching the auth type discards the previously saved app/)).toBeInTheDocument(); + }); + + it("shows the keep+warn banner when the upstream may no longer match", () => { + render( + + + , + ); + expect(screen.getByText(/registered for the previous upstream/)).toBeInTheDocument(); + }); +}); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx index af81f2713aec..4cd551e4d106 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/PassthroughAuthorizeSection.tsx @@ -1,6 +1,6 @@ import React from "react"; -import { Button, Form, Input } from "antd"; -import { isClientForwardedTokenMode } from "./types"; +import { Button, Checkbox, Form, Input } from "antd"; +import { credentialAuthClass, isClientForwardedTokenMode } from "./types"; interface PassthroughOAuthFlow { startOAuthFlow: () => void | Promise; @@ -11,20 +11,40 @@ interface PassthroughOAuthFlow { /** * Browser-only Authorize & Fetch for the client-forwarded token modes - * (true_passthrough / oauth_delegate). LiteLLM never stores upstream - * credentials for these modes, so the token obtained here lives in this - * browser session only: it is forwarded per-server for the tools preview and - * allowlist configuration, and is never written to the server row or the - * per-user credential store. The optional client credentials cover IdPs - * without dynamic client registration (e.g. a pre-registered Slack app) and - * ride the temporary authorize session only. + * (true_passthrough / oauth_delegate). Tokens are never stored: the token + * obtained here lives in this browser session only, forwarded per-server for + * the tools preview and allowlist configuration, and is never written to the + * server row or the per-user credential store. The optional OAuth client + * credentials cover IdPs without dynamic client registration (e.g. a + * pre-registered Slack app); unlike the token they ARE saved onto the server + * as declared config, so internal users' Authorize relays through the org's + * app instead of dead-ending on upstreams that cannot mint clients. + * + * Blank fields follow the same convention as the M2M credential fields. On + * create they mean "no app configured" (dynamic client registration). On edit + * they mean "keep existing" ONLY when the credential class is unchanged: the + * backend merges a partial update within the client-forwarded class, so a + * true_passthrough <-> oauth_delegate switch keeps the stored app, but a switch + * from a different class (e.g. oauth2) replaces it, so blanks then mean "no + * app". Removing a stored app is an explicit checkbox (edit only) that writes + * an explicit-null credential. */ export default function PassthroughAuthorizeSection({ authType, oauthFlow, + isEditing = false, + savedAuthType, + removeStoredApp = false, + onRemoveStoredAppChange, + appMayNotMatchUpstream = false, }: { authType?: string | null; oauthFlow: PassthroughOAuthFlow; + isEditing?: boolean; + savedAuthType?: string | null; + removeStoredApp?: boolean; + onRemoveStoredAppChange?: (remove: boolean) => void; + appMayNotMatchUpstream?: boolean; }) { if (!isClientForwardedTokenMode(authType)) return null; const authorizeButtonLabels: Record = { @@ -32,32 +52,60 @@ export default function PassthroughAuthorizeSection({ exchanging: "Exchanging authorization code...", }; const authorizeButtonLabel = authorizeButtonLabels[oauthFlow.status] ?? "Authorize & Fetch Tools (browser-only)"; + // On edit, "keep existing" only holds when the stored credential class is unchanged; a cross-class + // switch (e.g. oauth2 -> true_passthrough) replaces credentials, so blanks then mean "no app". + const classUnchanged = isEditing && credentialAuthClass(savedAuthType) === credentialAuthClass(authType); + const clientIdPlaceholder = classUnchanged + ? "Leave blank to keep the currently saved app (if any)" + : "Leave blank to use dynamic client registration"; + const clientSecretPlaceholder = classUnchanged + ? "Leave blank to keep the currently saved secret (if any)" + : "Leave blank for public clients / PKCE"; + const clientIdExtra = classUnchanged + ? "Set this to make everyone authorize through a specific app; required for upstreams without dynamic client registration (e.g. a pre-registered Slack app)." + : "Switching the auth type discards the previously saved app; enter a client ID here or leave blank to use dynamic client registration."; return (

- Callers bring their own upstream token for this auth type, so LiteLLM stores no upstream credentials. To preview - tools and configure the tool allowlist, authorize against the upstream here: the token stays in this browser - session only and is never saved to LiteLLM. + Callers bring their own upstream token for this auth type, so LiteLLM never stores tokens. To preview tools and + configure the tool allowlist, authorize against the upstream here: the token stays in this browser session only + 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 && ( +

+ 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. +

+ )} OAuth Client ID (optional, not saved)} + label={OAuth Client ID (optional, saved)} name={["credentials", "client_id"]} - extra="Only needed when the upstream does not support dynamic client registration (e.g. a pre-registered Slack app). Used for this browser authorization only." + extra={clientIdExtra} > OAuth Client Secret (optional, not saved)} + label={OAuth Client Secret (optional, saved)} name={["credentials", "client_secret"]} > + {isEditing && onRemoveStoredAppChange && ( + onRemoveStoredAppChange(e.target.checked)}> + + Remove the saved OAuth app on save (the server goes back to dynamic client registration) + + + )}
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 02374a3ffa47..bc33ed79e495 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 @@ -31,6 +31,7 @@ const oauthHook = vi.hoisted(() => ({ | ((token: Record | null, registeredClient?: { clientId?: string; clientSecret?: string }) => void) | null, getCredentials: null as (() => Record | undefined) | null, + getTemporaryPayload: null as (() => Record | null) | null, })); vi.mock("@/hooks/useMcpOAuthFlow", () => ({ useMcpOAuthFlow: (opts: { @@ -39,9 +40,11 @@ vi.mock("@/hooks/useMcpOAuthFlow", () => ({ registeredClient?: { clientId?: string; clientSecret?: string }, ) => void; getCredentials?: () => Record | undefined; + getTemporaryPayload?: () => Record | null; }) => { oauthHook.onTokenReceived = opts.onTokenReceived; oauthHook.getCredentials = opts.getCredentials ?? null; + oauthHook.getTemporaryPayload = opts.getTemporaryPayload ?? null; return { startOAuthFlow: vi.fn(), status: "idle", @@ -202,8 +205,8 @@ describe("CreateMCPServer", () => { await waitFor(() => { expect(screen.getByRole("button", { name: "Authorize & Fetch Tools (browser-only)" })).toBeInTheDocument(); }); - expect(screen.getByText("OAuth Client ID (optional, not saved)")).toBeInTheDocument(); - expect(screen.getByText("OAuth Client Secret (optional, not saved)")).toBeInTheDocument(); + expect(screen.getByText("OAuth Client ID (optional, saved)")).toBeInTheDocument(); + expect(screen.getByText("OAuth Client Secret (optional, saved)")).toBeInTheDocument(); }, ); @@ -436,6 +439,434 @@ describe("CreateMCPServer", () => { ); }); + it.each([ + ["true_passthrough", "True Passthrough (no LiteLLM auth)"], + ["oauth_delegate", "OAuth Delegate (client-supplied upstream token)"], + ])( + "persists admin-entered OAuth app credentials on create for %s while the token stays browser-held", + async (_authType, optionLabel) => { + oauthHook.tokenResponse = { access_token: "upstream-tok", token_type: "Bearer" }; + await selectHttpTransport(); + + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "CF_App_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + + await selectAntOption("Authentication", optionLabel); + + // Admin declares the org's pre-registered upstream app; unlike the browser-authorized + // token, this is config and must survive onto the server row so internal users' + // Tools-page Authorize relays through it (required for non-DCR upstreams like Slack). + 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); + }); + + const createdServer = { + server_id: "new-cf-app-server", + server_name: "CF_App_Server", + alias: "CF_App_Server", + url: "https://example.com/mcp", + transport: "http", + auth_type: _authType, + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(createdServer); + + const submitButton = screen.getByRole("button", { name: "Add MCP Server" }); + await act(async () => { + fireEvent.click(submitButton); + }); + + await waitFor(() => expect(networking.createMCPServer).toHaveBeenCalledTimes(1)); + const [, payload] = vi.mocked(networking.createMCPServer).mock.calls[0]; + + // The declared app persists; the browser-authorized token still appears nowhere in the + // payload and no per-user DB credential is written. + expect(payload.credentials).toEqual({ + client_id: "org-app-client-id", + client_secret: "org-app-secret", + }); + expect(JSON.stringify(payload)).not.toContain("upstream-tok"); + expect(networking.storeMCPOAuthUserCredential).not.toHaveBeenCalled(); + expect(setToken).toHaveBeenCalledWith( + "new-cf-app-server", + expect.objectContaining({ access_token: "upstream-tok" }), + undefined, + ); + }, + ); + + 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" }, + }); + }); + + const keptAppServer = { + 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", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(keptAppServer); + + 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)"); + + const switchedServer = { + 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", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(switchedServer); + + 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("keeps the DCR-minted client out of form.credentials but reuses it via getCredentials", async () => { + await selectHttpTransport(); + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "DCR_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + await selectAntOption("Authentication", "OAuth"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!( + { access_token: "oauth2-tok", token_type: "Bearer" }, + { clientId: "dcr-client", clientSecret: "dcr-secret" }, + ); + }); + + // The DCR client must NOT be in the form store (or it could be collected as a CF server's app), + // but getCredentials merges it so a re-authorize reuses the registered client instead of re-DCRing. + expect(oauthHook.getCredentials?.()?.client_id).toBe("dcr-client"); + // getTemporaryPayload must mirror getCredentials for oauth2, or a re-authorize's temp session omits + // the registered client and useMcpOAuthFlow re-registers instead of reusing it. + expect(oauthHook.getTemporaryPayload?.()?.credentials).toMatchObject({ client_id: "dcr-client" }); + }); + + it("clears the DCR ref and the upstream warning when the modal closes so nothing leaks to the next session", async () => { + const { rerender } = render(); + await selectAntOption("Transport Type", "Streamable HTTP"); + await waitFor(() => expect(screen.getByPlaceholderText("https://your-mcp-server.com")).toBeInTheDocument()); + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "Leak_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + await selectAntOption("Authentication", "OAuth"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!( + { access_token: "oauth2-tok", token_type: "Bearer" }, + { clientId: "leak-client", clientSecret: "leak-secret" }, + ); + }); + // Ref is held while the modal is open. + expect(oauthHook.getCredentials?.()?.client_id).toBe("leak-client"); + + // A parent dismiss (isModalVisible -> false) that does not route through Cancel/Create must still + // clear the DCR ref, or the next server's oauth2 submit would carry this server's registered client. + await act(async () => { + rerender(); + }); + + expect(oauthHook.getCredentials?.()?.client_id).toBeUndefined(); + expect(oauthHook.getTemporaryPayload?.()?.credentials ?? {}).not.toMatchObject({ client_id: "leak-client" }); + }); + + it("persists the DCR client on an oauth2 submit via the ref", async () => { + await selectHttpTransport(); + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "DCR_Submit_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + await selectAntOption("Authentication", "OAuth"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!( + { access_token: "oauth2-tok", token_type: "Bearer" }, + { clientId: "dcr-client", clientSecret: "dcr-secret" }, + ); + }); + + const dcrSubmitServer = { + server_id: "dcr-submit", + server_name: "DCR_Submit_Server", + alias: "DCR_Submit_Server", + url: "https://example.com/mcp", + transport: "http", + auth_type: "oauth2", + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(dcrSubmitServer); + 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.client_id).toBe("dcr-client"); + expect(payload.credentials.client_secret).toBe("dcr-secret"); + }); + + // These two tests drive multiple antd auth-type switches; use single-shot fireEvent.change for the + // text fields (not per-keystroke userEvent.type) and a wider timeout so they do not flake under CI + // resource contention. The behavior under test is the credential preserve across the switches. + const fillText = (el: HTMLElement, value: string) => fireEvent.change(el, { target: { value } }); + + it("preserves the typed app across a switch between the two client-forwarded modes", async () => { + await selectHttpTransport(); + fillText(getServerNameInput(), "CF_Switch_Keep"); + fillText(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)"); + fillText(screen.getByPlaceholderText("Leave blank to use dynamic client registration"), "app-id"); + fillText(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "app-secret"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined); + }); + + await selectAntOption("Authentication", "OAuth Delegate (client-supplied upstream token)"); + + const switched = { + server_id: "cf-switch-keep", + server_name: "CF_Switch_Keep", + alias: "CF_Switch_Keep", + url: "https://example.com/mcp", + transport: "http", + auth_type: "oauth_delegate", + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(switched); + 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).toEqual({ client_id: "app-id", client_secret: "app-secret" }); + }, 60_000); + + it("preserves the typed app across a client-forwarded -> oauth2 -> client-forwarded round trip", async () => { + await selectHttpTransport(); + fillText(getServerNameInput(), "CF_Round"); + fillText(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)"); + fillText(screen.getByPlaceholderText("Leave blank to use dynamic client registration"), "app-id"); + fillText(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "app-secret"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined); + }); + + await selectAntOption("Authentication", "OAuth"); + await selectAntOption("Authentication", "True Passthrough (no LiteLLM auth)"); + + const cfRoundServer = { + server_id: "cf-round", + server_name: "CF_Round", + alias: "CF_Round", + 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", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(cfRoundServer); + 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).toEqual({ client_id: "app-id", client_secret: "app-secret" }); + }, 60_000); + + it("keeps the typed app but warns when the URL changes after a client-forwarded authorize", async () => { + await selectHttpTransport(); + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "CF_Warn"); + 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"), "app-id"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined); + }); + + await act(async () => { + fireEvent.change(screen.getByPlaceholderText("https://your-mcp-server.com"), { + target: { value: "https://other.example.com/mcp" }, + }); + }); + + // 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(); + }); + + it("keeps client_secret when only client_id is edited after a client-forwarded authorize", async () => { + await selectHttpTransport(); + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "CF_Keystroke"); + 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"), "app-id"); + await user.type(screen.getByPlaceholderText("Leave blank for public clients / PKCE"), "app-secret"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + await act(async () => { + oauthHook.onTokenReceived!({ access_token: "cf-tok", token_type: "Bearer" }, undefined); + }); + + // Editing only client_id fires an invalidation whose changedValues carries only the client_id + // sub-field; the preserve + deep-merge re-apply must keep client_secret from being dropped. + await user.type(screen.getByPlaceholderText("Leave blank to use dynamic client registration"), "2"); + + const cfKeystrokeServer = { + server_id: "cf-keystroke", + server_name: "CF_Keystroke", + alias: "CF_Keystroke", + 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", + }; + vi.mocked(networking.createMCPServer).mockResolvedValue(cfKeystrokeServer); + 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).toEqual({ client_id: "app-id2", client_secret: "app-secret" }); + }); + + it("replaces the token set on re-authorize instead of leaving stale siblings", async () => { + await selectHttpTransport(); + const user = userEvent.setup({ delay: null }); + await user.type(getServerNameInput(), "Reauth_Server"); + await user.type(screen.getByPlaceholderText("https://your-mcp-server.com"), "https://example.com/mcp"); + await selectAntOption("Authentication", "OAuth"); + + await waitFor(() => expect(oauthHook.onTokenReceived).toBeTruthy()); + const firstToken = { access_token: "T1", refresh_token: "R1", scope: "read", token_type: "Bearer" }; + await act(async () => { + oauthHook.onTokenReceived!(firstToken, undefined); + }); + await act(async () => { + oauthHook.onTokenReceived!({ access_token: "T2", token_type: "Bearer" }, undefined); + }); + + const creds = oauthHook.getCredentials?.() ?? {}; + expect(creds.access_token).toBe("T2"); + expect(creds.refresh_token).toBeUndefined(); + expect(creds.scope).toBeUndefined(); + }); + 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 eb48fd024742..25c712e5b01c 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,8 @@ import { getOAuthAuthorizationIdentity, CLEARED_ON_INVALIDATION, isHeldOAuthTokenStale, + preservedDeclaredAppCredentials, + withoutMintedTokenCredentials, } from "./types"; import OAuthFormFields from "./OAuthFormFields"; import TruePassthroughWarning from "./TruePassthroughWarning"; @@ -60,6 +62,8 @@ const AUTH_TYPES_REQUIRING_CREDENTIALS = [ AUTH_TYPE.OAUTH2, AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE, AUTH_TYPE.AWS_SIGV4, + AUTH_TYPE.TRUE_PASSTHROUGH, + AUTH_TYPE.OAUTH_DELEGATE, ]; const CREATE_OAUTH_UI_STATE_KEY = "litellm-mcp-oauth-create-state"; @@ -106,6 +110,14 @@ const CreateMCPServer: React.FC = ({ // was fetched; undefined when no valid token is held. If any mint-relevant field diverges from this, // the held token is stale and is discarded so the admin must re-authorize. const [authorizedIdentity, setAuthorizedIdentity] = useState(undefined); + // The DCR-minted OAuth client from an interactive (oauth2) Authorize. Held OUT of form.credentials so + // it can never be collected as a client-forwarded server's declared app; injected into the payload + // only on an oauth2 submit (where persisting the registered client is correct), and cleared on any + // invalidation or modal close. An abandoned authorize leaves it null, which is the desired asymmetry. + const dcrClientRef = React.useRef<{ client_id: string; client_secret?: string } | null>(null); + // Set when the upstream identity (url/endpoints) changed while a declared app is present, so the + // section can warn that the saved app may not match the new upstream (the app is kept, not wiped). + const [appMayNotMatchUpstream, setAppMayNotMatchUpstream] = useState(false); // Single hook call shared by MCPConnectionStatus and MCPToolConfiguration to avoid duplicate requests. const { @@ -147,6 +159,9 @@ const CreateMCPServer: React.FC = ({ searchValue, aliasManuallyEdited, logoUrl, + // Persist the identity so invalidation stays armed across the OAuth redirect round trip: a + // post-restore url/mode edit must still discard the held token instead of silently keeping it. + authorizedIdentity, }; setSecureItem(CREATE_OAUTH_UI_STATE_KEY, JSON.stringify(uiState)); } catch (err) { @@ -162,7 +177,12 @@ const CreateMCPServer: React.FC = ({ reset: resetOAuthFlow, } = useMcpOAuthFlow({ accessToken, - getCredentials: () => form.getFieldValue("credentials"), + // Merge the ref-held DCR client so a re-authorize reuses the registered client instead of + // re-registering; the form store itself never holds the DCR client (see onTokenReceived). + getCredentials: () => ({ + ...((form.getFieldValue("credentials") as Record | undefined) ?? {}), + ...(dcrClientRef.current ?? {}), + }), getTemporaryPayload: () => { const values = form.getFieldsValue(true); const transport = values.transport || transportType; @@ -184,7 +204,12 @@ const CreateMCPServer: React.FC = ({ url, transport: transport === TRANSPORT.OPENAPI ? "http" : transport, auth_type: isClientForwardedTokenMode(values.auth_type) ? values.auth_type : AUTH_TYPE.OAUTH2, - credentials: values.credentials, + // Mirror getCredentials: merge the ref-held DCR client for oauth2 so a re-authorize reuses the + // registered client (useMcpOAuthFlow keys reuse off credentials.client_id) instead of re-DCRing; + // the client-forwarded modes carry only the declared app. + credentials: isClientForwardedTokenMode(values.auth_type) + ? preservedDeclaredAppCredentials(values.credentials) + : { ...((values.credentials as Record | undefined) ?? {}), ...(dcrClientRef.current ?? {}) }, authorization_url: values.authorization_url, token_url: values.token_url, registration_url: values.registration_url, @@ -209,23 +234,36 @@ const CreateMCPServer: React.FC = ({ // edit form's onTokenReceived early return. setAuthorizedIdentity(getOAuthAuthorizationIdentity(form.getFieldsValue(true))); NotificationsManager.success( - "Token held for this browser session. Tools can now be previewed and configured; nothing will be saved to LiteLLM.", + "Token held for this browser session. Tools can now be previewed and configured; the token is not saved to LiteLLM.", ); return; } - const credentials = { + // The DCR-minted client is held in a ref, NOT written into form.credentials, so it can never be + // collected as a client-forwarded server's declared app; it is injected into the payload only on + // an oauth2 submit. An admin-typed client already lives in form.credentials and is left untouched. + dcrClientRef.current = registeredClient?.clientId + ? { + client_id: registeredClient.clientId, + ...(registeredClient.clientSecret && { client_secret: registeredClient.clientSecret }), + } + : null; + + const current = (form.getFieldValue("credentials") as Record | undefined) ?? {}; + const nextCredentials = { + ...(preservedDeclaredAppCredentials(current) ?? {}), + ...(current.scopes !== undefined && { scopes: current.scopes }), access_token: token.access_token, ...(token.refresh_token && { refresh_token: token.refresh_token }), ...(token.expires_in && { expires_in: token.expires_in }), ...(token.scope && { scope: token.scope }), - ...(registeredClient?.clientId && { client_id: registeredClient.clientId }), - ...(registeredClient?.clientSecret && { client_secret: registeredClient.clientSecret }), }; - - form.setFieldsValue({ credentials }); - // Capture the identity AFTER writing the DCR'd credentials so the held token is not spuriously - // invalidated by its own credential write. + // Path-replace (not deep-merge) so a re-authorize with fewer token fields does not leave stale + // siblings from the previous token behind; the admin-typed client keys and scopes are carried + // explicitly above. + form.setFieldValue("credentials", nextCredentials); + // Capture the identity AFTER writing the token so the held token is not spuriously invalidated by + // its own credential write. setAuthorizedIdentity(getOAuthAuthorizationIdentity(form.getFieldsValue(true))); NotificationsManager.success( @@ -246,7 +284,17 @@ const CreateMCPServer: React.FC = ({ clearTools(); resetOAuthFlow(); setAuthorizedIdentity(undefined); + dcrClientRef.current = null; + // Capture the admin-typed app before resetFields destroys it, then re-apply it: the app is + // upstream-scoped config, not minted material, so it survives every invalidation (the token is + // what gets discarded). Token-shaped keys are excluded by the helper's key filter. + const keptAppCredentials = preservedDeclaredAppCredentials(form.getFieldValue("credentials")); form.resetFields([...CLEARED_ON_INVALIDATION]); + if (keptAppCredentials) { + form.setFieldsValue({ credentials: keptAppCredentials }); + } + // Re-apply the in-flight edit last; rc-field-form deep-merges nested objects, so a changed + // credentials sub-field composes with the preserved sibling instead of replacing the object. const preserved = Object.fromEntries( CLEARED_ON_INVALIDATION.filter((key) => key in changedValues).map((key) => [key, changedValues[key]]), ); @@ -274,7 +322,18 @@ const CreateMCPServer: React.FC = ({ setTransportType(restoredTransport); } if (parsed.formValues) { - setPendingRestoredValues({ values: parsed.formValues, transport: restoredTransport }); + // 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, + credentials: withoutMintedTokenCredentials(parsed.formValues.credentials), + }; + setPendingRestoredValues({ values: restoredValues, transport: restoredTransport }); + } + if (typeof parsed.authorizedIdentity === "string") { + // Re-arm invalidation: without this the remounted form has authorizedIdentity=undefined, so a + // post-restore mode/url edit would never fire the stale-token discard. + setAuthorizedIdentity(parsed.authorizedIdentity); } if (parsed.costConfig) { setCostConfig(parsed.costConfig); @@ -500,8 +559,20 @@ const CreateMCPServer: React.FC = ({ const includeCredentials = restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type); - if (includeCredentials && credentialsPayload && Object.keys(credentialsPayload).length > 0) { - payload.credentials = credentialsPayload; + // Client-forwarded rows persist ONLY the declared app; strip any token material that lingered in + // the form (e.g. from a prior oauth2 authorize on the same session) so it can never reach the row. + const submitCredentials = isClientForwardedTokenMode(restValues.auth_type) + ? preservedDeclaredAppCredentials(credentialsPayload) + : credentialsPayload; + + if (includeCredentials && submitCredentials && Object.keys(submitCredentials).length > 0) { + payload.credentials = submitCredentials; + } + + // An interactive (oauth2) create persists its DCR-minted client from the ref (kept out of the + // form store); reuse a re-authorize's registered client instead of re-registering. + if (restValues.auth_type === AUTH_TYPE.OAUTH2 && dcrClientRef.current) { + payload.credentials = { ...(payload.credentials ?? {}), ...dcrClientRef.current }; } if (accessToken != null) { @@ -576,6 +647,9 @@ const CreateMCPServer: React.FC = ({ setHasToolAllowlistInteraction(false); setAliasManuallyEdited(false); setLogoUrl(undefined); + setAuthorizedIdentity(undefined); + dcrClientRef.current = null; + setAppMayNotMatchUpstream(false); setModalVisible(false); }; @@ -655,6 +729,8 @@ const CreateMCPServer: React.FC = ({ clearTools(); resetOAuthFlow(); setAuthorizedIdentity(undefined); + dcrClientRef.current = null; + setAppMayNotMatchUpstream(false); } }, [isModalVisible, form, clearTools, resetOAuthFlow]); @@ -663,12 +739,31 @@ const CreateMCPServer: React.FC = ({ const handleFormValuesChange = (changedValues: Record, allValues: Record) => { // Any change to a mint-relevant field (url, auth_type, oauth_flow_type, client creds/scopes, or the // authorization/token/registration endpoints — see getOAuthAuthorizationIdentity) makes a held token - // stale, so discard it and force a fresh authorize. When that happens, formValues must be rebuilt - // from the form's post-reset state, not the pre-reset allValues snapshot: the snapshot still holds - // the discarded token in credentials, and useTestMCPConnection reads formValues for tool preview. - if (isHeldOAuthTokenStale(allValues, authorizedIdentity)) { + // stale, so discard it and force a fresh authorize. The stale check reads getFieldsValue(true): the + // onValuesChange allValues argument holds only MOUNTED paths, so an unmounted identity field (e.g. + // an oauth_flow_type initialValue while in a client-forwarded mode) would compare as changed on + // every keystroke and churn the held token. When a clear happens, formValues is rebuilt from the + // 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); + } 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), authorizedIdentity)) { clearHeldOAuthToken(changedValues); - setFormValues({ ...form.getFieldsValue(true), ...changedValues }); + setFormValues(form.getFieldsValue(true)); return; } setFormValues(allValues); @@ -995,6 +1090,7 @@ const CreateMCPServer: React.FC = ({ error: oauthError, tokenResponse: oauthTokenResponse, }} + appMayNotMatchUpstream={appMayNotMatchUpstream} /> {shouldShowAuthValueField && ( 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 adb3e161da59..cbd18d145ce5 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 @@ -1,7 +1,9 @@ import React from "react"; import { describe, it, expect, vi, beforeEach } from "vitest"; import { render, screen, waitFor, fireEvent, act } from "@testing-library/react"; -import MCPServerEdit from "./mcp_server_edit"; +import userEvent from "@testing-library/user-event"; +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"; @@ -1377,6 +1379,258 @@ describe("MCPServerEdit (OAuth token persistence on save)", () => { }, ); + it.each([["true_passthrough"], ["oauth_delegate"]])( + "persists admin-entered OAuth app credentials in the update payload for the %s mode", + async (authType) => { + mockOauth.tokenResponse = { access_token: "cf-tok", expires_in: 1800, token_type: "bearer" }; + vi.mocked(networking.updateMCPServer).mockResolvedValue({ + ...interactiveOAuthServer, + auth_type: authType, + }); + + render( + , + ); + + const user = userEvent.setup({ delay: null }); + await user.type( + screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)"), + "org-app-client-id", + ); + await user.type( + screen.getByPlaceholderText("Leave blank to keep the currently saved secret (if any)"), + "org-app-secret", + ); + + 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]; + + // The declared app is config and persists onto the row; the browser-held token still never + // reaches the payload or the per-user credential store. + expect(payload.credentials).toMatchObject({ + client_id: "org-app-client-id", + client_secret: "org-app-secret", + }); + expect(JSON.stringify(payload)).not.toContain("cf-tok"); + expect(networking.storeMCPOAuthUserCredential).not.toHaveBeenCalled(); + }, + ); + + 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 keep the currently saved app (if any)"), + "org-app-client-id", + ); + await user.type( + screen.getByPlaceholderText("Leave blank to keep the currently saved secret (if any)"), + "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("sends an explicit-null credential write when removing the saved app for true_passthrough", async () => { + vi.mocked(networking.updateMCPServer).mockResolvedValue({ + ...interactiveOAuthServer, + auth_type: "true_passthrough", + }); + + render( + , + ); + + // Blank fields keep the stored app (the backend merges partial credential updates), so the + // edit form states that convention and removal is an explicit checkbox that saves nulls. + expect(screen.getByPlaceholderText("Leave blank to keep the currently saved app (if any)")).toBeInTheDocument(); + + fireEvent.click( + screen.getByRole("checkbox", { + name: /Remove the saved OAuth app on save/, + }), + ); + + 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.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("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("resets the remove-app checkbox on a server switch so it never deletes the next server's stored app", async () => { + vi.mocked(networking.updateMCPServer).mockResolvedValue({ + ...interactiveOAuthServer, + auth_type: "true_passthrough", + }); + + const { rerender } = render( + , + ); + + // Check "remove saved app" on server A. + fireEvent.click(screen.getByRole("checkbox", { name: /Remove the saved OAuth app on save/ })); + expect( + (screen.getByRole("checkbox", { name: /Remove the saved OAuth app on save/ }) as HTMLInputElement).checked, + ).toBe(true); + + // Switch the panel to server B without unmounting. + rerender( + , + ); + + // The checkbox must have reset, so saving server B does not send the explicit-null delete write. + expect( + (screen.getByRole("checkbox", { name: /Remove the saved OAuth app on save/ }) as HTMLInputElement).checked, + ).toBe(false); + + 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.credentials).not.toEqual({ client_id: null, client_secret: null }); + }); + 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 7446c96c40e7..4ff9723f65ad 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,8 @@ import { getOAuthAuthorizationIdentity, CLEARED_ON_INVALIDATION, isHeldOAuthTokenStale, + preservedDeclaredAppCredentials, + withoutMintedTokenCredentials, OAUTH_FLOW, MCP_OAUTH2_FLOW_M2M, MCP_OAUTH2_FLOW_INTERACTIVE, @@ -56,6 +58,8 @@ const AUTH_TYPES_REQUIRING_CREDENTIALS = [ AUTH_TYPE.OAUTH2, AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE, AUTH_TYPE.AWS_SIGV4, + AUTH_TYPE.TRUE_PASSTHROUGH, + AUTH_TYPE.OAUTH_DELEGATE, ]; export const EDIT_OAUTH_UI_STATE_KEY = "litellm-mcp-oauth-edit-state"; @@ -74,6 +78,10 @@ const MCPServerEdit: React.FC = ({ const [toolsError, setToolsError] = useState(null); 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>({}); @@ -179,7 +187,9 @@ const MCPServerEdit: React.FC = ({ url, transport, auth_type: isClientForwardedTokenMode(values.auth_type) ? values.auth_type : AUTH_TYPE.OAUTH2, - credentials: values.credentials, + credentials: isClientForwardedTokenMode(values.auth_type) + ? preservedDeclaredAppCredentials(values.credentials) + : values.credentials, mcp_access_groups: values.mcp_access_groups || mcpServer.mcp_access_groups, static_headers: staticHeaders, command: values.command, @@ -202,19 +212,23 @@ const MCPServerEdit: React.FC = ({ }; setToken(mcpServer.server_id, browserHeldToken, userID); NotificationsManager.success( - "Token held for this browser session. Tools can now be loaded and configured; nothing was saved to LiteLLM.", + "Token held for this browser session. Tools can now be loaded and configured; the token is not saved to LiteLLM.", ); return; } - const credentials = { + const current = (form.getFieldValue("credentials") as Record | undefined) ?? {}; + const nextCredentials = { + ...(preservedDeclaredAppCredentials(current) ?? {}), + ...(current.scopes !== undefined && { scopes: current.scopes }), access_token: token.access_token, ...(token.refresh_token && { refresh_token: token.refresh_token }), ...(token.expires_in && { expires_in: token.expires_in }), ...(token.scope && { scope: token.scope }), }; - - form.setFieldsValue({ credentials }); + // Path-replace (not deep-merge) so a re-authorize with fewer token fields does not leave stale + // siblings behind; the admin-typed client keys and scopes are carried explicitly above. + form.setFieldValue("credentials", nextCredentials); // Re-capture after writing credentials so the token is not invalidated by its own credential write. authorizedIdentityRef.current = getOAuthAuthorizationIdentity(form.getFieldsValue(true)); @@ -295,6 +309,11 @@ const MCPServerEdit: React.FC = ({ } syncedServerIdRef.current = mcpServer.server_id; form.setFieldsValue(initialValues); + // Reset per-server OAuth UI state so it never carries across a server switch without an unmount: a + // stale removeStoredApp would send an explicit-null credential write that deletes the new server's + // stored app, and a stale warning would show on a server whose upstream did not change. + setAppMayNotMatchUpstream(false); + setRemoveStoredApp(false); }, [mcpServer.server_id, initialValues, form]); // Initialize cost config from existing server data @@ -332,8 +351,24 @@ const MCPServerEdit: React.FC = ({ return; } if (parsed.formValues) { - setPendingRestoredValues({ ...mcpServer, ...parsed.formValues }); + // 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, + credentials: restoredCredentials, + }; + setPendingRestoredValues(restoredValues); } + // The ref is re-armed by onTokenReceived when the redirect completes the code exchange, so there + // is no separate restore-side re-arm here (writing a ref inside an effect is disallowed). if (parsed.costConfig) { setCostConfig(parsed.costConfig); } @@ -407,7 +442,13 @@ const MCPServerEdit: React.FC = ({ } setTools([]); resetOAuthFlow(); + // The admin-typed app is upstream-scoped config, not minted material, so it survives every + // invalidation; only the held token is discarded. Token-shaped keys are excluded by the filter. + const keptAppCredentials = preservedDeclaredAppCredentials(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]]), ); @@ -417,6 +458,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); } @@ -850,8 +906,22 @@ const MCPServerEdit: React.FC = ({ const includeCredentials = restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type); - if (includeCredentials && credentialsPayload && Object.keys(credentialsPayload).length > 0) { - payload.credentials = credentialsPayload; + // Client-forwarded rows persist ONLY the declared app; strip any token material lingering in the + // form (e.g. from a prior oauth2 authorize this session) so it can never reach the row. + const submitCredentials = isClientForwardedTokenMode(restValues.auth_type) + ? preservedDeclaredAppCredentials(credentialsPayload) + : credentialsPayload; + + if (includeCredentials && submitCredentials && Object.keys(submitCredentials).length > 0) { + payload.credentials = submitCredentials; + } + + // Explicit removal of a saved app for the client-forwarded modes, applied AFTER the filter so it + // always wins. Blank fields are the keep-existing convention (the backend merges partial + // credential updates), so removal must be an explicit-null write: encrypt skips nulls and the + // merge overrides the stored keys, returning the server to dynamic client registration. + if (removeStoredApp && isClientForwardedTokenMode(restValues.auth_type)) { + payload.credentials = { client_id: null, client_secret: null }; } const updated = await updateMCPServer(accessToken, payload); @@ -895,6 +965,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}` : "")); @@ -1040,6 +1111,11 @@ const MCPServerEdit: React.FC = ({ error: oauthError, tokenResponse: oauthTokenResponse, }} + isEditing + 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 c6faca1fb510..7ce803d583c9 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.test.tsx @@ -10,6 +10,9 @@ import { getOAuthAuthorizationIdentity, isHeldOAuthTokenStale, oauth2FlowToFormValue, + preservedDeclaredAppCredentials, + withoutMintedTokenCredentials, + credentialAuthClass, } from "./types"; describe("getOAuthAuthorizationIdentity", () => { @@ -180,3 +183,51 @@ describe("oauth2FlowToFormValue", () => { expect(oauth2FlowToFormValue(undefined)).toBeUndefined(); }); }); + +describe("preservedDeclaredAppCredentials", () => { + it("keeps only non-empty string declared-app keys and never token-shaped keys", () => { + expect(preservedDeclaredAppCredentials(undefined)).toBeUndefined(); + expect(preservedDeclaredAppCredentials({})).toBeUndefined(); + expect(preservedDeclaredAppCredentials({ client_id: 123 })).toBeUndefined(); + expect(preservedDeclaredAppCredentials({ client_id: "" })).toBeUndefined(); + expect(preservedDeclaredAppCredentials({ client_id: "a", access_token: "t", scopes: ["s"] })).toEqual({ + client_id: "a", + }); + expect(preservedDeclaredAppCredentials({ client_secret: "s" })).toEqual({ client_secret: "s" }); + expect(preservedDeclaredAppCredentials({ client_id: "a", client_secret: "b", refresh_token: "r" })).toEqual({ + client_id: "a", + client_secret: "b", + }); + }); +}); + +describe("withoutMintedTokenCredentials", () => { + it("drops token keys and keeps the declared app and other config", () => { + expect(withoutMintedTokenCredentials(undefined)).toBeUndefined(); + const mixed = { + client_id: "a", + client_secret: "b", + access_token: "t", + refresh_token: "r", + expires_in: 3600, + scope: "read", + scopes: ["read"], + }; + 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", () => { + it("collapses the client-forwarded modes to one class and leaves others distinct", () => { + expect(credentialAuthClass(AUTH_TYPE.TRUE_PASSTHROUGH)).toBe("client_forwarded"); + expect(credentialAuthClass(AUTH_TYPE.OAUTH_DELEGATE)).toBe("client_forwarded"); + expect(credentialAuthClass(AUTH_TYPE.OAUTH2)).toBe(AUTH_TYPE.OAUTH2); + expect(credentialAuthClass(null)).toBeNull(); + }); +}); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 3eba8b309685..0aceff87143e 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -96,6 +96,54 @@ export const getOAuthAuthorizationIdentity = (values: Record): // edit forms so what gets wiped cannot drift. export const CLEARED_ON_INVALIDATION = ["credentials"] as const; +// The declared-app filter over form.credentials. It is a pure key filter with no mode/transition +// guard because the surrounding code establishes that a client_id/client_secret in form.credentials +// is ALWAYS admin-typed in every reachable state: the create form holds the DCR-minted client in a +// ref and never writes it into the form store, the edit form's onTokenReceived never writes client +// keys, and the invalidation reset clears the whole object atomically. So preserving the string +// client keys across any invalidation (URL/endpoint edit, true_passthrough<->oauth_delegate switch, +// or a round trip through another mode) is always legitimate, while the output key filter excludes +// token-shaped keys so a preserve can never carry minted material through. Shared by both forms. +const DECLARED_APP_CREDENTIAL_KEYS = ["client_id", "client_secret"] as const; + +// Minted token material the oauth2 authorize path writes beside the app keys; stripped from restored +// snapshots and from any credentials that transit to the temp-session preview so a stale token never +// reaches the backend or a client-forwarded server row. +export const MINTED_TOKEN_CREDENTIAL_KEYS = ["access_token", "refresh_token", "expires_in", "scope"] as const; + +export const preservedDeclaredAppCredentials = ( + credentials: Record | null | undefined, +): Record | undefined => { + if (!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; +}; + +// Drop minted token keys, keeping everything else (the declared app plus any non-token config). +export const withoutMintedTokenCredentials = ( + credentials: Record | null | undefined, +): Record | undefined => { + if (!credentials) return undefined; + 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 +// a switch between them must NOT be treated as an app change. Mirrors the backend _credential_auth_class +// in db.py; kept in sync so the UI's keep-existing copy and the backend's merge cannot disagree. +export const credentialAuthClass = (authType: string | null | undefined): string | null => { + if (authType === AUTH_TYPE.TRUE_PASSTHROUGH || authType === AUTH_TYPE.OAUTH_DELEGATE) return "client_forwarded"; + return authType ?? null; +}; + // 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 @@ -320,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;