Merge pull request #4731 from verifywise-ai/4678-rate-limiter-verification-suite

4678 rate limiter verification suite

Author: MuhammadKhalilzadeh

.github/workflows/backend-checks.yml

@@ -87,6 +87,16 @@ jobs:
       - name: Run tests
         run: npm test
 
+      # Blocking. The limiters are the only thing standing between /users/login and
+      # unlimited password guesses, and `npm test` above cannot see them: it excludes
+      # tests/integration/, and a limiter is invisible to a unit test with its store
+      # mocked out. These three suites need no database and no environment, so they
+      # run here rather than in the Postgres job. The wiring suite is excluded
+      # because it builds the real app, which needs ENCRYPTION_KEY at module load —
+      # it runs in tenant-isolation-tests instead.
+      - name: Rate-limiter verification suite
+        run: npm run test:ratelimit
+
       - name: i18n audit (catch dictionary drift)
         run: npm run i18n:audit:strict
 
@@ -188,7 +198,7 @@ jobs:
           # globalSetup; it fails closed without it. CI-only value.
           DB_APP_PASSWORD: test-app-role-password-for-ci
         run: |
-          npm run test:integration -- --testPathPatterns='tests/integration/(workflow-audit-log|user-deletion-fks|reporting-rls-policies|report-run-visibility|framework-gap-workflow|report-scope-membership|report-scope-authorization|workflow-approval-gate|file-manager)\.test\.ts'
+          npm run test:integration -- --testPathPatterns='tests/integration/(workflow-audit-log|user-deletion-fks|reporting-rls-policies|report-run-visibility|framework-gap-workflow|report-scope-membership|report-scope-authorization|workflow-approval-gate|file-manager|rate-limiting/limiter-wiring)\.test\.ts'
 
       - name: Run deadline-summary smoke test
         env:

Servers/app.ts

@@ -1,7 +1,6 @@
 import express, { RequestHandler } from "express";
 import cors from "cors";
 import helmet from "helmet";
-import rateLimit from "express-rate-limit";
 import cookieParser from "cookie-parser";
 import { csrfProtection } from "./middleware/csrf.middleware";
 
@@ -120,7 +119,7 @@ import virtualKeyProxyRoutes from "./routes/virtualKeyProxy.route";
 import internalRoutes from "./routes/internal.route";
 import superAdminRoutes from "./routes/superAdmin.route";
 import { i18nMiddleware } from "./middleware/i18n.middleware";
-import { generalApiLimiter } from "./middleware/rateLimit.middleware";
+import { generalApiLimiter, healthCheckLimiter } from "./middleware/rateLimit.middleware";
 import { sequelize } from "./database/db";
 import redisClient from "./database/redis";
 import ssoConfigRoutes from "./routes/ssoConfig.route";
@@ -192,57 +191,45 @@ export function createApp(preRoutesMiddleware?: RequestHandler[]): express.Appli
 
   // Generous rate limiter for the health endpoint. Load-balancer probes are
   // still allowed, but the endpoint is capped to prevent abuse.
