diff --git a/backend/__tests__/unit/middleware/integrationRateLimit.test.js b/backend/__tests__/unit/middleware/integrationRateLimit.test.js new file mode 100644 index 0000000000..4c35d8bd01 --- /dev/null +++ b/backend/__tests__/unit/middleware/integrationRateLimit.test.js @@ -0,0 +1,118 @@ +// TASK-125 — the module-level witness for `middleware/integrationRateLimit.ts`. +// +// Every route suite that touches these limiters stubs them to a pass-through +// (`discordBindingContainment`, `installables`, `slackOAuth.installables`, +// `integrations.routingState`, `integrations.linkedUserId`), and +// `routeRateLimitGuard` pins only the mount order — so nothing showed either +// limiter COUNTING. This file loads the real module and drives it. +// +// WHAT IT WITNESSES. The write budget is a count: 30 succeed in the window and +// the 31st is refused with the limiter's own body. Two presented bearer tokens +// are two buckets. And the no-header branch buckets by the Cloudflare-recorded +// address, so two requests differing only in `req.ip` share a bucket while two +// differing only in `cf-connecting-ip` do not. +// +// WHAT IT DOES NOT WITNESS, stated rather than implied. A key is chosen before +// any route's auth check runs, so these buckets are per PRESENTED credential or +// address — not per caller, and not a boundary against a caller who rotates +// either one. The other `ipKeyGenerator(req.ip)` fallbacks elsewhere in +// backend/ are TASK-126 and are deliberately not asserted here. +// +// The counts and the 429 body are restated from the module rather than derived +// (30 writes/60s). A change to either reddens the assertion below, which is the +// tripwire; the coupling is named in the comment at the constant. +const express = require('express'); +const request = require('supertest'); + +const { + writeIntegrationsRateLimit, + integrationsRateLimitKey, +} = require('../../../middleware/integrationRateLimit'); + +const WRITE_BUDGET = 30; +const WRITE_MSG = 'rate limit exceeded: 30 writes per 60s'; + +const app = express(); +app.use(express.json()); +app.post('/write', writeIntegrationsRateLimit, (req, res) => res.status(200).json({ ok: true })); + +// Sequential on purpose: express-rate-limit counts on arrival, so a concurrent +// burst would decide the last status by timing rather than by order. Each test +// uses its own bucket, because the limiter is module-level and its store +// persists across tests in this file. +const flood = (remaining, headers, seen) => { + if (remaining === 0) return Promise.resolve(seen); + return request(app) + .post('/write') + .set(headers) + .then((res) => flood(remaining - 1, headers, seen.concat({ + status: res.status, + msg: res.body && res.body.msg, + }))); +}; + +const token = (name) => ({ authorization: `Bearer ${name}` }); + +describe('the connector write limiter counts and buckets (TASK-125)', () => { + it('refuses the 31st write in the window, with its own body, after 30 successes', async () => { + const seen = await flood(WRITE_BUDGET + 1, token('t125-budget'), []); + + expect(seen).toHaveLength(WRITE_BUDGET + 1); + expect(seen.slice(0, WRITE_BUDGET).every((entry) => entry.status === 200)).toBe(true); + expect(seen[WRITE_BUDGET].status).toBe(429); + // Identified as THIS limiter, not a blanket refusal: the body is its own. + expect(seen[WRITE_BUDGET].msg).toBe(WRITE_MSG); + }); + + it('gives two presented bearer tokens separate buckets', async () => { + const exhausted = await flood(WRITE_BUDGET + 1, token('t125-token-a'), []); + expect(exhausted[WRITE_BUDGET].status).toBe(429); + + // A different credential is a different bucket, so the budget the first one + // spent does not refuse this one. + const fresh = await request(app).post('/write').set(token('t125-token-b')); + expect(fresh.status).toBe(200); + }); + + it('buckets the no-header branch by cf-connecting-ip, not by the proxy address', async () => { + const exhausted = await flood(WRITE_BUDGET + 1, { 'cf-connecting-ip': '198.51.100.1' }, []); + expect(exhausted.slice(0, WRITE_BUDGET).every((entry) => entry.status === 200)).toBe(true); + expect(exhausted[WRITE_BUDGET].status).toBe(429); + + // Under a `req.ip`-only fallback this is the SAME bucket as the flood above + // (supertest gives every request here one loopback address), so it would be + // 429 rather than 200. + const other = await request(app).post('/write').set({ 'cf-connecting-ip': '198.51.100.2' }); + expect(other.status).toBe(200); + }); + + it('shares a single bucket for one cf-connecting-ip value', async () => { + const seen = await flood(WRITE_BUDGET + 1, { 'cf-connecting-ip': '198.51.100.3' }, []); + expect(seen[WRITE_BUDGET - 1].status).toBe(200); + expect(seen[WRITE_BUDGET].status).toBe(429); + }); +}); + +describe('integrationsRateLimitKey', () => { + it('reads cf-connecting-ip before req.ip, so the proxy address cannot decide the bucket', () => { + const withCf = (cf, ip) => ({ headers: { 'cf-connecting-ip': cf }, ip }); + + // One edge address, two `req.ip` values (the two cloudflared pods): one bucket. + expect(integrationsRateLimitKey(withCf('203.0.113.7', '10.0.0.1'))) + .toBe(integrationsRateLimitKey(withCf('203.0.113.7', '10.0.0.2'))); + // Two edge addresses, one `req.ip`: two buckets. Under the previous + // `req.ip`-only fallback both of these assertions invert. + expect(integrationsRateLimitKey(withCf('203.0.113.7', '10.0.0.1'))) + .not.toBe(integrationsRateLimitKey(withCf('203.0.113.8', '10.0.0.1'))); + }); + + it('still buckets a presented Authorization header by that header', () => { + const withToken = (value, ip) => ({ get: () => value, ip }); + + expect(integrationsRateLimitKey(withToken('Bearer one', '10.0.0.1'))) + .toBe(integrationsRateLimitKey(withToken('Bearer one', '10.0.0.2'))); + expect(integrationsRateLimitKey(withToken('Bearer one', '10.0.0.1'))) + .not.toBe(integrationsRateLimitKey(withToken('Bearer two', '10.0.0.1'))); + expect(integrationsRateLimitKey(withToken('Bearer one', '10.0.0.1'))).toMatch(/^tok:/); + }); +}); diff --git a/backend/__tests__/unit/middleware/rateLimitDefaultKey.test.js b/backend/__tests__/unit/middleware/rateLimitDefaultKey.test.js index a29a952853..8599b6b24c 100644 --- a/backend/__tests__/unit/middleware/rateLimitDefaultKey.test.js +++ b/backend/__tests__/unit/middleware/rateLimitDefaultKey.test.js @@ -23,9 +23,11 @@ * * KNOWN LIMIT OF THIS FILE, stated rather than left implicit: the general scan * asserts a `keyGenerator` is PRESENT, not that it is the Cloudflare one — some - * limiters legitimately key on a token or a composite. The seven sites this - * change fixed are therefore ALSO pinned by name to the exact expression, which - * is what makes `keyGenerator: (req) => req.ip` red rather than green. + * limiters legitimately key on a token or a composite. The seven sites TASK-110 + * fixed — and, since TASK-125, `routes/users.ts`'s profile-write ingress cap — + * are therefore ALSO pinned by name to the exact expression, which is what makes + * `keyGenerator: (req) => req.ip` red rather than green. That eighth site is the + * one TASK-110's census missed by checking only that A key was set. * * SCOPE: `backend/**` `.ts` and `.js`, excluding `__tests__`, `node_modules` * and `coverage`. A limiter constructed from a variable is FLAGGED rather than @@ -116,7 +118,7 @@ const findConfigs = (rawSource) => { return found; }; -/** The seven sites the TASK-110 ruling fixed, pinned to the exact expression. */ +/** Pinned to the exact expression: the seven TASK-110 fixed, then TASK-125's. */ const PINNED = [ ['routes/pods.ts', 'podAdminRateLimit'], ['routes/pods.ts', 'agentStatesRateLimit'], @@ -125,6 +127,7 @@ const PINNED = [ ['routes/registry/provision.ts', 'provisionRateLimit'], ['routes/admin/pods.ts', 'adminPodsRateLimit'], ['routes/billing.ts', 'checkoutLimit'], + ['routes/users.ts', 'profileWriteIngressLimit'], ]; describe('rate limiters never take express-rate-limit\'s default (req.ip) key', () => { @@ -167,6 +170,17 @@ describe('rate limiters never take express-rate-limit\'s default (req.ip) key', expect(violations).toEqual([]); }); + // Vacuity guard for the pinned list itself: a row deleted from `PINNED` is + // otherwise a silent coverage loss, because `it.each` over a shorter array + // reports fewer passes and no failures. Exact rather than a floor (wren + // 73921): `PINNED` is a curated list, so adding or dropping a site is a + // deliberate act and should be a visible edit to this number. The + // `configs >= 60` floor above stays loose because it counts scanned code, + // which grows with the tree rather than by decision. + it('keeps pinning every site the list names', () => { + expect(PINNED).toHaveLength(8); + }); + it.each(PINNED)('pins %s %s to the Cloudflare key expression', (relPath, name) => { const source = stripCommentsAndStrings(fs.readFileSync(path.join(BACKEND_ROOT, relPath), 'utf8')); const re = new RegExp(`const\\s+${name}\\s*=\\s*rateLimit\\(\\{`); diff --git a/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js b/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js index b75a2cb1ce..8d0cbfed01 100644 --- a/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js +++ b/backend/__tests__/unit/routes/discordChannelsRateLimit.test.js @@ -10,9 +10,11 @@ // the budget is never reached. // // Bucket: `integrationsRateLimitKey` keys on the Authorization header when one -// is present and on `req.ip` otherwise, so this flood cannot disturb the -// token-keyed buckets other suites use. (`server.ts:94-101` records why `req.ip` -// is a cloudflared pod address rather than the client in production.) +// is present and on the Cloudflare-resolved address otherwise — +// `cf-connecting-ip`, then `req.ip` (`middleware/ipRateLimit.ts:49`, TASK-125). +// This flood sends neither header, so it lands on `req.ip` and cannot disturb +// the token-keyed buckets other suites use. (`server.ts:94-101` records why +// `req.ip` is a cloudflared pod address rather than the client in production.) const request = require('supertest'); const express = require('express'); diff --git a/backend/middleware/integrationRateLimit.ts b/backend/middleware/integrationRateLimit.ts index 4438d69f77..4635d6e4f6 100644 --- a/backend/middleware/integrationRateLimit.ts +++ b/backend/middleware/integrationRateLimit.ts @@ -1,20 +1,33 @@ -import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; +import rateLimit from 'express-rate-limit'; import { createHash } from 'crypto'; +import type { Request } from 'express'; +import { cloudflareIpRateLimitKeyGenerator } from './ipRateLimit'; type RateLimitRequest = { get?: (header: string) => string | undefined; ip?: string; + headers?: Request['headers']; }; // Connector writes can mint bearer connect codes. Keep the key and limits in // one module so every route that creates or re-mints a connector shares the // same bucket instead of each route being independently burstable. +// +// The key is taken before any route's auth check runs, so the token branch +// buckets by the PRESENTED credential: two requests carrying the same header +// share a bucket whether or not that header authenticates anything. The +// no-header branch defers to `cloudflareIpRateLimitKeyGenerator`, which owns the +// `cf-connecting-ip`-then-`req.ip` rule (`ipRateLimit.ts:49`) — read on its own +// here, `req.ip` is a cloudflared pod address for every external caller, so this +// branch was one instance-wide bucket (`server.ts:94-101`, TASK-110's class). export const integrationsRateLimitKey = (req: RateLimitRequest): string => { 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'; + // Structural type → the helper's express `Request`: this module keeps its own + // type so the key stays unit-testable without an express request. + return cloudflareIpRateLimitKeyGenerator(req as unknown as Request); }; export const writeIntegrationsRateLimit = rateLimit({ diff --git a/backend/routes/users.ts b/backend/routes/users.ts index a3aa738c0b..85a798730d 100644 --- a/backend/routes/users.ts +++ b/backend/routes/users.ts @@ -2,6 +2,7 @@ // DB-backed polling route below (same established pattern as messages.ts). import rateLimit, { ipKeyGenerator } from 'express-rate-limit'; import { createHash } from 'crypto'; +import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -44,12 +45,18 @@ const agentConnectionReadLimit = rateLimit({ // Profile writes change the label that every chat message renders. Keep a // coarse ingress cap ahead of auth (the CodeQL-recognized shape) before doing // JWT work; the per-user cap below is the durable write protection. +// +// Keyed by the Cloudflare-recorded address, like the other internet-facing +// limiters: this cap is mounted ahead of auth, so it is the one a header-free +// redirect reaches, and reading `req.ip` here would put every external caller in +// one bucket (`server.ts:94-101`). TASK-125; pinned by name in +// rateLimitDefaultKey.test.js. const profileWriteIngressLimit = rateLimit({ windowMs: 15 * 60_000, max: 120, standardHeaders: true, legacyHeaders: false, - keyGenerator: (req: { ip?: string }) => req.ip ? ipKeyGenerator(req.ip) : 'anon', + keyGenerator: cloudflareIpRateLimitKeyGenerator, handler: (_req: unknown, res: any) => res.status(429).json({ msg: 'rate limit exceeded: too many profile writes from this address', }),