Fix: Remove socket path leak in fail2ban error responses
- Change _fail2ban_connection_handler() to return generic message instead of leaking socket path in HTTP 502 response body - Change _fail2ban_protocol_handler() to return generic message instead of leaking raw exception details in HTTP 502 response body - Full error details are still logged server-side (error=str(exc)) for debugging - Update Backend-Development.md with error message hygiene section explaining the pattern: generic user-friendly messages in HTTP responses, full details in server logs only Fixes TASK-029: Fail2BanConnectionError leaks socket path in HTTP error responses Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -581,6 +581,36 @@ All domain exceptions raised by services propagate to handlers in `main.py`, ens
|
|||||||
2. No duplicated exception-to-HTTP-status mapping logic.
|
2. No duplicated exception-to-HTTP-status mapping logic.
|
||||||
3. Easy to audit all error codes — they are all in one place.
|
3. Easy to audit all error codes — they are all in one place.
|
||||||
|
|
||||||
|
### Error Message Hygiene
|
||||||
|
|
||||||
|
HTTP responses must never leak sensitive internal details that aid attackers or expose infrastructure:
|
||||||
|
|
||||||
|
- **Never include system paths** in HTTP error messages (e.g., `/var/run/fail2ban/fail2ban.sock`, `/etc/fail2ban/`).
|
||||||
|
- **Never include raw exception messages** that expose internal parsing or implementation logic.
|
||||||
|
- **Log full details server-side only** — exception handlers must log `error=str(exc)` with full exception context, but return generic user-friendly messages in the HTTP response.
|
||||||
|
|
||||||
|
```python
|
||||||
|
# ❌ BAD — leaks socket path and internal details to the client
|
||||||
|
async def _fail2ban_connection_handler(request: Request, exc: Fail2BanConnectionError) -> JSONResponse:
|
||||||
|
return JSONResponse(
|
||||||
|
status_code=502,
|
||||||
|
content={"detail": f"Cannot reach fail2ban: {exc}"}, # exc includes socket path!
|
||||||
|
)
|
||||||
|
|
||||||
|
# ✅ GOOD — generic message in response, full details in server logs
|
||||||
|
async def _fail2ban_connection_handler(request: Request, exc: Fail2BanConnectionError) -> JSONResponse:
|
||||||
|
log.warning(
|
||||||
|
"fail2ban_connection_error",
|
||||||
|
path=request.url.path,
|
||||||
|
method=request.method,
|
||||||
|
error=str(exc), # Full details logged server-side
|
||||||
|
)
|
||||||
|
return JSONResponse(
|
||||||
|
status_code=502,
|
||||||
|
content={"detail": "Cannot reach the fail2ban service. Check the server status page."},
|
||||||
|
)
|
||||||
|
```
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## 9. Testing
|
## 9. Testing
|
||||||
|
|||||||
@@ -1,47 +1,3 @@
|
|||||||
## TASK-028 — Fire-and-forget `asyncio.create_task()` silently discards exceptions
|
|
||||||
|
|
||||||
**Severity:** Low
|
|
||||||
|
|
||||||
### Where found
|
|
||||||
`backend/app/services/ban_service.py` line ~614:
|
|
||||||
```python
|
|
||||||
asyncio.create_task( # noqa: RUF006
|
|
||||||
geo_cache.lookup_batch(uncached, http_session, db=app_db),
|
|
||||||
name="geo_bans_by_country",
|
|
||||||
)
|
|
||||||
```
|
|
||||||
|
|
||||||
### Why this is needed
|
|
||||||
The task reference is immediately discarded. Any exception raised inside `geo_cache.lookup_batch()` — network errors, aiohttp timeouts, DB write failures — becomes an unhandled task exception. In Python 3.11+ this emits a `RuntimeWarning` to stderr but is otherwise silently swallowed. Errors in background geo resolution are invisible in structured logs.
|
|
||||||
|
|
||||||
### Goal
|
|
||||||
Ensure exceptions in fire-and-forget tasks are always logged.
|
|
||||||
|
|
||||||
### What to do
|
|
||||||
1. Wrap the task body in a logging wrapper:
|
|
||||||
```python
|
|
||||||
async def _logged_task(coro: Coroutine[Any, Any, Any], name: str) -> None:
|
|
||||||
try:
|
|
||||||
await coro
|
|
||||||
except Exception:
|
|
||||||
log.exception("background_task_failed", task_name=name)
|
|
||||||
|
|
||||||
asyncio.create_task(_logged_task(geo_cache.lookup_batch(...), "geo_bans_by_country"))
|
|
||||||
```
|
|
||||||
2. Extract `_logged_task` into `backend/app/utils/async_utils.py` as a reusable helper so the same pattern is used for all fire-and-forget tasks.
|
|
||||||
|
|
||||||
### Possible traps and issues
|
|
||||||
- The done callback must not re-raise the exception — only log it.
|
|
||||||
- `log.exception()` inside a callback/task captures the traceback automatically with structlog.
|
|
||||||
|
|
||||||
### Docs changes needed
|
|
||||||
- `Backend-Development.md` — fire-and-forget task conventions.
|
|
||||||
|
|
||||||
### Doc references
|
|
||||||
- [Backend-Development.md](Backend-Development.md) — async patterns
|
|
||||||
|
|
||||||
---
|
|
||||||
|
|
||||||
## TASK-029 — `Fail2BanConnectionError` leaks socket path in HTTP error responses
|
## TASK-029 — `Fail2BanConnectionError` leaks socket path in HTTP error responses
|
||||||
|
|
||||||
**Severity:** Medium
|
**Severity:** Medium
|
||||||
|
|||||||
@@ -231,7 +231,7 @@ async def _fail2ban_connection_handler(
|
|||||||
)
|
)
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
status_code=502,
|
status_code=502,
|
||||||
content={"detail": f"Cannot reach fail2ban: {exc}"},
|
content={"detail": "Cannot reach the fail2ban service. Check the server status page."},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -256,7 +256,7 @@ async def _fail2ban_protocol_handler(
|
|||||||
)
|
)
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
status_code=502,
|
status_code=502,
|
||||||
content={"detail": f"fail2ban protocol error: {exc}"},
|
content={"detail": "Cannot reach the fail2ban service. Check the server status page."},
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user