diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py index e27f0c44c20..c7c8b0cbef7 100644 --- a/litellm/proxy/_experimental/mcp_server/db.py +++ b/litellm/proxy/_experimental/mcp_server/db.py @@ -55,8 +55,10 @@ def decrypt_global_env_var_values(env_vars: Optional[Iterable[Any]]) -> None: """Decrypt ``scope="global"`` env var values in place after reading the DB. Accepts ``MCPEnvVar`` models (``LiteLLM_MCPServerTable``) or plain dicts - (raw rows / deserialized JSON). Decryption is best-effort: a value that no - longer decrypts (e.g. after a salt-key change) is left untouched. + (raw rows / deserialized JSON). Global values are always stored encrypted, + so a value that no longer decrypts (e.g. after a salt-key change) is dropped + and a warning is logged rather than forwarding the ciphertext into upstream + ``${NAME}`` headers, where it would silently fail. """ if not env_vars: return @@ -72,8 +74,16 @@ def decrypt_global_env_var_values(env_vars: Optional[Iterable[Any]]) -> None: value=value, key="mcp_global_env_var", exception_type="debug", - return_original_value=True, + return_original_value=False, ) + if decrypted is None: + name = entry.get("name") if is_dict else getattr(entry, "name", None) + verbose_proxy_logger.warning( + "MCP global env var %s failed to decrypt (LITELLM_SALT_KEY " + "changed?); dropping it so ciphertext is not sent upstream", + name, + ) + decrypted = "" if is_dict: entry["value"] = decrypted else: diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 66587c4bdbc..1c412be339b 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -2275,9 +2275,10 @@ if MCP_AVAILABLE: "/server/{server_id}/user-env-vars", description=( "Store the calling user's per-user MCP env var values for this " - "server. This fully replaces any previously stored values: a " - "variable omitted from the request (or sent empty) is cleared, " - "not preserved. Send the complete set you want retained." + "server. Submitted values are merged over any previously stored " + "values, so you only send the fields you want to set or change; a " + "variable omitted (or sent empty) keeps its stored value. Use " + "DELETE to clear all stored values." ), dependencies=[Depends(user_api_key_auth)], response_model=MCPUserEnvVarsStatus, @@ -2304,20 +2305,27 @@ if MCP_AVAILABLE: status_code=status.HTTP_404_NOT_FOUND, detail={"error": f"MCP Server {server_id} not found"}, ) - # Filter to only known per-user var names declared by the admin — - # never persist arbitrary keys the user invents. + # Only known per-user var names declared by the admin are accepted — + # never persist arbitrary keys the user invents. Submitted values are + # merged over the existing set so a user updating one credential does + # not have to re-enter the others (which are write-only and never shown + # back); an omitted/empty field keeps its stored value. _, user_specs = parse_admin_env_vars(getattr(server, "env_vars", None)) allowed_names = {spec["name"] for spec in user_specs} - filtered = { + updates = { k: v for k, v in payload.values.items() if k in allowed_names and v != "" } - await store_user_env_vars(prisma_client, user_id, server_id, filtered) + existing = await get_user_env_vars(prisma_client, user_id, server_id) + merged = { + k: v for k, v in {**existing, **updates}.items() if k in allowed_names + } + await store_user_env_vars(prisma_client, user_id, server_id, merged) from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( invalidate_user_env_vars_cache, ) invalidate_user_env_vars_cache(user_id, server_id) - return _compute_user_env_var_status(server=server, stored_values=filtered) + return _compute_user_env_var_status(server=server, stored_values=merged) @router.delete( "/server/{server_id}/user-env-vars", diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py index fadb6f7380e..20854621b91 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_env_vars.py @@ -889,6 +889,42 @@ async def test_build_mcp_server_from_table_decrypts_global_env_vars(env_vars_sal assert headers == {"X-Db": "s3cr3t-p@ss"} +def test_decrypt_global_env_var_drops_undecryptable_value( + env_vars_salt_key, monkeypatch +): + """A global value encrypted under a previous salt key must be dropped (not + forwarded as ciphertext) and surfaced as a warning, so a rotated + ``LITELLM_SALT_KEY`` can't silently leak ciphertext into ``${NAME}`` headers.""" + import json + from unittest.mock import MagicMock + + import litellm.proxy._experimental.mcp_server.db as mcp_db + from litellm.proxy._experimental.mcp_server.db import ( + _prepare_mcp_server_data, + decrypt_global_env_var_values, + ) + from litellm.proxy._types import MCPEnvVar + + req = _global_env_var_server_request( + [MCPEnvVar(name="DB_PASSWORD", value="s3cr3t-p@ss", scope="global")] + ) + entries = json.loads(_prepare_mcp_server_data(req)["env_vars"]) + ciphertext = entries[0]["value"] + assert ciphertext != "s3cr3t-p@ss" # encrypted under the original salt key + + # Rotate the salt key so the stored ciphertext no longer decrypts. + monkeypatch.setenv("LITELLM_SALT_KEY", "a-totally-different-salt-key-0000") + logger = MagicMock() + monkeypatch.setattr(mcp_db, "verbose_proxy_logger", logger) + + decrypt_global_env_var_values(entries) + + assert entries[0]["value"] == "" + assert ciphertext not in json.dumps(entries) + logger.warning.assert_called_once() + assert "DB_PASSWORD" in logger.warning.call_args.args + + # ── REST exception handling ─────────────────────────────────────────────── 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 6e3e0f913a8..873995cba8a 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 @@ -3910,6 +3910,9 @@ class TestStoreMCPUserEnvVars: patch.object( mgmt_endpoints, "get_mcp_server", AsyncMock(return_value=server) ), + patch.object( + mgmt_endpoints, "get_user_env_vars", AsyncMock(return_value={}) + ), patch.object(mgmt_endpoints, "store_user_env_vars", store_mock), ): result = await mgmt_endpoints.store_mcp_user_env_vars( @@ -3930,6 +3933,47 @@ class TestStoreMCPUserEnvVars: # CORP_PASSWORD remains unset in the returned status. assert result.missing_count == 1 + @pytest.mark.asyncio + async def test_merges_over_existing_values(self): + """Updating one credential must not wipe other already-stored values. + + The user updates only CORP_PASSWORD; their previously-stored + CORP_USERNAME (write-only, never shown back in the form) must be + preserved instead of being cleared. + """ + server = _make_env_var_server( + env_vars=_ENV_VARS_MIXED, static_headers=_STATIC_HEADERS_MIXED + ) + store_mock = AsyncMock() + with ( + patch.object( + mgmt_endpoints, "get_prisma_client_or_throw", return_value=MagicMock() + ), + patch.object( + mgmt_endpoints, "get_mcp_server", AsyncMock(return_value=server) + ), + patch.object( + mgmt_endpoints, + "get_user_env_vars", + AsyncMock( + return_value={"CORP_USERNAME": "alice", "CORP_PASSWORD": "old"} + ), + ), + patch.object(mgmt_endpoints, "store_user_env_vars", store_mock), + ): + result = await mgmt_endpoints.store_mcp_user_env_vars( + server_id="srv-1", + payload=mgmt_endpoints.MCPUserEnvVarsRequest( + values={"CORP_PASSWORD": "new"} + ), + user_api_key_dict=generate_mock_user_api_key_auth(user_id="alice"), + ) + store_mock.assert_awaited_once() + _, _, _, persisted = store_mock.await_args.args + assert persisted == {"CORP_USERNAME": "alice", "CORP_PASSWORD": "new"} + # Both credentials report as set in the returned status. + assert result.missing_count == 0 + @pytest.mark.asyncio async def test_missing_user_id_raises_400(self): with patch.object( diff --git a/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx b/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx index f9cb3f9318b..ecc1459a50e 100644 --- a/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx +++ b/ui/litellm-dashboard/src/components/mcp_tools/UserEnvVarsModal.tsx @@ -125,7 +125,8 @@ const UserEnvVarsModal: React.FC = ({ These values are private to you. Your admin configured this MCP server to require these per-user credentials. Saved values are - never shown back; re-enter a value to update it. + never shown back; leave an already-set field blank to keep it, or + enter a value to set or change it.
= ({ } extra={spec.description || undefined} - rules={[{ required: true, message: `${spec.name} is required` }]} + rules={ + spec.is_set + ? undefined + : [{ required: true, message: `${spec.name} is required` }] + } >