fix(auth): increment attempt counters atomically #474

Merged

Claude-Run merged 1 commit from fix/PMS-693-atomic-mfa-attempt-counter into main

2026-08-01 04:41:54 +02:00

Claude-Run commented

2026-08-01 04:31:48 +02:00

Member

The MFA lockout read mfa_failed_attempts in one transaction and wrote back prior_count + 1 as an absolute value in another. Under READ COMMITTED a burst of concurrent logins all read the same pre-burst value, so 500 wrong second-factor codes left the column at 1 and mfa_lockout_until never armed: the attacker got a whole burst of TOTP guesses per round trip instead of two. register_failed_mfa now takes no prior_count and its UPDATE increments the column relatively, deriving the new lock window from the post-increment value inside the same statement, which makes the stale read unrepresentable.

The anti-replay watermark had the same read-then-write shape: two concurrent logins presenting the same valid code both saw mfa_last_used_step < step and both succeeded. record_mfa_success now carries the comparison in its WHERE clause and returns whether a row moved; a zero-row update is treated as a replay and falls through to the failure branch.

The lock schedule now exists twice, as mfa_lockout_until in Rust and as the SQL expression mfa_lock_seconds_sql that the UPDATE evaluates, so a DB-backed parity test pins the two together across the whole documented table (failed_count 1..=12).

Invariant sweep over every persistent attempt counter: portal::service::register_failed_login (contacts.portal_failed_login_count) had the identical defect and gets the identical fix plus its own schedule parity test; verify_login_approval read login_approvals.attempts and wrote back attempts + 1, so concurrent guesses against an emailed approval code collapsed into one tick and the 5-attempt cap never bit, now a relative UPDATE ... RETURNING drives the destroy decision. notifications::worker claims rows with FOR UPDATE SKIP LOCKED (single writer, compliant) and knowledge_base recomputes its vote tallies from the votes table rather than incrementing (not applicable).

New pins in tests/auth.rs: 20 concurrent logins with distinct wrong codes leave mfa_failed_attempts >= 20 and the next attempt is RateLimited (pre-fix the counter reads 1); two concurrent logins with the same valid TOTP code yield exactly one success (pre-fix both succeeded). Both drive AuthService::login directly because the router's 5/min per-email limiter would reject most of the burst before it reached the MFA branch. A source-grep unit test fails if either register_failed_* regrows a count parameter or any counter write assigns a bind placeholder instead of incrementing the column.

#PMS-693

The MFA lockout read `mfa_failed_attempts` in one transaction and wrote back `prior_count + 1` as an absolute value in another. Under READ COMMITTED a burst of concurrent logins all read the same pre-burst value, so 500 wrong second-factor codes left the column at 1 and `mfa_lockout_until` never armed: the attacker got a whole burst of TOTP guesses per round trip instead of two. `register_failed_mfa` now takes no `prior_count` and its UPDATE increments the column relatively, deriving the new lock window from the post-increment value inside the same statement, which makes the stale read unrepresentable. The anti-replay watermark had the same read-then-write shape: two concurrent logins presenting the same valid code both saw `mfa_last_used_step < step` and both succeeded. `record_mfa_success` now carries the comparison in its WHERE clause and returns whether a row moved; a zero-row update is treated as a replay and falls through to the failure branch. The lock schedule now exists twice, as `mfa_lockout_until` in Rust and as the SQL expression `mfa_lock_seconds_sql` that the UPDATE evaluates, so a DB-backed parity test pins the two together across the whole documented table (failed_count 1..=12). Invariant sweep over every persistent attempt counter: `portal::service::register_failed_login` (`contacts.portal_failed_login_count`) had the identical defect and gets the identical fix plus its own schedule parity test; `verify_login_approval` read `login_approvals.attempts` and wrote back `attempts + 1`, so concurrent guesses against an emailed approval code collapsed into one tick and the 5-attempt cap never bit, now a relative UPDATE ... RETURNING drives the destroy decision. `notifications::worker` claims rows with FOR UPDATE SKIP LOCKED (single writer, compliant) and `knowledge_base` recomputes its vote tallies from the votes table rather than incrementing (not applicable). New pins in tests/auth.rs: 20 concurrent logins with distinct wrong codes leave `mfa_failed_attempts >= 20` and the next attempt is RateLimited (pre-fix the counter reads 1); two concurrent logins with the same valid TOTP code yield exactly one success (pre-fix both succeeded). Both drive `AuthService::login` directly because the router's 5/min per-email limiter would reject most of the burst before it reached the MFA branch. A source-grep unit test fails if either `register_failed_*` regrows a count parameter or any counter write assigns a bind placeholder instead of incrementing the column. #PMS-693

Claude-Run added 1 commit

2026-08-01 04:31:49 +02:00

fix(auth): increment attempt counters atomically

E2E / Playwright against staging (pull_request) Successful in 48s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 1m54s

Details

Integration / integration tests (pull_request) Successful in 9m32s

Details

Create release / Gate (release-branch merges only) (pull_request) Successful in 1s

Details

Create release / Create release from merged PR (pull_request) Has been skipped

Details

caeba42199

The MFA lockout read `mfa_failed_attempts` in one transaction and wrote back `prior_count + 1` as an absolute value in another. Under READ COMMITTED a burst of concurrent logins all read the same pre-burst value, so 500 wrong second-factor codes left the column at 1 and `mfa_lockout_until` never armed: the attacker got a whole burst of TOTP guesses per round trip instead of two. `register_failed_mfa` now takes no `prior_count` and its UPDATE increments the column relatively, deriving the new lock window from the post-increment value inside the same statement, which makes the stale read unrepresentable.

The anti-replay watermark had the same read-then-write shape: two concurrent logins presenting the same valid code both saw `mfa_last_used_step < step` and both succeeded. `record_mfa_success` now carries the comparison in its WHERE clause and returns whether a row moved; a zero-row update is treated as a replay and falls through to the failure branch.

The lock schedule now exists twice, as `mfa_lockout_until` in Rust and as the SQL expression `mfa_lock_seconds_sql` that the UPDATE evaluates, so a DB-backed parity test pins the two together across the whole documented table (failed_count 1..=12).

Invariant sweep over every persistent attempt counter: `portal::service::register_failed_login` (`contacts.portal_failed_login_count`) had the identical defect and gets the identical fix plus its own schedule parity test; `verify_login_approval` read `login_approvals.attempts` and wrote back `attempts + 1`, so concurrent guesses against an emailed approval code collapsed into one tick and the 5-attempt cap never bit, now a relative UPDATE ... RETURNING drives the destroy decision. `notifications::worker` claims rows with FOR UPDATE SKIP LOCKED (single writer, compliant) and `knowledge_base` recomputes its vote tallies from the votes table rather than incrementing (not applicable).

New pins in tests/auth.rs: 20 concurrent logins with distinct wrong codes leave `mfa_failed_attempts >= 20` and the next attempt is RateLimited (pre-fix the counter reads 1); two concurrent logins with the same valid TOTP code yield exactly one success (pre-fix both succeeded). Both drive `AuthService::login` directly because the router's 5/min per-email limiter would reject most of the burst before it reached the MFA branch. A source-grep unit test fails if either `register_failed_*` regrows a count parameter or any counter write assigns a bind placeholder instead of incrementing the column.

#PMS-693