From 7a93cceb9f63de29923e987d20dbbdb4b8d25ec6 Mon Sep 17 00:00:00 2001 From: Sameer Kankute Date: Fri, 22 May 2026 21:30:39 +0530 Subject: [PATCH] Add error_description and hint for oauth flows (#28471) * Add error_description and hint for oauth flows * Fix tests * fix(mcp-oauth): improve redirect_uri errors without leaking internal config Use NoReturn on _oauth_invalid_request, structured errors for BYOK loopback validation, and refactor validate_trusted_redirect_uri to satisfy PLR0915. Keep PROXY_BASE_URL and raw proxy_base_url in server logs only, not in the HTTP 400 body returned to unauthenticated callers. Co-authored-by: Cursor * fix(mcp-oauth): stop leaking internal proxy origin in redirect_uri 400 body The trusted-redirect-uri rejection helper included the proxy's resolved scheme/host/port (e.g. http://litellm-internal:4000) in both the error_description and as a top-level proxy_origin field. Since the OAuth /authorize endpoint is unauthenticated, any caller could probe with a crafted redirect_uri and enumerate the internal network topology behind a reverse proxy. Keep full diagnostic detail in the server-side warning log (including the computed proxy base) but omit proxy-side values from the HTTP 400 body. Also drop the duplicated origin computation in _raise_trusted_redirect_uri_rejected now that those values are no longer needed by the response. Co-authored-by: Yassin Kortam * fix(mcp-oauth): remove dead userinfo check in redirect_uri validation The first check combined missing netloc with userinfo presence, making the second userinfo-only check unreachable. Split into two distinct checks so each error message reflects the actual failure mode. Co-authored-by: Yassin Kortam --------- Co-authored-by: Cursor Co-authored-by: Yassin Kortam --- .../_experimental/mcp_server/oauth_utils.py | 327 ++++++++++++------ .../mcp_server/test_discoverable_endpoints.py | 8 +- 2 files changed, 229 insertions(+), 106 deletions(-) diff --git a/litellm/proxy/_experimental/mcp_server/oauth_utils.py b/litellm/proxy/_experimental/mcp_server/oauth_utils.py index 09176f7253..e8b591c39c 100644 --- a/litellm/proxy/_experimental/mcp_server/oauth_utils.py +++ b/litellm/proxy/_experimental/mcp_server/oauth_utils.py @@ -3,8 +3,8 @@ import os from ipaddress import ip_address -from typing import List, Optional -from urllib.parse import urlparse, urlunparse +from typing import Any, Dict, List, NoReturn, Optional +from urllib.parse import ParseResult, urlparse, urlunparse from fastapi import HTTPException, Request @@ -43,6 +43,33 @@ _DEFAULT_NATIVE_REDIRECT_URIS: List[str] = [ _warned_invalid_proxy_base_url: Optional[str] = None +def _oauth_invalid_request( + error_description: str, + *, + hint: Optional[str] = None, + **extra: Any, +) -> NoReturn: + """Raise ``invalid_request`` (RFC 6749) with a debuggable description. + + FastAPI serializes ``detail`` as JSON. Callers still see ``error``: + ``invalid_request``; ``error_description`` and ``hint`` explain what + failed and how to fix it (e.g. reverse-proxy / PROXY_BASE_URL issues). + """ + detail: Dict[str, Any] = { + "error": "invalid_request", + "error_description": error_description, + } + if hint: + detail["hint"] = hint + detail.update(extra) + raise HTTPException(status_code=400, detail=detail) + + +def _origin_label(scheme: str, netloc: str) -> str: + """Human-readable origin for error messages (scheme + host[:port]).""" + return f"{scheme}://{netloc}" if netloc else f"{scheme}://" + + def _resolve_proxy_base_url_env() -> Optional[str]: global _warned_invalid_proxy_base_url configured = os.environ.get("PROXY_BASE_URL", "").strip() @@ -118,17 +145,15 @@ def validate_loopback_redirect_uri(redirect_uri: str) -> None: ``"127.0.0.1"`` alone would miss ``127.0.0.2`` and the full-form IPv6 loopback ``0:0:0:0:0:0:0:1``. """ - try: - parsed = urlparse(redirect_uri) - except ValueError: - raise HTTPException(status_code=400, detail="invalid_request") + parsed = _parse_redirect_uri_for_validation(redirect_uri) if parsed.scheme not in ("http", "https"): - raise HTTPException(status_code=400, detail="invalid_request") - # Fragments are not allowed in OAuth redirect URIs (RFC 6749 §3.1.2) - # — rejecting them prevents a ``http://127.0.0.1/cb#frag?code=...`` - # from silently eating the authorization code. + _oauth_invalid_request( + f"redirect_uri scheme {parsed.scheme!r} is not allowed; use http or https.", + ) if parsed.fragment: - raise HTTPException(status_code=400, detail="invalid_request") + _oauth_invalid_request( + "redirect_uri must not contain a URL fragment (#...).", + ) host = (parsed.hostname or "").lower() if host == "localhost": return @@ -139,7 +164,10 @@ def validate_loopback_redirect_uri(redirect_uri: str) -> None: # Unparseable host (malformed IPv6, etc.) — treat as invalid, # don't let it bubble up as a 500. pass - raise HTTPException(status_code=400, detail="invalid_request") + _oauth_invalid_request( + "redirect_uri must use a loopback host (localhost or 127.0.0.0/8).", + hint="Native MCP clients should register a callback on http://127.0.0.1:/...", + ) def _strip_default_port(scheme: str, netloc: str) -> str: @@ -293,6 +321,180 @@ def _matches_trusted_native_redirect_uri(parsed) -> bool: return False +def _parse_redirect_uri_for_validation(redirect_uri: str) -> ParseResult: + try: + return urlparse(redirect_uri) + except ValueError: + _oauth_invalid_request( + "redirect_uri is not a valid URL.", + hint="Use a full absolute URL for redirect_uri (e.g. https://your-host/ui/mcp/oauth/callback).", + ) + + +def _validate_trusted_http_redirect_shape(parsed: ParseResult) -> bool: + """Return True when ``parsed`` is an allowlisted native callback (caller may return).""" + if parsed.scheme not in ("http", "https"): + if _matches_trusted_native_redirect_uri(parsed): + return True + _oauth_invalid_request( + f"redirect_uri scheme {parsed.scheme!r} is not allowed; use http/https " + "or a registered native callback (e.g. cursor://).", + hint="Add the full URI to MCP_TRUSTED_NATIVE_REDIRECT_URIS for custom native clients.", + ) + if parsed.fragment: + _oauth_invalid_request( + "redirect_uri must not contain a URL fragment (#...).", + ) + if not parsed.netloc: + _oauth_invalid_request( + "redirect_uri must include a host (e.g. https://your-host/path).", + ) + if parsed.username is not None or parsed.password is not None: + _oauth_invalid_request( + "redirect_uri must not contain userinfo (user:pass@host).", + ) + if "\\" in parsed.netloc: + _oauth_invalid_request( + "redirect_uri host must not contain backslashes.", + ) + return False + + +def _resolve_proxy_base_for_redirect(request: Request) -> Optional[str]: + try: + return get_request_base_url(request) + except Exception as exc: + verbose_logger.warning( + "validate_trusted_redirect_uri: could not determine proxy origin, " + "falling back to loopback + allowlist. error=%s", + exc, + ) + return None + + +def _trusted_redirect_uri_is_allowed( + parsed: ParseResult, + redirect_netloc: str, + proxy_base: Optional[str], +) -> bool: + if proxy_base: + proxy_parsed = urlparse(proxy_base) + if ( + parsed.scheme == proxy_parsed.scheme + and redirect_netloc + == _strip_default_port(proxy_parsed.scheme, proxy_parsed.netloc) + ): + return True + + host = (parsed.hostname or "").lower() + if host == "localhost": + return True + try: + if ip_address(host).is_loopback: + return True + except ValueError: + pass + + if parsed.scheme == "https": + for entry in _parse_trusted_redirect_origins(): + if _matches_trusted_origin_entry(redirect_netloc, entry): + return True + return False + + +def _build_trusted_redirect_rejection_message( + redirect_uri: str, + parsed: ParseResult, + redirect_netloc: str, + proxy_base: Optional[str], +) -> str: + """Build a client-facing rejection message. + + Intentionally omits the proxy's resolved scheme / host / port to avoid + leaking internal network topology (e.g. ``http://litellm-internal:4000``) + through an unauthenticated endpoint. Full diagnostic detail — including + the computed proxy base — is logged server-side by the caller. + """ + redirect_origin = _origin_label(parsed.scheme, redirect_netloc) + proxy_parsed = urlparse(proxy_base) if proxy_base else None + proxy_netloc_norm = ( + _strip_default_port(proxy_parsed.scheme, proxy_parsed.netloc) + if proxy_parsed and proxy_parsed.netloc + else "" + ) + + mismatch_parts: List[str] = [] + if proxy_parsed and proxy_parsed.netloc: + if parsed.scheme != proxy_parsed.scheme: + mismatch_parts.append( + f"scheme: redirect_uri uses {parsed.scheme!r}, but the proxy " + "resolved a different scheme " + "(TLS often terminates at ingress — set PROXY_BASE_URL to https://… " + "or trust X-Forwarded-Proto from your ingress)" + ) + if redirect_netloc != proxy_netloc_norm: + mismatch_parts.append( + f"host/port: redirect_uri {redirect_netloc!r} does not match " + "the proxy origin" + ) + + if mismatch_parts: + return ( + f"redirect_uri origin ({redirect_origin}) does not match the proxy " + "origin. " + "; ".join(mismatch_parts) + ) + return ( + f"redirect_uri ({redirect_uri!r}) is not allowed: not same-origin with " + f"the proxy origin, not loopback, and not listed in " + f"{_TRUSTED_REDIRECT_ORIGINS_ENV}." + ) + + +def _raise_trusted_redirect_uri_rejected( + request: Request, + redirect_uri: str, + parsed: ParseResult, + redirect_netloc: str, + proxy_base: Optional[str], +) -> NoReturn: + description = _build_trusted_redirect_rejection_message( + redirect_uri, parsed, redirect_netloc, proxy_base + ) + + hint = ( + "Align the proxy public URL with the browser URL. Set PROXY_BASE_URL to your " + "HTTPS origin (e.g. https://litellm.example.com), or enable " + "general_settings.use_x_forwarded_for with mcp_trusted_proxy_ranges for your " + "ingress. Verify: curl https:///.well-known/oauth-authorization-server " + "| jq .issuer — issuer must match window.location.origin in the UI." + ) + + verbose_logger.warning( + "MCP OAuth: rejecting redirect_uri %r. %s " + "Computed proxy base=%r (PROXY_BASE_URL=%r). " + "Inbound headers: X-Forwarded-Proto=%r X-Forwarded-Host=%r " + "X-Forwarded-Port=%r Host=%r. " + "Trusted-redirect-origins env=%r. " + "Trusted-native-redirect-uris env=%r.", + redirect_uri, + description, + proxy_base, + os.environ.get("PROXY_BASE_URL"), + request.headers.get("X-Forwarded-Proto"), + request.headers.get("X-Forwarded-Host"), + request.headers.get("X-Forwarded-Port"), + request.headers.get("Host"), + os.environ.get(_TRUSTED_REDIRECT_ORIGINS_ENV), + os.environ.get(_TRUSTED_NATIVE_REDIRECT_URIS_ENV), + ) + + _oauth_invalid_request( + description, + hint=hint, + redirect_uri=redirect_uri, + ) + + def validate_trusted_redirect_uri(request: Request, redirect_uri: str) -> None: """Accept ``redirect_uri`` when it is (a) same-origin with the proxy's own request origin, (b) loopback, (c) listed in the @@ -316,98 +518,13 @@ def validate_trusted_redirect_uri(request: Request, redirect_uri: str) -> None: BYOK endpoints, which only serve native MCP clients, retain :func:`validate_loopback_redirect_uri`. """ - try: - parsed = urlparse(redirect_uri) - except ValueError: - raise HTTPException(status_code=400, detail="invalid_request") - if parsed.scheme not in ("http", "https"): - if _matches_trusted_native_redirect_uri(parsed): - return - raise HTTPException(status_code=400, detail="invalid_request") - if parsed.fragment: - raise HTTPException(status_code=400, detail="invalid_request") - if not parsed.netloc or parsed.username is not None or parsed.password is not None: - raise HTTPException(status_code=400, detail="invalid_request") - # Reject userinfo (``user:pass@host``) outright: OAuth redirect_uris - # have no legitimate reason to carry credentials, and allowing them - # opens a host-confusion attack where the netloc *looks* allowlisted - # (``app.example.com:443@attacker.example``) but the browser navigates - # to the post-``@`` host and hands the authorization code to the - # attacker. We compare against ``hostname`` after this, but defense in - # depth keeps malformed netloc strings from reaching the wildcard - # splitter. - if parsed.username is not None or parsed.password is not None: - raise HTTPException(status_code=400, detail="invalid_request") - # Reject backslash in netloc: urlparse keeps ``\`` as part of netloc, - # but browsers normalize ``\`` to ``/`` for http(s) URLs and treat it - # as the start of the path. An attacker can exploit that split by - # crafting ``https://attacker.net\app.example.com/cb`` — urlparse sees - # ``attacker.net\app.example.com`` (matches ``*.example.com``) while - # the browser navigates to ``attacker.net`` with the auth code. - if "\\" in parsed.netloc: - raise HTTPException(status_code=400, detail="invalid_request") - - redirect_netloc = _strip_default_port(parsed.scheme, parsed.netloc) - - # (a) Same-origin. Swallow ``get_request_base_url`` failures so the - # loopback + allowlist paths remain reachable when the origin can't - # be determined (e.g. request came from an untrusted proxy and - # ``get_request_base_url`` raised). - proxy_base: Optional[str] = None - try: - proxy_base = get_request_base_url(request) - except Exception as exc: - verbose_logger.warning( - "validate_trusted_redirect_uri: could not determine proxy origin, " - "falling back to loopback + allowlist. error=%s", - exc, - ) - proxy_base = None - if proxy_base: - proxy_parsed = urlparse(proxy_base) - if ( - parsed.scheme == proxy_parsed.scheme - and redirect_netloc - == _strip_default_port(proxy_parsed.scheme, proxy_parsed.netloc) - ): - return - - # (b) Loopback — same rule as validate_loopback_redirect_uri. - host = (parsed.hostname or "").lower() - if host == "localhost": + parsed = _parse_redirect_uri_for_validation(redirect_uri) + if _validate_trusted_http_redirect_shape(parsed): return - try: - if ip_address(host).is_loopback: - return - except ValueError: - pass - - # (c) Ops allowlist. https only. - if parsed.scheme == "https": - for entry in _parse_trusted_redirect_origins(): - if _matches_trusted_origin_entry(redirect_netloc, entry): - return - - verbose_logger.warning( - "MCP OAuth: rejecting redirect_uri %r as invalid_request. " - "Computed proxy base=%r (PROXY_BASE_URL=%r). " - "Inbound headers: X-Forwarded-Proto=%r X-Forwarded-Host=%r " - "X-Forwarded-Port=%r Host=%r. " - "Trusted-redirect-origins env=%r. " - "Trusted-native-redirect-uris env=%r. " - "If this should be accepted, either align ingress X-Forwarded-* " - "with the browser URL, set PROXY_BASE_URL to your public origin, " - "add the redirect_uri host to MCP_TRUSTED_REDIRECT_ORIGINS, or " - "for native MCP clients (cursor://, etc.) add the full redirect_uri " - "to MCP_TRUSTED_NATIVE_REDIRECT_URIS.", - redirect_uri, - proxy_base, - os.environ.get("PROXY_BASE_URL"), - request.headers.get("X-Forwarded-Proto"), - request.headers.get("X-Forwarded-Host"), - request.headers.get("X-Forwarded-Port"), - request.headers.get("Host"), - os.environ.get(_TRUSTED_REDIRECT_ORIGINS_ENV), - os.environ.get(_TRUSTED_NATIVE_REDIRECT_URIS_ENV), + redirect_netloc = _strip_default_port(parsed.scheme, parsed.netloc) + proxy_base = _resolve_proxy_base_for_redirect(request) + if _trusted_redirect_uri_is_allowed(parsed, redirect_netloc, proxy_base): + return + _raise_trusted_redirect_uri_rejected( + request, redirect_uri, parsed, redirect_netloc, proxy_base ) - raise HTTPException(status_code=400, detail="invalid_request") diff --git a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py index b06cc7f0f1..c8789e0b0a 100644 --- a/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py +++ b/tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py @@ -1345,7 +1345,13 @@ def test_validate_trusted_redirect_uri_logs_diagnostic_on_rejection( "https://litellm.example.com/ui/mcp/oauth/callback", ) assert exc_info.value.status_code == 400 - assert exc_info.value.detail == "invalid_request" + detail = exc_info.value.detail + assert isinstance(detail, dict) + assert detail.get("error") == "invalid_request" + assert "error_description" in detail + assert "redirect_uri origin" in detail["error_description"] + assert "proxy origin" in detail["error_description"] + assert "hint" in detail matching = [r for r in caplog.records if "rejecting redirect_uri" in r.getMessage()] assert len(matching) == 1, (