fix(auth): count a failed recovery code against MFA lockout

Claude-Run commented

2026-08-01 06:58:35 +02:00

Member

The recovery-code branch of the login second-factor block returned Unauthorized on a miss without calling register_failed_mfa, so mfa_failed_attempts never moved and mfa_locked_until never armed. An attacker holding the password got unlimited second-factor guesses simply by sending recovery_code instead of mfa_code, and because nothing else arms the counter, that also left the lockout inert for the TOTP path.

The !removed branch now feeds the same per-account counter the TOTP branch does. Symmetrically, an accepted recovery code clears the counter and the lockout via a new clear_mfa_lockout helper, so a user who locked themselves out of TOTP and then used a recovery code is not still serving the remaining backoff. record_mfa_success is deliberately not reused there: its compare-and-set advances mfa_last_used_step, and a recovery code is not a TOTP step, so the anti-replay watermark must stay put.

Sweep of every second-factor gate: the two login branches are the only ones. enable_mfa verifies an enrollment code on an already-authenticated session before mfa_enabled is set, disable_mfa re-auths with a password only, and the portal path has no MFA at all.

Tests: failed_recovery_codes_lock_account pins three wrong recovery codes to mfa_failed_attempts = 3 with an armed window and the 4th attempt refused as RateLimited; recovery_code_success_clears_mfa_counters pins a valid code succeeding with two failures banked, leaving the counter at 0, the lockout NULL and mfa_last_used_step untouched. Both fail without the change.

The PMS-625 audit doc's F5 row is updated to record the finding as tracked here rather than as an open follow-up.

#PMS-694

The recovery-code branch of the login second-factor block returned `Unauthorized` on a miss without calling `register_failed_mfa`, so `mfa_failed_attempts` never moved and `mfa_locked_until` never armed. An attacker holding the password got unlimited second-factor guesses simply by sending `recovery_code` instead of `mfa_code`, and because nothing else arms the counter, that also left the lockout inert for the TOTP path. The `!removed` branch now feeds the same per-account counter the TOTP branch does. Symmetrically, an accepted recovery code clears the counter and the lockout via a new `clear_mfa_lockout` helper, so a user who locked themselves out of TOTP and then used a recovery code is not still serving the remaining backoff. `record_mfa_success` is deliberately not reused there: its compare-and-set advances `mfa_last_used_step`, and a recovery code is not a TOTP step, so the anti-replay watermark must stay put. Sweep of every second-factor gate: the two login branches are the only ones. `enable_mfa` verifies an enrollment code on an already-authenticated session before `mfa_enabled` is set, `disable_mfa` re-auths with a password only, and the portal path has no MFA at all. Tests: `failed_recovery_codes_lock_account` pins three wrong recovery codes to `mfa_failed_attempts = 3` with an armed window and the 4th attempt refused as `RateLimited`; `recovery_code_success_clears_mfa_counters` pins a valid code succeeding with two failures banked, leaving the counter at 0, the lockout NULL and `mfa_last_used_step` untouched. Both fail without the change. The PMS-625 audit doc's F5 row is updated to record the finding as tracked here rather than as an open follow-up. #PMS-694

Claude-Run added 1 commit

2026-08-01 06:58:35 +02:00

fix(auth): count a failed recovery code against MFA lockout

E2E / Playwright against staging (pull_request) Successful in 1m13s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 3m59s

Details

Integration / integration tests (pull_request) Successful in 13m7s

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

54ba114bb0

The recovery-code branch of the login second-factor block returned `Unauthorized` on a miss without calling `register_failed_mfa`, so `mfa_failed_attempts` never moved and `mfa_locked_until` never armed. An attacker holding the password got unlimited second-factor guesses simply by sending `recovery_code` instead of `mfa_code`, and because nothing else arms the counter, that also left the lockout inert for the TOTP path.

The `!removed` branch now feeds the same per-account counter the TOTP branch does. Symmetrically, an accepted recovery code clears the counter and the lockout via a new `clear_mfa_lockout` helper, so a user who locked themselves out of TOTP and then used a recovery code is not still serving the remaining backoff. `record_mfa_success` is deliberately not reused there: its compare-and-set advances `mfa_last_used_step`, and a recovery code is not a TOTP step, so the anti-replay watermark must stay put.

Sweep of every second-factor gate: the two login branches are the only ones. `enable_mfa` verifies an enrollment code on an already-authenticated session before `mfa_enabled` is set, `disable_mfa` re-auths with a password only, and the portal path has no MFA at all.

Tests: `failed_recovery_codes_lock_account` pins three wrong recovery codes to `mfa_failed_attempts = 3` with an armed window and the 4th attempt refused as `RateLimited`; `recovery_code_success_clears_mfa_counters` pins a valid code succeeding with two failures banked, leaving the counter at 0, the lockout NULL and `mfa_last_used_step` untouched. Both fail without the change.

The PMS-625 audit doc's F5 row is updated to record the finding as tracked here rather than as an open follow-up.

#PMS-694

Claude-Run merged commit 955e15169b into main

2026-08-01 07:12:43 +02:00

Claude-Run deleted branch fix/PMS-694-recovery-code-mfa-lockout

2026-08-01 07:12:43 +02:00

Claude-Run referenced this pull request from a commit

2026-08-01 07:12:43 +02:00

Merge pull request 'fix(auth): count a failed recovery code against MFA lockout' (#478) from fix/PMS-694-recovery-code-mfa-lockout into main

Rows
Columns

fix(auth): count a failed recovery code against MFA lockout #478