Validate admin config and registration input, and log config changes

fee_address was the dangerous one (B-05). PUT /admin/config assigned whatever it
was given, and a well-formed address from another chain (bc1...) parses fine as a
witness program — so every round's 30% commission would be signed and broadcast
to a script nobody holds the key for. A malformed one instead wedged the payout
with an unhandled EmbitError. It now has to pass is_valid_plm_address, the same
check user withdrawals already had. Numeric fields got bounds too:
fee_rate_sat_vb=0 produces transactions no node relays, which stalls bets,
payouts and withdrawals alike, and round_duration_seconds=0 expires a round the
instant it opens.

Config changes are audit-logged (B-10). /pause and /resume were logged but a
config edit wasn't, so the most sensitive setting in the system could be changed
without leaving any trace — contradicting CLAUDE.md, which says audit_log records
what changed. The entry carries a before/after diff per field, computed before
assignment, and no-op updates write nothing. `paused` was removed from
_CONFIG_FIELDS so the maintenance switch has exactly one audited path; it stays
in the response model.

Admin token comparison is constant-time (B-14), with the empty-token check kept
*ahead* of it: compare_digest("", "") returns True, so the obvious ordering would
have opened the panel on any instance without an ADMIN_TOKEN.

Registration input (B-12). It accepted an empty username and a one-character
password while /users/me/change-password demanded 8 — an odd place to be lenient
on a custodial system holding real funds. MIN_PASSWORD_LENGTH moved to
auth/security.py so both share it, and the username is constrained to 3-32 chars
of [A-Za-z0-9_.-]. The IntegrityError handler also distinguishes a username
collision (answers username_taken) from a derivation-index one (retries): a
concurrent duplicate username used to be retried five times and then reported as
derivation_index_conflict, which told the user the wrong thing.

verify_password (B-13) catches VerificationError and InvalidHashError, not just
VerifyMismatchError, so an unparseable stored hash reads as "wrong password"
instead of a 500 — logged as an error, since that one is a data problem.

guida-admin.md gains a table of the audit events worth watching, including
payout_failed, which needs manual intervention.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

app/api/routes/admin.py

@@ -2,7 +2,7 @@ import json
 import secrets
 
 from fastapi import APIRouter, Depends, Header, HTTPException, status
-from pydantic import BaseModel
+from pydantic import BaseModel, Field, field_validator
 from sqlalchemy import select
 from sqlalchemy.ext.asyncio import AsyncSession
 
@@ -12,16 +12,26 @@ from app.config import settings
 from app.db.models import AuditLog, PendingTransaction, Round, User
 from app.db.session import get_session
 from app.rounds.config import get_round_config
+from app.wallet.address import is_valid_plm_address
 from app.wallet.hd import derive_user_wif
 
 router = APIRouter(prefix="/admin", tags=["admin"])
 
 
 async def require_admin(x_admin_token: str = Header(default="")) -> None:
-    if not settings.admin_token or x_admin_token != settings.admin_token:
+    # An unset ADMIN_TOKEN denies everything — checked first, since compare_digest
+    # on two empty strings returns True and would otherwise open the panel to
+    # anyone on an instance that never configured a token.
+    if not settings.admin_token:
+        raise HTTPException(status.HTTP_403_FORBIDDEN, "invalid admin token")
+    if not secrets.compare_digest(x_admin_token, settings.admin_token):
         raise HTTPException(status.HTTP_403_FORBIDDEN, "invalid admin token")
 
 
