feat: add lockdown mode on multiple login attempts#727
Conversation
📝 WalkthroughWalkthroughIntroduces a global login lockdown in AuthService: when recorded failed attempts reach a configured cap, an asynchronous lockdown activates for a configured timeout, blocking lock checks for all identifiers and clearing stored attempt records until the lockdown expires. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Auth as AuthService
participant Lock as LockdownRoutine
participant Timer as Timer
Client->>Auth: RecordLoginAttempt(identifier, success=false)
Auth->>Auth: append attempt or check count
alt Count >= MaxLoginAttemptRecords
Auth-->>Lock: spawn async lockdownMode()
Lock->>Auth: lock loginMutex
Lock->>Auth: set auth.lockdown.Active = true
Lock->>Auth: set auth.lockdown.ActiveUntil = now + LoginTimeout
Lock->>Auth: clear auth.loginAttempts
Lock->>Auth: unlock loginMutex
Lock->>Timer: wait until ActiveUntil
Timer-->>Lock: timeout
Lock->>Auth: lock loginMutex
Lock->>Auth: set auth.lockdown = nil
Lock->>Auth: unlock loginMutex
else
Auth->>Auth: record attempt normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #727 +/- ##
==========================================
- Coverage 16.92% 16.81% -0.11%
==========================================
Files 50 50
Lines 3806 3829 +23
==========================================
Hits 644 644
- Misses 3098 3121 +23
Partials 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/auth_service.go`:
- Line 28: MaxLoginAttemptRecords is set too low (5) creating a trivial global
DoS; increase this threshold to a much higher value (e.g., 50–100) or replace
the global-count policy with a bounded eviction strategy: implement a FIFO or
LRU queue/map for failed identifiers and cap its size to the new limit so old
entries are evicted rather than triggering a global lockdown for LoginTimeout;
update the logic in the auth locking code that references MaxLoginAttemptRecords
and any routines using LoginTimeout/failed-attempt tracking to use the new
threshold or eviction data structure.
- Around line 244-247: The current check spawns lockdownMode() goroutines
without verifying if a lockdown is already active, allowing races between
concurrent requests; update the branch that checks len(auth.loginAttempts) >=
MaxLoginAttemptRecords to first test whether auth.lockdown is nil (or otherwise
indicates no active lockdown) and only then set/initialize auth.lockdown (or a
boolean) and spawn go auth.lockdownMode(); ensure the check-and-set is done
under the same mutex or lock that protects auth.loginAttempts and auth.lockdown
so the operation is atomic and prevents multiple goroutines from being started.
- Around line 769-795: In lockdownMode, reading auth.lockdown.ActiveUntil after
releasing auth.loginMutex causes a data race and possible nil dereference; fix
by computing and storing the needed duration (or the ActiveUntil time) into a
local variable while holding the lock inside lockdownMode (using auth.loginMutex
and auth.lockdown), then release the lock and create the timer from that local
value; also ensure any subsequent dereferences of auth.lockdown after the timer
waits re-acquire auth.loginMutex and handle a possibly nil auth.lockdown before
accessing fields (use symbols auth.lockdown, auth.loginMutex, lockdownMode, and
ActiveUntil).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d7fc29d-c5dc-4bb1-b7f8-2fa46269e0ca
📒 Files selected for processing (1)
internal/service/auth_service.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/service/auth_service.go (2)
210-217:⚠️ Potential issue | 🟠 MajorReturn global lockdown separately from per-account lock status.
This method now collapses two different states into the same
(locked, remaining)result. The callers ininternal/controller/user_controller.go(Line 65-75) andinternal/middleware/context_middleware.go(Line 151-160) can only emit “account locked” responses/audit events, so a system-wide protection mode will be reported as if the current username exhausted its own retries. Split this into a separate lockdown check or return the lock scope/reason as part of the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` around lines 210 - 217, The IsAccountLocked method currently mixes global lockdown (auth.lockdown) with per-account lock state, causing callers (e.g., user_controller.go and context_middleware.go) to misreport a system-wide lockdown as an account-specific lock; split the concerns by either adding a new method IsGlobalLockdown() that checks auth.lockdown (using auth.loginMutex and Active/ActiveUntil) and returns (bool, remainingSeconds) or change IsAccountLocked to only evaluate per-account locks (leave auth.lockdown out), then update callers to call IsGlobalLockdown() before IsAccountLocked(); ensure both methods use auth.loginMutex for concurrency and return clear separate results.
244-263:⚠️ Potential issue | 🔴 CriticalThe global cap can trip on normal successful traffic.
RecordLoginAttemptis also called on successful auth ininternal/controller/user_controller.go(Line 81-106) andinternal/middleware/context_middleware.go(Line 162-177). Because this method creates aLoginAttemptentry before the success branch resets it,len(auth.loginAttempts)becomes a count of remembered identifiers, not failed identifiers. After enough distinct successful logins, the next request hits the lockdown path even if it is successful or reuses an existing identifier. Only new failed identifiers should count towardMaxLoginAttemptRecords; success should delete/reset an existing entry instead of creating one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` around lines 244 - 263, In RecordLoginAttempt, avoid creating new LoginAttempt entries for successful authentications and ensure the global cap counts only tracked failures: first check the success flag and if true remove any existing entry from auth.loginAttempts (or reset it) and return; only create a new LoginAttempt when success==false. Also ensure the MaxLoginAttemptRecords / lockdown check uses the count of failure entries (auth.loginAttempts) after this change and trigger auth.lockdownMode() as before when the cap is exceeded.
♻️ Duplicate comments (1)
internal/service/auth_service.go (1)
245-248:⚠️ Potential issue | 🔴 CriticalMake lockdown activation atomic and fix the nil guard.
Line 245 can dereference
auth.lockdownwhen it isnil, so the first request that reaches the cap can panic instead of enabling lockdown. Separately,auth.lockdownis only initialized insidelockdownMode(), which leaves a window aftergo auth.lockdownMode()where another request can acquireloginMutexand start a second goroutine before the lockdown state is visible. Setauth.lockdownand clearauth.loginAttemptswhile holdingloginMutexinRecordLoginAttempt, then let the goroutine only wait and clear the lockdown at expiry.Verify the current guard and the delayed initialization with:
#!/bin/bash sed -n '241,249p' internal/service/auth_service.go sed -n '772,786p' internal/service/auth_service.goYou should still see the
auth.lockdown != nil || !auth.lockdown.Activeguard and theauth.lockdown = &Lockdown{...}assignment happening insidelockdownMode().Also applies to: 772-786
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` around lines 245 - 248, The current guard can dereference auth.lockdown and lockdown is only set inside lockdownMode, causing races; to fix, inside RecordLoginAttempt (while holding loginMutex) check if auth.lockdown == nil || !auth.lockdown.Active correctly, and when the cap is reached set auth.lockdown = &Lockdown{Expires: ...} and clear auth.loginAttempts there atomically; then start go auth.lockdownMode() which should only wait for expiry and clear auth.lockdown (do not reassign auth.lockdown inside lockdownMode) so the lockdown state is visible immediately and no second goroutine can be started.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/service/auth_service.go`:
- Around line 210-217: The IsAccountLocked method currently mixes global
lockdown (auth.lockdown) with per-account lock state, causing callers (e.g.,
user_controller.go and context_middleware.go) to misreport a system-wide
lockdown as an account-specific lock; split the concerns by either adding a new
method IsGlobalLockdown() that checks auth.lockdown (using auth.loginMutex and
Active/ActiveUntil) and returns (bool, remainingSeconds) or change
IsAccountLocked to only evaluate per-account locks (leave auth.lockdown out),
then update callers to call IsGlobalLockdown() before IsAccountLocked(); ensure
both methods use auth.loginMutex for concurrency and return clear separate
results.
- Around line 244-263: In RecordLoginAttempt, avoid creating new LoginAttempt
entries for successful authentications and ensure the global cap counts only
tracked failures: first check the success flag and if true remove any existing
entry from auth.loginAttempts (or reset it) and return; only create a new
LoginAttempt when success==false. Also ensure the MaxLoginAttemptRecords /
lockdown check uses the count of failure entries (auth.loginAttempts) after this
change and trigger auth.lockdownMode() as before when the cap is exceeded.
---
Duplicate comments:
In `@internal/service/auth_service.go`:
- Around line 245-248: The current guard can dereference auth.lockdown and
lockdown is only set inside lockdownMode, causing races; to fix, inside
RecordLoginAttempt (while holding loginMutex) check if auth.lockdown == nil ||
!auth.lockdown.Active correctly, and when the cap is reached set auth.lockdown =
&Lockdown{Expires: ...} and clear auth.loginAttempts there atomically; then
start go auth.lockdownMode() which should only wait for expiry and clear
auth.lockdown (do not reassign auth.lockdown inside lockdownMode) so the
lockdown state is visible immediately and no second goroutine can be started.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fe0d92a-d72a-482d-a04e-a87236965eae
📒 Files selected for processing (1)
internal/service/auth_service.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/auth_service.go (1)
244-263:⚠️ Potential issue | 🔴 CriticalThe cap currently counts successful logins too.
RecordLoginAttempt(..., true)still creates and retains aLoginAttempt, and this threshold check runs before the success-reset branch. Sinceinternal/middleware/context_middleware.go(Lines 164-177) records successful basic-auth logins, 256 distinct successful users will eventually trip a global lockdown even with zero failed attempts. Handlesuccessbefore the cap check and remove the identifier fromauth.loginAttemptsinstead of keeping a zeroed entry.💡 Suggested direction
func (auth *AuthService) RecordLoginAttempt(identifier string, success bool) { if auth.config.LoginMaxRetries <= 0 || auth.config.LoginTimeout <= 0 { return } auth.loginMutex.Lock() defer auth.loginMutex.Unlock() - if len(auth.loginAttempts) >= MaxLoginAttemptRecords { + if success { + delete(auth.loginAttempts, identifier) + return + } + + if _, exists := auth.loginAttempts[identifier]; !exists && len(auth.loginAttempts) >= MaxLoginAttemptRecords { if auth.lockdown != nil && auth.lockdown.Active { return } go auth.lockdownMode() return @@ - if success { - attempt.FailedAttempts = 0 - attempt.LockedUntil = time.Time{} // Reset lock time - return - } - attempt.FailedAttempts++🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` around lines 244 - 263, The global cap on auth.loginAttempts incorrectly counts successful logins; modify the RecordLoginAttempt logic to handle the success case before checking MaxLoginAttemptRecords: if success is true, remove the entry for identifier from auth.loginAttempts (or avoid creating one) and return, otherwise proceed with the existing cap check and lockdownMode invocation; ensure you reference auth.loginAttempts, MaxLoginAttemptRecords, lockdownMode, LoginAttempt and identifier when making the change so successful basic-auth logins do not increment the global count.
♻️ Duplicate comments (1)
internal/service/auth_service.go (1)
244-249:⚠️ Potential issue | 🔴 CriticalThe lockdown arm is still racy.
The new guard doesn't close the window because
auth.lockdownis only assigned insidelockdownMode(). Another waiter can acquireloginMutexafter this function returns but before the goroutine runs, still seeauth.lockdown == nil, and start a second lockdown goroutine; whichever one finishes first can clear the lockdown early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/auth_service.go` around lines 244 - 249, The code races because auth.lockdown is only set inside lockdownMode(), so between returning and the goroutine starting another caller can also spawn a lockdown; fix by setting a sentinel lockdown value while still holding loginMutex before spawning the goroutine: under the same lock used for inspecting auth.loginAttempts (loginMutex) create and assign auth.lockdown = &Lockdown{Active:true, /* initialize any needed fields */} if auth.lockdown == nil, then start go auth.lockdownMode() and return; this ensures only one goroutine is started and later lockdownMode() can update/clear the same auth.lockdown object safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/service/auth_service.go`:
- Around line 214-217: The current IsAccountLocked implementation collapses
global lockdown into the per-account lock return values; change the API to
expose lock scope so callers can tell account vs system lockdown (e.g., modify
IsAccountLocked to return (locked bool, remaining int, scope string) or add a
separate IsSystemLocked function). Specifically, update the logic around
auth.lockdown (lockdown.Active and lockdown.ActiveUntil) to set scope = "global"
(and keep scope = "account" for per-account locks), return the new scope value
instead of conflating it with account state, and update callers such as the
user_controller's use of IsAccountLocked to handle the new third return value
(or call the new IsSystemLocked) so they can distinguish and handle system-wide
lockdowns differently.
---
Outside diff comments:
In `@internal/service/auth_service.go`:
- Around line 244-263: The global cap on auth.loginAttempts incorrectly counts
successful logins; modify the RecordLoginAttempt logic to handle the success
case before checking MaxLoginAttemptRecords: if success is true, remove the
entry for identifier from auth.loginAttempts (or avoid creating one) and return,
otherwise proceed with the existing cap check and lockdownMode invocation;
ensure you reference auth.loginAttempts, MaxLoginAttemptRecords, lockdownMode,
LoginAttempt and identifier when making the change so successful basic-auth
logins do not increment the global count.
---
Duplicate comments:
In `@internal/service/auth_service.go`:
- Around line 244-249: The code races because auth.lockdown is only set inside
lockdownMode(), so between returning and the goroutine starting another caller
can also spawn a lockdown; fix by setting a sentinel lockdown value while still
holding loginMutex before spawning the goroutine: under the same lock used for
inspecting auth.loginAttempts (loginMutex) create and assign auth.lockdown =
&Lockdown{Active:true, /* initialize any needed fields */} if auth.lockdown ==
nil, then start go auth.lockdownMode() and return; this ensures only one
goroutine is started and later lockdownMode() can update/clear the same
auth.lockdown object safely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41474903-8c1d-41db-8fd3-3d094781017b
📒 Files selected for processing (1)
internal/service/auth_service.go
|
Yeah we can merge this. |
2 participants
Summary by CodeRabbit
Security Improvements
Performance