refactoring-backend #3

Merged
lukas.pupkalipinski merged 403 commits from refactoring-backend into main 2026-05-20 20:23:46 +02:00
3 changed files with 60 additions and 66 deletions
Showing only changes of commit cee3daffc1 - Show all commits

View File

@@ -1,57 +1,3 @@
### Issue #45: HIGH - Session Cache Not Invalidated on Login
**Where found**:
- `backend/app/routers/auth.py:184-195` logout invalidates the cache entry; login does not
**Why this is needed**:
A stolen token `S1` can be re-used for up to the cache TTL seconds after the legitimate user has logged out and logged back in, because the old cache entry is never cleared.
**Goal**:
Invalidate any existing cache entry for a user/session on every new login.
**What to do**:
1. On successful login, look up and delete any existing cache entries bound to the same user.
2. Alternatively, scope cache keys to `(user_id, issued_at)` so old tokens are naturally distinct.
**Possible traps and issues**:
- Cache keys may currently be token-hashes only; ensure the user dimension is queryable.
**Docs changes needed**:
- None beyond inline comments.
**Doc references**:
- `backend/app/dependencies.py` cache key construction
---
### Issue #46: HIGH - Fail2ban DB Opened Without Explicit Read-Only Enforcement
**Where found**:
- `backend/app/repositories/fail2ban_db_repo.py:72` uses `?mode=ro` URI flag
- `backend/app/utils/fail2ban_db_utils.py:46-47` no PRAGMA to enforce read-only semantics
**Why this is needed**:
The `?mode=ro` URI flag is SQLite-level but not verified after connection open. A malformed URI or library version inconsistency could silently open a writable connection, allowing accidental writes to the live fail2ban database.
**Goal**:
Guarantee that no write operations are ever executed against the fail2ban database.
**What to do**:
1. After opening the connection, execute `PRAGMA query_only = ON;` to enforce read-only at the connection level.
2. Wrap the connection factory in an assertion that tries a write and expects it to fail.
**Possible traps and issues**:
- `PRAGMA query_only` is SQLite ≥ 3.8.0; verify the bundled SQLite version.
- aiosqlite wraps the connection; ensure PRAGMA is executed before any queries.
**Docs changes needed**:
- Add inline comment explaining why both URI flag and PRAGMA are used.
**Doc references**:
- `backend/app/services/ban_service.py` module docstring ("read-only mode")
---
### Issue #47: HIGH - CSRF Middleware Hardcodes Cookie Name
**Where found**:

View File

