mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-30 01:52:18 +00:00
fix(mcp): reconcile cold-start bypass with x-mcp-servers header and skip non-absolute WWW-Authenticate fabrication
- _parse_mcp_server_names_from_path now fails closed when the x-mcp-servers header introduces any target not present in the path-derived target set, closing a header/path mismatch where the cold-start passthrough bypass could otherwise admit anonymously while the header advertises a non-passthrough server. - MCPUpstreamAuthError.to_http_exception no longer emits a relative resource_metadata URI when base_url is missing; per RFC 9728 3.2 the URI must be absolute, so we skip fabrication entirely rather than send a challenge strict MCP clients will reject. Co-authored-by: Yassin Kortam <yassin@berri.ai>
This commit is contained in:
parent
172c11b352
commit
1d73f1bac5
3 changed files with 34 additions and 13 deletions
|
|
@ -18,13 +18,22 @@ from litellm.proxy.auth.user_api_key_auth import user_api_key_auth
|
|||
from litellm.proxy.auth.ip_address_utils import IPAddressUtils
|
||||
|
||||
|
||||
def _parse_mcp_server_names_from_path(path: str) -> Optional[List[str]]:
|
||||
def _parse_mcp_server_names_from_path(
|
||||
path: str, mcp_servers_header: Optional[List[str]] = None
|
||||
) -> Optional[List[str]]:
|
||||
"""Resolve the single MCP server name a cold-start passthrough bypass may
|
||||
target. Delegates parsing to
|
||||
:meth:`MCPRequestHandler._extract_target_server_names_from_path` so the
|
||||
names used here always match the names downstream routing uses; returns
|
||||
``None`` whenever the bypass must not activate (aggregate ``/mcp``,
|
||||
multi-server CSV paths, or any other unrecognized path)."""
|
||||
multi-server CSV paths, or any other unrecognized path).
|
||||
|
||||
Also fails closed when the ``x-mcp-servers`` header introduces any server
|
||||
not present in the path-derived target set. Downstream routing for
|
||||
``/mcp/...`` paths overrides the header with path-derived names, but a
|
||||
header/path mismatch here is a sign of a confused or hostile caller —
|
||||
refuse the cold-start bypass rather than admit anonymously based on the
|
||||
path while the header advertises a stricter, non-passthrough target."""
|
||||
servers = MCPRequestHandler._extract_target_server_names_from_path(path)
|
||||
if len(servers) != 1:
|
||||
verbose_logger.debug(
|
||||
|
|
@ -34,6 +43,14 @@ def _parse_mcp_server_names_from_path(path: str) -> Optional[List[str]]:
|
|||
servers,
|
||||
)
|
||||
return None
|
||||
if mcp_servers_header is not None and (set(mcp_servers_header) - set(servers)):
|
||||
verbose_logger.debug(
|
||||
"MCP cold-start: x-mcp-servers header %r introduces target(s) not "
|
||||
"in path-derived set %r; passthrough 401 bypass will not activate",
|
||||
mcp_servers_header,
|
||||
servers,
|
||||
)
|
||||
return None
|
||||
return servers
|
||||
|
||||
|
||||
|
|
@ -268,7 +285,7 @@ class MCPRequestHandler:
|
|||
# budget / rate limited) and must propagate so those
|
||||
# controls are not bypassed via anonymous admission.
|
||||
mcp_servers_from_path = _parse_mcp_server_names_from_path(
|
||||
request_route
|
||||
request_route, mcp_servers
|
||||
)
|
||||
if (
|
||||
mcp_servers_from_path is not None
|
||||
|
|
@ -300,7 +317,9 @@ class MCPRequestHandler:
|
|||
# require unauthenticated requests to protected resources to receive
|
||||
# 401 + WWW-Authenticate. Defer to _raise_preemptive_401_for_unauthenticated_servers
|
||||
# for pass-through servers instead of surfacing a generic admission error.
|
||||
mcp_servers_from_path = _parse_mcp_server_names_from_path(request_route)
|
||||
mcp_servers_from_path = _parse_mcp_server_names_from_path(
|
||||
request_route, mcp_servers
|
||||
)
|
||||
client_ip = IPAddressUtils.get_mcp_client_ip(request)
|
||||
if (
|
||||
mcp_servers_from_path is not None
|
||||
|
|
|
|||
|
|
@ -39,13 +39,14 @@ class MCPUpstreamAuthError(Exception):
|
|||
that points at the gateway's standard-pattern well-known endpoint for
|
||||
this server, so MCP clients can still initiate RFC 9728 discovery
|
||||
against the upstream IdP via the gateway's proxied metadata. Callers
|
||||
should pass ``base_url`` (the gateway origin, no trailing slash) so
|
||||
the fabricated URI is absolute as RFC 9728 §3.2 requires; strict
|
||||
clients reject relative URIs in the Bearer challenge.
|
||||
must pass ``base_url`` (the gateway origin, no trailing slash) so the
|
||||
fabricated URI is absolute as RFC 9728 §3.2 requires; if ``base_url``
|
||||
is missing we skip fabrication entirely rather than emit a relative
|
||||
URI that strict clients reject in the Bearer challenge.
|
||||
"""
|
||||
challenge: Optional[str] = self.www_authenticate
|
||||
if challenge is None and self.status_code == 401:
|
||||
prefix = base_url.rstrip("/") if base_url else ""
|
||||
if challenge is None and self.status_code == 401 and base_url:
|
||||
prefix = base_url.rstrip("/")
|
||||
challenge = (
|
||||
"Bearer resource_metadata="
|
||||
f'"{prefix}/.well-known/oauth-protected-resource/mcp/{self.server_name}"'
|
||||
|
|
|
|||
|
|
@ -126,7 +126,10 @@ def test_to_http_exception_preserves_upstream_www_authenticate():
|
|||
}
|
||||
|
||||
|
||||
def test_to_http_exception_fabricates_resource_metadata_when_upstream_omits_header():
|
||||
def test_to_http_exception_skips_fabrication_when_base_url_missing():
|
||||
"""Without ``base_url`` we cannot build an RFC 9728 §3.2-compliant absolute
|
||||
URI, so we omit the fabricated ``WWW-Authenticate`` challenge entirely
|
||||
instead of emitting a relative URI strict clients reject."""
|
||||
err = MCPUpstreamAuthError(
|
||||
status_code=401,
|
||||
www_authenticate=None,
|
||||
|
|
@ -135,9 +138,7 @@ def test_to_http_exception_fabricates_resource_metadata_when_upstream_omits_head
|
|||
|
||||
http_exc = err.to_http_exception()
|
||||
assert http_exc.status_code == 401
|
||||
assert http_exc.headers == {
|
||||
"www-authenticate": 'Bearer resource_metadata="/.well-known/oauth-protected-resource/mcp/sample_docs"'
|
||||
}
|
||||
assert http_exc.headers is None
|
||||
|
||||
|
||||
def test_to_http_exception_fabricates_absolute_resource_metadata_with_base_url():
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue