fix(mcp): make per-user env var status write-only

The single-server and bulk per-user env var status endpoints echoed the
decrypted credential value back to any holder of the user's LiteLLM token,
so a leaked token could exfiltrate the raw upstream secret (e.g. a personal
access token) for use outside the proxy. Drop the value field from
MCPUserEnvVarSpec and the include_values plumbing so the status reports only
whether each credential is_set; users overwrite a field to rotate it. The
fill-in modal no longer pre-populates from the secret and flags already-set
fields instead.
This commit is contained in:
mateo-berri 2026-06-03 20:40:16 +00:00
parent 1f1bf4c421
commit e9eeca1bb7
No known key found for this signature in database
5 changed files with 30 additions and 24 deletions

View file

@ -1552,11 +1552,14 @@ class MCPUserEnvVarsRequest(LiteLLMPydanticObjectBase):
class MCPUserEnvVarSpec(LiteLLMPydanticObjectBase):
"""Describes one per-user env var slot for the calling user."""
"""Describes one per-user env var slot for the calling user.
Stored values are write-only: the status only reports whether a value
``is_set`` and never echoes the decrypted secret back to the client.
"""
name: str
description: Optional[str] = None
value: Optional[str] = None # current value if the user has set one
is_set: bool = False

View file

@ -2178,13 +2178,12 @@ 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.
``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).
Stored credentials are write-only: the response reports only whether
each value ``is_set`` and never echoes the decrypted secret back, so a
leaked token can't be used to exfiltrate the raw upstream credential.
"""
_, user_specs = parse_admin_env_vars(getattr(server, "env_vars", None))
@ -2214,7 +2213,6 @@ if MCP_AVAILABLE:
MCPUserEnvVarSpec(
name=name,
description=spec.get("description"),
value=value if include_values else None,
is_set=is_set,
)
)
@ -2354,7 +2352,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, include_values=False
server=server, stored_values=stored
)
if status_obj.required:
statuses.append(status_obj)

View file

@ -3613,9 +3613,10 @@ class TestComputeUserEnvVarStatus:
assert names == {"CORP_USERNAME", "CORP_PASSWORD"}
by_name = {spec.name: spec for spec in status.required}
assert by_name["CORP_USERNAME"].is_set is True
assert by_name["CORP_USERNAME"].value == "alice"
assert by_name["CORP_USERNAME"].description == "Your username"
assert by_name["CORP_PASSWORD"].is_set is False
# Stored credentials are write-only: the secret is never echoed back.
assert "alice" not in status.model_dump_json()
assert status.missing_count == 1
assert status.server_id == "srv-1"
assert status.server_name == "DB Server"
@ -3696,10 +3697,12 @@ 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.
# The single-server endpoint reports which credentials are set without
# ever echoing the decrypted secret back to the caller.
by_name = {s.name: s for s in result.required}
assert by_name["CORP_USERNAME"].value == "alice"
assert by_name["CORP_USERNAME"].is_set is True
assert by_name["CORP_PASSWORD"].is_set is False
assert "alice" not in result.model_dump_json()
@pytest.mark.asyncio
async def test_missing_user_id_raises_400(self):
@ -3968,9 +3971,8 @@ class TestListMCPUserEnvVarStatus:
)
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
assert "alice" not in result[0].model_dump_json()
class TestMCPUserEnvVarsAccessControl:

View file

@ -47,11 +47,7 @@ const UserEnvVarsModal: React.FC<UserEnvVarsModalProps> = ({
const fetched = await getMCPUserEnvVars(accessToken, server.server_id);
if (cancelled) return;
setStatus(fetched);
const initial: Record<string, string> = {};
for (const spec of fetched?.required ?? []) {
initial[spec.name] = spec.value ?? "";
}
form.setFieldsValue(initial);
form.resetFields();
} catch (err) {
if (!cancelled) {
NotificationsManager.fromBackend(
@ -128,7 +124,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:
server to require these per-user credentials. Saved values are
never shown back; re-enter a value to update it.
</Text>
<Form
form={form}
@ -141,15 +138,22 @@ const UserEnvVarsModal: React.FC<UserEnvVarsModalProps> = ({
key={spec.name}
name={spec.name}
label={
<span className="font-mono text-sm font-semibold">
{spec.name}
<span className="flex items-center gap-2">
<span className="font-mono text-sm font-semibold">
{spec.name}
</span>
{spec.is_set && <Tag color="green">Set</Tag>}
</span>
}
extra={spec.description || undefined}
rules={[{ required: true, message: `${spec.name} is required` }]}
>
<Input.Password
placeholder={spec.description || `Enter your ${spec.name}`}
placeholder={
spec.is_set
? "Enter a new value to overwrite"
: spec.description || `Enter your ${spec.name}`
}
visibilityToggle
/>
</Form.Item>

View file

@ -261,7 +261,6 @@ export interface MCPEnvVar {
export interface MCPUserEnvVarSpec {
name: string;
description?: string | null;
value?: string | null;
is_set: boolean;
}