fix(mcp): preserve restricted admin submission fields
Some checks failed
LiteLLM Rust / rust-lint (push) Has been cancelled
LiteLLM Rust / rust-test (push) Has been cancelled
LiteLLM Rust / rust-wheel (push) Has been cancelled
Terraform Modules / fmt, validate, test (aws) (push) Has been cancelled
Terraform Modules / fmt, validate, test (gcp) (push) Has been cancelled
Terraform Provider / gofmt, vet, build, test (push) Has been cancelled
Terraform Provider / Provider endpoints vs proxy OpenAPI schema (push) Has been cancelled

This commit is contained in:
Yucheng He 2026-09-18 14:55:31 -07:00
parent bbbd03089d
commit ca287c1b15
2 changed files with 40 additions and 34 deletions

View file

@ -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(

View file

@ -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):