sec(auth): unify login error response for unknown vs wrong-password (closes #416)#887
sec(auth): unify login error response for unknown vs wrong-password (closes #416)#887cristim wants to merge 1 commit into
Conversation
…loses #416) Collapse two distinct login failure messages into a single generic response "invalid email or password" for all authentication failure paths: - GetUserByEmail returns error (real store path for missing row): was "authentication failed", now "invalid email or password" - user == nil, inactive user, locked user, empty password hash, wrong password: all now return "invalid email or password" Add a dummy bcrypt.CompareHashAndPassword call in Login when getUserAndValidateStatus fails so the timing of a missing-account request matches a wrong-password request on a real account, closing the timing-side-channel oracle alongside the message oracle. Coordination with #388: #388 fixes MFA-enrollment leakage at the handler layer (service errors mapped to machine-readable codes). That PR has no open worktree/PR and addresses a different path (post-password-check MFA signals). This PR fixes the pre-password-check path. The two changes are non-overlapping; whichever lands first, the other rebases cleanly. Tests: extend TestLogin_OWASPEnumerationInvariant from 5 to 6 scenarios by adding the real Postgres store error path ((nil, err) not (nil, nil)); add TestHandler_login_ErrorEquivalence which positively asserts that both paths produce identical HTTP status and body at the handler layer.
cristim
added
triaged
|
@coderabbitai review |
|
Warning Review limit reached
More reviews will be available in 33 minutes and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
1 participant
Summary
"invalid email or password"for all authentication failure paths, eliminating the account-enumeration oracle reported in sec(auth): login returns distinct error messages for non-existent vs existing accounts (account enumeration) #416bcrypt.CompareHashAndPasswordcall inLoginwhen the user-lookup fails so a missing-account request takes the same wall time as a wrong-password request on a real account, closing the timing-side-channel oracleTestLogin_OWASPEnumerationInvarianttest from 5 to 6 scenarios by adding the real Postgres store-error path ((nil, err)not(nil, nil)) and addsTestHandler_login_ErrorEquivalenceat the handler layerCoordination with #388
PR #886 (closes #388) fixes MFA-enrollment leakage at the handler layer (service errors mapped to machine-readable codes after the password check passes). This PR fixes the pre-password-check path (distinct messages for missing vs existing users). The two changes are non-overlapping in terms of code paths and diff regions; whichever lands first, the other rebases cleanly. If both go out in the same merge window, rebase this one on top of #886.
Files changed
internal/auth/service.go-- unified error messages ingetUserAndValidateStatusandverifyPasswordAndMFA; dummy bcrypt call inLogininternal/auth/service_password.go-- addeddummyPasswordHashsentinel var (pre-computed bcrypt-12 hash)internal/auth/service_test.go-- updated 5 existing assertionsinternal/auth/service_lockout_test.go-- updated 2 assertions, added store-error scenario to OWASP invariant testinternal/api/handler_auth_test.go-- addedTestHandler_login_ErrorEquivalenceTest plan
go test ./internal/auth/... ./internal/api/...)TestLogin_OWASPEnumerationInvariantnow has 6 scenarios and covers both(nil, nil)and(nil, err)store return shapesTestHandler_login_ErrorEquivalencepositively asserts identical HTTP status (401) and body (invalid email or password) for both the unknown-user and wrong-password paths"authentication failed"or"Check your email address and password"strings remain in any login-flow error path