fix(security): make account-lockout failure counter atomic (#324)#337
fix(security): make account-lockout failure counter atomic (#324)#337dmytrocraft wants to merge 4 commits into
Conversation
The per-account failed-login counter in RedisAccountLockoutProvider used a non-atomic PSR-6 read-modify-write (getItem()->get(), +1 in PHP, set()/save()). Concurrent failed-login requests for the same email read the same starting value and each persisted value+1, so N parallel requests advanced the counter by far fewer than N (lost-update race, CWE-362). Because this email-keyed lockout (20 attempts / 1h) is the primary per-account brute-force control on the GraphQL signIn path, the undercount let an attacker exceed the 20-guess cap by firing concurrent requests before the lock was ever applied. Fix: replace the read-modify-write with a single server-side Lua script via Redis::eval that INCRs the attempts key, sets the window TTL on first increment, and atomically SETs the lock key once the threshold is crossed — mirroring the established atomic pattern in RedisOAuthStateRepository. The provider now binds the existing app.account_lockout_redis_connection \Redis service directly; the orphaned cache.account_lockout PSR-6 pool is removed. isLocked() uses EXISTS, clearFailures() uses DEL. Behat lockout helpers now operate on the same raw connection (DB 0 is already flushed per scenario). Local verification: - phpunit Unit (Lockout filter): OK (11 tests, 24 assertions) - phpunit full Unit: OK (2261 tests, 6212 assertions) - deptrac: Errors 0 - psalm (changed files): No errors found - php-cs-fixer (changed files): 0 of 4 files BMAD spec: specs/security-324-account-lockout-counter-race/ Closes #324 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
More reviews will be available in 2 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Confidence score: 2/5
- There is a high regression risk in
src/User/Infrastructure/Provider/RedisAccountLockoutProvider.php: lockout state is only written whenattempts == threshold, so once the lock expires, subsequent failures above threshold may no longer persist lockout across requests. recordFailure()appears to fail open insrc/User/Infrastructure/Provider/RedisAccountLockoutProvider.phpby treating Redisevalreturningfalseas0(unlocked), which can silently disable lockout enforcement when the script errors.- Given the high-severity, user-facing auth protection impact (sev 8/10 and 7/10 with moderate-high confidence), this is better treated as high merge risk until corrected.
- Pay close attention to
src/User/Infrastructure/Provider/RedisAccountLockoutProvider.php- lock persistence and fail-open error handling can weaken account lockout protection.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/User/Infrastructure/Provider/RedisAccountLockoutProvider.php">
<violation number="1" location="src/User/Infrastructure/Provider/RedisAccountLockoutProvider.php:38">
P1: Lock key is only set at `attempts == threshold`, so after lock expiry further over-threshold failures do not recreate the lock and lockout is no longer persisted between requests.</violation>
<violation number="2" location="src/User/Infrastructure/Provider/RedisAccountLockoutProvider.php:71">
P1: `recordFailure()` fails open: when Redis `eval` returns `false`, it is cast to `0` and treated as unlocked, silently disabling lockout enforcement on script errors.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as GraphQL Client
participant API as SignIn API Handler
participant Validator as UserCredentialValidator
participant Provider as RedisAccountLockoutProvider
participant Redis as Redis Server
Note over Client,Redis: Atomic Account Lockout Flow
Client->>API: signIn mutation
API->>Validator: validateCredentials(email, password)
alt Account locked (lock key exists)
Validator->>Provider: isLocked(email)
Provider->>Redis: EXISTS signin_lock_<sha256(email)>
Redis-->>Provider: 1
Provider-->>Validator: true
Validator-->>API: LockedHttpException
API-->>Client: 403 Locked
else Account not locked
Validator->>Provider: isLocked(email)
Provider->>Redis: EXISTS signin_lock_<sha256(email)>
Redis-->>Provider: 0
Provider-->>Validator: false
Validator->>Validator: Verify password
alt Password incorrect
Validator->>Provider: recordFailure(email)
Note over Provider,Redis: NEW: Atomic Lua script (single round trip)
Provider->>Redis: eval(RECORD_FAILURE_LUA_SCRIPT, KEYS[1]=signin_lockout_<sha256>, KEYS[2]=signin_lock_<sha256>, ARGV[1]=3600, ARGV[2]=20, ARGV[3]=900, 2)
Note over Redis: Lua script executes atomically:
Note over Redis: 1. INCR attempts counter
Note over Redis: 2. EXPIRE window on first increment
Note over Redis: 3. IF attempts >= 20, SET lock key with 900s TTL
Note over Redis: 4. Return 1 if locked, 0 otherwise
Redis-->>Provider: 0 (below threshold) or 1 (locked)
alt Lock threshold reached (returns 1)
Provider-->>Validator: true
Validator-->>API: LockedHttpException
API-->>Client: 403 Locked
else Threshold not reached (returns 0)
Provider-->>Validator: false
Validator-->>API: UnauthorizedHttpException
API-->>Client: 401 Unauthorized
end
else Password correct
Validator->>Provider: clearFailures(email)
Provider->>Redis: DEL signin_lockout_<sha256>, signin_lock_<sha256>
Redis-->>Provider: OK
Provider-->>Validator: void
Validator-->>API: Success
API-->>Client: 200 Authenticated
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 551 552 +1
Lines 9541 9543 +2
=========================================
+ Hits 9541 9543 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Kill the escaped CastInt mutant on RedisAccountLockoutProvider::recordFailure
by asserting the integer cast normalizes numeric-string Redis eval replies
("1" -> locked, "0" -> not locked) before the strict locked comparison.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
Recreate the account-lockout lock on every at-or-above-threshold failure so the lockout is re-persisted once the shorter lock TTL expires while the longer attempt window is still open. Fail closed when the Redis eval returns false, matching the OAuth state repository convention, instead of silently reporting "not locked" and disabling brute-force enforcement on a storage error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add a unit test for AccountLockoutStorageException asserting getMessage() equals the storage-unavailable message, killing the escaped MethodCallRemoval mutant on the parent::__construct() call and restoring Infection MSI to 100%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dmytrocraft
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
4c19479
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Security fix — Closes #324
Vulnerability (CWE-362, race condition)
RedisAccountLockoutProvider::recordFailure()implemented the per-account failed-login counter as a non-atomic PSR-6 read-modify-write:getItem()->get(),+1in PHP, thenset()/save(). Concurrent failed-login requests for the same email read the same starting value and each persistedvalue + 1, so N parallel requests advanced the counter by far fewer than N (lost-update race).Because this email-keyed lockout (20 attempts / 1h) is the primary per-account brute-force control on the GraphQL
signInpath (where thesignin_email/signin_iplimiters are bypassed), the undercount let an attacker exceed the intended 20-guess cap by firing concurrent requests before the lock was ever applied.Fix
Redis::evalthatINCRs the attempts key, sets the 3600s window TTL on the first increment, and atomicallySETs the lock key (900s TTL) once the threshold is crossed — mirroring the established atomic pattern inRedisOAuthStateRepository::validateAndConsume().app.account_lockout_redis_connection\Redisservice directly into the provider; remove the now-orphanedcache.account_lockoutPSR-6 RedisAdapter.isLocked()usesEXISTS,clearFailures()usesDEL. Behat lockout helpers now operate on the same raw connection (test DB 0 is already flushed before each scenario).KEYS/ARGVare passed (no user input in the script body). Public interface signatures, key names, and 20/900 semantics are unchanged.Local verification (one-off containers)
OK (11 tests, 24 assertions)OK (2261 tests, 6212 assertions)Errors 0No errors found!Found 0 of 4 filesA new regression test (
testRecordFailureUsesSingleAtomicEvalWithoutReadModifyWrite) asserts the script usesINCR/EXPIRE, binds both keys, and that the provider never performs a PHP-sideget/set/incr— it fails against the old read-modify-write implementation and passes with the atomic fix.BMAD spec
specs/security-324-account-lockout-counter-race/(prd.md,stories.md)🤖 Generated with Claude Code
Summary by cubic
Fixes the race condition in account lockout by making the failure counter atomic and ensuring the lock is applied reliably under concurrency. Also refreshes the lock on every over‑threshold failure and fails closed on Redis errors. Closes #324.
Redis::evalLua script thatINCRs attempts, sets the 1h TTL on first increment, andSETs/refreshes the 900s lock whenever attempts ≥ 20 (atomic; thresholds unchanged).evalreplies to integers and now throwAccountLockoutStorageExceptionwhenevalreturnsfalse; added tests for numeric-string casting, the atomic path, lock reapplication, fail-closed behavior, plus a dedicatedAccountLockoutStorageExceptionmessage test.App\User\Infrastructure\Provider\RedisAccountLockoutProviderto@app.account_lockout_redis_connection(\Redis) and updated Behat helpers to use the same raw keys; removedcache.account_lockout.Written for commit 4c19479. Summary will update on new commits.