Add session cleanup task and update documentation
- Implement session_cleanup task for removing expired sessions - Add comprehensive tests for session cleanup functionality - Update architecture and task documentation - Integrate cleanup task into application startup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -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/`)
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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:
|
||||
|
||||
72
backend/app/tasks/session_cleanup.py
Normal file
72
backend/app/tasks/session_cleanup.py
Normal file
@@ -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)
|
||||
193
backend/tests/test_tasks/test_session_cleanup.py
Normal file
193
backend/tests/test_tasks/test_session_cleanup.py
Normal file
@@ -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
|
||||
Reference in New Issue
Block a user