From aeb55e7a11e98c6f4a88b6018828fb10491080d3 Mon Sep 17 00:00:00 2001 From: Mateo Wang <277851410+mateo-berri@users.noreply.github.com> Date: Sat, 6 Jun 2026 16:51:25 -0700 Subject: [PATCH] fix(mcp): highlight MCP cards red when the logged-in user is missing per-user env vars (#29856) * fix(mcp): flag missing per-user env vars on the card for every accessible server The dashboard MCP card grid lists servers via the registry-backed manager (get_all_mcp_servers_unfiltered for admins in view_all mode, the allowed-context aggregation otherwise), but the per-user env-var status endpoint that drives the red "user fields missing" highlight resolved servers through the much narrower get_all_mcp_servers_for_user, which only returns servers explicitly granted on the calling key. An admin's dashboard session key carries no per-server MCP grant, so the status feed came back empty and the card never turned red even when the logged-in user had not filled in their required variables. Both surfaces now share a single _resolve_accessible_mcp_servers helper, so the status feed is computed over exactly the cards the user sees. The helper returns servers unredacted; the status endpoint needs the raw env_vars and still only ever reports is_set booleans, never the stored secret values. * test(mcp): drop dead get_all_mcp_servers_for_user patch from view_all regression test The bulk status endpoint resolves servers through _resolve_accessible_mcp_servers now, so the old get_all_mcp_servers_for_user patch in the admin view_all regression test is never hit. Removing it keeps the test honest about which code path it exercises. --- .../mcp_management_endpoints.py | 56 ++++++++++--------- .../test_mcp_management_endpoints.py | 51 ++++++++++++++++- 2 files changed, 77 insertions(+), 30 deletions(-) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index f1edcc9c7b..64e89ee40f 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -879,6 +879,32 @@ if MCP_AVAILABLE: return _redact_mcp_credentials_list(servers) + async def _resolve_accessible_mcp_servers( + user_api_key_dict: UserAPIKeyAuth, + ) -> List[LiteLLM_MCPServerTable]: + """The server set the dashboard grid shows (GET /v1/mcp/server, no team + filter), returned unredacted. Callers that surface this to a client must + apply their own redaction; the per-user env-var status endpoint relies on + the raw env_vars and only ever returns is_set booleans, never secrets. + + Sharing this resolution keeps the red "missing user fields" card status + aligned with the cards actually rendered: an admin in view_all mode sees + every server even when their key carries no per-server MCP grant. + """ + if ( + _get_user_mcp_management_mode() == "view_all" + and not _is_restricted_virtual_key_request(user_api_key_dict) + ): + return await global_mcp_server_manager.get_all_mcp_servers_unfiltered() + + aggregated: Dict[str, LiteLLM_MCPServerTable] = {} + for auth_context in await build_effective_auth_contexts(user_api_key_dict): + for server in await global_mcp_server_manager.get_all_allowed_mcp_servers( + user_api_key_auth=auth_context + ): + aggregated.setdefault(server.server_id, server) + return list(aggregated.values()) + @router.get( "/server", description="Returns the mcp server list with associated teams", @@ -950,30 +976,8 @@ if MCP_AVAILABLE: sanitized_team_id ) else: - user_mcp_management_mode = _get_user_mcp_management_mode() - - if user_mcp_management_mode == "view_all" and not is_restricted_virtual_key: - servers = ( - await global_mcp_server_manager.get_all_mcp_servers_unfiltered() - ) - redacted_mcp_servers = _redact_mcp_credentials_list(servers) - else: - auth_contexts = await build_effective_auth_contexts(user_api_key_dict) - - aggregated_servers: Dict[str, LiteLLM_MCPServerTable] = {} - for auth_context in auth_contexts: - servers = ( - await global_mcp_server_manager.get_all_allowed_mcp_servers( - user_api_key_auth=auth_context - ) - ) - for server in servers: - if server.server_id not in aggregated_servers: - aggregated_servers[server.server_id] = server - - redacted_mcp_servers = _redact_mcp_credentials_list( - aggregated_servers.values() - ) + servers = await _resolve_accessible_mcp_servers(user_api_key_dict) + redacted_mcp_servers = _redact_mcp_credentials_list(servers) # augment the mcp servers with public status if litellm.public_mcp_servers is not None: @@ -2391,9 +2395,7 @@ if MCP_AVAILABLE: user_id = user_api_key_dict.user_id or "" if not user_id: return [] - accessible = await get_all_mcp_servers_for_user( - prisma_client, user_api_key_dict - ) + accessible = await _resolve_accessible_mcp_servers(user_api_key_dict) if not accessible: return [] server_ids = [s.server_id for s in accessible] 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 b5eb091bb8..947b39e836 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 @@ -4146,7 +4146,7 @@ class TestListMCPUserEnvVarStatus: ), patch.object( mgmt_endpoints, - "get_all_mcp_servers_for_user", + "_resolve_accessible_mcp_servers", AsyncMock(return_value=[]), ), ): @@ -4174,7 +4174,7 @@ class TestListMCPUserEnvVarStatus: ), patch.object( mgmt_endpoints, - "get_all_mcp_servers_for_user", + "_resolve_accessible_mcp_servers", AsyncMock(return_value=[server_with, server_without]), ), patch.object( @@ -4204,7 +4204,7 @@ class TestListMCPUserEnvVarStatus: ), patch.object( mgmt_endpoints, - "get_all_mcp_servers_for_user", + "_resolve_accessible_mcp_servers", AsyncMock(return_value=[server]), ), patch.object( @@ -4221,6 +4221,51 @@ class TestListMCPUserEnvVarStatus: assert by_name["CORP_PASSWORD"].is_set is False assert "alice" not in result[0].model_dump_json() + @pytest.mark.asyncio + async def test_admin_view_all_flags_missing_fields_without_key_grants(self): + """Regression: the red "user fields missing" card must light up for an + admin in view_all mode even when their key carries no per-server MCP + grant. The bulk status feed has to resolve the same server set the + dashboard grid renders; the old narrow key-scoped listing returned + nothing for such an admin, leaving every card un-highlighted.""" + server = _make_env_var_server( + server_id="srv-with", + env_vars=_ENV_VARS_MIXED, + static_headers=_STATIC_HEADERS_MIXED, + ) + with ( + patch.object( + mgmt_endpoints, "get_prisma_client_or_throw", return_value=MagicMock() + ), + patch.object( + mgmt_endpoints, + "_get_user_mcp_management_mode", + return_value="view_all", + ), + patch.object( + mgmt_endpoints.global_mcp_server_manager, + "get_all_mcp_servers_unfiltered", + AsyncMock(return_value=[server]), + ), + patch.object( + mgmt_endpoints, + "get_user_env_vars_bulk", + AsyncMock(return_value={}), + ), + ): + result = await mgmt_endpoints.list_mcp_user_env_var_status( + user_api_key_dict=generate_mock_user_api_key_auth( + user_id="admin", + user_role=LitellmUserRoles.PROXY_ADMIN, + ) + ) + assert [s.server_id for s in result] == ["srv-with"] + assert result[0].missing_count == 2 + assert {f.name for f in result[0].required} == { + "CORP_USERNAME", + "CORP_PASSWORD", + } + class TestMCPUserEnvVarsAccessControl: """Per-server env-var endpoints must enforce the same access gate as