diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.test.ts new file mode 100644 index 00000000000..d1ee7023405 --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.test.ts @@ -0,0 +1,250 @@ +import { describe, it, expect } from "vitest"; + +import { buildEditServerPayload, EditServerUiState } from "./editServerPayload"; +import { MCPServer } from "@/components/mcp_tools/types"; + +const SERVER: MCPServer = { + server_id: "srv_1", + server_name: "srv", + alias: "srv_alias", + description: "a server", + transport: "http", + url: "https://example.com/mcp", + auth_type: "none", + created_at: "2024-01-01T00:00:00Z", + created_by: "user-1", + updated_at: "2024-01-01T00:00:00Z", + updated_by: "user-1", + mcp_access_groups: [], +}; + +const UI: EditServerUiState = { + costConfig: {}, + allowedTools: [], + hasExistingToolAllowlist: false, + hasToolAllowlistInteraction: false, + toolNameToDisplayName: {}, + toolNameToDescription: {}, + logoUrl: undefined, + removeStoredApp: false, +}; + +const VALUES = { transport: "http", auth_type: "none", url: "https://example.com/mcp" }; + +const build = (values: Record = {}, ui: Partial = {}, server: MCPServer = SERVER) => + buildEditServerPayload({ ...VALUES, ...values }, { ...UI, ...ui }, server); + +const ok = (result: ReturnType): Record => { + expect(result.kind).toBe("ok"); + if (result.kind !== "ok") throw new Error("unreachable"); + return result.payload; +}; + +describe("buildEditServerPayload rejections", () => { + it("rejects a tool display name with a space before anything else is parsed", () => { + const result = build({ stdio_config: "{" }, { toolNameToDisplayName: { a: "bad name" } }); + expect(result).toStrictEqual({ kind: "invalid_tool_display_name", displayName: "bad name" }); + }); + + it("accepts a display name made of letters, digits, underscores and hyphens", () => { + expect(build({}, { toolNameToDisplayName: { a: "Good_name-1" } }).kind).toBe("ok"); + }); + + it("rejects unparseable stdio config json", () => { + expect(build({ transport: "stdio", stdio_config: "{not json" }).kind).toBe("invalid_stdio_json"); + }); + + it("rejects stdio config json that parses but carries no command", () => { + const result = build({ transport: "stdio", stdio_config: JSON.stringify({ args: ["x"] }) }); + expect(result.kind).toBe("stdio_config_missing_command"); + }); + + it("rejects unparseable stdio env json on the dedicated-fields path", () => { + const result = build({ transport: "stdio", command: "npx", env_json: "{not json" }); + expect(result.kind).toBe("invalid_stdio_env_json"); + }); + + it("rejects a blank command on the dedicated-fields path", () => { + expect(build({ transport: "stdio", command: " " }).kind).toBe("stdio_missing_command"); + }); + + it("rejects unparseable token validation json", () => { + expect(build({ token_validation_json: "{not json" }).kind).toBe("invalid_token_validation_json"); + }); + + it("ignores a whitespace-only token validation body rather than rejecting it", () => { + expect(build({ token_validation_json: " " }).kind).toBe("ok"); + }); + + it("never leaves the stdio branch reachable for a non-stdio transport", () => { + expect(build({ transport: "http", stdio_config: "{not json" }).kind).toBe("ok"); + }); +}); + +describe("buildEditServerPayload stdio parsing", () => { + it("unwraps a pasted client config keyed under mcpServers", () => { + const config = JSON.stringify({ mcpServers: { fs: { command: "npx", args: ["-y", "pkg"], env: { A: "1" } } } }); + const payload = ok(build({ transport: "stdio", stdio_config: config })); + expect(payload.command).toBe("npx"); + expect(payload.args).toStrictEqual(["-y", "pkg"]); + expect(payload.env).toStrictEqual({ A: "1" }); + }); + + it("accepts a bare command object with no mcpServers wrapper", () => { + const config = JSON.stringify({ command: "uvx", args: [], env: {} }); + expect(ok(build({ transport: "stdio", stdio_config: config })).command).toBe("uvx"); + }); + + it("stringifies non-string args and drops blank ones", () => { + const config = JSON.stringify({ command: "npx", args: [1, " ", "keep", true] }); + expect(ok(build({ transport: "stdio", stdio_config: config })).args).toStrictEqual(["1", "keep", "true"]); + }); + + it("stringifies env values and drops blank keys", () => { + const config = JSON.stringify({ command: "npx", env: { A: 1, "": "x", B: null } }); + expect(ok(build({ transport: "stdio", stdio_config: config })).env).toStrictEqual({ A: "1", B: "" }); + }); + + it("trims the command on the dedicated-fields path", () => { + expect(ok(build({ transport: "stdio", command: " npx " })).command).toBe("npx"); + }); + + it("defaults env to an empty object when no env json is supplied", () => { + expect(ok(build({ transport: "stdio", command: "npx" })).env).toStrictEqual({}); + }); + + it("prefers the pasted json over the dedicated fields when both are present", () => { + const config = JSON.stringify({ command: "from-json" }); + expect(ok(build({ transport: "stdio", stdio_config: config, command: "from-field" })).command).toBe("from-json"); + }); +}); + +describe("buildEditServerPayload credentials", () => { + it("drops blank credential values so the backend keeps what it stored", () => { + const payload = ok(build({ auth_type: "api_key", credentials: { auth_value: "", client_id: "cid" } })); + expect(payload.credentials).toStrictEqual({ client_id: "cid" }); + }); + + it("sends an explicit null for a blanked admin-config key so the stored value is cleared", () => { + const payload = ok(build({ auth_type: "oauth2", credentials: { upstream_resource: "", client_id: "cid" } })); + expect(payload.credentials).toStrictEqual({ upstream_resource: null, client_id: "cid" }); + }); + + it("drops a scopes array once every entry is blank", () => { + const payload = ok(build({ auth_type: "oauth2", credentials: { client_id: "cid", scopes: ["", null] } })); + expect(payload.credentials).toStrictEqual({ client_id: "cid" }); + }); + + it("keeps the surviving scopes when only some are blank", () => { + const payload = ok(build({ auth_type: "oauth2", credentials: { client_id: "cid", scopes: ["read", ""] } })); + expect(payload.credentials).toStrictEqual({ client_id: "cid", scopes: ["read"] }); + }); + + it("omits credentials entirely for an auth type that takes none", () => { + const payload = ok(build({ auth_type: "none", credentials: { auth_value: "x" } })); + expect(payload).not.toHaveProperty("credentials"); + }); + + it("writes explicit nulls when a stored app is being removed", () => { + const payload = ok( + build({ auth_type: "true_passthrough", credentials: { client_id: "cid" } }, { removeStoredApp: true }), + ); + expect(payload.credentials).toStrictEqual({ client_id: null, client_secret: null }); + }); + + it("ignores removeStoredApp for a mode that does not forward client tokens", () => { + const payload = ok(build({ auth_type: "api_key", credentials: { auth_value: "v" } }, { removeStoredApp: true })); + expect(payload.credentials).toStrictEqual({ auth_value: "v" }); + }); +}); + +describe("buildEditServerPayload flags and clearing", () => { + it("maps the openapi transport to http for the backend", () => { + expect(ok(build({ transport: "openapi" })).transport).toBe("http"); + }); + + it("leaves a transport the backend already understands alone", () => { + expect(ok(build({ transport: "http" })).transport).toBe("http"); + }); + + it("nulls the oauth2 endpoints when moving off oauth2", () => { + const payload = ok(build({ auth_type: "api_key" }, {}, { ...SERVER, auth_type: "oauth2" })); + expect(payload.issuer).toBeNull(); + expect(payload.authorization_url).toBeNull(); + expect(payload.token_url).toBeNull(); + expect(payload.registration_url).toBeNull(); + }); + + it("leaves the oauth2 endpoints untouched when staying on oauth2", () => { + const payload = ok(build({ auth_type: "oauth2" }, {}, { ...SERVER, auth_type: "oauth2" })); + expect(payload).not.toHaveProperty("issuer"); + }); + + it("nulls the token exchange fields when moving off token exchange", () => { + const payload = ok(build({ auth_type: "none" }, {}, { ...SERVER, auth_type: "oauth2_token_exchange" })); + expect(payload.token_exchange_endpoint).toBeNull(); + expect(payload.audience).toBeNull(); + expect(payload.subject_token_type).toBeNull(); + expect(payload.token_exchange_profile).toBeNull(); + }); + + it("forces delegate_auth_to_upstream false for any auth type other than oauth2", () => { + const payload = ok(build({ auth_type: "api_key", delegate_auth_to_upstream: true })); + expect(payload.delegate_auth_to_upstream).toBe(false); + }); + + it("honours delegate_auth_to_upstream for oauth2", () => { + const payload = ok(build({ auth_type: "oauth2", delegate_auth_to_upstream: true })); + expect(payload.delegate_auth_to_upstream).toBe(true); + }); + + it("forces oauth_passthrough false unless an Authorization header is forwarded", () => { + const payload = ok(build({ auth_type: "none", oauth_passthrough: true, extra_headers: [] })); + expect(payload.oauth_passthrough).toBe(false); + }); + + it("honours oauth_passthrough for a none-auth server forwarding Authorization", () => { + const payload = ok(build({ auth_type: "none", oauth_passthrough: true, extra_headers: ["authorization"] })); + expect(payload.oauth_passthrough).toBe(true); + }); + + it("forces dcr_bridge false outside the client-forwarded modes", () => { + expect(ok(build({ auth_type: "oauth2", dcr_bridge: true })).dcr_bridge).toBe(false); + }); + + it("honours dcr_bridge for a client-forwarded mode", () => { + expect(ok(build({ auth_type: "true_passthrough", dcr_bridge: true })).dcr_bridge).toBe(true); + }); + + it("sends token_validation as null to clear a value the server already had", () => { + const payload = ok(build({ token_validation_json: "" }, {}, { ...SERVER, token_validation: { a: 1 } })); + expect(payload.token_validation).toBeNull(); + }); + + it("omits token_validation entirely when neither side has one", () => { + expect(ok(build({ token_validation_json: "" }))).not.toHaveProperty("token_validation"); + }); + + it("falls back through the name chain to the server url", () => { + const server = { ...SERVER, server_name: "", url: "https://fallback" }; + const payload = ok(build({ server_name: "", url: "" }, {}, server)); + expect((payload.mcp_info as Record).server_name).toBe("https://fallback"); + }); + + it("falls back to the literal unknown when every name candidate is blank", () => { + const server = { ...SERVER, server_name: "", url: "", alias: "" }; + const payload = ok(build({ server_name: "", url: "", alias: "" }, {}, server)); + expect((payload.mcp_info as Record).server_name).toBe("unknown"); + }); + + it("emits allowed_tools only once the allowlist is enforced", () => { + expect(ok(build())).not.toHaveProperty("allowed_tools"); + expect(ok(build({}, { allowedTools: ["a"] })).allowed_tools).toStrictEqual(["a"]); + expect(ok(build({}, { hasExistingToolAllowlist: true })).allowed_tools).toStrictEqual([]); + }); + + it("normalises object-shaped access groups to their names", () => { + const payload = ok(build({ mcp_access_groups: [{ name: "eng" }, "ops"] })); + expect(payload.mcp_access_groups).toStrictEqual(["eng", "ops"]); + }); +}); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.ts b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.ts new file mode 100644 index 00000000000..28f88cdf0f5 --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.ts @@ -0,0 +1,285 @@ +import { + ADMIN_CONFIG_CREDENTIAL_KEYS, + AUTH_TYPE, + MCPServer, + MCPServerCostInfo, + MCP_OAUTH2_FLOW_INTERACTIVE, + MCP_OAUTH2_FLOW_M2M, + OAUTH_FLOW, + TRANSPORT, + isClientForwardedTokenMode, + preservedAdminCredentials, +} from "@/components/mcp_tools/types"; +import { TOOL_DISPLAY_NAME_PATTERN, normalizeEnvVars } from "./utils"; +import { AUTH_TYPES_REQUIRING_CREDENTIALS, reduceStaticHeaders } from "./createServerPayload"; + +export interface EditServerUiState { + readonly costConfig: MCPServerCostInfo; + readonly allowedTools: readonly string[]; + readonly hasExistingToolAllowlist: boolean; + readonly hasToolAllowlistInteraction: boolean; + readonly toolNameToDisplayName: Readonly>; + readonly toolNameToDescription: Readonly>; + readonly logoUrl: string | undefined; + readonly removeStoredApp: boolean; +} + +export type BuildEditPayloadResult = + | { readonly kind: "ok"; readonly payload: Record } + | { readonly kind: "invalid_tool_display_name"; readonly displayName: string } + | { readonly kind: "invalid_stdio_json" } + | { readonly kind: "stdio_config_missing_command" } + | { readonly kind: "invalid_stdio_env_json" } + | { readonly kind: "stdio_missing_command" } + | { readonly kind: "invalid_token_validation_json" }; + +type StdioFieldsResult = + | { readonly kind: "ok"; readonly fields: Record } + | { readonly kind: "invalid_stdio_json" } + | { readonly kind: "stdio_config_missing_command" } + | { readonly kind: "invalid_stdio_env_json" } + | { readonly kind: "stdio_missing_command" }; + +type JsonParseResult = { readonly kind: "ok"; readonly value: unknown } | { readonly kind: "invalid" }; + +const tryParseJson = (raw: string): JsonParseResult => { + try { + return { kind: "ok", value: JSON.parse(raw) }; + } catch { + return { kind: "invalid" }; + } +}; + +const normalizeStdioArgs = (args: unknown): string[] => + Array.isArray(args) ? args.map((v: unknown) => String(v)).filter((v: string) => v.trim() !== "") : []; + +const normalizeStdioEnv = (env: unknown): Record => + env && typeof env === "object" && !Array.isArray(env) + ? Object.entries(env as Record).reduce((acc: Record, [k, v]) => { + if (k == null || String(k).trim() === "") return acc; + acc[String(k)] = v == null ? "" : String(v); + return acc; + }, {}) + : {}; + +const unwrapStdioConfig = (parsed: unknown): Record => { + const config = parsed as Record | null; + const nested = config?.mcpServers; + if (!nested || typeof nested !== "object") return (config ?? {}) as Record; + const names = Object.keys(nested as Record); + if (names.length === 0) return config as Record; + return (nested as Record)[names[0]] as Record; +}; + +const buildStdioFields = ( + rawStdioConfig: unknown, + rawEnvJson: unknown, + rawCommand: unknown, + rawArgs: unknown, +): StdioFieldsResult => { + if (rawStdioConfig) { + const parsed = tryParseJson(rawStdioConfig as string); + if (parsed.kind === "invalid") return { kind: "invalid_stdio_json" }; + + const config = unwrapStdioConfig(parsed.value); + const command = config?.command ? String(config.command) : undefined; + if (!command) return { kind: "stdio_config_missing_command" }; + + return { + kind: "ok", + fields: { command, args: normalizeStdioArgs(config?.args), env: normalizeStdioEnv(config?.env) }, + }; + } + + const envResult: JsonParseResult = rawEnvJson ? tryParseJson(rawEnvJson as string) : { kind: "ok", value: null }; + if (envResult.kind === "invalid") return { kind: "invalid_stdio_env_json" }; + + const command = rawCommand ? String(rawCommand).trim() : ""; + if (!command) return { kind: "stdio_missing_command" }; + + return { + kind: "ok", + fields: { command, args: normalizeStdioArgs(rawArgs), env: normalizeStdioEnv(envResult.value) }, + }; +}; + +const filterCredentials = (credentialValues: unknown): Record | undefined => { + if (!credentialValues || typeof credentialValues !== "object") return undefined; + return Object.entries(credentialValues as Record).reduce( + (acc: Record, [key, value]) => { + if (value === undefined || value === null || value === "") { + // Blank is the keep-existing convention, except for the admin-config keys, where an + // explicit null is how the backend is told to clear a previously stored value. + if (value === "" && (ADMIN_CONFIG_CREDENTIAL_KEYS as readonly string[]).includes(key)) { + acc[key] = null; + } + return acc; + } + if (key === "scopes") { + if (Array.isArray(value)) { + const filteredScopes = value.filter((scope) => scope != null && scope !== ""); + if (filteredScopes.length > 0) acc[key] = filteredScopes; + } + } else { + acc[key] = value; + } + return acc; + }, + {}, + ); +}; + +const firstInvalidToolDisplayName = (toolNameToDisplayName: Readonly>): string | undefined => + Object.entries(toolNameToDisplayName).find( + ([, displayName]) => displayName && !TOOL_DISPLAY_NAME_PATTERN.test(displayName), + )?.[1]; + +export const buildEditServerPayload = ( + values: Record, + ui: EditServerUiState, + mcpServer: MCPServer, +): BuildEditPayloadResult => { + const badDisplayName = firstInvalidToolDisplayName(ui.toolNameToDisplayName); + if (badDisplayName !== undefined) { + return { kind: "invalid_tool_display_name", displayName: badDisplayName }; + } + + const { + static_headers: staticHeadersList, + env_vars: envVarsList, + credentials: credentialValues, + stdio_config: rawStdioConfig, + env_json: rawEnvJson, + command: rawCommand, + args: rawArgs, + allow_all_keys: allowAllKeysRaw, + available_on_public_internet: availableOnPublicInternetRaw, + delegate_auth_to_upstream: delegateAuthToUpstreamRaw, + oauth_passthrough: oauthPassthroughRaw, + dcr_bridge: dcrBridgeRaw, + token_validation_json: rawTokenValidationJson, + ...restValues + } = values; + + const stdio: StdioFieldsResult = + restValues.transport === "stdio" + ? buildStdioFields(rawStdioConfig, rawEnvJson, rawCommand, rawArgs) + : { kind: "ok", fields: {} }; + if (stdio.kind !== "ok") return stdio; + + const rawTokenValidation = rawTokenValidationJson as string | undefined; + const tokenValidationResult: JsonParseResult = + rawTokenValidation && rawTokenValidation.trim() !== "" + ? tryParseJson(rawTokenValidation) + : { kind: "ok", value: null }; + if (tokenValidationResult.kind === "invalid") return { kind: "invalid_token_validation_json" }; + const tokenValidation = tokenValidationResult.value as Record | null; + + // "openapi" is a UI-only transport; the backend stores those servers as plain http. + const transport = restValues.transport === TRANSPORT.OPENAPI ? "http" : restValues.transport; + const authType = restValues.auth_type as string | undefined; + + const accessGroups = ((restValues.mcp_access_groups as unknown[] | undefined) || []).map((g: unknown) => + typeof g === "string" ? g : (g as { name?: string })?.name || String(g), + ); + + const mcpInfoServerName = + [ + restValues.server_name as string | undefined, + restValues.url as string | undefined, + mcpServer.server_name, + mcpServer.url, + restValues.alias as string | undefined, + mcpServer.alias, + ].find(Boolean) ?? "unknown"; + + const toolAllowlistEnforced = + ui.hasExistingToolAllowlist || ui.hasToolAllowlistInteraction || ui.allowedTools.length > 0; + + const extraHeaders = Array.isArray(restValues.extra_headers) ? restValues.extra_headers : []; + const hasAuthorizationHeader = extraHeaders.some( + (h: unknown) => typeof h === "string" && h.toLowerCase() === "authorization", + ); + const isNoneAuth = authType === AUTH_TYPE.NONE || authType == null; + + const credentialsPayload = filterCredentials(credentialValues); + const includeCredentials = authType !== undefined && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(authType); + // 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(authType) + ? preservedAdminCredentials(credentialsPayload) + : credentialsPayload; + const persistedCredentials = + includeCredentials && submitCredentials && Object.keys(submitCredentials).length > 0 + ? submitCredentials + : undefined; + // Explicit removal of a saved app wins over the filter above. 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. + const credentials = + ui.removeStoredApp && isClientForwardedTokenMode(authType) + ? { client_id: null, client_secret: null } + : persistedCredentials; + + return { + kind: "ok", + payload: { + ...restValues, + ...(transport === restValues.transport ? {} : { transport }), + ...stdio.fields, + stdio_config: undefined, + env_json: undefined, + ...(mcpServer.auth_type === AUTH_TYPE.OAUTH2 && authType !== AUTH_TYPE.OAUTH2 + ? { issuer: null, authorization_url: null, token_url: null, registration_url: null } + : {}), + ...(mcpServer.auth_type === AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE && authType !== AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE + ? { token_exchange_endpoint: null, audience: null, subject_token_type: null, token_exchange_profile: null } + : {}), + server_id: mcpServer.server_id, + mcp_info: { + ...(mcpServer.mcp_info ?? {}), + server_name: mcpInfoServerName, + description: restValues.description, + logo_url: ui.logoUrl || undefined, + mcp_server_cost_info: Object.keys(ui.costConfig).length > 0 ? ui.costConfig : null, + tool_allowlist_enforced: toolAllowlistEnforced, + }, + mcp_access_groups: accessGroups, + alias: restValues.alias, + extra_headers: restValues.extra_headers || [], + ...(toolAllowlistEnforced ? { allowed_tools: [...ui.allowedTools] } : {}), + tool_name_to_display_name: Object.keys(ui.toolNameToDisplayName).length > 0 ? ui.toolNameToDisplayName : null, + tool_name_to_description: Object.keys(ui.toolNameToDescription).length > 0 ? ui.toolNameToDescription : null, + disallowed_tools: restValues.disallowed_tools || [], + static_headers: reduceStaticHeaders(staticHeadersList), + env_vars: normalizeEnvVars(envVarsList), + allow_all_keys: Boolean(allowAllKeysRaw ?? mcpServer.allow_all_keys), + available_on_public_internet: Boolean(availableOnPublicInternetRaw ?? mcpServer.available_on_public_internet), + // ``delegate_auth_to_upstream`` is only honored server-side for ``auth_type=oauth2`` (PKCE + // passthrough). The field is conditionally rendered so the value drops out of the form on + // auth_type change; force false for any other configuration to avoid persisting a stale + // ``true`` that would silently re-activate if the configuration is later switched back. + delegate_auth_to_upstream: + authType === AUTH_TYPE.OAUTH2 + ? Boolean(delegateAuthToUpstreamRaw ?? mcpServer.delegate_auth_to_upstream) + : false, + // ``oauth_passthrough`` is the dedicated, non-oauth2 opt-in, only honored for + // ``auth_type=none`` servers that forward ``Authorization`` upstream. Kept separate from + // ``delegate_auth_to_upstream`` so enabling pass-through never regresses oauth2 servers. + oauth_passthrough: + isNoneAuth && hasAuthorizationHeader ? Boolean(oauthPassthroughRaw ?? mcpServer.oauth_passthrough) : false, + // ``dcr_bridge`` is only meaningful for the client-forwarded token modes. Same stale-value + // reasoning as the two flags above. + dcr_bridge: isClientForwardedTokenMode(authType) ? Boolean(dcrBridgeRaw ?? mcpServer.dcr_bridge) : false, + ...(authType === AUTH_TYPE.OAUTH2 && restValues.oauth_flow_type + ? { + oauth2_flow: + restValues.oauth_flow_type === OAUTH_FLOW.M2M ? MCP_OAUTH2_FLOW_M2M : MCP_OAUTH2_FLOW_INTERACTIVE, + } + : {}), + ...(tokenValidation !== null || mcpServer.token_validation ? { token_validation: tokenValidation } : {}), + ...(credentials === undefined ? {} : { credentials }), + }, + }; +}; diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx index 5c3fe6d8cda..b492495312e 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/mcp_server_edit.tsx @@ -11,7 +11,6 @@ import { isHeldOAuthTokenStale, preservedAdminCredentials, preservedDeclaredAppCredentials, - ADMIN_CONFIG_CREDENTIAL_KEYS, withoutMintedTokenCredentials, OAUTH_FLOW, MCP_OAUTH2_FLOW_M2M, @@ -41,13 +40,8 @@ import IdJagFormFields from "./IdJagFormFields"; import OAuthFormFields from "./OAuthFormFields"; import MCPLogoSelector from "./MCPLogoSelector"; import EnvVarsSection from "./EnvVarsSection"; -import { - validateMCPServerUrl, - validateMCPServerName, - normalizeEnvVars, - normalizeToolOverrideMap, - TOOL_DISPLAY_NAME_PATTERN, -} from "./utils"; +import { validateMCPServerUrl, validateMCPServerName, normalizeToolOverrideMap } from "./utils"; +import { BuildEditPayloadResult, buildEditServerPayload } from "./editServerPayload"; import { toast } from "@/lib/toast"; import { useMcpOAuthFlow } from "@/hooks/useMcpOAuthFlow"; import { getSecureItem, setSecureItem } from "@/utils/secureStorage"; @@ -71,6 +65,23 @@ const AUTH_TYPES_REQUIRING_CREDENTIALS = [ AUTH_TYPE.TRUE_PASSTHROUGH, AUTH_TYPE.OAUTH_DELEGATE, ]; +const editPayloadErrorMessage = (result: Exclude): string => { + switch (result.kind) { + case "invalid_tool_display_name": + return `Tool display name "${result.displayName}" is invalid. Only letters, digits, underscores, and hyphens are allowed (no spaces).`; + case "invalid_stdio_json": + return "Invalid JSON in stdio configuration"; + case "stdio_config_missing_command": + return "Stdio configuration must include a command"; + case "invalid_stdio_env_json": + return "Invalid JSON in stdio env configuration"; + case "stdio_missing_command": + return "Stdio transport requires a command"; + case "invalid_token_validation_json": + return "Invalid JSON in Token Validation Rules"; + } +}; + export const EDIT_OAUTH_UI_STATE_KEY = "litellm-mcp-oauth-edit-state"; const MCPServerEdit: React.FC = ({ @@ -672,283 +683,31 @@ const MCPServerEdit: React.FC = ({ const handleSave = async (values: Record) => { if (!accessToken) return; - const invalidDisplayName = Object.entries(toolNameToDisplayName).find( - ([, displayName]) => displayName && !TOOL_DISPLAY_NAME_PATTERN.test(displayName), + + const result = buildEditServerPayload( + values, + { + costConfig, + allowedTools, + hasExistingToolAllowlist, + hasToolAllowlistInteraction, + toolNameToDisplayName, + toolNameToDescription, + logoUrl, + removeStoredApp, + }, + mcpServer, ); - if (invalidDisplayName) { - toast.fromError( - `Tool display name "${invalidDisplayName[1]}" is invalid. Only letters, digits, underscores, and hyphens are allowed (no spaces).`, - ); + if (result.kind !== "ok") { + toast.fromError(editPayloadErrorMessage(result)); return; } + + const payload = result.payload; + const authType = values.auth_type; + const delegateAuthToUpstreamRaw = values.delegate_auth_to_upstream; + try { - // Ensure access groups is always a string array - const { - static_headers: staticHeadersList, - env_vars: envVarsList, - credentials: credentialValues, - stdio_config: rawStdioConfig, - env_json: rawEnvJson, - command: rawCommand, - args: rawArgs, - allow_all_keys: allowAllKeysRaw, - available_on_public_internet: availableOnPublicInternetRaw, - delegate_auth_to_upstream: delegateAuthToUpstreamRaw, - oauth_passthrough: oauthPassthroughRaw, - dcr_bridge: dcrBridgeRaw, - token_validation_json: rawTokenValidationJson, - ...restValues - } = values; - - const accessGroups = (restValues.mcp_access_groups || []).map((g: any) => - typeof g === "string" ? g : g.name || String(g), - ); - - const staticHeaders = Array.isArray(staticHeadersList) - ? staticHeadersList.reduce((acc: Record, entry: Record) => { - const header = entry?.header?.trim(); - if (!header) { - return acc; - } - acc[header] = (entry?.value ?? "").trim(); - return acc; - }, {}) - : ({} as Record); - - const envVars = normalizeEnvVars(envVarsList); - - const credentialsPayload = - credentialValues && typeof credentialValues === "object" - ? Object.entries(credentialValues).reduce((acc: Record, [key, value]) => { - if (value === undefined || value === null || value === "") { - if (value === "" && (ADMIN_CONFIG_CREDENTIAL_KEYS as readonly string[]).includes(key)) { - acc[key] = null; - } - return acc; - } - if (key === "scopes") { - if (Array.isArray(value)) { - const filteredScopes = value.filter((scope) => scope != null && scope !== ""); - if (filteredScopes.length > 0) { - acc[key] = filteredScopes; - } - } - } else { - acc[key] = value; - } - return acc; - }, {}) - : undefined; - - let stdioFields: Record = {}; - - if (restValues.transport === "stdio") { - // Prefer JSON config if provided (matches Create screen behavior) - if (rawStdioConfig) { - try { - const stdioConfig = JSON.parse(rawStdioConfig); - - let actualConfig = stdioConfig; - if (stdioConfig?.mcpServers && typeof stdioConfig.mcpServers === "object") { - const serverNames = Object.keys(stdioConfig.mcpServers); - if (serverNames.length > 0) { - actualConfig = stdioConfig.mcpServers[serverNames[0]]; - } - } - - const parsedArgs = Array.isArray(actualConfig?.args) - ? actualConfig.args.map((v: any) => String(v)).filter((v: string) => v.trim() !== "") - : []; - - const parsedEnv = - actualConfig?.env && typeof actualConfig.env === "object" && !Array.isArray(actualConfig.env) - ? Object.entries(actualConfig.env).reduce((acc: Record, [k, v]) => { - if (k == null || String(k).trim() === "") return acc; - acc[String(k)] = v == null ? "" : String(v); - return acc; - }, {}) - : {}; - - stdioFields = { - command: actualConfig?.command ? String(actualConfig.command) : undefined, - args: parsedArgs, - env: parsedEnv, - }; - - if (!stdioFields.command) { - toast.fromError("Stdio configuration must include a command"); - return; - } - } catch { - toast.fromError("Invalid JSON in stdio configuration"); - return; - } - } else { - // Dedicated fields path (command/args + env JSON) - let parsedEnv: Record = {}; - if (rawEnvJson) { - try { - const env = JSON.parse(rawEnvJson); - if (env && typeof env === "object" && !Array.isArray(env)) { - parsedEnv = Object.entries(env).reduce((acc: Record, [k, v]) => { - if (k == null || String(k).trim() === "") return acc; - acc[String(k)] = v == null ? "" : String(v); - return acc; - }, {}); - } - } catch { - toast.fromError("Invalid JSON in stdio env configuration"); - return; - } - } - const parsedArgs = Array.isArray(rawArgs) - ? rawArgs.map((v: any) => String(v)).filter((v: string) => v.trim() !== "") - : []; - - const parsedCommand = rawCommand ? String(rawCommand).trim() : ""; - if (!parsedCommand) { - toast.fromError("Stdio transport requires a command"); - return; - } - - stdioFields = { - command: parsedCommand, - args: parsedArgs, - env: parsedEnv, - }; - } - } - - // Map "openapi" transport to "http" for the backend - if (restValues.transport === TRANSPORT.OPENAPI) { - restValues.transport = "http"; - } - - // Parse token_validation JSON if provided - let tokenValidation: Record | null = null; - if (rawTokenValidationJson && rawTokenValidationJson.trim() !== "") { - try { - tokenValidation = JSON.parse(rawTokenValidationJson); - } catch { - toast.fromError("Invalid JSON in Token Validation Rules"); - return; - } - } - - // Prepare the payload with cost configuration and permission fields - const mcpInfoServerName = - restValues.server_name || - restValues.url || - mcpServer.server_name || - mcpServer.url || - restValues.alias || - mcpServer.alias || - "unknown"; - - const toolAllowlistEnforced = hasExistingToolAllowlist || hasToolAllowlistInteraction || allowedTools.length > 0; - - const payload: Record = { - ...restValues, - ...stdioFields, - // Remove UI-only fields - stdio_config: undefined, - env_json: undefined, - ...(mcpServer.auth_type === AUTH_TYPE.OAUTH2 && restValues.auth_type !== AUTH_TYPE.OAUTH2 - ? { issuer: null, authorization_url: null, token_url: null, registration_url: null } - : {}), - ...(mcpServer.auth_type === AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE && - restValues.auth_type !== AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE - ? { token_exchange_endpoint: null, audience: null, subject_token_type: null, token_exchange_profile: null } - : {}), - server_id: mcpServer.server_id, - mcp_info: { - ...(mcpServer.mcp_info ?? {}), - server_name: mcpInfoServerName, - description: restValues.description, - logo_url: logoUrl || undefined, - mcp_server_cost_info: Object.keys(costConfig).length > 0 ? costConfig : null, - tool_allowlist_enforced: toolAllowlistEnforced, - }, - mcp_access_groups: accessGroups, - alias: restValues.alias, - // Include permission management fields - extra_headers: restValues.extra_headers || [], - ...(toolAllowlistEnforced - ? { - allowed_tools: allowedTools, - } - : {}), - tool_name_to_display_name: Object.keys(toolNameToDisplayName).length > 0 ? toolNameToDisplayName : null, - tool_name_to_description: Object.keys(toolNameToDescription).length > 0 ? toolNameToDescription : null, - disallowed_tools: restValues.disallowed_tools || [], - static_headers: staticHeaders, - env_vars: envVars, - allow_all_keys: Boolean(allowAllKeysRaw ?? mcpServer.allow_all_keys), - available_on_public_internet: Boolean(availableOnPublicInternetRaw ?? mcpServer.available_on_public_internet), - // ``delegate_auth_to_upstream`` is only honored server-side for - // ``auth_type=oauth2`` (PKCE passthrough). The Form.Item is - // conditionally rendered so the value drops out of the form on - // auth_type change; force false for any other configuration to avoid - // persisting a stale ``true`` that would silently re-activate if the - // configuration is later switched back. - delegate_auth_to_upstream: (() => { - const isOauth2 = restValues.auth_type === AUTH_TYPE.OAUTH2; - return isOauth2 ? Boolean(delegateAuthToUpstreamRaw ?? mcpServer.delegate_auth_to_upstream) : false; - })(), - // ``oauth_passthrough`` is the dedicated, non-oauth2 opt-in. It is only - // honored for ``auth_type=none`` servers that forward ``Authorization`` - // upstream. Kept separate from ``delegate_auth_to_upstream`` so enabling - // pass-through never regresses oauth2 servers. Force false otherwise. - oauth_passthrough: (() => { - const isNoneAuth = restValues.auth_type === AUTH_TYPE.NONE || restValues.auth_type == null; - const extraHeaders = Array.isArray(restValues.extra_headers) ? restValues.extra_headers : []; - const hasAuthorizationHeader = extraHeaders.some( - (h: unknown) => typeof h === "string" && h.toLowerCase() === "authorization", - ); - return isNoneAuth && hasAuthorizationHeader - ? Boolean(oauthPassthroughRaw ?? mcpServer.oauth_passthrough) - : false; - })(), - // ``dcr_bridge`` is only meaningful for the client-forwarded token - // modes (true_passthrough / oauth_delegate). The Form.Item is - // conditionally rendered so the value drops out of the form on - // auth_type change; force false for any other configuration to avoid - // persisting a stale ``true`` that would silently re-activate if the - // mode is later switched back. - dcr_bridge: isClientForwardedTokenMode(restValues.auth_type) - ? Boolean(dcrBridgeRaw ?? mcpServer.dcr_bridge) - : false, - ...(restValues.auth_type === AUTH_TYPE.OAUTH2 && restValues.oauth_flow_type - ? { - oauth2_flow: - restValues.oauth_flow_type === OAUTH_FLOW.M2M ? MCP_OAUTH2_FLOW_M2M : MCP_OAUTH2_FLOW_INTERACTIVE, - } - : {}), - // Include token_validation when it is set (non-null) or when clearing an existing value - ...(tokenValidation !== null || mcpServer.token_validation ? { token_validation: tokenValidation } : {}), - }; - - const includeCredentials = - restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type); - - // 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) - ? preservedAdminCredentials(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); // Persist the token staged via "Authorize & Fetch" (mirrors the create flow's @@ -957,7 +716,7 @@ const MCPServerEdit: React.FC = ({ // never in the server row. M2M/static auth resolve server-side and need neither. if (oauthTokenResponse?.access_token) { const oauthMode = getMcpOAuthMode({ - auth_type: restValues.auth_type, + auth_type: authType, oauth2_flow: isM2MFlow ? MCP_OAUTH2_FLOW_M2M : null, delegate_auth_to_upstream: Boolean(delegateAuthToUpstreamRaw ?? mcpServer.delegate_auth_to_upstream), }); @@ -971,7 +730,7 @@ const MCPServerEdit: React.FC = ({ scopes: typeof scope === "string" && scope ? scope.split(" ") : undefined, }; await storeMCPOAuthUserCredential(accessToken, mcpServer.server_id, oauthCredentialPayload); - } else if (oauthMode === "passthrough" || isClientForwardedTokenMode(restValues.auth_type)) { + } else if (oauthMode === "passthrough" || isClientForwardedTokenMode(authType)) { const browserHeldToken = { access_token: oauthTokenResponse.access_token, expires_in: oauthTokenResponse.expires_in,