TASK-016: Validate delete_log_path query parameter with allowlist
- Extract path validation logic into shared helper function in
backend/app/utils/path_utils.py (validate_log_path)
- Refactor AddLogPathRequest to use the helper function
- Apply the same validation to DELETE /api/config/jails/{name}/logpath
endpoint by validating the log_path query parameter
- Return HTTP 422 with descriptive error if validation fails
- Add comprehensive unit tests for path validation
- Update Backend-Development.md with usage examples
This prevents path-traversal attacks on the delete_log_path endpoint
by ensuring all log paths are within allowlisted directories.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -219,27 +219,47 @@ For fields that require complex validation (e.g., file paths that must be within
|
||||
|
||||
```python
|
||||
from pydantic import field_validator
|
||||
from app.utils.path_utils import validate_log_path
|
||||
|
||||
class AddLogPathRequest(BaseModel):
|
||||
log_path: str = Field(..., description="Absolute path to the log file to monitor.")
|
||||
|
||||
@field_validator("log_path", mode="after")
|
||||
@classmethod
|
||||
def validate_log_path(cls, value: str) -> str:
|
||||
def validate_log_path_field(cls, value: str) -> str:
|
||||
"""Validate that the log path is within allowed directories."""
|
||||
settings = get_settings()
|
||||
resolved_path = Path(value).resolve()
|
||||
for allowed_dir in settings.allowed_log_dirs:
|
||||
if resolved_path.is_relative_to(Path(allowed_dir).resolve()):
|
||||
return value
|
||||
raise ValueError(f"Path {value!r} is outside allowed directories")
|
||||
return validate_log_path(value)
|
||||
```
|
||||
|
||||
**Path Validation Helper:**
|
||||
|
||||
For query parameters and other contexts where Pydantic validators cannot be used directly, use the `validate_log_path()` helper from `app.utils.path_utils`:
|
||||
|
||||
```python
|
||||
from fastapi import HTTPException, status
|
||||
from app.utils.path_utils import validate_log_path
|
||||
|
||||
@router.delete("/{name}/logpath")
|
||||
async def delete_log_path(
|
||||
name: str,
|
||||
log_path: str = Query(...),
|
||||
) -> None:
|
||||
try:
|
||||
validate_log_path(log_path)
|
||||
except ValueError as e:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
|
||||
detail=str(e),
|
||||
) from e
|
||||
# ... rest of handler
|
||||
```
|
||||
|
||||
**Key points:**
|
||||
- Use `mode="after"` to validate after Pydantic's basic type coercion.
|
||||
- Use `mode="after"` in model validators to validate after Pydantic's basic type coercion.
|
||||
- Raise `ValueError` if validation fails; Pydantic converts it to an HTTP 400 response.
|
||||
- **Never use string prefix matching** for path validation (e.g., `path.startswith("/var/log")`). Use `Path.is_relative_to()` to avoid bypasses like `/var/log_evil/file.log`.
|
||||
- Resolve symlinks before validating to prevent symlink-based escapes.
|
||||
- For query parameters that cannot use Pydantic validators, use the `validate_log_path()` helper and raise HTTP 422.
|
||||
- **Never use string prefix matching** for path validation (e.g., `path.startswith("/var/log")`). The helper uses `Path.relative_to()` to prevent bypasses like `/var/log_evil/file.log`.
|
||||
- Symlinks are resolved before validating to prevent symlink-based escapes.
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -1,37 +1,3 @@
|
||||
## TASK-015 — `GlobalConfigUpdate.log_target`/`log_level` have no validation
|
||||
|
||||
**Severity:** High
|
||||
|
||||
### Where found
|
||||
`backend/app/models/config.py` — `GlobalConfigUpdate`. `backend/app/services/config_service.py` — `update_global_config()`.
|
||||
|
||||
### Why this is needed
|
||||
`log_target` is forwarded raw to fail2ban via the Unix socket. fail2ban (running as root) creates or opens the file at that path if it does not exist. `log_level` is forwarded raw without checking it is a valid fail2ban log level. Both fields represent an injection path into fail2ban's internal state from an authenticated but potentially compromised account.
|
||||
|
||||
### Goal
|
||||
Validate both fields before forwarding to fail2ban.
|
||||
|
||||
### What to do
|
||||
1. Change `log_target` in `GlobalConfigUpdate` to accept only:
|
||||
- `Literal["STDOUT", "STDERR", "SYSLOG"]`, or
|
||||
- A path validated the same way as `AddLogPathRequest.log_path` (see TASK-014).
|
||||
2. Change `log_level` to `Literal["CRITICAL", "ERROR", "WARNING", "NOTICE", "INFO", "DEBUG"]`.
|
||||
3. Apply the same restrictions in `get_global_config` responses for consistency.
|
||||
|
||||
### Possible traps and issues
|
||||
- The allowlist for `log_target` paths must be consistent with TASK-014 (`BANGUI_ALLOWED_LOG_DIRS`).
|
||||
- Existing deployments using non-standard `log_target` values (e.g., `/var/log/fail2ban.log`) must still work — ensure `/var/log` is in the default allowlist.
|
||||
|
||||
### Docs changes needed
|
||||
- `Features.md` — document valid values for `log_target` and `log_level`.
|
||||
- `Backend-Development.md` — Pydantic Literal types for constrained string fields.
|
||||
|
||||
### Doc references
|
||||
- [Features.md](Features.md) — global fail2ban configuration
|
||||
- [Backend-Development.md](Backend-Development.md) — model validation
|
||||
|
||||
---
|
||||
|
||||
## TASK-016 — `delete_log_path` query parameter unvalidated
|
||||
|
||||
**Severity:** Medium
|
||||
|
||||
Reference in New Issue
Block a user