Add identifier and IP rate limits for OTP requests

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
 # ---------------------------------------------------------------------------

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

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",

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);

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;

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<string, number>;
+  let ipCounts: Map<string, number>;
+  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);
+  });
+});

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<OtpRateLimitState> {
+export async function checkOtpRateLimit(
+  identifier: string,
+  ip: string
+): Promise<OtpRateLimitState> {
+  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<OtpRateLimitStat
  * duration of an outage is the cheaper failure, and the error is logged so the
  * gap is visible rather than silent.
  */
-export async function rateLimitOtp(
-  req: Request,
-  res: Response,
-  next: NextFunction
-): Promise<void> {
-  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<boolean> {
+  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<void> {
+  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();
   }
 }