From f3170beac431b7ae72f9bb9e5298b60a14b3308e Mon Sep 17 00:00:00 2001 From: therealmercy-dev Date: Thu, 22 Oct 2026 10:55:58 +0000 Subject: [PATCH] fix: rate limit OTP requests by identifier and IP --- server/.env.example | 6 + server/sql/002_webhooks_and_rate_limit.sql | 7 +- server/src/config/validateEnv.ts | 2 + .../db/migrations/016_otp_rate_limit_ip.sql | 21 +++ .../migrations/down/016_otp_rate_limit_ip.sql | 4 + server/src/middleware/rateLimitOtp.test.ts | 103 +++++++++++++ server/src/middleware/rateLimitOtp.ts | 136 ++++++++++++------ server/src/routes/auth.recover.test.ts | 11 ++ server/src/routes/auth.ts | 5 +- 9 files changed, 248 insertions(+), 47 deletions(-) create mode 100644 server/src/db/migrations/016_otp_rate_limit_ip.sql create mode 100644 server/src/db/migrations/down/016_otp_rate_limit_ip.sql create mode 100644 server/src/middleware/rateLimitOtp.test.ts diff --git a/server/.env.example b/server/.env.example index 8bc1131..6c0e20b 100644 --- a/server/.env.example +++ b/server/.env.example @@ -91,6 +91,12 @@ PAYSTACK_PREFERRED_BANK=wema-bank # Must be a non-empty string of at least 20 characters. TERMII_API_KEY=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx +# [OPTIONAL] OTP requests allowed per identifier and IP within the sliding window. Defaults to 5. +OTP_RATE_LIMIT_MAX_REQUESTS=5 + +# [OPTIONAL] Sliding-window length for OTP request limits, in minutes. Defaults to 60. +OTP_RATE_LIMIT_WINDOW_MINUTES=60 + # --------------------------------------------------------------------------- # Auth & Encryption # --------------------------------------------------------------------------- diff --git a/server/sql/002_webhooks_and_rate_limit.sql b/server/sql/002_webhooks_and_rate_limit.sql index 4f2c1bb..0a0809b 100644 --- a/server/sql/002_webhooks_and_rate_limit.sql +++ b/server/sql/002_webhooks_and_rate_limit.sql @@ -47,13 +47,16 @@ CREATE INDEX IF NOT EXISTS idx_payment_events_received ON payment_events (receiv CREATE TABLE IF NOT EXISTS otp_requests ( id BIGSERIAL PRIMARY KEY, - phone VARCHAR(32) NOT NULL, + phone VARCHAR(320) NOT NULL, + ip VARCHAR(64), requested_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); --- The limiter's only query is "count rows for this phone since T". +-- The limiter counts rows by phone and IP over the same sliding window. CREATE INDEX IF NOT EXISTS idx_otp_requests_phone_time ON otp_requests (phone, requested_at DESC); +CREATE INDEX IF NOT EXISTS idx_otp_requests_ip_time + ON otp_requests (ip, requested_at DESC); -- --------------------------------------------------------------------------- -- Columns the deposit path expects diff --git a/server/src/config/validateEnv.ts b/server/src/config/validateEnv.ts index 612fc13..cfd214b 100644 --- a/server/src/config/validateEnv.ts +++ b/server/src/config/validateEnv.ts @@ -41,6 +41,8 @@ export const OPTIONAL_ENV_VARS = [ "MAX_TRADES_PER_HOUR", "MAX_DEPOSITS_PER_DAY", "MAX_WITHDRAWALS_PER_DAY", + "OTP_RATE_LIMIT_MAX_REQUESTS", + "OTP_RATE_LIMIT_WINDOW_MINUTES", // Storage — required in production when STORAGE_PROVIDER=s3 (default) "STORAGE_PROVIDER", "S3_BUCKET", diff --git a/server/src/db/migrations/016_otp_rate_limit_ip.sql b/server/src/db/migrations/016_otp_rate_limit_ip.sql new file mode 100644 index 0000000..d17592f --- /dev/null +++ b/server/src/db/migrations/016_otp_rate_limit_ip.sql @@ -0,0 +1,21 @@ +-- Track OTP requests by source IP as well as identifier. +-- Existing records remain valid and are excluded from IP counts. + +CREATE TABLE IF NOT EXISTS otp_requests ( + id BIGSERIAL PRIMARY KEY, + phone VARCHAR(320) NOT NULL, + ip VARCHAR(64), + requested_at TIMESTAMPTZ NOT NULL DEFAULT NOW() +); + +ALTER TABLE otp_requests + ADD COLUMN IF NOT EXISTS ip VARCHAR(64); + +ALTER TABLE otp_requests + ALTER COLUMN phone TYPE VARCHAR(320); + +CREATE INDEX IF NOT EXISTS idx_otp_requests_phone_time + ON otp_requests (phone, requested_at DESC); + +CREATE INDEX IF NOT EXISTS idx_otp_requests_ip_time + ON otp_requests (ip, requested_at DESC); \ No newline at end of file diff --git a/server/src/db/migrations/down/016_otp_rate_limit_ip.sql b/server/src/db/migrations/down/016_otp_rate_limit_ip.sql new file mode 100644 index 0000000..bb5e28f --- /dev/null +++ b/server/src/db/migrations/down/016_otp_rate_limit_ip.sql @@ -0,0 +1,4 @@ +DROP INDEX IF EXISTS idx_otp_requests_ip_time; + +ALTER TABLE otp_requests + DROP COLUMN IF EXISTS ip; \ No newline at end of file diff --git a/server/src/middleware/rateLimitOtp.test.ts b/server/src/middleware/rateLimitOtp.test.ts new file mode 100644 index 0000000..473ba7c --- /dev/null +++ b/server/src/middleware/rateLimitOtp.test.ts @@ -0,0 +1,103 @@ +import express from "express"; +import request from "supertest"; + +const mockClientQuery = jest.fn(); +const mockRelease = jest.fn(); + +jest.mock("../db/pool", () => ({ + pool: { + connect: async () => ({ + query: (...args: unknown[]) => mockClientQuery(...args), + release: () => mockRelease(), + }), + }, +})); + +import { rateLimitOtp } from "./rateLimitOtp"; + +describe("rateLimitOtp", () => { + const originalMaxRequests = process.env["OTP_RATE_LIMIT_MAX_REQUESTS"]; + const originalWindowMinutes = process.env["OTP_RATE_LIMIT_WINDOW_MINUTES"]; + let identifierCounts: Map; + let ipCounts: Map; + let app: express.Express; + + beforeEach(() => { + process.env["OTP_RATE_LIMIT_MAX_REQUESTS"] = "2"; + process.env["OTP_RATE_LIMIT_WINDOW_MINUTES"] = "60"; + identifierCounts = new Map(); + ipCounts = new Map(); + mockClientQuery.mockReset(); + mockRelease.mockReset(); + mockClientQuery.mockImplementation(async (sql: string, params?: unknown[]) => { + if (sql.includes("INSERT INTO otp_requests")) { + const [identifier, ip] = params as [string, string]; + identifierCounts.set(identifier, (identifierCounts.get(identifier) ?? 0) + 1); + ipCounts.set(ip, (ipCounts.get(ip) ?? 0) + 1); + } + + if (sql.includes("COUNT(*) FILTER")) { + const [identifier, ip] = params as [string, string, string]; + const oldest = new Date(Date.now() - 1000).toISOString(); + return { + rows: [{ + identifier_attempts: String(identifierCounts.get(identifier) ?? 0), + identifier_oldest: oldest, + ip_attempts: String(ipCounts.get(ip) ?? 0), + ip_oldest: oldest, + }], + }; + } + + return { rows: [] }; + }); + + app = express(); + app.set("trust proxy", true); + app.use(express.json()); + app.post("/request-otp", rateLimitOtp, (_req, res) => res.sendStatus(200)); + }); + + afterAll(() => { + if (originalMaxRequests === undefined) { + delete process.env["OTP_RATE_LIMIT_MAX_REQUESTS"]; + } else { + process.env["OTP_RATE_LIMIT_MAX_REQUESTS"] = originalMaxRequests; + } + if (originalWindowMinutes === undefined) { + delete process.env["OTP_RATE_LIMIT_WINDOW_MINUTES"]; + } else { + process.env["OTP_RATE_LIMIT_WINDOW_MINUTES"] = originalWindowMinutes; + } + }); + + it("rejects requests over the identifier limit with Retry-After", async () => { + const sendRequest = () => + request(app) + .post("/request-otp") + .set("x-forwarded-for", "203.0.113.10") + .send({ phone: "+2348123456789" }); + + expect((await sendRequest()).status).toBe(200); + expect((await sendRequest()).status).toBe(200); + + const blocked = await sendRequest(); + expect(blocked.status).toBe(429); + expect(Number(blocked.headers["retry-after"])).toBeGreaterThan(0); + }); + + it("rejects requests over the IP limit even when identifiers differ", async () => { + const sendRequest = (phone: string) => + request(app) + .post("/request-otp") + .set("x-forwarded-for", "203.0.113.11") + .send({ phone }); + + expect((await sendRequest("+2348123456781")).status).toBe(200); + expect((await sendRequest("+2348123456782")).status).toBe(200); + + const blocked = await sendRequest("+2348123456783"); + expect(blocked.status).toBe(429); + expect(Number(blocked.headers["retry-after"])).toBeGreaterThan(0); + }); +}); \ No newline at end of file diff --git a/server/src/middleware/rateLimitOtp.ts b/server/src/middleware/rateLimitOtp.ts index 641a192..5cbd123 100644 --- a/server/src/middleware/rateLimitOtp.ts +++ b/server/src/middleware/rateLimitOtp.ts @@ -3,7 +3,7 @@ import { Request, Response, NextFunction } from "express"; import { pool } from "../db/pool"; /** - * Per-phone rate limit for OTP requests (Issue #7). + * Per-identifier and per-IP rate limit for OTP requests. * * # Why this is database-backed rather than in-memory * @@ -13,19 +13,28 @@ import { pool } from "../db/pool"; * request costs real money at Termii, so a limiter that silently stops * counting is worse than none: it reads as protection while providing none. * - * The limit is keyed on **phone number, not IP**. The cost and the abuse both - * attach to the number: one attacker rotating IPs to SMS-bomb a single victim - * is the case that matters, and an IP limit does nothing about it. It also - * avoids punishing everyone behind a shared NAT, which is common on the mobile - * networks this platform targets. + * Identifier and IP budgets are checked independently. This prevents both + * rotating IPs to target one phone and rotating phone numbers from one IP. */ -/** Requests allowed per phone number per window. */ +/** Requests allowed per identifier and IP per window. */ export const OTP_MAX_REQUESTS = 5; /** Window length in minutes. */ export const OTP_WINDOW_MINUTES = 60; +function positiveEnvInt(name: string, fallback: number): number { + const value = Number(process.env[name]); + return Number.isSafeInteger(value) && value > 0 ? value : fallback; +} + +function getOtpRateLimitConfig() { + return { + maxRequests: positiveEnvInt("OTP_RATE_LIMIT_MAX_REQUESTS", OTP_MAX_REQUESTS), + windowMinutes: positiveEnvInt("OTP_RATE_LIMIT_WINDOW_MINUTES", OTP_WINDOW_MINUTES), + }; +} + export interface OtpRateLimitState { allowed: boolean; remaining: number; @@ -45,46 +54,71 @@ export interface OtpRateLimitState { * about to be rejected; otherwise a caller past the limit could keep retrying * for free and the window would never fill. */ -export async function checkOtpRateLimit(phone: string): Promise { +export async function checkOtpRateLimit( + identifier: string, + ip: string +): Promise { + const { maxRequests, windowMinutes } = getOtpRateLimitConfig(); const client = await pool.connect(); try { await client.query("BEGIN"); + // Serialize requests sharing either dimension so concurrent sends cannot + // all pass by observing the same pre-insert count. + const lockKeys = [`otp:identifier:${identifier}`, `otp:ip:${ip}`].sort(); + for (const key of lockKeys) { + await client.query("SELECT pg_advisory_xact_lock(hashtextextended($1, 0))", [key]); + } + await client.query( - `INSERT INTO otp_requests (phone, requested_at) VALUES ($1, NOW())`, - [phone] + `INSERT INTO otp_requests (phone, ip, requested_at) VALUES ($1, $2, NOW())`, + [identifier, ip] ); - const { rows } = await client.query<{ attempts: string; oldest: string | null }>( - `SELECT COUNT(*)::text AS attempts, - MIN(requested_at)::text AS oldest + const { rows } = await client.query<{ + identifier_attempts: string; + identifier_oldest: string | null; + ip_attempts: string; + ip_oldest: string | null; + }>( + `SELECT COUNT(*) FILTER (WHERE phone = $1)::text AS identifier_attempts, + MIN(requested_at) FILTER (WHERE phone = $1)::text AS identifier_oldest, + COUNT(*) FILTER (WHERE ip = $2)::text AS ip_attempts, + MIN(requested_at) FILTER (WHERE ip = $2)::text AS ip_oldest FROM otp_requests - WHERE phone = $1 - AND requested_at > NOW() - ($2 || ' minutes')::interval`, - [phone, String(OTP_WINDOW_MINUTES)] + WHERE requested_at > NOW() - ($3 || ' minutes')::interval + AND (phone = $1 OR ip = $2)`, + [identifier, ip, String(windowMinutes)] ); await client.query("COMMIT"); - const attempts = Number(rows[0]?.attempts ?? 0); - const oldest = rows[0]?.oldest ? new Date(rows[0].oldest) : null; + const identifierAttempts = Number(rows[0]?.identifier_attempts ?? 0); + const ipAttempts = Number(rows[0]?.ip_attempts ?? 0); + const identifierOldest = rows[0]?.identifier_oldest + ? new Date(rows[0].identifier_oldest) + : null; + const ipOldest = rows[0]?.ip_oldest ? new Date(rows[0].ip_oldest) : null; - // Capacity frees up when the oldest attempt in the window ages out, not - // at a fixed boundary. - const retryAfterSeconds = + const retryAfterFor = (oldest: Date | null): number => oldest === null - ? OTP_WINDOW_MINUTES * 60 + ? windowMinutes * 60 : Math.max( 1, - Math.ceil( - (oldest.getTime() + OTP_WINDOW_MINUTES * 60_000 - Date.now()) / 1000 - ) + Math.ceil((oldest.getTime() + windowMinutes * 60_000 - Date.now()) / 1000) ); + const blockedRetryAfters = [ + ...(identifierAttempts > maxRequests ? [retryAfterFor(identifierOldest)] : []), + ...(ipAttempts > maxRequests ? [retryAfterFor(ipOldest)] : []), + ]; + const retryAfterSeconds = blockedRetryAfters.length + ? Math.max(...blockedRetryAfters) + : windowMinutes * 60; return { - allowed: attempts <= OTP_MAX_REQUESTS, - remaining: Math.max(0, OTP_MAX_REQUESTS - attempts), + allowed: identifierAttempts <= maxRequests && ipAttempts <= maxRequests, + remaining: Math.max(0, Math.min(maxRequests - identifierAttempts, maxRequests - ipAttempts)), retryAfterSeconds, }; } catch (err) { @@ -104,24 +138,17 @@ export async function checkOtpRateLimit(phone: string): Promise { - const phone = (req.body as { phone?: string } | undefined)?.phone; - - // No phone means the request will fail validation anyway; let the validator - // produce the better error rather than reporting a rate limit. - if (!phone) { - next(); - return; - } +export async function enforceOtpRateLimit( + identifier: string, + ip: string, + res: Response +): Promise { + const { maxRequests } = getOtpRateLimitConfig(); try { - const state = await checkOtpRateLimit(phone); + const state = await checkOtpRateLimit(identifier, ip); - res.setHeader("X-RateLimit-Limit", String(OTP_MAX_REQUESTS)); + res.setHeader("X-RateLimit-Limit", String(maxRequests)); res.setHeader("X-RateLimit-Remaining", String(state.remaining)); if (!state.allowed) { @@ -131,12 +158,33 @@ export async function rateLimitOtp( state.retryAfterSeconds / 60 )} minute(s).`, }); - return; + return false; } - next(); + return true; } catch (err) { console.error("[rateLimitOtp] Rate limit check failed:", (err as Error).message); + return true; + } +} + +export async function rateLimitOtp( + req: Request, + res: Response, + next: NextFunction +): Promise { + const body = req.body as { phone?: unknown; email?: unknown } | undefined; + const rawIdentifier = body?.phone ?? body?.email; + const identifier = typeof rawIdentifier === "string" ? rawIdentifier.trim() : ""; + + // Validation should produce the useful error when no identifier is present. + if (!identifier) { + next(); + return; + } + + const ip = req.ip ?? req.socket.remoteAddress ?? "unknown"; + if (await enforceOtpRateLimit(identifier, ip, res)) { next(); } } diff --git a/server/src/routes/auth.recover.test.ts b/server/src/routes/auth.recover.test.ts index c7cf74f..1651a52 100644 --- a/server/src/routes/auth.recover.test.ts +++ b/server/src/routes/auth.recover.test.ts @@ -31,11 +31,17 @@ jest.mock("../services/recoveryCodes", () => ({ redeemRecoveryCode: jest.fn(), })); +jest.mock("../middleware/rateLimitOtp", () => ({ + rateLimitOtp: (_req: unknown, _res: unknown, next: () => void) => next(), + enforceOtpRateLimit: jest.fn().mockResolvedValue(true), +})); + // Mock fetch for Termii OTP send const mockFetch = jest.fn(); global.fetch = mockFetch as unknown as typeof fetch; import authRouter from "./auth"; +import { enforceOtpRateLimit } from "../middleware/rateLimitOtp"; const app = express(); app.use(express.json()); @@ -77,6 +83,11 @@ describe("POST /api/v1/auth/recover/change-phone", () => { .send({ token: validRecoveryToken, newPhone }); expect(res.status).toBe(200); + expect(enforceOtpRateLimit).toHaveBeenCalledWith( + newPhone, + expect.any(String), + expect.anything() + ); // Does NOT return a session token expect(res.body.token).toBeUndefined(); expect(res.body.requiresVerification).toBe(true); diff --git a/server/src/routes/auth.ts b/server/src/routes/auth.ts index 91cbf12..4942423 100644 --- a/server/src/routes/auth.ts +++ b/server/src/routes/auth.ts @@ -1,6 +1,6 @@ import { Router } from "express"; -import { rateLimitOtp } from "../middleware/rateLimitOtp"; +import { enforceOtpRateLimit, rateLimitOtp } from "../middleware/rateLimitOtp"; import { authenticate, AuthenticatedRequest } from "../middleware/authenticate"; import { isRecoveryLockedOut, @@ -461,6 +461,9 @@ router.post( return; } + const ip = req.ip ?? req.socket.remoteAddress ?? "unknown"; + if (!(await enforceOtpRateLimit(newPhone, ip, res))) return; + // Update phone number and bump token version await pool.query( `UPDATE users SET phone = $1, token_version = token_version + 1, updated_at = NOW() WHERE id = $2`,