mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-03 02:22:24 +00:00
fix(mcp): surface DB errors on tool-call path and redact global env var secrets
A transient DB failure while loading per-user env vars on the tool-call path was swallowed and turned into a misleading MCPMissingUserEnvVarsError (412 'set up your credentials'). _load_user_env_vars now propagates DB errors; the resolver keeps them non-blocking only on the listing path. Global-scope env var values hold admin-supplied plaintext secrets. They were returned verbatim inside LiteLLM_MCPServerTable.env_vars to non-admin and virtual-key callers via the server list/detail endpoints. Both sanitizers now blank global-scope values while leaving per-user placeholders intact.
This commit is contained in:
parent
7d11dd7def
commit
d781b62112
4 changed files with 117 additions and 16 deletions
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue