diff --git a/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js b/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js new file mode 100644 index 0000000000..8d97613b4f --- /dev/null +++ b/backend/__tests__/unit/middleware/rateLimitIpKeySeparation.test.js @@ -0,0 +1,283 @@ +/** + * TASK-126 — behavioural probe: no rate limiter mounted in the app may put two + * callers that differ only in `cf-connecting-ip` into one bucket. + * + * Why each walked instance is re-mounted on a stub instead of being exercised + * through the loaded app — all four measured, not assumed: + * - Every entry in the loaded router stack is a Layer whose `handle.name` is + * `''` and whose `entry.name` is ``, for middleware generally and + * for limiters specifically. Identity (`resetKey`/`getKey` own properties) + * is the only census instrument; anything name-keyed mis-attributes. + * - On a route carrying two limiters both write `RateLimit-Remaining` and the + * last one that runs wins. Proved on `POST /api/webhooks/groupme/:integrationId`: + * the 3000-max IP limiter reported `remaining` 599/598/597 (the 600-max + * limiter's values) and the walk entry for the 600-max limiter then read 596, + * because the first entry's three requests had decremented its bucket. So a + * hidden limiter's count is invisible in that response. + * - A limiter mounted behind `auth` (`routes/users.ts:171` + * `profileWriteUserLimit`) never runs for a header-free request. + * - Three instances never answer at all through the app: their handlers need + * Mongo and unit mode runs without it. + * On the stub there are no params and no auth header, so every key function + * takes its IP branch (each one falls back to the Cloudflare-aware helper), and + * the 204 terminal answers for all of them — no exemption list, no database. + * + * The A,A,B sequence is deliberate: two requests cannot separate "different + * buckets" from "not counting at all", so A2 is the instrument's own control. + * The two fixtures prove the instrument discriminates on the stub itself: one + * keyed the way the 21 swapped sites were — `req.ip ? ipKeyGenerator(req.ip) : + * 'anon'`, the spelling at `middleware/ipRateLimit.ts:54` — must COLLAPSE, and + * one keyed on `cloudflareIpRateLimitKeyGenerator` must SEPARATE. A stub that separates + * everything handed to it would pass all 84 and establish nothing. + */ +/* eslint-disable import/no-unresolved, import/extensions */ +const fs = require('fs'); +const express = require('express'); +const request = require('supertest'); +const rateLimit = require('express-rate-limit'); +const { ipKeyGenerator } = require('express-rate-limit'); +const { cloudflareIpRateLimitKeyGenerator } = require('../../../middleware/ipRateLimit'); + +// Captured at MODULE LOAD, before any test can flip it. A baseline re-read inside +// a test moves with a leak instead of catching it: measured, with the per-request +// restore removed an earlier test left 'development' set, the walk test's own +// `entry` then read 'development' too, and the comparison passed. +const ENTRY_NODE_ENV = process.env.NODE_ENV; + +// The app only mounts pg-messages/pg-status when PG_HOST is truthy AND the +// connect resolves truthy (server.ts:68,378-400), so this file sets it rather +// than inheriting it: setup.js used to leave the truthy STRING 'undefined' in +// PG_HOST (TASK-128 deletes the key instead), and a probe that silently needs +// that accident drops to 82 instances the day it is fixed. No restore machinery +// is needed — process.env is per test FILE (measured), so this assignment cannot +// reach any other suite. +process.env.PG_HOST = 'probe'; +jest.mock('../../../config/db', () => jest.fn()); +jest.mock('../../../config/db-pg', () => { + const actual = jest.requireActual('../../../config/db-pg'); + const pool = { query: jest.fn().mockResolvedValue({ rows: [] }), on: jest.fn(), end: jest.fn() }; + return { ...actual, connectPG: jest.fn().mockResolvedValue(pool) }; +}); +jest.mock('../../../config/init-pg-db', () => jest.fn().mockResolvedValue(true)); +jest.setTimeout(300000); + +const { app } = require('../../../server'); + +const isLimiter = (fn) => fn && typeof fn === 'function' + && Object.prototype.hasOwnProperty.call(fn, 'resetKey') + && Object.prototype.hasOwnProperty.call(fn, 'getKey'); + +const prefixOf = (layer) => { + const src = (layer.regexp && layer.regexp.source) || ''; + const m = src.match(/^\^\\?\/?([A-Za-z0-9_/\\-]*?)\\?\/\?\(\?=\\\/\|\$\)/); + return m ? `/${m[1].replace(/\\\//g, '/').replace(/\/$/, '')}` : ''; +}; +const normPath = (p) => p.replace(/:[A-Za-z0-9_]+/g, 'test-id').replace(/\/{2,}/g, '/'); + +const collect = (router, prefix, found, seen) => { + ((router && router.stack) || []).forEach((layer) => { + if (layer.route) { + const path = normPath(prefix + layer.route.path); + const method = Object.keys(layer.route.methods || {})[0] || 'get'; + (layer.route.stack || []).forEach((entry) => { + if (isLimiter(entry.handle) && !seen.has(entry.handle)) { + seen.add(entry.handle); + found.push({ instance: entry.handle, path, method }); + } + }); + return; + } + if (isLimiter(layer.handle) && !seen.has(layer.handle)) { + seen.add(layer.handle); + found.push({ instance: layer.handle, path: normPath(prefix) || '/', method: 'get' }); + } + if (layer.handle && layer.handle.stack) { + collect(layer.handle, prefix + prefixOf(layer), found, seen); + } + }); +}; + +const walked = []; +const seen = new Set(); + +const remainingOf = (res) => { + if (res.headers['ratelimit-remaining'] !== undefined) return Number(res.headers['ratelimit-remaining']); + const combined = res.headers.ratelimit; + const m = combined && String(combined).match(/remaining=(\d+)/); + return m ? Number(m[1]) : null; +}; + +const single = (instance) => { + const stub = express(); + stub.all('/', instance, (_req, res) => res.status(204).end()); + return stub; +}; + +const hit = async (stub, ip) => { + const entry = process.env.NODE_ENV; + let outcome; + try { + process.env.NODE_ENV = 'development'; + const res = await request(stub).get('/').set('cf-connecting-ip', ip) + .timeout({ response: 5000, deadline: 8000 }); + outcome = { remaining: remainingOf(res), status: res.status }; + } catch (err) { + outcome = { error: String((err && err.message) || err).slice(0, 80) }; + } finally { + // Restore per REQUEST so each iteration ends where it started; the assertion + // after the walk is what enforces that it did. + process.env.NODE_ENV = entry; + } + return outcome; +}; + +const observe = async (instance, tag) => { + const a = `198.51.100.${tag}`; + const b = `203.0.113.${tag}`; + const stub = single(instance); + const first = await hit(stub, a); + const second = await hit(stub, a); + const other = await hit(stub, b); + return { + r1: first.remaining, r2: second.remaining, r3: other.remaining, status: first.status, error: first.error, + }; +}; + +const fmt = (label, rec) => { + const head = `${label} [r1=${rec.r1} r2=${rec.r2} r3=${rec.r3} status=${rec.status}`; + return `${head}${rec.error ? ` err=${rec.error}` : ''}]`; +}; + +describe('TASK-126 ip key separation', () => { + // The walk's own assertion covers the probe path; this covers every test in the + // file, the two controls included — the leak mutation was introduced by the + // negative control, before the walk ever ran. + afterAll(() => { + expect(process.env.NODE_ENV).toBe(ENTRY_NODE_ENV); + }); + + beforeAll(async () => { + // pg-messages and pg-status mount from promise callbacks (server.ts:380-400), + // so the walk has to wait for the PG connect to resolve or those limiters are + // invisible to it. + await new Promise((resolve) => { setTimeout(resolve, 2000); }); + collect(app._router || app.router, '', walked, seen); + }); + + it('walks the loaded app and finds limiter instances by identity', () => { + expect(walked.some((w) => w.path.startsWith('/api/pg/messages'))).toBe(true); + expect(walked.length).toBeGreaterThanOrEqual(80); + const distinct = new Set(walked.map((w) => w.instance)); + expect(distinct.size).toBe(walked.length); + }); + + it('negative control: the instrument catches a limiter keyed the way the swapped sites were', async () => { + // The defect's own spelling, not a cousin of it (vera 73704): the 21 sites + // keyed `req.ip ? ipKeyGenerator(req.ip) : 'anon'`. A bare `req.ip` collapses + // on this stub for the same reason — no `trust proxy`, so both requests carry + // the loopback socket address — but it also hands express-rate-limit's IPv6 + // key validation a raw value, which would make this control measure a second + // thing. The form below is what was actually there, so the fixture proves the + // defect rather than a paraphrase of it. + const raw = rateLimit({ + windowMs: 60000, + max: 5, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: (req) => (req.ip ? ipKeyGenerator(req.ip) : 'anon'), + }); + const rec = await observe(raw, 250); + expect(rec.r2).toBe(rec.r1 - 1); + expect(rec.r3).toBe(rec.r2 - 1); + expect(rec.r3).not.toBe(rec.r1); + }); + + it('positive control: the CF-aware key separates on the same stub', async () => { + const cf = rateLimit({ + windowMs: 60000, + max: 5, + standardHeaders: true, + legacyHeaders: false, + keyGenerator: (req) => cloudflareIpRateLimitKeyGenerator(req), + }); + const rec = await observe(cf, 251); + expect(rec.r2).toBe(rec.r1 - 1); + expect(rec.r3).toBe(rec.r1); + }); + + // What this measures (wren 73671, ruling (b)), not what it might be read as: + // a request with no credential, no param and no body — built from nothing and + // carrying only `cf-connecting-ip` — is bucketed per `cf-connecting-ip` on + // every instance. That is stricter than a per-arm check: a limiter with no IP + // arm at all keys such a request on a CONSTANT, one bucket for every anonymous + // caller, which is the same defect as raw `req.ip` behind cloudflared — and it + // collapses A and B here exactly the same way. + it('every walked instance counts and separates; none collapses', async () => { + const results = []; + // Serial, not `Promise.all` (wren 73692): with 84 chains started in one tick, + // only the first reads the suite's NODE_ENV; the other 83 read the + // 'development' it just set and each restores what it read — so the walk ends + // flipped, and no per-request check can see it. Serialised, every iteration + // restores before the next begins, which is what makes the assertion below a + // witness rather than decoration: it fails if the walk does not end where it + // started. + /* eslint-disable no-await-in-loop, no-restricted-syntax -- serialised on + purpose: the NODE_ENV restore of one iteration must complete before the + next reads it, which is the property under test. */ + for (const [i, w] of walked.entries()) { + results[i] = { path: w.path, method: w.method, ...(await observe(w.instance, i + 1)) }; + } + /* eslint-enable no-await-in-loop, no-restricted-syntax */ + expect(process.env.NODE_ENV).toBe(ENTRY_NODE_ENV); + + // Stub-only arm, disclosed rather than left implied (vera 73673): on the real + // app `users.ts:65` `profileWriteUserLimit` keys on + // `req.userId || req.user?.id || req.user?._id` FIRST, and `auth` runs before + // it (`users.ts:171`, `router.put('/profile', profileWriteIngressLimit, auth, + // profileWriteUserLimit, ...)`) — an unauthenticated request stops at auth, so + // its IP arm is unreachable in production. The arm is real and must stay + // CF-derived, but what this walk proves about it is the stub's behaviour, not + // liveness: in production that limiter keys per account, not per address. + const unseen = results.filter((r) => r.r1 === null || r.r1 === undefined); + const notCounting = results.filter((r) => r.r1 !== null && r.r1 !== undefined && r.r2 !== r.r1 - 1); + // Collapse signature is r3 === r2 - 1, NOT r3 === r2: a third request into the + // same bucket keeps counting down. Measured on the raw-req.ip fixture below. + const collapsed = results.filter((r) => r.r1 !== null && r.r3 === r.r2 - 1); + const separated = results.filter((r) => r.r1 !== null && r.r3 === r.r1); + + expect(unseen.map((r) => fmt(`${r.method.toUpperCase()} ${r.path}`, r))).toEqual([]); + expect(notCounting.map((r) => fmt(`${r.method.toUpperCase()} ${r.path}`, r))).toEqual([]); + expect(collapsed.map((r) => fmt(`${r.method.toUpperCase()} ${r.path}`, r))).toEqual([]); + expect(separated.length).toBe(results.length); + // The floor is on observations, not violations: an assertion that every + // member of an EMPTY set is fine would pass on a walk that found nothing. + expect(separated.length).toBeGreaterThanOrEqual(80); + // Census dump for reconciliation, not an assertion: it is what let 84 be + // accounted as 75 direct + 9 factory invocations instead of assumed. Opt-in, + // so CI does not write a /tmp file on every run. + if (process.env.PROBE_CENSUS) { + fs.writeFileSync(process.env.PROBE_CENSUS, JSON.stringify({ + instances: results.length, + separated: separated.length, + notCounting: notCounting.length, + collapsed: collapsed.length, + unseen: unseen.length, + paths: results.map((r) => `${r.method.toUpperCase()} ${r.path}`), + }, null, 1)); + } + }); + + it('finds the limiters that mount late or behind a router', () => { + const modules = ['../../../routes/agentHooks', '../../../routes/podInvites', '../../../routes/pg-messages']; + const report = modules.map((mod) => { + // eslint-disable-next-line global-require, import/no-dynamic-require + const router = require(mod); + const found = []; + collect(router, '', found, new Set()); + const missing = found.filter((f) => !seen.has(f.instance)); + return { mod, found: found.length, missing: missing.length }; + }); + expect(report.filter((r) => r.found === 0).map((r) => r.mod)).toEqual([]); + expect(report.filter((r) => r.missing > 0).map((r) => r.mod)).toEqual([]); + }); +}); diff --git a/backend/middleware/agentRateLimit.ts b/backend/middleware/agentRateLimit.ts index 0af3dbd040..399e62c521 100644 --- a/backend/middleware/agentRateLimit.ts +++ b/backend/middleware/agentRateLimit.ts @@ -15,7 +15,7 @@ import type { Request } from 'express'; import { createHash } from 'crypto'; -import { ipKeyGenerator } from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from './ipRateLimit'; export const agentRateLimitKeyGenerator = (req: Request): string => { // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -25,9 +25,10 @@ export const agentRateLimitKeyGenerator = (req: Request): string => { if (typeof auth === 'string' && auth.length > 0) { return `hdr:${createHash('sha256').update(auth).digest('hex')}`; } - // ipKeyGenerator collapses IPv6 to its prefix so a client can't rotate - // through its /64 to bypass the limit (ERR_ERL_KEY_GEN_IPV6, #652). - return `ip:${req.ip ? ipKeyGenerator(req.ip) : 'unknown'}`; + // The Cloudflare-aware helper keys on the client address the edge recorded, + // and collapses IPv6 to its prefix so a client can't rotate through its /64 + // to bypass the limit (ERR_ERL_KEY_GEN_IPV6, #652). + return `ip:${cloudflareIpRateLimitKeyGenerator(req)}`; }; // CJS compat for the require()-style imports used elsewhere in backend/. diff --git a/backend/routes/agentBinding.ts b/backend/routes/agentBinding.ts index 0ac15ef79b..4a1bf80e6f 100644 --- a/backend/routes/agentBinding.ts +++ b/backend/routes/agentBinding.ts @@ -5,7 +5,8 @@ // predicate is enforced HERE from the daemon credential's server-side // machineId — never from a caller-supplied value. import express from 'express'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; import daemonAuth, { DaemonAuthedRequest } from '../middleware/daemonAuth'; import { GRANT_BROKER_ID, GRANT_BROKER_URL } from '../services/installable/toolInstallables'; @@ -41,7 +42,7 @@ const bindingRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: { status: (n: number) => { json: (b: unknown) => void } }) => { res.status(429).json({ msg: 'rate limit exceeded: 120 binding ops per 60s' }); diff --git a/backend/routes/agentHooks.ts b/backend/routes/agentHooks.ts index 59c853b8cd..d61b9a69cc 100644 --- a/backend/routes/agentHooks.ts +++ b/backend/routes/agentHooks.ts @@ -1,6 +1,7 @@ export {}; import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; const express = require('express'); @@ -26,7 +27,7 @@ const hookRateLimit = rateLimit({ const token = String(req.header('x-commonly-agent-token') || req.header('authorization') || '').trim(); return token ? `token:${createHash('sha256').update(token).digest('hex')}` - : `ip:${req.ip || 'unknown'}`; + : cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: any, res: any) => res.status(429).json({ code: 'rate_limited', diff --git a/backend/routes/approvals.ts b/backend/routes/approvals.ts index 25708a37b2..25135fb045 100644 --- a/backend/routes/approvals.ts +++ b/backend/routes/approvals.ts @@ -4,7 +4,8 @@ // the service as defense in depth. // ESM import (not require) so CodeQL's js/missing-rate-limiting query // recognises the limiter on the POST route — same pattern as messages.ts. -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; import type { Request } from 'express'; @@ -30,7 +31,7 @@ const approvalResolveLimit = rateLimit({ if (authHeader) { return `apr:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req, res) => res.status(429).json({ error: 'rate limit exceeded: 30 approval decisions per 60s' }), }); diff --git a/backend/routes/credentials.ts b/backend/routes/credentials.ts index 2021963e99..61483e2287 100644 --- a/backend/routes/credentials.ts +++ b/backend/routes/credentials.ts @@ -5,7 +5,8 @@ import express from 'express'; import { Types } from 'mongoose'; // ESM import (not require) so CodeQL's js/missing-rate-limiting query // recognizes the limiter (same pattern as routes/messages.ts). -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; import AgentCredential from '../models/AgentCredential'; @@ -28,7 +29,7 @@ const credentialRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: { status: (n: number) => { json: (b: unknown) => void } }) => { res.status(429).json({ msg: 'rate limit exceeded: 60 credential ops per 60s' }); diff --git a/backend/routes/grants.ts b/backend/routes/grants.ts index 0fa8b5b1ba..122958ddbb 100644 --- a/backend/routes/grants.ts +++ b/backend/routes/grants.ts @@ -1,5 +1,6 @@ import express from 'express'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; import { Types } from 'mongoose'; import Integration from '../models/Integration'; @@ -52,7 +53,7 @@ const grantRateLimit = rateLimit({ keyGenerator: (req: express.Request): string => { const authHeader = req.get('Authorization') || req.get('x-auth-token'); if (authHeader) return `grant:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req, res) => res.status(429).json({ message: 'rate limit exceeded: 60 grant operations per 60s' }), }); diff --git a/backend/routes/hosted.ts b/backend/routes/hosted.ts index 8ea9012614..742ad5648b 100644 --- a/backend/routes/hosted.ts +++ b/backend/routes/hosted.ts @@ -13,7 +13,8 @@ * bypass here; provisioning someone else's agent is not an ops action. */ import express from 'express'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; const auth = require('../middleware/auth'); @@ -38,7 +39,7 @@ const hostedRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: express.Response) => { res.status(429).json({ message: 'rate limit exceeded: 30 hosted-runtime requests per 60s' }); diff --git a/backend/routes/machines.ts b/backend/routes/machines.ts index 8de069fff6..584b5f17d2 100644 --- a/backend/routes/machines.ts +++ b/backend/routes/machines.ts @@ -1,6 +1,7 @@ import express from 'express'; import { createHash } from 'crypto'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { Types } from 'mongoose'; import { listMachinesForOwner, @@ -32,7 +33,7 @@ const machineRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: express.Response) => { res.status(429).json({ message: 'rate limit exceeded: 60 machine requests per 60s' }); diff --git a/backend/routes/messages.ts b/backend/routes/messages.ts index 10b8c4d4ca..fb766f9a44 100644 --- a/backend/routes/messages.ts +++ b/backend/routes/messages.ts @@ -1,6 +1,7 @@ // ESM import (not require) so CodeQL's js/missing-rate-limiting query // recognises the limiter on the POST route — same pattern as uploads.ts. -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; // eslint-disable-next-line global-require const express = require('express'); @@ -40,7 +41,7 @@ interface RateLimitRes { // for a minute straight) and tight enough to blunt a runaway client looping // on a flaky send. Keyed on the Authorization header (hashed) so each user's // bearer token gets its own bucket — NAT'd users sharing one office IP don't -// collide. Falls back to the IPv6-safe ipKeyGenerator for the rare unauth +// collide. Falls back to the Cloudflare-aware key helper for the rare unauth // path. Applied as the FIRST middleware on POST so CodeQL's // js/missing-rate-limiting query sees it. const sendMessageRateLimit = rateLimit({ @@ -53,7 +54,7 @@ const sendMessageRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: RateLimitRes) => { res.status(429).json({ msg: 'rate limit exceeded: 60 messages per 60s' }); @@ -73,7 +74,7 @@ const readMessageRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: RateLimitRes) => { res.status(429).json({ msg: 'rate limit exceeded: 240 message reads per 60s' }); @@ -100,7 +101,7 @@ const reactionRateLimit = rateLimit({ if (authHeader) { return `rxn:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: RateLimitRes) => { res.status(429).json({ msg: 'rate limit exceeded: 120 reactions per 60s' }); diff --git a/backend/routes/pg-messages.ts b/backend/routes/pg-messages.ts index 4ce9dcedf9..d92578c548 100644 --- a/backend/routes/pg-messages.ts +++ b/backend/routes/pg-messages.ts @@ -1,7 +1,8 @@ // ESM import (not require) so CodeQL's js/missing-rate-limiting query can // trace the middleware — it does not follow a rate-limit factory through a // require() return. Same shape as routes/messages.ts. -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; // eslint-disable-next-line global-require @@ -27,7 +28,7 @@ const keyByToken = (req: RateLimitReq) => { if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }; const readLimit = rateLimit({ diff --git a/backend/routes/podInvites.ts b/backend/routes/podInvites.ts index a5e011cf0c..9d4c567033 100644 --- a/backend/routes/podInvites.ts +++ b/backend/routes/podInvites.ts @@ -7,7 +7,8 @@ import express from 'express'; import crypto, { createHash } from 'crypto'; import mongoose from 'mongoose'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; const router = express.Router(); const auth = require('../middleware/auth'); const Pod = require('../models/Pod'); @@ -23,7 +24,7 @@ const inviteRateLimitKey = (req: any) => { if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }; const inviteReadRateLimit = rateLimit({ diff --git a/backend/routes/pods.ts b/backend/routes/pods.ts index 04607c1032..50764e244a 100644 --- a/backend/routes/pods.ts +++ b/backend/routes/pods.ts @@ -1,7 +1,7 @@ import path from 'path'; import fs from 'fs'; import { createHash } from 'crypto'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -60,7 +60,7 @@ const podJoinRateLimitKey = (req: any) => { if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }; const podFocusRateLimit = rateLimit({ diff --git a/backend/routes/registry/agent-tokens.ts b/backend/routes/registry/agent-tokens.ts index 22f5844bb1..583764b17f 100644 --- a/backend/routes/registry/agent-tokens.ts +++ b/backend/routes/registry/agent-tokens.ts @@ -4,7 +4,8 @@ // ESM import (not require) so CodeQL's js/missing-rate-limiting query can // trace the middleware; it cannot follow a limiter through a require() return // or a router.use wrapper. Same shape as routes/messages.ts. -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../../middleware/ipRateLimit'; import { createHash } from 'crypto'; interface TokenRateReq { get?: (name: string) => string | undefined; ip?: string } @@ -23,7 +24,7 @@ const tokenRouteLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: TokenRateRes) => { res.status(429).json({ error: 'rate limit exceeded: 60 token requests per 60s' }); diff --git a/backend/routes/registry/files.ts b/backend/routes/registry/files.ts index f9eb80ed44..3314770726 100644 --- a/backend/routes/registry/files.ts +++ b/backend/routes/registry/files.ts @@ -21,20 +21,20 @@ // `req.userId` is not set yet, which is why the key generators below hash the // Authorization header instead — the idiom `routes/messages.ts` already uses // for its pre-auth limiters, and the reason those are clean. -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../../middleware/ipRateLimit'; const { createHash } = require('crypto'); // Per-caller key that works BEFORE `auth` has run. Hashing the raw header // keeps one token's budget isolated from another's without leaking the token -// into rate-limiter state; unauthenticated callers fall back to -// ipKeyGenerator so IPv6 clients can't rotate within their /64 -// (ERR_ERL_KEY_GEN_IPV6, #652). +// into rate-limiter state; unauthenticated callers fall back to the +// Cloudflare-aware helper (IPv6-safe — see middleware/ipRateLimit.ts). const preAuthCallerKey = (req: any) => { const authHeader = req.get?.('authorization'); if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }; const express = require('express'); diff --git a/backend/routes/spawnCredentials.ts b/backend/routes/spawnCredentials.ts index e62e3fe6de..3af4cf0394 100644 --- a/backend/routes/spawnCredentials.ts +++ b/backend/routes/spawnCredentials.ts @@ -24,7 +24,8 @@ import express from 'express'; // ESM import (not require) so CodeQL's js/missing-rate-limiting query recognizes // the limiter (same pattern as routes/credentials.ts and routes/messages.ts). -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; import { createHash } from 'crypto'; import { Types } from 'mongoose'; import { @@ -61,7 +62,7 @@ const spawnCredentialRateLimit = rateLimit({ if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: { status: (n: number) => { json: (b: unknown) => void } }) => { res.status(429).json({ message: 'rate limit exceeded: 240 spawn credential ops per 60s' }); diff --git a/backend/routes/summaries.ts b/backend/routes/summaries.ts index efe00ece3e..02af044d04 100644 --- a/backend/routes/summaries.ts +++ b/backend/routes/summaries.ts @@ -17,7 +17,9 @@ const { AgentInstallation } = require('../models/AgentRegistry'); // eslint-disable-next-line global-require const dailyDigestService = require('../services/dailyDigestService'); // eslint-disable-next-line global-require -const { rateLimit, ipKeyGenerator } = require('express-rate-limit'); +const { rateLimit } = require('express-rate-limit'); +// eslint-disable-next-line global-require +const { cloudflareIpRateLimitKeyGenerator } = require('../middleware/ipRateLimit'); // eslint-disable-next-line global-require const { createHash } = require('crypto'); // eslint-disable-next-line global-require @@ -49,7 +51,7 @@ const summariesRateLimitKey = (req: { get?: (h: string) => string | undefined; i if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }; // Reads hit Mongo (and canViewPod adds per-pod lookups) — throttle so a diff --git a/backend/routes/tasksApi.ts b/backend/routes/tasksApi.ts index bd1aa826c8..2e828710cf 100644 --- a/backend/routes/tasksApi.ts +++ b/backend/routes/tasksApi.ts @@ -1,6 +1,7 @@ import { createHash } from 'crypto'; import type { Request, Response } from 'express'; -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -100,7 +101,7 @@ const taskWriteRateLimitKey = (req: Request): string => { if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }; // Create and claim were limited; complete, updates and patch were not, though diff --git a/backend/routes/users.ts b/backend/routes/users.ts index 85a798730d..3f6ddaaaa3 100644 --- a/backend/routes/users.ts +++ b/backend/routes/users.ts @@ -1,6 +1,6 @@ // ESM imports keep CodeQL's missing-rate-limiting dataflow connected to the // DB-backed polling route below (same established pattern as messages.ts). -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; import { createHash } from 'crypto'; import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require @@ -35,7 +35,7 @@ const agentConnectionReadLimit = rateLimit({ if (authHeader) { return `agent-connection:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: any) => res.status(429).json({ msg: 'rate limit exceeded: 60 agent connection reads per 60s', @@ -76,7 +76,7 @@ const profileWriteUserLimit = rateLimit({ }) => { const userId = req.userId || req.user?.id || req.user?._id; if (userId) return `profile-write:${String(userId)}`; - return req.ip ? ipKeyGenerator(req.ip) : 'anon'; + return cloudflareIpRateLimitKeyGenerator(req as never); }, handler: (_req: unknown, res: any) => res.status(429).json({ msg: 'rate limit exceeded: 30 profile writes per 15 minutes',