diff --git a/BUGS.md b/BUGS.md index 63a0639..4fd8552 100644 --- a/BUGS.md +++ b/BUGS.md @@ -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-33 … B-49. B-25 through B-33 are fixed (see "Previously -fixed" below) — no Critical-severity finding remains open; the other 16 are High/Medium/Low. +7 medium, 8 low), listed below as B-33 … B-49. B-25 through B-34 are fixed (see "Previously +fixed" below) — no Critical-severity finding remains open; the other 15 are High/Medium/Low. 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 nine fixes so far brought the suite -from 139 to 192). +coverage — every fix lands with a regression test (the ten fixes so far brought the suite +from 139 to 194). 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,21 +18,6 @@ single-process assumptions, no user-facing history, etc.), see "Known gaps / TOD --- -## High - -### B-34 — Password change and admin reset do not invalidate existing sessions - -Neither `/users/me/change-password` nor `/admin/users/{id}/reset-password` invalidates -already-issued JWTs (24h default lifetime, no revocation, no `token_version` on the user). The -admin reset exists precisely for the "account compromised" case and **does not evict the -attacker**. - -**Proposed fix.** Add a `token_version` (or `password_changed_at`) column on `User`, embed it -in the JWT claims, and reject any token whose value is stale in -`auth/dependencies.py:get_current_user`. Bump it on both endpoints. - ---- - ## Medium ### B-35 — Every API timestamp is naive, so the frontend renders it in the wrong timezone @@ -212,9 +197,10 @@ already does. - **B-31** — resubscribing on reconnect ran serially before anything else started, freezing the chain tip (and so an in-flight draw) for the whole sweep - **B-32** — an RBF bump's fee delta could fall below BIP125's relay-mandated minimum, so the node rejected it and the same tick retried identically forever; also had no ceiling on how high the fee rate could climb - **B-33** — `POST /auth/login` had no rate limiting on a custodial wallet, so a patient distributed attack could brute-force a password against an enumerable username list; fixed with per-username *and* per-IP exponential backoff (`app/auth/rate_limit.py`), registration throttled per-IP too (also bounds B-31's attacker-controlled user count) +- **B-34** — neither self-service password change nor the admin reset invalidated already-issued JWTs, so a stolen token (or an attacker's own session) survived a password change meant to lock it out; fixed with a `User.token_version` column embedded in every JWT (`"tv"` claim) and checked on every request in `get_current_user`/`get_optional_user`, bumped on both endpoints — change-password hands back a fresh token so the caller's own session keeps working, the admin reset does not 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 fixes). Suite grew from 139 to 192 tests over the nine. +B-28/B-29/B-30/B-31/B-32/B-33/B-34 fixes). Suite grew from 139 to 194 tests over the ten. 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, diff --git a/app/api/routes/admin.py b/app/api/routes/admin.py index d4f036c..246b624 100644 --- a/app/api/routes/admin.py +++ b/app/api/routes/admin.py @@ -215,6 +215,11 @@ async def reset_user_password( new_password = secrets.token_urlsafe(12) user.password_hash = hash_password(new_password) + # B-34: this endpoint exists precisely for the "account compromised" case — + # without bumping token_version, whoever was already logged in (the + # attacker, if that's who prompted the reset) stayed logged in on their + # existing token until it naturally expired, unaffected by the reset. + user.token_version += 1 await write_audit_log(session, "admin_password_reset", {"user_id": user_id}, user_id=user_id) await session.commit() return AdminPasswordResetResponse(username=user.username, new_password=new_password) diff --git a/app/api/routes/users.py b/app/api/routes/users.py index f467f92..8188569 100644 --- a/app/api/routes/users.py +++ b/app/api/routes/users.py @@ -5,7 +5,7 @@ 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 MIN_PASSWORD_LENGTH, hash_password, verify_password +from app.auth.security import MIN_PASSWORD_LENGTH, create_access_token, 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 @@ -45,12 +45,16 @@ class ChangePasswordRequest(BaseModel): new_password: str -@router.post("/me/change-password", status_code=status.HTTP_204_NO_CONTENT) +class ChangePasswordResponse(BaseModel): + access_token: str + + +@router.post("/me/change-password", response_model=ChangePasswordResponse) async def change_password( body: ChangePasswordRequest, user: User = Depends(get_current_user), session: AsyncSession = Depends(get_session), -) -> None: +) -> ChangePasswordResponse: """Self-service password change — requires the current password, unlike the admin-only /admin/users/{id}/reset-password (which is for a user who's actually locked out and can't provide it).""" @@ -67,7 +71,15 @@ async def change_password( ) user.password_hash = hash_password(body.new_password) + # B-34: bumping token_version invalidates every token issued before this + # point — including this very request's own bearer token, and any an + # attacker who knew the old password might be holding. A fresh token is + # handed back so *this* session keeps working without forcing a re-login; + # every other open session (this user's other devices, or an attacker's) + # gets "session_expired" on its next request. + user.token_version += 1 await session.commit() + return ChangePasswordResponse(access_token=create_access_token(user.id, user.token_version)) class LastRoundResultResponse(BaseModel): diff --git a/app/auth/dependencies.py b/app/auth/dependencies.py index 04503f4..a44a15a 100644 --- a/app/auth/dependencies.py +++ b/app/auth/dependencies.py @@ -16,13 +16,19 @@ async def get_current_user( session: AsyncSession = Depends(get_session), ) -> User: try: - user_id = decode_access_token(credentials.credentials) + user_id, token_version = decode_access_token(credentials.credentials) except Exception as exc: raise http_error(status.HTTP_401_UNAUTHORIZED, "session_expired", "invalid token") from exc user = await session.scalar(select(User).where(User.id == user_id)) if user is None: raise http_error(status.HTTP_401_UNAUTHORIZED, "session_expired", "user not found") + if user.token_version != token_version: + # B-34: a password change (self-service or admin reset) bumps + # token_version, so a token issued before it — including one an + # attacker who had the old password is still holding — reads as + # expired rather than staying valid until it naturally times out. + raise http_error(status.HTTP_401_UNAUTHORIZED, "session_expired", "token has been superseded") return user @@ -37,7 +43,10 @@ async def get_optional_user( if not auth_header.startswith("Bearer "): return None try: - user_id = decode_access_token(auth_header.removeprefix("Bearer ")) + user_id, token_version = decode_access_token(auth_header.removeprefix("Bearer ")) except Exception: return None - return await session.scalar(select(User).where(User.id == user_id)) + user = await session.scalar(select(User).where(User.id == user_id)) + if user is None or user.token_version != token_version: + return None + return user diff --git a/app/auth/routes.py b/app/auth/routes.py index 3abfbf4..8564e29 100644 --- a/app/auth/routes.py +++ b/app/auth/routes.py @@ -111,7 +111,9 @@ async def register( continue await session.refresh(user) request.app.state.electrum_listener.address_for_new_user(user.id, user.address) - return TokenResponse(access_token=create_access_token(user.id), address=user.address) + return TokenResponse( + access_token=create_access_token(user.id, user.token_version), address=user.address + ) raise http_error( status.HTTP_409_CONFLICT, @@ -148,4 +150,6 @@ async def login( # on its own, so one correct login can't be used to wipe out an IP's failure # count while it's mid-attack against other accounts. limiters.login.record_success(username_key) - return TokenResponse(access_token=create_access_token(user.id), address=user.address) + return TokenResponse( + access_token=create_access_token(user.id, user.token_version), address=user.address + ) diff --git a/app/auth/security.py b/app/auth/security.py index 30deb23..6b83de4 100644 --- a/app/auth/security.py +++ b/app/auth/security.py @@ -39,12 +39,20 @@ def verify_password(password: str, password_hash: str) -> bool: return False -def create_access_token(user_id: int) -> str: +def create_access_token(user_id: int, token_version: int = 0) -> str: expires_at = datetime.now(timezone.utc) + timedelta(minutes=settings.jwt_expire_minutes) - payload = {"sub": str(user_id), "exp": expires_at} + # "tv" lets get_current_user (app/auth/dependencies.py) reject a token issued + # before the account's password was last changed (B-34): change-password and + # the admin reset both bump User.token_version, so every token that still + # carries the old value stops working immediately instead of staying valid + # for up to jwt_expire_minutes after a compromise is supposedly handled. + payload = {"sub": str(user_id), "tv": token_version, "exp": expires_at} return jwt.encode(payload, settings.jwt_secret, algorithm=settings.jwt_algorithm) -def decode_access_token(token: str) -> int: +def decode_access_token(token: str) -> tuple[int, int]: payload = jwt.decode(token, settings.jwt_secret, algorithms=[settings.jwt_algorithm]) - return int(payload["sub"]) + # .get(..., 0) covers tokens issued before "tv" existed (pre-B-34 deploy) — + # they carry no claim at all, and 0 is what a freshly migrated user's + # token_version starts at, so those sessions keep working across the deploy. + return int(payload["sub"]), int(payload.get("tv", 0)) diff --git a/app/db/models.py b/app/db/models.py index 2ceb766..47c4d7c 100644 --- a/app/db/models.py +++ b/app/db/models.py @@ -21,6 +21,12 @@ class User(Base): # Read cache only; must always be written in the same transaction as the # utxo_events rows it summarizes. Source of truth is utxo_events. cached_balance_sats: Mapped[int] = mapped_column(BigInteger, default=0) + # Embedded in every issued JWT (app/auth/security.py) and checked on every + # request (app/auth/dependencies.py:get_current_user). Bumped on a + # self-service or admin password change so every token issued before that + # point stops working immediately, instead of staying valid for up to + # jwt_expire_minutes after a compromised account's password is reset (B-34). + token_version: Mapped[int] = mapped_column(default=0, server_default="0") created_at: Mapped[datetime] = mapped_column(default=utcnow) diff --git a/app/static/app.js b/app/static/app.js index 3bba472..de2ee3a 100644 --- a/app/static/app.js +++ b/app/static/app.js @@ -749,10 +749,15 @@ async function changePassword() { await withLoading(btn, t('loading.updating'), async () => { try { - await call('POST', '/users/me/change-password', { + const data = await call('POST', '/users/me/change-password', { current_password: currentPassword, new_password: newPassword, }); + // The server just invalidated every previously issued token (B-34) — + // including the one this very request was authenticated with — and + // handed back a fresh one so this tab doesn't get logged out too. + token = data.access_token; + localStorage.setItem('plm_token', token); document.getElementById('settings-current-password').value = ''; document.getElementById('settings-new-password').value = ''; document.getElementById('settings-new-password-confirm').value = ''; diff --git a/migrations/versions/943dbd74d983_add_token_version_to_users.py b/migrations/versions/943dbd74d983_add_token_version_to_users.py new file mode 100644 index 0000000..7f9063c --- /dev/null +++ b/migrations/versions/943dbd74d983_add_token_version_to_users.py @@ -0,0 +1,40 @@ +"""add token_version to users + +Revision ID: 943dbd74d983 +Revises: 861e76aaf34c +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 = '943dbd74d983' +down_revision: Union[str, Sequence[str], None] = '861e76aaf34c' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + # server_default backfills every existing user to 0 (their current sessions + # stay valid, since 0 also matches what already-issued tokens carry + # implicitly — see the "sub"-only tokens issued before this migration); + # dropped right after so new rows go through the ORM default instead of a + # stale constant. + op.add_column( + 'users', sa.Column('token_version', sa.Integer(), nullable=False, server_default='0') + ) + with op.batch_alter_table('users') as batch_op: + batch_op.alter_column('token_version', server_default=None) + # ### end Alembic commands ### + + +def downgrade() -> None: + """Downgrade schema.""" + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column('users', 'token_version') + # ### end Alembic commands ### diff --git a/tests/unit/test_admin.py b/tests/unit/test_admin.py index 6690027..bdf8b95 100644 --- a/tests/unit/test_admin.py +++ b/tests/unit/test_admin.py @@ -205,6 +205,10 @@ async def test_admin_resets_user_password(client): assert refreshed.password_hash != old_hash assert verify_password(new_password, refreshed.password_hash) assert not verify_password("original-password", refreshed.password_hash) + # B-34: the reset must bump token_version so a session opened before + # the reset (e.g. an attacker who had the old password) is evicted + # immediately rather than staying valid until the JWT naturally expires. + assert refreshed.token_version == 1 async def test_admin_reset_password_requires_token(client): diff --git a/tests/unit/test_security.py b/tests/unit/test_security.py index 67291a5..fa2b073 100644 --- a/tests/unit/test_security.py +++ b/tests/unit/test_security.py @@ -9,8 +9,21 @@ def test_password_hash_roundtrip(): 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 + token = security.create_access_token(user_id=42, token_version=3) + assert security.decode_access_token(token) == (42, 3) + + +def test_jwt_decode_defaults_token_version_for_tokens_issued_before_it_existed(monkeypatch): + """B-34: a token minted before the "tv" claim existed has no such key at + all. It must still decode — as token_version 0, matching a freshly + migrated user's starting value — rather than raising or being treated as + permanently stale.""" + import jwt as pyjwt + + monkeypatch.setattr(security.settings, "jwt_secret", "test-secret") + payload = {"sub": "42"} + token = pyjwt.encode(payload, "test-secret", algorithm=security.settings.jwt_algorithm) + assert security.decode_access_token(token) == (42, 0) def test_verify_password_returns_false_for_an_unparseable_hash(): diff --git a/tests/unit/test_users.py b/tests/unit/test_users.py index 994469d..475474a 100644 --- a/tests/unit/test_users.py +++ b/tests/unit/test_users.py @@ -81,7 +81,8 @@ async def test_change_password_updates_login(client): headers=headers, json={"current_password": "original-password", "new_password": "brand-new-password"}, ) - assert resp.status_code == 204 + assert resp.status_code == 200 + assert resp.json()["access_token"] resp = await client.post("/auth/login", json={"username": "alice", "password": "original-password"}) assert resp.status_code == 401 @@ -90,6 +91,36 @@ async def test_change_password_updates_login(client): assert resp.status_code == 200 +async def test_change_password_invalidates_the_old_token_but_not_the_new_one(client): + """B-34: neither self-service change-password nor the admin reset used to + invalidate already-issued JWTs, so a stolen token (or an attacker who + already had the old password) stayed logged in until the token's natural + 24h expiry — even past a password change meant to lock them out.""" + old_token = await _register(client) + old_headers = {"Authorization": f"Bearer {old_token}"} + + resp = await client.post( + "/users/me/change-password", + headers=old_headers, + json={"current_password": "original-password", "new_password": "brand-new-password"}, + ) + assert resp.status_code == 200 + new_token = resp.json()["access_token"] + assert new_token != old_token + + # The old token (what an attacker holding the old password would still + # have) is now rejected... + resp = await client.get("/users/me", headers=old_headers) + assert resp.status_code == 401 + assert resp.json()["detail"]["code"] == "session_expired" + + # ...but the freshly issued one keeps this same session working, so the + # user who just changed their own password isn't logged out too. + new_headers = {"Authorization": f"Bearer {new_token}"} + resp = await client.get("/users/me", headers=new_headers) + assert resp.status_code == 200 + + async def test_change_password_rejects_too_short(client): token = await _register(client) headers = {"Authorization": f"Bearer {token}"}