-  const nodeEnv = (process.env.NODE_ENV ?? "").trim().toLowerCase();
-  const isNonProduction = nodeEnv === "development" || nodeEnv === "test" || nodeEnv === "local";
-  app.get(
-    "/health",
-    rateLimit({
-      windowMs: 60 * 1000,
-      max: isNonProduction ? 100000 : 1000,
-      standardHeaders: true,
-      legacyHeaders: false,
-      message: "Too many health-check requests from this IP, please slow down",
-    }),
-    async (_req, res) => {
-      const AI_GATEWAY_URL = process.env.AI_GATEWAY_URL || "http://localhost:8100";
-      const checks: Record<string, { status: "ok" | "error"; error?: string }> = {};
+  app.get("/health", healthCheckLimiter, async (_req, res) => {
+    const AI_GATEWAY_URL = process.env.AI_GATEWAY_URL || "http://localhost:8100";
+    const checks: Record<string, { status: "ok" | "error"; error?: string }> = {};
 
-      try {
-        await sequelize.query("SELECT 1");
-        checks.database = { status: "ok" };
-      } catch (err: unknown) {
-        checks.database = { status: "error", error: (err as Error).message };
-      }
+    try {
+      await sequelize.query("SELECT 1");
+      checks.database = { status: "ok" };
+    } catch (err: unknown) {
+      checks.database = { status: "error", error: (err as Error).message };
+    }
 
-      try {
-        const pong = await redisClient.ping();
-        checks.redis =
-          pong === "PONG"
-            ? { status: "ok" }
-            : { status: "error", error: `Unexpected PING response: ${pong}` };
-      } catch (err: unknown) {
-        checks.redis = { status: "error", error: (err as Error).message };
-      }
+    try {
+      const pong = await redisClient.ping();
+      checks.redis =
+        pong === "PONG"
+          ? { status: "ok" }
+          : { status: "error", error: `Unexpected PING response: ${pong}` };
+    } catch (err: unknown) {
+      checks.redis = { status: "error", error: (err as Error).message };
+    }
 
+    try {
+      const controller = new AbortController();
+      const timeout = setTimeout(() => controller.abort(), 5000);
       try {
-        const controller = new AbortController();
-        const timeout = setTimeout(() => controller.abort(), 5000);
-        try {
-          const gwRes = await fetch(`${AI_GATEWAY_URL}/health`, { signal: controller.signal });
-          checks.ai_gateway = gwRes.ok
-            ? { status: "ok" }
-            : { status: "error", error: `HTTP ${gwRes.status}` };
-        } finally {
-          clearTimeout(timeout);
-        }
-      } catch (err: unknown) {
-        checks.ai_gateway = { status: "error", error: (err as Error).message };
+        const gwRes = await fetch(`${AI_GATEWAY_URL}/health`, { signal: controller.signal });
+        checks.ai_gateway = gwRes.ok
+          ? { status: "ok" }
+          : { status: "error", error: `HTTP ${gwRes.status}` };
+      } finally {
+        clearTimeout(timeout);
       }
+    } catch (err: unknown) {
+      checks.ai_gateway = { status: "error", error: (err as Error).message };
+    }
 
-      const allOk = Object.values(checks).every((c) => c.status === "ok");
-      res.status(allOk ? 200 : 503).json({ status: allOk ? "ok" : "degraded", checks });
-    },
-  );
+    const allOk = Object.values(checks).every((c) => c.status === "ok");
+    res.status(allOk ? 200 : 503).json({ status: allOk ? "ok" : "degraded", checks });
+  });
 
   // Track every request: metrics + access log (shipped to the central
   // observability stack when monitoring is enabled). Mounted after /health so

Servers/extensions/slack/slack.route.ts

@@ -13,7 +13,6 @@
  */
 
 import express from "express";
