From 1f7c1c16ec061a42d1bb052852ed10d5d0270954 Mon Sep 17 00:00:00 2001 From: Sebastion Date: Fri, 3 Jul 2026 20:35:40 +0100 Subject: [PATCH] fix(api): lock out password-reset OTP after 5 failed attempts 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. --- api/src/auth/auth.service.spec.ts | 141 ++++++++++++++++++ api/src/auth/auth.service.ts | 34 ++++- api/src/auth/schemas/password-reset.schema.ts | 5 + 3 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 api/src/auth/auth.service.spec.ts diff --git a/api/src/auth/auth.service.spec.ts b/api/src/auth/auth.service.spec.ts new file mode 100644 index 00000000..d8eb49d1 --- /dev/null +++ b/api/src/auth/auth.service.spec.ts @@ -0,0 +1,141 @@ +import { Test, TestingModule } from '@nestjs/testing' +import { getModelToken } from '@nestjs/mongoose' +import { HttpException } from '@nestjs/common' +import * as bcrypt from 'bcryptjs' +import { AuthService } from './auth.service' +import { UsersService } from '../users/users.service' +import { JwtService } from '@nestjs/jwt' +import { MailService } from '../mail/mail.service' +import { TurnstileService } from '../common/turnstile.service' +import { ApiKey } from './schemas/api-key.schema' +import { PasswordReset } from './schemas/password-reset.schema' +import { AccessLog } from './schemas/access-log.schema' +import { EmailVerification } from './schemas/email-verification.schema' + +// Regression tests for CWE-640 password reset OTP brute-force protection. +describe('AuthService.resetPassword — OTP brute-force protection', () => { + let service: AuthService + + const buildUser = () => + ({ + _id: 'user-1', + email: 'victim@example.com', + password: 'existing-hash', + save: jest.fn().mockResolvedValue(undefined), + }) as any + + const buildResetDoc = async (rawOtp: string, overrides: any = {}) => { + const doc: any = { + user: 'user-1', + otp: await bcrypt.hash(rawOtp, 4), + expiresAt: new Date(Date.now() + 10 * 60 * 1000), + attempts: 0, + ...overrides, + } + doc.save = jest.fn().mockImplementation(() => Promise.resolve(doc)) + return doc + } + + const mockUsersService = { findOne: jest.fn() } + const mockJwtService = { sign: jest.fn() } + const mockMailService = { sendEmailFromTemplate: jest.fn() } + const mockTurnstileService = { verify: jest.fn() } + const mockPasswordResetModel = { findOne: jest.fn() } + const emptyModel = {} + + beforeEach(async () => { + const module: TestingModule = await Test.createTestingModule({ + providers: [ + AuthService, + { provide: UsersService, useValue: mockUsersService }, + { provide: JwtService, useValue: mockJwtService }, + { provide: MailService, useValue: mockMailService }, + { provide: TurnstileService, useValue: mockTurnstileService }, + { provide: getModelToken(ApiKey.name), useValue: emptyModel }, + { + provide: getModelToken(PasswordReset.name), + useValue: mockPasswordResetModel, + }, + { provide: getModelToken(AccessLog.name), useValue: emptyModel }, + { + provide: getModelToken(EmailVerification.name), + useValue: emptyModel, + }, + ], + }).compile() + + service = module.get(AuthService) + jest.clearAllMocks() + }) + + it('locks out the reset record after 5 wrong OTP attempts (brute-force prevention)', async () => { + const user = buildUser() + const reset = await buildResetDoc('123456') + mockUsersService.findOne.mockResolvedValue(user) + mockPasswordResetModel.findOne.mockResolvedValue(reset) + + // 5 wrong attempts — each should be rejected. The 5th one must consume + // the reset record so no further attempts (right OR wrong) can succeed. + for (let i = 0; i < 5; i++) { + await expect( + service.resetPassword({ + email: user.email, + otp: '000000', + newPassword: 'a-new-password', + }), + ).rejects.toBeInstanceOf(HttpException) + } + + // After the lockout, submitting the CORRECT otp must still be rejected — + // the record has been invalidated by the failed-attempts counter. + mockPasswordResetModel.findOne.mockResolvedValue(reset) + await expect( + service.resetPassword({ + email: user.email, + otp: '123456', + newPassword: 'a-new-password', + }), + ).rejects.toBeInstanceOf(HttpException) + + // Password must not have been rewritten by any of the above. + expect(user.save).not.toHaveBeenCalled() + // The reset record must have been persisted with attempts incremented + // (i.e., the failed-attempt tracking is durable, not in-memory only). + expect(reset.save).toHaveBeenCalled() + expect(reset.attempts).toBeGreaterThanOrEqual(5) + }) + + it('accepts the correct OTP when attempts are below the limit', async () => { + const user = buildUser() + const reset = await buildResetDoc('654321', { attempts: 2 }) + mockUsersService.findOne.mockResolvedValue(user) + mockPasswordResetModel.findOne.mockResolvedValue(reset) + + await expect( + service.resetPassword({ + email: user.email, + otp: '654321', + newPassword: 'a-new-password', + }), + ).resolves.toEqual(expect.objectContaining({ message: expect.any(String) })) + + expect(user.save).toHaveBeenCalled() + }) + + it('rejects immediately if the record is already locked out (attempts >= limit)', async () => { + const user = buildUser() + const reset = await buildResetDoc('654321', { attempts: 5 }) + mockUsersService.findOne.mockResolvedValue(user) + mockPasswordResetModel.findOne.mockResolvedValue(reset) + + await expect( + service.resetPassword({ + email: user.email, + otp: '654321', // correct OTP + newPassword: 'a-new-password', + }), + ).rejects.toBeInstanceOf(HttpException) + + expect(user.save).not.toHaveBeenCalled() + }) +}) diff --git a/api/src/auth/auth.service.ts b/api/src/auth/auth.service.ts index 8b658f63..3e48df46 100644 --- a/api/src/auth/auth.service.ts +++ b/api/src/auth/auth.service.ts @@ -194,6 +194,11 @@ export class AuthService { return { message: 'Password reset email sent' } } + // Reject after this many failed OTP submissions against the same record. + // A 6-digit OTP has ~1e6 possibilities and lives for 20 minutes, so we + // cap online guessing well below what an attacker would need to succeed. + private static readonly MAX_PASSWORD_RESET_ATTEMPTS = 5 + async resetPassword({ email, otp, newPassword }: ResetPasswordInputDTO) { const user = await this.usersService.findOne({ email }) if (!user) { @@ -208,7 +213,34 @@ export class AuthService { { sort: { createdAt: -1 } }, ) - if (!passwordReset || !(await bcrypt.compare(otp, passwordReset.otp))) { + if (!passwordReset) { + throw new HttpException({ error: 'Invalid OTP' }, HttpStatus.BAD_REQUEST) + } + + // Consume the record if the attempt limit has already been hit. This + // prevents an attacker from brute-forcing a 6-digit OTP within the + // 20-minute window: after MAX_PASSWORD_RESET_ATTEMPTS wrong guesses the + // record is invalidated and a fresh reset request is required. + if ( + (passwordReset.attempts ?? 0) >= AuthService.MAX_PASSWORD_RESET_ATTEMPTS + ) { + passwordReset.expiresAt = new Date(Date.now()) + await passwordReset.save() + throw new HttpException({ error: 'Invalid OTP' }, HttpStatus.BAD_REQUEST) + } + + if (!(await bcrypt.compare(otp, passwordReset.otp))) { + // Track the failed attempt durably so retries across requests count + // against the same lockout budget. + passwordReset.attempts = (passwordReset.attempts ?? 0) + 1 + if ( + passwordReset.attempts >= AuthService.MAX_PASSWORD_RESET_ATTEMPTS + ) { + // Invalidate the record on the last failed attempt so a subsequent + // correct-guess submission cannot succeed. + passwordReset.expiresAt = new Date(Date.now()) + } + await passwordReset.save() throw new HttpException({ error: 'Invalid OTP' }, HttpStatus.BAD_REQUEST) } diff --git a/api/src/auth/schemas/password-reset.schema.ts b/api/src/auth/schemas/password-reset.schema.ts index 06947fa0..412a65b5 100644 --- a/api/src/auth/schemas/password-reset.schema.ts +++ b/api/src/auth/schemas/password-reset.schema.ts @@ -16,6 +16,11 @@ export class PasswordReset { @Prop({ type: Date }) expiresAt: Date + + // Number of failed OTP verification attempts against this record. + // Used to lock out brute-force attempts (see auth.service.resetPassword). + @Prop({ type: Number, default: 0 }) + attempts: number } export const PasswordResetSchema = SchemaFactory.createForClass(PasswordReset)