davideandClaude Opus 5
c0314e2bf0
_apply_header's linkage check only fires on a single-block advance, so a header at the height we already held one for was applied on nothing but its own self-consistency — and that check, as header_meets_its_own_target's own docstring says, a server can satisfy with a self-declared easy target. So the one value the draw is seeded from could be replaced under us at the current height, by a reorg at the tip or by a single server disagreeing with the rest, with no check able to speak to it. Separately, a header carrying no hex set tip_header_hex back to None while advancing tip_height, leaving the two describing different blocks — the exact pairing that function exists to keep. Both are now refused without ending the session, unlike the fabrication cases above them: neither is evidence of a hostile server, and rotating away would cost us the one connection that also credits deposits and broadcasts transactions. - A same-height header is ignored (logged when it actually differs). The hash committed to for a height is not swapped under us; if ours turns out to be the orphan, corroborate_header already refuses to seed a draw from it and the draw waits for a further block. - A hex-less header is ignored outright: nothing to validate, nothing to draw from. A server that only ever pushed heights now freezes the draw — visibly, via B-36's draw_stalled — instead of costing us the connection. Because ignoring is not fatal, _run_once additionally refuses to publish the client when the initial header leaves the tip still unknown, so this cannot reopen B-63's window from the other side. The two _run_once tests are bounded with asyncio.wait_for: without their guard that call waits on session tasks nothing ends, and a regression must fail rather than hang the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
147 lines
7.1 KiB
Markdown
147 lines
7.1 KiB
Markdown
# BUGS.md — audit of 2026-08-03
|
|
|
|
Third full-codebase audit, opened after the 2026-07-26 (B-01 … B-24) and
|
|
2026-07-27 (B-25 … B-49) lists were emptied. Numbering continues from the last
|
|
fixed finding, B-51.
|
|
|
|
The list opened at B-52 … B-72 and holds only what is still **open**: a finding is
|
|
removed from this file once it is fixed, and is not listed here afterwards. Per
|
|
CLAUDE.md's convention each entry gets its own commit with its own regression test,
|
|
and the `B-nn` marker goes in a comment next to the fix, so
|
|
`git log --all --grep 'B-nn'` is the record of how any closed finding was closed.
|
|
|
|
State of the tree at audit time: 264 unit tests, all passing; `tests/integration/`
|
|
still empty; withdrawal and the RBF bump path still never live-broadcast.
|
|
|
|
Verified as *not* broken while looking for these: i18n key parity (146 identical
|
|
keys across all 7 languages), HTML escaping of every user-controlled value in
|
|
`admin.js`, the Alembic chain (linear, single head, matching `models.py`),
|
|
strictly-integer satoshi arithmetic everywhere, and `.gitignore` coverage of
|
|
secrets/DB/logs (nothing sensitive is tracked in git).
|
|
|
|
Severity is about consequence, not likelihood:
|
|
**critical** = money stuck or lost, or the lottery stops;
|
|
**high** = a security control that does not hold;
|
|
**medium** = wrong behaviour with a bounded blast radius;
|
|
**low** = drift between documentation and code.
|
|
|
|
---
|
|
|
|
## Critical
|
|
|
|
### (not new) `drawing` does not resume after a restart
|
|
|
|
Already tracked as an accepted gap in CLAUDE.md's "Known gaps", not re-numbered
|
|
here. Worth restating in context: with `restart: unless-stopped` on the app
|
|
container, this is the one state that gets stuck with money in play, and it
|
|
remains the last prerequisite for running unattended.
|
|
|
|
---
|
|
|
|
## Medium — correctness and robustness
|
|
|
|
### B-65 — `/rounds/current` counts unconfirmed participants; the draw and payout do not
|
|
|
|
`app/api/routes/rounds.py:131-143` vs `app/rounds/scheduler.py:126`.
|
|
|
|
`participant_count` and `jackpot_sats` are computed over *all* `round_participants`
|
|
rows, while the draw and the payout only use `status == "confirmed"`. So the
|
|
advertised jackpot can exceed what is actually paid out, and a participant whose
|
|
bet is later abandoned appears in the count and then vanishes from it.
|
|
|
|
Fix: either count only `confirmed` (and accept that a fresh bet takes a block to
|
|
show up), or expose the two figures separately (confirmed vs in-flight) so the
|
|
number on screen and the number that gets paid agree by construction.
|
|
|
|
### B-66 — nothing stops rounds from opening with no `fee_address` configured
|
|
|
|
`app/rounds/config.py:12-17`, `app/rounds/scheduler.py:330-335`.
|
|
|
|
A fresh instance starts with `fee_address = ""`. Rounds open, bets are accepted and
|
|
confirm, and only then does the payout refuse to build — leaving the round in
|
|
`paying_out`, retrying every 60 s, with the audit log as the only signal.
|
|
|
|
Fix: refuse to open a round while `fee_address` is unset (and surface it on
|
|
`/admin` and as a maintenance-style banner), so the failure happens before anyone's
|
|
money is committed.
|
|
|
|
### B-67 — `/qr/{address}` is unauthenticated, synchronous and only shape-validated
|
|
|
|
`app/api/routes/qr.py:11-21`.
|
|
|
|
`qrcode.make` runs on the event loop, so the endpoint is a cheap CPU amplifier for
|
|
an unauthenticated caller, and the regex accepts any `plm1[a-z0-9]{10,90}` string
|
|
without validating the bech32 checksum — so it happily renders a QR for a
|
|
non-address.
|
|
|
|
Fix: validate with `is_valid_plm_address` (already used by withdrawals and by the
|
|
admin `fee_address` validator), and either offload the render or cache it per
|
|
address.
|
|
|
|
---
|
|
|
|
## Low — documentation and consistency drift
|
|
|
|
### B-68 — CLAUDE.md and README describe a state the code has moved past
|
|
|
|
- CLAUDE.md's tech-stack line still says JWT has **no revocation (B-34)**; `token_version`
|
|
implements exactly that revocation (`app/db/models.py:29`, `app/auth/dependencies.py:26`).
|
|
- "Known gaps" still says **no rate limiting anywhere (B-33)**; login and registration
|
|
are throttled (`app/auth/rate_limit.py`). What is genuinely still unthrottled is
|
|
bets, withdrawals and the admin endpoints — that is the claim worth keeping.
|
|
- "Known gaps" still says **`/report-bug` is a placeholder**; it is fully implemented
|
|
and translated, with admin triage. Only `/guida` is still a stub.
|
|
- Test counts are stale in three places: 264 actual, CLAUDE.md says 253 twice,
|
|
README says 232.
|
|
- The code map omits `app/auth/rate_limit.py`, `app/api/client_ip.py` and
|
|
`app/api/routes/bug_reports.py`.
|
|
- README links `flowchart.mmd`, which does not exist (the diagrams live in
|
|
`flowchart/`), describes `docs/running-the-server.md` as covering "local venv vs.
|
|
Docker" (that workflow was removed in B-44), and links the anchor
|
|
`CLAUDE.md#tech-stack-mvp`, which no longer exists.
|
|
|
|
### B-69 — stale in-code comments
|
|
|
|
- `app/tx/reconcile.py:206` calls the payout retry "a future payout-retry routine —
|
|
still an open gap"; it exists (B-26).
|
|
- `app/db/base.py:9` says "five concurrent background tasks"; there are six.
|
|
- `app/auth/routes.py:31` cites B-31 where it means B-33 (see B-58).
|
|
|
|
### B-70 — the flowchart's WITHDRAW precondition is not implemented as written
|
|
|
|
`flowchart/platform-overview.mmd:39` (node E1) states a withdrawal cannot happen
|
|
together with a bet in progress. The code only serializes the *builds* through the
|
|
per-user lock (`app/tx/locks.py`): a withdrawal is accepted while a bet is still
|
|
unconfirmed, as long as confirmed UTXOs cover it.
|
|
|
|
CLAUDE.md declares every node and edge label of the diagrams a behaviour that must
|
|
be implemented as described, so one of the two has to move — most likely the
|
|
diagram, since the lock already prevents the actual double-spend hazard, but that
|
|
is a decision, not a cleanup.
|
|
|
|
### B-71 — `.env` points `MASTER_KEY_PATH` at a second copy of the master key
|
|
|
|
CLAUDE.md's deployment section prescribes pointing `MASTER_KEY_PATH` at the
|
|
host-side `./data/keys/master.xprv.enc` so the venv scripts and the container read
|
|
one file. `.env` instead sets `./master.xprv.enc`, and both files now exist in the
|
|
working tree (both gitignored). They were verified during this audit to decrypt to
|
|
the *same* xprv, so nothing has diverged yet — but `scripts/decrypt_master_key.py`
|
|
reads a different file from the one the container uses, and a future
|
|
`generate_master_key.py --overwrite` would split them silently, with an ops
|
|
recovery path that then reports the wrong key.
|
|
|
|
The same duplication exists for the database (`./plm_lottery.db` next to
|
|
`data/db/plm_lottery.db`), which is less dangerous but equally confusing.
|
|
|
|
Fix: set `MASTER_KEY_PATH=./data/keys/master.xprv.enc` in `.env`, delete the stray
|
|
root copy once confirmed redundant, and state the same for `DATABASE_URL`.
|
|
|
|
### B-72 — `docs/setup.md` still frames setup as "locally or via Docker"
|
|
|
|
`docs/setup.md:1-10` lists Python as a prerequisite "for the local/venv workflow"
|
|
and describes the master-key step as local *or* Docker, while B-44 made Docker the
|
|
only supported way to run the server (`docs/running-the-server.md` and README were
|
|
updated, this file was not). The venv genuinely is needed for tests, migrations and
|
|
the key scripts — the wording just needs to say that instead of implying a second
|
|
way to run the server.
|