Bound admin list endpoint limits, add status filter to pending-transactions (B-45)
/admin/rounds and /admin/audit-log accepted any limit, including -1 (which SQLite treats as "no limit"), and /admin/pending-transactions had no limit at all -- it grows without end. Add Query(default=..., ge=1, le=500) to all three, plus an optional status filter on pending-transactions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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-45 … B-49. B-25 through B-44 are fixed (see "Previously
|
7 medium, 8 low), listed below as B-46 … B-49. B-25 through B-45 are fixed (see "Previously
|
||||||
fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 5 are
|
fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 4 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 fixes so far brought
|
by existing coverage — every fix lands with a regression test (the twenty-one fixes so far brought
|
||||||
the suite from 139 to 232).
|
the suite from 139 to 245).
|
||||||
|
|
||||||
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,14 +18,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume
|
|||||||
|
|
||||||
## Low / hygiene
|
## Low / hygiene
|
||||||
|
|
||||||
### B-45 — Unvalidated and unpaginated admin list endpoints
|
|
||||||
|
|
||||||
`limit: int = 50` on `/admin/rounds` and `/admin/audit-log` has no bounds (`-1` means
|
|
||||||
"everything" on SQLite), and `/admin/pending-transactions` has no limit at all — it grows
|
|
||||||
without end.
|
|
||||||
**Fix:** `Query(default=50, ge=1, le=500)` on both, and the same treatment plus a status filter
|
|
||||||
on the pending-transaction list.
|
|
||||||
|
|
||||||
### B-46 — `secrets.compare_digest` on a `str` raises on non-ASCII input
|
### 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
|
`api/routes/admin.py:27` raises `TypeError` — a 500 instead of a 403 — when the header contains
|
||||||
@@ -68,6 +60,7 @@ already does.
|
|||||||
- **B-42** — Swagger/ReDoc/the raw OpenAPI JSON enumerated the entire API surface, admin endpoints included, to anyone who requested them; now off by default and gated behind `ENABLE_API_DOCS`
|
- **B-42** — Swagger/ReDoc/the raw OpenAPI JSON enumerated the entire API surface, admin endpoints included, to anyone who requested them; now off by default and gated behind `ENABLE_API_DOCS`
|
||||||
- **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-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
|
||||||
@@ -80,7 +73,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 fixes). Suite grew from 139 to 232 tests over the twenty.
|
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.
|
||||||
|
|
||||||
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,
|
||||||
|
|||||||
@@ -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 **5 are still open** — no Critical, High or Medium remains, only Low/hygiene: an admin list endpoint with no pagination bound (B-45), unbounded `String` columns for large text (B-47), 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 **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.
|
||||||
|
|
||||||
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) (5 open) — not duplicated here.
|
Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (4 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.
|
||||||
|
|||||||
+11
-6
@@ -1,7 +1,7 @@
|
|||||||
import json
|
import json
|
||||||
import secrets
|
import secrets
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, Header, HTTPException, status
|
from fastapi import APIRouter, Depends, Header, HTTPException, Query, status
|
||||||
from pydantic import BaseModel, Field, field_validator
|
from pydantic import BaseModel, Field, field_validator
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
from sqlalchemy.ext.asyncio import AsyncSession
|
from sqlalchemy.ext.asyncio import AsyncSession
|
||||||
@@ -242,7 +242,9 @@ class AdminRoundResponse(BaseModel):
|
|||||||
|
|
||||||
|
|
||||||
@router.get("/rounds", response_model=list[AdminRoundResponse], dependencies=[Depends(require_admin)])
|
@router.get("/rounds", response_model=list[AdminRoundResponse], dependencies=[Depends(require_admin)])
|
||||||
async def list_rounds(session: AsyncSession = Depends(get_session), limit: int = 50) -> list[AdminRoundResponse]:
|
async def list_rounds(
|
||||||
|
session: AsyncSession = Depends(get_session), limit: int = Query(default=50, ge=1, le=500)
|
||||||
|
) -> list[AdminRoundResponse]:
|
||||||
rounds = (await session.scalars(select(Round).order_by(Round.id.desc()).limit(limit))).all()
|
rounds = (await session.scalars(select(Round).order_by(Round.id.desc()).limit(limit))).all()
|
||||||
winner_ids = {r.winner_user_id for r in rounds if r.winner_user_id is not None}
|
winner_ids = {r.winner_user_id for r in rounds if r.winner_user_id is not None}
|
||||||
winners = {}
|
winners = {}
|
||||||
@@ -282,7 +284,7 @@ class AdminAuditLogResponse(BaseModel):
|
|||||||
"/audit-log", response_model=list[AdminAuditLogResponse], dependencies=[Depends(require_admin)]
|
"/audit-log", response_model=list[AdminAuditLogResponse], dependencies=[Depends(require_admin)]
|
||||||
)
|
)
|
||||||
async def list_audit_log(
|
async def list_audit_log(
|
||||||
session: AsyncSession = Depends(get_session), limit: int = 200
|
session: AsyncSession = Depends(get_session), limit: int = Query(default=200, ge=1, le=500)
|
||||||
) -> list[AdminAuditLogResponse]:
|
) -> list[AdminAuditLogResponse]:
|
||||||
entries = (await session.scalars(select(AuditLog).order_by(AuditLog.id.desc()).limit(limit))).all()
|
entries = (await session.scalars(select(AuditLog).order_by(AuditLog.id.desc()).limit(limit))).all()
|
||||||
return [
|
return [
|
||||||
@@ -319,10 +321,13 @@ class AdminPendingTransactionResponse(BaseModel):
|
|||||||
)
|
)
|
||||||
async def list_pending_transactions(
|
async def list_pending_transactions(
|
||||||
session: AsyncSession = Depends(get_session),
|
session: AsyncSession = Depends(get_session),
|
||||||
|
limit: int = Query(default=50, ge=1, le=500),
|
||||||
|
status_filter: str | None = Query(default=None, alias="status"),
|
||||||
) -> list[AdminPendingTransactionResponse]:
|
) -> list[AdminPendingTransactionResponse]:
|
||||||
entries = (
|
query = select(PendingTransaction).order_by(PendingTransaction.id.desc())
|
||||||
await session.scalars(select(PendingTransaction).order_by(PendingTransaction.id.desc()))
|
if status_filter is not None:
|
||||||
).all()
|
query = query.where(PendingTransaction.status == status_filter)
|
||||||
|
entries = (await session.scalars(query.limit(limit))).all()
|
||||||
return [
|
return [
|
||||||
AdminPendingTransactionResponse(
|
AdminPendingTransactionResponse(
|
||||||
id=p.id,
|
id=p.id,
|
||||||
|
|||||||
@@ -277,3 +277,90 @@ async def test_pause_cannot_be_toggled_through_the_config_endpoint(client):
|
|||||||
resp = await client.put("/admin/config", headers=headers, json={"paused": True})
|
resp = await client.put("/admin/config", headers=headers, json={"paused": True})
|
||||||
assert resp.status_code in (200, 422) # ignored or refused, but never applied
|
assert resp.status_code in (200, 422) # ignored or refused, but never applied
|
||||||
assert (await client.get("/admin/config", headers=headers)).json()["paused"] is False
|
assert (await client.get("/admin/config", headers=headers)).json()["paused"] is False
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("endpoint", ["/admin/rounds", "/admin/audit-log", "/admin/pending-transactions"])
|
||||||
|
@pytest.mark.parametrize("bad_limit", [0, -1, 501])
|
||||||
|
async def test_admin_list_endpoints_reject_out_of_range_limit(client, endpoint, bad_limit):
|
||||||
|
"""B-45: `limit` had no bounds — `-1` means "everything" on SQLite, so an
|
||||||
|
unvalidated limit could dump the entire table in one response."""
|
||||||
|
headers = {"X-Admin-Token": "test-admin-token"}
|
||||||
|
resp = await client.get(endpoint, headers=headers, params={"limit": bad_limit})
|
||||||
|
assert resp.status_code == 422
|
||||||
|
|
||||||
|
|
||||||
|
async def test_admin_list_rounds_respects_limit(client):
|
||||||
|
from app.db import base as db_base
|
||||||
|
from app.db.models import Round
|
||||||
|
|
||||||
|
async with db_base.AsyncSessionLocal() as session:
|
||||||
|
session.add_all([Round(status="closed") for _ in range(3)])
|
||||||
|
await session.commit()
|
||||||
|
|
||||||
|
headers = {"X-Admin-Token": "test-admin-token"}
|
||||||
|
resp = await client.get("/admin/rounds", headers=headers, params={"limit": 2})
|
||||||
|
assert resp.status_code == 200
|
||||||
|
assert len(resp.json()) == 2
|
||||||
|
|
||||||
|
|
||||||
|
async def test_admin_audit_log_respects_limit(client):
|
||||||
|
from app.db import base as db_base
|
||||||
|
from app.audit.log import write_audit_log
|
||||||
|
|
||||||
|
async with db_base.AsyncSessionLocal() as session:
|
||||||
|
for _ in range(3):
|
||||||
|
await write_audit_log(session, "test_event", {})
|
||||||
|
await session.commit()
|
||||||
|
|
||||||
|
headers = {"X-Admin-Token": "test-admin-token"}
|
||||||
|
resp = await client.get("/admin/audit-log", headers=headers, params={"limit": 2})
|
||||||
|
assert resp.status_code == 200
|
||||||
|
assert len(resp.json()) == 2
|
||||||
|
|
||||||
|
|
||||||
|
async def _make_pending_transaction(session, *, kind="bet", status="pending"):
|
||||||
|
from app.db.models import PendingTransaction
|
||||||
|
import secrets as _secrets
|
||||||
|
|
||||||
|
tx = PendingTransaction(
|
||||||
|
kind=kind,
|
||||||
|
current_txid=_secrets.token_hex(32),
|
||||||
|
fee_rate_sat_vb=1,
|
||||||
|
raw_tx_hex="00",
|
||||||
|
status=status,
|
||||||
|
)
|
||||||
|
session.add(tx)
|
||||||
|
return tx
|
||||||
|
|
||||||
|
|
||||||
|
async def test_admin_pending_transactions_respects_limit(client):
|
||||||
|
from app.db import base as db_base
|
||||||
|
|
||||||
|
async with db_base.AsyncSessionLocal() as session:
|
||||||
|
for _ in range(3):
|
||||||
|
await _make_pending_transaction(session)
|
||||||
|
await session.commit()
|
||||||
|
|
||||||
|
headers = {"X-Admin-Token": "test-admin-token"}
|
||||||
|
resp = await client.get("/admin/pending-transactions", headers=headers, params={"limit": 2})
|
||||||
|
assert resp.status_code == 200
|
||||||
|
assert len(resp.json()) == 2
|
||||||
|
|
||||||
|
|
||||||
|
async def test_admin_pending_transactions_status_filter(client):
|
||||||
|
from app.db import base as db_base
|
||||||
|
|
||||||
|
async with db_base.AsyncSessionLocal() as session:
|
||||||
|
await _make_pending_transaction(session, status="pending")
|
||||||
|
await _make_pending_transaction(session, status="confirmed")
|
||||||
|
await _make_pending_transaction(session, status="failed")
|
||||||
|
await session.commit()
|
||||||
|
|
||||||
|
headers = {"X-Admin-Token": "test-admin-token"}
|
||||||
|
resp = await client.get(
|
||||||
|
"/admin/pending-transactions", headers=headers, params={"status": "confirmed"}
|
||||||
|
)
|
||||||
|
assert resp.status_code == 200
|
||||||
|
entries = resp.json()
|
||||||
|
assert len(entries) == 1
|
||||||
|
assert entries[0]["status"] == "confirmed"
|
||||||
|
|||||||
Reference in New Issue
Block a user