Skip to content

fix(auth): accept a TOTP code only once (#849) - #851

Merged
alex-dembele merged 2 commits into
masterfrom
849-securityauth-a-totp-code-can-be-replayed-within-its-validity-window
Oct 2, 2026
Merged

alex-dembele merged 2 commits into
masterfrom
849-securityauth-a-totp-code-can-be-replayed-within-its-validity-window

Conversation

@alex-dembele

Copy link
Copy Markdown
Member

Closes #849

A TOTP code used to be accepted as many times as it was typed within its ±30 s window (about 90 s). Now each code is accepted once, everywhere a code is checked: login challenge, enrolment verification, turning MFA off (SSO accounts, #754), and the step-up gate (authmfa.Gate).

Stacked on #848 (base: 754-…), because the disable path for SSO accounts only exists there. Once #848 is merged, GitHub retargets this PR to master.

How it works

  • otp.MatchTOTPStep(secret, code, now) returns the time step the code belongs to (±1 step, constant-time comparison).
  • mfa_secrets.last_totp_step (bigint, nullable) stores the step of the last accepted code. It's added by AutoMigrate and by migration 0066. The down removes nothing, on purpose: dropping the column would reopen the replay, and the CLAUDE.md rule on dropping columns applies.
  • GormMFARepository.ConsumeTOTPStep does one conditional UPDATE … WHERE user_id = ? AND tenant_id = ? AND (last_totp_step IS NULL OR last_totp_step < ?). One row changed means accepted; zero rows means a replay. Postgres serialises the row, so two concurrent requests can't both win. The same UPDATE sets last_used_at, which was previously written but never read.
  • UpdateMFASecret now leaves out last_totp_step. Without that, a Save of a stale struct after a challenge would lower the mark and reopen the replay.
  • A replayed code gets exactly the same response as a wrong code, so it never tells an observer the code was once valid.

Side effect: the code typed to finish enrolment can't be reused to sign in within the same 30 s; the next code is needed. That's the behaviour RFC 6238 §5.2 recommends.

Verification

$ go test ./pkg/otp/ -run MatchTOTPStep -v                       # ±1 step, ±2 refused, bad input
--- PASS: TestMatchTOTPStep (5 subtests)

$ go test ./internal/application/auth/ -run 'Replay|EarlierStep|NewerCode|EnrolmentCode' -v
--- PASS: TestChallengeMFA_ReplayedCodeIsRefused
--- PASS: TestChallengeMFA_CodeFromAnEarlierStepIsRefused
--- PASS: TestChallengeMFA_ANewerCodeIsStillAccepted
--- PASS: TestVerifyMFA_EnrolmentCodeCannotThenOpenALogin
--- PASS: TestDisableMFA_ReplayedCodeIsRefused
  (with ConsumeTOTPStep neutralised: 4 FAIL — the "newer code" test passes both ways, as it should)

$ go test ./internal/infrastructure/authmfa/ -v                  # real GORM repository on SQLite
--- PASS: TestGate_ReplayedCodeIsRefused
--- PASS: TestGate_CodeIsScopedToItsTenant

$ DATABASE_URL=<postgres:16-alpine, 16.15> go test ./internal/infrastructure/repository/ -run ConsumeTOTPStep -v
--- PASS: TestGormMFARepository_ConsumeTOTPStep_ConcurrentReplay_Postgres
  20 concurrent uses of one code → exactly 1 accepted; earlier step refused; later step accepted;
  another tenant touches nothing; a stale UpdateMFASecret does not lower the mark.
  Without the WHERE clause → FAIL "exactly one of 20…"; without Omit → FAIL "must leave last_totp_step alone".

$ DATABASE_URL=<pg16> go test ./... -count=1        → no failures; 8 Postgres-gated tests PASS

Migration 0066 on a fresh PG 16 database: up applied twice (idempotent, NOTICE on the second run), down ran without error, and the column is bigint.

Live run (branch server on PostgreSQL 16 + Redis 7, fresh database):

first_use          200  token_pair issued
replay_same_code   400  {'error': 'invalid MFA code'}   ← same response as a wrong code
previous_step_code 400  {'error': 'invalid MFA code'}
mfa_secrets.last_totp_step = 59691900 (= current step), last_used_at set

Not done

  • Backup codes are unchanged. They were already single-use (MarkBackupCodeAsUsed), and criterion 4 asks for no change.
  • The mark is per secret, not per session. That's intended: a code seen on one screen can't be used on any other.

🤖 Generated with Claude Code

VerifyTOTP answers yes or no, so a caller cannot tell a fresh code from one it
already accepted. MatchTOTPStep returns the matching step within the same ±1
tolerance, comparing in constant time, so the caller can refuse a replay.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
A code could be used again for about 90 s after it was typed: to open a second
session, confirm a second sensitive action, or turn MFA off. Each secret now
records the step of the last accepted code (mfa_secrets.last_totp_step, added
by AutoMigrate and migration 0066), and a code is accepted only for a later
step, in one conditional UPDATE so concurrent requests cannot both win. Login,
enrolment, the MFA disable and the step-up gate all go through it; a replay is
refused exactly like a wrong code. Saves of the secret no longer write the
column, so a stale struct cannot lower the mark.

Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Base automatically changed from 754-securityauth-disabling-mfa-does-not-re-verify-the-password to master October 1, 2026 12:21
@alex-dembele alex-dembele self-assigned this Oct 2, 2026
@alex-dembele alex-dembele reopened this Oct 2, 2026
@alex-dembele
alex-dembele merged commit d3c66de into master Oct 2, 2026
13 of 26 checks passed
@alex-dembele
alex-dembele deleted the 849-securityauth-a-totp-code-can-be-replayed-within-its-validity-window branch October 2, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

security(auth): a TOTP code can be replayed within its validity window

1 participant