fix(auth): lock out password-reset OTP after 5 failed attempts#238
Open
sebastiondev wants to merge 1 commit into
Open
fix(auth): lock out password-reset OTP after 5 failed attempts#238sebastiondev wants to merge 1 commit into
sebastiondev wants to merge 1 commit into
Conversation
The password-reset flow issues a 6-digit numeric OTP that lives for 20 minutes. The verification path (auth.service.resetPassword) had no per-record failed-attempt counter and did not invalidate the reset record on failure, so an attacker who triggered a reset email could brute-force the OTP in-window (1e6 guesses). Track a durable 'attempts' counter on the PasswordReset document and invalidate the record after 5 failed submissions. A correct guess after lockout is rejected as well.
|
@sebastiondev is attempting to deploy a commit to the vernu's projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
Summary
POST /auth/reset-passwordverifies a 6-digit numeric OTP without any per-record attempt limit, which makes online brute-force of the OTP feasible within its 20-minute validity window. This PR adds a durable attempts counter to thePasswordResetdocument and locks out the reset record after 5 failed submissions.api/src/auth/auth.service.ts→AuthService.resetPassword.POST /auth/reset-password(api/src/auth/auth.controller.ts:170) — unauthenticated, body-controlledemail+otp+newPassword.Why the existing mitigations don't cover this
The password-reset flow already had some hardening from earlier commits, but none of it stops OTP guessing within a single reset window:
ThrottlerByIpGuardallows 500 requests/minute/IP → up to 10,000 attempts per IP over the 20-minute OTP lifetime. Against a 6-digit code (~9.0×10⁵ effective keyspace) that's roughly a 1.1% success chance per IP per window, and effectively certain from a small proxy pool.POST /auth/request-password-reset.POST /auth/reset-password— the submit path — never callsturnstileService.verify, so a single Turnstile solve unlocks unlimited guessing attempts against the resulting OTP.6252e4f) limits how many fresh OTPs the attacker can trigger, but doesn't limit guesses within one OTP record.bcrypt.compareprotects the stored OTP hash from offline attacks, not online guessing.So pre-fix the practical guessing budget is ≈10k attempts/IP/window × N IPs × 5 records/day, which is more than enough to take over a targeted account.
The fix
Three small changes, kept as narrow as possible:
api/src/auth/schemas/password-reset.schema.ts— addattempts: number(default0) to thePasswordResetschema so the counter is persisted alongside the OTP record.api/src/auth/auth.service.ts— inresetPassword:attempts >= MAX_PASSWORD_RESET_ATTEMPTS(5).bcrypt.compare, incrementattemptsandsave()the record.expiresAt = new Date(Date.now())so a subsequent correct-guess submission against the same record cannot succeed.api/src/auth/auth.service.spec.ts— new regression tests (see below).Rationale for the details:
bcrypt.compare, so a locked-out record costs constant time regardless of the guess.Proof of concept (pre-fix)
Run the API locally with the current
main:Register a victim account via the UI or:
Trigger a reset (this needs one Turnstile solve in production, none locally if Turnstile is unconfigured):
Now brute-force the OTP against the same record — on the pre-fix code every wrong guess just returns
400 Invalid OTPand the record stays valid:On the pre-fix code this loop eventually succeeds within the OTP's 20-minute window (or across a handful of
request-password-resetcycles) and the attacker'sAttackerP@ss1becomes the account password. On the post-fix code the 6th request against any single reset record returns400 Invalid OTPand further guesses (including the correct one) are rejected because the record'sexpiresAthas been set to now.Tests
api/src/auth/auth.service.spec.tsadds three focused regression tests aroundresetPassword:user.savewas never called andreset.attempts >= 5was persisted.attempts: 2succeeds and the user's password is written.attempts: 5rejects the correct OTP withHttpExceptionand does not modify the user.Run just this file:
All three cases pass on the fix branch.
Adversarial review
Before submitting, we tried to disprove the finding:
AuthGuardwe missed." No —api/src/auth/auth.controller.ts:168-173shows@Post('/reset-password')with no@UseGuards. TheAuthGuarddecorator is used further down the same controller (e.g./send-email-verification-email), so its absence here is intentional and the endpoint is genuinely unauthenticated.turnstileService.verifyshows it's only invoked fromrequestResetPassword. The submit path (resetPassword) never calls it, so a bot that solved Turnstile once can reuse the resulting OTP window freely.ThrottlerByIpGuardis set to 500 rpm/IP globally, which is ~10,000 attempts per IP over the OTP's 20-minute lifetime — orders of magnitude too loose to defend a 6-digit OTP.bcrypt.comparetiming already limits throughput." bcrypt slows offline cracking of the stored hash, but online guessing throughput is bounded by network + throttler, not by hash cost.Scope
The change is limited to the password-reset OTP path. Email-verification OTPs (
email-verification.schema.ts) use a similar shape but have lower stakes and are out of scope for this PR; happy to open a follow-up if you'd like the same treatment applied there.Discovered by the Sebastion AI GitHub App.