From b80a91ffc54fe8b11e12361d0f0cbc8e2a2e4545 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 21 May 2026 18:11:35 +0000 Subject: [PATCH] refactor(mcp): centralize caller Authorization strip decision Extracted the security-sensitive logic that decides whether the caller's Authorization header is forwarded to (or stripped from) an outgoing MCP request into a single helper, _should_strip_caller_authorization, in mcp_server_manager.py. Previously the same condition was duplicated across _call_regular_mcp_tool (mcp_server_manager.py) and _prepare_mcp_server_headers (server.py). Keeping two copies of this check risked future divergence and credential-leak / broken-passthrough bugs. Both call sites now share the helper, preserving exact behavior. Co-authored-by: Yassin Kortam --- .../mcp_server/mcp_server_manager.py | 70 ++++++++++++++----- .../proxy/_experimental/mcp_server/server.py | 48 +++---------- 2 files changed, 63 insertions(+), 55 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index 53ca189905e..a9054a9d846 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -118,6 +118,52 @@ _AZURE_ENTRA_HOSTS = { } +def _should_strip_caller_authorization( + mcp_server: MCPServer, + raw_headers: Optional[Dict[str, str]], + user_api_key_auth: Optional[UserAPIKeyAuth], +) -> bool: + """Decide whether the caller's ``Authorization`` header must NOT be + forwarded upstream when populating ``extra_headers`` for an MCP server. + + Centralized so ``_call_regular_mcp_tool`` (this module) and + ``_prepare_mcp_server_headers`` (``server.py``) cannot drift apart on + this security-sensitive decision. + + Strip rules: + - **M2M (client_credentials) servers**: never forward the caller's + ``Authorization`` — the proxy fetches its own upstream token. + - **OAuth pass-through servers**: strip when the ``Authorization`` + header is actually the LiteLLM API key — either because admission + validated it (``user_api_key_auth.api_key`` is set) and the caller + did NOT also supply ``x-litellm-api-key`` to disambiguate, or + because the legacy ``user_api_key_auth is None`` call sites did + not supply an explicit admission header. In the anonymous / + pass-through cold-start case (RFC 9728) the bearer in + ``Authorization`` is the upstream OAuth token and must be + forwarded, so we keep it. + """ + if mcp_server.has_client_credentials: + return True + if not mcp_server.is_oauth_passthrough: + return False + + normalized_raw_headers = { + str(k).lower(): v for k, v in (raw_headers or {}).items() if isinstance(k, str) + } + has_explicit_litellm_admission_header = ( + normalized_raw_headers.get("x-litellm-api-key") is not None + ) + admission_consumed_authorization_as_litellm_key = ( + user_api_key_auth is not None + and bool(getattr(user_api_key_auth, "api_key", None)) + and not has_explicit_litellm_admission_header + ) + return admission_consumed_authorization_as_litellm_key or ( + user_api_key_auth is None and not has_explicit_litellm_admission_header + ) + + def _extract_upstream_auth_failure( exc: BaseException, ) -> Optional[Tuple[int, Optional[str]]]: @@ -2829,29 +2875,17 @@ class MCPServerManager: normalized_raw_headers = { str(k).lower(): v for k, v in raw_headers.items() if isinstance(k, str) } - has_explicit_litellm_admission_header = ( - normalized_raw_headers.get("x-litellm-api-key") is not None - ) - admission_consumed_authorization_as_litellm_key = ( - user_api_key_auth is not None - and bool(getattr(user_api_key_auth, "api_key", None)) - and not has_explicit_litellm_admission_header + strip_caller_authorization = _should_strip_caller_authorization( + mcp_server=mcp_server, + raw_headers=raw_headers, + user_api_key_auth=user_api_key_auth, ) for header in mcp_server.extra_headers: if not isinstance(header, str): continue - if header.lower() == "authorization": - if mcp_server.has_client_credentials: - continue - if mcp_server.is_oauth_passthrough and ( - admission_consumed_authorization_as_litellm_key - or ( - user_api_key_auth is None - and not has_explicit_litellm_admission_header - ) - ): - continue + if header.lower() == "authorization" and strip_caller_authorization: + continue header_value = normalized_raw_headers.get(header.lower()) if header_value is None: continue diff --git a/litellm/proxy/_experimental/mcp_server/server.py b/litellm/proxy/_experimental/mcp_server/server.py index 8ca2be87bf7..9a92e7fe080 100644 --- a/litellm/proxy/_experimental/mcp_server/server.py +++ b/litellm/proxy/_experimental/mcp_server/server.py @@ -160,6 +160,7 @@ if MCP_AVAILABLE: ) from litellm.proxy._experimental.mcp_server.mcp_server_manager import ( MCPServerManager, + _should_strip_caller_authorization, global_mcp_server_manager, ) from litellm.proxy._experimental.mcp_server.openapi_to_mcp_generator import ( @@ -1175,48 +1176,21 @@ if MCP_AVAILABLE: str(k).lower(): v for k, v in raw_headers.items() if isinstance(k, str) } - has_explicit_litellm_admission_header = ( - normalized_raw_headers.get("x-litellm-api-key") is not None - ) - # Admission consumed ``Authorization`` as a LiteLLM key only when - # auth produced a validated ``api_key`` AND the caller did not - # supply ``x-litellm-api-key``. When admission was anonymous - # (e.g. pass-through cold-start return per RFC 9728), the bearer - # in ``Authorization`` is the upstream OAuth token and must be - # forwarded — not stripped — for the delegated flow to work. - admission_consumed_authorization_as_litellm_key = ( - user_api_key_auth is not None - and bool(getattr(user_api_key_auth, "api_key", None)) - and not has_explicit_litellm_admission_header + # Centralized strip decision shared with + # ``MCPServerManager._call_regular_mcp_tool`` so the two + # code paths cannot drift on this security-sensitive choice. + # See ``_should_strip_caller_authorization`` for the rules. + strip_caller_authorization = _should_strip_caller_authorization( + mcp_server=server, + raw_headers=raw_headers, + user_api_key_auth=user_api_key_auth, ) for header in server.extra_headers: if not isinstance(header, str): continue - if header.lower() == "authorization": - # M2M servers fetch their own upstream token via the - # client_credentials flow — never forward the caller's - # Authorization header. - if server.has_client_credentials: - continue - # Transparent OAuth pass-through: forward the caller's - # Authorization header only when LiteLLM admission used - # a different header (`x-litellm-api-key`) or when - # admission was anonymous (delegated to upstream). - # Without that signal `Authorization` may itself be the - # LiteLLM key — strip it to avoid leaking the gateway - # credential upstream. The legacy ``user_api_key_auth - # is None`` callers keep the conservative pre-PR - # behavior of stripping when no explicit admission - # header was supplied. - if server.is_oauth_passthrough and ( - admission_consumed_authorization_as_litellm_key - or ( - user_api_key_auth is None - and not has_explicit_litellm_admission_header - ) - ): - continue + if header.lower() == "authorization" and strip_caller_authorization: + continue header_value = normalized_raw_headers.get(header.lower()) if header_value is None: continue