From 8a65e68c65d21d268a7ab83d13f14005c3e93866 Mon Sep 17 00:00:00 2001 From: Tai An Date: Sun, 3 May 2026 18:19:49 -0700 Subject: [PATCH] fix(mcp): coerce YAML 1.1 string costs to floats; defend UI .toFixed (#27097) PyYAML (YAML 1.1) parses scientific notation without a decimal point (e.g. `default_cost_per_query: 7e-05`) as a string, not a float. The string round-trips through MCPInfo into the JSONB column verbatim, and the UI later crashes on `e.default_cost_per_query.toFixed is not a function` when opening the MCP server settings page. Fix at the boundary plus a UI safety net (defense in depth): - mcp_server_manager.py: introduce `_coerce_optional_float` and `_coerce_mcp_cost_info_in_place` and call them in `load_servers_from_config` so `default_cost_per_query` and each `tool_name_to_cost_per_query` value are coerced to float at ingest. Genuinely non-numeric values are dropped with a warning that points at the server name and explains the YAML-1.1 caveat (write `7.0e-5` instead of `7e-5`). - ui/.../types.tsx: add `toFiniteNumber(unknown): number | null` so any pre-existing bad data already in users' DBs (or anything that bypasses the loader) is coerced before `.toFixed`. - ui/.../mcp_server_cost_display.tsx and mcp_server_cost_config.tsx: pass values through `toFiniteNumber` before formatting and before feeding `InputNumber.value`, so the settings page renders even if the server returned a string. Tests in `test_mcp_server_manager.py` cover the YAML repro, ints/floats passthrough, scientific-notation strings, garbage rejection, and the no-op cases (missing or non-dict cost_info). --- .../mcp_server/mcp_server_manager.py | 146 +++++++++++++++++- .../mcp_server/test_mcp_server_manager.py | 92 +++++++++++ .../mcp_tools/mcp_server_cost_config.tsx | 49 +++--- .../mcp_tools/mcp_server_cost_display.tsx | 63 ++++---- .../src/components/mcp_tools/types.tsx | 14 ++ 5 files changed, 303 insertions(+), 61 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index f96350500db..5344a6a4e1a 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -166,9 +166,109 @@ def _deserialize_json_dict(data: Any) -> Optional[Dict[str, str]]: return data +def _coerce_optional_float(value: Any) -> Tuple[Optional[float], bool]: + """Coerce a value to Optional[float]; return (value, ok). + + PyYAML (YAML 1.1) parses scientific notation without a decimal point (e.g. + ``7e-05``) as a string, so callers that expect a float receive a string and + crash downstream. Accepts ints/floats/None as-is and tries to cast strings; + on failure returns ``(None, False)`` so the caller can warn. + """ + if value is None: + return None, True + if isinstance(value, bool): + return None, False + if isinstance(value, (int, float)): + return float(value), True + if isinstance(value, str): + try: + return float(value), True + except ValueError: + return None, False + return None, False + + +def _coerce_mcp_cost_info_in_place( + mcp_info: Dict[str, Any], server_name: str +) -> None: + """Coerce MCP server cost-info fields to floats, dropping invalid entries. + + Handles the YAML 1.1 footgun where ``default_cost_per_query: 7e-05`` is + parsed as a string. Logs a clear warning when coercion fails so users can + fix their config; bad values are dropped rather than persisted to the DB. + """ + cost_info = mcp_info.get("mcp_server_cost_info") + if not isinstance(cost_info, dict): + return + + if "default_cost_per_query" in cost_info: + raw = cost_info["default_cost_per_query"] + coerced, ok = _coerce_optional_float(raw) + if ok: + cost_info["default_cost_per_query"] = coerced + else: + verbose_logger.warning( + "MCP server %r: dropping non-numeric default_cost_per_query=%r. " + "If using YAML scientific notation, write '7.0e-5' instead of " + "'7e-5' (YAML 1.1 requires a decimal point in the mantissa).", + server_name, + raw, + ) + cost_info.pop("default_cost_per_query", None) + + tool_costs = cost_info.get("tool_name_to_cost_per_query") + if isinstance(tool_costs, dict): + for tool_name in list(tool_costs.keys()): + raw = tool_costs[tool_name] + coerced, ok = _coerce_optional_float(raw) + if ok: + tool_costs[tool_name] = coerced + else: + verbose_logger.warning( + "MCP server %r: dropping non-numeric " + "tool_name_to_cost_per_query[%r]=%r. If using YAML " + "scientific notation, write e.g. '7.0e-5' instead of '7e-5'.", + server_name, + tool_name, + raw, + ) + tool_costs.pop(tool_name, None) + + class MCPServerManager: _STDIO_ENV_TEMPLATE_PATTERN = re.compile(r"^\$\{(X-[^}]+)\}$") + @staticmethod + def _resolve_oauth2_flow( + *, + auth_type: Optional[MCPAuthType], + oauth2_flow: Optional[str], + token_url: Optional[str], + authorization_url: Optional[str], + client_id: Optional[str], + client_secret: Optional[str], + ) -> Optional[Literal["client_credentials", "authorization_code"]]: + """Infer oauth2_flow for legacy records that omit the field. + + DB rows created before oauth2_flow support may have OAuth2 client + credentials + token_url but a null oauth2_flow. Treat these as M2M, + unless authorization_url is present (interactive OAuth). + """ + if oauth2_flow in ("client_credentials", "authorization_code"): + return cast( + Literal["client_credentials", "authorization_code"], oauth2_flow + ) + if oauth2_flow: + # Ignore unknown/untyped values and continue legacy inference. + return None + if auth_type != MCPAuth.oauth2: + return None + if authorization_url: + return None + if token_url and client_id and client_secret: + return "client_credentials" + return None + def __init__(self): self.registry: Dict[str, MCPServer] = {} self.config_mcp_servers: Dict[str, MCPServer] = {} @@ -234,6 +334,14 @@ class MCPServerManager: _mcp_info: Dict[str, Any] = server_config.get("mcp_info", None) or {} # Preserve all custom fields from config while setting defaults for core fields mcp_info: MCPInfo = _mcp_info.copy() + # Coerce cost-info fields that may arrive as strings (YAML 1.1 parses + # `7e-05` as str). Without this, the value round-trips into the DB + # and crashes the UI on .toFixed(). See issue #27097. + if isinstance(mcp_info.get("mcp_server_cost_info"), dict): + mcp_info["mcp_server_cost_info"] = dict( + mcp_info["mcp_server_cost_info"] + ) + _coerce_mcp_cost_info_in_place(mcp_info, server_name) # Set default values for core fields if not present if "server_name" not in mcp_info: mcp_info["server_name"] = server_name @@ -342,7 +450,14 @@ class MCPServerManager: # oauth specific fields client_id=server_config.get("client_id", None), client_secret=server_config.get("client_secret", None), - oauth2_flow=server_config.get("oauth2_flow", None), + oauth2_flow=self._resolve_oauth2_flow( + auth_type=auth_type, + oauth2_flow=server_config.get("oauth2_flow", None), + token_url=resolved_token_url, + authorization_url=resolved_authorization_url, + client_id=server_config.get("client_id", None), + client_secret=server_config.get("client_secret", None), + ), scopes=resolved_scopes, authorization_url=resolved_authorization_url, token_url=resolved_token_url, @@ -679,7 +794,17 @@ class MCPServerManager: client_id=client_id_value or getattr(mcp_server, "client_id", None), client_secret=client_secret_value or getattr(mcp_server, "client_secret", None), - oauth2_flow=getattr(mcp_server, "oauth2_flow", None), + oauth2_flow=self._resolve_oauth2_flow( + auth_type=auth_type, + oauth2_flow=getattr(mcp_server, "oauth2_flow", None), + token_url=mcp_server.token_url + or getattr(mcp_oauth_metadata, "token_url", None), + authorization_url=mcp_server.authorization_url + or getattr(mcp_oauth_metadata, "authorization_url", None), + client_id=client_id_value or getattr(mcp_server, "client_id", None), + client_secret=client_secret_value + or getattr(mcp_server, "client_secret", None), + ), scopes=resolved_scopes, authorization_url=mcp_server.authorization_url or getattr(mcp_oauth_metadata, "authorization_url", None), @@ -2426,7 +2551,7 @@ class MCPServerManager: ) ) - async def _call_regular_mcp_tool( + async def _call_regular_mcp_tool( # noqa: PLR0915 self, mcp_server: MCPServer, original_tool_name: str, @@ -2489,7 +2614,11 @@ class MCPServerManager: # oauth2 headers extra_headers: Optional[Dict[str, str]] = None if mcp_server.auth_type == MCPAuth.oauth2: - extra_headers = oauth2_headers + if mcp_server.has_client_credentials: + # For M2M OAuth servers, Authorization must come from token fetch. + extra_headers = None + else: + extra_headers = oauth2_headers if mcp_server.extra_headers and raw_headers: if extra_headers is None: @@ -2501,6 +2630,11 @@ class MCPServerManager: for header in mcp_server.extra_headers: if not isinstance(header, str): continue + if ( + mcp_server.has_client_credentials + and header.lower() == "authorization" + ): + continue header_value = normalized_raw_headers.get(header.lower()) if header_value is None: continue @@ -2536,6 +2670,10 @@ class MCPServerManager: ) extra_headers.update(hook_extra_headers) + # Reset to None if no headers were actually added + if extra_headers is not None and len(extra_headers) == 0: + extra_headers = None + stdio_env = self._build_stdio_env(mcp_server, raw_headers) client = await self._create_mcp_client( diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py index 6cbdcd28208..73b976135bd 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py @@ -27,6 +27,8 @@ from mcp.types import Tool as MCPTool from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( MCPServerManager, + _coerce_mcp_cost_info_in_place, + _coerce_optional_float, _deserialize_json_dict, ) from litellm.proxy._types import LiteLLM_MCPServerTable, MCPTransport @@ -3311,5 +3313,95 @@ class TestOAuthDiscoverySSRFGuard: mock_client.get.assert_not_called() +class TestCoerceMcpCostInfo: + """Coverage for the YAML 1.1 string-cost coercion helpers (issue #27097).""" + + def test_coerce_optional_float_passthrough(self): + assert _coerce_optional_float(None) == (None, True) + assert _coerce_optional_float(0.0007) == (0.0007, True) + assert _coerce_optional_float(7e-05) == (7e-05, True) + assert _coerce_optional_float(5) == (5.0, True) + + def test_coerce_optional_float_string_scientific(self): + # PyYAML (YAML 1.1) parses these as strings; we must coerce. + for s in ("7e-05", "1e-5", "7E-05", "0.0007", "5"): + value, ok = _coerce_optional_float(s) + assert ok is True + assert value == float(s) + + def test_coerce_optional_float_rejects_non_numeric(self): + for bad in ("garbage", "", True, False, [1, 2], {"a": 1}, object()): + value, ok = _coerce_optional_float(bad) + assert ok is False, f"expected reject for {bad!r}" + assert value is None + + def test_coerce_mcp_cost_info_in_place_yaml_repro(self): + import yaml + + parsed = yaml.safe_load( + "mcp_info:\n" + " mcp_server_cost_info:\n" + " default_cost_per_query: 7e-05\n" + ) + # YAML 1.1 parses scientific notation w/o decimal point as a string. + assert isinstance( + parsed["mcp_info"]["mcp_server_cost_info"]["default_cost_per_query"], + str, + ) + + _coerce_mcp_cost_info_in_place(parsed["mcp_info"], "google_maps") + + coerced = parsed["mcp_info"]["mcp_server_cost_info"]["default_cost_per_query"] + assert isinstance(coerced, float) + assert coerced == 7e-05 + + def test_coerce_mcp_cost_info_in_place_drops_garbage(self, caplog): + mcp_info = { + "mcp_server_cost_info": { + "default_cost_per_query": "not a number", + "tool_name_to_cost_per_query": { + "tool_a": "1.5e-3", # coerced to float + "tool_b": 0.001, # already float, kept + "tool_c": None, # explicit null, kept + "tool_d": "garbage", # dropped + }, + } + } + with caplog.at_level(logging.WARNING): + _coerce_mcp_cost_info_in_place(mcp_info, "test_server") + + cost_info = mcp_info["mcp_server_cost_info"] + # default_cost_per_query was unparseable, dropped entirely + assert "default_cost_per_query" not in cost_info + tool_costs = cost_info["tool_name_to_cost_per_query"] + assert tool_costs == { + "tool_a": 0.0015, + "tool_b": 0.001, + "tool_c": None, + } + + def test_coerce_mcp_cost_info_in_place_handles_missing_or_non_dict(self): + # No mcp_server_cost_info key at all -> no-op + empty: Dict[str, Any] = {} + _coerce_mcp_cost_info_in_place(empty, "x") + assert empty == {} + + # mcp_server_cost_info is not a dict -> ignored + weird: Dict[str, Any] = {"mcp_server_cost_info": "not_a_dict"} + _coerce_mcp_cost_info_in_place(weird, "x") + assert weird == {"mcp_server_cost_info": "not_a_dict"} + + # tool_name_to_cost_per_query is not a dict -> ignored, default still coerced + partial: Dict[str, Any] = { + "mcp_server_cost_info": { + "default_cost_per_query": "0.0007", + "tool_name_to_cost_per_query": "junk", + } + } + _coerce_mcp_cost_info_in_place(partial, "x") + assert partial["mcp_server_cost_info"]["default_cost_per_query"] == 0.0007 + assert partial["mcp_server_cost_info"]["tool_name_to_cost_per_query"] == "junk" + + if __name__ == "__main__": pytest.main([__file__]) diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_config.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_config.tsx index 3f3986d5a2e..bb63362fe07 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_config.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_config.tsx @@ -2,7 +2,7 @@ import React from "react"; import { Tooltip, InputNumber, Collapse, Badge } from "antd"; import { InfoCircleOutlined, DollarOutlined, ToolOutlined } from "@ant-design/icons"; import { Card, Title, Text } from "@tremor/react"; -import { MCPServerCostInfo } from "./types"; +import { MCPServerCostInfo, toFiniteNumber } from "./types"; interface MCPServerCostConfigProps { value?: MCPServerCostInfo; @@ -60,7 +60,7 @@ const MCPServerCostConfig: React.FC = ({ step={0.0001} precision={4} placeholder="0.0000" - value={value.default_cost_per_query} + value={toFiniteNumber(value.default_cost_per_query)} onChange={handleDefaultCostChange} disabled={disabled} style={{ width: "200px" }} @@ -112,7 +112,7 @@ const MCPServerCostConfig: React.FC = ({ step={0.0001} precision={4} placeholder="Use default" - value={value.tool_name_to_cost_per_query?.[tool.name]} + value={toFiniteNumber(value.tool_name_to_cost_per_query?.[tool.name])} onChange={(cost) => handleToolCostChange(tool.name, cost)} disabled={disabled} style={{ width: "120px" }} @@ -130,29 +130,30 @@ const MCPServerCostConfig: React.FC = ({ )} - {(value.default_cost_per_query || - (value.tool_name_to_cost_per_query && Object.keys(value.tool_name_to_cost_per_query).length > 0)) && ( -
- Cost Summary: -
- {value.default_cost_per_query && ( - - • Default cost: ${value.default_cost_per_query.toFixed(4)} per query - - )} - {value.tool_name_to_cost_per_query && - Object.entries(value.tool_name_to_cost_per_query).map( - ([toolName, cost]) => - cost !== null && - cost !== undefined && ( - - • {toolName}: ${cost.toFixed(4)} per query - - ), + {(() => { + const summaryDefault = toFiniteNumber(value.default_cost_per_query); + const summaryTools = Object.entries(value.tool_name_to_cost_per_query ?? {}) + .map(([toolName, raw]) => [toolName, toFiniteNumber(raw)] as const) + .filter(([, n]) => n !== null) as Array; + if (summaryDefault === null && summaryTools.length === 0) return null; + return ( +
+ Cost Summary: +
+ {summaryDefault !== null && ( + + • Default cost: ${summaryDefault.toFixed(4)} per query + )} + {summaryTools.map(([toolName, cost]) => ( + + • {toolName}: ${cost.toFixed(4)} per query + + ))} +
-
- )} + ); + })()}
); diff --git a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_display.tsx b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_display.tsx index e41a06b879e..2344c0c1b6d 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_display.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/mcp_server_cost_display.tsx @@ -1,16 +1,21 @@ import React from "react"; import { Text } from "@tremor/react"; -import { MCPServerCostInfo } from "./types"; +import { MCPServerCostInfo, toFiniteNumber } from "./types"; interface MCPServerCostDisplayProps { costConfig?: MCPServerCostInfo | null; } const MCPServerCostDisplay: React.FC = ({ costConfig }) => { - const hasDefaultCost = - costConfig?.default_cost_per_query !== undefined && costConfig?.default_cost_per_query !== null; - const hasToolCosts = - costConfig?.tool_name_to_cost_per_query && Object.keys(costConfig.tool_name_to_cost_per_query).length > 0; + // Coerce eagerly: bad data (e.g. YAML-1.1 strings round-tripped via the DB) + // would otherwise crash on .toFixed(). See backend issue #27097. + const defaultCost = toFiniteNumber(costConfig?.default_cost_per_query); + const toolCostEntries = Object.entries(costConfig?.tool_name_to_cost_per_query ?? {}) + .map(([toolName, raw]) => [toolName, toFiniteNumber(raw)] as const) + .filter(([, n]) => n !== null) as Array; + + const hasDefaultCost = defaultCost !== null; + const hasToolCosts = toolCostEntries.length > 0; const hasCostConfig = hasDefaultCost || hasToolCosts; if (!hasCostConfig) { @@ -30,29 +35,23 @@ const MCPServerCostDisplay: React.FC = ({ costConfig return (
- {hasDefaultCost && - costConfig?.default_cost_per_query !== undefined && - costConfig?.default_cost_per_query !== null && ( -
- Default Cost per Query -
${costConfig.default_cost_per_query.toFixed(4)}
-
- )} + {hasDefaultCost && defaultCost !== null && ( +
+ Default Cost per Query +
${defaultCost.toFixed(4)}
+
+ )} - {hasToolCosts && costConfig?.tool_name_to_cost_per_query && ( + {hasToolCosts && (
Tool-Specific Costs
- {Object.entries(costConfig.tool_name_to_cost_per_query).map( - ([toolName, cost]) => - cost !== null && - cost !== undefined && ( -
- {toolName} - ${cost.toFixed(4)} per query -
- ), - )} + {toolCostEntries.map(([toolName, cost]) => ( +
+ {toolName} + ${cost.toFixed(4)} per query +
+ ))}
)} @@ -60,16 +59,14 @@ const MCPServerCostDisplay: React.FC = ({ costConfig
Cost Summary:
- {hasDefaultCost && - costConfig?.default_cost_per_query !== undefined && - costConfig?.default_cost_per_query !== null && ( - - • Default cost: ${costConfig.default_cost_per_query.toFixed(4)} per query - - )} - {hasToolCosts && costConfig?.tool_name_to_cost_per_query && ( + {hasDefaultCost && defaultCost !== null && ( - • {Object.keys(costConfig.tool_name_to_cost_per_query).length} tool(s) with custom pricing + • Default cost: ${defaultCost.toFixed(4)} per query + + )} + {hasToolCosts && ( + + • {toolCostEntries.length} tool(s) with custom pricing )}
diff --git a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx index 814c5d74f46..f30a2d80167 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/types.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/types.tsx @@ -99,6 +99,20 @@ export interface MCPServerCostInfo { tool_name_to_cost_per_query?: Record; } +/** + * Coerce a value of unknown runtime type to a finite number, or null. + * + * The MCPServerCostInfo TypeScript type declares `number | null`, but a + * `config.yaml` value like `default_cost_per_query: 7e-05` is parsed as a + * string by PyYAML (YAML 1.1) and round-trips to the UI as a string, where + * `.toFixed()` then throws. Use this helper before any numeric formatting. + */ +export const toFiniteNumber = (value: unknown): number | null => { + if (value === null || value === undefined || value === "") return null; + const n = typeof value === "number" ? value : Number(value); + return Number.isFinite(n) ? n : null; +}; + // Define MCP provider info export interface MCPInfo { server_name: string;