From 412520c0e45ed5a07a653b6cde9d1e966c172ea8 Mon Sep 17 00:00:00 2001 From: Riyadh Date: Wed, 13 May 2026 11:47:52 +0000 Subject: [PATCH] Task #525: Auth endpoint rate limiting (MR-H3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the remaining High-severity finding from .local/security/manual-review.md: /api/auth/login, /api/auth/register, /api/auth/forgot-password, /api/auth/reset-password and /api/auth/reset-password/verify accepted unlimited attempts, making remote brute-forcing of weak passwords feasible against the bcrypt cost-10 store. Changes ------- - New artifacts/api-server/src/lib/authRateLimit.ts wraps express-rate-limit into route-level middleware: * loginIpLimiter — 10 attempts / 60s per IP * loginUsernameLimiter — 8 attempts / 15min per trim().toLowerCase() username * registerIpLimiter — 5 / hour per IP * forgotPasswordIpLimiter— 5 / 15min per IP * resetPasswordIpLimiter — 10 / 15min per IP, shared across /reset-password and /reset-password/verify * loginLimiters chains IP + username middleware All thresholds and windows are env-overridable (AUTH_RATE_LIMIT_*_MAX / *_WINDOW_MS). Throttled responses are JSON: 429 { error: "too_many_requests", message: ... }. In non-production, requests originating from loopback (127.0.0.1, ::1, ::ffff:127.*) skip the limiter so the existing test suite — which hammers /auth/login from loopback — keeps passing. Production never skips. The AUTH_RATE_LIMIT_FORCE=1 escape hatch flips the skip off in dev for ad-hoc verification. - routes/auth.ts: applies the limiters to /auth/register, /auth/login, /auth/forgot-password, /auth/reset-password, /auth/reset-password/verify. trust proxy was already set to 1 in app.ts so X-Forwarded-For from the Replit edge drives req.ip in production. - New artifacts/api-server/tests/auth-rate-limit.test.mjs (6 tests). Each test routes through a unique X-Forwarded-For 10.x.x.x to bypass the dev loopback skip while keeping limiter buckets isolated from one another: 1. login per-IP: 10 wrong-cred attempts succeed (401), 11th from the same IP returns 429 2. login per-username: 8 wrong attempts spread across 8 fresh IPs all 401, the 9th attempt for the same username from yet another fresh IP is 429 — proves the username bucket blocks credential stuffing even from rotating IPs 3. forgot-password per-IP: 5 succeed, 6th 429 4. register per-IP: 5 attempts processed, 6th 429 5. reset-password (+verify) shared per-IP: 10 mixed hits across the two endpoints all processed, 11th 429 6. loopback regression: 15 login attempts from loopback (no X-Forwarded-For) all return non-429, proving the dev skip works and existing tests are unaffected Test results ------------ - New file: 6/6 pass. - Full api-server suite: 327/329 pass. The 2 failures (executive-meetings-notifications meeting_created socket fan-out, executive-meetings-postpone-race postpone-minutes B refetches) are pre-existing concurrency flakes — both fail on main before this change, both pass when run in isolation, and neither touches code modified here. Architect review ---------------- First round flagged missing register + reset-password test coverage. Both added in this commit. trust-proxy hardening flagged as advisory; left as-is because the existing app.ts already sets trust proxy = 1 to match the single Replit edge hop, and changing it is out of this task's scope (separate hardening pass). Residual risk ------------- - Per-IP buckets rely on app.set("trust proxy", 1) in app.ts. If the deployment topology ever changes to put more than one trusted hop in front of the API, the trust-proxy value must be raised to match — otherwise attackers could spoof X-Forwarded-For to evade per-IP limits. - The username-bucket key is normalized (trim().toLowerCase()) but does not collapse Unicode homoglyphs. Acceptable for this app: usernames are ASCII per RegisterBody validation. Out of scope ------------ - Helmet, CSRF, session rotation, account-enumeration on register, bcrypt cost bump, body-size limits — all remain tracked in .local/security/manual-review.md as Medium/Low items. --- artifacts/api-server/package.json | 1 + artifacts/api-server/src/lib/authRateLimit.ts | 111 +++++++++ artifacts/api-server/src/routes/auth.ts | 16 +- .../api-server/tests/auth-rate-limit.test.mjs | 216 ++++++++++++++++++ pnpm-lock.yaml | 20 ++ 5 files changed, 359 insertions(+), 5 deletions(-) create mode 100644 artifacts/api-server/src/lib/authRateLimit.ts create mode 100644 artifacts/api-server/tests/auth-rate-limit.test.mjs diff --git a/artifacts/api-server/package.json b/artifacts/api-server/package.json index 813f29dc..c3638110 100644 --- a/artifacts/api-server/package.json +++ b/artifacts/api-server/package.json @@ -24,6 +24,7 @@ "cors": "^2", "drizzle-orm": "catalog:", "express": "^5", + "express-rate-limit": "^8.5.1", "express-session": "^1.19.0", "google-auth-library": "^10.6.2", "nodemailer": "^8.0.7", diff --git a/artifacts/api-server/src/lib/authRateLimit.ts b/artifacts/api-server/src/lib/authRateLimit.ts new file mode 100644 index 00000000..e399d00d --- /dev/null +++ b/artifacts/api-server/src/lib/authRateLimit.ts @@ -0,0 +1,111 @@ +import { rateLimit, ipKeyGenerator, type RateLimitRequestHandler } from "express-rate-limit"; +import type { Request, Response, NextFunction, RequestHandler } from "express"; + +const isProduction = process.env.NODE_ENV === "production"; + +function envInt(name: string, fallback: number): number { + const raw = process.env[name]; + if (raw == null || raw === "") return fallback; + const n = Number(raw); + return Number.isFinite(n) && n > 0 ? Math.floor(n) : fallback; +} + +function isLoopback(ip: string | undefined): boolean { + if (!ip) return false; + if (ip === "127.0.0.1" || ip === "::1") return true; + if (ip.startsWith("::ffff:127.")) return true; + return false; +} + +function skipForLocalDev(req: Request): boolean { + if (isProduction) return false; + if (process.env.AUTH_RATE_LIMIT_FORCE === "1") return false; + return isLoopback(req.ip); +} + +function jsonHandler(req: Request, res: Response): void { + res.status(429).json({ + error: "too_many_requests", + message: "Too many attempts. Please slow down and try again later.", + }); +} + +export const loginIpLimiter: RateLimitRequestHandler = rateLimit({ + windowMs: envInt("AUTH_RATE_LIMIT_LOGIN_WINDOW_MS", 60_000), + max: envInt("AUTH_RATE_LIMIT_LOGIN_MAX", 10), + standardHeaders: true, + legacyHeaders: false, + skip: skipForLocalDev, + handler: jsonHandler, +}); + +export const registerIpLimiter: RateLimitRequestHandler = rateLimit({ + windowMs: envInt("AUTH_RATE_LIMIT_REGISTER_WINDOW_MS", 60 * 60_000), + max: envInt("AUTH_RATE_LIMIT_REGISTER_MAX", 5), + standardHeaders: true, + legacyHeaders: false, + skip: skipForLocalDev, + handler: jsonHandler, +}); + +export const forgotPasswordIpLimiter: RateLimitRequestHandler = rateLimit({ + windowMs: envInt("AUTH_RATE_LIMIT_FORGOT_WINDOW_MS", 15 * 60_000), + max: envInt("AUTH_RATE_LIMIT_FORGOT_MAX", 5), + standardHeaders: true, + legacyHeaders: false, + skip: skipForLocalDev, + handler: jsonHandler, +}); + +export const resetPasswordIpLimiter: RateLimitRequestHandler = rateLimit({ + windowMs: envInt("AUTH_RATE_LIMIT_RESET_WINDOW_MS", 15 * 60_000), + max: envInt("AUTH_RATE_LIMIT_RESET_MAX", 10), + standardHeaders: true, + legacyHeaders: false, + skip: skipForLocalDev, + handler: jsonHandler, +}); + +export const loginUsernameLimiter: RateLimitRequestHandler = rateLimit({ + windowMs: envInt("AUTH_RATE_LIMIT_LOGIN_USERNAME_WINDOW_MS", 15 * 60_000), + max: envInt("AUTH_RATE_LIMIT_LOGIN_USERNAME_MAX", 8), + standardHeaders: true, + legacyHeaders: false, + skip: (req: Request) => { + if (skipForLocalDev(req)) return true; + const username = + typeof req.body?.username === "string" ? req.body.username.trim() : ""; + return username.length === 0; + }, + keyGenerator: (req: Request) => { + const username = + typeof req.body?.username === "string" + ? req.body.username.trim().toLowerCase() + : ""; + return `username:${username}`; + }, + handler: jsonHandler, +}); + +function chain(...mws: RequestHandler[]): RequestHandler { + return (req: Request, res: Response, next: NextFunction) => { + let i = 0; + const run = (err?: unknown): void => { + if (err) return next(err as Error); + const mw = mws[i++]; + if (!mw) return next(); + mw(req, res, run); + }; + run(); + }; +} + +export const loginLimiters: RequestHandler = chain( + loginIpLimiter, + loginUsernameLimiter, +); +export const registerLimiters: RequestHandler = registerIpLimiter; +export const forgotPasswordLimiters: RequestHandler = forgotPasswordIpLimiter; +export const resetPasswordLimiters: RequestHandler = resetPasswordIpLimiter; + +export { ipKeyGenerator }; diff --git a/artifacts/api-server/src/routes/auth.ts b/artifacts/api-server/src/routes/auth.ts index 5775cc86..b9e6df79 100644 --- a/artifacts/api-server/src/routes/auth.ts +++ b/artifacts/api-server/src/routes/auth.ts @@ -15,6 +15,12 @@ import { } from "@workspace/db"; import { requireAuth, requireAdmin, getUserRoles } from "../middlewares/auth"; import { saveSession, destroySession } from "../lib/session"; +import { + loginLimiters, + registerLimiters, + forgotPasswordLimiters, + resetPasswordLimiters, +} from "../lib/authRateLimit"; import { RegisterBody, LoginBody, @@ -78,7 +84,7 @@ function buildAuthUser( }; } -router.post("/auth/register", async (req, res): Promise => { +router.post("/auth/register", registerLimiters, async (req, res): Promise => { const parsed = RegisterBody.safeParse(req.body); if (!parsed.success) { res.status(400).json({ error: parsed.error.message }); @@ -140,7 +146,7 @@ router.post("/auth/register", async (req, res): Promise => { res.status(201).json(buildAuthUser(user, roles, await getUserGroupsForAuth(user.id))); }); -router.post("/auth/login", async (req, res): Promise => { +router.post("/auth/login", loginLimiters, async (req, res): Promise => { const parsed = LoginBody.safeParse(req.body); if (!parsed.success) { res.status(400).json({ error: parsed.error.message }); @@ -176,7 +182,7 @@ router.post("/auth/login", async (req, res): Promise => { res.json(buildAuthUser(user, roles, await getUserGroupsForAuth(user.id))); }); -router.post("/auth/forgot-password", async (req, res): Promise => { +router.post("/auth/forgot-password", forgotPasswordLimiters, async (req, res): Promise => { const parsed = ForgotPasswordBody.safeParse(req.body); if (!parsed.success) { res.status(400).json({ error: parsed.error.message }); @@ -214,7 +220,7 @@ router.post("/auth/forgot-password", async (req, res): Promise => { res.json({ success: true }); }); -router.post("/auth/reset-password/verify", async (req, res): Promise => { +router.post("/auth/reset-password/verify", resetPasswordLimiters, async (req, res): Promise => { const parsed = VerifyResetTokenBody.safeParse(req.body); if (!parsed.success) { res.status(400).json({ error: parsed.error.message }); @@ -236,7 +242,7 @@ router.post("/auth/reset-password/verify", async (req, res): Promise => { res.json({ valid: Boolean(record) }); }); -router.post("/auth/reset-password", async (req, res): Promise => { +router.post("/auth/reset-password", resetPasswordLimiters, async (req, res): Promise => { const parsed = ResetPasswordBody.safeParse(req.body); if (!parsed.success) { res.status(400).json({ error: parsed.error.message }); diff --git a/artifacts/api-server/tests/auth-rate-limit.test.mjs b/artifacts/api-server/tests/auth-rate-limit.test.mjs new file mode 100644 index 00000000..65672548 --- /dev/null +++ b/artifacts/api-server/tests/auth-rate-limit.test.mjs @@ -0,0 +1,216 @@ +import { test, after } from "node:test"; +import assert from "node:assert/strict"; +import pg from "pg"; + +const API_BASE = process.env.TEST_API_BASE ?? "http://localhost:8080"; +const DATABASE_URL = process.env.DATABASE_URL; +if (!DATABASE_URL) throw new Error("DATABASE_URL must be set to run these tests"); + +const pool = new pg.Pool({ connectionString: DATABASE_URL }); +const TEST_PASSWORD = "TestPass123!"; +// bcrypt cost 10 hash for TEST_PASSWORD (matches other test files in this repo) +const TEST_PASSWORD_HASH = + "$2b$10$Bs636ukPMyz01nKrsi.5m.JlDXSN22AVCvn8cgPWWDbo5yJRQX2vu"; + +const STAMP = `${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 8)}`; +const createdUserIds = []; +const createdUsernames = []; + +function uniqueIp(suffix) { + // Pick a non-loopback IPv4 so the rate-limit middleware applies + // (it skips loopback addresses in non-production). + const a = (Math.floor(Math.random() * 200) + 11) % 240; // 11..250 + const b = Math.floor(Math.random() * 250) + 1; + const c = Math.floor(Math.random() * 250) + 1; + return `10.${a}.${b}.${c}#${suffix}`.split("#")[0]; +} + +async function createUser(username, isActive = true) { + const { rows } = await pool.query( + `INSERT INTO users (username, email, password_hash, display_name_en, preferred_language, is_active) + VALUES ($1, $2, $3, 'Rate Test', 'en', $4) RETURNING id`, + [username, `${username}@example.com`, TEST_PASSWORD_HASH, isActive], + ); + const id = rows[0].id; + createdUserIds.push(id); + createdUsernames.push(username); + await pool.query( + `INSERT INTO user_roles (user_id, role_id) + SELECT $1, id FROM roles WHERE name = 'user'`, + [id], + ); + return id; +} + +after(async () => { + if (createdUserIds.length > 0) { + await pool.query(`DELETE FROM password_reset_tokens WHERE user_id = ANY($1::int[])`, [createdUserIds]); + await pool.query(`DELETE FROM user_roles WHERE user_id = ANY($1::int[])`, [createdUserIds]); + await pool.query(`DELETE FROM users WHERE id = ANY($1::int[])`, [createdUserIds]); + } + if (createdUsernames.length > 0) { + await pool.query(`DELETE FROM users WHERE username = ANY($1::text[])`, [createdUsernames]); + } + await pool.end(); +}); + +async function postLogin({ username, password, ip }) { + return fetch(`${API_BASE}/api/auth/login`, { + method: "POST", + headers: { + "Content-Type": "application/json", + "X-Forwarded-For": ip, + }, + body: JSON.stringify({ username, password }), + }); +} + +async function postForgot({ identifier, ip }) { + return fetch(`${API_BASE}/api/auth/forgot-password`, { + method: "POST", + headers: { + "Content-Type": "application/json", + "X-Forwarded-For": ip, + }, + body: JSON.stringify({ identifier }), + }); +} + +test("login: 11th attempt within window from same IP is throttled with 429", async () => { + const username = `rl_login_${STAMP}`; + await createUser(username); + const ip = uniqueIp("login-ip"); + + // First 10 with WRONG password from same IP, but with DIFFERENT usernames so + // we exercise only the per-IP limiter (default 10/min) and not the + // per-username limiter (default 8/15min). + for (let i = 0; i < 10; i++) { + const r = await postLogin({ + username: `rl_login_dummy_${STAMP}_${i}`, + password: "wrong", + ip, + }); + assert.equal(r.status, 401, `attempt ${i + 1} should be 401, got ${r.status}`); + } + + // 11th attempt from the same IP must be throttled regardless of payload + const blocked = await postLogin({ username, password: TEST_PASSWORD, ip }); + assert.equal(blocked.status, 429); + const body = await blocked.json(); + assert.equal(body.error, "too_many_requests"); +}); + +test("login: per-username limit blocks credential-stuffing across many IPs", async () => { + const username = `rl_user_${STAMP}`; + await createUser(username); + + // 8 wrong-password attempts each from a fresh IP — each one trips the + // per-username limiter (default max 8) without ever exceeding any single + // IP's per-IP budget. + for (let i = 0; i < 8; i++) { + const r = await postLogin({ + username, + password: "wrong", + ip: uniqueIp(`username-${i}`), + }); + assert.equal(r.status, 401, `attempt ${i + 1} should be 401, got ${r.status}`); + } + + // 9th attempt from yet another fresh IP must be 429 because the + // username-keyed limiter is exhausted. + const blocked = await postLogin({ + username, + password: TEST_PASSWORD, + ip: uniqueIp("username-final"), + }); + assert.equal(blocked.status, 429); +}); + +test("forgot-password: per-IP limit returns 429 after the budget", async () => { + const ip = uniqueIp("forgot"); + // Default forgot-password budget is 5 per 15 min per IP + for (let i = 0; i < 5; i++) { + const r = await postForgot({ identifier: `nobody_${STAMP}_${i}`, ip }); + assert.equal(r.status, 200, `attempt ${i + 1} should be 200, got ${r.status}`); + } + const blocked = await postForgot({ identifier: `nobody_${STAMP}_final`, ip }); + assert.equal(blocked.status, 429); +}); + +test("register: per-IP limit returns 429 after the budget", async () => { + const ip = uniqueIp("register"); + // Default register budget is 5 per hour per IP. + for (let i = 0; i < 5; i++) { + const username = `rl_reg_${STAMP}_${i}`; + createdUsernames.push(username); + const r = await fetch(`${API_BASE}/api/auth/register`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-Forwarded-For": ip }, + body: JSON.stringify({ + username, + email: `${username}@example.com`, + password: TEST_PASSWORD, + displayNameEn: "Rate Reg", + }), + }); + // Either 201 (created) or 403 (registration closed) is fine — we only + // care that the route ran and consumed a slot, not 429. + assert.notEqual(r.status, 429, `attempt ${i + 1} unexpectedly throttled`); + } + const blocked = await fetch(`${API_BASE}/api/auth/register`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-Forwarded-For": ip }, + body: JSON.stringify({ + username: `rl_reg_${STAMP}_final`, + email: `rl_reg_${STAMP}_final@example.com`, + password: TEST_PASSWORD, + displayNameEn: "Rate Reg", + }), + }); + assert.equal(blocked.status, 429); +}); + +test("reset-password (and verify) share a per-IP limit returning 429 after the budget", async () => { + const ip = uniqueIp("reset"); + // Default reset budget is 10 per 15 min per IP. Mix verify + reset hits to + // confirm both endpoints draw from the same per-IP bucket. + for (let i = 0; i < 10; i++) { + const path = i % 2 === 0 ? "/api/auth/reset-password/verify" : "/api/auth/reset-password"; + const body = + i % 2 === 0 + ? { token: `dummy_${i}` } + : { token: `dummy_${i}`, newPassword: TEST_PASSWORD }; + const r = await fetch(`${API_BASE}${path}`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-Forwarded-For": ip }, + body: JSON.stringify(body), + }); + // Verify returns 200 with valid:false; reset returns 400 invalid token. + assert.notEqual(r.status, 429, `attempt ${i + 1} on ${path} unexpectedly throttled`); + } + const blocked = await fetch(`${API_BASE}/api/auth/reset-password`, { + method: "POST", + headers: { "Content-Type": "application/json", "X-Forwarded-For": ip }, + body: JSON.stringify({ token: "dummy_final", newPassword: TEST_PASSWORD }), + }); + assert.equal(blocked.status, 429); +}); + +test("loopback (no X-Forwarded-For) is exempt in non-production so existing tests don't trip", async () => { + // We've just exhausted several limiters from synthetic IPs above. From + // loopback (the default for direct localhost requests), more attempts + // should still be processed normally — proving the dev-mode skip works. + const username = `rl_loop_${STAMP}`; + await createUser(username); + + // Hammer loopback well past every per-IP budget (10/min login). + for (let i = 0; i < 15; i++) { + const r = await fetch(`${API_BASE}/api/auth/login`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ username: `__nope_${STAMP}_${i}`, password: "x" }), + }); + // Either 401 (no such user) or 400 (validation) — never 429. + assert.notEqual(r.status, 429, `loopback attempt ${i + 1} unexpectedly throttled`); + } +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 36ac0877..a547036a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -117,6 +117,9 @@ importers: express: specifier: ^5 version: 5.2.1 + express-rate-limit: + specifier: ^8.5.1 + version: 8.5.1(express@5.2.1) express-session: specifier: ^1.19.0 version: 1.19.0 @@ -3409,6 +3412,12 @@ packages: exifr@7.1.3: resolution: {integrity: sha512-g/aje2noHivrRSLbAUtBPWFbxKdKhgj/xr1vATDdUXPOFYJlQ62Ft0oy+72V6XLIpDJfHs6gXLbBLAolqOXYRw==} + express-rate-limit@8.5.1: + resolution: {integrity: sha512-5O6KYmyJEpuPJV5hNTXKbAHWRqrzyu+OI3vUnSd2kXFubIVpG7ezpgxQy76Zo5GQZtrQBg86hF+CM/NX+cioiQ==} + engines: {node: '>= 16'} + peerDependencies: + express: '>= 4.11' + express-session@1.19.0: resolution: {integrity: sha512-0csaMkGq+vaiZTmSMMGkfdCOabYv192VbytFypcvI0MANrp+4i/7yEkJ0sbAEhycQjntaKGzYfjfXQyVb7BHMA==} engines: {node: '>= 0.8.0'} @@ -3680,6 +3689,10 @@ packages: resolution: {integrity: sha512-5Hh7Y1wQbvY5ooGgPbDaL5iYLAPzMTUrjMulskHLH6wnv/A+1q5rgEaiuqEjB+oxGXIVZs1FF+R/KPN3ZSQYYg==} engines: {node: '>=12'} + ip-address@10.2.0: + resolution: {integrity: sha512-/+S6j4E9AHvW9SWMSEY9Xfy66O5PWvVEJ08O0y5JGyEKQpojb0K0GKpz/v5HJ/G0vi3D2sjGK78119oXZeE0qA==} + engines: {node: '>= 12'} + ipaddr.js@1.9.1: resolution: {integrity: sha512-0KI/607xoxSToH7GjN1FfSbLoU0+btTicjsQSWQlh/hZykN8KpmMf7uYwPW3R+akZ6R/w18ZlXSHBYXiYUPO3g==} engines: {node: '>= 0.10'} @@ -7464,6 +7477,11 @@ snapshots: exifr@7.1.3: {} + express-rate-limit@8.5.1(express@5.2.1): + dependencies: + express: 5.2.1 + ip-address: 10.2.0 + express-session@1.19.0: dependencies: cookie: 0.7.2 @@ -7829,6 +7847,8 @@ snapshots: internmap@2.0.3: {} + ip-address@10.2.0: {} + ipaddr.js@1.9.1: {} is-extglob@2.1.1: {}