Repository navigation
fix(auth): limit MFA challenge attempts and revoke the access token on logout (#689) - #875
Closed
alex-dembele wants to merge 6 commits into
Closed
alex-dembele wants to merge 6 commits into
alex-dembele wants to merge 6 commits into
Conversation
The challenge needs exact per-token and per-account counters that hold across instances. AllowRate counts in wall-clock buckets, so a five-minute challenge token can straddle two of them and get up to eight tries. The store counts from the first failure, sets locks once (so the lock is audited and mailed once), and falls back to a per-instance memory store when Redis errors instead of failing open. The spec for the issue is committed alongside. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
The challenge checked a TOTP code, then the backup codes, and never counted a miss, so a stolen password plus enough requests was a login. Each attempt is now reserved against the challenge token and the account before the code is read, which keeps parallel requests from sharing one slot. A token gets five attempts and is blacklisted when the fifth fails. An account that fails ten times in fifteen minutes, over any tokens, is locked for a fixed fifteen minutes and the owner is told by mail. Backup codes are the same attempt as TOTP codes. If the store cannot answer, the challenge is refused rather than run unchecked. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
A lock means someone got past the password and then guessed codes. The owner is the only one who can fix that, by resetting the password, so they get a notice in their language the moment the lock is set. Sent through the async security mailer, so a slow SMTP server never delays the challenge response. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
The handler passes the challenge token's JTI and expiry to the use case and answers each refusal with a code the sign-in screen can act on: MFA_CHALLENGE_EXHAUSTED (401, sign in again) and MFA_LOCKED (429, with Retry-After). A lock is audited once, when it is set. Store and decryption errors now answer 500 without their text instead of a 400 that echoed it. The route gets its own per-IP limit, 20 per 5 minutes in a prefixed bucket, mounted before the token middleware so a flood never reaches token parsing and never spends the login budget. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Logout deleted the refresh token and cleared the cookies, but the access token kept working for up to fifteen minutes. Whoever had copied it kept the session. The handler now reads the access token from the Authorization header or the session cookie, verifies its signature, and the use case blacklists its JTI for the time it has left. The access token is revoked before the refresh token, and both are always attempted, so a stale refresh token or a Redis outage on one does not spare the other. A forged token is ignored rather than trusted. Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
Signed-off-by: alex-dembele <alexandredembele16@gmail.com>
This was referenced Oct 2, 2026
Member
Author
|
Superseded: the #689 work is already on master. PR #871 (titled for #870) was opened from the original Closing to avoid an empty merge. Reopen if I have missed something. |
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
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.
Closes #689
Anyone holding a user's password could guess the second factor:
POST /auth/mfa/challengehad no attempt counter and no rate limit, and its token was reusable for five minutes. Logout also left the access token alive for up to fifteen minutes.Spec, with the owner's decisions of 2026-10-02:
docs/689_MFA_CHALLENGE_LIMITS.md.What changes
Per challenge token. Five attempts. When the fifth one fails, the token's JTI is blacklisted, and the next request with it gets
401 TOKEN_REVOKEDfrom the middleware. The fifth failure itself answers401 MFA_CHALLENGE_EXHAUSTED.Per account. Ten failures in fifteen minutes, over any tokens and any addresses, lock the account for a fixed fifteen minutes. During the lock, requests answer
429 MFA_LOCKEDwithRetry-After. The lock is audited once (mfa_verify, reasonmfa_locked), and the owner gets one email in their language. A successful challenge forgives earlier mistypes.Per IP. The route has its own bucket, 20 requests per 5 minutes, under a
mfa-challenge-ip:prefix. It is mounted before the token middleware, so a flood never reaches token parsing, and it never spends the login budget.Backup codes are the same attempt as TOTP codes. One request is one attempt, whatever was typed.
Logout verifies the access token sent in the header or the session cookie and blacklists its JTI for the time it has left. The access token is revoked before the refresh token, and both revocations are always attempted, so a stale refresh token or a Redis hiccup on one does not spare the other. A forged token is ignored.
A few implementation notes:
authmfa.AttemptStore. It is backed by Redis, counts from the first failure, and falls back to a per-instance memory store if Redis errors, so the limits never fail open. I did not reuseAllowRate: its windows are aligned to the wall clock, so a five-minute token can straddle two of them and get eight tries.Verification
On the final tip: exit 0, 80 packages
ok, 0FAIL.An earlier full run failed once in
internal/handler:TestRealtimeStream_HelloDocumentsTheRevocationBoundhit a data race between a test spy and a stream goroutine. The same failure reproduces on master (-count=10on a master worktree), and this branch does not touch that package. Tracked in #873.New tests:
TestMFAChallenge_{Success,NotFound,Unauthorized,TokenDiesAfterFiveFailures,UserBackoffAcrossTokens,BackupCodesCounted,SuccessClearsFailureCount,LockSendsOneEmail,StoreErrorFailsClosed}andTestLogout_{Success,NotFound,Unauthorized,ExpiredAccessTokenNeedsNoBlacklist,BlacklistOutageStillRevokesRefreshToken};TestMFAChallengeE2E_*(4) andTestLogout_{AccessTokenRejectedAfterLogout,CookieSessionAccessTokenRejectedAfterLogout,ForgedTokenIsIgnored};authmfa(4) andauthmail(2).I checked that
TestLogout_AccessTokenRejectedAfterLogoutcatches the bug: with revocation unwired, it gets 200 where it expects 401.The live check against a local instance (Postgres 18, Redis 7, an SMTP sink, and the admin account enrolled in TOTP) is in the issue comment.
Not in this PR