domain: login lockout uses a stale whole-row read-modify-write, racing the counter and clobbering profile edits #56

New issue

Closed

opened 2026-07-03 01:41:46 +00:00 by rosa · 1 comment

rosa commented

2026-07-03 01:41:46 +00:00

Owner

Found during a code review of the domain crate.

Location: crates/domain/src/services.rs:221 (authenticate)
Severity: Security / Correctness — concurrency

Problem

authenticate atomically increments the failed-attempt counter, then writes a whole-row update_user built from the pre-increment user snapshot to set/clear the lock. The increment and the lock write are separate non-atomic calls, and update_user overwrites the whole row (including name/email/biography and the counter).

Failure scenario

A user at 9 failed attempts gets two concurrent wrong-password requests: both increment (DB to 10 then 11), both build the update from a stale clone and write it; interleavings can leave the account unlocked with the counter reset, weakening brute-force protection. Separately, the full-row write on any login can revert a name/email/bio edit made in parallel.

Suggested fix

Move the lockout decision onto the model (e.g. User::record_failed_login(now) -> LockoutDecision) and expose narrow repo ops (record_failed_login/clear_lockout) that update only the lock columns, instead of round-tripping the whole row.

Found during a code review of the `domain` crate. **Location:** `crates/domain/src/services.rs:221` (`authenticate`) **Severity:** Security / Correctness — concurrency ## Problem `authenticate` atomically increments the failed-attempt counter, then writes a whole-row `update_user` built from the pre-increment `user` snapshot to set/clear the lock. The increment and the lock write are separate non-atomic calls, and `update_user` overwrites the whole row (including name/email/biography and the counter). ## Failure scenario A user at 9 failed attempts gets two concurrent wrong-password requests: both increment (DB to 10 then 11), both build the update from a stale clone and write it; interleavings can leave the account unlocked with the counter reset, weakening brute-force protection. Separately, the full-row write on any login can revert a name/email/bio edit made in parallel. ## Suggested fix Move the lockout decision onto the model (e.g. `User::record_failed_login(now) -> LockoutDecision`) and expose narrow repo ops (`record_failed_login`/`clear_lockout`) that update only the lock columns, instead of round-tripping the whole row.

rosa added the

needs-triage

label

2026-07-03 01:43:55 +00:00

rosa commented

2026-07-04 03:26:20 +00:00

Author

Owner

Agent Brief

Category: bug
Summary: Lockout state becomes writable only through two narrow atomic repository operations, ending the read-modify-write races in authenticate

Design decisions were settled in a triage session and recorded in ADR 0011 (lockout state is written only by narrow atomic repo ops) and the Authentication section of CONTEXT.md (Lockout, Record). Read both before starting; the ADR also records the rejected alternatives, so don't re-litigate them.

Current behavior:
authenticate increments the failed-login counter atomically, but decides whether to lock from the pre-increment User snapshot and writes the result back through the whole-row update_user. Two consequences:

Concurrent failed logins around the threshold can interleave so the account ends unlocked with the counter reset, weakening brute-force protection.
Every login's whole-row write can silently revert a concurrent profile (name/email/biography) edit — and profile saves can likewise clobber in-flight lockout state, since update_user_settings and confirm_email also round-trip the lock columns from stale snapshots.

Additionally, confirm_email clears the lockout as an unintended side effect, and the attempt that crosses the threshold reports IncorrectPassword rather than the lock it just created.

Desired behavior:
The two lock columns (failed_login_attempts, locked_until) are written by exactly two repository operations and nothing else:

Record a failed login — one atomic statement (single SQL UPDATE in the Postgres adapter) that increments the counter and, when the post-increment count reaches the policy threshold, zeroes the counter and sets locked_until = now + duration in the same statement. There must be no observable state where the count has moved but the lock decision hasn't. It reports what happened.
Clear the lockout — zeroes the counter and nulls locked_until, called unconditionally on every successful login. Idempotent.

authenticate uses these instead of its two whole-row writes. When recording reports that this attempt entered the Lockout, authenticate returns UserLocked(until) immediately (behavior change: previously the crossing attempt read as one more wrong password). The pre-verification "is the user currently locked" gate stays as-is.

confirm_email stops touching lockout state entirely — it only sets the email-confirmed timestamp (deliberate behavior change per ADR 0011).

Key interfaces:

