Invalidate existing sessions on password change/reset (B-34)

Neither self-service password change nor the admin reset invalidated
already-issued JWTs — a 24h-lifetime token stayed valid regardless, so
a stolen token (or an attacker who already had the old password) kept
working past a password change meant to lock them out. The admin reset
exists precisely for the "account compromised" case and didn't evict
the attacker at all.

Add User.token_version (migration 943dbd74d983), embedded in every JWT
as a "tv" claim and checked against the DB on every request in
get_current_user/get_optional_user; a mismatch reads as session_expired.
Both change-password and the admin reset bump it. change-password hands
back a freshly minted token so the caller's own session keeps working
instead of being logged out by its own request; the admin reset does
not, since that session isn't the one making the call.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
2026-07-27 12:02:23 +02:00
co-authored by Claude Sonnet 5
parent 16802cafb6
commit 739fc9fed2
12 changed files with 159 additions and 36 deletions
+6 -20
View File
@@ -1,11 +1,11 @@
# Known bugs # Known bugs
A second full-codebase audit on 2026-07-27 found **25 further issues** (4 critical, 6 high, 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 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 16 are High/Medium/Low. 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 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 coverage — every fix lands with a regression test (the ten fixes so far brought the suite
from 139 to 192). from 139 to 194).
The recurring pattern across the open findings is worth stating once: the code is rigorous 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. 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 ## Medium
### B-35 — Every API timestamp is naive, so the frontend renders it in the wrong timezone ### 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-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-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-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 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 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, module under `app/`, both static frontends, and the Docker/Caddy deployment — 5 critical,
+5
View File
@@ -215,6 +215,11 @@ async def reset_user_password(
new_password = secrets.token_urlsafe(12) new_password = secrets.token_urlsafe(12)
user.password_hash = hash_password(new_password) 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 write_audit_log(session, "admin_password_reset", {"user_id": user_id}, user_id=user_id)
await session.commit() await session.commit()
return AdminPasswordResetResponse(username=user.username, new_password=new_password) return AdminPasswordResetResponse(username=user.username, new_password=new_password)
+15 -3
View File
@@ -5,7 +5,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
from app.api.errors import http_error from app.api.errors import http_error
from app.auth.dependencies import get_current_user 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.models import Round, RoundParticipant, User
from app.db.session import get_session from app.db.session import get_session
from app.wallet.balance import compute_pending_balance from app.wallet.balance import compute_pending_balance
@@ -45,12 +45,16 @@ class ChangePasswordRequest(BaseModel):
new_password: str 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( async def change_password(
body: ChangePasswordRequest, body: ChangePasswordRequest,
user: User = Depends(get_current_user), user: User = Depends(get_current_user),
session: AsyncSession = Depends(get_session), session: AsyncSession = Depends(get_session),
) -> None: ) -> ChangePasswordResponse:
"""Self-service password change — requires the current password, unlike the """Self-service password change — requires the current password, unlike the
admin-only /admin/users/{id}/reset-password (which is for a user who's admin-only /admin/users/{id}/reset-password (which is for a user who's
actually locked out and can't provide it).""" actually locked out and can't provide it)."""
@@ -67,7 +71,15 @@ async def change_password(
) )
user.password_hash = hash_password(body.new_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() await session.commit()
return ChangePasswordResponse(access_token=create_access_token(user.id, user.token_version))
class LastRoundResultResponse(BaseModel): class LastRoundResultResponse(BaseModel):
+12 -3
View File
@@ -16,13 +16,19 @@ async def get_current_user(
session: AsyncSession = Depends(get_session), session: AsyncSession = Depends(get_session),
) -> User: ) -> User:
try: try:
user_id = decode_access_token(credentials.credentials) user_id, token_version = decode_access_token(credentials.credentials)
except Exception as exc: except Exception as exc:
raise http_error(status.HTTP_401_UNAUTHORIZED, "session_expired", "invalid token") from 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)) user = await session.scalar(select(User).where(User.id == user_id))
if user is None: if user is None:
raise http_error(status.HTTP_401_UNAUTHORIZED, "session_expired", "user not found") 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 return user
@@ -37,7 +43,10 @@ async def get_optional_user(
if not auth_header.startswith("Bearer "): if not auth_header.startswith("Bearer "):
return None return None
try: try:
user_id = decode_access_token(auth_header.removeprefix("Bearer ")) user_id, token_version = decode_access_token(auth_header.removeprefix("Bearer "))
except Exception: except Exception:
return None 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
+6 -2
View File
@@ -111,7 +111,9 @@ async def register(
continue continue
await session.refresh(user) await session.refresh(user)
request.app.state.electrum_listener.address_for_new_user(user.id, user.address) 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( raise http_error(
status.HTTP_409_CONFLICT, 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 # 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. # count while it's mid-attack against other accounts.
limiters.login.record_success(username_key) 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
)
+12 -4
View File
@@ -39,12 +39,20 @@ def verify_password(password: str, password_hash: str) -> bool:
return False 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) 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) 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]) 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))
+6
View File
@@ -21,6 +21,12 @@ class User(Base):
# Read cache only; must always be written in the same transaction as the # Read cache only; must always be written in the same transaction as the
# utxo_events rows it summarizes. Source of truth is utxo_events. # utxo_events rows it summarizes. Source of truth is utxo_events.
cached_balance_sats: Mapped[int] = mapped_column(BigInteger, default=0) 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) created_at: Mapped[datetime] = mapped_column(default=utcnow)
+6 -1
View File
@@ -749,10 +749,15 @@ async function changePassword() {
await withLoading(btn, t('loading.updating'), async () => { await withLoading(btn, t('loading.updating'), async () => {
try { try {
await call('POST', '/users/me/change-password', { const data = await call('POST', '/users/me/change-password', {
current_password: currentPassword, current_password: currentPassword,
new_password: newPassword, 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-current-password').value = '';
document.getElementById('settings-new-password').value = ''; document.getElementById('settings-new-password').value = '';
document.getElementById('settings-new-password-confirm').value = ''; document.getElementById('settings-new-password-confirm').value = '';
@@ -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 ###
+4
View File
@@ -205,6 +205,10 @@ async def test_admin_resets_user_password(client):
assert refreshed.password_hash != old_hash assert refreshed.password_hash != old_hash
assert verify_password(new_password, refreshed.password_hash) assert verify_password(new_password, refreshed.password_hash)
assert not verify_password("original-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): async def test_admin_reset_password_requires_token(client):
+15 -2
View File
@@ -9,8 +9,21 @@ def test_password_hash_roundtrip():
def test_jwt_roundtrip(monkeypatch): def test_jwt_roundtrip(monkeypatch):
monkeypatch.setattr(security.settings, "jwt_secret", "test-secret") monkeypatch.setattr(security.settings, "jwt_secret", "test-secret")
token = security.create_access_token(user_id=42) token = security.create_access_token(user_id=42, token_version=3)
assert security.decode_access_token(token) == 42 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(): def test_verify_password_returns_false_for_an_unparseable_hash():
+32 -1
View File
@@ -81,7 +81,8 @@ async def test_change_password_updates_login(client):
headers=headers, headers=headers,
json={"current_password": "original-password", "new_password": "brand-new-password"}, 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"}) resp = await client.post("/auth/login", json={"username": "alice", "password": "original-password"})
assert resp.status_code == 401 assert resp.status_code == 401
@@ -90,6 +91,36 @@ async def test_change_password_updates_login(client):
assert resp.status_code == 200 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): async def test_change_password_rejects_too_short(client):
token = await _register(client) token = await _register(client)
headers = {"Authorization": f"Bearer {token}"} headers = {"Authorization": f"Bearer {token}"}