Fix/otp attempts verification

src/database/migrations/20251017090000-add-attempts-by-ip-to-otps.js

@@ -0,0 +1,13 @@
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+  async up(queryInterface, Sequelize) {
+    await queryInterface.addColumn("otps", "attempts_by_ip", {
+      allowNull: false,
+      type: Sequelize.JSONB,
+      defaultValue: {},
+    })
+  },
+  async down(queryInterface, Sequelize) {
+    await queryInterface.removeColumn("otps", "attempts_by_ip")
+  },
+}

src/database/models/Otp.ts

@@ -58,4 +58,12 @@ export class Otp extends Model {
 
   @UpdatedAt
   updatedAt!: Date
+
+  // JSON map of ip -> attempts for per-IP tracking
+  @Column({
+    allowNull: false,
+    type: DataType.JSONB,
+    defaultValue: {},
+  })
+  attemptsByIp!: Record<string, number>
 }

src/routes/v2/auth.js

@@ -9,7 +9,7 @@ const logger = require("@logger/logger").default
 const { attachReadRouteHandlerWrapper } = require("@middleware/routeHandler")
 
 const FRONTEND_URL = config.get("app.frontendUrl")
-const { isSecure } = require("@utils/auth-utils")
+const { isSecure, getUserIPAddress } = require("@utils/auth-utils")
 
 const {
   EmailSchema,
@@ -106,7 +106,10 @@ class AuthRouter {
         message: `Invalid request format: ${error.message}`,
       })
     const email = rawEmail.toLowerCase()
-    const userInfo = (await this.authService.verifyOtp({ email, otp })).value
+    const userIp = getUserIPAddress(req)
+    const userInfo = (
+      await this.authService.verifyOtp({ email, otp, clientIp: userIp })
+    ).value
     Object.assign(req.session, { userInfo })
     logger.info(`User ${userInfo.email} successfully logged in`)
     return res.sendStatus(200)

src/routes/v2/authenticated/users.ts

@@ -14,6 +14,7 @@ import UserSessionData from "@classes/UserSessionData"
 import DatabaseError from "@root/errors/DatabaseError"
 import { isError, RequestHandler } from "@root/types"
 import { nameAnonymousMethods } from "@root/utils/apm-utils"
