Compare admin token as UTF-8 bytes to avoid TypeError on non-ASCII input (B-46)

secrets.compare_digest raises TypeError instead of returning False when a
str argument contains non-ASCII characters, turning a bad admin token into
an unhandled 500 instead of the expected 403. Encode both sides before
comparing.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-27 16:17:40 +02:00
co-authored by Claude Sonnet 5
parent 6045c89ed0
commit 31bc9a327f
4 changed files with 20 additions and 14 deletions
+6 -11
View File
@@ -1,11 +1,11 @@
# Known bugs # Known bugs
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high, A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high,
7 medium, 8 low), listed below as B-46 … B-49. B-25 through B-45 are fixed (see "Previously 7 medium, 8 low), listed below as B-47 … B-49. B-25 through B-46 are fixed (see "Previously
fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 4 are fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 3 are
Low/hygiene. The 139-test suite was green at the time of the audit, so none of these were caught Low/hygiene. The 139-test suite was green at the time of the audit, so none of these were caught
by existing coverage — every fix lands with a regression test (the twenty-one fixes so far brought by existing coverage — every fix lands with a regression test (the twenty-two fixes so far brought
the suite from 139 to 245). the suite from 139 to 246).
The recurring pattern across the open findings is worth stating once: the code is rigorous The recurring pattern across the open findings is worth stating once: the code is rigorous
about the failure modes that have actually been hit, and silent about the ones that have not. about the failure modes that have actually been hit, and silent about the ones that have not.
@@ -18,12 +18,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume
## Low / hygiene ## Low / hygiene
### B-46 — `secrets.compare_digest` on a `str` raises on non-ASCII input
`api/routes/admin.py:27` raises `TypeError` — a 500 instead of a 403 — when the header contains
non-ASCII characters.
**Fix:** compare the UTF-8 encoded bytes of both sides.
### B-47 — Unbounded `String` columns for large text ### B-47 — Unbounded `String` columns for large text
`raw_tx_hex` (`db/models.py:146`) and `payload_json` (`:178`) should be `Text`. It works on `raw_tx_hex` (`db/models.py:146`) and `payload_json` (`:178`) should be `Text`. It works on
@@ -61,6 +55,7 @@ already does.
- **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-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-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-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-32** — an RBF bump could retry forever below BIP125's relay-mandated minimum fee delta, with no ceiling on the fee rate either - **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-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-34** — password change/reset didn't invalidate already-issued JWTs, so a stolen token survived a change meant to lock it out
@@ -73,7 +68,7 @@ already does.
- **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 - **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 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 fixes). Suite grew from 139 to 245 tests over the twenty-one. 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 fixes). Suite grew from 139 to 246 tests over the twenty-two.
A full-codebase audit on 2026-07-26 (commit `d4e0974`) found 24 bugs across every Python 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, module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
+2 -2
View File
@@ -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. 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), of which **4 are still open** — no Critical, High or Medium remains, only Low/hygiene: unbounded `String` columns for large text (B-47), no cap on input count in `select_utxos` (B-48), among others. BUGS.md is the live open list with a proposed fix per finding; "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. **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), of which **3 are still open** — no Critical, High or Medium remains, only Low/hygiene: unbounded `String` columns for large text (B-47), no cap on input count in `select_utxos` (B-48), among others. BUGS.md is the live open list with a proposed fix per finding; "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.
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 <file>.mmd` after editing either. 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 <file>.mmd` after editing either.
@@ -233,7 +233,7 @@ Explicit design choices, not derivable from any single file — respect them:
## Known gaps / TODO ## Known gaps / TODO
Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (4 open) — not duplicated here. Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (3 open) — 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. - **`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. - **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.
+3 -1
View File
@@ -26,7 +26,9 @@ async def require_admin(x_admin_token: str = Header(default="")) -> None:
# anyone on an instance that never configured a token. # anyone on an instance that never configured a token.
if not settings.admin_token: if not settings.admin_token:
raise HTTPException(status.HTTP_403_FORBIDDEN, "invalid admin token") raise HTTPException(status.HTTP_403_FORBIDDEN, "invalid admin token")
if not secrets.compare_digest(x_admin_token, settings.admin_token): # compare_digest raises TypeError on a str containing non-ASCII characters
# (B-46) -- comparing the UTF-8 bytes instead accepts any input safely.
if not secrets.compare_digest(x_admin_token.encode(), settings.admin_token.encode()):
raise HTTPException(status.HTTP_403_FORBIDDEN, "invalid admin token") raise HTTPException(status.HTTP_403_FORBIDDEN, "invalid admin token")
+9
View File
@@ -67,6 +67,15 @@ async def test_admin_rejects_wrong_token(client):
assert resp.status_code == 403 assert resp.status_code == 403
async def test_admin_rejects_non_ascii_token_with_403_not_500(client):
"""B-46: secrets.compare_digest raises TypeError on a non-ASCII str, which
used to bubble up as a 500 instead of the expected 403. httpx encodes str
header values as ASCII client-side, so the raw UTF-8 bytes are passed
directly to reproduce what a real non-ASCII header on the wire looks like."""
resp = await client.get("/admin/config", headers={"X-Admin-Token": "café".encode("utf-8")})
assert resp.status_code == 403
async def test_admin_reads_and_updates_config(client): async def test_admin_reads_and_updates_config(client):
headers = {"X-Admin-Token": "test-admin-token"} headers = {"X-Admin-Token": "test-admin-token"}