## 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 ### Where found `backend/app/routers/jail_config.py` — `DELETE /api/config/jails/{name}/logpath` — `log_path: str = Query(...)`. ### Why this is needed The `log_path` query parameter is passed directly to the fail2ban socket command `["set", name, "dellogpath", log_path]` without any path validation. An attacker could pass traversal strings or paths to sensitive files, instructing fail2ban to stop monitoring them and potentially confusing fail2ban's internal state. ### Goal Apply the same allowlist validation as `add_log_path` (TASK-014) to `delete_log_path`. ### What to do 1. Extract the log path validation logic from TASK-014 into a shared helper function in `backend/app/utils/path_utils.py` (e.g., `validate_log_path(path: str, allowed_dirs: list[str]) -> str`). 2. Call the helper from both `AddLogPathRequest` validator and the `delete_log_path` route handler. 3. Return 422 with a descriptive error if validation fails. ### Possible traps and issues - Query parameters cannot have Pydantic field validators directly in FastAPI — use a `Depends` dependency that validates and returns the resolved path, or validate explicitly at the start of the route handler. ### Docs changes needed - `Backend-Development.md` — path validation helper usage. ### Doc references - [Backend-Development.md](Backend-Development.md) — input validation patterns --- ## TASK-017 — `ip LIKE ?` without escaping `%` and `_` wildcards **Severity:** Medium ### Where found `backend/app/repositories/fail2ban_db_repo.py` and `backend/app/repositories/history_archive_repo.py` — SQL queries using `ip LIKE ?` with `f"{ip_filter}%"` interpolation. ### Why this is needed SQLite's `LIKE` operator treats `%` (any sequence of characters) and `_` (any single character) as wildcards. If an IP filter value contains these characters — unusual for well-formed IPs, but possible via crafted input — the query matches unintended rows. For example, `ip_filter = "10.0.0_"` would match `10.0.0.1` through `10.0.0.9`. ### Goal Escape LIKE metacharacters in all `LIKE` query parameters. ### What to do 1. Escape `%` → `\%` and `_` → `\_` in the filter string before use. 2. Add `ESCAPE '\'` to the SQL: `ip LIKE ? ESCAPE '\'`. 3. Extract this into a helper: `def escape_like(s: str) -> str: return s.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_")`. ### Possible traps and issues - The backslash escape character itself must also be escaped first to avoid double-escaping. Process in the order: `\` → `\\`, then `%` → `\%`, then `_` → `\_`. - Test with IPs that contain dots — dots are not LIKE wildcards in SQLite, so they do not need escaping. ### Docs changes needed - `Backend-Development.md` — database query conventions (LIKE escaping). ### Doc references - [Backend-Development.md](Backend-Development.md) — database access patterns --- ## TASK-018 — `_write_conf_file` and `_create_conf_file` not atomic **Severity:** Medium ### Where found `backend/app/services/config_file_helpers.py` lines ~190 (`_write_conf_file`) and ~226 (`_create_conf_file`) — both use `target.write_text(content, encoding="utf-8")` directly. ### Why this is needed `Path.write_text()` overwrites the file in place. If the process is killed mid-write, the config file is left in a truncated or corrupt state. fail2ban config files are critical — a corrupt `jail.d/sshd.conf` prevents fail2ban from reloading and may disable active protection. ### Goal Make all config file writes atomic using write-to-temp + rename. ### What to do 1. Replace `target.write_text(content)` with: ```python import tempfile, os tmp_fd, tmp_path = tempfile.mkstemp(dir=target.parent, suffix=".tmp") try: with os.fdopen(tmp_fd, "w", encoding="utf-8") as f: f.write(content) os.replace(tmp_path, target) except Exception: os.unlink(tmp_path) raise ``` 2. Apply to both `_write_conf_file` and `_create_conf_file`. 3. This pattern is already used correctly in `jail_config_service.py` — follow that exact implementation. ### Possible traps and issues - The temp file must be in the same directory as the target (`dir=target.parent`) so `os.replace()` is atomic (same filesystem, single rename syscall). - On Windows, `os.replace()` may fail if the target is open — not relevant for Linux containers, but worth noting. ### Docs changes needed - `Backend-Development.md` — atomic file write conventions. ### Doc references - [Backend-Development.md](Backend-Development.md) — file I/O patterns --- ## TASK-019 — `session_secret` has no minimum-length enforcement **Severity:** Medium ### Where found `backend/app/config.py` — `session_secret: str = Field(..., description="...")`. No `min_length` constraint. ### Why this is needed `session_secret` is the HMAC key used to sign all session tokens. A secret shorter than 32 characters (256 bits) significantly weakens HMAC-SHA256. The app currently accepts any non-empty string, including a single character. ### Goal Enforce a minimum secret length of 32 characters at startup. ### What to do 1. Add `min_length=32` to the `session_secret` Field definition. 2. Update the error message to explain: `"session_secret must be at least 32 characters. Generate one with: python -c \"import secrets; print(secrets.token_hex(32))\""`. ### Possible traps and issues - This is a breaking change for any existing deployment where the secret is shorter than 32 characters. Include a migration note in the changelog. - The debug compose file uses `dev-secret-do-not-use-in-production` (42 chars) — this already passes the 32-char check, so dev environments are unaffected. ### Docs changes needed - `Backend-Development.md` — document the `session_secret` constraint. ### Doc references - [Backend-Development.md](Backend-Development.md) — configuration reference --- ## TASK-020 — `log_target` accepts arbitrary paths — root file write via fail2ban (CRITICAL) **Severity:** Critical ### Where found `backend/app/models/config.py` — `GlobalConfigUpdate.log_target: str | None`. `backend/app/services/config_service.py` — `update_global_config()` forwards the value to fail2ban without validation. ### Why this is needed fail2ban runs as root. When `log_target` is set to a path, fail2ban opens (and if necessary creates) that file for writing. An authenticated user can send `PUT /api/config/global` with `{"log_target": "/etc/cron.d/bangui-pwned"}`, causing fail2ban to create that file as root. With crafted content appended via fail2ban's own logging, this escalates to a root write primitive and potentially to Remote Code Execution. ### Goal Block all `log_target` values that are not `"STDOUT"`, `"STDERR"`, `"SYSLOG"`, or a path under the configured allowed log directories. ### What to do 1. **Immediate:** Add a strict `@field_validator("log_target")` to `GlobalConfigUpdate` that enforces the allowlist (see TASK-015 — this task and TASK-015 share the same fix). 2. **Defense in depth:** Before sending the command to fail2ban in `update_global_config()`, validate again at the service layer (not just the model layer). 3. Add a regression test: `POST /api/config/global` with `log_target="/etc/passwd"` must return 422. ### Possible traps and issues - This must be fixed before TASK-015 since it is the more severe variant. The fixes are identical — implement them together. - Pydantic model validators run before the service receives the value, but an integration test confirming the full request path is essential. ### Docs changes needed - `Features.md` — document valid log_target values. - `Backend-Development.md` — critical input validation requirement for config endpoints. ### Doc references - [Features.md](Features.md) — fail2ban global configuration - [Backend-Development.md](Backend-Development.md) — input validation --- ## TASK-021 — `set_jail_config_enabled` and `write_jail_config_file` not atomic **Severity:** Medium ### Where found `backend/app/services/raw_config_io_service.py` lines ~268 (`set_jail_config_enabled`) and ~344 (`write_jail_config_file`) — both use `path.write_text(updated)` directly. ### Why this is needed Same root cause as TASK-018. A process kill mid-write leaves the jail config file corrupted, disabling that jail on next fail2ban reload. ### Goal Atomic writes for `set_jail_config_enabled` and `write_jail_config_file`. ### What to do Same as TASK-018: replace `path.write_text(content)` with the `NamedTemporaryFile` + `os.replace()` pattern in both functions. This is most efficiently done as part of TASK-018 by extracting a shared `atomic_write(path, content)` helper in `config_file_helpers.py`. ### Possible traps and issues - Same as TASK-018. - Extracting the helper makes TASK-018 and TASK-021 a single coordinated change. ### Docs changes needed - `Backend-Development.md` — atomic write helper documentation. ### Doc references - [Backend-Development.md](Backend-Development.md) — file I/O conventions --- ## TASK-022 — Session tokens stored in plaintext in SQLite **Severity:** High ### Where found `backend/app/db.py` — `sessions` table schema: `token TEXT NOT NULL UNIQUE`. `backend/app/repositories/session_repo.py` — `INSERT INTO sessions (token, ...)` and `SELECT ... WHERE token = ?` both use the raw token value. ### Why this is needed If the BanGUI SQLite database file is exposed (volume mount misconfiguration, backup leak, path traversal via another vulnerability), all active session tokens are immediately usable — no cracking required. The attacker can directly use the token in the `bangui_session` cookie or `Authorization: Bearer` header. ### Goal Store a one-way hash of the session token in the database so that the DB file alone is not sufficient to hijack a session. ### What to do 1. In `session_repo.create_session()`, store `hashlib.sha256(token.encode()).hexdigest()` instead of `token` in the `token` column. 2. In `session_repo.get_session()` and `delete_session()`, hash the supplied token before the SQL lookup. 3. The `Session` model's `token` field returned to the service layer still contains the raw token (for use in signing and response) — only the DB column changes. 4. Add a migration (`_MIGRATIONS[2]`) that renames the existing `sessions` table to `sessions_old`, creates a new one, and drops `sessions_old` (or simply truncates all sessions on upgrade, since they are all compromised anyway once the DB was readable in plaintext). ### Possible traps and issues - Coordinate with TASK-025 (HMAC bypass) — both fixes invalidate all existing sessions. Do them in the same release. - The migration must be atomic (see TASK-023). - The `Session.token` field name is slightly misleading once it stores a hash — consider renaming the DB column to `token_hash`. ### Docs changes needed - `Architekture.md` — update session data model description. - `Backend-Development.md` — document the session token hashing pattern. ### Doc references - [Architekture.md](Architekture.md) — authentication and session model - [Backend-Development.md](Backend-Development.md) — security patterns --- ## TASK-023 — Database migration is non-atomic **Severity:** Medium ### Where found `backend/app/db.py` — `_apply_migration()`: calls `db.executescript(migration_script)` (which auto-commits per SQLite Python driver behavior) and then separately `db.execute("INSERT INTO schema_migrations ...")` + `db.commit()`. ### Why this is needed `executescript()` issues an implicit `COMMIT` before executing the script, so the schema change and the migration record insertion are in two separate transactions. A process crash between them leaves the database in a migrated-but-unrecorded state. On next startup, the migration is re-applied. For a migration that is not idempotent (e.g., `INSERT` without `OR IGNORE`, `ALTER TABLE ADD COLUMN` without `IF NOT EXISTS`), this causes a runtime error or data duplication. ### Goal Wrap each migration's DDL and its `schema_migrations` record in a single atomic transaction. ### What to do 1. Replace `db.executescript(migration_script)` with individual `await db.execute(stmt)` calls for each DDL statement in the migration (split on `;`). 2. Wrap the entire migration (all DDL statements + the `INSERT INTO schema_migrations`) in an explicit `BEGIN IMMEDIATE` ... `COMMIT` transaction. 3. Test: verify that a simulated crash mid-migration (mocked `execute` that raises on the second statement) leaves the DB at its prior version. ### Possible traps and issues - SQLite DDL in WAL mode: `CREATE TABLE IF NOT EXISTS` and `CREATE INDEX IF NOT EXISTS` are safe to re-run. `ALTER TABLE ADD COLUMN` is not — it must be guarded with a `PRAGMA table_info` check if used in future migrations. - Splitting a migration script on `;` must handle semicolons inside string literals and comments. Consider storing each migration as a `list[str]` of individual statements instead of a single script string. ### Docs changes needed - `Backend-Development.md` — migration authoring guidelines. ### Doc references - [Backend-Development.md](Backend-Development.md) — database schema and migrations --- ## 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 `