mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-25 01:02:15 +00:00
fix(mcp): correct passthrough probe 401 + slashed-name cold start parser
- _check_passthrough_upstream_auth now emits 'Bearer resource_metadata="..."' pointing at the gateway's oauth-protected-resource well-known URL, mirroring the pre-emptive 401 path. Pass-through servers don't use the gateway as an authorization server, so the previous 'authorization_uri=' challenge sent clients to the wrong metadata endpoint. - _parse_mcp_server_names_from_path now accepts server names that contain a single slash (e.g. custom_solutions/user_123), mirroring MCPRequestHandler._extract_target_server_names_from_path. Without this, the cold-start bypass missed slashed-name servers and the generic admission error propagated instead of the spec-compliant 401 challenge. - _is_mcp_passthrough_cold_start drops the unused scope parameter from its signature. Co-authored-by: Yassin Kortam <yassin@berri.ai>
This commit is contained in:
parent
ee38ba16e3
commit
f0eb54ea9f
3 changed files with 54 additions and 22 deletions
|
|
@ -23,14 +23,29 @@ def _parse_mcp_server_names_from_path(path: str) -> Optional[List[str]]:
|
|||
|
||||
Multi-server CSV paths like ``/mcp/server1,server2`` also return ``None`` —
|
||||
the cold-start bypass must not activate when any of the co-targeted servers
|
||||
might not be passthrough-eligible. The regex stops at ``/?#`` only; the
|
||||
comma check is handled explicitly below."""
|
||||
m = re.match(r"^/mcp/([^/?#]+)", path)
|
||||
if m:
|
||||
segment = m.group(1)
|
||||
if "," in segment:
|
||||
might not be passthrough-eligible.
|
||||
|
||||
Server names may contain a single slash (e.g. ``custom_solutions/user_123``),
|
||||
so the ``/mcp/...`` form must mirror
|
||||
:meth:`MCPRequestHandler._extract_target_server_names_from_path` rather than
|
||||
stop at the first ``/``. Otherwise the cold-start lookup would miss
|
||||
slashed-name servers and fall through to the generic admission error
|
||||
instead of the spec-compliant 401 challenge."""
|
||||
mcp_path_match = re.match(r"^/mcp/([^?#]+)", path)
|
||||
if mcp_path_match:
|
||||
servers_and_path = mcp_path_match.group(1)
|
||||
if not servers_and_path:
|
||||
return None
|
||||
return [segment]
|
||||
if "," in servers_and_path:
|
||||
return None
|
||||
# Server name may contain at most one slash; strip any trailing
|
||||
# path segments so the registry lookup uses the canonical name.
|
||||
single_server_match = re.match(
|
||||
r"^([^/]+(?:/[^/]+)?)(?:/.*)?$", servers_and_path
|
||||
)
|
||||
if single_server_match:
|
||||
return [single_server_match.group(1)]
|
||||
return [servers_and_path]
|
||||
m = re.match(r"^/([^/,?#]+)/mcp", path)
|
||||
if m:
|
||||
return [m.group(1)]
|
||||
|
|
@ -38,7 +53,7 @@ def _parse_mcp_server_names_from_path(path: str) -> Optional[List[str]]:
|
|||
|
||||
|
||||
def _is_mcp_passthrough_cold_start(
|
||||
scope: Scope, mcp_servers: Optional[List[str]], client_ip: Optional[str]
|
||||
mcp_servers: Optional[List[str]], client_ip: Optional[str]
|
||||
) -> bool:
|
||||
"""True only when EVERY targeted server is a pass-through server with no
|
||||
auth headers — the cold-start OAuth discovery case per RFC 9728 / MCP
|
||||
|
|
@ -272,7 +287,7 @@ class MCPRequestHandler:
|
|||
)
|
||||
and _is_litellm_auth_admission_error(exc)
|
||||
and _is_mcp_passthrough_cold_start(
|
||||
scope, mcp_servers_from_path, client_ip=client_ip
|
||||
mcp_servers_from_path, client_ip=client_ip
|
||||
)
|
||||
):
|
||||
verbose_logger.debug(
|
||||
|
|
|
|||
|
|
@ -3043,17 +3043,32 @@ if MCP_AVAILABLE:
|
|||
)
|
||||
request = StarletteRequest(scope)
|
||||
base_url = get_request_base_url(request)
|
||||
_path = scope.get("_original_path") or scope.get("path", "") or ""
|
||||
for srv, (probe_status, _) in zip(passthrough_servers, probe_results):
|
||||
if probe_status == 401:
|
||||
# Token is missing or expired — direct the client to re-authorize.
|
||||
authorization_uri = (
|
||||
f"Bearer authorization_uri="
|
||||
f"{base_url}/.well-known/oauth-authorization-server/{srv.name}"
|
||||
)
|
||||
# Token is missing or expired — direct the client at the
|
||||
# gateway's oauth-protected-resource well-known URL (which
|
||||
# proxies the upstream IdP's metadata), matching the
|
||||
# pre-emptive 401 path in
|
||||
# _raise_preemptive_401_for_unauthenticated_servers. The
|
||||
# gateway is not the authorization server for pass-through
|
||||
# servers, so emitting ``authorization_uri=`` would point
|
||||
# clients at the wrong AS metadata.
|
||||
if _path.startswith(f"/{srv.name}/mcp"):
|
||||
resource_metadata_url = (
|
||||
f"{base_url}/.well-known/oauth-protected-resource/"
|
||||
f"{srv.name}/mcp"
|
||||
)
|
||||
else:
|
||||
resource_metadata_url = (
|
||||
f"{base_url}/.well-known/oauth-protected-resource/mcp/"
|
||||
f"{srv.name}"
|
||||
)
|
||||
www_authenticate = f'Bearer resource_metadata="{resource_metadata_url}"'
|
||||
raise HTTPException(
|
||||
status_code=401,
|
||||
detail="Unauthorized",
|
||||
headers={"WWW-Authenticate": authorization_uri},
|
||||
headers={"WWW-Authenticate": www_authenticate},
|
||||
)
|
||||
if probe_status == 403:
|
||||
# Token is valid but the caller lacks permission — do not hint
|
||||
|
|
|
|||
|
|
@ -72,7 +72,7 @@ def test_passthrough_cold_start_emits_401_with_matching_resource_metadata(
|
|||
scope["_original_path"] = route
|
||||
|
||||
servers = _parse_mcp_server_names_from_path(scope.get("path", ""))
|
||||
assert _is_mcp_passthrough_cold_start(scope, servers, client_ip=None) is True
|
||||
assert _is_mcp_passthrough_cold_start(servers, client_ip=None) is True
|
||||
|
||||
server_name = "sample_docs"
|
||||
base_url = "http://localhost:4000"
|
||||
|
|
@ -117,8 +117,7 @@ def test_is_mcp_passthrough_cold_start_false_for_oauth2_server():
|
|||
)
|
||||
global_mcp_server_manager.registry[oauth2_server.server_id] = oauth2_server
|
||||
|
||||
scope = _make_scope("/mcp/keycloak_whoami")
|
||||
result = _is_mcp_passthrough_cold_start(scope, ["keycloak_whoami"], client_ip=None)
|
||||
result = _is_mcp_passthrough_cold_start(["keycloak_whoami"], client_ip=None)
|
||||
assert result is False
|
||||
|
||||
|
||||
|
|
@ -128,16 +127,19 @@ def test_is_mcp_passthrough_cold_start_false_for_empty_servers():
|
|||
_is_mcp_passthrough_cold_start,
|
||||
)
|
||||
|
||||
scope = _make_scope("/mcp")
|
||||
assert _is_mcp_passthrough_cold_start(scope, None, client_ip=None) is False
|
||||
assert _is_mcp_passthrough_cold_start(scope, [], client_ip=None) is False
|
||||
assert _is_mcp_passthrough_cold_start(None, client_ip=None) is False
|
||||
assert _is_mcp_passthrough_cold_start([], client_ip=None) is False
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"path,expected",
|
||||
[
|
||||
("/mcp/sample_docs", ["sample_docs"]),
|
||||
("/mcp/sample_docs/tools/list", ["sample_docs"]),
|
||||
# Server names may contain at most one slash (mirrors
|
||||
# ``_extract_target_server_names_from_path``), so when more than two
|
||||
# segments follow ``/mcp/`` the first two are treated as the name.
|
||||
("/mcp/sample_docs/tools/list", ["sample_docs/tools"]),
|
||||
("/mcp/custom_solutions/user_123", ["custom_solutions/user_123"]),
|
||||
("/sample_docs/mcp", ["sample_docs"]),
|
||||
("/sample_docs/mcp/tools/list", ["sample_docs"]),
|
||||
("/mcp", None),
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue