Make usernames one case-insensitive namespace (B-57)
The login throttle keyed on body.username.lower() while the lookup matched User.username exactly, so "Bob" and "bob" were two accounts sharing one rate-limit bucket — each able to lock the other out — and registration happily accepted near-duplicate names, which on a custodial system is an impersonation vector. Uniqueness is now the database's job: a unique index on lower(username), with register and login both matching through func.lower(). The name is still stored exactly as typed, since that's what /admin and the audit log display, and the username pattern is ASCII-only so lower() is the whole of the normalization. The migration refuses to run if two existing accounts differ only by case. It can't merge or rename one automatically: both are custodial accounts that may hold funds, so that would be the migration silently deciding who owns what. It names the collisions and leaves them to the operator — the container runs `alembic upgrade head` at startup, so it surfaces as a refusal to start rather than a half-applied schema. Verified both directions against a scratch DB, plus `alembic check` (clean) and the collision guard actually firing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -40,18 +40,6 @@ remains the last prerequisite for running unattended.
|
|||||||
|
|
||||||
## High — security
|
## 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
|
### B-58 — the registration throttle counts successes as failures and is IP-only
|
||||||
|
|
||||||
`app/auth/routes.py:66-71`.
|
`app/auth/routes.py:66-71`.
|
||||||
|
|||||||
@@ -8,7 +8,7 @@ The user communicates in Italian in chat — reply to them in Italian. Everythin
|
|||||||
|
|
||||||
## Project status
|
## 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.
|
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/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
|
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 # one file
|
||||||
python -m pytest tests/unit/test_hd.py::test_derivation_is_deterministic # one test
|
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.
|
- **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.
|
- 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.
|
- `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.
|
- 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`).
|
- 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`).
|
||||||
|
|
||||||
|
|||||||
+6
-2
@@ -75,7 +75,11 @@ async def register(
|
|||||||
raise _rate_limited_error(retry_after)
|
raise _rate_limited_error(retry_after)
|
||||||
limiters.register_ip.record_failure(ip_key)
|
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:
|
if existing is not None:
|
||||||
raise http_error(status.HTTP_409_CONFLICT, "username_taken", "username already taken")
|
raise http_error(status.HTTP_409_CONFLICT, "username_taken", "username already taken")
|
||||||
|
|
||||||
@@ -134,7 +138,7 @@ async def login(
|
|||||||
if retry_after > 0:
|
if retry_after > 0:
|
||||||
raise _rate_limited_error(retry_after)
|
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):
|
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
|
# Same code path (and therefore the same response) whether the username
|
||||||
# doesn't exist or the password is wrong — no enumeration oracle here.
|
# doesn't exist or the password is wrong — no enumeration oracle here.
|
||||||
|
|||||||
@@ -13,6 +13,16 @@ def utcnow() -> datetime:
|
|||||||
class User(Base):
|
class User(Base):
|
||||||
__tablename__ = "users"
|
__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)
|
id: Mapped[int] = mapped_column(primary_key=True)
|
||||||
username: Mapped[str] = mapped_column(String(64), unique=True, index=True)
|
username: Mapped[str] = mapped_column(String(64), unique=True, index=True)
|
||||||
password_hash: Mapped[str] = mapped_column(String(256))
|
password_hash: Mapped[str] = mapped_column(String(256))
|
||||||
|
|||||||
@@ -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")
|
||||||
@@ -124,3 +124,47 @@ async def test_registration_is_rate_limited_per_ip(client):
|
|||||||
)
|
)
|
||||||
assert resp.status_code == 429
|
assert resp.status_code == 429
|
||||||
assert resp.json()["detail"]["code"] == "rate_limited"
|
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()
|
||||||
|
|||||||
Reference in New Issue
Block a user