diff --git a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py index 13d514f16db..42677229add 100644 --- a/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py +++ b/litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py @@ -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( 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 fd5e4b7d62b..d494e1c6cb3 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 @@ -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="Error 404 stack trace: secret internals") + ) + + 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="Tomcat stack trace with internals", + 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)