+import { getUserIPAddress } from "@root/utils/auth-utils"
 import {
   VerifyEmailOtpSchema,
   VerifyMobileNumberOtpSchema,
@@ -83,8 +84,9 @@ export class UsersRouter {
     const userId = userSessionData.isomerUserId
     const parsedEmail = email.toLowerCase()
 
+    const userIp = getUserIPAddress(req)
     return this.usersService
-      .verifyEmailOtp(parsedEmail, otp)
+      .verifyEmailOtp(parsedEmail, otp, userIp)
       .andThen(() =>
         ResultAsync.fromPromise(
           this.usersService.updateUserByIsomerId(userId, {
@@ -142,8 +144,9 @@ export class UsersRouter {
     const { userSessionData } = res.locals
     const userId = userSessionData.isomerUserId
 
+    const userIp = getUserIPAddress(req)
     return this.usersService
-      .verifyMobileOtp(mobile, otp)
+      .verifyMobileOtp(mobile, otp, userIp)
       .andThen(() =>
         ResultAsync.fromPromise(
           this.usersService.updateUserByIsomerId(userId, {

src/services/identity/UsersService.ts

@@ -212,13 +212,40 @@ class UsersService {
 
   async sendEmailOtp(email: string) {
     const normalizedEmail = email.toLowerCase()
+
+    // Check if there's already a valid OTP for this email
+    // This prevents creating new OTPs while a valid one exists, mitigating
+    // the attack vector where an attacker spam OTP requests
+    // to prevent the user from logging in
+    const existingOtp = await this.otpRepository.findOne({
+      where: {
+        email: normalizedEmail,
+        hashedOtp: {
+          [Op.regexp]: "\\S+", // at least one non-whitespace character (i.e. is truthy!)
+        },
+      },
+    })
+    if (existingOtp && existingOtp.expiresAt >= new Date()) {
+      logger.info({
+        message: "OTP request blocked: valid OTP already exists",
+        meta: {
+          email: normalizedEmail,
+          expiresAt: existingOtp.expiresAt,
+        },
+      })
+      // Return silently to avoid revealing whether an OTP exists
+      // This maintains security by not leaking information about existing OTPs
+      return
+    }
+
     const { otp, hashedOtp } = await this.otpService.generateLoginOtpWithHash()
 
     // Reset attempts to login
     await this.otpRepository.upsert({
       email: normalizedEmail,
       hashedOtp,
       attempts: 0,
+      attemptsByIp: {},
       expiresAt: this.getOtpExpiry(),
     })
 
@@ -239,6 +266,7 @@ class UsersService {
       mobileNumber,
       hashedOtp,
       attempts: 0,
+      attemptsByIp: {},
       expiresAt: this.getOtpExpiry(),
     })
 
@@ -271,10 +299,12 @@ class UsersService {
     otp,
     findConditions,
     findErrorMessage,
+    clientIp = "unknown", // default to 'unknown' bucket when IP missing to ensure users are not locked out
   }: {
     otp: string | undefined
     findConditions: { email: string } | { mobileNumber: string }
     findErrorMessage: string
+    clientIp?: string
   }) {
     if (!otp) {
       return errAsync(new BadRequestError("Empty OTP provided"))
@@ -316,17 +346,27 @@ class UsersService {
           )
         }
 
-        if (otpEntry.attempts >= MAX_NUM_OTP_ATTEMPTS) {
+        // GTA-80-009 WP2: Enforce per-IP attempts;
+        const attemptsByIp = otpEntry.attemptsByIp || {}
+        const attemptsForIp = attemptsByIp[clientIp] ?? 0
+        if (attemptsForIp > MAX_NUM_OTP_ATTEMPTS) {
           // should this delete the otpEntry as well?
           return errAsync(new BadRequestError("Max number of attempts reached"))
         }
 
         // We must successfully be able to increment the otp record attempts before any processing, to prevent brute-force race condition
         // Consult GTA-24-012 WP3 for details
+        // atomically increment attempts for this IP using JSONB concatenation guarded by current value
+        const newAttemptsForIp = attemptsForIp + 1
+        const attemptsByIpUpdated = { ...(otpEntry.attemptsByIp || {}) }
+        attemptsByIpUpdated[clientIp] = newAttemptsForIp
+
         return ResultAsync.fromPromise(
           this.otpRepository.update(
             {
+              // maintain legacy aggregate attempts for monitoring, but do not enforce with it
               attempts: otpEntry.attempts + 1,
+              attemptsByIp: attemptsByIpUpdated,
             },
             {
               where: {
@@ -377,21 +417,27 @@ class UsersService {
     /* eslint-enable @typescript-eslint/no-non-null-assertion */
   }
 
-  verifyEmailOtp(email: string, otp: string | undefined) {
+  verifyEmailOtp(email: string, otp: string | undefined, clientIp?: string) {
     const normalizedEmail = email.toLowerCase()
 
     return this.verifyOtp({
       otp,
       findConditions: { email: normalizedEmail },
       findErrorMessage: "Error finding OTP entry when verifying email OTP",
+      clientIp,
     })
   }
 
-  verifyMobileOtp(mobileNumber: string, otp: string | undefined) {
+  verifyMobileOtp(
+    mobileNumber: string,
+    otp: string | undefined,
+    clientIp?: string
+  ) {
     return this.verifyOtp({
       otp,
       findConditions: { mobileNumber },
       findErrorMessage: "Error finding OTP entry when verifying mobile OTP",
+      clientIp,
     })
   }
 

src/services/identity/__tests__/UsersService.spec.ts

@@ -1,7 +1,9 @@
 import { ModelStatic } from "sequelize/types"
 import { Sequelize } from "sequelize-typescript"
 
+import { config } from "@root/config/config"
 import { Otp, User, Whitelist } from "@root/database/models"
+import { BadRequestError } from "@root/errors/BadRequestError"
 import SmsClient from "@services/identity/SmsClient"
 import TotpGenerator from "@services/identity/TotpGenerator"
 import MailClient from "@services/utilServices/MailClient"
@@ -39,8 +41,30 @@ const MockWhitelist = {
   findAll: jest.fn(),
 }
 
+let dbAttempts = 0
+let dbAttemptsByIp: Record<string, number> = {}
+
+const futureDate = new Date(Date.now() + 60 * 60 * 1000)
+
 const MockOtp = {
-  findOne: jest.fn(),
+  findOne: jest.fn().mockImplementation(() =>
+    Promise.resolve({
+      id: 1,
+      email: mockEmail,
+      hashedOtp: "hashed",
+      attempts: dbAttempts,
+      attemptsByIp: { ...dbAttemptsByIp },
+      expiresAt: futureDate,
+      destroy: jest.fn(),
+    })
+  ),
+  update: jest.fn().mockImplementation((values: any, options: any) => {
+    if (options?.where?.attempts !== dbAttempts) return Promise.resolve([0])
+
+    dbAttempts = values.attempts
+    dbAttemptsByIp = { ...(values.attemptsByIp || {}) }
+    return Promise.resolve([1])
+  }),
 }
 
 const UsersService = new _UsersService({
@@ -57,7 +81,11 @@ const mockEmail = "[email protected]"
 const mockGithubId = "sudowoodo"
 
 describe("User Service", () => {
-  afterEach(() => jest.clearAllMocks())
+  afterEach(() => {
+    jest.clearAllMocks()
+    dbAttempts = 0
+    dbAttemptsByIp = {}
+  })
 
   it("should return the result of calling the findOne method by email on the db model", () => {
     // Arrange
@@ -74,6 +102,56 @@ describe("User Service", () => {
     })
   })
 
+  describe("verifyOtp per-IP behavior", () => {
+    const maxAttempts = config.get("auth.maxNumOtpAttempts")
+    const wrongOtp = "000000"
+
+    it("increments attempts for provided IP and returns invalid until max", async () => {
+      (MockOtpService.verifyOtp as jest.Mock).mockResolvedValue(false)
+      const ip = "1.1.1.1"
+
+      for (let i = 1; i <= maxAttempts; i += 1) {
+        // eslint-disable-next-line no-await-in-loop
+        const result = await UsersService.verifyEmailOtp(mockEmail, wrongOtp, ip)
+        expect(result.isErr()).toBe(true)
+        const err = result._unsafeUnwrapErr()
+        expect(err).toBeInstanceOf(BadRequestError)
+        if (i < maxAttempts) {
+          expect((err as BadRequestError).message).toBe("OTP is not valid")
+        } else {
+          expect((err as BadRequestError).message).toBe(
+            "Max number of attempts reached"
+          )
+        }
+      }
+
+      expect(MockOtp.update).toHaveBeenCalled()
+      expect(dbAttemptsByIp[ip]).toBe(maxAttempts - 1) // last call rejected before increment
+    })
+
+    it("uses 'unknown' bucket when clientIp is missing", async () => {
+      (MockOtpService.verifyOtp as jest.Mock).mockResolvedValue(false)
+
+      const result = await UsersService.verifyEmailOtp(mockEmail, wrongOtp)
+      expect(result.isErr()).toBe(true)
+      result._unsafeUnwrapErr() // ensure unwrap doesn't throw
+      expect(dbAttemptsByIp.unknown).toBe(1)
+    })
+
+    it("tracks per-IP separately", async () => {
+      (MockOtpService.verifyOtp as jest.Mock).mockResolvedValue(false)
+      const ipA = "1.1.1.1"
+      const ipB = "2.2.2.2"
+
+      await UsersService.verifyEmailOtp(mockEmail, wrongOtp, ipA)
+      await UsersService.verifyEmailOtp(mockEmail, wrongOtp, ipA)
+      await UsersService.verifyEmailOtp(mockEmail, wrongOtp, ipB)
+
+      expect(dbAttemptsByIp[ipA]).toBe(2)
+      expect(dbAttemptsByIp[ipB]).toBe(1)
+    })
+  })
+
   it("should return the result of calling the findOne method by githubId on the db model", () => {
     // Arrange
     const expected = "user1"

src/services/utilServices/AuthService.js

@@ -125,9 +125,9 @@ class AuthService {
     }
   }
 
-  async verifyOtp({ email, otp }) {
+  async verifyOtp({ email, otp, clientIp }) {
     return this.usersService
-      .verifyEmailOtp(email, otp)
+      .verifyEmailOtp(email, otp, clientIp)
       .andThen(() =>
         ResultAsync.fromPromise(
           this.usersService.loginWithEmail(email),

src/services/utilServices/RateLimiter.ts

@@ -1,7 +1,7 @@
 import rateLimit from "express-rate-limit"
 
 import { BaseIsomerError } from "@root/errors/BaseError"
-import { isSecure } from "@root/utils/auth-utils"
+import { getUserIPAddress } from "@root/utils/auth-utils"
 
 const DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS = 900000
 
@@ -21,7 +21,7 @@ export const rateLimiter = rateLimit({
   // We know that this key exists in a secure env (Cloudflare)
   // See https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#cf-connecting-ip
   keyGenerator: (req) => {
-    const userIp = isSecure ? req.get("cf-connecting-ip") : req.ip
+    const userIp = getUserIPAddress(req)
     if (!userIp) {
       // This should never happen, but if it does, we should know about it
       throw new BaseIsomerError({

src/utils/auth-utils.js

@@ -4,6 +4,14 @@ const NODE_ENV = config.get("env")
 
 const isSecure = NODE_ENV !== "dev" && NODE_ENV !== "test"
 
+// FIXME: This makes a strong assumption that the app is always behind
+// Cloudflare, but may not necessarily be the case when Cloudflare is disabled.
+// Fix this to fallback to other headers or req.ip if Cloudflare headers are not
+// present.
+const getUserIPAddress = (req) =>
+  isSecure ? req.get("cf-connecting-ip") : req.ip
+
 module.exports = {
   isSecure,
+  getUserIPAddress,
 }