diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 128a44ff6cd..32aa273c548 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -1562,7 +1562,21 @@ class MCPServerManager: user_values: Dict[str, str] = {} if referenced_user_vars: - user_values = await self._load_user_env_vars(server, user_api_key_auth) + try: + user_values = await self._load_user_env_vars(server, user_api_key_auth) + except Exception as exc: + # On the tool-call path a DB failure must surface as a real + # server error, not a misleading "set up your credentials" 412. + # On the listing path we stay best-effort and leave the + # unfilled ${NAME} references untouched so tools still appear. + if raise_on_missing: + raise + verbose_logger.debug( + "MCPServerManager: best-effort user env var load failed for " + "server=%s: %s", + server.server_id, + exc, + ) if raise_on_missing: missing = sorted( @@ -1586,11 +1600,11 @@ class MCPServerManager: server: MCPServer, user_api_key_auth: Optional[UserAPIKeyAuth], ) -> Dict[str, str]: - """Best-effort lookup of the calling user's env var values for ``server``. + """Look up the calling user's env var values for ``server``. - Returns an empty dict when no user is available or the DB lookup - fails — callers detect missing values via name lookup, not by an - exception here. + Returns an empty dict when no user is available. DB errors propagate + so the caller can decide between failing the request (tool-call path) + and staying best-effort (listing path). """ if user_api_key_auth is None: return {} @@ -1605,17 +1619,7 @@ class MCPServerManager: get_user_env_vars, ) - try: - return await get_user_env_vars(prisma_client, user_id, server.server_id) - except Exception as exc: - verbose_logger.debug( - "MCPServerManager: failed to load user env vars for " - "user=%s server=%s: %s", - user_id, - server.server_id, - exc, - ) - return {} + return await get_user_env_vars(prisma_client, user_id, server.server_id) async def _create_mcp_client( self, diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 543905ea023..f5b4b583599 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -146,6 +146,7 @@ if MCP_AVAILABLE: LitellmUserRoles, MakeMCPServersPublicRequest, MCPApprovalStatus, + MCPEnvVarScope, MCPOAuthUserCredentialRequest, MCPOAuthUserCredentialStatus, MCPSubmissionsSummary, @@ -483,6 +484,18 @@ if MCP_AVAILABLE: ) -> List[LiteLLM_MCPServerTable]: return [_redact_mcp_credentials(server) for server in mcp_servers] + def _redact_global_env_var_values(mcp_server: LiteLLM_MCPServerTable) -> None: + """Blank admin-supplied ``scope="global"`` env var secrets in place. + + Global entries hold the admin's plaintext credential (API key, + password, ...) and must never reach non-admin callers. Per-user + entries only carry a placeholder the user fills in themselves, so + their value is left intact. + """ + for env_var in mcp_server.env_vars or []: + if env_var.scope == MCPEnvVarScope.global_: + env_var.value = "" + def _is_restricted_virtual_key_request(user_api_key_dict: UserAPIKeyAuth) -> bool: """Best-effort detection for route-restricted virtual keys. @@ -523,6 +536,7 @@ if MCP_AVAILABLE: sanitized.authorization_url = None sanitized.token_url = None sanitized.registration_url = None + _redact_global_env_var_values(sanitized) return sanitized def _sanitize_mcp_server_list_for_non_admin( @@ -554,6 +568,7 @@ if MCP_AVAILABLE: sanitized.allowed_tools = [] sanitized.mcp_access_groups = [] sanitized.teams = [] + _redact_global_env_var_values(sanitized) sanitized.authorization_url = None sanitized.token_url = None diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py index 2316f609fb0..0d0c4995ed6 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py @@ -250,6 +250,55 @@ async def test_resolve_static_headers_missing_is_non_blocking_for_listing( } +@pytest.mark.asyncio +async def test_resolve_static_headers_propagates_db_error_on_tool_call( + mock_server, monkeypatch +): + """A DB failure on the tool-call path must surface as a real error, not be + masked as a "missing credentials" MCPMissingUserEnvVarsError (412).""" + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + MCPServerManager, + ) + + manager = MCPServerManager() + + async def boom(server, user_api_key_auth): + raise RuntimeError("db down") + + monkeypatch.setattr(manager, "_load_user_env_vars", boom) + + with pytest.raises(RuntimeError, match="db down"): + await manager._resolve_static_headers_with_env_vars( + mock_server, user_api_key_auth=object() + ) + + +@pytest.mark.asyncio +async def test_resolve_static_headers_swallows_db_error_on_listing( + mock_server, monkeypatch +): + """On the listing path a DB failure is non-blocking: globals interpolate + and unfilled per-user ${NAME} refs are left untouched.""" + from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( + MCPServerManager, + ) + + manager = MCPServerManager() + + async def boom(server, user_api_key_auth): + raise RuntimeError("db down") + + monkeypatch.setattr(manager, "_load_user_env_vars", boom) + + headers = await manager._resolve_static_headers_with_env_vars( + mock_server, user_api_key_auth=object(), raise_on_missing=False + ) + assert headers == { + "X-DB-URL": "postgres://${CORP_USERNAME}:${CORP_PASSWORD}@db.local/db", + "X-Other": "literal", + } + + @pytest.mark.asyncio async def test_resolve_static_headers_passthrough_when_no_env_vars(): """Servers without env_vars should keep static_headers untouched.""" diff --git a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py index 6fcbac562ff..1162cf9c978 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py @@ -3319,6 +3319,39 @@ def test_sanitize_mcp_server_for_non_admin_clears_credential_fields(): assert sanitized.alias == server.alias +@pytest.mark.parametrize( + "sanitizer_name", + ["_sanitize_mcp_server_for_non_admin", "_sanitize_mcp_server_for_virtual_key"], +) +def test_sanitize_masks_global_env_var_secrets(sanitizer_name): + """Non-admin and virtual-key views must never expose the admin-supplied + global env var secret, while per-user placeholders are left intact.""" + import litellm.proxy.management_endpoints.mcp_management_endpoints as mgmt + + sanitizer = getattr(mgmt, sanitizer_name) + + base = generate_mock_mcp_server_db_record() + server = LiteLLM_MCPServerTable( + **{ + **base.model_dump(), + "env_vars": [ + {"name": "ADMIN_API_KEY", "value": "super-secret", "scope": "global"}, + {"name": "USER_TOKEN", "value": "placeholder-hint", "scope": "user"}, + ], + } + ) + + sanitized = sanitizer(server) + + by_name = {ev.name: ev for ev in sanitized.env_vars} + assert by_name["ADMIN_API_KEY"].value == "" + assert by_name["USER_TOKEN"].value == "placeholder-hint" + + # The original object must not be mutated. + original_by_name = {ev.name: ev for ev in server.env_vars} + assert original_by_name["ADMIN_API_KEY"].value == "super-secret" + + def _make_env_var_server( *, server_id: str = "srv-1",