diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 6ad731e7113..f510dc16ad1 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -806,6 +806,13 @@ class MCPServerManager: self.initialize_tool_name_to_mcp_server_name_mapping() async def add_server(self, mcp_server: LiteLLM_MCPServerTable): + # The runtime registry is the allowlist for tool calls and health + # probes (which spawn the underlying transport, including stdio + # subprocesses). Match the eligibility set used by the bulk DB + # filter in reload_servers_from_database() — NULL is legacy and + # "approved" is a legacy alias for "active". + if mcp_server.approval_status not in (None, "active", "approved"): + return try: if mcp_server.server_id not in self.registry: new_server = await self.build_mcp_server_from_table(mcp_server) @@ -819,6 +826,13 @@ class MCPServerManager: raise e async def update_server(self, mcp_server: LiteLLM_MCPServerTable): + # If a previously-active server has been moved out of the active + # state, evict any stale registry entry so subsequent tool calls and + # health probes can't reach it. + if mcp_server.approval_status not in (None, "active", "approved"): + if mcp_server.server_id in self.registry: + del self.registry[mcp_server.server_id] + return try: if mcp_server.server_id in self.registry: new_server = await self.build_mcp_server_from_table(mcp_server) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 7bda0f87ccd..f2e64fdde83 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -142,6 +142,7 @@ if MCP_AVAILABLE: MCPOAuthUserCredentialRequest, MCPOAuthUserCredentialStatus, MCPSubmissionsSummary, + MCPTransport, MCPUserCredentialListItem, MCPUserCredentialRequest, MCPUserCredentialResponse, @@ -1070,6 +1071,24 @@ if MCP_AVAILABLE: }, ) + # stdio servers spawn a local subprocess on the proxy host with the + # configured command + args, so accepting them from non-admin callers + # would let a team member propose a server config that an admin could + # rubber-stamp into local code execution. Restrict stdio submission to + # the admin POST /v1/mcp/server path or to config.yaml. + if payload.transport == MCPTransport.stdio: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "error": ( + "stdio MCP servers cannot be submitted via the user " + "registration workflow. Ask a proxy admin to add this " + "server via POST /v1/mcp/server or to declare it in " + "config.yaml." + ) + }, + ) + prisma_client = get_prisma_client_or_throw( "Database not connected. Connect a database to your proxy" ) diff --git a/tests/mcp_tests/test_mcp_server.py b/tests/mcp_tests/test_mcp_server.py index 6af07585796..409f4fad99a 100644 --- a/tests/mcp_tests/test_mcp_server.py +++ b/tests/mcp_tests/test_mcp_server.py @@ -1494,6 +1494,7 @@ async def test_add_update_server_with_alias(): mock_mcp_server.created_at = None mock_mcp_server.updated_at = None mock_mcp_server.instructions = None + mock_mcp_server.approval_status = "active" # Add server to manager await test_manager.add_server(mock_mcp_server) @@ -1551,6 +1552,7 @@ async def test_add_update_server_without_alias(): mock_mcp_server.created_at = None mock_mcp_server.updated_at = None mock_mcp_server.instructions = None + mock_mcp_server.approval_status = "active" # Add server to manager await test_manager.add_server(mock_mcp_server) @@ -1609,6 +1611,7 @@ async def test_add_update_server_fallback_to_server_id(): mock_mcp_server.created_at = None mock_mcp_server.updated_at = None mock_mcp_server.instructions = None + mock_mcp_server.approval_status = "active" # Add server to manager await test_manager.add_server(mock_mcp_server) 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 11e9dbbdd57..c862de35c7e 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 @@ -29,7 +29,11 @@ from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( MCPServerManager, _deserialize_json_dict, ) -from litellm.proxy._types import LiteLLM_MCPServerTable, MCPTransport +from litellm.proxy._types import ( + LiteLLM_MCPServerTable, + MCPApprovalStatus, + MCPTransport, +) from litellm.types.mcp import MCPAuth from litellm.types.mcp_server.mcp_server_manager import MCPOAuthMetadata, MCPServer @@ -3311,5 +3315,82 @@ class TestOAuthDiscoverySSRFGuard: mock_client.get.assert_not_called() +class TestApprovalStatusGate: + """ + Regression tests for GHSA-gm4g-h72v-jhc3. + + The runtime registry must only contain servers an admin has approved. + A non-admin can submit a pending stdio MCP server with an attacker-chosen + command/args; before this gate, an admin opening the per-row endpoint + triggered ``add_server`` + ``health_check_server``, which spawned the + attacker's process under the proxy. The data-layer gate in + ``add_server`` / ``update_server`` blocks pending and rejected rows + from entering the registry regardless of which caller passes them in. + """ + + def _make_server(self, server_id: str, approval_status): + return LiteLLM_MCPServerTable( + server_id=server_id, + alias=f"server_{server_id}", + description="test", + url=None, + transport=MCPTransport.stdio, + command="python", + args=["-c", "print('attacker payload')"], + env={}, + approval_status=approval_status, + created_at=datetime.now(), + updated_at=datetime.now(), + ) + + @pytest.mark.parametrize( + "approval_status,expect_in_registry", + [ + (MCPApprovalStatus.pending_review, False), + (MCPApprovalStatus.rejected, False), + (MCPApprovalStatus.active, True), + # Legacy rows: NULL predates the approval workflow; "approved" is + # a legacy alias for "active" still present in older deployments. + # Both must continue to load to match the DB-level filter in + # reload_servers_from_database(). + (None, True), + ("approved", True), + ], + ) + async def test_add_server_respects_approval_status( + self, approval_status, expect_in_registry + ): + manager = MCPServerManager() + server_id = f"sid-{approval_status}" + await manager.add_server(self._make_server(server_id, approval_status)) + assert (server_id in manager.registry) is expect_in_registry + + async def test_update_server_evicts_when_transitioned_away_from_active(self): + # An admin updates a previously-active server to rejected (or pending). + # The stale registry entry must be evicted so subsequent tool calls + # and health probes can't reach it. + manager = MCPServerManager() + await manager.add_server( + self._make_server("evict-me", MCPApprovalStatus.active) + ) + assert "evict-me" in manager.registry + + await manager.update_server( + self._make_server("evict-me", MCPApprovalStatus.rejected) + ) + assert "evict-me" not in manager.registry + + async def test_update_server_noop_for_unregistered_pending(self): + # update_server called with a pending row that was never registered + # should silently return without adding it. Locks in the early-return + # so a future refactor can't accidentally route the pending row to + # build_mcp_server_from_table. + manager = MCPServerManager() + await manager.update_server( + self._make_server("never-seen", MCPApprovalStatus.pending_review) + ) + assert "never-seen" not in manager.registry + + if __name__ == "__main__": pytest.main([__file__]) 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 f0909afcbf6..30ad84e18b8 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 @@ -2491,6 +2491,32 @@ class TestMCPApprovalWorkflow: assert exc_info.value.status_code == 400 assert "team" in str(exc_info.value.detail).lower() + @pytest.mark.asyncio + async def test_register_mcp_server_rejects_stdio_transport(self): + # stdio servers spawn a local subprocess on the proxy host. Accepting + # them from the non-admin submission endpoint would let a team member + # propose a config that an admin could rubber-stamp into local code + # execution. Admins use POST /v1/mcp/server or config.yaml instead. + from litellm.proxy.management_endpoints.mcp_management_endpoints import ( + register_mcp_server, + ) + + payload = NewMCPServerRequest( + alias="local", + transport=MCPTransport.stdio, + command="python3", + args=["-m", "mcp_server_filesystem", "/tmp"], + ) + user_auth = generate_mock_user_api_key_auth( + user_role=LitellmUserRoles.INTERNAL_USER, + team_id="team-123", + user_id="user-abc", + ) + with pytest.raises(HTTPException) as exc_info: + await register_mcp_server(payload=payload, user_api_key_dict=user_auth) + assert exc_info.value.status_code == 400 + assert "stdio" in str(exc_info.value.detail).lower() + @pytest.mark.asyncio async def test_register_mcp_server_sets_pending_review(self): from litellm.proxy._types import MCPApprovalStatus