From bb8b71278aa4015cbee2f538beabb562bf737ee4 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 12:20:22 +0200 Subject: [PATCH] Stamp UTC on naive API timestamps before serializing (B-35) SQLite/aiosqlite returns DateTime columns as naive even though every value is written in UTC, so a bare .isoformat() dropped the offset and the frontend's new Date() parsed it as local time. Add a shared isoformat_utc() helper and use it at every call site that was missing the fix already applied ad hoc in rounds.py. Co-Authored-By: Claude Sonnet 5 --- BUGS.md | 32 +++++++++----------------------- CLAUDE.md | 2 +- app/api/routes/admin.py | 11 ++++++----- app/api/routes/users.py | 3 ++- app/api/timeutil.py | 17 +++++++++++++++++ tests/unit/test_api_timeutil.py | 21 +++++++++++++++++++++ tests/unit/test_users.py | 14 ++++++++++++++ 7 files changed, 70 insertions(+), 30 deletions(-) create mode 100644 app/api/timeutil.py create mode 100644 tests/unit/test_api_timeutil.py diff --git a/BUGS.md b/BUGS.md index 4fd8552..bc78a33 100644 --- a/BUGS.md +++ b/BUGS.md @@ -1,11 +1,11 @@ # Known bugs A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high, -7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-34 are fixed (see "Previously -fixed" below) — no Critical-severity finding remains open; the other 15 are High/Medium/Low. +7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-35 are fixed (see "Previously +fixed" below) — no Critical-severity finding remains open; the other 14 are High/Medium/Low. The 139-test suite was green at the time of the audit, so none of these were caught by existing -coverage — every fix lands with a regression test (the ten fixes so far brought the suite -from 139 to 194). +coverage — every fix lands with a regression test (the eleven fixes so far brought the suite +from 139 to 198). The recurring pattern across the open findings is worth stating once: the code is rigorous about the failure modes that have actually been hit, and silent about the ones that have not. @@ -20,21 +20,6 @@ single-process assumptions, no user-facing history, etc.), see "Known gaps / TOD ## Medium -### B-35 — Every API timestamp is naive, so the frontend renders it in the wrong timezone - -Verified empirically: the `DateTime` columns carry no timezone, so SQLite returns naive -datetimes and `.isoformat()` produces `2026-07-27T06:56:47.489110` — **no `Z`**. JavaScript's -`new Date()` parses that as **local time**, so every date in `/admin` (rounds, pending -transactions, audit log, via `fmtDate` in `app/static/admin.js:50`) and `created_at` in -`/users/me` display two hours off in Italy. - -The codebase knows about this — `api/routes/rounds.py` calls `.replace(tzinfo=timezone.utc)` -on `opened_at` explicitly — but the fix was never applied systematically. - -**Proposed fix.** Make the columns `DateTime(timezone=True)` (Alembic migration) so the value -round-trips as aware, rather than patching each call site. Until then, at minimum a shared -serialization helper that stamps UTC, used by every `.isoformat()` in the API layer. - ### B-36 — `_wait_for_next_block` waits forever, with no timeout and no visibility `rounds/scheduler.py:158-161` loops until a higher block arrives. No timeout, no log, no audit @@ -195,12 +180,13 @@ already does. - **B-29** — a UTXO absent from one server's `listunspent` was marked spent immediately, irreversibly, on a single unauthenticated reply - **B-30** — a lost scripthash subscription meant a user's deposits were never credited, with no periodic safety net - **B-31** — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep -- **B-32** — an RBF bump's fee delta could fall below BIP125's relay-mandated minimum, so the node rejected it and the same tick retried identically forever; also had no ceiling on how high the fee rate could climb -- **B-33** — `POST /auth/login` had no rate limiting on a custodial wallet, so a patient distributed attack could brute-force a password against an enumerable username list; fixed with per-username *and* per-IP exponential backoff (`app/auth/rate_limit.py`), registration throttled per-IP too (also bounds B-31's attacker-controlled user count) -- **B-34** — neither self-service password change nor the admin reset invalidated already-issued JWTs, so a stolen token (or an attacker's own session) survived a password change meant to lock it out; fixed with a `User.token_version` column embedded in every JWT (`"tv"` claim) and checked on every request in `get_current_user`/`get_optional_user`, bumped on both endpoints — change-password hands back a fresh token so the caller's own session keeps working, the admin reset does not +- **B-32** — an RBF bump could retry forever below BIP125's relay-mandated minimum fee delta, with no ceiling on the fee rate either +- **B-33** — `POST /auth/login` had no rate limiting, so a password could be brute-forced against an enumerable username list +- **B-34** — password change/reset didn't invalidate already-issued JWTs, so a stolen token survived a change meant to lock it out +- **B-35** — API timestamps round-tripped as naive datetimes, so the frontend parsed them as local time instead of UTC See git history for the fix-by-fix breakdown (commits `f13f685`, `50a43ae`, `933760e`, and the -B-28/B-29/B-30/B-31/B-32/B-33/B-34 fixes). Suite grew from 139 to 194 tests over the ten. +B-28/B-29/B-30/B-31/B-32/B-33/B-34/B-35 fixes). Suite grew from 139 to 198 tests over the eleven. A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical, diff --git a/CLAUDE.md b/CLAUDE.md index 3e821ed..0f85ee1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,7 +12,7 @@ All 10 stages of the original build order are code-complete and unit-tested — 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. -**Read [BUGS.md](BUGS.md) before trusting any behaviour here.** Two audits: 2026-07-26 found 24 bugs (5 critical), all fixed; 2026-07-27 found 25 more (B-32 … B-49), of which **18 are still open** — no Critical, but High covers an RBF bump that can loop forever on a replacement the node always rejects (B-32), no brute-force protection on login (B-33), and password change/reset not invalidating existing JWTs (B-34). BUGS.md is the live open list with a proposed fix per finding; "Known gaps" at the end of this file is for limitations accepted **by design** instead. Don't fix a BUGS.md item silently as a side effect of other work — each fix lands with its own regression test. +**Read [BUGS.md](BUGS.md) before trusting any behaviour here.** Two audits: 2026-07-26 found 24 bugs (5 critical), all fixed; 2026-07-27 found 25 more (B-25 … B-49), of which **14 are still open** — no Critical or High remains, only Medium/Low: `_wait_for_next_block` has no timeout or visibility if a round gets stuck in `drawing` (B-36), no WAL/`busy_timeout` under five concurrent SQLite writer tasks (B-39), among others. BUGS.md is the live open list with a proposed fix per finding; "Known gaps" at the end of this file is for limitations accepted **by design** instead. Don't fix a BUGS.md item silently as a side effect of other work — each fix lands with its own regression test. Before writing code, read the "Architecture" section below in full plus the diagrams in [flowchart/](flowchart/): [platform-overview.mmd](flowchart/platform-overview.mmd) (the 5-phase flow) and [round-lifecycle.mmd](flowchart/round-lifecycle.mmd) (the round/draw lifecycle). Every node **and edge label** (conditions, retries, loops) is a behaviour that must be implemented as described. Regenerate the companion PDFs with `flowchart/render-pdf.sh .mmd` after editing either. diff --git a/app/api/routes/admin.py b/app/api/routes/admin.py index 246b624..9dde0b0 100644 --- a/app/api/routes/admin.py +++ b/app/api/routes/admin.py @@ -6,6 +6,7 @@ from pydantic import BaseModel, Field, field_validator from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession +from app.api.timeutil import isoformat_utc from app.audit.log import write_audit_log from app.auth.security import hash_password from app.config import settings @@ -163,7 +164,7 @@ async def list_users(session: AsyncSession = Depends(get_session)) -> list[Admin username=u.username, address=u.address, balance_sats=u.cached_balance_sats, - created_at=u.created_at.isoformat(), + created_at=isoformat_utc(u.created_at), ) for u in users ] @@ -253,8 +254,8 @@ async def list_rounds(session: AsyncSession = Depends(get_session), limit: int = AdminRoundResponse( id=r.id, status=r.status, - opened_at=r.opened_at.isoformat(), - closed_at=r.closed_at.isoformat() if r.closed_at else None, + opened_at=isoformat_utc(r.opened_at), + closed_at=isoformat_utc(r.closed_at), draw_block_height=r.draw_block_height, draw_block_hash=r.draw_block_hash, winner_user_id=r.winner_user_id, @@ -291,7 +292,7 @@ async def list_audit_log( payload=json.loads(e.payload_json), user_id=e.user_id, round_id=e.round_id, - created_at=e.created_at.isoformat(), + created_at=isoformat_utc(e.created_at), ) for e in entries ] @@ -333,7 +334,7 @@ async def list_pending_transactions( current_txid=p.current_txid, fee_rate_sat_vb=p.fee_rate_sat_vb, attempt_count=p.attempt_count, - broadcast_at=p.broadcast_at.isoformat(), + broadcast_at=isoformat_utc(p.broadcast_at), replaced_by_txid=p.replaced_by_txid, ) for p in entries diff --git a/app/api/routes/users.py b/app/api/routes/users.py index 8188569..a5d3758 100644 --- a/app/api/routes/users.py +++ b/app/api/routes/users.py @@ -4,6 +4,7 @@ from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession from app.api.errors import http_error +from app.api.timeutil import isoformat_utc from app.auth.dependencies import get_current_user from app.auth.security import MIN_PASSWORD_LENGTH, create_access_token, hash_password, verify_password from app.db.models import Round, RoundParticipant, User @@ -36,7 +37,7 @@ async def me( balance_sats=user.cached_balance_sats, pending_balance_sats=pending_balance_sats, has_pending=has_pending, - created_at=user.created_at.isoformat(), + created_at=isoformat_utc(user.created_at), ) diff --git a/app/api/timeutil.py b/app/api/timeutil.py new file mode 100644 index 0000000..8f17e81 --- /dev/null +++ b/app/api/timeutil.py @@ -0,0 +1,17 @@ +from datetime import datetime, timezone + + +def isoformat_utc(dt: datetime | None) -> str | None: + """Serialize a datetime for API responses, stamping it UTC first. + + Every DateTime column is written via app.db.models.utcnow() but SQLite/aiosqlite + round-trips it as a naive datetime, so a bare .isoformat() drops the "Z"/offset + and JavaScript's `new Date()` on the frontend parses the result as local time + instead of UTC (B-35). All stored values are UTC in practice, so a naive value + can be safely stamped rather than converted. + """ + if dt is None: + return None + if dt.tzinfo is None: + dt = dt.replace(tzinfo=timezone.utc) + return dt.isoformat() diff --git a/tests/unit/test_api_timeutil.py b/tests/unit/test_api_timeutil.py new file mode 100644 index 0000000..157ec90 --- /dev/null +++ b/tests/unit/test_api_timeutil.py @@ -0,0 +1,21 @@ +from datetime import datetime, timezone + +from app.api.timeutil import isoformat_utc + + +def test_naive_datetime_is_stamped_utc(): + # SQLite/aiosqlite round-trips DateTime columns as naive even though every + # value written is UTC (app.db.models.utcnow) — this is the exact shape + # returned by the ORM after a read (B-35). + naive = datetime(2026, 7, 27, 6, 56, 47, 489110) + result = isoformat_utc(naive) + assert result == "2026-07-27T06:56:47.489110+00:00" + + +def test_aware_datetime_is_left_unchanged(): + aware = datetime(2026, 7, 27, 6, 56, 47, tzinfo=timezone.utc) + assert isoformat_utc(aware) == aware.isoformat() + + +def test_none_passes_through(): + assert isoformat_utc(None) is None diff --git a/tests/unit/test_users.py b/tests/unit/test_users.py index 475474a..03cb8e1 100644 --- a/tests/unit/test_users.py +++ b/tests/unit/test_users.py @@ -162,3 +162,17 @@ async def test_register_rejects_weak_credentials(client, payload): async def test_register_accepts_valid_credentials(client): resp = await client.post("/auth/register", json={"username": "goodname", "password": "longenough1"}) assert resp.status_code == 201 + + +async def test_me_created_at_is_utc_stamped(client): + """B-35: SQLite/aiosqlite returns DateTime columns as naive, even though every + value written is UTC (app.db.models.utcnow). A bare .isoformat() on that naive + value has no "Z"/offset, and JavaScript's `new Date()` then parses it as local + time instead of UTC.""" + token = await _register(client) + headers = {"Authorization": f"Bearer {token}"} + + resp = await client.get("/users/me", headers=headers) + assert resp.status_code == 200 + created_at = resp.json()["created_at"] + assert created_at.endswith("+00:00") or created_at.endswith("Z")