refactoring-backend #3
@@ -522,6 +522,28 @@ environment:
|
||||
|
||||
**Important:** If `Secure=true` is set, browsers will reject the session cookie when the backend is served over HTTP. Ensure your nginx/reverse proxy terminates TLS and passes `X-Forwarded-Proto: https` so FastAPI knows the connection is secure.
|
||||
|
||||
### fail2ban_start_command Configuration
|
||||
|
||||
The `fail2ban_start_command` setting specifies the shell command used to start the fail2ban daemon during recovery operations (e.g., after a rollback).
|
||||
|
||||
**Format & Parsing:**
|
||||
- The command is split into arguments using `shlex.split()`, which respects shell quoting rules.
|
||||
- Paths with spaces must be quoted. Example: `"/opt/my tools/fail2ban-client" start`.
|
||||
- The command is **not** executed through a shell — no shell variables or globbing are interpreted.
|
||||
|
||||
**Validation:**
|
||||
- The command is validated at startup using `shlex.split()`. Mismatched quotes will raise a `ValueError` with the problematic command in the error message.
|
||||
|
||||
**Environment Variables:**
|
||||
```bash
|
||||
BANGUI_FAIL2BAN_START_COMMAND="fail2ban-client start" # Default
|
||||
BANGUI_FAIL2BAN_START_COMMAND="systemctl start fail2ban" # systemd
|
||||
BANGUI_FAIL2BAN_START_COMMAND='"/opt/my tools/fail2ban" start' # Quoted path
|
||||
```
|
||||
|
||||
**Common Pitfall:**
|
||||
Using `.split()` instead of `shlex.split()` would break commands with spaces in paths. Always use quoted strings for paths that contain whitespace.
|
||||
|
||||
### Login Rate Limiting
|
||||
|
||||
The login endpoint (`POST /api/auth/login`) is protected against brute-force attacks using an in-memory rate limiter.
|
||||
|
||||
@@ -1,39 +1,3 @@
|
||||
## TASK-009 — Blocklist URL has no scheme/host validation — SSRF risk
|
||||
|
||||
**Severity:** High
|
||||
|
||||
### Where found
|
||||
`backend/app/models/blocklist.py` — `BlocklistSourceCreate.url: str = Field(..., min_length=1)`. `backend/app/services/blocklist_service.py` — `preview_source()` and `_download_text_with_retries()`.
|
||||
|
||||
### Why this is needed
|
||||
An authenticated user can supply `file:///etc/passwd`, `http://169.254.169.254/latest/meta-data/` (AWS metadata service), `http://10.0.0.1/admin`, or `http://localhost:8000/api/setup` as a blocklist URL. The backend fetches it and either returns its contents in the preview response or attempts to parse it as an IP list. This is a Server-Side Request Forgery (SSRF) vulnerability.
|
||||
|
||||
### Goal
|
||||
Restrict blocklist URLs to safe, public HTTP/HTTPS endpoints only.
|
||||
|
||||
### What to do
|
||||
1. Change `url: str` to `url: AnyHttpUrl` in `BlocklistSourceCreate` — this rejects `file://`, `ftp://`, and other non-http schemes.
|
||||
2. Add a `@field_validator("url")` that:
|
||||
- Parses the hostname.
|
||||
- Resolves it via `socket.getaddrinfo()` (or uses `ipaddress.ip_address()` if it is a bare IP).
|
||||
- Rejects RFC 1918 private ranges (`10.0.0.0/8`, `172.16.0.0/12`, `192.168.0.0/16`), loopback (`127.0.0.0/8`), link-local (`169.254.0.0/16`), and IPv6 equivalents.
|
||||
3. Add a validator for `http://` URLs — consider requiring `https://` only, or adding a configuration flag.
|
||||
|
||||
### Possible traps and issues
|
||||
- DNS rebinding: the hostname resolves to a public IP at validation time but to a private IP at fetch time. Mitigate by re-validating the final connection IP in aiohttp (custom `TCPConnector` or response callback).
|
||||
- `AnyHttpUrl` allows ports — ensure `http://evil.com:22/` is also blocked or at least safe.
|
||||
- `socket.getaddrinfo()` is blocking — use `asyncio.get_event_loop().getaddrinfo()` or run in executor.
|
||||
|
||||
### Docs changes needed
|
||||
- `Features.md` — document URL validation constraints for blocklist sources.
|
||||
- `Backend-Development.md` — SSRF prevention pattern.
|
||||
|
||||
### Doc references
|
||||
- [Features.md](Features.md) — blocklist management
|
||||
- [Backend-Development.md](Backend-Development.md) — input validation
|
||||
|
||||
---
|
||||
|
||||
## TASK-010 — `fail2ban_start_command` split with `.split()` instead of `shlex.split()`
|
||||
|
||||
**Severity:** Low
|
||||
|
||||
@@ -4,6 +4,7 @@ Follows pydantic-settings patterns: all values are prefixed with BANGUI_
|
||||
and validated at startup via the Settings singleton.
|
||||
"""
|
||||
|
||||
import shlex
|
||||
from typing import Literal
|
||||
|
||||
from pydantic import Field, field_validator
|
||||
@@ -151,6 +152,32 @@ class Settings(BaseSettings):
|
||||
),
|
||||
)
|
||||
|
||||
@field_validator("fail2ban_start_command", mode="after")
|
||||
@classmethod
|
||||
def _validate_fail2ban_start_command(cls, value: str) -> str:
|
||||
"""Validate fail2ban_start_command by attempting to parse it with shlex.
|
||||
|
||||
Ensures the command can be split into arguments without shell interpretation.
|
||||
Raises ValueError if the command contains mismatched quotes.
|
||||
|
||||
Args:
|
||||
value: The fail2ban start command string.
|
||||
|
||||
Returns:
|
||||
The validated command string.
|
||||
|
||||
Raises:
|
||||
ValueError: If the command contains mismatched quotes.
|
||||
"""
|
||||
try:
|
||||
shlex.split(value)
|
||||
except ValueError as e:
|
||||
raise ValueError(
|
||||
f"fail2ban_start_command contains mismatched quotes or is otherwise "
|
||||
f"unparseable: {value!r} — {e}"
|
||||
) from e
|
||||
return value
|
||||
|
||||
model_config = SettingsConfigDict(
|
||||
env_prefix="BANGUI_",
|
||||
env_file=".env",
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import shlex
|
||||
from typing import Annotated
|
||||
|
||||
import structlog
|
||||
@@ -149,7 +150,7 @@ async def restart_fail2ban(
|
||||
``POST /api/config/jails/{name}/rollback``
|
||||
if a specific jail is suspect.
|
||||
"""
|
||||
start_cmd_parts: list[str] = start_cmd.split()
|
||||
start_cmd_parts: list[str] = shlex.split(start_cmd)
|
||||
|
||||
restarted = await jail_service.restart_daemon(
|
||||
socket_path,
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import shlex
|
||||
from typing import Annotated
|
||||
|
||||
from fastapi import APIRouter, Path, Query, Request, status
|
||||
@@ -445,7 +446,7 @@ async def rollback_jail(
|
||||
HTTPException: 400 if *name* contains invalid characters.
|
||||
HTTPException: 500 if writing the .local override file fails.
|
||||
"""
|
||||
start_cmd_parts: list[str] = start_cmd.split()
|
||||
start_cmd_parts: list[str] = shlex.split(start_cmd)
|
||||
|
||||
result = await jail_config_service.rollback_jail(config_dir, socket_path, name, start_cmd_parts)
|
||||
|
||||
|
||||
107
backend/tests/test_config.py
Normal file
107
backend/tests/test_config.py
Normal file
@@ -0,0 +1,107 @@
|
||||
"""Unit tests for application configuration and validation."""
|
||||
|
||||
import pytest
|
||||
from pydantic import ValidationError
|
||||
|
||||
from app.config import Settings
|
||||
|
||||
|
||||
def test_fail2ban_start_command_validates_simple_command() -> None:
|
||||
"""Simple fail2ban commands without special characters are accepted."""
|
||||
settings = Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command="fail2ban-client start",
|
||||
)
|
||||
assert settings.fail2ban_start_command == "fail2ban-client start"
|
||||
|
||||
|
||||
def test_fail2ban_start_command_validates_systemctl_command() -> None:
|
||||
"""systemctl commands are accepted."""
|
||||
settings = Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command="systemctl start fail2ban",
|
||||
)
|
||||
assert settings.fail2ban_start_command == "systemctl start fail2ban"
|
||||
|
||||
|
||||
def test_fail2ban_start_command_accepts_quoted_paths() -> None:
|
||||
"""Commands with quoted paths containing spaces are accepted."""
|
||||
settings = Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command='"/opt/my tools/fail2ban-client" start',
|
||||
)
|
||||
assert settings.fail2ban_start_command == '"/opt/my tools/fail2ban-client" start'
|
||||
|
||||
|
||||
def test_fail2ban_start_command_rejects_mismatched_quotes() -> None:
|
||||
"""Commands with mismatched quotes raise ValidationError."""
|
||||
with pytest.raises(ValidationError) as exc_info:
|
||||
Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command='"/opt/my tools/fail2ban-client start',
|
||||
)
|
||||
error_msg = str(exc_info.value)
|
||||
assert "fail2ban_start_command" in error_msg
|
||||
assert "mismatched quotes" in error_msg or "No closing quotation" in error_msg
|
||||
|
||||
|
||||
def test_fail2ban_start_command_error_includes_problematic_value() -> None:
|
||||
"""Validation errors include the problematic command value."""
|
||||
problematic_command = '"/opt/broken start'
|
||||
with pytest.raises(ValidationError) as exc_info:
|
||||
Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command=problematic_command,
|
||||
)
|
||||
error_msg = str(exc_info.value)
|
||||
assert problematic_command in error_msg
|
||||
|
||||
|
||||
def test_fail2ban_start_command_default_value_is_valid() -> None:
|
||||
"""The default fail2ban_start_command value is valid and parseable."""
|
||||
settings = Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
)
|
||||
assert settings.fail2ban_start_command == "fail2ban-client start"
|
||||
|
||||
|
||||
def test_fail2ban_start_command_single_quoted() -> None:
|
||||
"""Commands with single quotes are accepted."""
|
||||
settings = Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command="'/usr/bin/fail2ban-client' start",
|
||||
)
|
||||
assert settings.fail2ban_start_command == "'/usr/bin/fail2ban-client' start"
|
||||
|
||||
|
||||
def test_fail2ban_start_command_multiple_arguments() -> None:
|
||||
"""Commands with multiple arguments are accepted."""
|
||||
settings = Settings(
|
||||
database_path="/tmp/test.db",
|
||||
fail2ban_socket="/tmp/fake_fail2ban.sock",
|
||||
fail2ban_config_dir="/tmp/fail2ban",
|
||||
session_secret="test-secret-key-do-not-use-in-production",
|
||||
fail2ban_start_command="fail2ban-client -c /etc/fail2ban start",
|
||||
)
|
||||
assert settings.fail2ban_start_command == "fail2ban-client -c /etc/fail2ban start"
|
||||
Reference in New Issue
Block a user