-import rateLimit from "express-rate-limit";
 import authenticateJWT from "../../middleware/auth.middleware";
 import { requireExtensionEnabled } from "../../middleware/requireExtensionEnabled.middleware";
 import {
@@ -32,20 +31,13 @@ router.use(requireExtensionEnabled("slack"));
 
 // Rate limit the OAuth-exchange endpoint so a leaked JWT can't spam
 // Slack's OAuth API on our behalf.
-const createWorkspaceLimiter = rateLimit({
-  windowMs: 60 * 60 * 1000,
-  max: 10,
-  message: {
-    error:
-      "Too many Slack workspace creation requests from this IP, please try again after an hour.",
-  },
-});
+import { slackWorkspaceCreateLimiter } from "../../middleware/rateLimit.middleware";
 
 // OAuth workspaces — exposed at /api/extensions/slack/oauth/workspaces to match
 // the extension's UI conventions. Handlers are shared with /api/slackWebhooks.
 router.get("/oauth/workspaces", getAllSlackWebhooks);
 router.get("/oauth/workspaces/:id", getSlackWebhookById);
-router.post("/oauth/workspaces", createWorkspaceLimiter, createNewSlackWebhook);
+router.post("/oauth/workspaces", slackWorkspaceCreateLimiter, createNewSlackWebhook);
 router.patch("/oauth/workspaces/:id", updateSlackWebhookById);
 router.delete("/oauth/workspaces/:id", deleteSlackWebhookById);
 

Servers/middleware/rateLimit.middleware.ts

@@ -4,13 +4,28 @@
  * Provides production-ready rate limiting for API endpoints to prevent abuse and DoS attacks.
  * Uses express-rate-limit with IPv6-safe IP normalization.
  *
- * Rate Limiters:
- * - fileOperationsLimiter: 100 requests/15min (for file uploads, downloads, deletions)
- * - generalApiLimiter: 300 requests/min per IP (loose global ceiling mounted
- *   ahead of all route mounts in app.ts; stricter per-route limiters still
- *   apply on top)
- * - authLimiter: 5 requests/15min (for login/register/reset to prevent brute force)
- * - tokenRefreshLimiter: 60 requests/15min (for automatic access-token refresh)
+ * Every limiter in the application is defined here and built through
+ * `createRateLimiter`, so they all share one contract: draft-6 `RateLimit-*`
+ * headers, no legacy `X-RateLimit-*`, and the canonical STATUS_CODE[429] body
+ * `{ message: "Too Many Requests", data: <limiter message> }`. Defining a limiter
+ * inline in a route file re-introduces express-rate-limit's defaults (legacy
+ * headers, a non-standard body) and silently breaks that contract — see
+ * tests/integration/rate-limiting/.
+ *
+ * Production limits:
+ * - generalApiLimiter: 300/min — loose global ceiling mounted ahead of all route
+ *   mounts in app.ts; stricter per-route limiters still apply on top
+ * - authLimiter: 5/15min — register, password reset, change password
+ * - loginLimiter: 5/min — login and login-microsoft
+ * - tokenRefreshLimiter: 60/15min — automatic access-token refresh
+ * - fileOperationsLimiter: 100/15min — file uploads, downloads, deletions
+ * - aiDetectionScanLimiter: 10/hour — expensive scans
+ * - mrmIngestionLimiter: 5000/15min, keyed by token — machine-to-machine push
+ * - webhookLimiter: 100/min — inbound signature-verified webhooks
+ * - passwordResetEmailLimiter / inviteEmailLimiter / invitationResendLimiter:
+ *   5/min each — outbound email
+ * - slackWebhookCreateLimiter / slackWorkspaceCreateLimiter: 10/hour each
+ * - healthCheckLimiter: 1000/min — probe endpoint
  *
  * The strict auth/refresh limits apply by default. They are relaxed ONLY when
  * NODE_ENV is an explicit dev/test value, so a single developer hammering
@@ -36,7 +51,7 @@ export const isNonProduction =
 /**
  * Rate limit configuration with time window and request limits
  */
