mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-27 01:22:18 +00:00
fix(proxy/cwe-209): apply Greptile P2 fixes and add MCP exception-path tests
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 <noreply@anthropic.com>
This commit is contained in:
parent
401aa807ee
commit
70dea1f9f2
3 changed files with 80 additions and 24 deletions
|
|
@ -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),
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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):
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue