From 26f15b420ce31318ec55fb2b45395eea040d1953 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Wed, 3 Jun 2026 18:20:47 +0000 Subject: [PATCH] fix(mcp): stop echoing credential values in bulk env-var status and let admins describe per-user vars The bulk /user-env-vars/status feed only drives the dashboard "N fields missing" badge, which needs is_set, so it no longer returns the stored credential values; the single-server endpoint still returns them for the fill-in modal to pre-populate. Adds a description input to the admin env-var form for per-user scope so admins can tell users what to enter; the per-user modal already surfaces that description as a hint. --- .../mcp_management_endpoints.py | 12 +++-- .../test_mcp_management_endpoints.py | 37 ++++++++++++++ .../components/mcp_tools/EnvVarsSection.tsx | 51 +++++++++---------- 3 files changed, 71 insertions(+), 29 deletions(-) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 61ff14a92fc..a97cfc7ef05 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -2163,8 +2163,14 @@ if MCP_AVAILABLE: *, server: LiteLLM_MCPServerTable, stored_values: Dict[str, str], + include_values: bool = True, ) -> MCPUserEnvVarsStatus: - """Build a status object for one server given the user's stored values.""" + """Build a status object for one server given the user's stored values. + + ``include_values=False`` omits the stored credential values from the + response (the bulk status feed only needs ``is_set`` for the dashboard + badge, so there's no reason to echo secrets back across every server). + """ _, user_specs = parse_admin_env_vars(getattr(server, "env_vars", None)) # Limit "required" to vars that are actually referenced by static_headers. @@ -2193,7 +2199,7 @@ if MCP_AVAILABLE: MCPUserEnvVarSpec( name=name, description=spec.get("description"), - value=value, + value=value if include_values else None, is_set=is_set, ) ) @@ -2333,7 +2339,7 @@ if MCP_AVAILABLE: for server in accessible: stored = stored_bulk.get(server.server_id, {}) status_obj = _compute_user_env_var_status( - server=server, stored_values=stored + server=server, stored_values=stored, include_values=False ) if status_obj.required: statuses.append(status_obj) 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 be8c72cb136..f5a49f0d293 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 @@ -3491,6 +3491,10 @@ class TestGetMCPUserEnvVars: assert result.server_id == "srv-1" assert result.missing_count == 1 assert {s.name for s in result.required} == {"CORP_USERNAME", "CORP_PASSWORD"} + # The single-server endpoint backs the fill-in modal, so it must echo the + # already-stored value back for pre-population. + by_name = {s.name: s for s in result.required} + assert by_name["CORP_USERNAME"].value == "alice" @pytest.mark.asyncio async def test_missing_user_id_raises_400(self): @@ -3730,6 +3734,39 @@ class TestListMCPUserEnvVarStatus: assert [s.server_id for s in result] == ["srv-with"] assert result[0].missing_count == 1 + @pytest.mark.asyncio + async def test_bulk_status_omits_stored_credential_values(self): + """The bulk feed only drives the "fields missing" badge, so it must not + echo stored credential values back; is_set still reflects presence.""" + 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_all_mcp_servers_for_user", + AsyncMock(return_value=[server]), + ), + patch.object( + mgmt_endpoints, + "get_user_env_vars_bulk", + AsyncMock(return_value={"srv-with": {"CORP_USERNAME": "alice"}}), + ), + ): + result = await mgmt_endpoints.list_mcp_user_env_var_status( + user_api_key_dict=generate_mock_user_api_key_auth(user_id="alice") + ) + by_name = {s.name: s for s in result[0].required} + assert by_name["CORP_USERNAME"].is_set is True + assert by_name["CORP_USERNAME"].value is None + assert by_name["CORP_PASSWORD"].is_set is False + assert by_name["CORP_PASSWORD"].value is None + class TestMCPUserEnvVarsAccessControl: """Per-server env-var endpoints must enforce the same access gate as diff --git a/ui/litellm-dashboard/src/components/mcp_tools/EnvVarsSection.tsx b/ui/litellm-dashboard/src/components/mcp_tools/EnvVarsSection.tsx index b666d9676a4..d571017eb94 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/EnvVarsSection.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/EnvVarsSection.tsx @@ -59,7 +59,7 @@ const EnvVarsSection: React.FC = () => { {fields.length > 0 && (
Variable Name
-
Value
+
Value / Description
Scope
@@ -84,15 +84,9 @@ const EnvVarsSection: React.FC = () => { className="rounded-md font-mono" /> - - - +
+ +
{ ); }; -// Disables the value field when scope=user (those values come from each -// user later), keeping the column visible so the row layout stays consistent. -const ValueField: React.FC<{ - fieldName: number; - value?: string; - onChange?: (v: string) => void; -}> = ({ fieldName, value, onChange }) => { - const scope = Form.useWatch(["env_vars", fieldName, "scope"]); - const isPerUser = scope === "user"; +// For instance-scoped vars this column holds the admin value. For per-user +// vars the value comes from each user later, so the column instead captures an +// optional description that the per-user fill-in modal shows as a hint. +const ScopedValueOrDescription: React.FC<{ + name: number; + restField: object; +}> = ({ name, restField }) => { + const isPerUser = Form.useWatch(["env_vars", name, "scope"]) === "user"; + if (isPerUser) { + return ( + + + + ); + } return ( - onChange?.(e.target.value)} - placeholder={isPerUser ? "Defined per user" : "e.g. postgresql"} - disabled={isPerUser} - className="rounded-md font-mono" - /> + + + ); };