diff --git a/BUGS.md b/BUGS.md index 1a9d1d6..d326a91 100644 --- a/BUGS.md +++ b/BUGS.md @@ -40,18 +40,6 @@ remains the last prerequisite for running unattended. ## High — security -### 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`. diff --git a/CLAUDE.md b/CLAUDE.md index 9549b70..f03836e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin ## Project status -All 10 stages of the original build order are code-complete and unit-tested — 287 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. +All 10 stages of the original build order are code-complete and unit-tested — 290 tests, all under `tests/unit/` (`tests/integration/` is an empty package). Beyond them: Docker + Caddy deployment, admin dashboard (`/admin`), static test UI (`/`), pending-inclusive balance display, an SSE push channel layered over the original polling, self-service password change + admin password reset, and the reconciliation/corroboration machinery below. 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. @@ -33,7 +33,7 @@ PYTHONPATH=. python scripts/decrypt_master_key.py # ops recovery: decrypt+pr PYTHONPATH=. python scripts/encrypt_master_key.py # ops bootstrap: import an externally-generated xprv (--overwrite to replace) PYTHONPATH=. python scripts/electrum_smoke_test.py # manual check: connect, handshake, subscribe to headers, print the tip -python -m pytest # all 287 tests +python -m pytest # all 290 tests python -m pytest tests/unit/test_hd.py # one file python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test ``` @@ -232,6 +232,7 @@ Explicit design choices, not derivable from any single file — respect them: - **1 confirmation** for every tx kind. Don't introduce differing thresholds (3, 6, …) without an explicit decision. - The draw algorithm is a **replaceable component**, not the final design — don't architect around its current form. - `GET /admin/users/{id}/privkey` exporting a raw WIF is **intentional**, not a vulnerability: the server already holds the master key, so this only exposes via API what an operator could script anyway. Every access writes `admin_privkey_accessed` — don't remove that logging. +- **Usernames are case-insensitive** (B-57): one namespace, enforced by a unique index on `lower(username)` (`app/db/models.py`) and matched with `func.lower(...)` on both register and login. The name is still *stored* as typed — that's what `/admin` and the audit log show. The migration refuses to run if two existing accounts differ only by case, rather than guessing which one to rename: both may hold funds. - Argon2 hashing means **no password recovery, only reset**: `POST /admin/users/{id}/reset-password` sets a new random password, returns it once for the operator to relay, and logs `admin_password_reset`. No self-service reset exists (no email is ever collected); a logged-in user can only *change* their password by supplying the current one. - RBF bumps are paid by whoever's change the tx pays back to — the user for bets/withdrawals, the pool for payouts. Counterparty outputs (recipient, winner, fee address) are never touched; only the sender's own change shrinks (`bump_fee`). diff --git a/app/auth/routes.py b/app/auth/routes.py index 59eba0e..7a68ba0 100644 --- a/app/auth/routes.py +++ b/app/auth/routes.py @@ -75,7 +75,11 @@ async def register( raise _rate_limited_error(retry_after) limiters.register_ip.record_failure(ip_key) - existing = await session.scalar(select(User).where(User.username == body.username)) + # B-57: case-insensitive, matching the unique index on lower(username) — and + # matching the throttle key below, which has always been lowercased. + existing = await session.scalar( + select(User).where(func.lower(User.username) == body.username.lower()) + ) if existing is not None: raise http_error(status.HTTP_409_CONFLICT, "username_taken", "username already taken") @@ -134,7 +138,7 @@ async def login( if retry_after > 0: raise _rate_limited_error(retry_after) - user = await session.scalar(select(User).where(User.username == body.username)) + user = await session.scalar(select(User).where(func.lower(User.username) == body.username.lower())) if user is None or not await verify_password_async(body.password, user.password_hash): # Same code path (and therefore the same response) whether the username # doesn't exist or the password is wrong — no enumeration oracle here. diff --git a/app/db/models.py b/app/db/models.py index 911085f..70f9ed6 100644 --- a/app/db/models.py +++ b/app/db/models.py @@ -13,6 +13,16 @@ def utcnow() -> datetime: class User(Base): __tablename__ = "users" + # B-57: usernames are compared case-insensitively, and that has to be the + # database's job, not a convention the query layer remembers. "Bob" and "bob" + # used to be two accounts sharing one rate-limit bucket (each locking the other + # out) and, worse on a custodial system, a ready-made impersonation vector. + # A functional unique index rather than a normalized column: the name stays + # stored exactly as the user typed it, which is what /admin and the audit log + # display. The username pattern (auth/routes.py) is ASCII-only, so lower() is + # the whole of the normalization — no Unicode casefolding subtleties apply. + __table_args__ = (Index("ix_users_username_lower", text("lower(username)"), unique=True),) + id: Mapped[int] = mapped_column(primary_key=True) username: Mapped[str] = mapped_column(String(64), unique=True, index=True) password_hash: Mapped[str] = mapped_column(String(256)) diff --git a/migrations/versions/c1d4a97b5e10_case_insensitive_usernames.py b/migrations/versions/c1d4a97b5e10_case_insensitive_usernames.py new file mode 100644 index 0000000..8c2c1ed --- /dev/null +++ b/migrations/versions/c1d4a97b5e10_case_insensitive_usernames.py @@ -0,0 +1,46 @@ +"""case-insensitive usernames (B-57) + +Revision ID: c1d4a97b5e10 +Revises: be71fdac734e +Create Date: 2026-08-03 18:10:00.000000 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = 'c1d4a97b5e10' +down_revision: Union[str, Sequence[str], None] = 'be71fdac734e' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + # The index cannot be created while two accounts differ only by case, and + # nothing here may guess which of them is the "real" one: both are custodial + # accounts that may hold funds, so merging or renaming one automatically would + # be the migration silently deciding who owns what. Fail loudly instead, naming + # the collisions, and let the operator rename one account (and tell that user) + # before retrying. The container runs `alembic upgrade head` at startup, so this + # surfaces as a refusal to start rather than as a half-applied schema. + collisions = op.get_bind().exec_driver_sql( + "SELECT group_concat(username, ', ') FROM users " + "GROUP BY lower(username) HAVING count(*) > 1" + ).fetchall() + if collisions: + groups = "; ".join(row[0] for row in collisions) + raise RuntimeError( + "cannot enforce case-insensitive usernames: these accounts differ only " + f"by case and must be resolved by hand first — {groups}" + ) + + op.create_index("ix_users_username_lower", "users", [sa.text("lower(username)")], unique=True) + + +def downgrade() -> None: + """Downgrade schema.""" + op.drop_index("ix_users_username_lower", table_name="users") diff --git a/tests/unit/test_auth.py b/tests/unit/test_auth.py index e1d33e1..7bb2204 100644 --- a/tests/unit/test_auth.py +++ b/tests/unit/test_auth.py @@ -124,3 +124,47 @@ async def test_registration_is_rate_limited_per_ip(client): ) assert resp.status_code == 429 assert resp.json()["detail"]["code"] == "rate_limited" + + +# --- B-57: usernames are one namespace, case included --------------------------- + + +async def test_registration_refuses_a_username_differing_only_by_case(client): + """"Bob" and "bob" used to be two accounts. On a custodial system that's an + impersonation vector — and the two also shared a single rate-limit bucket, since + the throttle key has always been lowercased, so each could lock the other out.""" + await _register(client, username="Bob") + + resp = await client.post("/auth/register", json={"username": "bob", "password": "another-password"}) + + assert resp.status_code == 409 + assert resp.json()["detail"]["code"] == "username_taken" + + +async def test_login_accepts_the_username_in_any_case(client): + """The flip side of the same rule: one account, reachable however it's typed.""" + await _register(client, username="Alice", password="original-password") + + resp = await client.post("/auth/login", json={"username": "ALICE", "password": "original-password"}) + + assert resp.status_code == 200 + assert resp.json()["access_token"] + + +async def test_the_database_itself_rejects_a_case_variant(client): + """Not just the pre-check in the handler: two requests racing between the SELECT + and the INSERT must still leave only one account, which is what the unique index + on lower(username) guarantees.""" + from sqlalchemy.exc import IntegrityError + + from app.db.base import AsyncSessionLocal + from app.db.models import User + + await _register(client, username="Carol") + + async with AsyncSessionLocal() as session: + session.add( + User(username="CAROL", password_hash="x", derivation_index=999, address="plm1-unused") + ) + with pytest.raises(IntegrityError): + await session.commit()