feat(mcp): add admin-declared per-user fields for MCP servers - #28218
feat(mcp): add admin-declared per-user fields for MCP servers#28218mateo-berri wants to merge 23 commits into
Conversation
Generalizes the existing BYOK pattern (one user-provided credential per
server) to N admin-declared fields. Each field can target an HTTP header
(http/sse transports) or env var (stdio), with an optional value template
for prefixes like "Bearer {value}". Field values are encrypted at rest,
stored in the existing LiteLLM_MCPUserCredentials table with a
"type": "user_fields" discriminator so they don't collide with BYOK
strings or OAuth2 blobs.
Demo flow:
1. Admin adds a server with one or more user fields.
2. The user dashboard shows a red "N missing fields" badge until each
required field has a value.
3. Calling the server via Claude Code (or any MCP client) before saving
returns HTTP 401 with error="user_fields_missing", the list of
missing field descriptors, and a config_url pointing at the
dashboard.
4. After the user saves their values, the badge clears and tool calls
dispatch with the user's values injected as the configured headers
or env vars.
Backend
- New JSONB column LiteLLM_MCPServerTable.user_fields with a migration.
- MCPUserField / MCPUserFieldValuesRequest / MCPUserFieldsStatus types
on the existing create/update/read models.
- DB helpers store/get/delete user-field values via the same encryption
path as BYOK; a "type" discriminator keeps the three formats apart.
- New endpoints GET/POST/DELETE /v1/mcp/server/{id}/user-field-values
and GET /v1/mcp/user-field-values (aggregated for dashboard badges).
- GET /v1/mcp/server is annotated per-caller with missing_user_field_keys.
- execute_mcp_tool enforces required fields with a friendly 401 carrying
the dashboard config_url; the managed MCP dispatch path injects the
resolved headers and stdio env vars.
UI
- Admin "Add MCP Server" form gains a dynamic User Fields section.
- Dashboard servers list shows a red badge with the missing-field count
for each affected server and opens a new UserFieldsModal where the
end-user fills in their values.
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR generalises the existing BYOK pattern to N admin-declared per-user fields on MCP servers, backed by a new
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/db.py | Adds store_user_field_values, get_user_field_values, delete_user_field_values with type-discriminated JSON blob in credential_b64; refactors BYOK and OAuth2 writers to use optimistic CAS instead of unconditional upsert, guarding against cross-type overwrites. |
| litellm/proxy/_experimental/mcp_server/user_fields.py | New helper module: coerce_user_fields normalises dict/Pydantic-model/JSON-string inputs; compute_missing_user_fields, resolve_user_field_headers, resolve_user_field_env are pure functions with no side effects. Uses str.replace instead of str.format to prevent template-injection from admin-supplied header_value_template. |
| litellm/proxy/_experimental/mcp_server/server.py | Adds in-process user-fields cache mirroring the BYOK cache, _enforce_user_fields for the streamable-HTTP entrypoint, and injects resolved user-field headers via _request_user_field_headers ContextVar for local/OpenAPI tools. |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Adds _enforce_required_user_fields and _resolve_user_field_values to cover the Responses-API dispatch path; injects user-field headers/env into _call_regular_mcp_tool and _call_openapi_tool_handler; delegates cache lookups to server._get_user_field_values_cached. |
| litellm/proxy/management_endpoints/mcp_management_endpoints.py | Adds GET/POST/DELETE /server/{id}/user-field-values and GET /user-field-values endpoints; refactors BYOK annotation into _annotate_user_credential_flags (now also covers user-fields); adds _validate_mcp_user_fields_exclusive guard. Minor: oauth2_token_exchange not covered by the guard; dead RecordNotFoundError catch in the delete handler. |
| litellm/proxy/_types.py | Adds MCPUserField, MCPUserFieldValuesRequest, MCPUserFieldsStatus types; adds user_fields field to create/update/read models with duplicate-key validator and a model_validator that coerces JSONB strings to lists before Pydantic validates them. |
Reviews (23): Last reviewed commit: "fix(mcp): enforce required user_fields i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Add missing 'import time' in mcp_server_manager.py; _resolve_user_field_values() was calling time.monotonic() without the module imported, raising NameError on every cached tool call against a server with user_fields. - list_mcp_user_field_values now filters via get_all_mcp_servers_for_user instead of get_all_mcp_servers so non-admin callers no longer see server IDs and user-field metadata (header_name/env_var_name) for servers they cannot access. - store_user_field_values now refuses to overwrite a non-user-fields credential (BYOK / OAuth2) sharing the same (user_id, server_id) row, mirroring the existing skip_byok_guard pattern in store_user_oauth_credential. The POST endpoint surfaces this as a 409 conflict. - Deduplicate user-fields parsing helpers: replace _coerce_user_fields_list / _has_required_user_fields / _compute_missing_user_field_keys with the existing coerce_user_fields / server_has_user_fields / compute_missing_user_fields from user_fields.py. - Remove dead lookup_cached_user_fields() (and its now-unused 'time' import) from user_fields.py. Co-authored-by: Yassin Kortam <yassin@berri.ai>
PR overviewMedium: Required MCP user fields can be bypassed on direct tool callsThis PR adds per-user MCP field storage and injection, with enforcement in Security review
Risk: 5/10 |
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
fetch_all_mcp_servers was 55 statements (limit 50) after adding the user-fields annotation. Move the BYOK + missing-user-fields batch annotation into _annotate_user_credential_flags.
The per-server GET/POST/DELETE /v1/mcp/server/{server_id}/user-field-values
handlers previously called get_mcp_server directly with no access check.
A non-admin user who knew (or guessed) another team's server_id could
fetch the admin-declared user_fields metadata (display names, header
names, env var names) or write/delete user-field values for that server.
Gate the three endpoints behind get_all_mcp_servers_for_user (same scoping
already used by the aggregated /v1/mcp/user-field-values endpoint and by
fetch_mcp_server). Admins bypass the check; everyone else gets a 404
(not 403) so the status code does not leak server existence.
The helpers use getattr(server, 'user_fields', None) and work with any server-shaped object. Widen the type so the management endpoint callers that pass LiteLLM_MCPServerTable type-check cleanly.
…tion - db.get_user_credential / store_user_credential: refuse to read or overwrite a user-fields payload, so BYOK never injects the raw JSON blob as an Authorization header. - server._enforce_user_fields: when the caller has no user_id but the server only declares optional user_fields, return without raising the spurious 401. - Promote _get_user_field_values_cached to module level so the dispatch path in mcp_server_manager._resolve_user_field_values shares the same cache + DB lookup implementation. - _call_regular_mcp_tool: resolve user-field values once and pass them to resolve_user_field_headers / resolve_user_field_env directly, eliminating the duplicate cache lookup and the two wrapper helpers. Co-authored-by: Yassin Kortam <yassin@berri.ai>
This comment has been minimized.
This comment has been minimized.
|
Reply to greptile's summary comment (id 4484770031), Comments Outside Diff finding " This is already fixed on HEAD. Commit e45eda4 ("fix(mcp): tighten user-fields/BYOK collisions and dedupe value resolution") added the symmetric conflict guard at the top of existing = await prisma_client.db.litellm_mcpusercredentials.find_unique(
where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
)
if (
existing is not None
and _decode_user_fields_payload(existing.credential_b64) is not None
):
raise ValueError(
f"Existing credential for user {user_id} and server "
f"{server_id} holds user-fields values. Refusing to overwrite "
f"with a BYOK credential."
)That is exactly the symmetric guard greptile asked for: a BYOK save now refuses to clobber a stored user-fields payload, breaking the deadlock chain (1)→(2). Same commit also hardened |
- coerce_user_fields / _build_user_fields_status: accept MCPUserField Pydantic instances in addition to raw dicts, so a fully-typed LiteLLM_MCPServerTable passed through these helpers no longer silently drops every entry. - store_user_field_values: replace read-then-write with optimistic concurrency (create + UniqueViolation retry, then compare-and-swap on credential_b64 via update_many) so a concurrent BYOK / OAuth2 write to the same (user_id, server_id) row cannot be silently overwritten. Co-authored-by: Yassin Kortam <yassin@berri.ai>
This comment has been minimized.
This comment has been minimized.
…er_fields - store_mcp_user_credential now catches the ValueError raised by store_user_credential when the (user, server) row already holds a user-fields payload, returning HTTP 409 instead of leaking a 500. - coerce_user_fields now also accepts MCPUserField Pydantic instances (the shape used by LiteLLM_MCPServerTable.user_fields), so the management-layer annotation and enforcement paths no longer return silently empty when callers pass a parsed server record.
Pass through payload.user_fields when building the temporary MCP server record so session-cached servers retain their admin-declared fields. Replace the bespoke list/string/instance handling in _build_user_fields_status with the shared coerce_user_fields helper.
| # after a salt-key rotation). Either way, a credential row | ||
| # exists for this (user, server), so surface it as present | ||
| # instead of silently telling the user to reconnect. | ||
| byok_set.add(row.server_id) |
There was a problem hiding this comment.
Batch annotation decrypts every credential row individually
Medium Severity
_annotate_user_credential_flags calls _decode_user_credential (which performs NaCl decryption with a base64 fallback) for every credential row in the batch. This runs synchronously on the event loop during the server-list endpoint. For deployments with many BYOK/user-fields servers and many users, this adds significant CPU-bound latency to every GET /v1/mcp/server call, blocking the async event loop during decryption. The previous code only checked row existence without decrypting.
Reviewed by Cursor Bugbot for commit 3851d8a. Configure here.
Add unit tests for the previously-uncovered branches in user_fields.py so the patch hits the project's coverage target: - coerce_user_fields with a JSON-encoded string (Prisma JSONB fallback), invalid JSON, non-list JSON, unsupported scalar types, and entries that are neither dicts nor model_dump-able (including a model_dump that raises). - compute_missing_user_fields skipping malformed entries (no key, empty key, non-string key). - resolve_user_field_headers skipping entries without header_name. - resolve_user_field_env skipping entries without env_var_name and empty stored values. Also fixes test_coerce_user_fields_accepts_json_string, which was inadvertently round-tripping through json.loads(json.dumps(...)) and therefore never actually exercising the string-input branch it was documenting.
The OpenAPI/local-tool dispatch path validated required user-field values via _enforce_user_fields but never injected them into the upstream request. The managed MCP path already did this in _call_regular_mcp_tool, so users configuring a user-fields-enabled server backed by an OpenAPI spec would see the dashboard turn green but still hit upstream auth failures. Add a _request_user_field_headers ContextVar mirroring the existing _request_auth_header/_request_extra_headers pattern, populate it in the local-tool branch of execute_mcp_tool, and merge it after static_headers in _merge_openapi_tool_request_headers so admin-declared per-user values win over operator-configured static headers (matching managed-MCP precedence). BYOK Authorization override still wins over the user-field Authorization to mirror the managed path's separate mcp_auth_header parameter.
… and BYOK annotation
- resolve_user_field_headers: switch from str.format to str.replace so
admin-supplied templates cannot use Python format-string attribute or
item access (e.g. {value.__class__}) to leak object internals into
outbound HTTP headers. Update fallback test to assert literal token
preservation, and add a regression test for the attribute-access case.
- _annotate_user_credential_flags: skip OAuth2 rows when populating
has_user_credential for is_byok=True servers. Otherwise a server
reconfigured from OAuth2 to BYOK shows a misleading 'Connected' badge
while the actual tool call would fail (get_user_credential filters
OAuth2 rows out at execution time). Re-uses the already-decrypted
plaintext, so no extra crypto cost.
Co-authored-by: Yassin Kortam <yassin@berri.ai>
|
Both file-level observations the summary flags as warranting a second look are intentional choices in this PR:
Also, the file-table note that "user-field headers are not injected into the local/OpenAPI tool path" has since been addressed in commit All five inline review comments ( |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high mode and found 4 potential issues.
There are 6 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, have a team admin raise the spend limit in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 19b6872. Configure here.
| registration_url=payload.registration_url, | ||
| allow_all_keys=payload.allow_all_keys, | ||
| available_on_public_internet=payload.available_on_public_internet, | ||
| user_fields=payload.user_fields, |
There was a problem hiding this comment.
Virtual key sanitization leaks user field metadata
Low Severity
_sanitize_mcp_server_for_virtual_key and _sanitize_mcp_server_for_non_admin strip static_headers, env, extra_headers, command, and args to hide internal config from unprivileged callers, but neither strips the new user_fields list or missing_user_field_keys. The user_fields entries contain header_name, env_var_name, and header_value_template — the same kind of internal injection details the sanitizers were designed to redact.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 19b6872. Configure here.
There was a problem hiding this comment.
Not a real concern. user_fields is schema, not credentials.
Each entry contains field_key, display_name, description, required, header_name, header_value_template, and env_var_name — i.e. the form definition the dashboard needs to render the end-user input UI. The actual values the user supplies are stored encrypted in LiteLLM_MCPUserCredentials.credential_b64 and are never echoed back via any read path (cf. _decode_user_fields_payload + the GET /server/{id}/user-field-values handler in mcp_management_endpoints.py, which returns MCPUserFieldsStatus — user_fields descriptors + stored_field_keys / missing_field_keys, never the values).
Compare with what the sanitizers actually strip:
static_headers→{"Authorization": "Bearer sk-XXXX"}— values, often raw bearer tokens.env→{"OPENAI_API_KEY": "sk-XXXX"}— values, often raw API keys.url→ oftenhttps://host/path?token=XXXX— values.args→ can contain--api-key XXXX— values.
Those redactions exist because the field is itself a credential carrier. user_fields is the inverse: it advertises that the upstream needs a per-user Authorization (or X-Workspace-Id, etc.) header, in the same way an OpenAPI securitySchemes definition does. That information is required for the end-user to know what they need to provide.
The non-admin sanitizer in particular cannot strip user_fields / missing_user_field_keys: the dashboard's per-server "N missing fields" badge and the UserFieldsModal (which is the only UI for non-admins to save their values) are driven directly off those two fields. Stripping them would break the feature for exactly the audience it was built for.
For restricted virtual keys the data is descriptive (no values), and there is no security model under which the name Authorization or the template literal Bearer {value} is sensitive — they are inherent to whatever upstream API the server proxies and would be discoverable via a single 401 response from the upstream itself.
Keeping HEAD.
| try: | ||
| await delete_user_field_values(prisma_client, user_id, server_id) | ||
| except RecordNotFoundError: | ||
| pass |
There was a problem hiding this comment.
delete_user_field_values never raises RecordNotFoundError despite handler catching it
Low Severity
The DELETE endpoint catches RecordNotFoundError from delete_user_field_values, but that function never raises it — it returns False when no eligible row exists. This is dead exception handling. The mismatch suggests the endpoint was modeled after delete_user_credential (which does raise RecordNotFoundError) but delete_user_field_values uses a different contract. Not harmful but the return value is silently ignored.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 19b6872. Configure here.
There was a problem hiding this comment.
The try/except RecordNotFoundError is not dead — it covers a real race in delete_user_field_values.
async def delete_user_field_values(...):
row = await prisma_client.db.litellm_mcpusercredentials.find_unique(...) # (1)
if row is None:
return False
if _decode_user_fields_payload(row.credential_b64) is None:
return False
await prisma_client.db.litellm_mcpusercredentials.delete(where=...) # (2)
return TrueBetween (1) and (2), another concurrent request (e.g. the same user in another browser tab also clicking 'Clear', or a BYOK conversion that swaps the row out) can drop the row. Prisma's db.<model>.delete(where=…) raises RecordNotFoundError when the targeted record is missing — that's the contract documented in prisma-client-py and the reason the BYOK delete endpoint has the same guard (mcp_management_endpoints.py:2015).
Bugbot's summary says the function 'returns False when no eligible row exists' — true for the two if … return False branches, but not for the await prisma_client.db.…delete(…) call below them. So delete_user_field_values does propagate RecordNotFoundError in the race-loss case, and the handler exists to swallow that loss and respond 200 (the row the caller asked us to delete is gone, which is the requested end state).
The existing regression test test_delete_user_field_values_swallows_record_not_found (tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_user_fields.py) pins this contract: it mocks delete_user_field_values to raise RecordNotFoundError and asserts the endpoint still returns 200. Removing the guard would regress that case to a 500.
Keeping HEAD.
| # with a config_url pointing at the dashboard when any required | ||
| # field is missing. | ||
| if server_has_user_fields(mcp_server): | ||
| await _enforce_user_fields(mcp_server, user_api_key_auth) |
There was a problem hiding this comment.
Conflicting is_byok and user_fields creates unresolvable state
Medium Severity
A server configured with both is_byok=True and non-empty user_fields traps users in an unresolvable state. The BYOK check in execute_mcp_tool runs first and requires a stored credential, but store_user_credential refuses to overwrite a user-fields row (raises ValueError). Conversely, store_user_field_values refuses to overwrite a BYOK row. The dashboard column rendering prioritizes the user-fields badge, showing "Ready" once fields are filled, while the tool call still 401s with byok_auth_required. No validation prevents this configuration.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 19b6872. Configure here.
| if user_field_headers: | ||
| if extra_headers is None: | ||
| extra_headers = {} | ||
| extra_headers.update(user_field_headers) |
There was a problem hiding this comment.
Managed path header merge lacks case-insensitive dedup
Low Severity
The managed MCP dispatch path in _call_regular_mcp_tool merges user-field headers with extra_headers.update(user_field_headers), which is case-sensitive — if static_headers has "Authorization" and a user field targets "authorization", both headers are sent. The OpenAPI/local path in _merge_openapi_tool_request_headers correctly handles this with case-insensitive collision detection and dedup. This inconsistency means the same server configuration can send duplicate (differently-cased) authorization headers on one path but not the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 19b6872. Configure here.
…clear false UI Ready badge - mcp_server_manager.py: resolve admin-declared per-user field values and forward them as upstream headers through the managed _call_openapi_tool_handler path via the openapi generator's _request_user_field_headers ContextVar. The local-registry dispatch in server.execute_mcp_tool already injects these; the managed fallback was silently dropping them, so a server with spec_path + user_fields would pass _enforce_user_fields but still fail upstream with no credentials. - mcp_server_columns.tsx: distinguish null/undefined (annotation never ran — e.g. empty user_id, no DB connection) from [] (verified complete) when rendering the user-fields credential cell. Previously a server whose missing_user_field_keys was never populated rendered a green 'Ready' badge while enforcement at tool-call time would still return 401, lying to the user. Now show a neutral 'Not verified' badge when the annotation was skipped. Co-authored-by: Yassin Kortam <yassin@berri.ai>
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Preview (bd9a6e4872)diff --git a/litellm-proxy-extras/litellm_proxy_extras/migrations/20260519120000_add_mcp_user_fields/migration.sql b/litellm-proxy-extras/litellm_proxy_extras/migrations/20260519120000_add_mcp_user_fields/migration.sql
new file mode 100644
--- /dev/null
+++ b/litellm-proxy-extras/litellm_proxy_extras/migrations/20260519120000_add_mcp_user_fields/migration.sql
@@ -1,0 +1,2 @@
+-- AlterTable
+ALTER TABLE "LiteLLM_MCPServerTable" ADD COLUMN IF NOT EXISTS "user_fields" JSONB NOT NULL DEFAULT '[]';
diff --git a/litellm-proxy-extras/litellm_proxy_extras/schema.prisma b/litellm-proxy-extras/litellm_proxy_extras/schema.prisma
--- a/litellm-proxy-extras/litellm_proxy_extras/schema.prisma
+++ b/litellm-proxy-extras/litellm_proxy_extras/schema.prisma
@@ -328,6 +328,11 @@
is_byok Boolean @default(false)
byok_description String[] @default([])
byok_api_key_help_url String?
+ // Admin-defined per-user fields (e.g. bearer tokens, workspace IDs) that
+ // each end-user must supply via the dashboard before they can use the
+ // server. Each entry is a JSON object describing how the field is rendered
+ // in the dashboard and injected at request time.
+ user_fields Json @default("[]")
source_url String?
// BYOM submission lifecycle
approval_status String? @default("active")
diff --git a/litellm/proxy/_experimental/mcp_server/db.py b/litellm/proxy/_experimental/mcp_server/db.py
--- a/litellm/proxy/_experimental/mcp_server/db.py
+++ b/litellm/proxy/_experimental/mcp_server/db.py
@@ -1,9 +1,12 @@
+import asyncio
import base64
import binascii
import json
from datetime import datetime, timedelta, timezone
-from typing import Any, Dict, Iterable, List, Optional, Set, Union, cast
+from typing import Any, Callable, Dict, Iterable, List, Optional, Set, Union, cast
+from prisma.errors import RecordNotFoundError, UniqueViolationError
+
from litellm._logging import verbose_proxy_logger
from litellm._uuid import uuid
from litellm.proxy._types import (
@@ -85,6 +88,13 @@
# but be explicit to ensure a False value is always written to the DB).
data_dict["is_byok"] = getattr(data, "is_byok", False)
+ # user_fields is a list of MCPUserField models. exclude_none=True will
+ # already have dict-ified them, but the JSONB column expects a JSON
+ # string when written through Prisma.
+ user_fields = data_dict.get("user_fields")
+ if user_fields is not None:
+ data_dict["user_fields"] = safe_dumps(user_fields)
+
return data_dict
@@ -581,19 +591,66 @@
server_id: str,
credential: str,
) -> None:
- """Store a user credential for a BYOK MCP server."""
+ """Store a user credential for a BYOK MCP server.
+ BYOK, OAuth2, and user-fields payloads share the same ``credential_b64``
+ column. Refuse to overwrite a stored user-fields payload so saving a
+ BYOK credential does not silently destroy the user's saved field values.
+
+ Uses optimistic concurrency (compare-and-swap on ``credential_b64``) so
+ a concurrent ``store_user_field_values`` write between the read and the
+ write cannot be silently overwritten — mirroring the protection that
+ user-fields writes already have against BYOK writes.
+ """
+
encoded = encrypt_value_helper(credential)
- await prisma_client.db.litellm_mcpusercredentials.upsert(
- where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}},
- data={
- "create": {
+
+ for _attempt in range(5):
+ existing = await prisma_client.db.litellm_mcpusercredentials.find_unique(
+ where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
+ )
+
+ if existing is None:
+ try:
+ await prisma_client.db.litellm_mcpusercredentials.create(
+ data={
+ "user_id": user_id,
+ "server_id": server_id,
+ "credential_b64": encoded,
+ }
+ )
+ return
+ except UniqueViolationError:
+ # A concurrent writer inserted the row first; restart so we
+ # can inspect what they wrote before clobbering it.
+ await asyncio.sleep(0)
+ continue
+
+ if _decode_user_fields_payload(existing.credential_b64) is not None:
+ raise ValueError(
+ f"Existing credential for user {user_id} and server "
+ f"{server_id} holds user-fields values. Refusing to overwrite "
+ f"with a BYOK credential."
+ )
+
+ # Compare-and-swap on credential_b64: only succeed if the row still
+ # matches what we just inspected, so a concurrent user-fields write
+ # between the read and the write is not silently overwritten.
+ updated = await prisma_client.db.litellm_mcpusercredentials.update_many(
+ where={
"user_id": user_id,
"server_id": server_id,
- "credential_b64": encoded,
+ "credential_b64": existing.credential_b64,
},
- "update": {"credential_b64": encoded},
- },
+ data={"credential_b64": encoded},
+ )
+ if updated:
+ return
+ await asyncio.sleep(0)
+
+ raise RuntimeError(
+ f"store_user_credential: gave up after repeated concurrent "
+ f"modifications for user {user_id} and server {server_id}"
)
@@ -602,13 +659,22 @@
user_id: str,
server_id: str,
) -> Optional[str]:
- """Return credential for a user+server pair, or None."""
+ """Return credential for a user+server pair, or None.
+ The ``credential_b64`` column is multiplexed between BYOK strings,
+ OAuth2 blobs and user-fields blobs. A row holding a user-fields
+ payload is not a BYOK credential — return ``None`` so the caller
+ triggers the normal "no credential stored" flow instead of injecting
+ the raw JSON blob as an Authorization header.
+ """
+
row = await prisma_client.db.litellm_mcpusercredentials.find_unique(
where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
)
if row is None:
return None
+ if _decode_user_fields_payload(row.credential_b64) is not None:
+ return None
return _decode_user_credential(row.credential_b64)
@@ -629,12 +695,240 @@
user_id: str,
server_id: str,
) -> None:
- """Delete the user's stored credential for a BYOK MCP server."""
+ """Delete the user's stored credential for a BYOK MCP server.
+
+ BYOK, OAuth2, and user-fields payloads share the same
+ ``(user_id, server_id)`` row. Refuse to delete a row that holds a
+ user-fields payload so the BYOK delete endpoint does not silently
+ destroy the user's saved field values — mirroring the overwrite
+ guards on the write paths.
+ """
+ existing = await prisma_client.db.litellm_mcpusercredentials.find_unique(
+ where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
+ )
+ if existing is None:
+ raise RecordNotFoundError(
+ data={"error": {"message": "no BYOK credential row", "meta": {}}}
+ )
+ if _decode_user_fields_payload(existing.credential_b64) is not None:
+ # Treat as "no BYOK credential present" so the endpoint reports
+ # has_credential=False without clobbering the user-fields row.
+ raise RecordNotFoundError(
+ data={
+ "error": {
+ "message": "row holds user-fields payload, not a BYOK credential",
+ "meta": {},
+ }
+ }
+ )
await prisma_client.db.litellm_mcpusercredentials.delete(
where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
)
+# ── User-fields helpers ───────────────────────────────────────────────────────
+#
+# User-fields share the credential_b64 column with BYOK and OAuth2. We tag
+# the JSON payload with ``"type": "user_fields"`` so the read path can
+# distinguish formats without an extra column.
+
+
+def _parse_user_fields_plaintext(decoded: str) -> Optional[Dict[str, str]]:
+ """Return the field-values dict if ``decoded`` is a user-fields JSON payload.
+
+ Takes already-decrypted plaintext so callers that have already paid
+ the decryption cost (e.g. annotating server-list responses) can avoid
+ a redundant decryption round-trip.
+ """
+ try:
+ parsed = json.loads(decoded)
+ except (ValueError, TypeError):
+ return None
+ if not isinstance(parsed, dict) or parsed.get("type") != "user_fields":
+ return None
+ values = parsed.get("values")
+ if not isinstance(values, dict):
+ return {}
+ return {str(k): str(v) for k, v in values.items()}
+
+
+def _decode_user_fields_payload(stored: str) -> Optional[Dict[str, str]]:
+ """Return the field-values dict if ``stored`` holds a user-fields payload."""
+ decoded = _decode_user_credential(stored)
+ if decoded is None:
+ return None
+ return _parse_user_fields_plaintext(decoded)
+
+
+async def store_user_field_values(
+ prisma_client: PrismaClient,
+ user_id: str,
+ server_id: str,
+ values: Optional[Dict[str, str]] = None,
+ *,
+ merge_fn: Optional[Callable[[Dict[str, str]], Dict[str, str]]] = None,
+) -> Dict[str, str]:
+ """Persist the calling user's values for an MCP server's user fields.
+
+ The full set of values is encoded into a single encrypted JSON blob in
+ ``LiteLLM_MCPUserCredentials.credential_b64``. A ``"type"`` discriminator
+ lets ``get_user_field_values`` tell user-fields rows apart from BYOK
+ strings and OAuth2 payloads sharing the same column.
+
+ BYOK and OAuth2 credentials share the same ``(user_id, server_id)`` row.
+ Refuse to overwrite a non-user-fields credential so saving user-field
+ values does not silently destroy a stored BYOK API key or OAuth2 token.
+
+ Exactly one of ``values`` or ``merge_fn`` must be supplied:
+
+ * ``values`` writes the dict as-is.
+ * ``merge_fn`` is invoked with the currently-stored user-fields values
+ (``{}`` if no row exists) and must return the dict to persist. It is
+ re-invoked on every CAS retry, so a concurrent partial save from the
+ same user in another tab is merged into the result instead of being
+ silently overwritten.
+
+ Uses optimistic concurrency to close the read-then-write race against
+ concurrent ``store_user_credential`` / ``store_user_oauth_credential``
+ calls: the write is gated on the previously-observed ``credential_b64``
+ being unchanged, and we retry from the re-read on contention.
+
+ Returns the dict that was ultimately persisted.
+ """
+
+ if (values is None) == (merge_fn is None):
+ raise ValueError(
+ "store_user_field_values: exactly one of `values` or `merge_fn` "
+ "must be provided"
+ )
+
+ def _encode(field_values: Dict[str, str]) -> str:
+ return encrypt_value_helper(
+ json.dumps({"type": "user_fields", "values": field_values})
+ )
+
+ # Pre-compute encoding for the ``values`` path so we don't pay encryption
+ # cost on every retry iteration when there is no merge function.
+ static_encoded: Optional[str] = None
+ if merge_fn is None:
+ assert values is not None
+ static_encoded = _encode(values)
+
+ # Bound the retry loop; in practice contention is resolved in a single
+ # extra round-trip but we leave headroom for pathological interleavings.
+ for _attempt in range(5):
+ existing = await prisma_client.db.litellm_mcpusercredentials.find_unique(
+ where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
+ )
+
+ if existing is None:
+ if merge_fn is not None:
+ current_values = merge_fn({})
+ encoded = _encode(current_values)
+ else:
+ assert values is not None and static_encoded is not None
+ current_values = values
+ encoded = static_encoded
+ try:
+ await prisma_client.db.litellm_mcpusercredentials.create(
+ data={
+ "user_id": user_id,
+ "server_id": server_id,
+ "credential_b64": encoded,
+ }
+ )
+ return current_values
+ except UniqueViolationError:
+ # A concurrent writer inserted the row first; restart so we
+ # can inspect what they wrote before clobbering it.
+ await asyncio.sleep(0)
+ continue
+
+ existing_field_values = _decode_user_fields_payload(existing.credential_b64)
+ if existing_field_values is None:
+ raise ValueError(
+ f"Existing credential for user {user_id} and server "
+ f"{server_id} is not a user-fields payload (likely BYOK or "
+ f"OAuth2). Refusing to overwrite."
+ )
+
+ # When a merge function is supplied, recompute on every retry against
+ # the freshly-read existing values so a concurrent user-fields write
+ # from the same user in another tab is not silently dropped.
+ if merge_fn is not None:
+ current_values = merge_fn(existing_field_values)
+ encoded = _encode(current_values)
+ else:
+ assert values is not None and static_encoded is not None
+ current_values = values
+ encoded = static_encoded
+
+ # Compare-and-swap on credential_b64: only succeed if the row still
+ # matches what we just inspected, so a concurrent BYOK/OAuth2 write
+ # between the read and the write is not silently overwritten.
+ updated = await prisma_client.db.litellm_mcpusercredentials.update_many(
+ where={
+ "user_id": user_id,
+ "server_id": server_id,
+ "credential_b64": existing.credential_b64,
+ },
+ data={"credential_b64": encoded},
+ )
+ if updated:
+ return current_values
+ await asyncio.sleep(0)
+
+ raise RuntimeError(
+ f"store_user_field_values: gave up after repeated concurrent "
+ f"modifications for user {user_id} and server {server_id}"
+ )
+
+
+async def get_user_field_values(
+ prisma_client: PrismaClient,
+ user_id: str,
+ server_id: str,
+) -> Optional[Dict[str, str]]:
+ """Return the user's stored field values, or ``None`` if not stored.
+
+ Returns ``None`` both when no row exists and when the row holds a
+ different credential type (BYOK / OAuth2) — callers should treat both
+ as "no user-fields configured for this user yet".
+ """
+
+ row = await prisma_client.db.litellm_mcpusercredentials.find_unique(
+ where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
+ )
+ if row is None:
+ return None
+ return _decode_user_fields_payload(row.credential_b64)
+
+
+async def delete_user_field_values(
+ prisma_client: PrismaClient,
+ user_id: str,
+ server_id: str,
+) -> bool:
+ """Delete the user's stored field values.
+
+ Only removes the row when it actually holds a user-fields payload, so a
+ co-located BYOK / OAuth2 credential for the same (user, server) pair is
+ not accidentally wiped. Returns True if a row was deleted.
+ """
+
+ row = await prisma_client.db.litellm_mcpusercredentials.find_unique(
+ where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
+ )
+ if row is None:
+ return False
+ if _decode_user_fields_payload(row.credential_b64) is None:
+ return False
+ await prisma_client.db.litellm_mcpusercredentials.delete(
+ where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
+ )
+ return True
+
+
# ── OAuth2 user-credential helpers ────────────────────────────────────────────
@@ -673,38 +967,60 @@
if scopes:
payload["scopes"] = scopes
- # Guard against silently overwriting a BYOK credential with an OAuth token.
- # Skip the guard when the caller knows the row is already an OAuth2 credential
- # (e.g. during token refresh), saving an extra DB round-trip.
- if not skip_byok_guard:
+ encoded = encrypt_value_helper(json.dumps(payload))
+
+ # Optimistic concurrency: compare-and-swap on ``credential_b64`` so a
+ # concurrent BYOK or user-fields write between the read and the write is
+ # not silently overwritten. ``skip_byok_guard`` (token refresh) bypasses
+ # the type check but still uses CAS to avoid clobbering data.
+ for _attempt in range(5):
existing = await prisma_client.db.litellm_mcpusercredentials.find_unique(
where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}}
)
+
+ if existing is None:
+ try:
+ await prisma_client.db.litellm_mcpusercredentials.create(
+ data={
+ "user_id": user_id,
+ "server_id": server_id,
+ "credential_b64": encoded,
+ }
+ )
+ return
+ except UniqueViolationError:
+ await asyncio.sleep(0)
+ continue
+
if (
- existing is not None
+ not skip_byok_guard
and _decode_oauth_payload(existing.credential_b64) is None
):
- # Existing row is either a BYOK secret or an OAuth2 row that no
- # longer decrypts (e.g. after a salt-key rotation). In either
- # case, refuse to overwrite — the caller would clobber data
- # that may still be recoverable.
+ # Existing row is either a BYOK secret, a user-fields blob, or an
+ # OAuth2 row that no longer decrypts (e.g. after a salt-key
+ # rotation). In any case, refuse to overwrite — the caller would
+ # clobber data that may still be recoverable.
raise ValueError(
f"Existing credential for user {user_id} and server "
f"{server_id} could not be verified as an OAuth2 token. "
f"Refusing to overwrite."
)
- encoded = encrypt_value_helper(json.dumps(payload))
- await prisma_client.db.litellm_mcpusercredentials.upsert(
- where={"user_id_server_id": {"user_id": user_id, "server_id": server_id}},
- data={
- "create": {
+ updated = await prisma_client.db.litellm_mcpusercredentials.update_many(
+ where={
"user_id": user_id,
"server_id": server_id,
- "credential_b64": encoded,
+ "credential_b64": existing.credential_b64,
},
- "update": {"credential_b64": encoded},
- },
+ data={"credential_b64": encoded},
+ )
+ if updated:
+ return
+ await asyncio.sleep(0)
+
+ raise RuntimeError(
+ f"store_user_oauth_credential: gave up after repeated concurrent "
+ f"modifications for user {user_id} and server {server_id}"
)
diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
--- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
+++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
@@ -190,6 +190,25 @@
return data
+def _deserialize_user_fields(data: Any) -> List[Dict[str, Any]]:
+ """Decode the JSON-encoded ``user_fields`` blob from the MCP server row.
+
+ Always returns a list — falsy or malformed values become ``[]`` so callers
+ can iterate without a None check.
+ """
+ if not data:
+ return []
+ if isinstance(data, list):
+ return data
+ if isinstance(data, str):
+ try:
+ decoded = json.loads(data)
+ except (json.JSONDecodeError, TypeError):
+ return []
+ return decoded if isinstance(decoded, list) else []
+ return []
+
+
class MCPServerManager:
_STDIO_ENV_TEMPLATE_PATTERN = re.compile(r"^\$\{(X-[^}]+)\}$")
@@ -812,6 +831,9 @@
is_byok=bool(getattr(mcp_server, "is_byok", False)),
byok_description=getattr(mcp_server, "byok_description", None) or [],
byok_api_key_help_url=getattr(mcp_server, "byok_api_key_help_url", None),
+ user_fields=_deserialize_user_fields(
+ getattr(mcp_server, "user_fields", None)
+ ),
# AWS SigV4 fields
aws_access_key_id=aws_creds.get("aws_access_key_id"),
aws_secret_access_key=aws_creds.get("aws_secret_access_key"),
@@ -1276,11 +1298,23 @@
self,
server: MCPServer,
raw_headers: Optional[Dict[str, str]] = None,
+ user_field_env: Optional[Dict[str, str]] = None,
) -> Optional[Dict[str, str]]:
- """Resolve stdio env values, supporting header-driven placeholders."""
+ """Resolve stdio env values, supporting header-driven placeholders.
- if server.transport != MCPTransport.stdio or not server.env:
+ ``user_field_env`` carries values resolved from admin-declared
+ user_fields with an ``env_var_name`` set. They take precedence
+ over the static server.env entries so a user's stored value
+ always overrides any placeholder default.
+ """
+
+ if server.transport != MCPTransport.stdio:
+ # Non-stdio transports (HTTP/SSE) don't take an env dict; user
+ # fields with env_var_name are stdio-only. Match the legacy
+ # contract of always returning None for non-stdio servers.
return None
+ if not server.env:
+ return user_field_env or None
resolved_env: Dict[str, str] = {}
normalized_headers = {k.lower(): v for k, v in (raw_headers or {}).items()}
@@ -1297,8 +1331,46 @@
else:
resolved_env[env_key] = env_value
+ if user_field_env:
+ resolved_env.update(user_field_env)
+ # Preserve the legacy contract: when an env dict is present on the
+ # server we always return a dict (even if empty after template
+ # resolution). Only return None when no env is configured at all.
return resolved_env
+ async def _resolve_user_field_values(
+ self,
+ mcp_server: MCPServer,
+ user_api_key_auth: Optional["UserAPIKeyAuth"],
+ ) -> Dict[str, str]:
+ """Read the calling user's stored user-field values for ``mcp_server``.
+
+ Returns ``{}`` when the user has no row, no user_id, or there's no
+ DB. The caller decides what to do with missing required fields —
+ ``server.execute_mcp_tool`` raises a 401 with a config_url before
+ we even get here, so by the time injection happens we already
+ know the values are present.
+
+ Delegates to ``server._get_user_field_values_cached`` so the
+ enforcement check and dispatch share a single cache + DB-lookup
+ implementation.
+ """
+ from litellm.proxy._experimental.mcp_server.user_fields import (
+ coerce_user_fields,
+ )
+
+ if not coerce_user_fields(mcp_server):
+ return {}
+ if user_api_key_auth is None or not getattr(user_api_key_auth, "user_id", None):
+ return {}
+
+ from litellm.proxy._experimental.mcp_server.server import ( # noqa: PLC0415
+ _get_user_field_values_cached,
+ )
+
+ _, values = await _get_user_field_values_cached(mcp_server, user_api_key_auth)
+ return values or {}
+
async def _create_mcp_client(
self,
server: MCPServer,
@@ -2410,6 +2482,7 @@
server: MCPServer,
tool_name: str,
arguments: Dict[str, Any],
+ user_field_headers: Optional[Dict[str, str]] = None,
) -> CallToolResult:
"""
Call an OpenAPI tool handler directly.
@@ -2421,12 +2494,20 @@
Args:
tool_name: The full tool name (with prefix) to call
arguments: Tool arguments to pass to the handler
+ user_field_headers: Optional admin-declared per-user header values
+ resolved from ``MCPServer.user_fields`` for the calling user.
+ Forwarded via the openapi generator's request ContextVar so
+ the closure-baked handler picks them up — mirrors the
+ local-registry dispatch path in ``server.execute_mcp_tool``.
Returns:
CallToolResult with the response from the API
"""
from mcp.types import TextContent
+ from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import (
+ _request_user_field_headers,
+ )
from litellm.proxy._experimental.mcp_server.tool_registry import (
global_mcp_tool_registry,
)
@@ -2442,6 +2523,11 @@
isError=True,
)
+ _user_field_token = (
+ _request_user_field_headers.set(user_field_headers)
+ if user_field_headers
+ else None
+ )
try:
# Call the tool handler with the arguments
# The handler is an async function that makes the HTTP request
@@ -2462,6 +2548,9 @@
content=[TextContent(type="text", text=error_msg)],
isError=True,
)
+ finally:
+ if _user_field_token is not None:
+ _request_user_field_headers.reset(_user_field_token)
async def pre_call_tool_check(
self,
@@ -2633,6 +2722,7 @@
proxy_logging_obj: Optional[ProxyLogging],
host_progress_callback: Optional[Callable] = None,
hook_extra_headers: Optional[Dict[str, str]] = None,
+ user_api_key_auth: Optional[UserAPIKeyAuth] = None,
) -> CallToolResult:
"""
Call a regular MCP tool using the MCP client.
@@ -2717,6 +2807,28 @@
extra_headers = {}
extra_headers.update(mcp_server.static_headers)
+ # User-fields: resolve the calling user's stored values once and
+ # reuse them for both the header (http/sse) and env (stdio) paths
+ # below — calling the resolver twice would repeat cache lookups
+ # and coercion work for the same data.
+ from litellm.proxy._experimental.mcp_server.user_fields import (
+ resolve_user_field_env,
+ resolve_user_field_headers,
+ )
+
+ stored_user_field_values = await self._resolve_user_field_values(
+ mcp_server, user_api_key_auth
+ )
+ user_field_headers = (
+ resolve_user_field_headers(mcp_server, stored_user_field_values)
+ if stored_user_field_values
+ else {}
+ )
+ if user_field_headers:
+ if extra_headers is None:
+ extra_headers = {}
+ extra_headers.update(user_field_headers)
+
if hook_extra_headers:
if extra_headers is None:
extra_headers = {}
@@ -2724,8 +2836,8 @@
if "Authorization" in extra_headers:
verbose_logger.warning(
"MCPServerManager: hook_extra_headers 'Authorization' will overwrite "
- "the existing Authorization header from static_headers. "
- "The hook JWT will take precedence."
+ "the existing Authorization header (from static_headers, forwarded raw "
+ "headers, or user-fields). The hook JWT will take precedence."
)
elif server_auth_header is not None:
# server_auth_header is passed separately to _create_mcp_client as
@@ -2746,7 +2858,14 @@
if extra_headers is not None and len(extra_headers) == 0:
extra_headers = None
- stdio_env = self._build_stdio_env(mcp_server, raw_headers)
+ user_field_env = (
+ resolve_user_field_env(mcp_server, stored_user_field_values)
+ if stored_user_field_values
+ else {}
+ )
+ stdio_env = self._build_stdio_env(
+ mcp_server, raw_headers, user_field_env=user_field_env or None
+ )
client = await self._create_mcp_client(
server=mcp_server,
@@ -2904,9 +3023,34 @@
"transport to enable hook header injection.",
server_name,
)
+ # User-fields: resolve the calling user's stored values and forward
+ # them as upstream headers so admin-declared required fields reach
+ # the OpenAPI handler. Mirrors the local-registry dispatch path in
+ # ``server.execute_mcp_tool`` — without this, _enforce_user_fields
+ # would gate the call on the values being present but the values
+ # would be silently dropped before dispatch.
+ user_field_headers: Optional[Dict[str, str]] = None
+ from litellm.proxy._experimental.mcp_server.user_fields import (
+ resolve_user_field_headers,
+ )
+
+ stored_user_field_values = await self._resolve_user_field_values(
+ mcp_server, user_api_key_auth
+ )
+ if stored_user_field_values:
+ resolved = resolve_user_field_headers(
+ mcp_server, stored_user_field_values
+ )
+ if resolved:
+ user_field_headers = resolved
tasks.append(
asyncio.create_task(
- self._call_openapi_tool_handler(mcp_server, name, arguments)
+ self._call_openapi_tool_handler(
+ mcp_server,
+ name,
+ arguments,
+ user_field_headers=user_field_headers,
+ )
)
)
else:
@@ -2923,6 +3067,7 @@
proxy_logging_obj=proxy_logging_obj,
host_progress_callback=host_progress_callback,
hook_extra_headers=hook_result.get("extra_headers"),
+ user_api_key_auth=user_api_key_auth,
)
# For OpenAPI tools, await outside the client context
diff --git a/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py b/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py
--- a/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py
+++ b/litellm/proxy/_experimental/mcp_server/openapi_to_mcp_generator.py
@@ -62,7 +62,15 @@
contextvars.ContextVar("_request_extra_headers", default=None)
)
+# Per-request user-field headers resolved from MCPServer.user_fields and the
+# calling user's stored values. Set this ContextVar before calling a local
+# tool handler so admin-declared per-user values reach the upstream API even
+# when the tool dispatch goes through the OpenAPI/local registry path.
+_request_user_field_headers: contextvars.ContextVar[Optional[Dict[str, str]]] = (
+ contextvars.ContextVar("_request_user_field_headers", default=None)
+)
+
def _sanitize_path_parameter_value(param_value: Any, param_name: str) -> str:
"""Ensure path params cannot introduce directory traversal."""
if param_value is None:
@@ -311,23 +319,26 @@
Precedence (highest to lowest):
1. ``_request_auth_header`` — BYOK override of ``Authorization``
- 2. ``static_headers`` — operator-configured headers baked into the
+ 2. ``_request_user_field_headers`` — admin-declared per-user values
+ resolved from ``MCPServer.user_fields`` and the calling user's
+ stored values
+ 3. ``static_headers`` — operator-configured headers baked into the
tool closure at registration time
- 3. ``_request_extra_headers`` — per-request headers forwarded from
+ 4. ``_request_extra_headers`` — per-request headers forwarded from
the MCP caller (allowlisted by ``MCPServer.extra_headers``)
- This matches the existing MCP invariant in
- :func:`litellm.proxy._experimental.mcp_server.utils.merge_mcp_headers`
- and the managed MCP path, where ``static_headers`` always wins over
- caller-forwarded headers. Keeping the same precedence here prevents an
- authenticated caller from overriding an operator-configured value
- (e.g. a tenant id or upstream API key) by sending the same header name.
+ User-field headers win over ``static_headers`` because the admin
+ explicitly declared a field requiring a per-user value for that header
+ name. This matches the managed MCP dispatch path
+ (:meth:`MCPServerManager._call_regular_mcp_tool`), where user-field
+ headers are merged after ``static_headers``.
Header names are compared case-insensitively so different casing cannot
bypass the precedence rules.
"""
request_extra = _request_extra_headers.get() or {}
static = static_headers or {}
+ user_field = _request_user_field_headers.get() or {}
static_lower_names = {k.lower() for k in static}
... diff truncated: showing 800 of 3873 linesYou can send follow-ups to the cloud agent here. |
- Reject combining is_byok=True with non-empty user_fields at server create/update time: both share the (user_id, server_id) credential row and the store paths refuse cross-type overwrites, so the combination trapped users in an unresolvable 401 loop. - Make the managed MCP dispatch path apply user-field headers with the same case-insensitive precedence as the OpenAPI/local path, so a static_header named 'Authorization' is no longer left alongside a user-field header named 'authorization' on the outbound request.
Adds a regression test for the AttributeError fallback in resolve_user_field_headers when a corrupt JSONB row carries a non-string header_value_template. Bumps patch coverage past the codecov auto target.
…lock The exclusivity guard already rejected is_byok+user_fields because both serialize into the single credential_b64 row of LiteLLM_MCPUserCredentials and the write paths refuse to overwrite a foreign payload type. The same mutual exclusion applies to auth_type='oauth2' (interactive) — whichever credential the user saves first permanently locks the other, and the user has no way to escape without admin intervention. Extend the check to also reject auth_type=oauth2 combined with non-empty user_fields, and rename the helper to reflect that it covers more than the BYOK case. oauth2_token_exchange remains allowed because its tokens are cached in-memory (token_exchange.py) and never touch credential_b64.
| resolve_user_field_headers, | ||
| ) | ||
|
|
||
| stored_user_field_values = await self._resolve_user_field_values( |
There was a problem hiding this comment.
Medium: Required user-field enforcement bypass
MCPServerManager.call_tool is called directly by the Responses MCP path, not only through server.execute_mcp_tool. In this path, an authenticated user can invoke a server with required user_fields without saving those values first; the manager simply sends the request without the per-user headers/env instead of raising the missing-fields 401. Add the same compute_missing_user_fields enforcement in the manager before dispatching both OpenAPI and regular MCP tools.
There was a problem hiding this comment.
Already addressed on HEAD by commit 7218458 ("fix(mcp): enforce required user_fields in MCPServerManager.call_tool").
MCPServerManager._enforce_required_user_fields (added at mcp_server_manager.py:1374) now runs at the top of call_tool (mcp_server_manager.py:2997), before either dispatch path:
- the OpenAPI branch at
mcp_server_manager.py:3063(if mcp_server.spec_path: ... _call_openapi_tool_handler), and - the regular MCP client path that follows it.
It calls server_has_user_fields(mcp_server), resolves the caller's stored values via _resolve_user_field_values, then compute_missing_user_fields(...) and raises the same HTTPException(401, build_user_fields_missing_error(...)) payload (error="user_fields_missing" + config_url) that server.execute_mcp_tool raises.
Since the Responses API path (LiteLLM_Proxy_MCP_Handler._execute_tool_calls at litellm/responses/mcp/litellm_proxy_mcp_handler.py:813) goes through global_mcp_server_manager.call_tool, it now hits the same gate. Coverage in tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_user_fields.py exercises this directly.
|
Reply to greptile's summary comment (id 4484770031), Confidence-Score-blocking finding: "_validate_mcp_user_fields_exclusive needs to also cover the oauth2_token_exchange variant — combining user_fields with that auth type leaves users permanently deadlocked." This is a false positive. The two flows do not share any storage and the dispatch path for Why the deadlock claim doesn't apply to oauth2_token_exchange The @property
def needs_user_oauth_token(self) -> bool:
return self.auth_type == MCPAuth.oauth2 and not self.has_client_credentialsIt returns
The So for an admin who legitimately wants
No deadlock, no first-save failure, no missing recovery path. Why adding the guard would be net-negative
All other findings in the summary (CAS concurrency overhaul, |
The streamable-HTTP entrypoint (server.execute_mcp_tool) gated tool calls on a server's admin-declared user_fields, but the Responses API path (LiteLLM_Proxy_MCP_Handler) bypassed that gate by calling MCPServerManager.call_tool directly. With required fields unset, the manager just resolved an empty value map and dispatched anyway, so the upstream call went out without the configured headers/env. Move the enforcement into MCPServerManager.call_tool itself so every dispatch path raises the same friendly 401 (user_fields_missing + config_url) when required values are absent.
|
Reply to greptile's summary comment (id 4484770031), the two remaining concerns flagged at 4/5: 1. _validate_mcp_user_fields_exclusive doesn't cover oauth2_token_exchange. Already rebutted in detail at comment 4486592260. To recap: The write path that puts an OAuth2 access token into @property
def needs_user_oauth_token(self) -> bool:
return self.auth_type == MCPAuth.oauth2 and not self.has_client_credentialsReturns Extending the guard to 2. Dead The catch at
This is the same pattern used for the BYOK credential delete handler at line 2031 (
Removing the catch would surface a 500 on a concurrent double-delete instead of returning the same idempotent response the BYOK delete returns. Net-negative. All inline review comments (3263943728 / 3263943767 / 3263944066 / 3264150064 / 3264172511) are already addressed on HEAD per the earlier walk-through at comment 4486086015. |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 24 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 24 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
Closing this PR isn't a rejection of the change. We want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later"; your work is still here, the diff is still here, and getting it reopened is one comment away. Take your time. To bring this PR back:
What "end-to-end QA proof" means, since it's the most common gap: at least one of a short before/after screen recording / video (the bug reproducing, then the fix working; for a brand-new feature, a recording of it working end-to-end), a screenshot (or before/after screenshots) of it working, or the exact commands you ran paired with their real output against the real system. Running Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, comment |
4 participants
Generalizes the existing BYOK pattern (one user-provided credential per
server) to N admin-declared fields. Each field can target an HTTP header
(http/sse transports) or env var (stdio), with an optional value template
for prefixes like "Bearer {value}". Field values are encrypted at rest,
stored in the existing LiteLLM_MCPUserCredentials table with a
"type": "user_fields" discriminator so they don't collide with BYOK
strings or OAuth2 blobs.
Demo flow:
required field has a value.
returns HTTP 401 with error="user_fields_missing", the list of
missing field descriptors, and a config_url pointing at the
dashboard.
dispatch with the user's values injected as the configured headers
or env vars.
Backend
on the existing create/update/read models.
path as BYOK; a "type" discriminator keeps the three formats apart.
and GET /v1/mcp/user-field-values (aggregated for dashboard badges).
the dashboard config_url; the managed MCP dispatch path injects the
resolved headers and stdio env vars.
UI
for each affected server and opens a new UserFieldsModal where the
end-user fills in their values.
Note
Medium Risk
Touches MCP request dispatch and credential storage by multiplexing new encrypted user-field blobs into the existing
credential_b64row with new CAS write logic; bugs could impact auth/header injection or inadvertently block tool calls.Overview
Adds per-user field configuration for MCP servers. MCP server records gain a
user_fieldsJSONB column (with migration) and new types (MCPUserField, status/request models) to let admins declare required per-user values.Persists and enforces user field values. New DB helpers store/read/delete an encrypted
{"type":"user_fields"}payload in the existingLiteLLM_MCPUserCredentials.credential_b64row, add optimistic concurrency + conflict guards with BYOK/OAuth2, and update BYOK/OAuth2 write paths to avoid clobbering concurrent writes.Wires injection + UX end-to-end.
execute_mcp_toolnow returns a friendly 401 (user_fields_missing+config_url) when required fields are absent, injects resolved user-field headers/env-vars for both managed MCP and OpenAPI/local tool paths (new ContextVar), and management endpoints add GET/POST/DELETE per-server user-field routes plus an aggregate listing; the dashboard UI adds admin configuration, per-server missing-field badges, and a modal for users to save values (values never echoed back).Reviewed by Cursor Bugbot for commit 19b6872. Bugbot is set up for automated code reviews on this repo. Configure here.