diff --git a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py index baa4a5a9e0c1..bb54d0f885a3 100644 --- a/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py +++ b/litellm/proxy/_experimental/mcp_server/mcp_server_manager.py @@ -3352,27 +3352,31 @@ async def _call_regular_mcp_tool( if hook_extra_headers: if extra_headers is None: extra_headers = {} - if "Authorization" in hook_extra_headers: - 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." - ) - elif server_auth_header is not None: - # server_auth_header is passed separately to _create_mcp_client as - # auth_value. Both will reach the upstream server — warn so admins - # know two Authorization credentials are being sent. - verbose_logger.warning( - "MCPServerManager: hook_extra_headers injects 'Authorization' while " - "server '%s' already has a configured authentication_token. " - "Both credentials will be sent; the hook header is in extra_headers " - "and the server token is in auth_value — the upstream server decides " - "which one wins. Consider unsetting authentication_token if you want " - "the hook JWT to be the sole credential.", - mcp_server.server_name or mcp_server.name, - ) - extra_headers.update(hook_extra_headers) + # Per-user OAuth and admin-configured static Authorization must take + # precedence over the signer's JWT (matches the tools/list guard) — skip the + # hook Authorization when extra_headers already carries one so the signed JWT + # doesn't clobber the resolved per-user token. + has_existing_authorization = any( + isinstance(k, str) and k.lower() == "authorization" for k in extra_headers.keys() + ) + for header, value in hook_extra_headers.items(): + if isinstance(header, str) and header.lower() == "authorization": + if has_existing_authorization: + continue + if server_auth_header is not None: + # server_auth_header is passed separately to _create_mcp_client as + # auth_value. Both will reach the upstream server — warn so admins + # know two Authorization credentials are being sent. + verbose_logger.warning( + "MCPServerManager: hook_extra_headers injects 'Authorization' while " + "server '%s' already has a configured authentication_token. " + "Both credentials will be sent; the hook header is in extra_headers " + "and the server token is in auth_value — the upstream server decides " + "which one wins. Consider unsetting authentication_token if you want " + "the hook JWT to be the sole credential.", + mcp_server.server_name or mcp_server.name, + ) + extra_headers[header] = value # Reset to None if no headers were actually added if extra_headers is not None and len(extra_headers) == 0: diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py index 363948ff4e61..24d7ba31bd75 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_hook_extra_headers.py @@ -536,14 +536,15 @@ def _make_server( ) @pytest.mark.asyncio - async def test_hook_headers_override_static_headers(self): - """Hook headers should take precedence over static_headers.""" + async def test_hook_non_auth_headers_merge_but_static_authorization_wins(self): + """Hook non-Authorization headers merge in, but an existing static + Authorization must survive the hook (JWT) merge (matches tools/list).""" manager = MCPServerManager() server = self._make_server( static_headers={"Authorization": "Bearer static-token", "X-Static": "yes"} ) - hook_headers = {"Authorization": "Bearer hook-signed-jwt"} + hook_headers = {"Authorization": "Bearer hook-signed-jwt", "X-Trace-Id": "t1"} captured_extra_headers: Dict[str, Any] = {} @@ -576,8 +577,9 @@ async def fake_create_mcp_client( pass headers = captured_extra_headers.get("value", {}) - assert headers["Authorization"] == "Bearer hook-signed-jwt" + assert headers["Authorization"] == "Bearer static-token" assert headers["X-Static"] == "yes" + assert headers["X-Trace-Id"] == "t1" @pytest.mark.asyncio async def test_no_hook_headers_preserves_existing_behavior(self): @@ -619,8 +621,18 @@ async def fake_create_mcp_client( assert headers == {"X-Static": "static-value"} @pytest.mark.asyncio - async def test_hook_headers_merge_with_oauth2(self): - """Hook headers merge on top of OAuth2 headers.""" + async def test_hook_headers_do_not_overwrite_per_user_oauth_authorization(self): + """Regression for #31977: a per-user OAuth Authorization already resolved + into extra_headers must survive the MCP JWT signer (hook) merge on the + tools/call path, matching the precedence enforced on tools/list. The + hook's non-Authorization headers still merge in. + + Uses delegate_auth_to_upstream so the server stays on the v1 path + (to_server_spec is None) where the caller's OAuth token is NOT stripped + and therefore legitimately lives in extra_headers at the hook-merge + point — the exact scenario the reporter hits (tools/list works because + that guard already skips the signer when Authorization exists; tools/call + was clobbering it).""" manager = MCPServerManager() server = MCPServer( server_id="test-id", @@ -629,6 +641,7 @@ async def test_hook_headers_merge_with_oauth2(self): url="https://example.com", transport=MCPTransport.http, auth_type=MCPAuth.oauth2, + delegate_auth_to_upstream=True, ) captured_extra_headers: Dict[str, Any] = {} @@ -668,7 +681,7 @@ async def fake_create_mcp_client( pass headers = captured_extra_headers.get("value", {}) - assert headers["Authorization"] == "Bearer hook-jwt" + assert headers["Authorization"] == "Bearer oauth2-token" assert headers["X-OAuth"] == "yes" assert headers["X-Trace-Id"] == "trace-123"