refactoring-backend #3

Merged
lukas.pupkalipinski merged 403 commits from refactoring-backend into main 2026-05-20 20:23:46 +02:00
4 changed files with 62 additions and 34 deletions
Showing only changes of commit 744275d17f - Show all commits

View File

@@ -112,6 +112,32 @@ The CI pipeline enforces the same 80% minimum coverage threshold.
---
## Security Rules
### Never echo raw user input in error messages
User-supplied values (jail names, filter names, action names, IPs, filenames, etc.)
MUST be sanitized before interpolation into any string that may be rendered in an
HTML context (error messages, admin UI, email notifications).
Use the `sanitize_for_display()` helper from `app.utils.display_sanitizer`:
```python
from app.utils.display_sanitizer import sanitize_for_display
# Good: sanitized before display
super().__init__(f"Jail not found: {sanitize_for_display(name)!r}")
# Bad: raw user input echoed — XSS vector if rendered as HTML
super().__init__(f"Jail not found: {name!r}")
```
This rule applies even when the value has been validated: validation checks the
format, not the rendering context. JSON API responses do NOT need sanitization
(JSON is not HTML); apply it only at HTML render boundaries.
---
## Stack
| Layer | Stack |

View File

@@ -1,24 +0,0 @@
### Issue #69: LOW - Jail Names Echoed in Error Messages Without Sanitization
**Where found**:
- `backend/app/exceptions.py:138,351` jail names interpolated directly into error strings
**Why this is needed**:
Although Python's `repr()` provides basic escaping, user-supplied jail names are reflected back in error messages. If these messages are ever rendered in an HTML context (e.g., a future admin UI or email notification), they become XSS vectors. They also act as confirmation oracles when combined with timing attacks.
**Goal**:
Error messages referencing user input are sanitized before inclusion.
**What to do**:
1. Pass user-supplied values through a dedicated `sanitize_for_display()` helper before interpolation.
2. Ensure the helper strips or escapes HTML special characters.
3. For API responses, always return the original (validated) field name rather than the raw user input.
**Possible traps and issues**:
- Over-escaping in JSON responses is not needed (JSON is not HTML); apply sanitization only at HTML render boundaries.
**Docs changes needed**:
- `CONTRIBUTING.md`: document the rule that user input must not be echoed raw in messages.
**Doc references**:
- `backend/app/exceptions.py`

View File

@@ -41,6 +41,8 @@ from __future__ import annotations
from typing import TYPE_CHECKING
from app.utils.display_sanitizer import sanitize_for_display
if TYPE_CHECKING:
from app.models.response import ErrorMetadata
@@ -137,7 +139,7 @@ class JailNotFoundError(NotFoundError):
def __init__(self, name: str) -> None:
self.name = name
super().__init__(f"Jail not found: {name!r}")
super().__init__(f"Jail not found: {sanitize_for_display(name)!r}")
def get_error_metadata(self) -> ErrorMetadata:
return {"jail_name": self.name}
@@ -277,8 +279,7 @@ class FilterRegexTooLongError(BadRequestError):
self.max_length = max_length
self.actual_length = len(pattern)
super().__init__(
f"Regex pattern exceeds maximum length of {max_length} characters: "
f"{self.actual_length} provided"
f"Regex pattern exceeds maximum length of {max_length} characters: {self.actual_length} provided"
)
def get_error_metadata(self) -> ErrorMetadata:
@@ -318,7 +319,7 @@ class JailNotFoundInConfigError(NotFoundError):
def __init__(self, name: str) -> None:
self.name = name
super().__init__(f"Jail not found in config: {name!r}")
super().__init__(f"Jail not found in config: {sanitize_for_display(name)!r}")
def get_error_metadata(self) -> ErrorMetadata:
return {"jail_name": self.name}
@@ -350,7 +351,7 @@ class JailAlreadyActiveError(ConflictError):
def __init__(self, name: str) -> None:
self.name = name
super().__init__(f"Jail is already active: {name!r}")
super().__init__(f"Jail is already active: {sanitize_for_display(name)!r}")
def get_error_metadata(self) -> ErrorMetadata:
return {"jail_name": self.name}
@@ -363,7 +364,7 @@ class JailAlreadyInactiveError(ConflictError):
def __init__(self, name: str) -> None:
self.name = name
super().__init__(f"Jail is already inactive: {name!r}")
super().__init__(f"Jail is already inactive: {sanitize_for_display(name)!r}")
def get_error_metadata(self) -> ErrorMetadata:
return {"jail_name": self.name}
@@ -463,7 +464,6 @@ class ActionReadonlyError(ConflictError):
return {"action_name": self.name}
class SetupAlreadyCompleteError(ConflictError):
"""Raised when attempting to run setup when it has already been completed."""
@@ -473,7 +473,6 @@ class SetupAlreadyCompleteError(ConflictError):
super().__init__("Setup has already been completed.")
class BlocklistSourceNotFoundError(NotFoundError):
"""Raised when a blocklist source is not found."""
@@ -495,8 +494,7 @@ class BlocklistSourceHasLogsError(ConflictError):
def __init__(self, source_id: int) -> None:
self.source_id = source_id
super().__init__(
f"Blocklist source {source_id} cannot be deleted because it has import logs. "
"Delete the import logs first."
f"Blocklist source {source_id} cannot be deleted because it has import logs. Delete the import logs first."
)
def get_error_metadata(self) -> ErrorMetadata:

View File

@@ -0,0 +1,28 @@
"""Display sanitization utilities for HTML render contexts.
All user-supplied values echoed in error messages or other HTML-rendered
output MUST be sanitized first. This module provides the canonical
sanitize_for_display() function.
"""
from __future__ import annotations
import html
def sanitize_for_display(value: str) -> str:
"""Escape HTML special characters in user-supplied strings.
Use this before interpolating user input into any string that will be
rendered in an HTML context (e.g. error messages, admin UI, email).
Does NOT over-escape: JSON responses are not HTML contexts and do not
need this treatment. Apply sanitization only at HTML render boundaries.
Args:
value: Raw user-supplied string.
Returns:
The string with HTML special characters escaped.
"""
return html.escape(value, quote=True)