@@ -20,6 +20,8 @@ from fastapi import status
from fastapi.responses import JSONResponse
from starlette.middleware.base import BaseHTTPMiddleware
from app.utils.constants import SESSION_COOKIE_NAME
if TYPE_CHECKING:
from collections.abc import Awaitable, Callable
@@ -35,9 +37,6 @@ _CSRF_HEADER_VALUE: str = "1"
# HTTP methods that require CSRF protection.
_CSRF_PROTECTED_METHODS: frozenset[str] = frozenset({"POST", "PUT", "DELETE", "PATCH"})
# Session cookie name for detecting cookie-based authentication.
_SESSION_COOKIE_NAME: str = "bangui_session"
class CsrfMiddleware(BaseHTTPMiddleware):
"""Protect cookie-authenticated state-mutating requests with custom header check.
@@ -73,7 +72,7 @@ class CsrfMiddleware(BaseHTTPMiddleware):
return await call_next(request)
# Skip check if not using cookie-based authentication.
if _SESSION_COOKIE_NAME not in request.cookies:
if SESSION_COOKIE_NAME not in request.cookies:
return await call_next(request)
# Enforce CSRF header for cookie-authenticated state-mutating requests.

View File

@@ -10,6 +10,7 @@ service layers can focus on business logic and formatting.
from __future__ import annotations
from contextlib import asynccontextmanager
from dataclasses import dataclass
from typing import TYPE_CHECKING
@@ -18,6 +19,7 @@ import aiosqlite
from app.utils.fail2ban_db_utils import escape_like
if TYPE_CHECKING:
from collections.abc import AsyncIterator
from collections.abc import Iterable
from app.models.ban import BanOrigin
@@ -72,6 +74,53 @@ def _make_db_uri(db_path: str) -> str:
return f"file:{db_path}?mode=ro"
async def _acquire_readonly_connection(
db_path: str,
) -> aiosqlite.Connection:
"""Open a read-only connection to the fail2ban database.
Defense-in-depth: both the ``?mode=ro`` URI flag AND the SQLite-level
``PRAGMA query_only = ON`` are applied. The URI flag is a library-level hint
that can be bypassed by malformed URIs or version inconsistencies;
``query_only`` is a connection-level enforcement that makes all write
operations fail. We verify enforcement by reading back the PRAGMA value.
Args:
db_path: Path to the fail2ban SQLite database.
Returns:
An aiosqlite connection in guaranteed read-only mode.
Raises:
AssertionError: If PRAGMA query_only is not confirmed as enabled.
"""
conn = await aiosqlite.connect(_make_db_uri(db_path), uri=True)
# Set connection-level read-only enforcement and verify in one statement.
# Even if the ?mode=ro URI flag is bypassed, this PRAGMA blocks writes.
cursor = await conn.execute("PRAGMA query_only = ON")
await cursor.close()
# Verify the PRAGMA took effect.
cursor = await conn.execute("PRAGMA query_only")
row = await cursor.fetchone()
await cursor.close()
if not row or row[0] != 1:
await conn.close()
raise AssertionError(
"PRAGMA query_only is not enabled; connection may be writable"
)
return conn
@asynccontextmanager
async def _readonly_connection(db_path: str) -> AsyncIterator[aiosqlite.Connection]:
"""Async context manager that yields a read-only fail2ban DB connection."""
conn = await _acquire_readonly_connection(db_path)
try:
yield conn
finally:
await conn.close()
def _origin_sql_filter(origin: BanOrigin | None) -> tuple[str, tuple[str, ...]]:
"""Return a SQL fragment and parameters for the origin filter."""
@@ -116,7 +165,7 @@ def _rows_to_history_records(rows: Iterable[aiosqlite.Row]) -> list[HistoryRecor
async def check_db_nonempty(db_path: str) -> bool:
"""Return True if the fail2ban database contains at least one ban row."""
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db, db.execute(
async with _readonly_connection(db_path) as db, db.execute(
"SELECT 1 FROM bans LIMIT 1"
) as cur:
row = await cur.fetchone()
@@ -155,7 +204,7 @@ async def get_currently_banned(
placeholder = ", ".join("?" for _ in ip_filter)
ip_filter_clause = f" AND ip IN ({placeholder})"
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
@@ -195,7 +244,7 @@ async def get_ban_counts_by_bucket(
origin_clause, origin_params = _origin_sql_filter(origin)
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
"SELECT CAST((timeofban - ?) / ? AS INTEGER) AS bucket_idx, "
@@ -225,7 +274,7 @@ async def get_ban_event_counts(
origin_clause, origin_params = _origin_sql_filter(origin)
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
"SELECT ip, COUNT(*) AS event_count "
@@ -250,7 +299,7 @@ async def get_bans_by_jail(
origin_clause, origin_params = _origin_sql_filter(origin)
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
@@ -283,7 +332,7 @@ async def get_bans_table_summary(
empty the min/max values will be ``None``.
"""
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
"SELECT COUNT(*), MIN(timeofban), MAX(timeofban) FROM bans"
@@ -337,7 +386,7 @@ async def get_history_page(
effective_page_size: int = page_size
offset: int = (page - 1) * effective_page_size
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
@@ -362,7 +411,7 @@ async def get_history_page(
async def get_history_for_ip(db_path: str, ip: str) -> list[HistoryRecord]:
"""Return the full ban timeline for a specific IP."""
async with aiosqlite.connect(_make_db_uri(db_path), uri=True) as db:
async with _readonly_connection(db_path) as db:
db.row_factory = aiosqlite.Row
async with db.execute(
"SELECT jail, ip, timeofban, bancount, data "