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.
This commit is contained in:
mateo-berri 2026-06-03 18:20:47 +00:00
parent b38320d29c
commit 26f15b420c
No known key found for this signature in database
3 changed files with 71 additions and 29 deletions

View file

@ -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)

View file

@ -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

View file

@ -59,7 +59,7 @@ const EnvVarsSection: React.FC = () => {
{fields.length > 0 && (
<div className="flex gap-3 px-1 text-xs font-medium text-gray-500 uppercase tracking-wide">
<div style={{ flex: 1 }}>Variable Name</div>
<div style={{ flex: 1 }}>Value</div>
<div style={{ flex: 1 }}>Value / Description</div>
<div style={{ width: 160 }}>Scope</div>
<div style={{ width: 24 }} />
</div>
@ -84,15 +84,9 @@ const EnvVarsSection: React.FC = () => {
className="rounded-md font-mono"
/>
</Form.Item>
<Form.Item
{...restField}
name={[name, "value"]}
className="mb-0"
style={{ flex: 1 }}
shouldUpdate
>
<ValueField fieldName={name} />
</Form.Item>
<div style={{ flex: 1 }}>
<ScopedValueOrDescription name={name} restField={restField} />
</div>
<Form.Item
{...restField}
name={[name, "scope"]}
@ -128,23 +122,28 @@ const EnvVarsSection: React.FC = () => {
);
};
// 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 (
<Form.Item {...restField} name={[name, "description"]} className="mb-0">
<Input
placeholder="Description shown to users, e.g. Your DB username"
className="rounded-md"
/>
</Form.Item>
);
}
return (
<Input
value={value ?? ""}
onChange={(e) => onChange?.(e.target.value)}
placeholder={isPerUser ? "Defined per user" : "e.g. postgresql"}
disabled={isPerUser}
className="rounded-md font-mono"
/>
<Form.Item {...restField} name={[name, "value"]} className="mb-0">
<Input placeholder="e.g. postgresql" className="rounded-md font-mono" />
</Form.Item>
);
};