diff --git a/src/database/migrations/20251017090000-add-attempts-by-ip-to-otps.js b/src/database/migrations/20251017090000-add-attempts-by-ip-to-otps.js new file mode 100644 index 000000000..84979e885 --- /dev/null +++ b/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") + }, +} diff --git a/src/database/models/Otp.ts b/src/database/models/Otp.ts index c9324acec..240fad433 100644 --- a/src/database/models/Otp.ts +++ b/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 } diff --git a/src/routes/v2/auth.js b/src/routes/v2/auth.js index e404ae488..65bd7db01 100644 --- a/src/routes/v2/auth.js +++ b/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) diff --git a/src/routes/v2/authenticated/users.ts b/src/routes/v2/authenticated/users.ts index e8d6c956e..12b910a7a 100644 --- a/src/routes/v2/authenticated/users.ts +++ b/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, { diff --git a/src/services/identity/UsersService.ts b/src/services/identity/UsersService.ts index eae744e48..3444e3fae 100644 --- a/src/services/identity/UsersService.ts +++ b/src/services/identity/UsersService.ts @@ -212,6 +212,32 @@ 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 @@ -219,6 +245,7 @@ class UsersService { 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, }) } diff --git a/src/services/identity/__tests__/UsersService.spec.ts b/src/services/identity/__tests__/UsersService.spec.ts index 53c2dd44f..ad88eba0c 100644 --- a/src/services/identity/__tests__/UsersService.spec.ts +++ b/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 = {} + +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 = "someone@tech.gov.sg" 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" diff --git a/src/services/utilServices/AuthService.js b/src/services/utilServices/AuthService.js index fde329584..292ae28a1 100644 --- a/src/services/utilServices/AuthService.js +++ b/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), diff --git a/src/services/utilServices/RateLimiter.ts b/src/services/utilServices/RateLimiter.ts index 60ae16168..a051436fb 100644 --- a/src/services/utilServices/RateLimiter.ts +++ b/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({ diff --git a/src/utils/auth-utils.js b/src/utils/auth-utils.js index 460823425..6cee422e5 100644 --- a/src/utils/auth-utils.js +++ b/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, }