From 70dea1f9f2652a272b50798b85be815705022535 Mon Sep 17 00:00:00 2001 From: Drishna Trivedi Date: Wed, 20 May 2026 14:43:07 +0530 Subject: [PATCH] fix(proxy/cwe-209): apply Greptile P2 fixes and add MCP exception-path tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile 4/5 review identified two remaining gaps and Codecov reported 0% coverage on the two MCP handler exception branches: 1. caching_routes.py — str(e) in "Service Unhealthy ({str(e)})" could still leak Redis hostnames/IPs; replaced with static "Service Unhealthy". HTTPException is now re-raised before the generic handler so the "cache not initialized" 503 still reaches callers with its detail. Removed the redundant str(e) arg from verbose_proxy_logger.exception() (exception() already appends the traceback automatically). 2. tests — two new unit tests cover the exception paths in dynamic_mcp_route and toolset_mcp_route that were previously at 0%: - test_dynamic_mcp_route_unexpected_exception_returns_500_without_traceback - test_toolset_mcp_route_unexpected_exception_returns_500_without_traceback All 25 tests pass (9 caching + 16 MCP). CWE-209: Generation of Error Message Containing Sensitive Information. Co-Authored-By: Claude Sonnet 4.6 --- litellm/proxy/caching_routes.py | 8 +-- .../test_litellm/proxy/test_caching_routes.py | 42 +++++++-------- .../proxy/test_dynamic_mcp_route.py | 54 +++++++++++++++++++ 3 files changed, 80 insertions(+), 24 deletions(-) diff --git a/litellm/proxy/caching_routes.py b/litellm/proxy/caching_routes.py index 7a2c965c38b..00f99d2dbba 100644 --- a/litellm/proxy/caching_routes.py +++ b/litellm/proxy/caching_routes.py @@ -97,10 +97,12 @@ async def cache_ping(): cache_type=str(litellm.cache.type), litellm_cache_params=safe_dumps(litellm_cache_params), ) - except Exception as e: - verbose_proxy_logger.exception("Cache health check failed: %s", str(e)) + except HTTPException: + raise + except Exception: + verbose_proxy_logger.exception("Cache health check failed") error_message = { - "message": f"Service Unhealthy ({str(e)})", + "message": "Service Unhealthy", "litellm_cache_params": safe_dumps(litellm_cache_params), "health_check_cache_params": safe_dumps(cleaned_cache_params), } diff --git a/tests/test_litellm/proxy/test_caching_routes.py b/tests/test_litellm/proxy/test_caching_routes.py index 2ad750c7be4..cdd7f940047 100644 --- a/tests/test_litellm/proxy/test_caching_routes.py +++ b/tests/test_litellm/proxy/test_caching_routes.py @@ -124,12 +124,12 @@ def test_cache_ping_failure(mock_redis_failure): assert "litellm_cache_params" in error_details assert "health_check_cache_params" in error_details - # Verify specific error message - assert "invalid username-password pair" in error_details["message"] + # Verify generic static message (exception text must not leak to clients) + assert error_details["message"] == "Service Unhealthy" def test_cache_ping_failure_does_not_expose_traceback(mock_redis_failure): - """CWE-209: Stack trace must not appear in the HTTP 503 response body.""" + """CWE-209: Stack trace and exception text must not appear in the HTTP 503 response body.""" response = client.get("/cache/ping", headers={"Authorization": "Bearer sk-1234"}) assert response.status_code == 503 @@ -145,33 +145,33 @@ def test_cache_ping_failure_does_not_expose_traceback(mock_redis_failure): assert ( 'File "' not in raw_body ), "CWE-209: Python stack frame paths exposed in HTTP 503 response body" + # Exception text (e.g. Redis hostnames/IPs) must not leak either + assert ( + "invalid username-password pair" not in raw_body + ), "CWE-209: Exception message text exposed in HTTP 503 response body" - # The error message field should still describe the failure + # The error message should be the safe static string error_details = json.loads(error["message"]) - assert "invalid username-password pair" in error_details["message"] + assert error_details["message"] == "Service Unhealthy" def test_cache_ping_no_cache_initialized(): - """Test cache ping when no cache is initialized""" - # Set cache to None + """Test cache ping when no cache is initialized returns 503 with a clear message.""" original_cache = litellm.cache litellm.cache = None - response = client.get("/cache/ping", headers={"Authorization": "Bearer sk-1234"}) - assert response.status_code == 503 + try: + response = client.get( + "/cache/ping", headers={"Authorization": "Bearer sk-1234"} + ) + assert response.status_code == 503 - data = response.json() - print("response data=", json.dumps(data, indent=4)) - assert "error" in data - error = data["error"] - - # Verify error contains all expected fields - assert "message" in error - error_details = json.loads(error["message"]) - assert "Cache not initialized. litellm.cache is None" in error_details["message"] - - # Restore original cache - litellm.cache = original_cache + data = response.json() + print("response data=", json.dumps(data, indent=4)) + # HTTPException propagates directly; detail is a plain string in the response + assert "Cache not initialized" in str(data) + finally: + litellm.cache = original_cache def test_cache_ping_health_check_includes_only_cache_attributes(mock_redis_success): diff --git a/tests/test_litellm/proxy/test_dynamic_mcp_route.py b/tests/test_litellm/proxy/test_dynamic_mcp_route.py index 2462aff2119..592cebd957c 100644 --- a/tests/test_litellm/proxy/test_dynamic_mcp_route.py +++ b/tests/test_litellm/proxy/test_dynamic_mcp_route.py @@ -486,3 +486,57 @@ async def test_dynamic_mcp_route_empty_access_group_returns_404(): await dynamic_mcp_route("empty_group", request) assert exc_info.value.status_code == 404 + + +# --------------------------------------------------------------------------- +# 6. Unexpected exception → 500 without leaking stack trace (CWE-209) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_dynamic_mcp_route_unexpected_exception_returns_500_without_traceback(): + """CWE-209: an unexpected exception must return 500 with a generic message, + never leaking str(e) or a Python traceback to the caller.""" + from litellm.proxy.proxy_server import dynamic_mcp_route + + request = _make_request("/boom/mcp") + + fake_mgr = MagicMock() + fake_mgr.get_mcp_server_by_name = MagicMock( + side_effect=RuntimeError("internal host: redis://10.0.0.1:6379") + ) + + with patch(_MCP_MANAGER, fake_mgr): + with pytest.raises(HTTPException) as exc_info: + await dynamic_mcp_route("boom", request) + + assert exc_info.value.status_code == 500 + assert exc_info.value.detail == "Internal server error" + assert "10.0.0.1" not in str(exc_info.value.detail) + assert "traceback" not in str(exc_info.value.detail).lower() + + +@pytest.mark.asyncio +async def test_toolset_mcp_route_unexpected_exception_returns_500_without_traceback(): + """CWE-209: toolset_mcp_route must return 500 with a generic message on + unexpected errors, never leaking exception text to the caller.""" + from litellm.proxy.proxy_server import toolset_mcp_route + + request = _make_request("/toolset/broken_toolset/mcp") + + fake_mgr = MagicMock() + fake_mgr.get_toolset_by_name_cached = AsyncMock( + side_effect=RuntimeError("connection to db-host:5432 refused") + ) + + with ( + patch(_MCP_MANAGER, fake_mgr), + patch(_PRISMA, new=MagicMock()), + ): + with pytest.raises(HTTPException) as exc_info: + await toolset_mcp_route("broken_toolset", request) + + assert exc_info.value.status_code == 500 + assert exc_info.value.detail == "Internal server error" + assert "db-host" not in str(exc_info.value.detail) + assert "traceback" not in str(exc_info.value.detail).lower()