From b7fd2ec55063383871b51970afbcffc8a6eb1332 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Tue, 19 May 2026 09:41:57 +0000 Subject: [PATCH] fix(mcp): reject oauth2+user_fields admin config to prevent user deadlock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../mcp_management_endpoints.py | 40 +++++++++++++------ .../test_mcp_management_endpoints.py | 40 +++++++++++++++++++ 2 files changed, 68 insertions(+), 12 deletions(-) diff --git a/litellm/proxy/management_endpoints/mcp_management_endpoints.py b/litellm/proxy/management_endpoints/mcp_management_endpoints.py index 7e66b558d28..dc9ad5ed682 100644 --- a/litellm/proxy/management_endpoints/mcp_management_endpoints.py +++ b/litellm/proxy/management_endpoints/mcp_management_endpoints.py @@ -216,22 +216,24 @@ if MCP_AVAILABLE: def validate_and_normalize_mcp_server_payload(payload: Any) -> None: _base_validate_and_normalize_mcp_server_payload(payload) _validate_mcp_server_name_fields(payload) - _validate_mcp_user_fields_byok_exclusive(payload) + _validate_mcp_user_fields_exclusive(payload) - def _validate_mcp_user_fields_byok_exclusive(payload: Any) -> None: - """Reject server configurations that combine is_byok with user_fields. + def _validate_mcp_user_fields_exclusive(payload: Any) -> None: + """Reject server configurations that combine user_fields with another + credential type that shares the same per-user storage row. - Both credential types share the same (user_id, server_id) row in - LiteLLM_MCPUserCredentials, and the store paths refuse to overwrite - the other type — so a user can save BYOK or user-fields for a given - server, never both. Allowing this combination at admin time would - trap end-users in an unresolvable state: every tool call would 401 - on whichever check the user has not (and cannot) satisfy. + BYOK strings, OAuth2 access tokens, and user_fields blobs all encode + into the single ``credential_b64`` column of LiteLLM_MCPUserCredentials, + and the write paths refuse to overwrite a different type. Mixing + ``is_byok=True`` or ``auth_type=oauth2`` with non-empty ``user_fields`` + would let whichever credential the user saves first permanently block + the other — every tool call would 401 on the unsatisfied check, and + the user has no path to resolve it without admin intervention. """ - if not getattr(payload, "is_byok", False): - return user_fields = getattr(payload, "user_fields", None) or [] - if user_fields: + if not user_fields: + return + if getattr(payload, "is_byok", False): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail={ @@ -243,6 +245,20 @@ if MCP_AVAILABLE: ) }, ) + if getattr(payload, "auth_type", None) == MCPAuth.oauth2: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail={ + "error": ( + "MCP servers cannot combine auth_type='oauth2' with " + "user_fields. The interactive OAuth2 token and " + "user_fields share the same per-user credential row, " + "so saving one would block saving the other. Use " + "user_fields for per-user secrets or oauth2 for the " + "primary upstream auth, not both." + ) + }, + ) _VALID_MCP_REQUIRED_FIELDS: frozenset = frozenset(NewMCPServerRequest.model_fields) 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 5d66c184495..5ffa0359846 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 @@ -2468,6 +2468,46 @@ class TestManagementPayloadValidation: assert payload.alias == "valid_server" + def test_rejects_is_byok_with_user_fields(self): + payload = SimpleNamespace( + server_name="valid_server", + alias=None, + is_byok=True, + user_fields=[{"field_key": "TOKEN", "header_name": "Authorization"}], + ) + + with pytest.raises(HTTPException) as exc_info: + mgmt_endpoints.validate_and_normalize_mcp_server_payload(payload) + + assert exc_info.value.status_code == 400 + assert "is_byok and user_fields" in exc_info.value.detail["error"] + + def test_rejects_oauth2_with_user_fields(self): + payload = SimpleNamespace( + server_name="valid_server", + alias=None, + is_byok=False, + auth_type=MCPAuth.oauth2, + user_fields=[{"field_key": "WORKSPACE", "header_name": "X-Workspace"}], + ) + + with pytest.raises(HTTPException) as exc_info: + mgmt_endpoints.validate_and_normalize_mcp_server_payload(payload) + + assert exc_info.value.status_code == 400 + assert "oauth2" in exc_info.value.detail["error"] + + def test_accepts_oauth2_token_exchange_with_user_fields(self): + payload = SimpleNamespace( + server_name="valid_server", + alias=None, + is_byok=False, + auth_type=MCPAuth.oauth2_token_exchange, + user_fields=[{"field_key": "WORKSPACE", "header_name": "X-Workspace"}], + ) + + mgmt_endpoints.validate_and_normalize_mcp_server_payload(payload) + @pytest.mark.asyncio async def test_health_check_view_all_mode(self): """view_all mode should return health info for all MCP servers."""