From 77e07e87dc8cb3f5f48444e33e6173398ed62381 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 3 Aug 2026 22:59:00 +0200 Subject: [PATCH] Audit-log bug report status changes (B-60) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other admin mutation — config edit, pause/resume, privkey export, password reset — leaves a trace; this one could silently mark a report resolved. With one shared ADMIN_TOKEN and no per-admin identity, the audit log is the only accountability there is. bug_report_status_changed records the report id and the before/after status, and carries the report's author as user_id so the entry is traceable from either side. Nothing is written when the status doesn't actually change, matching config_updated: an edit that changes nothing isn't an event, and noise hides the real changes. Also documents the new event in docs/guida-admin.md's audit table. Co-Authored-By: Claude Opus 5 --- BUGS.md | 13 --------- CLAUDE.md | 4 +-- app/api/routes/admin.py | 15 +++++++++- docs/guida-admin.md | 1 + tests/unit/test_bug_reports.py | 53 ++++++++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 16 deletions(-) diff --git a/BUGS.md b/BUGS.md index 02453c2..33d44ca 100644 --- a/BUGS.md +++ b/BUGS.md @@ -38,19 +38,6 @@ remains the last prerequisite for running unattended. --- -## High — security - -### B-60 — `POST /admin/bug-reports/{id}/status` writes no audit entry - -`app/api/routes/admin.py:404-419`. - -Every other admin mutation (config edit, pause/resume, privkey export, password -reset) is audit-logged. This one is not, so a report can be silently marked -`resolved` with no trace — and with one shared `ADMIN_TOKEN` and no per-admin -identity, the audit log is the only accountability there is. - ---- - ## Medium — correctness and robustness ### B-61 — config edits apply retroactively to the round already in progress diff --git a/CLAUDE.md b/CLAUDE.md index 08c0906..c2c0f0c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 302 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 305 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. Verified on mainnet with real money: registration + address derivation, deposit crediting (1-conf), a real 10 PLM bet (broadcast → confirmed → change credited back), and one full round cycle (close → draw on a real block hash → 70/30 payout with sat math checked against the broadcast tx → confirmation → close → next round auto-opened). **Withdrawal and the RBF bump path have never been exercised against a live broadcast** — unit-tested only. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 302 tests +python -m pytest # all 305 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` diff --git a/app/api/routes/admin.py b/app/api/routes/admin.py index ca357b9..c515e7c 100644 --- a/app/api/routes/admin.py +++ b/app/api/routes/admin.py @@ -408,7 +408,20 @@ async def update_bug_report_status( if report is None: raise HTTPException(status.HTTP_404_NOT_FOUND, "bug report not found") - report.status = body.status + # B-60: every other admin mutation (config edit, pause/resume, privkey export, + # password reset) leaves a trace; this one silently marked a report `resolved`. + # With one shared ADMIN_TOKEN and no per-admin identity, the audit log is the + # only accountability there is. Before/after like config_updated, and nothing + # written when the status doesn't actually change — re-clicking the status a + # report already has isn't an event. + if report.status != body.status: + await write_audit_log( + session, + "bug_report_status_changed", + {"report_id": report_id, "from": report.status, "to": body.status}, + user_id=report.user_id, + ) + report.status = body.status await session.commit() username = None diff --git a/docs/guida-admin.md b/docs/guida-admin.md index abcb212..fceca32 100644 --- a/docs/guida-admin.md +++ b/docs/guida-admin.md @@ -116,6 +116,7 @@ Eventi a cui vale la pena prestare attenzione: | `bet_broadcast_failed` / `withdrawal_broadcast_failed` | La rete ha rifiutato la transazione. Non è stato speso nulla: gli UTXO sono stati liberati e il saldo dell'utente è tornato come prima. | | `pending_tx_abandoned` | Una transazione trasmessa è scomparsa dalla catena e il sistema l'ha dichiarata persa: UTXO liberati, bet rimossa o prelievo segnato `failed`. Se capita spesso, la fee rate configurata è probabilmente troppo bassa. | | `pending_tx_recovered` | Una transazione che si credeva incompleta è invece finita in catena (tipicamente dopo un riavvio a metà invio) e il sistema l'ha ripresa da sé. | +| `bug_report_status_changed` | Un admin ha cambiato lo stato di una segnalazione (payload: `report_id`, stato precedente e nuovo). Con un token admin unico e condiviso, questa riga è l'unica traccia di chi tocca le segnalazioni. | | `payout_failed` | Il payout di un round non è partito. Il round resta in `paying_out` e **richiede intervento manuale**: non esiste un retry automatico. Controlla `fee_address`, il saldo dell'indirizzo pool e la connessione Electrum. | ## Alternative all'interfaccia grafica diff --git a/tests/unit/test_bug_reports.py b/tests/unit/test_bug_reports.py index 88324ba..9f3af58 100644 --- a/tests/unit/test_bug_reports.py +++ b/tests/unit/test_bug_reports.py @@ -179,3 +179,56 @@ async def test_update_status_unknown_report_is_404(client): "/admin/bug-reports/999/status", headers=_ADMIN_HEADERS, json={"status": "read"} ) assert resp.status_code == 404 + + +# --- B-60: the status change is an admin mutation, so it leaves a trace ---------- + + +async def _audit_entries(event_type: str) -> list: + from sqlalchemy import select + + from app.db.base import AsyncSessionLocal + from app.db.models import AuditLog + + async with AsyncSessionLocal() as session: + return ( + await session.scalars(select(AuditLog).where(AuditLog.event_type == event_type)) + ).all() + + +async def test_status_change_is_audit_logged(client): # B-60 + """One shared ADMIN_TOKEN and no per-admin identity means the audit log is the + only accountability there is — a report could be silently marked resolved.""" + token = await _register(client, username="reporter") + resp = await client.post( + "/bug-reports", + json={"description": "some bug"}, + headers={"Authorization": f"Bearer {token}"}, + ) + report_id = resp.json()["id"] + + await client.post( + f"/admin/bug-reports/{report_id}/status", headers=_ADMIN_HEADERS, json={"status": "resolved"} + ) + + entries = await _audit_entries("bug_report_status_changed") + assert len(entries) == 1 + import json + + payload = json.loads(entries[0].payload_json) + assert payload == {"report_id": report_id, "from": "open", "to": "resolved"} + assert entries[0].user_id is not None # the report's author, so it's traceable both ways + + +async def test_setting_the_status_it_already_has_logs_nothing(client): # B-60 + """Same rule as config_updated: an edit that changes nothing isn't an event, or + the log fills with noise that hides the real changes.""" + resp = await client.post("/bug-reports", json={"description": "some bug"}) + report_id = resp.json()["id"] + + for _ in range(3): + await client.post( + f"/admin/bug-reports/{report_id}/status", headers=_ADMIN_HEADERS, json={"status": "open"} + ) + + assert await _audit_entries("bug_report_status_changed") == []