From ca287c1b1590194546a01f6eb50be7256cd54e39 Mon Sep 17 00:00:00 2001 From: Yucheng He Date: Fri, 18 Sep 2026 14:55:31 -0700 Subject: [PATCH] fix(mcp): preserve restricted admin submission fields --- .../mcp_management_endpoints.py | 2 - .../test_mcp_management_endpoints.py | 72 ++++++++++--------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 6ed131417d6..c1388e8bb81 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -1488,8 +1488,6 @@ if MCP_AVAILABLE: submissions.items = _redact_mcp_credentials_list(submissions.items) if not _user_is_full_admin(user_api_key_dict): submissions.items = _sanitize_mcp_server_list_for_non_admin(submissions.items) - elif _is_restricted_virtual_key_request(user_api_key_dict): - submissions.items = _sanitize_mcp_server_list_for_virtual_key(submissions.items) return submissions @router.put( 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 79b34ded13e..f3fc45480e1 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 @@ -6,7 +6,7 @@ import logging from contextlib import ExitStack from datetime import datetime, timedelta from types import SimpleNamespace -from typing import Final, List, Optional, cast +from typing import List, Optional, cast from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -4583,17 +4583,20 @@ class TestMCPApprovalWorkflow: assert result.pending_review == 1 @pytest.mark.asyncio - @pytest.mark.parametrize("allowed_routes", [[], ["llm_api_routes"], ["mcp_routes"]]) - async def test_get_submissions_sanitizes_for_view_only_admin(self, allowed_routes: list[str]) -> None: + async def test_get_submissions_sanitizes_for_view_only_admin(self): + """PROXY_ADMIN_VIEW_ONLY reviewing the submission queue must go through + the non-admin sanitizer that fetch/list endpoints use: url, + static_headers, env, env_vars, and credentials are all dropped. A + mutation swapping the gate back to the old partial-blank pattern (which + left url/static_headers/env and env-var names intact) would fail this.""" from litellm.proxy._types import MCPSubmissionsSummary from litellm.proxy.management_endpoints.mcp_management_endpoints import ( get_mcp_server_submissions, ) - item: Final = _leaky_list_server().model_copy( - update={"approval_status": "pending_review", "spec_path": "https://example.com/spec?key=secret"} - ) - summary: Final = MCPSubmissionsSummary(total=1, pending_review=1, active=0, rejected=0, items=[item]) + item = _leaky_list_server() + item.approval_status = "pending_review" + summary = MCPSubmissionsSummary(total=1, pending_review=1, active=0, rejected=0, items=[item]) with ( patch( @@ -4605,15 +4608,12 @@ class TestMCPApprovalWorkflow: AsyncMock(return_value=summary), ), ): - result: Final = await get_mcp_server_submissions( - user_api_key_dict=UserAPIKeyAuth( - user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY, allowed_routes=allowed_routes - ), + result = await get_mcp_server_submissions( + user_api_key_dict=generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY), ) assert len(result.items) == 1 - sanitized: Final = result.items[0] - assert sanitized.spec_path is None + sanitized = result.items[0] assert sanitized.url is None assert sanitized.static_headers is None assert sanitized.env == {} @@ -4625,17 +4625,18 @@ class TestMCPApprovalWorkflow: assert item.static_headers == {"Authorization": "Bearer sk-secret-header"} @pytest.mark.asyncio - @pytest.mark.parametrize("allowed_routes", [[], ["llm_api_routes"], ["mcp_routes"]]) - async def test_get_submissions_respects_admin_key_route_restrictions(self, allowed_routes: list[str]) -> None: + async def test_get_submissions_full_admin_still_sees_secrets(self): + """The view-only redaction must not over-redact for a full PROXY_ADMIN, + who needs url/static_headers/env/env_vars to review the pending + submission. Only the explicit credentials field is cleared.""" from litellm.proxy._types import MCPSubmissionsSummary - - credentials: Final[MCPCredentials] = {"scopes": ["scope:review"], "client_secret": "secret-sentinel"} - item: Final = _leaky_list_server().model_copy( - update={"approval_status": "pending_review", "credentials": credentials} + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + get_mcp_server_submissions, ) - original: Final = item.model_dump() - summary: Final = MCPSubmissionsSummary(total=1, pending_review=1, active=0, rejected=0, items=[item]) - admin: Final = UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN, allowed_routes=allowed_routes) + + item = _leaky_list_server() + item.approval_status = "pending_review" + summary = MCPSubmissionsSummary(total=1, pending_review=1, active=0, rejected=0, items=[item]) with ( patch( @@ -4647,18 +4648,25 @@ class TestMCPApprovalWorkflow: AsyncMock(return_value=summary), ), ): - result: Final = await mgmt_endpoints.get_mcp_server_submissions(user_api_key_dict=admin) + result = await get_mcp_server_submissions( + user_api_key_dict=generate_mock_user_api_key_auth(user_role=LitellmUserRoles.PROXY_ADMIN), + ) - assert (result.total, result.pending_review, result.active, result.rejected) == (1, 1, 0, 0) assert len(result.items) == 1 - returned: Final = result.items[0] - assert returned.server_id == item.server_id - assert returned.credentials == (None if allowed_routes else {"scopes": credentials["scopes"]}) - assert returned.url == (None if allowed_routes else item.url) - assert returned.static_headers == (None if allowed_routes else item.static_headers) - assert returned.env == ({} if allowed_routes else item.env) - assert returned.env_vars == (None if allowed_routes else item.env_vars) - assert item.model_dump() == original + raw = result.items[0] + assert raw.url == "https://leaky.example.com/mcp?api_key=sk-embedded-in-url" + assert raw.static_headers == {"Authorization": "Bearer sk-secret-header"} + assert raw.env == {"UPSTREAM_TOKEN": "sk-secret-env"} + assert raw.credentials is None + assert raw.env_vars is not None + assert len(raw.env_vars) == 1 + # ``model_construct`` in ``_leaky_list_server`` skips validation, so + # env_vars stays as raw dicts; mirror the fixture shape here. + entry = raw.env_vars[0] + name = entry["name"] if isinstance(entry, dict) else entry.name + value = entry["value"] if isinstance(entry, dict) else entry.value + assert name == "GLOBAL_KEY" + assert value == "super-secret" @pytest.mark.asyncio async def test_approve_non_pending_server_raises_400(self):