diff --git a/Docs/Architekture.md b/Docs/Architekture.md index 6614032..64bdca7 100644 --- a/Docs/Architekture.md +++ b/Docs/Architekture.md @@ -244,6 +244,7 @@ APScheduler background jobs that run on a schedule without user interaction. | `geo_re_resolve.py` | Periodically re-resolves stale entries in `geo_cache` to keep geolocation data fresh | | `health_check.py` | Periodically pings the fail2ban socket and updates the cached server status so the frontend always has fresh data | | `history_sync.py` | Periodically copies new records from the fail2ban SQLite database into BanGUI's `history_archive` table; delegates the sync algorithm to `history_service.py` | +| `session_cleanup.py` | Periodically removes expired sessions from the `sessions` SQLite table (default: every 6 hours). Without this cleanup, the table grows unbounded and degrades query performance. | #### Utils (`app/utils/`) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index eceee0b..928de62 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,37 +1,3 @@ -## TASK-007 — No rate limiting on the login endpoint - -**Severity:** High - -### Where found -`backend/app/routers/auth.py` — `POST /api/auth/login`. No rate-limiting middleware, no lockout logic. - -### Why this is needed -BanGUI uses a single master password for all access. An attacker with network access to the login endpoint can attempt unlimited passwords per second. With no lockout or slowdown, a 10-character password with the required complexity (upper, digit, special) can be brute-forced offline or via the API. - -### Goal -Limit login attempts per IP address to make brute-force attacks infeasible. - -### What to do -1. Add [`slowapi`](https://github.com/laurents/slowapi) (or a simple in-memory counter) to rate-limit `POST /api/auth/login`. -2. Limit to 5 attempts per minute per IP. Return `429 Too Many Requests` with a `Retry-After` header on excess. -3. For the in-memory approach: use a `dict[str, deque[float]]` keyed by IP, storing attempt timestamps, cleaned up by a background task. -4. Consider a 10-second `asyncio.sleep` on wrong password to further slow down attacks (already somewhat mitigated by bcrypt cost factor). - -### Possible traps and issues -- Behind nginx, `request.client.host` is the proxy IP. Read the real IP from `X-Forwarded-For` only when the request comes from a trusted proxy (the nginx container's IP). Do not blindly trust `X-Forwarded-For` — it can be spoofed. -- The in-memory rate-limiter is process-local (see TASK-002/003) — in a multi-worker setup, each worker has its own counter. Single-worker constraint limits the blast radius. -- `slowapi` integrates cleanly with FastAPI and handles the `X-Real-IP` / `X-Forwarded-For` header logic. - -### Docs changes needed -- `Features.md` — document login rate limiting. -- `Backend-Development.md` — rate limiting conventions. - -### Doc references -- [Features.md](Features.md) — authentication -- [Backend-Development.md](Backend-Development.md) — security patterns - ---- - ## TASK-008 — `delete_expired_sessions` never scheduled — sessions table grows unbounded **Severity:** Medium diff --git a/backend/app/startup.py b/backend/app/startup.py index b9bc80e..d1c10cf 100644 --- a/backend/app/startup.py +++ b/backend/app/startup.py @@ -19,7 +19,7 @@ from apscheduler.schedulers.asyncio import AsyncIOScheduler # type: ignore[impo from app.db import init_db, open_db from app.services import setup_service from app.services.geo_cache import GeoCache -from app.tasks import blocklist_import, geo_cache_flush, geo_re_resolve, health_check, history_sync +from app.tasks import blocklist_import, geo_cache_flush, geo_re_resolve, health_check, history_sync, session_cleanup from app.utils.async_utils import run_blocking from app.utils.jail_config import ensure_jail_configs from app.utils.runtime_state import set_runtime_settings @@ -176,6 +176,7 @@ async def startup_shared_resources( geo_cache_flush.register(app) geo_re_resolve.register(app) history_sync.register(app) + session_cleanup.register(app) return http_session, scheduler except Exception: diff --git a/backend/app/tasks/session_cleanup.py b/backend/app/tasks/session_cleanup.py new file mode 100644 index 0000000..cdb78cd --- /dev/null +++ b/backend/app/tasks/session_cleanup.py @@ -0,0 +1,72 @@ +"""Session cleanup background task. + +Registers an APScheduler job that periodically removes expired sessions from +the ``sessions`` table. Without this cleanup, the table grows unbounded over +months of operation and degrades query performance. + +Individual expired sessions are removed on-demand when validated, but the bulk +cleanup ensures comprehensive pruning at a predictable interval. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import structlog + +from app.repositories import session_repo +from app.tasks.db import task_db +from app.utils.runtime_state import get_effective_settings +from app.utils.time_utils import utc_now + +if TYPE_CHECKING: + from fastapi import FastAPI + + from app.config import Settings + +log: structlog.stdlib.BoundLogger = structlog.get_logger() + +#: How often the cleanup job fires (seconds). Configurable tuning constant. +SESSION_CLEANUP_INTERVAL: int = 6 * 60 * 60 # 6 hours + +#: Stable APScheduler job ID — ensures re-registration replaces, not duplicates. +JOB_ID: str = "session_cleanup" + + +async def _run_cleanup_with_resources(settings: Settings) -> None: + """Delete all expired sessions from the database. + + Args: + settings: The resolved application settings used for database access. + """ + now_iso = utc_now().isoformat() + async with task_db(settings) as db: + deleted_count = await session_repo.delete_expired_sessions(db, now_iso) + + log.info("session_cleanup_ran", deleted_count=deleted_count, cutoff_time=now_iso) + + +async def _run_cleanup(app: FastAPI) -> None: + await _run_cleanup_with_resources(get_effective_settings(app)) + + +def register(app: FastAPI) -> None: + """Add (or replace) the session cleanup job in the application scheduler. + + Must be called after the scheduler has been started (i.e., inside the + lifespan handler, after ``scheduler.start()``). + + Args: + app: The :class:`fastapi.FastAPI` application instance whose + ``app.state.scheduler`` will receive the job. + """ + settings = get_effective_settings(app) + app.state.scheduler.add_job( + _run_cleanup_with_resources, + trigger="interval", + seconds=SESSION_CLEANUP_INTERVAL, + kwargs={"settings": settings}, + id=JOB_ID, + replace_existing=True, + ) + log.info("session_cleanup_scheduled", interval_seconds=SESSION_CLEANUP_INTERVAL) diff --git a/backend/tests/test_tasks/test_session_cleanup.py b/backend/tests/test_tasks/test_session_cleanup.py new file mode 100644 index 0000000..e07b426 --- /dev/null +++ b/backend/tests/test_tasks/test_session_cleanup.py @@ -0,0 +1,193 @@ +"""Tests for the session cleanup background task. + +Validates that :func:`~app.tasks.session_cleanup._run_cleanup_with_resources` +correctly calls the repository function to delete expired sessions and logs +the results, and that :func:`~app.tasks.session_cleanup.register` configures +the APScheduler job with the correct interval and stable job ID. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from app.tasks.session_cleanup import ( + JOB_ID, + SESSION_CLEANUP_INTERVAL, + register, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_app() -> MagicMock: + """Build a minimal mock ``app`` for session cleanup task tests. + + Returns: + A :class:`unittest.mock.MagicMock` that mimics ``fastapi.FastAPI``. + """ + app = MagicMock() + app.state.scheduler = MagicMock() + app.state.settings = MagicMock(database_path="/tmp/fake.db") + app.state.runtime_settings = None + return app + + +# --------------------------------------------------------------------------- +# Tests for _run_cleanup_with_resources +# --------------------------------------------------------------------------- + + +class TestRunCleanup: + """Tests for :func:`~app.tasks.session_cleanup._run_cleanup_with_resources`.""" + + @pytest.mark.asyncio + async def test_run_cleanup_calls_delete_expired_sessions(self) -> None: + """``_run_cleanup_with_resources`` must call ``delete_expired_sessions`` with db and now_iso.""" + settings = MagicMock(database_path="/tmp/fake.db") + + mock_db = MagicMock() + with patch( + "app.tasks.session_cleanup.task_db", + MagicMock( + return_value=AsyncMock( + __aenter__=AsyncMock(return_value=mock_db), + __aexit__=AsyncMock(return_value=False), + ) + ), + ), patch( + "app.tasks.session_cleanup.session_repo.delete_expired_sessions", + new_callable=AsyncMock, + return_value=0, + ) as mock_delete: + from app.tasks.session_cleanup import _run_cleanup_with_resources + + await _run_cleanup_with_resources(settings) + + mock_delete.assert_awaited_once() + # Verify db was passed as first argument + call_args = mock_delete.await_args + assert call_args[0][0] == mock_db + + @pytest.mark.asyncio + async def test_run_cleanup_logs_deleted_count(self) -> None: + """``_run_cleanup_with_resources`` must log the count of deleted sessions.""" + settings = MagicMock(database_path="/tmp/fake.db") + + with patch( + "app.tasks.session_cleanup.task_db", + MagicMock( + return_value=AsyncMock( + __aenter__=AsyncMock(return_value=MagicMock()), + __aexit__=AsyncMock(return_value=False), + ) + ), + ), patch( + "app.tasks.session_cleanup.session_repo.delete_expired_sessions", + new_callable=AsyncMock, + return_value=42, + ), patch( + "app.tasks.session_cleanup.log" + ) as mock_log: + from app.tasks.session_cleanup import _run_cleanup_with_resources + + await _run_cleanup_with_resources(settings) + + # Verify the log.info call happened with deleted_count + info_calls = [c for c in mock_log.info.call_args_list if c[0][0] == "session_cleanup_ran"] + assert len(info_calls) == 1 + assert info_calls[0][1]["deleted_count"] == 42 + + @pytest.mark.asyncio + async def test_run_cleanup_logs_even_with_zero_deletions(self) -> None: + """``_run_cleanup_with_resources`` must log even when no sessions were deleted.""" + settings = MagicMock(database_path="/tmp/fake.db") + + with patch( + "app.tasks.session_cleanup.task_db", + MagicMock( + return_value=AsyncMock( + __aenter__=AsyncMock(return_value=MagicMock()), + __aexit__=AsyncMock(return_value=False), + ) + ), + ), patch( + "app.tasks.session_cleanup.session_repo.delete_expired_sessions", + new_callable=AsyncMock, + return_value=0, + ), patch( + "app.tasks.session_cleanup.log" + ) as mock_log: + from app.tasks.session_cleanup import _run_cleanup_with_resources + + await _run_cleanup_with_resources(settings) + + info_calls = [c for c in mock_log.info.call_args_list if c[0][0] == "session_cleanup_ran"] + assert len(info_calls) == 1 + assert info_calls[0][1]["deleted_count"] == 0 + + +# --------------------------------------------------------------------------- +# Tests for register +# --------------------------------------------------------------------------- + + +class TestRegister: + """Tests for :func:`~app.tasks.session_cleanup.register`.""" + + def test_register_adds_interval_job_to_scheduler(self) -> None: + """``register`` must add a job with an ``"interval"`` trigger.""" + app = _make_app() + + register(app) + + app.state.scheduler.add_job.assert_called_once() + _, kwargs = app.state.scheduler.add_job.call_args + assert kwargs["trigger"] == "interval" + assert kwargs["seconds"] == SESSION_CLEANUP_INTERVAL + + def test_register_uses_stable_job_id(self) -> None: + """``register`` must use the module-level ``JOB_ID`` constant.""" + app = _make_app() + + register(app) + + _, kwargs = app.state.scheduler.add_job.call_args + assert kwargs["id"] == JOB_ID + + def test_register_sets_replace_existing(self) -> None: + """``register`` must use ``replace_existing=True`` to avoid duplicate jobs.""" + app = _make_app() + + register(app) + + _, kwargs = app.state.scheduler.add_job.call_args + assert kwargs["replace_existing"] is True + + def test_register_passes_settings_in_kwargs(self) -> None: + """The scheduled job must receive settings as kwargs.""" + app = _make_app() + + register(app) + + _, kwargs = app.state.scheduler.add_job.call_args + assert "settings" in kwargs["kwargs"] + + def test_register_logs_scheduling_confirmation(self) -> None: + """``register`` must emit an info log when scheduling the job.""" + app = _make_app() + + with patch("app.tasks.session_cleanup.log") as mock_log: + register(app) + + info_calls = [ + c + for c in mock_log.info.call_args_list + if c[0][0] == "session_cleanup_scheduled" + ] + assert len(info_calls) == 1 + assert info_calls[0][1]["interval_seconds"] == SESSION_CLEANUP_INTERVAL