Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 25 additions & 21 deletions litellm/proxy/_experimental/mcp_server/mcp_server_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +3363 to +3365

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 When the hook's Authorization is silently skipped because an existing one was already in extra_headers, nothing is logged. A debug-level message here would help operators understand why the signer JWT was not forwarded, matching the level of observability that the old warning provided for the inverse scenario.

Suggested change
if isinstance(header, str) and header.lower() == "authorization":
if has_existing_authorization:
continue
if isinstance(header, str) and header.lower() == "authorization":
if has_existing_authorization:
verbose_logger.debug(
"MCPServerManager: hook_extra_headers 'Authorization' skipped — "
"an existing Authorization header (per-user OAuth or static) takes precedence."
)
continue

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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] = {}

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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",
Expand All @@ -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] = {}
Expand Down Expand Up @@ -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"

Expand Down
Loading