[Tim020]

Summary

• Failed logins are now throttled in memory per client address (20 free attempts) and per submitted username (5 free attempts), whether or not the username exists, with exponential lockout (30s doubling, capped at 15 min) answered with 429 and a Retry-After header. Successful login clears the username counter. Tables are size-bounded and decay; state lives on the app instance (injectable clock) so it is test/xdist safe.
• Unknown usernames now run a bcrypt check against a dummy hash so response time does not reveal which accounts exist.
• Login no longer holds a per-username lock across the bcrypt check; NamedLockRegistry now discards locks once idle so it cannot grow from user-supplied names.
• Minimum password length raised from 6 to 8 (server validation, both client/ and client-v3/ validators and hints); admin-reset temporary passwords now use 4 words.
• First-run / user creation (/api/v1/auth/create and /api/v2/users) runs under a single lock with the has-any-users check inside it, so concurrent first-run requests cannot create two admins. A one-time setup token was not added to keep this PR contained; the residual risk (anyone who can reach a fresh server first can claim the admin account) is documented in the getting-started docs.
• Docs updated (getting started, user config).

Tests

• New backend tests: throttle unit tests (fake clock), 429/Retry-After flow, unknown-user throttling, dummy-hash path, min length, 4-word temp passwords, concurrent first-run on both endpoints, lock pruning. Existing tests updated for the new minimum.
• Playwright: added throttle and short-password checks to the auth spec; full chromium suite passes (227). Client lint/typecheck/vitest pass for both clients; ruff and full pytest pass.

🤖 Generated with Claude Code

[github-actions]

Client V3 Test Results

227 tests   227 ✅  1s ⏱️
 20 suites    0 💤
  1 files      0 ❌

Results for commit 179f818.

♻️ This comment has been updated with latest results.

[github-actions]

Client Test Results

102 tests   102 ✅  0s ⏱️
  9 suites    0 💤
  1 files      0 ❌

Results for commit 179f818.

♻️ This comment has been updated with latest results.

[github-actions]

Python Test Results

    1 files      1 suites   1m 0s ⏱️
1 183 tests 1 183 ✅ 0 💤 0 ❌
1 206 runs  1 206 ✅ 0 💤 0 ❌

Results for commit 179f818.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (firefox)

227 tests   227 ✅  2m 21s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 179f818.

♻️ This comment has been updated with latest results.

[github-actions]

Playwright E2E Results (chromium)

227 tests   227 ✅  2m 27s ⏱️
 14 suites    0 💤
  1 files      0 ❌

Results for commit 179f818.

♻️ This comment has been updated with latest results.

[Tim020]

Review round 1

Head SHA reviewed: b0be93fdbcf5f8e4e7faf23cba3dec12f2563f9f

Verdict: changes needed. This is a well-structured hardening PR with good test infrastructure (fake clock, genuine concurrency tests, injectable state), but two of the core security properties it claims to deliver don't fully hold under the conditions that matter most — concurrent requests and sustained attack — and one existing test is masking exactly that.

SonarQube

Quality Gate: PASSED. All conditions green (reliability/security/maintainability ratings A, duplication 2.2% < 3% threshold, security hotspots 100% reviewed — of zero hotspots). new_coverage is not a gate condition for this project and new_lines_to_cover = 0, so coverage didn't factor in. 0 open issues, 0 hotspots. Nothing Sonar-side blocks this PR.

New findings (round 1, first pass — no prior threads)

• Critical: 2 — a check-then-act race lets concurrent login attempts bypass the per-IP/per-username throttle entirely, and the decay window exactly equals the max lockout so the counter silently resets the moment a 15-minute lockout ends (both independently confirmed by multiple review passes, one via empirical fake-clock simulation, one via mutation testing on the test suite).
• Important: 3 — the test meant to verify the lockout cap is vacuous (passes even with the cap deleted); IP-level lockout refuses correct passwords too, and the docs' remedy for shared-IP-behind-a-proxy doesn't exist in the code; the cross-endpoint admin-creation lock name is duplicated as a bare string literal in two files with no test exercising contention between them.
• Suggestion: 5 — unsynchronized dummy-hash lazy-init (wasteful + reopens the timing gap briefly on cold start), NamedLockRegistry.get_lock() as an untested bypass around the new refcounting, a docstring off-by-one plus missing input validation on _Tracker, missing :param: fields on two methods, and a pre-existing bytes-vs-characters message mismatch in the password length error that the PR's doc edits didn't catch.

Could not be cleanly anchored to a diff line (noted here instead):

