Add error_description and hint for oauth flows#28471
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR improves MCP OAuth error responses by replacing plain-string
Confidence Score: 5/5Safe to merge — the change only affects error response shape on the rejection path of an experimental OAuth endpoint, with all previously flagged concerns (NoReturn annotation, PROXY_BASE_URL leakage, loopback inconsistency) addressed in this revision. The logic for accepting or rejecting redirect URIs is unchanged; only the error payload on the rejection path is modified. Internal proxy topology is kept out of HTTP responses by design, logged server-side instead. The test update reflects the genuine new contract and is strictly more specific than before. No authentication bypass, no data mutation, and no new network calls are introduced. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/oauth_utils.py | Refactors OAuth redirect URI validation into focused helper functions and replaces plain-string "invalid_request" errors with structured RFC 6749 JSON responses (error + error_description + hint). The _oauth_invalid_request helper is correctly annotated -> NoReturn; _build_trusted_redirect_rejection_message intentionally omits proxy internals from the HTTP body while logging full diagnostic detail server-side. Both validate_trusted_redirect_uri and validate_loopback_redirect_uri are updated consistently. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Test updated to assert the new structured error dict shape (error, error_description, hint) instead of the old plain-string check. The new assertions are strictly more specific and correctly reflect the changed contract, not weakened coverage. |
Reviews (3): Last reviewed commit: "fix(mcp-oauth): remove dead userinfo che..." | Re-trigger Greptile
…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 <[email protected]>
…0 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 <[email protected]>
|
|
|
@greptileai rereview |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high mode and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Unreachable dead code with misleading error message
- Split the combined netloc+userinfo check into two distinct checks so the userinfo error path is reachable and each message reflects the actual failure mode.
Preview (a31111e8d7)
diff --git a/litellm/proxy/_experimental/mcp_server/oauth_utils.py b/litellm/proxy/_experimental/mcp_server/oauth_utils.py
--- 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 @@
_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 @@
``"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 @@
# 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,76 +321,62 @@
return False
-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
- ``MCP_TRUSTED_REDIRECT_ORIGINS`` ops allowlist, or (d) a built-in /
- env-configured native MCP client callback (e.g. ``cursor://``).
+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).",
+ )
- Same-origin is VERIA-57's threat-model-safe equivalent of loopback:
- an attacker who can host content on the proxy's own HTTPS origin
- has already compromised the proxy, so the open-redirect + code-
- theft primitive that motivated the loopback-only rule does not
- apply. The same reasoning extends to ops-trusted first-party
- hosts (e.g. an internal web app registering as an OAuth client of
- the proxy on a sister domain).
- Allowlisted non-loopback hosts are accepted only when the
- redirect_uri scheme is ``https`` — an attacker on the network
- cannot elevate to https without controlling the host's TLS key.
-
- Use this in the discoverable OAuth proxy endpoints that serve both
- native clients and the proxy's UI / cross-origin web clients. The
- 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")
+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
- raise HTTPException(status_code=400, detail="invalid_request")
+ 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:
- 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:[email protected]``) 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.
+ _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:
- 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.
+ _oauth_invalid_request(
+ "redirect_uri must not contain userinfo (user:pass@host).",
+ )
if "\\" in parsed.netloc:
- raise HTTPException(status_code=400, detail="invalid_request")
+ _oauth_invalid_request(
+ "redirect_uri host must not contain backslashes.",
+ )
+ return False
- 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
+def _resolve_proxy_base_for_redirect(request: Request) -> Optional[str]:
try:
- proxy_base = get_request_base_url(request)
+ 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,
)
- proxy_base = None
+ 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 (
@@ -370,37 +384,100 @@
and redirect_netloc
== _strip_default_port(proxy_parsed.scheme, proxy_parsed.netloc)
):
- return
+ return True
- # (b) Loopback — same rule as validate_loopback_redirect_uri.
host = (parsed.hostname or "").lower()
if host == "localhost":
- return
+ return True
try:
if ip_address(host).is_loopback:
- return
+ return True
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
+ 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 as invalid_request. "
+ "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. "
- "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.",
+ "Trusted-native-redirect-uris env=%r.",
redirect_uri,
+ description,
proxy_base,
os.environ.get("PROXY_BASE_URL"),
request.headers.get("X-Forwarded-Proto"),
@@ -410,4 +487,44 @@
os.environ.get(_TRUSTED_REDIRECT_ORIGINS_ENV),
os.environ.get(_TRUSTED_NATIVE_REDIRECT_URIS_ENV),
)
- raise HTTPException(status_code=400, detail="invalid_request")
+
+ _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
+ ``MCP_TRUSTED_REDIRECT_ORIGINS`` ops allowlist, or (d) a built-in /
+ env-configured native MCP client callback (e.g. ``cursor://``).
+
+ Same-origin is VERIA-57's threat-model-safe equivalent of loopback:
+ an attacker who can host content on the proxy's own HTTPS origin
+ has already compromised the proxy, so the open-redirect + code-
+ theft primitive that motivated the loopback-only rule does not
+ apply. The same reasoning extends to ops-trusted first-party
+ hosts (e.g. an internal web app registering as an OAuth client of
+ the proxy on a sister domain).
+
+ Allowlisted non-loopback hosts are accepted only when the
+ redirect_uri scheme is ``https`` — an attacker on the network
+ cannot elevate to https without controlling the host's TLS key.
+
+ Use this in the discoverable OAuth proxy endpoints that serve both
+ native clients and the proxy's UI / cross-origin web clients. The
+ BYOK endpoints, which only serve native MCP clients, retain
+ :func:`validate_loopback_redirect_uri`.
+ """
+ parsed = _parse_redirect_uri_for_validation(redirect_uri)
+ if _validate_trusted_http_redirect_shape(parsed):
+ return
+ 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
+ )
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
--- 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 @@
"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, (You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit bb800ab. Configure here.
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 <[email protected]>
|
@greptileai rereview |
|
@Sameerlite — could you add a screenshot or short video showing that this change works as expected? It really helps reviewers verify the fix quickly. Thanks! |
|
Added |
7a93cce
into
litellm_internal_staging
* 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> --------- Co-authored-by: Cursor <[email protected]> Co-authored-by: Yassin Kortam <[email protected]>

Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
CI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Type
🆕 New Feature
🐛 Bug Fix
🧹 Refactoring
📖 Documentation
🚄 Infrastructure
✅ Test
Changes