Refactor backend architecture and update documentation
- Add CSRF protection middleware implementation - Update API client with improved configuration - Enhance documentation for backend development - Add architecture documentation updates - Reorganize and clean up task documentation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1,67 +1,3 @@
|
||||
## 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
|
||||
|
||||
Reference in New Issue
Block a user