From 9b6a4a240cd74d14e21afa8c73d4e3b5589d8f16 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Sun, 26 Jul 2026 22:14:52 +0200 Subject: [PATCH] Give every BUGS.md entry an explicit fix and regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass left 11 of the 24 entries with their remedy buried in prose and only 7 with a named test, while the header claimed a regression test for each. Every entry now follows one shape: symptom, root cause, *Proposed fix:*, *Test:* — with the handful that genuinely cannot be unit-tested (Docker image layout, browser behaviour) marked manual instead of left implied. A few fixes gained detail while being written out: B-14 needs the empty-token short-circuit kept ahead of compare_digest, B-15 breaks the current tests' short JWT secret, B-19 must not update tip_header_hex from a losing header, and B-20's column ends up holding the previous txid rather than the next one, so its name is backwards. Co-Authored-By: Claude Opus 5 --- BUGS.md | 158 ++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 130 insertions(+), 28 deletions(-) diff --git a/BUGS.md b/BUGS.md index cdf6364..36c7392 100644 --- a/BUGS.md +++ b/BUGS.md @@ -6,8 +6,11 @@ deployment. The test suite was green at the time of the audit (79 passed), so ** the findings below are caught by the existing tests** — every one of them needs a regression test alongside its fix. -Findings are ordered by severity. Each entry is self-contained: symptom, root cause with -file references, how to reproduce, and the proposed fix. Items already listed under +Findings are ordered by severity. Each entry is self-contained and follows the same shape: +symptom, root cause with `file:line` references, a `*Proposed fix:*` block, and a `*Test:*` +block naming the regression test to add (a few are marked manual where an automated test +would be testing the Docker image layout or the browser, not the code). Items already listed +under "Known gaps / TODO" in `CLAUDE.md` are cross-referenced rather than repeated, except where this audit found the gap to be worse than documented. @@ -300,6 +303,10 @@ the node's message in `params`. Add `error.broadcast_failed` to all 7 languages `app/static/i18n.js` (per the i18n contract in `CLAUDE.md`). Answer 502/503 rather than 400, since the failure is not the client's fault. +*Test:* `place_bet` / `request_withdrawal` with a client stub whose `broadcast` raises must +raise `BetError`/`WithdrawalError` with code `broadcast_failed` — and must leave no +`spent_txid`, no participant and no pending row behind. + --- ### B-08 @@ -323,6 +330,10 @@ before broadcast but only committed after. to `broadcast` in a second commit. A crash between the two leaves a row the reconciliation task from [B-04](#b-04) can resolve against the chain in either direction. +*Test:* with a client stub that broadcasts successfully but a session whose second commit +raises, the `PendingTransaction` must still exist afterwards (in its pre-broadcast state) +rather than the bet vanishing entirely. + --- ### B-09 @@ -373,6 +384,10 @@ endpoints. `_CONFIG_FIELDS` (keep it in the response model) so the pause switch has exactly one audited path. +*Test:* `PUT /admin/config` changing `fee_address` must produce exactly one +`config_updated` audit row carrying both the old and the new value; `paused` must be +rejected (or ignored) on that endpoint. + --- ### B-11 @@ -409,6 +424,11 @@ actually receives", which is the opposite of the behaviour. subtract an estimated payout fee or relabel the field as an estimate in the UI. Keep `participant_count` as-is for display but consider exposing `confirmed_count` separately. +*Test:* with two confirmed participants whose stored `bet_amount_sats` is below the +configured `bet_amount_sats` (fee already deducted), `GET /rounds/current` must report a +`jackpot_sats` derived from the stored amounts, and must not change when +`RoundConfig.bet_amount_sats` is edited mid-round. + --- ### B-12 @@ -433,6 +453,10 @@ registration) is therefore retried five times and finally reported as constraints in the HTML form. Inspect the `IntegrityError` and re-raise `username_taken` when it is the username constraint that failed. +*Test:* `POST /auth/register` with an empty username and with a 3-character password must +both be rejected (422/400) and create no user; a registration racing an existing username +must answer `username_taken`, not `derivation_index_conflict`. + --- ## Medium @@ -445,8 +469,14 @@ when it is the username constraint that failed. `verify_password` catches only `VerifyMismatchError`. Argon2 raises `InvalidHashError` for a hash it cannot parse and `VerificationError` for other verification failures, both of -which escape as an unhandled 500. Catch `argon2.exceptions.VerificationError` (the -superclass of `VerifyMismatchError`) plus `InvalidHashError` and return `False`. +which escape as an unhandled 500. + +*Proposed fix:* catch `argon2.exceptions.VerificationError` (the superclass of +`VerifyMismatchError`) plus `InvalidHashError` and return `False`, so an unusable stored +hash reads as "wrong password" rather than as a server fault. + +*Test:* `verify_password("x", "not-a-hash")` returns `False`; `POST /auth/login` against a +user row with a corrupted `password_hash` answers 401. ### B-14 @@ -455,8 +485,14 @@ superclass of `VerifyMismatchError`) plus `InvalidHashError` and return `False`. *File:* `app/api/routes/admin.py:20-22` `x_admin_token != settings.admin_token` is a short-circuiting comparison. This token gates -the private-key export endpoint, so it deserves `secrets.compare_digest`. Keep the -existing "empty token means always deny" behaviour. +the private-key export endpoint, so it deserves a constant-time comparison. + +*Proposed fix:* `secrets.compare_digest(x_admin_token, settings.admin_token)`, keeping the +existing "empty configured token means always deny" short-circuit *before* it (compare_digest +on two empty strings returns `True`). + +*Test:* the existing admin auth tests still pass, plus one asserting an empty +`ADMIN_TOKEN` setting denies an empty `X-Admin-Token` header. ### B-15 @@ -467,9 +503,16 @@ existing "empty token means always deny" behaviour. `jwt_secret` and `xprv_encryption_key` both default to `""`. With an empty `JWT_SECRET`, PyJWT raises `InvalidKeyError: HMAC key must not be empty` on every login and registration — a 500 with no hint about the real cause (verified locally). With an empty -`XPRV_ENCRYPTION_KEY`, Fernet fails on the first key derivation instead. Both should fail -fast: a Pydantic validator that rejects empty values (and, for `jwt_secret`, a minimum -length) so the container refuses to start rather than half-working. +`XPRV_ENCRYPTION_KEY`, Fernet fails on the first key derivation instead. Either way the +container starts up healthy and only fails once a user touches the broken path. + +*Proposed fix:* a Pydantic `field_validator` (or `model_validator`) on `Settings` rejecting +empty `jwt_secret`/`xprv_encryption_key`, with a minimum length on `jwt_secret` (32 bytes, +per the `InsecureKeyLengthWarning` PyJWT already emits in the test suite). The process must +refuse to boot instead of half-working. Note the tests currently rely on a short secret, so +they need updating alongside. + +*Test:* constructing `Settings(jwt_secret="")` raises `ValidationError`. ### B-16 @@ -480,8 +523,16 @@ length) so the container refuses to start rather than half-working. The image copies `pyproject.toml`, `app/`, `migrations/`, `alembic.ini` and `scripts/`. `docs/` is never copied (it is not in `.dockerignore` — it is simply not `COPY`-ed), so `FileResponse("docs/guida-utente.md")` raises inside the container and the help link in -the navbar returns a 500 in every real deployment. Add `COPY docs ./docs` to the -Dockerfile (and treat the guide as a shipped asset), or serve the guide from `app/static/`. +the navbar returns a 500 in every real deployment. + +*Proposed fix:* add `COPY docs ./docs` to the `Dockerfile` (treating the guide as a shipped +asset), or move the guide under `app/static/` and serve it from there. Whichever is chosen, +`GET /guida` should degrade to a 404 with a clear message rather than an unhandled +exception when the file is absent. + +*Test:* not unit-testable as-is (it depends on the image layout) — verify with +`docker compose up -d --build && curl -k https://localhost/guida`, and add that check to +`docs/running-the-server.md`'s smoke list. ### B-17 @@ -501,6 +552,10 @@ distinct error code — it is a user mistake, not a system limit), and/or identi change output by index rather than by address, recording it on `PendingTransaction` at build time. +*Test:* `request_withdrawal` to `user.address` is rejected; and given a hand-built tx with +two outputs to the same address, the bump reduces the *change* one (the last), leaving the +recipient amount untouched. + ### B-18 **DB session held open across Electrum network calls during payout** @@ -510,8 +565,16 @@ build time. `_trigger_payout` opens a session and, inside it, awaits `client.listunspent(...)` and `client.broadcast(...)` before committing. On SQLite that holds the write lock for the duration of two network round-trips (unbounded, per [B-01](#b-01)); on Postgres it becomes -a long-running transaction. Restructure as: read config/winner and close the session → -do the network work → reopen a session to persist the result. +a long-running transaction. + +*Proposed fix:* restructure into three phases — read config/winner and close the session → +do the network work (`listunspent`, build, `broadcast`) → reopen a session to persist the +result and the `PendingTransaction`. This also makes the [B-05](#b-05) error handling +easier to place, since the failure-prone part is no longer inside a transaction. + +*Test:* the existing payout tests must still pass; add one asserting no session is open +while the stub client's `broadcast` is being awaited (e.g. by having the stub attempt a +write through a second session). ### B-19 @@ -521,9 +584,16 @@ do the network work → reopen a session to persist the result. `self.tip_height = header["height"]` accepts a lower height on a reorg. Since `_wait_for_next_block` compares `self._listener.tip_height <= tip_at_close`, a regression -silently extends the wait by a block. Reorg handling is out of scope for v1 by explicit -design decision, but the assignment should at least be `max(...)`, or log a warning when -the height goes backwards. +silently extends the wait by a block. + +*Proposed fix:* full reorg handling is out of scope for v1 by explicit design decision, but +the assignment should be guarded: keep `max(self.tip_height, header["height"])` and log a +warning when a header arrives with a lower height, so the condition is at least visible in +the logs when it happens. Do not update `tip_header_hex` from a header that loses this +comparison, or height and hash would describe different blocks. + +*Test:* feeding the consumer a header at height N then N-1 leaves `tip_height == N` and the +hash unchanged. ## Low / hygiene @@ -536,8 +606,15 @@ the height goes backwards. `bump_fee` overwrites `current_txid` in place and never records what the old txid was. The column exists, is selected, and is rendered in the admin "Transazioni pendenti" table, -where it therefore always shows `—`. Set it in `bump_fee` (part of the [B-02](#b-02) fix) -so the replacement chain is auditable. +where it therefore always shows `—`. + +*Proposed fix:* set it in `bump_fee` (naturally part of the [B-02](#b-02) fix) so the +replacement chain is auditable. Note the column semantics are the reverse of what the name +suggests for an in-place update — it will hold the *previous* txid, so either rename it +(`previous_txid`, via a migration) or document the direction on the model. + +*Test:* after a bump, the row's `replaced_by_txid` holds the pre-bump txid and +`current_txid` the new one. ### B-21 @@ -548,9 +625,14 @@ so the replacement chain is auditable. `pending_ids` (line 32) is computed and never used. Line 35 rebuilds the same tuples from ORM objects whose session has already been closed; it works today only because `expire_on_commit=False` and no commit intervened, so flipping that engine setting would -break it silently. Select the plain columns in the query -(`select(PendingTransaction.id, ..., PendingTransaction.kind)`) instead of hydrating -entities. +break it silently. + +*Proposed fix:* drop `pending_ids` and select the plain columns +(`select(PendingTransaction.id, PendingTransaction.current_txid, PendingTransaction.kind)`) +instead of hydrating entities, so nothing outlives the session. + +*Test:* covered by the existing `tests/unit/test_confirmation.py` — it must stay green with +`expire_on_commit=True` forced on the test session factory. ### B-22 @@ -560,9 +642,15 @@ entities. `app/static/admin.js` (table cells) Every amount is displayed as `sats / SATS_PER_PLM` with no formatting, so values like -`0.7000000000000001` are reachable in the balance, the jackpot and the admin tables. Add a -single `formatPlm(sats)` helper (fixed decimals, locale-aware grouping) and route every -display through it. +`0.7000000000000001` are reachable in the balance, the jackpot and the admin tables. + +*Proposed fix:* one `formatPlm(sats)` helper (fixed decimals, locale-aware grouping via +`Intl.NumberFormat` with the language already resolved by `i18n.js`), used by every display +site. Amounts sent *to* the server must keep going through `Math.round(x * SATS_PER_PLM)` — +the formatter is for display only. + +*Test:* manual — with a 0.7 PLM jackpot and a 12345678.9 PLM balance, no artefacts and no +locale mismatch against the selected language. ### B-23 @@ -574,8 +662,16 @@ display through it. invoked from the SSE handler, the poll chain, `placeBet`, `withdraw` and `showDashboard`, all sharing `#refresh-btn`. Two overlapping calls make the second snapshot the *loading* label, which it then restores permanently — leaving the button stuck on -"Aggiornamento…". Guard with a per-button in-flight flag (bail out, or await the existing -promise). +"Aggiornamento…". + +*Proposed fix:* keep the in-flight promise on the element itself (e.g. a +`button._loadingPromise` / `WeakMap`); a second call either awaits the existing one or +returns immediately, so only the outermost call restores the markup. Also worth +de-duplicating the SSE burst: `onRoundServerEvent` fires three fetches per event, and the +broadcaster is generic, so every client reacts to every event. + +*Test:* manual — trigger a bet while an SSE-driven `refreshMe()` is in flight and confirm +the refresh button returns to its icon+label state. ### B-24 @@ -586,8 +682,14 @@ promise). It answers `{"detail": "internal server error"}` — a bare string, while `app/api/errors.py` documents `detail` as `{"code", "message", "params"}`. The frontend tolerates a string (`apiErrorMessage` handles that case), but the contract should be -uniform: return `ApiError("internal_error", "internal server error").as_detail()` and add -`error.internal_error` to the i18n table. +uniform. + +*Proposed fix:* return `ApiError("internal_error", "internal server error").as_detail()` and +add `error.internal_error` to all 7 languages in `app/static/i18n.js`. Keep the response +body free of exception details — the traceback belongs in `logs/app.log` only. + +*Test:* an endpoint stubbed to raise answers 500 with `detail.code == "internal_error"` and +no exception text in the body. ---