Widen raw_tx_hex and payload_json from String to Text (B-47)
Both held arbitrary-length data (a raw signed transaction hex, an audit payload) in a bare String, which SQLAlchemy compiles to VARCHAR with no length. SQLite and PostgreSQL accept that; other backends like MySQL require a length on VARCHAR and would reject it. Add a migration (verified upgrade/downgrade/upgrade round-trip, and confirmed with `alembic check` that it leaves no further diff against the models). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Claude Sonnet 5
@@ -1,11 +1,11 @@
|
||||
# Known bugs
|
||||
|
||||
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-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 3 are
|
||||
7 medium, 8 low), listed below as B-48 … B-49. B-25 through B-47 are fixed (see "Previously
|
||||
fixed" below) — no Critical-, High- or Medium-severity finding remains open; the remaining 2 are
|
||||
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-two fixes so far brought
|
||||
the suite from 139 to 246).
|
||||
by existing coverage — every fix lands with a regression test (the twenty-three fixes so far
|
||||
brought the suite from 139 to 248).
|
||||
|
||||
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.
|
||||
@@ -18,12 +18,6 @@ admin auth, single-process assumptions, no user-facing history, etc.) are docume
|
||||
|
||||
## Low / hygiene
|
||||
|
||||
### 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
|
||||
SQLite and PostgreSQL and breaks elsewhere.
|
||||
**Fix:** switch both to `Text` in a migration.
|
||||
|
||||
### B-48 — No cap on input count in `select_utxos`
|
||||
|
||||
A user with hundreds of small UTXOs builds a huge transaction whose fee — deducted from the bet
|
||||
@@ -56,6 +50,7 @@ already does.
|
||||
- **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-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-47** — `raw_tx_hex` and `payload_json` were unbounded `String` columns (`VARCHAR` with no length) — fine on SQLite/PostgreSQL, rejected by backends like MySQL that require a length
|
||||
- **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-34** — password change/reset didn't invalidate already-issued JWTs, so a stolen token survived a change meant to lock it out
|
||||
@@ -68,7 +63,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
|
||||
|
||||
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/B-46 fixes). Suite grew from 139 to 246 tests over the twenty-two.
|
||||
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/B-47 fixes). Suite grew from 139 to 248 tests over the twenty-three.
|
||||
|
||||
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,
|
||||
|
||||
@@ -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.
|
||||
|
||||
**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.
|
||||
**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 **2 are still open** — no Critical, High or Medium remains, only Low/hygiene: no cap on input count in `select_utxos` (B-48) and rollback paths not publishing an SSE update (B-49). 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.
|
||||
|
||||
@@ -233,7 +233,7 @@ Explicit design choices, not derivable from any single file — respect them:
|
||||
|
||||
## Known gaps / TODO
|
||||
|
||||
Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (3 open) — not duplicated here.
|
||||
Accepted **by design**. For actual bugs see [BUGS.md](BUGS.md) (2 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.
|
||||
- **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
-3
@@ -1,6 +1,6 @@
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from sqlalchemy import BigInteger, ForeignKey, Index, String, UniqueConstraint, text
|
||||
from sqlalchemy import BigInteger, ForeignKey, Index, String, Text, UniqueConstraint, text
|
||||
from sqlalchemy.orm import Mapped, mapped_column
|
||||
|
||||
from app.db.base import Base
|
||||
@@ -154,7 +154,7 @@ class PendingTransaction(Base):
|
||||
user_id: Mapped[int | None] = mapped_column(ForeignKey("users.id"), default=None)
|
||||
current_txid: Mapped[str] = mapped_column(String(64))
|
||||
fee_rate_sat_vb: Mapped[int]
|
||||
raw_tx_hex: Mapped[str] = mapped_column(String)
|
||||
raw_tx_hex: Mapped[str] = mapped_column(Text)
|
||||
# The *first* broadcast — never rewritten by a bump — since this is what the
|
||||
# reconciler's abandon-after-N-hours grace period (app/tx/reconcile.py) measures
|
||||
# from. Bumping used to overwrite this field, which reset that clock on every
|
||||
@@ -193,7 +193,7 @@ class AuditLog(Base):
|
||||
|
||||
id: Mapped[int] = mapped_column(primary_key=True)
|
||||
event_type: Mapped[str] = mapped_column(String(32))
|
||||
payload_json: Mapped[str] = mapped_column(String)
|
||||
payload_json: Mapped[str] = mapped_column(Text)
|
||||
user_id: Mapped[int | None] = mapped_column(ForeignKey("users.id"), default=None)
|
||||
round_id: Mapped[int | None] = mapped_column(ForeignKey("rounds.id"), default=None)
|
||||
created_at: Mapped[datetime] = mapped_column(default=utcnow)
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
"""widen raw_tx_hex and payload_json to Text
|
||||
|
||||
Fixes B-47: both columns held arbitrary-length data (a raw signed transaction
|
||||
hex, and a JSON audit payload) in an unbounded `String`, which SQLAlchemy
|
||||
compiles to `VARCHAR` with no length. That's accepted by SQLite and
|
||||
PostgreSQL but rejected by other backends (e.g. MySQL requires a length on
|
||||
VARCHAR) — `Text` is the portable type for both.
|
||||
|
||||
Revision ID: 87a0c640355c
|
||||
Revises: 9ef6a51509f7
|
||||
Create Date: 2026-07-27
|
||||
|
||||
"""
|
||||
from typing import Sequence, Union
|
||||
|
||||
from alembic import op
|
||||
import sqlalchemy as sa
|
||||
|
||||
|
||||
# revision identifiers, used by Alembic.
|
||||
revision: str = '87a0c640355c'
|
||||
down_revision: Union[str, Sequence[str], None] = '9ef6a51509f7'
|
||||
branch_labels: Union[str, Sequence[str], None] = None
|
||||
depends_on: Union[str, Sequence[str], None] = None
|
||||
|
||||
|
||||
def upgrade() -> None:
|
||||
with op.batch_alter_table('audit_log') as batch_op:
|
||||
batch_op.alter_column('payload_json', existing_type=sa.VARCHAR(), type_=sa.Text(), existing_nullable=False)
|
||||
with op.batch_alter_table('pending_transactions') as batch_op:
|
||||
batch_op.alter_column('raw_tx_hex', existing_type=sa.VARCHAR(), type_=sa.Text(), existing_nullable=False)
|
||||
|
||||
|
||||
def downgrade() -> None:
|
||||
with op.batch_alter_table('pending_transactions') as batch_op:
|
||||
batch_op.alter_column('raw_tx_hex', existing_type=sa.Text(), type_=sa.VARCHAR(), existing_nullable=False)
|
||||
with op.batch_alter_table('audit_log') as batch_op:
|
||||
batch_op.alter_column('payload_json', existing_type=sa.Text(), type_=sa.VARCHAR(), existing_nullable=False)
|
||||
@@ -0,0 +1,16 @@
|
||||
"""B-47: raw_tx_hex (a full raw signed transaction hex) and payload_json (an
|
||||
arbitrary audit payload) must stay `Text`, not a bare `String`/`VARCHAR` with
|
||||
no length -- SQLite and PostgreSQL accept that, but other backends (e.g.
|
||||
MySQL) require a length on VARCHAR and would reject it."""
|
||||
|
||||
from sqlalchemy import Text
|
||||
|
||||
from app.db.models import AuditLog, PendingTransaction
|
||||
|
||||
|
||||
def test_pending_transaction_raw_tx_hex_is_text():
|
||||
assert isinstance(PendingTransaction.__table__.c.raw_tx_hex.type, Text)
|
||||
|
||||
|
||||
def test_audit_log_payload_json_is_text():
|
||||
assert isinstance(AuditLog.__table__.c.payload_json.type, Text)
|
||||
Reference in New Issue
Block a user