davideandClaude Opus 5
907e32e9e0
client_ip() read the first element of X-Forwarded-For, which is correct only if
the proxy replaces the header. Caddy appends the peer address to whatever the
client sent, so element 0 was whatever the caller claimed: rotating a fake value
per request minted a fresh identity every time and walked straight through the
login and registration throttles (B-33) and the SSE per-IP subscriber cap
(B-38). Only the per-username login bucket, which doesn't key on the IP, still
bit.
Both halves of the audit's fix, since they hold independently:
- the Caddyfile overwrites the header with `header_up X-Forwarded-For
{remote_host}`, so what reaches the app is the actual peer and nothing else.
This is the one that makes the app's assumption true at the source.
- client_ip() reads the *last* hop rather than the first — the element written
by the hop closest to us, i.e. by our own proxy. Exactly one trusted proxy
sits in front of the app (`app` is only `expose`d on the compose network,
never published to the host), so that element is the real peer.
An empty or comma-only header now falls back to request.client.host instead of
returning "", which was its own shared-bucket evasion.
Regression tests both sides: two requests spoofing different prefixes must key
to the same IP, and the Caddyfile must keep the header_up directive (checked by
`caddy validate`).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
14 KiB
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. 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' finds it later.
Already fixed and removed: B-52 (a round past ~50 participants deadlocked the
payout — 025754c), B-53 (a bet could pay into the pool of a round it was left
out of — 64f6229), B-54 (X-Forwarded-For was read from the front, so every
IP-keyed control was bypassable — PLACEHOLDER).
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.
High — security
B-55 — Argon2 hashing runs on the event loop
app/auth/security.py:20-39, called from app/auth/routes.py and
app/api/routes/users.py.
hash_password/verify_password are synchronous and cost tens of milliseconds
each, so every login, registration and password change blocks the whole process —
including all six background tasks (scheduler, confirmation poller, RBF bumper,
listener, both reconcilers). A burst of unauthenticated login attempts (which,
per B-54, is not effectively throttled) is a cheap denial of service that also
delays draws and confirmations.
Fix: run both through starlette.concurrency.run_in_threadpool.
B-56 — RateLimiter._buckets is never pruned
app/auth/rate_limit.py:37.
The dict grows without bound, keyed by attacker-chosen strings (arbitrary
usernames, and — via B-54 — arbitrary IPs). decay_seconds ages a bucket's
counter but never removes the entry.
Fix: evict entries whose last failure is older than decay_seconds (opportunistically
on record_failure, or on a periodic sweep), and cap the dict size.
B-57 — username matching is case-sensitive while the login throttle key is not
app/auth/routes.py:126 (body.username.lower()) vs :132
(User.username == body.username).
Two consequences: Bob and bob are separate accounts sharing a single rate-limit
bucket (one locks the other out), and registration happily accepts near-duplicate
usernames, which is an impersonation vector on a custodial system.
Fix: make username uniqueness case-insensitive (store a normalized form, or a functional unique index) and key the throttle on the same normalized value.
B-58 — the registration throttle counts successes as failures and is IP-only
app/auth/routes.py:66-71.
record_failure is called on every registration attempt, successful ones
included. Five legitimate signups from one shared/NAT address lock the sixth real
user out with exponential backoff up to 600 s — while an attacker skips the limiter
entirely through B-54. The intent (bounding accounts per source) is reasonable;
the current shape punishes only honest users. The inline comment also cites B-31
(the resubscribe finding) where it means B-33.
Fix: keep an accounts-per-IP quota if that is the goal, but express it as a quota rather than as failure backoff, and fix the B-nn reference.
B-59 — deposit crediting is not corroborated, unlike external-spend detection
app/deposits/service.py:31-45, app/electrum/listener.py:401-433.
A candidate external spend is corroborated across the other configured servers
before it can reduce a balance (B-29), but value and height for a credit
are taken from the single active connection with no cross-check. A hostile or
broken server can inflate a user's displayed balance with outpoints that do not
exist. The blast radius is bounded — a bet or withdrawal built on a phantom UTXO
is refused at broadcast and the rollback releases it — but it wedges the user's
balance display and burns build attempts.
Fix: either corroborate credits the same way (symmetry with B-29), or record the asymmetry explicitly as an accepted risk in CLAUDE.md with its bound stated.
B-60 — POST /admin/bug-reports/{id}/status writes no audit entry
app/api/routes/admin.py:404-419.
Every other admin mutation (config edit, pause/resume, privkey export, password
reset) is audit-logged. This one is not, so a report can be silently marked
resolved with no trace — and with one shared ADMIN_TOKEN and no per-admin
identity, the audit log is the only accountability there is.
Medium — correctness and robustness
B-61 — config edits apply retroactively to the round already in progress
app/rounds/scheduler.py:71, app/rounds/service.py:51-61,
app/api/routes/admin.py:106-128.
round_duration_seconds is read live on every tick and on every bet check, and the
deadline is computed as opened_at + duration. Lowering it from 600 to 60 while a
round is 300 s in closes that round instantly; raising it moves the closes_at
clients are already counting down to. round_cooldown_seconds has the same
property for the gap after a close.
B-11 fixed exactly this class of problem for bet_amount_sats (an in-progress
round's advertised jackpot must not move when an operator edits the bet amount);
the timing fields were left live.
Fix: snapshot the duration (and cooldown) onto the Round row when it opens and
read them from there, leaving the config row as the value for the next round.
B-62 — "withdraw the full amount" reliably produces an unbumpable transaction
app/static/app.js:798-807, app/wallet/psbt_builder.py:138-141,
app/tx/broadcast.py:150-152.
The max-amount checkbox sends amount_sats == myBalanceSats == total_in, so
change == 0, the change output is dropped, and the tx has a single output. bump_fee
then finds no change output to absorb the increase and raises RbfError every 30 s
until the reconciler abandons the row six hours later. The RBF single-change-output
limitation is a documented gap, but the UI makes it the default withdrawal path
rather than a corner case (the same applies to an exact-amount bet).
Fix directions: leave a change output above DUST_LIMIT_SATS when the requested
amount would consume the whole input total (i.e. reserve a little), or warn in the
UI that a full-balance withdrawal cannot be fee-bumped, or implement the
extra-input RBF fallback.
B-63 — tip_height == 0 window right after connecting can seed a draw from a pre-close block
app/electrum/listener.py:160-167, app/rounds/scheduler.py:153.
_run_once assigns self.client before subscribe_headers() returns, so there
is a window in which the client looks alive while tip_height is still 0 and
tip_header_hex is None. A _close_and_draw entering that window records
tip_at_close = 0, and the first header applied — the current tip, a block mined
before the round closed — satisfies tip_height > tip_at_close and becomes the
draw's entropy. The draw must use a block that did not exist at close time; a block
whose hash was already public before betting closed is not the guarantee the
flowchart describes.
Fix: publish self.client only after the first header has been applied, or refuse
to draw while tip_header_hex is None / tip_height == 0.
B-64 — _apply_header accepts a same-height header without the chaining check
app/electrum/listener.py:271-296.
The chain check only runs for height == self.tip_height + 1. A header at exactly
the current tip height replaces tip_header_hex after passing only the
self-target check — which, as the docstring of header_meets_its_own_target
already notes, a server can satisfy with a self-declared easy target. Separately,
a header with no hex field sets tip_header_hex = None, discarding a tip we
otherwise accepted.
Fix: treat a same-height header as either ignorable or as a reorg signal rather
than silently replacing the entropy source, and don't clear tip_header_hex on a
hex-less notification.
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_versionimplements 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-bugis a placeholder; it is fully implemented and translated, with admin triage. Only/guidais 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.pyandapp/api/routes/bug_reports.py. - README links
flowchart.mmd, which does not exist (the diagrams live inflowchart/), describesdocs/running-the-server.mdas covering "local venv vs. Docker" (that workflow was removed in B-44), and links the anchorCLAUDE.md#tech-stack-mvp, which no longer exists.
B-69 — stale in-code comments
app/tx/reconcile.py:206calls the payout retry "a future payout-retry routine — still an open gap"; it exists (B-26).app/db/base.py:9says "five concurrent background tasks"; there are six.app/auth/routes.py:31cites 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.