fix(2fa): accept the previous TOTP step and consume every code #418

Merged

Claude-Run merged 1 commit from fix/BUNYIP-428-totp-step-window-and-single-use into main

2026-07-31 16:15:25 +02:00

Claude-Run commented

2026-07-31 15:59:17 +02:00

Member

A TOTP code read near the end of a 30s step was rejected: check_code verified with skew = 0, so only the step current at the instant the request reached the server was accepted. The real tolerance was not "30 seconds" but "whatever is left of the current step", which averages 15s and is routinely under 2s once typing and network latency are counted; the reported HAR shows a submission 0.321s past a boundary failing while the next code succeeded. Separately, no verification path recorded that a code had been consumed, so an accepted code stayed valid for the rest of its step and could complete a second login after logout (RFC 6238 section 5.2).

check_code now matches the current step and the immediately preceding one, backward only, and returns the matched step. The tolerance is applied outside TOTP::new because totp_rs's skew is symmetric and would also accept a FUTURE code, letting a client with a fast clock authenticate early. A new migration adds last_used_step BIGINT to user_totp, and every accepted code claims its step through a guarded conditional UPDATE that only fires when the step is strictly newer than the last consumed one; zero rows updated is returned as a plain verification failure, so a replay is indistinguishable from a wrong code (same status, same message, same per-account failure counter) and a concurrent double-submit has exactly one winner. The claim lives in the service, so it covers the login challenge, setup confirm, re-key confirm and every sensitive-op step-up without touching a handler.

Widening the window to two live codes takes the per-guess probability from 1e-6 to 2e-6, still negligible against the unchanged 5-failures-per-900s per-account cap, and single use strictly reduces the surface relative to today, where an observed code stayed replayable for the rest of its step.

Unit tests cover previous-step acceptance, two-steps-ago and future-step rejection, and replay at the same step and across the accepted pair. A new env-gated integration test proves the real SQL guard against Postgres, including the concurrent case. e2e/lib/login.ts drops the "sleep when under 5s remain" workaround and instead resubmits once after waiting out the step, which is what a single-use collision between parallel workers on the shared E2E account now looks like.

#BUNYIP-428

A TOTP code read near the end of a 30s step was rejected: `check_code` verified with `skew = 0`, so only the step current at the instant the request reached the server was accepted. The real tolerance was not "30 seconds" but "whatever is left of the current step", which averages 15s and is routinely under 2s once typing and network latency are counted; the reported HAR shows a submission 0.321s past a boundary failing while the next code succeeded. Separately, no verification path recorded that a code had been consumed, so an accepted code stayed valid for the rest of its step and could complete a second login after logout (RFC 6238 section 5.2). `check_code` now matches the current step and the immediately preceding one, backward only, and returns the matched step. The tolerance is applied outside `TOTP::new` because totp_rs's `skew` is symmetric and would also accept a FUTURE code, letting a client with a fast clock authenticate early. A new migration adds `last_used_step BIGINT` to `user_totp`, and every accepted code claims its step through a guarded conditional UPDATE that only fires when the step is strictly newer than the last consumed one; zero rows updated is returned as a plain verification failure, so a replay is indistinguishable from a wrong code (same status, same message, same per-account failure counter) and a concurrent double-submit has exactly one winner. The claim lives in the service, so it covers the login challenge, setup confirm, re-key confirm and every sensitive-op step-up without touching a handler. Widening the window to two live codes takes the per-guess probability from 1e-6 to 2e-6, still negligible against the unchanged 5-failures-per-900s per-account cap, and single use strictly reduces the surface relative to today, where an observed code stayed replayable for the rest of its step. Unit tests cover previous-step acceptance, two-steps-ago and future-step rejection, and replay at the same step and across the accepted pair. A new env-gated integration test proves the real SQL guard against Postgres, including the concurrent case. `e2e/lib/login.ts` drops the "sleep when under 5s remain" workaround and instead resubmits once after waiting out the step, which is what a single-use collision between parallel workers on the shared E2E account now looks like. #BUNYIP-428

Claude-Run added 1 commit

2026-07-31 15:59:17 +02:00

fix(2fa): accept the previous TOTP step and consume every code

E2E / Playwright against deployment (pull_request) Successful in 1m56s

Details

Check / fmt + clippy + build + tests (pull_request) Successful in 15m26s

Details

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

Details

4da31a4aa6

A TOTP code read near the end of a 30s step was rejected: `check_code` verified with `skew = 0`, so only the step current at the instant the request reached the server was accepted. The real tolerance was not "30 seconds" but "whatever is left of the current step", which averages 15s and is routinely under 2s once typing and network latency are counted; the reported HAR shows a submission 0.321s past a boundary failing while the next code succeeded. Separately, no verification path recorded that a code had been consumed, so an accepted code stayed valid for the rest of its step and could complete a second login after logout (RFC 6238 section 5.2).

`check_code` now matches the current step and the immediately preceding one, backward only, and returns the matched step. The tolerance is applied outside `TOTP::new` because totp_rs's `skew` is symmetric and would also accept a FUTURE code, letting a client with a fast clock authenticate early. A new migration adds `last_used_step BIGINT` to `user_totp`, and every accepted code claims its step through a guarded conditional UPDATE that only fires when the step is strictly newer than the last consumed one; zero rows updated is returned as a plain verification failure, so a replay is indistinguishable from a wrong code (same status, same message, same per-account failure counter) and a concurrent double-submit has exactly one winner. The claim lives in the service, so it covers the login challenge, setup confirm, re-key confirm and every sensitive-op step-up without touching a handler.

Widening the window to two live codes takes the per-guess probability from 1e-6 to 2e-6, still negligible against the unchanged 5-failures-per-900s per-account cap, and single use strictly reduces the surface relative to today, where an observed code stayed replayable for the rest of its step.

Unit tests cover previous-step acceptance, two-steps-ago and future-step rejection, and replay at the same step and across the accepted pair. A new env-gated integration test proves the real SQL guard against Postgres, including the concurrent case. `e2e/lib/login.ts` drops the "sleep when under 5s remain" workaround and instead resubmits once after waiting out the step, which is what a single-use collision between parallel workers on the shared E2E account now looks like.

#BUNYIP-428