fix(auth): make login-approval consume atomic to close a TOCTOU (LC-601) #557

Merged

nrupard merged 1 commit from fix/LC-601-login-approval-consume-toctou into main

2026-07-19 02:59:15 +02:00

nrupard commented

2026-07-19 02:43:52 +02:00

Owner

Summary

Closes a TOCTOU in the LC-587 suspicious-login approval flow found in post-merge review.

login_approval::verify read the pending challenge (SELECT ... WHERE consumed_at IS NULL), compared the submitted code, then consumed the row with an unconditional UPDATE whose result was ignored. Two concurrent correct-code submits (a double-clicked Approve, two tabs) could both pass the read and the comparison and both mint a session, breaking the single-use invariant.

Fix

db::auth::consume_login_approval guards its UPDATE with AND consumed_at IS NULL and returns the rows affected, making it the atomic winner-take-all gate.
verify mints a session only when its own consume reported one row (Ok(1)); a racing second submit consumes zero rows and gets Invalid instead of a second session.

Impact / severity

Low: the trigger requires the valid emailed code, so it is a double-submit by the legitimate user, not an attacker bypass. But it violated the single-use invariant (two sessions from one challenge), so it is worth fixing.

Tests

New db_login_approval::consume_is_conditional_single_winner: first consume returns 1, a second returns 0, and a verify with the correct code after the row is consumed is Invalid. Green on fmt + clippy -D warnings (standalone + saas, all targets) + the login-approval tests.

#LC-601

## Summary Closes a TOCTOU in the LC-587 suspicious-login approval flow found in post-merge review. `login_approval::verify` read the pending challenge (SELECT ... WHERE `consumed_at IS NULL`), compared the submitted code, then consumed the row with an unconditional UPDATE whose result was ignored. Two concurrent correct-code submits (a double-clicked Approve, two tabs) could both pass the read and the comparison and both mint a session, breaking the single-use invariant. ## Fix - `db::auth::consume_login_approval` guards its UPDATE with `AND consumed_at IS NULL` and returns the rows affected, making it the atomic winner-take-all gate. - `verify` mints a session only when its own consume reported one row (`Ok(1)`); a racing second submit consumes zero rows and gets `Invalid` instead of a second session. ## Impact / severity Low: the trigger requires the *valid* emailed code, so it is a double-submit by the legitimate user, not an attacker bypass. But it violated the single-use invariant (two sessions from one challenge), so it is worth fixing. ## Tests New `db_login_approval::consume_is_conditional_single_winner`: first consume returns 1, a second returns 0, and a `verify` with the correct code after the row is consumed is `Invalid`. Green on `fmt` + `clippy -D warnings` (standalone + saas, all targets) + the login-approval tests. #LC-601

nrupard added 1 commit

2026-07-19 02:43:52 +02:00

fix(auth): make login-approval consume atomic to close a TOCTOU (LC-601)

check-secrets / TruffleHog (push) Successful in 4s

Details

check-secrets / Kingfisher (pull_request) Successful in 6s

Details

check-secrets / TruffleHog (pull_request) Successful in 7s

Details

check-secrets / Kingfisher (push) Successful in 5s

Details

check-secrets / Nosey parker (push) Successful in 6s

Details

check-secrets / Nosey parker (pull_request) Successful in 6s

Details

Check / clippy + fmt + tests (pull_request) Successful in 4m5s

Details

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

Details

b1ce19e7f2

LC-587's `login_approval::verify` read the pending challenge (SELECT ... WHERE consumed_at IS NULL), compared the submitted code, then consumed the row unconditionally and ignored the result. Two concurrent correct-code submits (e.g. a double-clicked Approve, or two tabs) could both pass the read and the code comparison and both mint a session, breaking the single-use invariant.

`consume_login_approval` now guards its UPDATE with `AND consumed_at IS NULL` and returns the number of rows affected, making it the atomic winner-take-all gate. `verify` mints a session only when its own consume reported one row (Ok(1)); a racing second submit consumes zero rows and gets Invalid instead of a second session.

A new `db_login_approval::consume_is_conditional_single_winner` test asserts the first consume returns 1, a second returns 0, and a `verify` with the correct code after the row is consumed is Invalid. Green on fmt + clippy -D warnings across both the standalone and saas feature sets, plus the login-approval tests.

#LC-601

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>