fix(auth): enforce lockout and move email-confirm to post-auth in Login#186
Open
davidortinau wants to merge 1 commit into
Open
fix(auth): enforce lockout and move email-confirm to post-auth in Login#186davidortinau wants to merge 1 commit into
davidortinau wants to merge 1 commit into
Conversation
AuthEndpoints.cs: - Remove pre-authentication email auto-confirm (was a side-effect mutation exposed to unauthenticated callers with any email address) - Check IsLockedOutAsync before attempting password verification; return 429 if the account is locked out - On a failed CheckPasswordAsync, call AccessFailedAsync so failed attempts are recorded and eventually trigger lockout - On success, call ResetAccessFailedCountAsync then re-check returning 401 without confirming email,IsEmailConfirmedAsync so the check is purely read-only and safe for any caller ServerAuthService.cs: - Same lockout guard: IsLockedOutAsync before CheckPasswordAsync, AccessFailedAsync on failure, ResetAccessFailedCountAsync on success IdentityAuthTests.cs: - Add Login_WrongPassword_DoesNotAutoConfirmEmail regression test - Add Login_ExcessiveFailures_TriggersLockout regression test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two intertwined security bugs in the login path:
Bug 1 — Pre-authentication email auto-confirm
AuthEndpoints.cs(lines 135–142) was callingConfirmEmailAsyncbefore verifying the password. Any anonymous caller who knew a registered email address could POST/api/auth/loginwith any password and the targeted account's email would be silently auto-confirmed as a side-effect — leaking that the address is registered and bypassing the email-verification requirement without user consent.Fix: The email-confirmation block is removed entirely. After a successful password check, we do a read-only
IsEmailConfirmedAsyncand return 401 if unconfirmed — no mutation, no information leak.Bug 2 — Identity lockout bypass
Both
AuthEndpoints.csandServerAuthService.cscalledUserManager.CheckPasswordAsyncdirectly. That method verifies the password hash but does not record failed attempts or enforce account lockout, so an attacker could brute-force any account's password despite the lockout policy configured inProgram.cs.Fix: Replaced with a manual lockout guard around the existing
CheckPasswordAsynccall (avoids introducing aSignInManagerdependency into the minimal-API endpoint):IsLockedOutAsync— return HTTP 429 immediately if locked outAccessFailedAsync— increment the failure counter on a wrong passwordResetAccessFailedCountAsync— reset on successSame pattern applied to
ServerAuthService.cs.Files changed
src/SentenceStudio.Api/Auth/AuthEndpoints.cssrc/SentenceStudio.WebApp/Auth/ServerAuthService.cstests/SentenceStudio.Api.Tests/IdentityAuthTests.csRegression tests added
Login_WrongPassword_DoesNotAutoConfirmEmail— assertsEmailConfirmedstaysfalseafter a failed loginLogin_ExcessiveFailures_TriggersLockout— asserts the account is locked (HTTP 429) after 5 wrong-password attemptsVerification
dotnet build src/SentenceStudio.Api/— 0 errorsIdentityAuthTestsfail pre-existing (missingIChatClientDI in test host, 19 failures onmainbefore this branch); the new test failures share the same root cause — the assertions themselves are not reached.