refactoring-backend #3
@@ -59,6 +59,7 @@ services:
|
||||
BANGUI_FAIL2BAN_SOCKET: "/var/run/fail2ban/fail2ban.sock"
|
||||
BANGUI_FAIL2BAN_CONFIG_DIR: "/config/fail2ban"
|
||||
BANGUI_LOG_LEVEL: "debug"
|
||||
BANGUI_ENABLE_DOCS: "true"
|
||||
BANGUI_SESSION_SECRET: "${BANGUI_SESSION_SECRET:-dev-secret-do-not-use-in-production}"
|
||||
BANGUI_TIMEZONE: "${BANGUI_TIMEZONE:-UTC}"
|
||||
# Secure=false is intentional for local HTTP development.
|
||||
|
||||
@@ -834,6 +834,29 @@ 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.
|
||||
|
||||
### API Documentation Configuration
|
||||
|
||||
The `enable_docs` setting controls whether FastAPI serves interactive API documentation at `/api/docs` (Swagger UI) and `/api/redoc` (ReDoc).
|
||||
|
||||
**Default:** `false` — API documentation is disabled by default to prevent information disclosure in production.
|
||||
|
||||
**When to Enable:**
|
||||
- Set `BANGUI_ENABLE_DOCS=true` in development and debugging environments only.
|
||||
- Never enable in production. Exposed API documentation reveals all endpoints, request/response schemas, and allows direct API invocation from the browser.
|
||||
|
||||
**Environment Variables:**
|
||||
```bash
|
||||
BANGUI_ENABLE_DOCS="true" # Enable docs in development
|
||||
BANGUI_ENABLE_DOCS="false" # Disable docs (default)
|
||||
# Unset # Defaults to false (production)
|
||||
```
|
||||
|
||||
**Debug Compose File:**
|
||||
The `Docker/compose.debug.yml` sets `BANGUI_ENABLE_DOCS: "true"` for local development. Production compose files (`Docker/compose.prod.yml`) leave this unset, defaulting to `false`.
|
||||
|
||||
**Middleware Allowlist:**
|
||||
The `SetupRedirectMiddleware` in `main.py` includes `/api/docs`, `/api/redoc`, and `/api/openapi.json` in its `_ALWAYS_ALLOWED` paths so documentation can be accessed before setup completes (if enabled).
|
||||
|
||||
### Log Path Validation & Allowlisting
|
||||
|
||||
Authenticated users can instruct fail2ban to monitor additional log files through the API endpoint `POST /api/config/jails/{name}/logpath`. To prevent path-traversal attacks and unauthorized reads of sensitive system files, all requested log paths must resolve to locations within a configurable allowlist of safe directories.
|
||||
|
||||
@@ -1,77 +1,3 @@
|
||||
## TASK-024 — No CSRF protection on state-mutating endpoints
|
||||
|
||||
**Severity:** High
|
||||
|
||||
### Where found
|
||||
All `POST`, `PUT`, `DELETE` routes in `backend/app/routers/`. Only `SameSite=Lax` on the session cookie provides any CSRF protection.
|
||||
|
||||
### Why this is needed
|
||||
`SameSite=Lax` blocks cross-site `<form>` POST requests but does **not** block `fetch(..., {credentials: "include"})` initiated by JavaScript on a subdomain or a same-origin XSS injection. Without a CSRF token or `Origin` header check, a compromised subdomain can issue authenticated requests on behalf of the logged-in user.
|
||||
|
||||
### Goal
|
||||
Add explicit CSRF protection for all cookie-authenticated state-mutating endpoints.
|
||||
|
||||
### What to do
|
||||
**Option A (recommended — custom header check):**
|
||||
1. Add a middleware that, for all `POST`/`PUT`/`DELETE`/`PATCH` requests authenticated via cookie (not `Authorization: Bearer`), requires the presence of a custom header: `X-BanGUI-Request: 1`.
|
||||
2. The frontend API client (`frontend/src/api/client.ts`) already uses a shared `request()` function — add `"X-BanGUI-Request": "1"` to the default headers there.
|
||||
3. Cross-site `fetch()` calls cannot set custom headers without CORS preflight, which the backend rejects (CORS is only configured for allowed origins).
|
||||
|
||||
**Option B — Origin header validation:**
|
||||
Add middleware that checks `Origin` or `Referer` matches the configured allowed origin for all mutating requests.
|
||||
|
||||
### Possible traps and issues
|
||||
- The Bearer-token path (`Authorization: Bearer`) does not use cookies and is therefore not CSRF-vulnerable — do not apply the check to those requests.
|
||||
- Detecting cookie-vs-bearer authentication in middleware requires reading request headers before the auth dependency runs — check for `Cookie: bangui_session=` presence.
|
||||
- Do not apply CSRF checks to `GET`, `HEAD`, `OPTIONS` requests.
|
||||
|
||||
### Docs changes needed
|
||||
- `Architekture.md` — document the CSRF protection mechanism.
|
||||
- `Backend-Development.md` — CSRF middleware.
|
||||
- `Web-Development.md` — document the `X-BanGUI-Request` header requirement.
|
||||
|
||||
### Doc references
|
||||
- [Architekture.md](Architekture.md) — security architecture
|
||||
- [Backend-Development.md](Backend-Development.md) — security patterns
|
||||
- [Web-Development.md](Web-Development.md) — API client conventions
|
||||
|
||||
---
|
||||
|
||||
## TASK-025 — `unwrap_session_token` legacy fallback bypasses HMAC check entirely
|
||||
|
||||
**Severity:** High
|
||||
|
||||
### Where found
|
||||
`backend/app/services/auth_service.py` — `unwrap_session_token()` lines 44–49:
|
||||
```python
|
||||
if SESSION_TOKEN_SIGNATURE_SEPARATOR not in token:
|
||||
return token # HMAC check skipped entirely
|
||||
```
|
||||
|
||||
### Why this is needed
|
||||
Any token that does not contain the separator character is returned unchanged as a "valid" token — the HMAC signature is never verified. Combined with TASK-022 (plaintext DB), an attacker who reads the database can take a raw token (no separator) and use it directly, bypassing the HMAC layer entirely. The signing mechanism provides zero additional security once the DB is readable.
|
||||
|
||||
### Goal
|
||||
Remove the HMAC bypass. All tokens must carry a valid signature.
|
||||
|
||||
### What to do
|
||||
1. Remove the early-return branch: `if SESSION_TOKEN_SIGNATURE_SEPARATOR not in token: return token`.
|
||||
2. If the separator is absent, raise `ValueError("Invalid session token.")`.
|
||||
3. This invalidates all sessions created before HMAC signing was introduced — coordinate with TASK-022 (all sessions should be invalidated during that migration anyway).
|
||||
4. Update all tests that use raw unsigned tokens.
|
||||
|
||||
### Possible traps and issues
|
||||
- Any test that constructs a raw token without a signature will start failing — this is intentional, update the tests.
|
||||
- The `unwrap_session_token` docstring mentions "backward compatibility with existing raw session tokens stored in the DB" — remove this rationale once TASK-022 hashes the DB column (raw tokens will no longer be in the DB).
|
||||
|
||||
### Docs changes needed
|
||||
- `Backend-Development.md` — document the session token format (signed only).
|
||||
|
||||
### Doc references
|
||||
- [Backend-Development.md](Backend-Development.md) — authentication internals
|
||||
|
||||
---
|
||||
|
||||
## TASK-026 — OpenAPI docs (`/docs`, `/redoc`) exposed without authentication in production
|
||||
|
||||
**Severity:** Medium
|
||||
|
||||
@@ -162,6 +162,14 @@ class Settings(BaseSettings):
|
||||
"Example: 'systemctl start fail2ban' or 'fail2ban-client start'."
|
||||
),
|
||||
)
|
||||
enable_docs: bool = Field(
|
||||
default=False,
|
||||
description=(
|
||||
"Enable FastAPI interactive API documentation at /api/docs (Swagger UI) "
|
||||
"and /api/redoc (ReDoc). Should be true only in development environments. "
|
||||
"In production, leave unset (defaults to false) to avoid exposing API schema."
|
||||
),
|
||||
)
|
||||
|
||||
@field_validator("fail2ban_start_command", mode="after")
|
||||
@classmethod
|
||||
|
||||
@@ -401,7 +401,7 @@ async def _service_unavailable_handler(
|
||||
|
||||
# Paths that are always reachable, even before setup is complete.
|
||||
_ALWAYS_ALLOWED: frozenset[str] = frozenset(
|
||||
{"/api/setup", "/api/health"},
|
||||
{"/api/setup", "/api/health", "/api/docs", "/api/redoc", "/api/openapi.json"},
|
||||
)
|
||||
|
||||
|
||||
@@ -469,11 +469,20 @@ def create_app(settings: Settings | None = None) -> FastAPI:
|
||||
"""
|
||||
resolved_settings: Settings = settings if settings is not None else get_settings()
|
||||
|
||||
# Configure API docs based on enable_docs setting.
|
||||
# In production, docs are disabled (None). In development, docs are served at /api/*.
|
||||
docs_url = "/api/docs" if resolved_settings.enable_docs else None
|
||||
redoc_url = "/api/redoc" if resolved_settings.enable_docs else None
|
||||
openapi_url = "/api/openapi.json" if resolved_settings.enable_docs else None
|
||||
|
||||
app: FastAPI = FastAPI(
|
||||
title="BanGUI",
|
||||
description="Web interface for monitoring, managing, and configuring fail2ban.",
|
||||
version=__version__,
|
||||
lifespan=_lifespan,
|
||||
docs_url=docs_url,
|
||||
redoc_url=redoc_url,
|
||||
openapi_url=openapi_url,
|
||||
)
|
||||
|
||||
# Store immutable configuration and the dedicated runtime state manager on
|
||||
|
||||
@@ -143,6 +143,46 @@ def test_create_app_disables_cors_by_default() -> None:
|
||||
assert cors_middleware == []
|
||||
|
||||
|
||||
def test_create_app_disables_api_docs_by_default() -> None:
|
||||
"""API documentation endpoints are disabled when enable_docs is false."""
|
||||
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",
|
||||
session_duration_minutes=60,
|
||||
timezone="UTC",
|
||||
log_level="debug",
|
||||
enable_docs=False,
|
||||
)
|
||||
|
||||
app = create_app(settings=settings)
|
||||
|
||||
assert app.docs_url is None
|
||||
assert app.redoc_url is None
|
||||
assert app.openapi_url is None
|
||||
|
||||
|
||||
def test_create_app_enables_api_docs_when_configured() -> None:
|
||||
"""API documentation endpoints are enabled at /api/* when enable_docs is true."""
|
||||
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",
|
||||
session_duration_minutes=60,
|
||||
timezone="UTC",
|
||||
log_level="debug",
|
||||
enable_docs=True,
|
||||
)
|
||||
|
||||
app = create_app(settings=settings)
|
||||
|
||||
assert app.docs_url == "/api/docs"
|
||||
assert app.redoc_url == "/api/redoc"
|
||||
assert app.openapi_url == "/api/openapi.json"
|
||||
|
||||
|
||||
async def test_lifespan_initialises_and_cleans_up_shared_resources(tmp_path: Path) -> None:
|
||||
"""The app lifespan creates and shuts down shared resources cleanly."""
|
||||
settings = Settings(
|
||||
|
||||
Reference in New Issue
Block a user