+# `paused` is deliberately NOT here: it has its own audit-logged endpoints
+# (/admin/pause, /admin/resume), and accepting it on PUT /config as well gave the
+# operator an unlogged way to stop the lottery (B-10). It stays in the response
+# model, so the dashboard still reads its current value from here.
 _CONFIG_FIELDS = (
     "fee_address",
     "bet_amount_sats",
@@ -30,7 +40,6 @@ _CONFIG_FIELDS = (
     "fee_rate_sat_vb",
     "rbf_timeout_seconds",
     "draw_animation_seconds",
-    "paused",
 )
 
 
@@ -46,18 +55,41 @@ class RoundConfigResponse(BaseModel):
 
 
 class RoundConfigUpdate(BaseModel):
+    """Bounds are enforced here rather than trusting the operator: a value like
+    fee_rate_sat_vb=0 produces transactions no node will relay (stalling every bet,
+    payout and withdrawal), and round_duration_seconds=0 expires a round the instant
+    it opens. `paused` is not accepted — see _CONFIG_FIELDS."""
+
+    # An unvalidated fee_address was the worst of the lot: a malformed one wedged the
+    # payout with an unhandled EmbitError, and a well-formed *foreign* one (bc1...)
+    # parses fine as a witness program, so every round's 30 % commission would be
+    # broadcast to a script nobody holds the key for (B-05).
     fee_address: str | None = None
-    bet_amount_sats: int | None = None
+    bet_amount_sats: int | None = Field(default=None, gt=0, le=100_000 * 100_000_000)
-    round_duration_seconds: int | None = None
+    round_duration_seconds: int | None = Field(default=None, ge=30, le=7 * 24 * 3600)
-    round_cooldown_seconds: int | None = None
+    round_cooldown_seconds: int | None = Field(default=None, ge=0, le=24 * 3600)
-    fee_rate_sat_vb: int | None = None
+    fee_rate_sat_vb: int | None = Field(default=None, ge=1, le=10_000)
-    rbf_timeout_seconds: int | None = None
+    rbf_timeout_seconds: int | None = Field(default=None, ge=60, le=7 * 24 * 3600)
-    draw_animation_seconds: int | None = None
+    draw_animation_seconds: int | None = Field(default=None, ge=0, le=600)
-    paused: bool | None = None
+
+    @field_validator("fee_address")
+    @classmethod
+    def _validate_fee_address(cls, value: str | None) -> str | None:
+        if value is None:
+            return None
+        value = value.strip()
+        if not is_valid_plm_address(value):
+            raise ValueError(
+                "fee_address must be a valid PLM bech32 address (plm1...) — an address from "
+                "another chain would send every round's commission somewhere unspendable"
+            )
+        return value
 
 
 def _config_response(config) -> RoundConfigResponse:
-    return RoundConfigResponse(**{field: getattr(config, field) for field in _CONFIG_FIELDS})
+    fields = {field: getattr(config, field) for field in _CONFIG_FIELDS}
+    fields["paused"] = config.paused
+    return RoundConfigResponse(**fields)
 
 
 @router.get("/config", response_model=RoundConfigResponse, dependencies=[Depends(require_admin)])
@@ -72,10 +104,22 @@ async def update_config(
     body: RoundConfigUpdate, session: AsyncSession = Depends(get_session)
 ) -> RoundConfigResponse:
     config = await get_round_config(session)
+    # Diff computed before assignment so the audit entry records both sides. Without
+    # it, the most sensitive setting in the system (fee_address — where 30 % of every
+    # pool goes) could be changed without leaving any trace at all (B-10).
+    changes: dict[str, dict] = {}
     for field in _CONFIG_FIELDS:
         value = getattr(body, field)
-        if value is not None:
+        if value is None:
+            continue
+        previous = getattr(config, field)
+        if previous == value:
+            continue
+        changes[field] = {"from": previous, "to": value}
         setattr(config, field, value)
+
+    if changes:
+        await write_audit_log(session, "config_updated", changes)
     await session.commit()
     return _config_response(config)
 

app/api/routes/users.py

@@ -5,15 +5,13 @@ from sqlalchemy.ext.asyncio import AsyncSession
 
 from app.api.errors import http_error
 from app.auth.dependencies import get_current_user
-from app.auth.security import hash_password, verify_password
+from app.auth.security import MIN_PASSWORD_LENGTH, hash_password, verify_password
 from app.db.models import Round, RoundParticipant, User
 from app.db.session import get_session
 from app.wallet.balance import compute_pending_balance
 
 router = APIRouter(prefix="/users", tags=["users"])
 
-_MIN_PASSWORD_LENGTH = 8
-
 
 class MeResponse(BaseModel):
     id: int
@@ -60,12 +58,12 @@ async def change_password(
         raise http_error(
             status.HTTP_401_UNAUTHORIZED, "current_password_incorrect", "current password is incorrect"
         )
-    if len(body.new_password) < _MIN_PASSWORD_LENGTH:
+    if len(body.new_password) < MIN_PASSWORD_LENGTH:
         raise http_error(
             status.HTTP_400_BAD_REQUEST,
             "password_too_short",
-            f"new password must be at least {_MIN_PASSWORD_LENGTH} characters",
+            f"new password must be at least {MIN_PASSWORD_LENGTH} characters",
-            minimum=_MIN_PASSWORD_LENGTH,
+            minimum=MIN_PASSWORD_LENGTH,
         )
 
     user.password_hash = hash_password(body.new_password)

app/auth/routes.py

@@ -1,11 +1,11 @@
 from fastapi import APIRouter, Depends, Request, status
-from pydantic import BaseModel
+from pydantic import BaseModel, Field
 from sqlalchemy import func, select
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.ext.asyncio import AsyncSession
 
 from app.api.errors import http_error
-from app.auth.security import create_access_token, hash_password, verify_password
+from app.auth.security import MIN_PASSWORD_LENGTH, create_access_token, hash_password, verify_password
 from app.db.models import User
 from app.db.session import get_session
 from app.wallet.hd import derive_user_address
@@ -16,8 +16,13 @@ _MAX_REGISTER_RETRIES = 5
 
 
 class RegisterRequest(BaseModel):
-    username: str
+    """Registration used to accept an empty username and a one-character password,
-    password: str
+    while /users/me/change-password demanded 8 characters — an odd place to be
+    lenient on a custodial system holding real funds (B-12). MIN_PASSWORD_LENGTH is
+    shared with that endpoint so the two can't drift apart again."""
+
+    username: str = Field(min_length=3, max_length=32, pattern=r"^[A-Za-z0-9_.-]+$")
+    password: str = Field(min_length=MIN_PASSWORD_LENGTH, max_length=256)
 
 
 class TokenResponse(BaseModel):
@@ -48,8 +53,16 @@ async def register(
         session.add(user)
         try:
             await session.commit()
-        except IntegrityError:
+        except IntegrityError as exc:
             await session.rollback()
+            # Only a derivation-index collision is worth retrying. A username
+            # collision (someone registered the same name between the check above and
+            # this commit) is permanent, and retrying it five times only to report
+            # "derivation_index_conflict" told the user the wrong thing entirely (B-12).
+            if "username" in str(exc.orig).lower():
+                raise http_error(
+                    status.HTTP_409_CONFLICT, "username_taken", "username already taken"
+                ) from exc
             continue
         await session.refresh(user)
         request.app.state.electrum_listener.address_for_new_user(user.id, user.address)

app/auth/security.py

@@ -1,11 +1,19 @@
 from datetime import datetime, timedelta, timezone
 
+import logging
+
 import jwt
 from argon2 import PasswordHasher
-from argon2.exceptions import VerifyMismatchError
+from argon2.exceptions import InvalidHashError, VerificationError
 
 from app.config import settings
 
+logger = logging.getLogger(__name__)
+
+# Shared by registration (app/auth/routes.py) and the self-service password change
+# (app/api/routes/users.py) so the two can't enforce different minimums.
+MIN_PASSWORD_LENGTH = 8
+
 _hasher = PasswordHasher()
 
 
@@ -14,9 +22,20 @@ def hash_password(password: str) -> str:
 
 
 def verify_password(password: str, password_hash: str) -> bool:
+    """Any failure to verify reads as "wrong password", never as a server error.
+
+    Catching only VerifyMismatchError left the other two cases as unhandled 500s
+    (B-13): VerificationError covers argon2's other verification failures, and
+    InvalidHashError fires when the stored hash can't be parsed at all — which is a
+    data problem worth logging, but from the caller's side it still just means this
+    password does not open this account.
+    """
     try:
         return _hasher.verify(password_hash, password)
-    except VerifyMismatchError:
+    except InvalidHashError:
+        logger.error("stored password hash is unparseable — password verification cannot succeed")
+        return False
+    except VerificationError:
         return False
 
 

docs/guida-admin.md

@@ -108,6 +108,16 @@ piazzate, payout inviati, round chiusi, configurazione modificata, accessi
 alle chiavi private, ecc. È il primo posto da controllare per ricostruire
 cosa è successo dopo un problema.
 
+Eventi a cui vale la pena prestare attenzione:
+
+| Evento | Significato |
+|---|---|
+| `config_updated` | Un parametro è stato modificato; il payload contiene valore precedente e nuovo per ogni campo cambiato. |
+| `bet_broadcast_failed` / `withdrawal_broadcast_failed` | La rete ha rifiutato la transazione. Non è stato speso nulla: gli UTXO sono stati liberati e il saldo dell'utente è tornato come prima. |
+| `pending_tx_abandoned` | Una transazione trasmessa è scomparsa dalla catena e il sistema l'ha dichiarata persa: UTXO liberati, bet rimossa o prelievo segnato `failed`. Se capita spesso, la fee rate configurata è probabilmente troppo bassa. |
+| `pending_tx_recovered` | Una transazione che si credeva incompleta è invece finita in catena (tipicamente dopo un riavvio a metà invio) e il sistema l'ha ripresa da sé. |
+| `payout_failed` | Il payout di un round non è partito. Il round resta in `paying_out` e **richiede intervento manuale**: non esiste un retry automatico. Controlla `fee_address`, il saldo dell'indirizzo pool e la connessione Electrum. |
+
 ## Alternative all'interfaccia grafica
 
 Le stesse operazioni si possono fare da terminale o da Swagger UI
@@ -125,6 +135,18 @@ curl -X PUT https://<host>/admin/config \
   -d '{"fee_address": "plm1q...", "bet_amount_sats": 1000000000, "round_duration_seconds": 600}'
 ```
 
+I valori vengono validati: `fee_address` deve essere un indirizzo bech32 PLM
+valido (`plm1...`) e i parametri numerici hanno limiti di buon senso
+(`fee_rate_sat_vb` almeno 1, durata round almeno 30s, ecc.). Un valore fuori
+range viene rifiutato con un errore 422 e la configurazione resta invariata.
+Il controllo su `fee_address` è deliberatamente severo: un indirizzo di
+un'altra catena (per esempio `bc1...`) sarebbe formalmente valido come witness
+program, e il 30% di commissione di ogni round finirebbe su uno script di cui
+nessuno ha la chiave.
+
+```bash
+```
+
 ## Utenti, chiave privata e reset password
 
 La card "Utenti" elenca id, username, indirizzo e saldo di ogni utente

tests/unit/test_admin.py

@@ -4,6 +4,11 @@ from httpx import ASGITransport, AsyncClient
 
 from app.config import settings
 
+# A real PLM bech32 address: PUT /admin/config now validates fee_address, since a
+# foreign-chain address there would send every round's commission to a script
+# nobody can spend (B-05).
+_VALID_FEE_ADDRESS = "plm1q0xcqpzrky6eff2g52qdye53xkk9jxkvraxkkwd"
+
 
 @pytest.fixture
 async def client(monkeypatch, tmp_path):
@@ -70,15 +75,15 @@ async def test_admin_reads_and_updates_config(client):
     assert resp.json()["fee_address"] == ""
 
     resp = await client.put(
-        "/admin/config", headers=headers, json={"fee_address": "plm1qfeeaddress", "bet_amount_sats": 500_000_000}
+        "/admin/config", headers=headers, json={"fee_address": _VALID_FEE_ADDRESS, "bet_amount_sats": 500_000_000}
     )
     assert resp.status_code == 200
     body = resp.json()
-    assert body["fee_address"] == "plm1qfeeaddress"
+    assert body["fee_address"] == _VALID_FEE_ADDRESS
     assert body["bet_amount_sats"] == 500_000_000
 
     resp = await client.get("/admin/config", headers=headers)
-    assert resp.json()["fee_address"] == "plm1qfeeaddress"
+    assert resp.json()["fee_address"] == _VALID_FEE_ADDRESS
 
 
 async def test_admin_can_pause_and_resume_the_lottery(client):
@@ -211,3 +216,60 @@ async def test_admin_reset_password_404_for_unknown_user(client):
     headers = {"X-Admin-Token": "test-admin-token"}
     resp = await client.post("/admin/users/999/reset-password", headers=headers)
     assert resp.status_code == 404
+
+
+async def test_config_update_is_audit_logged(client):
+    """B-10: /pause and /resume were logged but a config change wasn't, so the most
+    sensitive setting in the system — fee_address, where 30% of every pool goes —
+    could be changed without leaving any trace."""
+    headers = {"X-Admin-Token": "test-admin-token"}
+    await client.put("/admin/config", headers=headers, json={"fee_address": _VALID_FEE_ADDRESS})
+
+    resp = await client.get("/admin/audit-log", headers=headers)
+    entries = [e for e in resp.json() if e["event_type"] == "config_updated"]
+    assert len(entries) == 1
+    assert entries[0]["payload"]["fee_address"] == {"from": "", "to": _VALID_FEE_ADDRESS}
+
+
+async def test_config_update_without_changes_logs_nothing(client):
+    headers = {"X-Admin-Token": "test-admin-token"}
+    await client.put("/admin/config", headers=headers, json={"fee_address": _VALID_FEE_ADDRESS})
+    await client.put("/admin/config", headers=headers, json={"fee_address": _VALID_FEE_ADDRESS})
+
+    resp = await client.get("/admin/audit-log", headers=headers)
+    assert len([e for e in resp.json() if e["event_type"] == "config_updated"]) == 1
+
+
+@pytest.mark.parametrize(
+    "payload",
+    [
+        {"fee_address": "bc1qw508d6qejxtdg4y5r3zarvary0c5xw7kv8f3t4"},  # valid bech32, wrong chain
+        {"fee_address": "plm1qbogus"},  # right HRP, broken checksum
+        {"fee_address": "garbage"},
+        {"fee_rate_sat_vb": 0},  # fee-less txs are never relayed: everything would stall
+        {"round_duration_seconds": 0},  # a round that expires the instant it opens
+        {"bet_amount_sats": -1},
+        {"rbf_timeout_seconds": 1},
+    ],
+)
+async def test_config_rejects_unusable_values(client, payload):
+    """B-05: every one of these was accepted before. The bc1 case is the worst — it
+    parses as a valid witness program, so each round's commission would be broadcast
+    to a script nobody holds the key for."""
+    headers = {"X-Admin-Token": "test-admin-token"}
+    resp = await client.put("/admin/config", headers=headers, json=payload)
+    assert resp.status_code == 422
+
+    # and nothing was written
+    current = (await client.get("/admin/config", headers=headers)).json()
+    for field, value in payload.items():
+        assert current[field] != value
+
+
+async def test_pause_cannot_be_toggled_through_the_config_endpoint(client):
+    """B-10: `paused` used to be settable here, bypassing the audit-logged
+    pause/resume endpoints."""
+    headers = {"X-Admin-Token": "test-admin-token"}
+    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 (await client.get("/admin/config", headers=headers)).json()["paused"] is False

tests/unit/test_security.py

@@ -11,3 +11,21 @@ def test_jwt_roundtrip(monkeypatch):
     monkeypatch.setattr(security.settings, "jwt_secret", "test-secret")
     token = security.create_access_token(user_id=42)
     assert security.decode_access_token(token) == 42
+
+
+def test_verify_password_returns_false_for_an_unparseable_hash():
+    """B-13: only VerifyMismatchError was caught, so a corrupted stored hash raised
+    InvalidHashError and became an unhandled 500 on the login endpoint instead of a
+    plain "wrong credentials" 401."""
+    from app.auth.security import verify_password
+
+    assert verify_password("whatever", "not-an-argon2-hash") is False
+    assert verify_password("whatever", "") is False
+
+
+def test_verify_password_still_rejects_a_wrong_password():
+    from app.auth.security import hash_password, verify_password
+
+    stored = hash_password("correct-horse-battery")
+    assert verify_password("correct-horse-battery", stored) is True
+    assert verify_password("wrong", stored) is False

tests/unit/test_users.py

@@ -45,7 +45,7 @@ async def client(monkeypatch, tmp_path):
     app = FastAPI()
     app.include_router(auth_router)
     app.include_router(users_router)
-    app.state.electrum_listener = ElectrumListener(lambda: None, db_base.AsyncSessionLocal)
+    app.state.electrum_listener = ElectrumListener(lambda endpoint: None, db_base.AsyncSessionLocal)
 
     transport = ASGITransport(app=app)
     async with AsyncClient(transport=transport, base_url="http://test") as ac:
@@ -108,3 +108,26 @@ async def test_change_password_requires_auth(client):
         json={"current_password": "x", "new_password": "brand-new-password"},
     )
     assert resp.status_code in (401, 403)
+
+
+@pytest.mark.parametrize(
+    "payload",
+    [
+        {"username": "", "password": "longenough1"},
+        {"username": "ab", "password": "longenough1"},          # under 3 chars
+        {"username": "bad user!", "password": "longenough1"},   # disallowed characters
+        {"username": "validname", "password": "short"},         # under MIN_PASSWORD_LENGTH
+        {"username": "validname", "password": ""},
+    ],
+)
+async def test_register_rejects_weak_credentials(client, payload):
+    """B-12: registration accepted an empty username and a one-character password,
+    while /users/me/change-password demanded 8 — an odd place to be lenient on a
+    custodial system holding real funds."""
+    resp = await client.post("/auth/register", json=payload)
+    assert resp.status_code == 422
+
+
+async def test_register_accepts_valid_credentials(client):
+    resp = await client.post("/auth/register", json={"username": "goodname", "password": "longenough1"})
+    assert resp.status_code == 201