mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-03 02:22:24 +00:00
fix(mcp): drop undecryptable global env vars and merge per-user env var updates
Global env var values are always stored encrypted, so a value that no longer
decrypts (typically a rotated LITELLM_SALT_KEY) was being forwarded into
upstream ${NAME} headers as ciphertext with only a debug log. Drop the value
and log a warning so the failure surfaces instead of silently sending ciphertext.
Per-user env var stores now merge over the existing values instead of replacing
them. Per-user credentials are write-only and never shown back, so requiring the
full set on every save forced users to re-enter credentials they could not see
just to change one field. Omitting (or sending empty) a field now keeps its
stored value; DELETE still clears everything. The modal no longer marks
already-set fields as required.
This commit is contained in:
parent
f08bc13d67
commit
de72a84366
5 changed files with 116 additions and 13 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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 ───────────────────────────────────────────────
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -125,7 +125,8 @@ const UserEnvVarsModal: React.FC<UserEnvVarsModalProps> = ({
|
|||
<Text className="text-sm text-gray-600 block">
|
||||
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.
|
||||
</Text>
|
||||
<Form
|
||||
form={form}
|
||||
|
|
@ -146,7 +147,11 @@ const UserEnvVarsModal: React.FC<UserEnvVarsModalProps> = ({
|
|||
</span>
|
||||
}
|
||||
extra={spec.description || undefined}
|
||||
rules={[{ required: true, message: `${spec.name} is required` }]}
|
||||
rules={
|
||||
spec.is_set
|
||||
? undefined
|
||||
: [{ required: true, message: `${spec.name} is required` }]
|
||||
}
|
||||
>
|
||||
<Input.Password
|
||||
placeholder={
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue