mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-09 03:18:44 +00:00
fix(mcp): keep out-of-contract upstream error bodies out of client responses
The token and DCR relays serve unauthenticated OAuth clients, so only the RFC 6749/7591 error fields may cross the trust boundary. A rejection body outside those contracts (HTML error page, proxy banner, stack trace) is now logged server-side, bounded, and the client response names only the upstream status. Addresses the Veria information-exposure finding
This commit is contained in:
parent
e940199a00
commit
0d9c3cf97b
2 changed files with 95 additions and 12 deletions
|
|
@ -1004,12 +1004,19 @@ def _upstream_token_error_response(response: httpx.Response) -> JSONResponse:
|
|||
Only the RFC 6749 §5.2 fields (``error`` / ``error_description`` / ``error_uri``) are relayed,
|
||||
each bounded, on the upstream's own 400/401 status; a rejection outside the §5.2 contract
|
||||
(no JSON ``error`` field, or a status §5.2 does not define) maps to 502 so a broken upstream
|
||||
is not misattributed to the caller's request."""
|
||||
is not misattributed to the caller's request. Out-of-contract bodies (HTML error pages, proxy
|
||||
banners, stack traces) never cross the trust boundary: these endpoints serve unauthenticated
|
||||
OAuth clients, so the body is logged server-side and the client sees only the upstream status."""
|
||||
parsed = _response_json_or_none(response)
|
||||
error_code = parsed.get("error") if isinstance(parsed, dict) else None
|
||||
if not isinstance(error_code, str) or not error_code:
|
||||
detail = (response.text or response.reason_phrase or "")[:_MAX_UPSTREAM_ERROR_CHARS]
|
||||
return _upstream_token_fault_response(f"upstream token endpoint returned HTTP {response.status_code}: {detail}")
|
||||
verbose_logger.warning(
|
||||
"MCP upstream token endpoint returned HTTP %s with a non-RFC6749 body (first %s chars): %s",
|
||||
response.status_code,
|
||||
_MAX_UPSTREAM_ERROR_CHARS,
|
||||
(response.text or "")[:_MAX_UPSTREAM_ERROR_CHARS],
|
||||
)
|
||||
return _upstream_token_fault_response(f"upstream token endpoint returned HTTP {response.status_code}")
|
||||
fields = parsed if isinstance(parsed, dict) else {}
|
||||
relayed = {
|
||||
key: value[:_MAX_UPSTREAM_ERROR_CHARS]
|
||||
|
|
@ -1444,15 +1451,25 @@ _MAX_UPSTREAM_ERROR_CHARS = 500
|
|||
|
||||
|
||||
def _safe_upstream_error_detail(response: httpx.Response) -> str:
|
||||
"""Bounded plaintext summary of an upstream registration failure for the client.
|
||||
|
||||
RFC 7591 error bodies are small JSON objects (``error`` / ``error_description``); relaying the
|
||||
text lets the client read the real reason instead of a bare 500, and the length bound keeps a
|
||||
hostile or oversized upstream body from bloating the gateway response."""
|
||||
body = response.text
|
||||
if not body:
|
||||
return response.reason_phrase or "upstream registration failed"
|
||||
return body[:_MAX_UPSTREAM_ERROR_CHARS]
|
||||
"""Client-safe summary of an upstream registration failure: only the RFC 7591 §3.2.2 fields
|
||||
(``error`` / ``error_description``) cross the trust boundary, each bounded, since these endpoints
|
||||
serve unauthenticated OAuth clients. A body outside that contract (HTML error pages, proxy
|
||||
banners, stack traces) is logged server-side and summarized by status so upstream internals are
|
||||
never relayed to callers."""
|
||||
parsed = _response_json_or_none(response)
|
||||
error_code = parsed.get("error") if isinstance(parsed, dict) else None
|
||||
if isinstance(error_code, str) and error_code:
|
||||
description = parsed.get("error_description") if isinstance(parsed, dict) else None
|
||||
if isinstance(description, str) and description:
|
||||
return f"{error_code[:_MAX_UPSTREAM_ERROR_CHARS]}: {description[:_MAX_UPSTREAM_ERROR_CHARS]}"
|
||||
return error_code[:_MAX_UPSTREAM_ERROR_CHARS]
|
||||
verbose_logger.warning(
|
||||
"MCP upstream registration endpoint returned HTTP %s with a non-RFC7591 body (first %s chars): %s",
|
||||
response.status_code,
|
||||
_MAX_UPSTREAM_ERROR_CHARS,
|
||||
(response.text or "")[:_MAX_UPSTREAM_ERROR_CHARS],
|
||||
)
|
||||
return f"upstream registration failed with HTTP {response.status_code}"
|
||||
|
||||
|
||||
async def register_client_with_server(
|
||||
|
|
|
|||
|
|
@ -4274,6 +4274,9 @@ async def test_register_bridge_relay_surfaces_upstream_error_not_500():
|
|||
error_response = MagicMock()
|
||||
error_response.status_code = 400
|
||||
error_response.text = '{"error":"invalid_redirect_uri","error_description":"redirect_uri not allowed"}'
|
||||
error_response.json = MagicMock(
|
||||
return_value={"error": "invalid_redirect_uri", "error_description": "redirect_uri not allowed"}
|
||||
)
|
||||
error_response.raise_for_status = MagicMock(
|
||||
side_effect=httpx.HTTPStatusError("bad", request=MagicMock(), response=error_response)
|
||||
)
|
||||
|
|
@ -4320,6 +4323,7 @@ async def test_register_non_bridge_upstream_error_relays_status_not_500():
|
|||
error_response = MagicMock()
|
||||
error_response.status_code = 400
|
||||
error_response.text = '{"error":"invalid_client_metadata"}'
|
||||
error_response.json = MagicMock(return_value={"error": "invalid_client_metadata"})
|
||||
error_response.raise_for_status = MagicMock(
|
||||
side_effect=httpx.HTTPStatusError("bad", request=MagicMock(), response=error_response)
|
||||
)
|
||||
|
|
@ -6205,3 +6209,65 @@ async def test_register_relays_rejection_when_http_client_raises():
|
|||
|
||||
assert exc.value.status_code == 400
|
||||
assert "invalid_client_metadata" in str(exc.value.detail)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_token_exchange_never_relays_out_of_contract_body_to_client():
|
||||
"""These endpoints serve unauthenticated OAuth clients, so a non-RFC6749 upstream body (HTML
|
||||
error page, proxy banner, stack trace) must stay in server logs; the client sees only the
|
||||
upstream status."""
|
||||
response = await _exchange_with_upstream_response(
|
||||
_upstream_token_response(404, text_body="<html>Error 404 stack trace: secret internals</html>")
|
||||
)
|
||||
|
||||
assert response.status_code == 502
|
||||
body = json.loads(response.body)
|
||||
assert body == {"error": "server_error", "error_description": "upstream token endpoint returned HTTP 404"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_register_never_relays_out_of_contract_body_to_client():
|
||||
"""Same trust boundary for DCR: a non-RFC7591 rejection body is logged server-side and the
|
||||
client detail names only the status."""
|
||||
import httpx
|
||||
|
||||
from litellm.proxy._experimental.mcp_server.discoverable_endpoints import (
|
||||
register_client_with_server,
|
||||
)
|
||||
|
||||
rejection = httpx.Response(
|
||||
500,
|
||||
text="<html>Tomcat stack trace with internals</html>",
|
||||
request=httpx.Request("POST", "https://idp.example.com/register"),
|
||||
)
|
||||
raising_client = MagicMock()
|
||||
raising_client.post = AsyncMock(
|
||||
side_effect=httpx.HTTPStatusError("Server error '500'", request=rejection.request, response=rejection)
|
||||
)
|
||||
|
||||
oauth2_server = _bridge_server(auth_type=MCPAuth.oauth2, dcr_bridge=None)
|
||||
|
||||
with (
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.discoverable_endpoints.get_async_httpx_client",
|
||||
return_value=raising_client,
|
||||
),
|
||||
patch(
|
||||
"litellm.proxy._experimental.mcp_server.discoverable_endpoints._reuse_persisted_dcr_client_if_available",
|
||||
new_callable=AsyncMock,
|
||||
return_value=False,
|
||||
),
|
||||
):
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
await register_client_with_server(
|
||||
request=_bridge_mock_request(),
|
||||
mcp_server=oauth2_server,
|
||||
client_name="Claude",
|
||||
grant_types=None,
|
||||
response_types=None,
|
||||
token_endpoint_auth_method=None,
|
||||
)
|
||||
|
||||
assert exc.value.status_code == 500
|
||||
assert str(exc.value.detail) == "upstream registration failed with HTTP 500"
|
||||
assert "Tomcat" not in str(exc.value.detail)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue