fix(rate-limit): key the connector and profile-ingress limiters by the Cloudflare address (TASK-125)

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:/);
+  });
+});

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\\(\\{`);

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

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({

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',
   }),