-interface RateLimitConfig {
+export interface RateLimitConfig {
   windowMinutes: number;
   maxRequests: number;
   message: string;
@@ -46,9 +61,17 @@ interface RateLimitConfig {
 }
 
 /**
- * Predefined rate limit configurations for different endpoint types
+ * Builds the rate limit configurations for every endpoint type.
+ *
+ * Parameterised on `relaxed` rather than reading `isNonProduction` directly so the
+ * PRODUCTION limits can be built inside a test process (which necessarily runs with
+ * NODE_ENV=test). Without this, a brute-force test would silently exercise the
+ * relaxed dev limits — 1000 auth attempts instead of 5 — and pass without ever
+ * reaching the limit it claims to verify.
+ *
+ * @param relaxed - true to apply the loosened dev/test limits, false for production
  */
-const RATE_LIMIT_CONFIGS: Record<string, RateLimitConfig> = {
+export const buildRateLimitConfigs = (relaxed: boolean): Record<string, RateLimitConfig> => ({
   fileOperations: {
     windowMinutes: 15,
     maxRequests: 100,
@@ -61,22 +84,22 @@ const RATE_LIMIT_CONFIGS: Record<string, RateLimitConfig> = {
   // a developer hammering localhost from one IP is not locked out.
   generalApi: {
     windowMinutes: 1,
-    maxRequests: isNonProduction ? 100000 : 300,
+    maxRequests: relaxed ? 100000 : 300,
     message: "Too many requests from this IP, please slow down and retry",
   },
   auth: {
     windowMinutes: 15,
     // Strict by default to prevent brute force; relaxed only in explicit
     // dev/test so a single developer on one localhost IP is not locked out.
-    maxRequests: isNonProduction ? 1000 : 5,
+    maxRequests: relaxed ? 1000 : 5,
     message: "Too many authentication attempts from this IP, please try again after 15 minutes",
   },
   // Token refresh happens automatically and legitimately many times in a normal
   // session, so it gets its own generous limit rather than sharing the strict
   // brute-force limiter. It still requires a valid refresh-token cookie.
   tokenRefresh: {
     windowMinutes: 15,
-    maxRequests: isNonProduction ? 1000 : 60,
+    maxRequests: relaxed ? 1000 : 60,
     message: "Too many token refresh attempts from this IP, please try again after 15 minutes",
   },
   aiDetectionScan: {
@@ -93,7 +116,7 @@ const RATE_LIMIT_CONFIGS: Record<string, RateLimitConfig> = {
   // runaway token must not 429 every other tenant on the same egress IP.
   mrmIngestion: {
     windowMinutes: 15,
-    maxRequests: isNonProduction ? 100000 : 5000,
+    maxRequests: relaxed ? 100000 : 5000,
     message: "Too many metric ingestion requests for this token, please slow down and retry",
     keyGenerator: (req) => {
       const tokenId = (req as { mrmIngestionToken?: { tokenId?: number } }).mrmIngestionToken
@@ -106,10 +129,61 @@ const RATE_LIMIT_CONFIGS: Record<string, RateLimitConfig> = {
   },
   webhook: {
     windowMinutes: 1,
-    maxRequests: isNonProduction ? 100000 : 100,
+    maxRequests: relaxed ? 100000 : 100,
     message: "Too many webhook requests from this IP, please slow down and retry",
   },
-};
+  // Brute-force control on the actual login endpoints. Separate from `auth`
+  // (which guards register/reset/change-password) because the window is shorter:
+  // a credential-stuffing run is fast, and a one-minute lockout costs a real user
+  // far less than fifteen. Relaxed in explicit dev/test so the E2E suite's
+  // repeated UI logins from one localhost IP are not blocked.
+  login: {
+    windowMinutes: 1,
+    maxRequests: relaxed ? 1000 : 5,
+    message: "Too many login attempts from this IP, please try again after a minute",
+  },
+  // Outbound-email endpoints. These are not relaxed in dev/test: the cost being
+  // controlled is sending mail to a third party, which is just as real locally.
+  passwordResetEmail: {
+    windowMinutes: 1,
+    maxRequests: 5,
+    message: "Too many password reset requests from this IP, please try again later",
+  },
+  inviteEmail: {
+    windowMinutes: 1,
+    maxRequests: 5,
+    message: "Too many invite requests from this IP, please try again later",
+  },
+  invitationResend: {
+    windowMinutes: 1,
+    maxRequests: 5,
+    message: "Too many resend requests from this IP, please try again later",
+  },
+  slackWebhookCreate: {
+    windowMinutes: 60,
+    maxRequests: 10,
+    message: "Too many webhook creation requests from this IP, please try again after an hour",
+  },
+  slackWorkspaceCreate: {
+    windowMinutes: 60,
+    maxRequests: 10,
+    message:
+      "Too many Slack workspace creation requests from this IP, please try again after an hour",
+  },
+  // Load-balancer and uptime probes are legitimately frequent, so this ceiling is
+  // generous — it exists to stop /health being used as a free amplification
+  // endpoint, not to throttle monitoring.
+  healthCheck: {
+    windowMinutes: 1,
+    maxRequests: relaxed ? 100000 : 1000,
+    message: "Too many health-check requests from this IP, please slow down",
+  },
+});
+
+/**
+ * The configurations the running process actually uses, resolved once from NODE_ENV.
+ */
+export const RATE_LIMIT_CONFIGS = buildRateLimitConfigs(isNonProduction);
 
 /**
  * Creates a standardized rate limit error handler
@@ -189,3 +263,30 @@ export const mrmIngestionLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.mrmInges
  * bounded so a misconfigured or malicious sender cannot flood the system.
  */
 export const webhookLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.webhook);
+
+/**
+ * Brute-force limiter for POST /users/login and /users/login-microsoft.
+ * Tighter window than authLimiter because credential stuffing is fast and a
+ * one-minute lockout is cheap for a legitimate user who mistyped a password.
+ */
+export const loginLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.login);
+
+/** Limiter for the password-reset email endpoint (POST /mail/reset-password). */
+export const passwordResetEmailLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.passwordResetEmail);
+
+/** Limiter for the user-invite email endpoint (POST /mail/invite). */
+export const inviteEmailLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.inviteEmail);
+
+/** Limiter for re-sending an invitation (POST /invitations/:id/resend). */
+export const invitationResendLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.invitationResend);
+
+/** Limiter for creating a Slack webhook (POST /slack-webhooks). */
+export const slackWebhookCreateLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.slackWebhookCreate);
+
+/** Limiter for connecting a Slack workspace (POST /extensions/slack/oauth/workspaces). */
+export const slackWorkspaceCreateLimiter = createRateLimiter(
+  RATE_LIMIT_CONFIGS.slackWorkspaceCreate,
+);
+
+/** Generous limiter for the GET /health probe endpoint. */
+export const healthCheckLimiter = createRateLimiter(RATE_LIMIT_CONFIGS.healthCheck);

Servers/package.json

@@ -6,6 +6,7 @@
     "test:unit": "jest --testPathIgnorePatterns=/tests/integration/ --testPathIgnorePatterns=/helpers/ --detectOpenHandles --forceExit",
     "test:coverage": "jest --coverage --testPathIgnorePatterns=/tests/integration/ --testPathIgnorePatterns=/helpers/ --detectOpenHandles --forceExit",
     "test:smoke": "jest --config jest.config.js --globalSetup=\"<rootDir>/tests/integration/globalSetup.js\" --testPathPatterns=deadline-summary --runInBand",
+    "test:ratelimit": "jest --config jest.config.js --testMatch=\"**/tests/integration/rate-limiting/**/*.test.ts\" --testPathIgnorePatterns=/helpers/ --testPathIgnorePatterns=limiter-wiring --runInBand --forceExit",
     "smoke:api-contract": "ts-node --transpile-only scripts/apiContractSmoke.ts",
     "generate:enum-manifest": "ts-node --transpile-only scripts/generateEnumManifest.ts",
     "check:enum-drift": "npm run generate:enum-manifest && ts-node --transpile-only scripts/checkEnumLabelDrift.ts",

Servers/routes/__tests__/integration/user.route.test.ts

@@ -48,15 +48,16 @@ jest.mock("../../../middleware/accessControl.middleware", () => ({
   default: jest.fn(() => (_req: any, _res: any, next: any) => next()),
 }));
 
+// Every limiter this router mounts must be stubbed here. They all now come from
+// rateLimit.middleware (loginLimiter used to be declared inline in user.route.ts and
+// was covered by an express-rate-limit mock), so a limiter missing from this list
+// mounts `undefined` and the whole suite fails to load.
 jest.mock("../../../middleware/rateLimit.middleware", () => ({
   authLimiter: jest.fn((_req: any, _res: any, next: any) => next()),
+  loginLimiter: jest.fn((_req: any, _res: any, next: any) => next()),
   tokenRefreshLimiter: jest.fn((_req: any, _res: any, next: any) => next()),
 }));
 
-jest.mock("express-rate-limit", () =>
-  jest.fn(() => (_req: unknown, _res: unknown, next: () => void) => next()),
-);
-
 import userRoutes from "../../user.route";
 
 function createUserTestApp(): express.Application {