From 85dce221c5f1a4c24e66b9bc0393a1e5815789a6 Mon Sep 17 00:00:00 2001 From: Davide Grilli Date: Mon, 27 Jul 2026 00:32:48 +0200 Subject: [PATCH] Validate admin config and registration input, and log config changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/api/routes/admin.py | 70 ++++++++++++++++++++++++++++++------- app/api/routes/users.py | 10 +++--- app/auth/routes.py | 23 +++++++++--- app/auth/security.py | 23 ++++++++++-- docs/guida-admin.md | 22 ++++++++++++ tests/unit/test_admin.py | 68 +++++++++++++++++++++++++++++++++-- tests/unit/test_security.py | 18 ++++++++++ tests/unit/test_users.py | 25 ++++++++++++- 8 files changed, 229 insertions(+), 30 deletions(-) diff --git a/app/api/routes/admin.py b/app/api/routes/admin.py index 26b08f7..11b3db5 100644 --- a/app/api/routes/admin.py +++ b/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 - round_duration_seconds: int | None = None - round_cooldown_seconds: int | None = None - fee_rate_sat_vb: int | None = None - rbf_timeout_seconds: int | None = None - draw_animation_seconds: int | None = None - paused: bool | None = None + bet_amount_sats: int | None = Field(default=None, gt=0, le=100_000 * 100_000_000) + round_duration_seconds: int | None = Field(default=None, ge=30, le=7 * 24 * 3600) + round_cooldown_seconds: int | None = Field(default=None, ge=0, le=24 * 3600) + fee_rate_sat_vb: int | None = Field(default=None, ge=1, le=10_000) + rbf_timeout_seconds: int | None = Field(default=None, ge=60, le=7 * 24 * 3600) + draw_animation_seconds: int | None = Field(default=None, ge=0, le=600) + + @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: - setattr(config, field, value) + 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) diff --git a/app/api/routes/users.py b/app/api/routes/users.py index 3f46c06..f467f92 100644 --- a/app/api/routes/users.py +++ b/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", - minimum=_MIN_PASSWORD_LENGTH, + f"new password must be at least {MIN_PASSWORD_LENGTH} characters", + minimum=MIN_PASSWORD_LENGTH, ) user.password_hash = hash_password(body.new_password) diff --git a/app/auth/routes.py b/app/auth/routes.py index c9b58a9..a744346 100644 --- a/app/auth/routes.py +++ b/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 - password: str + """Registration used to accept an empty username and a one-character password, + 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) diff --git a/app/auth/security.py b/app/auth/security.py index 947bcfc..30deb23 100644 --- a/app/auth/security.py +++ b/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 diff --git a/docs/guida-admin.md b/docs/guida-admin.md index 75d9339..be07fc9 100644 --- a/docs/guida-admin.md +++ b/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:///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 diff --git a/tests/unit/test_admin.py b/tests/unit/test_admin.py index 1bc243f..6690027 100644 --- a/tests/unit/test_admin.py +++ b/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 diff --git a/tests/unit/test_security.py b/tests/unit/test_security.py index b0ded21..67291a5 100644 --- a/tests/unit/test_security.py +++ b/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 diff --git a/tests/unit/test_users.py b/tests/unit/test_users.py index 68ce33e..994469d 100644 --- a/tests/unit/test_users.py +++ b/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