davideandClaude Opus 5
23d58796b6
fee_address has no column default, because an operator has to supply their own — and the payout pays the 30% commission to it, so build_payout_transaction cannot even be built without one. A fresh instance nonetheless opened rounds happily: each took bets, confirmed them, and only then discovered it was unpayable, wedging in "paying_out" and retrying every 60s with money already in the pool. One manual recovery per round, until somebody noticed. open_new_round_if_needed now checks rounds_can_open(config) alongside `paused`: no payout address, no round. Nothing has moved yet at that point, which is the whole difference. Same scope as pausing — a round already in progress still closes, draws and pays out, since clearing the address mid-round is exactly the operator slip that must not strand a live round. Surfaced rather than silent, in the two places that matter: lottery_configured on GET /rounds/current, which makes / show a *different* banner from the maintenance one (telling a player "come back later" would be false — nothing is coming until setup finishes), and a warning at the top of /admin's Parametri card, the one screen that can fix it. rounds_can_open is where any future would-make-a-round-unpayable prerequisite belongs, instead of being discovered at payout time. The test churn is the finding restated: 26 tests expected a round to open on an instance with no payout address. Their fixtures now seed one, so each goes back to testing what it says — several would otherwise have passed for the wrong reason, returning None because of the missing address rather than because of the cooldown or pause under test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
122 lines
5.9 KiB
Markdown
122 lines
5.9 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-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.
|