From 8bb85250474702975401bacc70cd80258b2269c5 Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Tue, 18 Aug 2026 22:56:09 -0700 Subject: [PATCH] refactor(ui): extract the MCP server edit save payload into a pure builder (#37436) * refactor(ui): extract the MCP server edit save payload into a pure builder `handleSave` built the update payload inline across 276 lines, spreading `...restValues` straight off a mounted-only `onFinish`. That makes the payload a function of which fields happen to be mounted, and it leaves no seam to test the shape without rendering the whole edit form. Move the payload construction into `editServerPayload.ts` as `buildEditServerPayload(values, ui)`, a pure function over the submitted values plus the nine pieces of component state the handler reads. Failures become values rather than early returns with a toast: the six error branches are a tagged union that `editPayloadErrorMessage` maps back to the exact strings shown today, via an exhaustive switch. `handleSave` keeps the network call, the OAuth token persistence and its own try/catch. This is a move, not a rewrite. To prove that, `editServerPayload.differential.test.ts` holds a baseline machine-extracted from the pre-refactor function body by line range, with the failure branches converted by exact string replacement. The generator refuses to emit unless the slice is still present verbatim in the source, every conversion matches exactly once, no toast call survives, and a deliberately corrupted probe still trips that check. 59 scenarios run both implementations and compare the payload object, its key order, and its serialised bytes, so a re-ordering that leaves values untouched is caught too. The duplicate local `AUTH_TYPES_REQUIRING_CREDENTIALS` is dropped in favour of the identical exported list, and `reduceStaticHeaders` is shared with the create side. Both were verified equal before reuse. Behaviour is unchanged. The 466 pre-existing tests in the directory pass unedited. * refactor(ui): type the MCP edit payload builder instead of Record The extraction created a new public signature, so it should carry a real contract. buildEditServerPayload now takes EditServerFormValues and returns EditServerPayload, both declaring every field the builder actually reads and writes, with an unknown-valued index signature for the keys the form passes straight through. handleSave is annotated too, so antd's untyped onFinish value is narrowed once at the boundary rather than travelling as any. Fields that arrive from the store with their own runtime validation (static_headers, env_vars, credentials) stay unknown rather than being given a narrower declared type the form does not actually guarantee. Values are not run through a parser: the payload's serialised key order is part of the contract this module exists to hold, and rebuilding the object would reorder it. The credentials assignment moves from two post-hoc mutations to a single resolved entry, which keeps the payload readonly end to end and lands the key in the same position in all four branches. Behaviour is unchanged. The 59 differential scenarios still match the frozen pre-extraction body on object, Object.keys order and JSON.stringify bytes, and the mcp-servers suite is 525/525 across all 30 files. Three tsc probes confirm the new types have teeth: a wrong payload assignment, a misspelled field read and an invalid value each fail the type check. --- .../editServerPayload.differential.cases.ts | 391 +++++++++++++++++ .../editServerPayload.differential.test.ts | 331 ++++++++++++++ .../_components/editServerPayload.ts | 405 ++++++++++++++++++ .../_components/mcp_server_edit.tsx | 316 +------------- 4 files changed, 1148 insertions(+), 295 deletions(-) create mode 100644 ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.cases.ts create mode 100644 ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.test.ts create mode 100644 ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.ts diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.cases.ts b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.cases.ts new file mode 100644 index 00000000000..c350ac085b6 --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.cases.ts @@ -0,0 +1,391 @@ +import { MCPServer } from "@/components/mcp_tools/types"; +import type { EditServerUiState } from "./editServerPayload"; + +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: [], +}; + +export const baseUi: EditServerUiState = { + mcpServer: SERVER, + logoUrl: undefined, + costConfig: {}, + allowedTools: [], + hasExistingToolAllowlist: false, + hasToolAllowlistInteraction: false, + toolNameToDisplayName: {}, + toolNameToDescription: {}, + removeStoredApp: false, +}; + +export interface DifferentialCase { + readonly label: string; + readonly values: Record; + readonly ui: Partial; +} + +// Always-mounted root fields. A "collapsed" section is modelled as its keys being +// ABSENT, which is what antd's mounted-only onFinish produces, and an expanded but +// untouched one as the key present holding `undefined`. +const ROOT = { + server_name: "srv", + alias: "srv_alias", + description: "a server", + transport: "http", + url: "https://example.com/mcp", + auth_type: "none", + max_concurrent_requests: undefined, + mcp_access_groups: [], + extra_headers: [], + static_headers: [], + env_vars: [], + allow_all_keys: false, + available_on_public_internet: true, +}; + +const CREDS = { + auth_value: "secret-value", + client_id: "cid", + client_secret: "csec", + scopes: ["read", "write"], + aws_region_name: "us-east-1", + aws_service_name: "bedrock", + aws_access_key_id: "AKIA", + aws_secret_access_key: "shh", + aws_session_token: "tok", + aws_role_name: "role", + aws_session_name: "sess", + id_jag_resource: "api://res", + id_jag_resource_token_endpoint: "https://idp/jag", + client_private_key: "-----KEY-----", + client_private_key_id: "kid", + client_assertion_signing_alg: "RS256", + token_endpoint_auth_method: "client_secret_basic", + upstream_resource: "api://up", +}; + +const AUTH_TYPES = [ + "none", + "api_key", + "bearer_token", + "token", + "basic", + "oauth2", + "oauth2_token_exchange", + "oauth2_id_jag", + "aws_sigv4", + "true_passthrough", + "oauth_delegate", +] as const; + +export const CASES: readonly DifferentialCase[] = [ + // --- mount-state axis: everything collapsed / expanded-undefined / expanded-valued --- + { label: "everything collapsed, only the always-mounted root", values: { ...ROOT }, ui: {} }, + { + label: "everything expanded, all optional keys present as undefined", + values: { + ...ROOT, + credentials: {}, + stdio_config: undefined, + env_json: undefined, + command: undefined, + args: undefined, + delegate_auth_to_upstream: undefined, + oauth_passthrough: undefined, + dcr_bridge: undefined, + token_validation_json: undefined, + spec_path: undefined, + oauth_flow_type: undefined, + issuer: undefined, + authorization_url: undefined, + token_url: undefined, + registration_url: undefined, + token_storage_ttl_seconds: undefined, + token_exchange_endpoint: undefined, + token_exchange_profile: undefined, + audience: undefined, + subject_token_type: undefined, + disallowed_tools: undefined, + }, + ui: {}, + }, + { + label: "expand then collapse: the key is absent again even though a value was typed", + values: { ...ROOT }, + ui: {}, + }, + + // --- transport axis --- + { + label: "stdio via the JSON config path", + values: { + ...ROOT, + transport: "stdio", + url: undefined, + stdio_config: JSON.stringify({ command: "npx", args: ["-y", "pkg"], env: { A: "1" } }), + }, + ui: {}, + }, + { + label: "stdio via a wrapped mcpServers config", + values: { + ...ROOT, + transport: "stdio", + url: undefined, + stdio_config: JSON.stringify({ mcpServers: { first: { command: "uvx", args: [1, " ", "b"] } } }), + }, + ui: {}, + }, + { + label: "stdio via the dedicated command/args/env fields", + values: { + ...ROOT, + transport: "stdio", + url: undefined, + command: " npx ", + args: ["-y", " ", "pkg"], + env_json: JSON.stringify({ A: 1, "": "skipped", B: null }), + }, + ui: {}, + }, + { + label: "openapi swaps url for spec_path", + values: { ...ROOT, transport: "openapi", url: undefined, spec_path: "https://x/openapi.json" }, + ui: {}, + }, + + // --- auth-type axis, one case per type, credentials mounted --- + ...AUTH_TYPES.map((auth) => ({ + label: `auth_type ${auth} with the full credential bag mounted`, + values: { ...ROOT, auth_type: auth, credentials: { ...CREDS } }, + ui: {}, + })), + + // --- oauth2 flow sub-branches --- + { + label: "oauth2 m2m flow", + values: { + ...ROOT, + auth_type: "oauth2", + oauth_flow_type: "m2m", + token_url: "https://idp/token", + credentials: { ...CREDS }, + }, + ui: {}, + }, + { + label: "oauth2 interactive flow", + values: { + ...ROOT, + auth_type: "oauth2", + oauth_flow_type: "interactive", + issuer: "https://idp", + credentials: { ...CREDS }, + }, + ui: {}, + }, + { + label: "oauth2 with delegate_auth_to_upstream on", + values: { ...ROOT, auth_type: "oauth2", delegate_auth_to_upstream: true, credentials: { ...CREDS } }, + ui: {}, + }, + + // --- the five coalesced keys: unbound raw against each server-side fallback --- + { + label: "coalesced booleans unbound, server has them all true", + values: { ...ROOT, auth_type: "oauth2", allow_all_keys: undefined, available_on_public_internet: undefined }, + ui: { + mcpServer: { + ...SERVER, + auth_type: "oauth2", + allow_all_keys: true, + available_on_public_internet: true, + delegate_auth_to_upstream: true, + oauth_passthrough: true, + dcr_bridge: true, + }, + }, + }, + { + label: "coalesced booleans unbound, server has them all false", + values: { ...ROOT, allow_all_keys: undefined, available_on_public_internet: undefined }, + ui: { mcpServer: { ...SERVER, allow_all_keys: false, available_on_public_internet: false } }, + }, + { + label: "dcr_bridge bound true on a client-forwarded mode", + values: { ...ROOT, auth_type: "true_passthrough", dcr_bridge: true }, + ui: { mcpServer: { ...SERVER, auth_type: "true_passthrough" } }, + }, + { + label: "dcr_bridge bound true but auth switched away, forced false", + values: { ...ROOT, auth_type: "api_key", dcr_bridge: true }, + ui: {}, + }, + { + label: "oauth_passthrough with an Authorization extra header", + values: { ...ROOT, auth_type: "none", extra_headers: ["Authorization"], oauth_passthrough: true }, + ui: {}, + }, + { + label: "oauth_passthrough without the Authorization header, forced false", + values: { ...ROOT, auth_type: "none", extra_headers: ["X-Other"], oauth_passthrough: true }, + ui: {}, + }, + + // --- auth-type transitions that null out the previous subtree --- + { + label: "was oauth2, now api_key: nulls the four oauth endpoints", + values: { ...ROOT, auth_type: "api_key", credentials: { auth_value: "v" } }, + ui: { mcpServer: { ...SERVER, auth_type: "oauth2" } }, + }, + { + label: "was token_exchange, now none: nulls the four exchange fields", + values: { ...ROOT, auth_type: "none" }, + ui: { mcpServer: { ...SERVER, auth_type: "oauth2_token_exchange" } }, + }, + + // --- credentials filtering --- + // ADMIN_CONFIG_CREDENTIAL_KEYS is exactly ["upstream_resource"], so only that key + // takes the blank-to-explicit-null branch. A blank client_id is dropped instead. + { + label: "blank upstream_resource becomes an explicit null", + values: { ...ROOT, auth_type: "oauth2", credentials: { upstream_resource: "", client_secret: "keep" } }, + ui: {}, + }, + { + label: "blank non-admin credential is dropped, not nulled", + values: { ...ROOT, auth_type: "oauth2", credentials: { client_id: "", client_secret: "keep", scopes: [] } }, + ui: {}, + }, + { + label: "scopes array filters empties and drops when nothing survives", + values: { ...ROOT, auth_type: "oauth2", credentials: { scopes: ["", null, "read"] } }, + ui: {}, + }, + { + label: "scopes entirely empty drops the key", + values: { ...ROOT, auth_type: "oauth2", credentials: { scopes: ["", null] } }, + ui: {}, + }, + { label: "credentials absent entirely", values: { ...ROOT, auth_type: "oauth2" }, ui: {} }, + { + label: "removeStoredApp forces an explicit-null app write", + values: { ...ROOT, auth_type: "true_passthrough", credentials: { ...CREDS } }, + ui: { removeStoredApp: true, mcpServer: { ...SERVER, auth_type: "true_passthrough" } }, + }, + { + label: "removeStoredApp ignored outside a client-forwarded mode", + values: { ...ROOT, auth_type: "api_key", credentials: { ...CREDS } }, + ui: { removeStoredApp: true }, + }, + + // --- headers, env vars, access groups --- + { + label: "static headers trim and drop blank names", + values: { + ...ROOT, + static_headers: [ + { header: " X-A ", value: " v " }, + { header: " ", value: "x" }, + ], + }, + ui: {}, + }, + { + label: "env vars normalize", + values: { ...ROOT, env_vars: [{ name: "A", value: "1", scope: "server", description: "d" }] }, + ui: {}, + }, + { label: "access groups given as objects", values: { ...ROOT, mcp_access_groups: [{ name: "g1" }, "g2"] }, ui: {} }, + { label: "extra_headers absent falls back to an empty array", values: { ...ROOT, extra_headers: undefined }, ui: {} }, + + // --- tool allowlist and overrides --- + { + label: "existing allowlist enforces and emits allowed_tools", + values: { ...ROOT }, + ui: { hasExistingToolAllowlist: true, allowedTools: ["alpha"] }, + }, + { + label: "allowlist interaction with an empty list still enforces", + values: { ...ROOT }, + ui: { hasToolAllowlistInteraction: true }, + }, + { + label: "tool display name and description maps present", + values: { ...ROOT }, + ui: { toolNameToDisplayName: { a: "Alpha" }, toolNameToDescription: { a: "desc" } }, + }, + + // --- cost config, logo, token validation --- + // mcp_info.server_name walks a six-step fallback chain. Every step needs a case whose + // earlier terms are falsy, or the chain is unreachable and a mutation deleting it survives. + { label: "mcp_info server name falls back to the form url", values: { ...ROOT, server_name: "" }, ui: {} }, + { + label: "mcp_info server name falls back to the stored server_name", + values: { ...ROOT, server_name: "", url: "" }, + ui: {}, + }, + { + label: "mcp_info server name falls back to the stored url", + values: { ...ROOT, server_name: "", url: "" }, + ui: { mcpServer: { ...SERVER, server_name: "" } }, + }, + { + label: "mcp_info server name falls back to the form alias", + values: { ...ROOT, server_name: "", url: "" }, + ui: { mcpServer: { ...SERVER, server_name: "", url: "" } }, + }, + { + label: "mcp_info server name falls back to unknown", + values: { ...ROOT, server_name: "", url: "", alias: "" }, + ui: { mcpServer: { ...SERVER, server_name: "", url: "", alias: "" } }, + }, + + { label: "cost config present", values: { ...ROOT }, ui: { costConfig: { default_cost_per_query: 0.01 } as never } }, + { label: "logo url present", values: { ...ROOT }, ui: { logoUrl: "https://cdn/logo.png" } }, + { label: "token validation JSON parses", values: { ...ROOT, token_validation_json: '{"aud":"x"}' }, ui: {} }, + { + label: "token validation blank clears an existing value", + values: { ...ROOT, token_validation_json: " " }, + ui: { mcpServer: { ...SERVER, token_validation: { aud: "old" } } }, + }, + { + label: "token validation blank with no existing value omits the key", + values: { ...ROOT, token_validation_json: "" }, + ui: {}, + }, + + // --- the six failure branches --- + { label: "ERR invalid tool display name", values: { ...ROOT }, ui: { toolNameToDisplayName: { a: "has spaces" } } }, + { + label: "ERR stdio config missing a command", + values: { ...ROOT, transport: "stdio", stdio_config: JSON.stringify({ args: [] }) }, + ui: {}, + }, + { + label: "ERR stdio config invalid JSON", + values: { ...ROOT, transport: "stdio", stdio_config: "{not json" }, + ui: {}, + }, + { + label: "ERR stdio env invalid JSON", + values: { ...ROOT, transport: "stdio", command: "npx", env_json: "{not json" }, + ui: {}, + }, + { + label: "ERR stdio dedicated path with a blank command", + values: { ...ROOT, transport: "stdio", command: " " }, + ui: {}, + }, + { label: "ERR token validation invalid JSON", values: { ...ROOT, token_validation_json: "{not json" }, ui: {} }, +]; diff --git a/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.test.ts new file mode 100644 index 00000000000..05e64468f4f --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.differential.test.ts @@ -0,0 +1,331 @@ +import { describe, expect, it } from "vitest"; +import { + ADMIN_CONFIG_CREDENTIAL_KEYS, + AUTH_TYPE, + MCPServer, + MCP_OAUTH2_FLOW_INTERACTIVE, + MCP_OAUTH2_FLOW_M2M, + OAUTH_FLOW, + TRANSPORT, + isClientForwardedTokenMode, + preservedAdminCredentials, +} from "@/components/mcp_tools/types"; +import { AUTH_TYPES_REQUIRING_CREDENTIALS } from "./createServerPayload"; +import { TOOL_DISPLAY_NAME_PATTERN, normalizeEnvVars } from "./utils"; +import { buildEditServerPayload, type EditServerUiState } from "./editServerPayload"; +import { CASES, baseUi } from "./editServerPayload.differential.cases"; + +// GENERATED by scratchpad/emit_test.py. The body below is machine-extracted from +// mcp_server_edit.tsx lines 675-950 and converted by exact string replacement; do +// not hand-edit it. Regenerate instead, so the baseline can never drift toward +// the implementation it is supposed to check. +const legacyBuild = (values: Record, ui: EditServerUiState) => { + const { + mcpServer, + logoUrl, + costConfig, + allowedTools, + hasExistingToolAllowlist, + hasToolAllowlistInteraction, + toolNameToDisplayName, + toolNameToDescription, + removeStoredApp, + } = ui; + const invalidDisplayName = Object.entries(toolNameToDisplayName).find( + ([, displayName]) => displayName && !TOOL_DISPLAY_NAME_PATTERN.test(displayName), + ); + if (invalidDisplayName) { + return { kind: "invalid_tool_display_name" as const, displayName: String(invalidDisplayName[1]) }; + } + // 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) { + return { kind: "stdio_config_missing_command" as const }; + } + } catch { + return { kind: "invalid_stdio_json" as const }; + } + } 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 { + return { kind: "invalid_stdio_env_json" as const }; + } + } + const parsedArgs = Array.isArray(rawArgs) + ? rawArgs.map((v: any) => String(v)).filter((v: string) => v.trim() !== "") + : []; + + const parsedCommand = rawCommand ? String(rawCommand).trim() : ""; + if (!parsedCommand) { + return { kind: "stdio_command_required" as const }; + } + + 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 { + return { kind: "invalid_token_validation_json" as const }; + } + } + + // 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 }; + } + return { kind: "ok" as const, payload }; +}; + +const clone = (v: T): T => structuredClone(v); + +describe("buildEditServerPayload matches the pre-extraction handleSave body", () => { + it.each(CASES.map((c) => [c.label, c] as const))("%s", (_label, testCase) => { + const ui: EditServerUiState = { ...baseUi, ...testCase.ui } as EditServerUiState; + const legacy = legacyBuild(clone(testCase.values), clone(ui) as EditServerUiState); + const next = buildEditServerPayload(clone(testCase.values), clone(ui) as EditServerUiState); + + expect(next.kind).toBe(legacy.kind); + if (legacy.kind !== "ok" || next.kind !== "ok") { + expect(next).toStrictEqual(legacy); + return; + } + expect(next.payload).toStrictEqual(legacy.payload); + expect(Object.keys(next.payload)).toStrictEqual(Object.keys(legacy.payload)); + expect(JSON.stringify(next.payload)).toBe(JSON.stringify(legacy.payload)); + }); +}); + +void ADMIN_CONFIG_CREDENTIAL_KEYS; +void AUTH_TYPE; +void AUTH_TYPES_REQUIRING_CREDENTIALS; +void MCP_OAUTH2_FLOW_INTERACTIVE; +void MCP_OAUTH2_FLOW_M2M; +void OAUTH_FLOW; +void TOOL_DISPLAY_NAME_PATTERN; +void TRANSPORT; +void isClientForwardedTokenMode; +void normalizeEnvVars; +void preservedAdminCredentials; +export type { MCPServer }; 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..3930f1edb9b --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/mcp-servers/_components/editServerPayload.ts @@ -0,0 +1,405 @@ +import { + ADMIN_CONFIG_CREDENTIAL_KEYS, + AUTH_TYPE, + MCPEnvVar, + MCPInfo, + MCPServer, + MCPServerCostInfo, + MCP_OAUTH2_FLOW_INTERACTIVE, + MCP_OAUTH2_FLOW_M2M, + OAUTH_FLOW, + TRANSPORT, + isClientForwardedTokenMode, + preservedAdminCredentials, +} from "@/components/mcp_tools/types"; +import { AUTH_TYPES_REQUIRING_CREDENTIALS, reduceStaticHeaders } from "./createServerPayload"; +import { TOOL_DISPLAY_NAME_PATTERN, normalizeEnvVars } from "./utils"; + +export type MCPAccessGroupValue = string | { readonly name?: string }; + +export interface EditServerFormValues { + readonly static_headers?: unknown; + readonly env_vars?: unknown; + readonly credentials?: unknown; + readonly stdio_config?: string; + readonly env_json?: string; + readonly command?: string; + readonly args?: readonly string[]; + readonly allow_all_keys?: boolean; + readonly available_on_public_internet?: boolean; + readonly delegate_auth_to_upstream?: boolean; + readonly oauth_passthrough?: boolean; + readonly dcr_bridge?: boolean; + readonly token_validation_json?: string; + readonly mcp_access_groups?: readonly MCPAccessGroupValue[]; + readonly transport?: string; + readonly server_name?: string; + readonly url?: string; + readonly alias?: string; + readonly description?: string; + readonly auth_type?: string; + readonly extra_headers?: readonly string[]; + readonly disallowed_tools?: readonly string[]; + readonly oauth_flow_type?: string; + readonly [key: string]: unknown; +} + +export interface EditServerPayload { + readonly server_id: string; + readonly mcp_info: MCPInfo; + readonly mcp_access_groups: readonly string[]; + readonly alias: string | undefined; + readonly extra_headers: readonly string[]; + readonly disallowed_tools: readonly string[]; + readonly static_headers: Readonly>; + readonly env_vars: readonly MCPEnvVar[]; + readonly allow_all_keys: boolean; + readonly available_on_public_internet: boolean; + readonly delegate_auth_to_upstream: boolean; + readonly oauth_passthrough: boolean; + readonly dcr_bridge: boolean; + readonly stdio_config: undefined; + readonly env_json: undefined; + readonly command?: string; + readonly args?: readonly string[]; + readonly env?: Readonly>; + readonly allowed_tools?: readonly string[]; + readonly tool_name_to_display_name: Readonly> | null; + readonly tool_name_to_description: Readonly> | null; + readonly oauth2_flow?: string; + readonly token_validation?: unknown; + readonly credentials?: Readonly>; + readonly [key: string]: unknown; +} + +export interface EditServerUiState { + readonly mcpServer: MCPServer; + readonly logoUrl: string | undefined; + readonly costConfig: MCPServerCostInfo; + readonly allowedTools: readonly string[]; + readonly hasExistingToolAllowlist: boolean; + readonly hasToolAllowlistInteraction: boolean; + readonly toolNameToDisplayName: Readonly>; + readonly toolNameToDescription: Readonly>; + readonly removeStoredApp: boolean; +} + +export type BuildEditPayloadResult = + | { readonly kind: "ok"; readonly payload: EditServerPayload } + | { readonly kind: "invalid_tool_display_name"; readonly displayName: string } + | { readonly kind: "stdio_config_missing_command" } + | { readonly kind: "invalid_stdio_json" } + | { readonly kind: "invalid_stdio_env_json" } + | { readonly kind: "stdio_command_required" } + | { readonly kind: "invalid_token_validation_json" }; + +interface StdioFields { + readonly command: string; + readonly args: readonly string[]; + readonly env: Readonly>; +} + +type StdioFieldsResult = + | { readonly kind: "ok"; readonly fields: StdioFields | Record } + | { readonly kind: "stdio_config_missing_command" } + | { readonly kind: "invalid_stdio_json" } + | { readonly kind: "invalid_stdio_env_json" } + | { readonly kind: "stdio_command_required" }; + +type TokenValidationResult = { readonly kind: "ok"; readonly value: unknown } | { readonly kind: "invalid" }; + +const assertNever = (value: never): never => { + throw new Error(`unhandled edit payload result: ${JSON.stringify(value)}`); +}; + +export 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 "stdio_config_missing_command": + return "Stdio configuration must include a command"; + case "invalid_stdio_json": + return "Invalid JSON in stdio configuration"; + case "invalid_stdio_env_json": + return "Invalid JSON in stdio env configuration"; + case "stdio_command_required": + return "Stdio transport requires a command"; + case "invalid_token_validation_json": + return "Invalid JSON in Token Validation Rules"; + default: + return assertNever(result); + } +}; + +const toStringArgs = (raw: unknown): readonly string[] => + Array.isArray(raw) ? raw.map((v: unknown) => String(v)).filter((v: string) => v.trim() !== "") : []; + +const toEnvRecord = (raw: unknown): Readonly> => + raw && typeof raw === "object" && !Array.isArray(raw) + ? Object.fromEntries( + Object.entries(raw) + .filter(([k]) => k != null && String(k).trim() !== "") + .map(([k, v]) => [String(k), v == null ? "" : String(v)]), + ) + : {}; + +const buildStdioFields = ( + rawStdioConfig: string | undefined, + rawEnvJson: string | undefined, + rawCommand: string | undefined, + rawArgs: readonly string[] | undefined, +): StdioFieldsResult => { + if (rawStdioConfig) { + try { + const stdioConfig: unknown = JSON.parse(rawStdioConfig); + const configRecord = stdioConfig && typeof stdioConfig === "object" ? (stdioConfig as StdioConfigShape) : null; + const namedServers = + configRecord?.mcpServers && typeof configRecord.mcpServers === "object" ? configRecord.mcpServers : null; + const named = namedServers ? Object.keys(namedServers) : []; + const actualConfig = named.length > 0 && namedServers ? namedServers[named[0]] : configRecord; + const command = actualConfig?.command ? String(actualConfig.command) : undefined; + if (!command) { + return { kind: "stdio_config_missing_command" }; + } + return { + kind: "ok", + fields: { command, args: toStringArgs(actualConfig?.args), env: toEnvRecord(actualConfig?.env) }, + }; + } catch { + return { kind: "invalid_stdio_json" }; + } + } + + const envResult = ((): Readonly> | "invalid" => { + if (!rawEnvJson) return {}; + try { + return toEnvRecord(JSON.parse(rawEnvJson)); + } catch { + return "invalid"; + } + })(); + if (envResult === "invalid") { + return { kind: "invalid_stdio_env_json" }; + } + + const parsedCommand = rawCommand ? String(rawCommand).trim() : ""; + if (!parsedCommand) { + return { kind: "stdio_command_required" }; + } + return { kind: "ok", fields: { command: parsedCommand, args: toStringArgs(rawArgs), env: envResult } }; +}; + +interface StdioConfigShape { + readonly mcpServers?: Readonly>; + readonly command?: unknown; + readonly args?: unknown; + readonly env?: unknown; +} + +const buildCredentials = (credentialValues: unknown): Readonly> | undefined => { + if (!credentialValues || typeof credentialValues !== "object") return undefined; + const kept = Object.entries(credentialValues).flatMap(([key, value]): readonly (readonly [string, unknown])[] => { + if (value === undefined || value === null || value === "") { + return value === "" && (ADMIN_CONFIG_CREDENTIAL_KEYS as readonly string[]).includes(key) + ? [[key, null] as const] + : []; + } + if (key !== "scopes") return [[key, value] as const]; + if (!Array.isArray(value)) return []; + const filteredScopes = value.filter((scope: unknown) => scope != null && scope !== ""); + return filteredScopes.length > 0 ? [[key, filteredScopes] as const] : []; + }); + return Object.fromEntries(kept); +}; + +interface CredentialsEntryInput { + readonly authType: string | undefined; + readonly credentials: Readonly> | undefined; + readonly includeCredentials: boolean; + readonly removeStoredApp: boolean; +} + +// 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. +const resolveCredentialsEntry = ({ + authType, + credentials, + includeCredentials, + removeStoredApp, +}: CredentialsEntryInput): { readonly credentials?: Readonly> } => { + if (removeStoredApp && isClientForwardedTokenMode(authType)) { + return { credentials: { client_id: null, client_secret: null } }; + } + if (includeCredentials && credentials && Object.keys(credentials).length > 0) { + return { credentials }; + } + return {}; +}; + +export const buildEditServerPayload = (values: EditServerFormValues, ui: EditServerUiState): BuildEditPayloadResult => { + const { + mcpServer, + logoUrl, + costConfig, + allowedTools, + hasExistingToolAllowlist, + hasToolAllowlistInteraction, + toolNameToDisplayName, + toolNameToDescription, + removeStoredApp, + } = ui; + + const invalidDisplayName = Object.entries(toolNameToDisplayName).find( + ([, displayName]) => displayName && !TOOL_DISPLAY_NAME_PATTERN.test(displayName), + ); + if (invalidDisplayName) { + return { kind: "invalid_tool_display_name", displayName: String(invalidDisplayName[1]) }; + } + + 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, + ...rawRestValues + } = values; + + const accessGroups = (rawRestValues.mcp_access_groups || []).map((g) => + typeof g === "string" ? g : g.name || String(g), + ); + const staticHeaders = reduceStaticHeaders(staticHeadersList); + const envVars = normalizeEnvVars(envVarsList); + const credentialsPayload = buildCredentials(credentialValues); + + const stdio = + rawRestValues.transport === "stdio" + ? buildStdioFields(rawStdioConfig, rawEnvJson, rawCommand, rawArgs) + : ({ kind: "ok", fields: {} } as StdioFieldsResult); + if (stdio.kind !== "ok") { + return stdio; + } + + // Map "openapi" transport to "http" for the backend. Rewriting the existing key keeps its + // position, which the payload's serialised key order depends on. + const restValues = + rawRestValues.transport === TRANSPORT.OPENAPI ? { ...rawRestValues, transport: "http" } : rawRestValues; + + const tokenValidation = ((): TokenValidationResult => { + if (!rawTokenValidationJson || rawTokenValidationJson.trim() === "") return { kind: "ok", value: null }; + try { + return { kind: "ok", value: JSON.parse(rawTokenValidationJson) }; + } catch { + return { kind: "invalid" }; + } + })(); + if (tokenValidation.kind === "invalid") { + return { kind: "invalid_token_validation_json" }; + } + + 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 extraHeaders = restValues.extra_headers || []; + const hasAuthorizationHeader = extraHeaders.some((h) => typeof h === "string" && h.toLowerCase() === "authorization"); + const isNoneAuth = restValues.auth_type === AUTH_TYPE.NONE || restValues.auth_type == null; + + // 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; + const includeCredentials = restValues.auth_type && AUTH_TYPES_REQUIRING_CREDENTIALS.includes(restValues.auth_type); + + const credentialsEntryInput: CredentialsEntryInput = { + authType: restValues.auth_type, + credentials: submitCredentials, + includeCredentials: Boolean(includeCredentials), + removeStoredApp, + }; + const credentialsEntry = resolveCredentialsEntry(credentialsEntryInput); + + const payload: EditServerPayload = { + ...restValues, + ...stdio.fields, + // 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: extraHeaders, + ...(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 field unmounts on auth_type change, so force false for any other + // configuration to avoid persisting a stale ``true`` that would silently re-activate. + delegate_auth_to_upstream: + restValues.auth_type === AUTH_TYPE.OAUTH2 + ? Boolean(delegateAuthToUpstreamRaw ?? mcpServer.delegate_auth_to_upstream) + : false, + // ``oauth_passthrough`` is the dedicated, non-oauth2 opt-in, honored only 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. The field unmounts on + // auth_type change, so force false otherwise rather than persisting a stale ``true``. + 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.value !== null || mcpServer.token_validation + ? { token_validation: tokenValidation.value } + : {}), + ...credentialsEntry, + }; + + return { kind: "ok", payload }; +}; 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..c6ebde13bb8 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 { EditServerFormValues, buildEditServerPayload, editPayloadErrorMessage } from "./editServerPayload"; import { toast } from "@/lib/toast"; import { useMcpOAuthFlow } from "@/hooks/useMcpOAuthFlow"; import { getSecureItem, setSecureItem } from "@/utils/secureStorage"; @@ -62,15 +56,6 @@ interface MCPServerEditProps { } const AUTH_TYPES_REQUIRING_AUTH_VALUE = [AUTH_TYPE.API_KEY, AUTH_TYPE.BEARER_TOKEN, AUTH_TYPE.TOKEN, AUTH_TYPE.BASIC]; -const AUTH_TYPES_REQUIRING_CREDENTIALS = [ - ...AUTH_TYPES_REQUIRING_AUTH_VALUE, - AUTH_TYPE.OAUTH2, - AUTH_TYPE.OAUTH2_TOKEN_EXCHANGE, - AUTH_TYPE.OAUTH2_ID_JAG, - AUTH_TYPE.AWS_SIGV4, - AUTH_TYPE.TRUE_PASSTHROUGH, - AUTH_TYPE.OAUTH_DELEGATE, -]; export const EDIT_OAUTH_UI_STATE_KEY = "litellm-mcp-oauth-edit-state"; const MCPServerEdit: React.FC = ({ @@ -670,284 +655,25 @@ const MCPServerEdit: React.FC = ({ } }; - const handleSave = async (values: Record) => { + const handleSave = async (values: EditServerFormValues) => { if (!accessToken) return; - const invalidDisplayName = Object.entries(toolNameToDisplayName).find( - ([, displayName]) => displayName && !TOOL_DISPLAY_NAME_PATTERN.test(displayName), - ); - if (invalidDisplayName) { - toast.fromError( - `Tool display name "${invalidDisplayName[1]}" is invalid. Only letters, digits, underscores, and hyphens are allowed (no spaces).`, - ); - return; - } 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 built = buildEditServerPayload(values, { + mcpServer, + logoUrl, + costConfig, + allowedTools, + hasExistingToolAllowlist, + hasToolAllowlistInteraction, + toolNameToDisplayName, + toolNameToDescription, + removeStoredApp, + }); + if (built.kind !== "ok") { + toast.fromError(editPayloadErrorMessage(built)); + return; } + const payload = built.payload; const updated = await updateMCPServer(accessToken, payload); @@ -957,9 +683,9 @@ 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: values.auth_type, oauth2_flow: isM2MFlow ? MCP_OAUTH2_FLOW_M2M : null, - delegate_auth_to_upstream: Boolean(delegateAuthToUpstreamRaw ?? mcpServer.delegate_auth_to_upstream), + delegate_auth_to_upstream: Boolean(values.delegate_auth_to_upstream ?? mcpServer.delegate_auth_to_upstream), }); try { if (oauthMode === "authorization_code") { @@ -971,7 +697,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(values.auth_type)) { const browserHeldToken = { access_token: oauthTokenResponse.access_token, expires_in: oauthTokenResponse.expires_in,