• No unit tests on either client (client/, client-v3/) for the new 8-character password minimum validator — only server-side and one E2E test that calls the API directly, bypassing the client-side validator entirely.
• The dummy-hash test (test_unknown_user_still_runs_a_bcrypt_verification) asserts the dummy-hash path is called, not that it does the same amount of work as the real-user path — low priority, timing-based assertions are flaky in CI.
• The 429 response body's message field format has no contract test (only status code + Retry-After header are checked).
• Worth considering for later: an admin "clear throttle" action, since today the only way to clear a maliciously-triggered per-username lockout during a show is a server restart.

Strengths

• NamedLockRegistry's ref-count-before-await pattern for pruning idle locks is correct and well-tested for its blessed entry point (acquire()), with concurrency tests that genuinely use asyncio.gather, not sequential calls dressed up as concurrent.
• LoginThrottle's fake-clock test design is clean and correctly keeps all state off any module-level singleton, which is exactly what pytest-xdist safety requires.
• The first-run admin-race fix itself (single lock around check-and-create) is correct for the common case, and the getting-started docs accurately disclose the residual risk the PR description calls out.
• Good feature parity between client/ and client-v3/ for the password-length bump, and SQLAlchemy 2.0 select() usage throughout.

🤖 Generated with Claude Code

[Tim020]

Review round 1 - response (head 88d4436)

Inline threads are answered individually. Summary of the points that were not inline:

• Client unit tests for the 8-character minimum: added client-v3/src/composables/usePasswordValidation.test.ts and client/src/mixins/passwordValidation.test.ts.
• 429 body contract: the throttle test now also asserts the message text.
• Dummy-hash work-equivalence test: declined. Timing assertions are flaky in CI; the test keeps asserting that the dummy path runs, and the first-call cost is now structurally the same (one bcrypt run).
• Admin "clear throttle" action: declined for this PR (new endpoint/UI surface). The docs now say a restart clears lockouts. Worth a follow-up if a victim lockout during a show turns out to matter.
• Known limitations kept deliberately: a victim can be briefly locked out by failed logins as them; behind a reverse proxy all clients share one address counter (documented more plainly now); the old-password check on change-password is not throttled; first-run claim relies on the documented residual risk (no setup token).

Changes: atomic reserve-then-settle in LoginThrottle (fixes the concurrent-burst bypass), decay measured from the end of the lockout, shared USER_CREATE_LOCK, constructor validation, docstrings, tests. Backend pytest 1181 passed, ruff clean, both clients lint/typecheck/vitest pass, full chromium Playwright 227 passed.

🤖 Generated with Claude Code

[Tim020]

Review round 2

Head SHA reviewed: 88d4436188ae190065ef20b8de3269055cfcf78e

Verdict: changes needed — but close. Every round-1 finding is resolved or reasonably declined; one new Important-severity regression was introduced by the fix itself and is worth addressing before merge, since it affects the same lockout logic round 1 already flagged twice.

Previous round follow-up

• Verified fixed (6): TOCTOU race (atomic begin_attempt, confirmed by hand and by a genuine concurrent-HTTP test), decay/cap bug (_expired() now measured from the later of last-failure/lockout-end), the vacuous test_lockout_is_capped test (now a real regression test), the duplicated USER_CREATE_LOCK (hoisted to named_locks.py + new cross-endpoint test), the _Tracker docstring off-by-one plus missing constructor validation, and the missing :param: docstrings.
• Accepted — fix + reasonable decline (4): the proxy/IP-lockout docs wording is now accurate; declined adding xheaders/checking-password-before-lockout, both with sound security reasoning (a spoofable header would let an attacker rotate addresses past the per-IP limit; checking the password first would spend bcrypt on locked-out clients and let a guess succeed mid-lockout). The dummy-hash cold-start cost is fixed (first call is now a single bcrypt run, matching later calls); declined eager startup warm-up as not worth the per-test-run cost. get_lock() now has a docstring marking it internal; declined the extra paused-waiter test since the described race can't occur with the current ref-before-lookup ordering. The bytes-vs-characters wording issue is accepted as pre-existing and deferred to a follow-up (touches ~8 strings across both clients).
• Needs human decision: none — all responses were either clearly correct fixes or well-reasoned declines.

SonarQube

Quality Gate: PASSED, confirmed fresh against head 88d4436. All 5 new-code conditions green (reliability/security/maintainability A, duplication 1.7% < 3%, hotspots 100% of zero). 0 open issues, 0 unreviewed hotspots.

New findings (round 2, scoped to the diff since last round)

• Important: 1 — the new reserve-then-settle mechanism (begin_attempt/refund) that correctly fixes the TOCTOU race has its own regression: a successful login from an address with accumulated failures can escalate that address's lockout instead of leaving it untouched, and ongoing legitimate logins from a shared address can prevent its old failure history from ever decaying. Empirically confirmed by running the real LoginThrottle class with a fake clock (see inline comment for the exact repro and numbers).
• Noted, not posted as separate comments (bounded, lower priority): a victim's successful login fully clears their username's failure history, which can also wipe out an attacker's already-reserved-but-not-yet-settled concurrent guesses if the two happen to race within the same bcrypt window — only matters when an attacker is guessing at the exact moment the real user logs in. Separately, because an attempt is now counted as a failure for the whole duration of its bcrypt check rather than instantaneously, two legitimate concurrent requests for the same near-threshold username/IP can transiently 429 each other during that window — an inherent, bounded consequence of reserving pessimistically, not a correctness bug.

