From 0d2fef650256630d937186f12fd1daaeda845c01 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 23:37:02 +0200 Subject: [PATCH] Delete BUGS.md now that both audits' findings are closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit B-01 … B-49 are all fixed, so the file held no open work — only a history that git already keeps. CLAUDE.md now explains how to resolve the B-nn markers left throughout the code against that history, and repeats the caveat the empty list does not carry on its own: no open findings is not the same as no bugs. The one remaining reference, in an already-applied migration's docstring, is left as the historical record it is. --- BUGS.md | 58 ----------------------------------------------------- CLAUDE.md | 4 ++-- app/main.py | 2 +- 3 files changed, 3 insertions(+), 61 deletions(-) delete mode 100644 BUGS.md diff --git a/BUGS.md b/BUGS.md deleted file mode 100644 index 9ea0621..0000000 --- a/BUGS.md +++ /dev/null @@ -1,58 +0,0 @@ -# Known bugs - -A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high, -7 medium, 8 low). B-25 through B-49 are now **all fixed** (see "Previously fixed" below) — -nothing from either audit remains open. The 139-test suite was green at the time of the audit, -so none of these were caught by existing coverage — every fix landed with a regression test, -which brought the suite from 139 to 253. - -The recurring pattern behind those findings is worth keeping in mind for the next one: the code -is rigorous about the failure modes that have actually been hit, and silent about the ones that -have not. The payout phase is now fully recoverable; the "drawing" phase (waiting on a block) is -now observable (B-36) but still has no equivalent resume-after-restart — see "Known gaps / TODO" -in [CLAUDE.md](CLAUDE.md), which is also where the by-design limitations (single-shared-token -admin auth, single-process assumptions, no user-facing history, etc.) are documented. This file -stays as the record of what was found and fixed; a new finding gets the next B-nn and its own -regression test. - ---- - -## Previously fixed - -- **B-25** — the payout had no two-phase write, unlike bets and withdrawals -- **B-26** — a payout failure or a process restart could wedge a round in `paying_out` forever -- **B-27** — an RBF bump reset the reconciler's own abandon clock, so a repeatedly-bumped tx was never abandoned -- **B-28** — a hostile Electrum server (or a MITM) could single-handedly pick the round's winner -- **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-42** — Swagger/ReDoc/the raw OpenAPI JSON enumerated the entire API surface, admin endpoints included, to anyone who requested them; now off by default and gated behind `ENABLE_API_DOCS` -- **B-43** — the Caddyfile sent no CSP, no `X-Frame-Options`/`frame-ancestors`, and no HSTS, on a page whose JWT lives in `localStorage` -- **B-44** — README's Quick start documented a bare `uvicorn --reload` workflow, and `docs/running-the-server.md` still had a matching "Locale / venv" section, both contradicting CLAUDE.md's Docker-only policy -- **B-45** — `/admin/rounds`/`/admin/audit-log`'s `limit` had no bounds (`-1` means "everything" on SQLite), and `/admin/pending-transactions` had no limit or status filter at all -- **B-46** — `secrets.compare_digest` on a `str` raises `TypeError` on non-ASCII input, turning an invalid admin token with non-ASCII characters into a 500 instead of a 403 -- **B-49** — `_release_failed_bet`/`_release_failed_withdrawal` restored the balance without publishing an SSE update, so a rolled-back bet or withdrawal stayed on the dashboards until their next poll -- **B-48** — `select_utxos` had no cap on input count, so a fragmented address built an ever-larger transaction whose fee (deducted from the amount being moved) ate into the bet or withdrawal, up to the point of being non-standard -- **B-47** — `raw_tx_hex` and `payload_json` were unbounded `String` columns (`VARCHAR` with no length) — fine on SQLite/PostgreSQL, rejected by backends like MySQL that require a length -- **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 -- **B-36** — a stalled draw wait had no timeout, no log, and no audit trail, so a frozen round showed nothing in `/admin` -- **B-37** — a withdrawal covered by unconfirmed change answered "insufficient balance" instead of distinguishing it from actually having no funds -- **B-38** — the SSE subscriber cap was global, so one client opening enough connections degraded every other user to polling -- **B-39** — SQLite ran without WAL or a `busy_timeout`, so a writer could block every reader and a second writer failed immediately instead of waiting -- **B-40** — `bump_fee` held a DB session open across N slow network calls, and computed a prevout's value from a server-reported float instead of an exact integer -- **B-41** — confirmation/reconciliation depended on a verbose `blockchain.transaction.get` reply many Electrum servers reject, and abandonment relied on fragile substring-matching of an error message - -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/B-35/B-36/B-37/B-38/B-39/B-40/B-41/B-42/B-43/B-44/B-45/B-46/B-47 fixes). Suite grew from 139 to 248 tests over the twenty-three. - -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, -7 high, 7 medium, 5 low. All 24 were fixed and verified against the current code on -2026-07-27; the fixes are covered by the regression suite (grew from 79 to 139 tests) and -five of them were additionally confirmed against a real mainnet deployment (see git history -between `fb734bb` (documenting the findings) and `845ba98` (recording the audit outcome) for -the fix-by-fix breakdown — each commit message names the bugs it closes and where their -tests live). diff --git a/CLAUDE.md b/CLAUDE.md index a983235..a70bbb8 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-25 … B-49), all fixed as of 2026-07-27 — **nothing is open**. BUGS.md is the record of what each one was; "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. +Two full-codebase audits — 2026-07-26 (24 findings, 5 critical) and 2026-07-27 (25 more, B-25 … B-49) — are **all fixed** as of 2026-07-27, each with its own regression test. They were tracked in a `BUGS.md` that was deleted once the list emptied, so the ~276 `B-nn` markers left in comments across the code are pointers into git history (`git log --all --grep 'B-nn'` finds the commit that fixed one, and `git show f1a1145:BUGS.md`-style the file as it stood). A closed list is not the same as no bugs: the suite is unit-only (`tests/integration/` is empty), and withdrawal and the RBF bump have never been live-broadcast. "Known gaps" at the end of this file is for limitations accepted **by design** instead. A new finding gets the next B-nn, in its own commit 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. @@ -234,7 +234,7 @@ Explicit design choices, not derivable from any single file — respect them: ## Known gaps / TODO -Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (none open) — not duplicated here. +Accepted **by design** — distinct from the audit findings above (all fixed), which are not duplicated here. - **`drawing` doesn't resume after a restart.** `_tick()` handles `open`, `closing` and `paying_out` (the last via `_retry_payout_if_due`); nothing re-enters `_wait_for_next_block` after a crash. That wait is unbounded by design (the draw's entropy genuinely depends on a future block) but no longer silent — past `_DRAW_STALL_THRESHOLD_SECONDS` it logs progress and writes a `draw_stalled` audit entry, and `GET /rounds/current`'s `draw_waiting_since` surfaces it live (B-36). Restart-resumption itself remains the last prerequisite for running unattended. - **RBF handles one shape only**: a single change output, back to the tx's own sender, big enough to absorb the increase. No extra-input fallback — an exact-amount tx or too-small change raises `RbfError`. Not permanent, though: an unbumpable tx that never confirms is eventually abandoned and its UTXOs released. diff --git a/app/main.py b/app/main.py index ff547d0..82c87bd 100644 --- a/app/main.py +++ b/app/main.py @@ -41,7 +41,7 @@ def _make_electrum_client(endpoint: ElectrumEndpoint) -> ElectrumClient: @asynccontextmanager async def lifespan(app: FastAPI): - # Refuses to serve rather than starting up half-configured — see B-15 in BUGS.md. + # Refuses to serve rather than starting up half-configured (B-15). validate_runtime_secrets() endpoints = parse_endpoints(