LockoutPolicy — new domain type alongside User: threshold (attempts) and duration, with a Default of 10 attempts / 15 minutes. The Service holds one (initialized to default in its constructor) so tests can inject a small threshold instead of scripting ten failures. No app-config surface.
LockoutOutcome — what recording returns: either not-locked carrying the fresh attempt count, or locked carrying the expiry instant. (Named Outcome, not Decision — the store reports, the domain doesn't decide from a snapshot.)
AppRepository::record_failed_login(user_id, &LockoutPolicy) -> LockoutOutcome and AppRepository::clear_lockout(user_id) — the only writers of lock columns. The existing increment_login_attempts port op is subsumed and should be removed.
UpdateUserRequest — loses failed_login_attempts and locked_until entirely; update_user's SQL stops setting those columns. The type system, not discipline, is what prevents profile writes from clobbering lockout state.
Both adapters (the Postgres repository and the in-memory test repository) implement the new ops with identical semantics.

Acceptance criteria:

A lockout repository conformance suite exists in the established both-adapters pattern (generic behavior functions run against Postgres via sqlx::test and the memory repo via tokio::test), covering:
- recording below the threshold increments and reports the fresh count, without locking
- the recording that reaches the threshold reports locked with an expiry ≈ now + duration, and leaves the counter at zero
- clearing zeroes the counter and lifts the lock; clearing an untouched user is a no-op
- a profile update via update_user leaves an existing count and lock unchanged
Service-level authenticate tests (mock repo) cover: the crossing attempt returns UserLocked(until); an already-locked user is refused before password verification; a successful login clears the lockout; confirm_email leaves lockout state untouched
UpdateUserRequest no longer has lock-column fields (compile-time guarantee)
The ci mise task passes

Out of scope:

Making LockoutPolicy operator-configurable (env/app config)
A concurrency/race integration test (atomicity is by single-statement construction, pinned by the conformance suite)
Narrowing the remaining profile columns of update_user (profile-vs-profile write races are a separate concern)
Admin unlock, lockout notifications, or any early-lift mechanism — a Lockout expires on its own by definition (see CONTEXT.md)

## Agent Brief **Category:** bug **Summary:** Lockout state becomes writable only through two narrow atomic repository operations, ending the read-modify-write races in `authenticate` Design decisions were settled in a triage session and recorded in **ADR 0011** (lockout state is written only by narrow atomic repo ops) and the **Authentication section of CONTEXT.md** (Lockout, Record). Read both before starting; the ADR also records the rejected alternatives, so don't re-litigate them. **Current behavior:** `authenticate` increments the failed-login counter atomically, but decides whether to lock from the pre-increment `User` snapshot and writes the result back through the whole-row `update_user`. Two consequences: 1. Concurrent failed logins around the threshold can interleave so the account ends unlocked with the counter reset, weakening brute-force protection. 2. Every login's whole-row write can silently revert a concurrent profile (name/email/biography) edit — and profile saves can likewise clobber in-flight lockout state, since `update_user_settings` and `confirm_email` also round-trip the lock columns from stale snapshots. Additionally, `confirm_email` clears the lockout as an unintended side effect, and the attempt that crosses the threshold reports `IncorrectPassword` rather than the lock it just created. **Desired behavior:** The two lock columns (`failed_login_attempts`, `locked_until`) are written by exactly two repository operations and nothing else: - **Record a failed login** — one atomic statement (single SQL `UPDATE` in the Postgres adapter) that increments the counter and, when the post-increment count reaches the policy threshold, zeroes the counter and sets `locked_until = now + duration` *in the same statement*. There must be no observable state where the count has moved but the lock decision hasn't. It reports what happened. - **Clear the lockout** — zeroes the counter and nulls `locked_until`, called unconditionally on every successful login. Idempotent. `authenticate` uses these instead of its two whole-row writes. When recording reports that this attempt entered the Lockout, `authenticate` returns `UserLocked(until)` immediately (behavior change: previously the crossing attempt read as one more wrong password). The pre-verification "is the user currently locked" gate stays as-is. `confirm_email` stops touching lockout state entirely — it only sets the email-confirmed timestamp (deliberate behavior change per ADR 0011). **Key interfaces:** - `LockoutPolicy` — new domain type alongside `User`: `threshold` (attempts) and `duration`, with a `Default` of 10 attempts / 15 minutes. The `Service` holds one (initialized to default in its constructor) so tests can inject a small threshold instead of scripting ten failures. No app-config surface. - `LockoutOutcome` — what recording returns: either not-locked carrying the fresh attempt count, or locked carrying the expiry instant. (Named *Outcome*, not *Decision* — the store reports, the domain doesn't decide from a snapshot.) - `AppRepository::record_failed_login(user_id, &LockoutPolicy) -> LockoutOutcome` and `AppRepository::clear_lockout(user_id)` — the only writers of lock columns. The existing `increment_login_attempts` port op is subsumed and should be removed. - `UpdateUserRequest` — loses `failed_login_attempts` and `locked_until` entirely; `update_user`'s SQL stops setting those columns. The type system, not discipline, is what prevents profile writes from clobbering lockout state. - Both adapters (the Postgres repository and the in-memory test repository) implement the new ops with identical semantics. **Acceptance criteria:** - [ ] A lockout repository conformance suite exists in the established both-adapters pattern (generic behavior functions run against Postgres via `sqlx::test` and the memory repo via `tokio::test`), covering: - [ ] recording below the threshold increments and reports the fresh count, without locking - [ ] the recording that reaches the threshold reports locked with an expiry ≈ now + duration, and leaves the counter at zero - [ ] clearing zeroes the counter and lifts the lock; clearing an untouched user is a no-op - [ ] a profile update via `update_user` leaves an existing count and lock unchanged - [ ] Service-level `authenticate` tests (mock repo) cover: the crossing attempt returns `UserLocked(until)`; an already-locked user is refused before password verification; a successful login clears the lockout; `confirm_email` leaves lockout state untouched - [ ] `UpdateUserRequest` no longer has lock-column fields (compile-time guarantee) - [ ] The `ci` mise task passes **Out of scope:** - Making `LockoutPolicy` operator-configurable (env/app config) - A concurrency/race integration test (atomicity is by single-statement construction, pinned by the conformance suite) - Narrowing the remaining profile columns of `update_user` (profile-vs-profile write races are a separate concern) - Admin unlock, lockout notifications, or any early-lift mechanism — a Lockout expires on its own by definition (see CONTEXT.md)

rosa added

bug

ready-for-agent

and removed

needs-triage

labels

2026-07-04 03:26:27 +00:00

rosa referenced this issue from a commit

2026-07-04 03:28:12 +00:00

domain: Anchor the lockout vocabulary and record ADR 0011

rosa referenced this issue from a commit

2026-07-04 03:48:33 +00:00