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 <cursoragent@cursor.com> * 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 <yassin@berri.ai> * 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 <yassin@berri.ai> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Yassin Kortam <yassin@berri.ai>
2026-08-06 10:21:32 +00:00 · 2026-05-22 09:00:39 -07:00
parent d96e26064f
commit 7a93cceb9f
2 changed files with 229 additions and 106 deletions
@@ -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:<port>/...",
+    )


 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://<host>/.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")
@@ -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, (