Strengths

• The TOCTOU fix is genuinely verified under real concurrency, not just simulated: test_concurrent_wrong_password_burst_is_bounded_by_the_throttle fires 15 real concurrent HTTP requests and asserts the split lands exactly on the free-attempts boundary.
• The decay fix was carefully reasoned through by hand and holds up against hand-traced arithmetic.
• Docs are now accurate about a real limitation (shared IP behind a proxy) rather than implying a fix that doesn't exist.
• Constructor validation and RST docstring completeness are both now in good shape.

🤖 Generated with Claude Code

[Tim020]

Review round 2 - response (head 179f818)

The one inline finding is fixed and its thread resolved (reply has the details). The two non-inline notes are accepted as bounded trade-offs, no change:

• A victim's successful login clears their username counter, which can also drop an attacker's still-pending reservations if both race inside one bcrypt window. Clearing on success is the intended behaviour (a legitimate login should reset the user's lockout), and the attacker still faces the per-IP limit and any later failures.
• Because an attempt is counted while its bcrypt check runs, two legitimate concurrent requests right at the threshold can transiently 429 each other. That is the inherent cost of reserving up front, which is what closes the burst bypass.

Local checks on the new head: pytest 1183 passed, ruff clean, full chromium Playwright 227 passed (no client files changed).

🤖 Generated with Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.6% Duplication on New Code

See analysis details on SonarQube Cloud

[Tim020]

Review round 3

Head SHA reviewed: 179f818193d2ba59a1fd86a309570acd05777a99

Verdict: ready to merge, pending CI. Every finding from rounds 1 and 2 is now resolved. All CI checks are green except E2E Tests (firefox), which has been stuck in_progress for 20+ minutes — this repo has a known history of flaky/slow Firefox E2E runs (see prior fix commits in the log), so this looks like an infrastructure stall rather than a code problem, but I'm not calling it fully green until that run actually completes.

Previous round follow-up

• Verified fixed (1): the round-2 refund() regression (successful logins escalating or preventing decay of an address's lockout) is fixed. begin_attempt now returns a ticket snapshotting the pre-reservation state, and record_success restores it exactly when nothing else touched the entry since. Re-ran both original repros directly against the class: the escalation case now returns 0 instead of 120, and the non-decay case now fully decays instead of persisting indefinitely. Both new tests (test_successful_attempt_does_not_escalate_an_expired_lockout, test_successful_logins_do_not_keep_stale_failures_alive) are genuine, non-vacuous regression tests. Thread resolved (by the author; reply posted confirming).
• Accepted, no change needed (2): the two bounded trade-offs noted in round 2 (a victim's successful login can clear an attacker's concurrently in-flight reservations if they race within one bcrypt window; two legitimate concurrent requests near the threshold can transiently 429 each other) — the author's reasoning in the round-2 response matches what round 2 already called out as inherent to the reserve-up-front design, so no further action.
• Needs human decision: none.

SonarQube

Quality Gate: PASSED, confirmed fresh against head 179f818. All 5 new-code conditions green (reliability/security/maintainability A, duplication 1.6% < 3%, hotspots 100% of zero). 0 open issues, 0 unreviewed hotspots.

New findings (round 3)

• Critical/Important: 0.
• Suggestion: 1 (not posted inline, noted here) — the entry[1] == reserved_at identity check that refund() uses to decide whether to restore the exact pre-reservation state has one residual edge case: if two reservations on the same key settle out of order (the later reservation's login resolves before the earlier one's), the restored last_failure ends up nudged forward by the gap between the two reservations rather than exactly matching the true original timestamp. Traced through several orderings (including same-clock-tick concurrent reservations) and the lockout state itself (locked_until) always comes out correct — never escalated, never held open incorrectly — this only delays when the decay clock resets for the remaining count, bounded by how close together concurrent login requests actually land in practice (milliseconds under normal load). Not worth blocking on; a per-entry generation counter instead of a timestamp would close it exactly if ever revisited.

Strengths

• The fix for the round-2 regression is well-targeted: a dataclass ticket carrying exactly the state needed to undo a reservation, with a sensible fallback (reduce-count-only) when the identity check can't guarantee a clean restore.
• Both new tests target the exact scenarios found, with a fixed fake clock so they're not vacuous.
• Three rounds in, every finding has either been fixed-and-verified or declined with sound reasoning — no outstanding disagreements.

🤖 Generated with Claude Code