fix(integrations): rate-limit the connector connect and disconnect routes (TASK-261)

backend/__tests__/unit/middleware/integrationRateLimit.test.js

@@ -8,7 +8,9 @@
 //
 // 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
+// are two buckets, and so are two `x-auth-token` values, the header `auth` falls
+// back to; one token is one bucket in every form `auth` accepts. 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.
 //
@@ -115,4 +117,30 @@ describe('integrationsRateLimitKey', () => {
       .not.toBe(integrationsRateLimitKey(withToken('Bearer two', '10.0.0.1')));
     expect(integrationsRateLimitKey(withToken('Bearer one', '10.0.0.1'))).toMatch(/^tok:/);
   });
+
+  it('buckets an x-auth-token by that token, and lets Authorization win as auth does', () => {
+    const withHeaders = (headers, ip) => ({ get: (name) => headers[name], headers, ip });
+
+    // Two x-auth-token callers behind one address are two buckets. Keyed by the
+    // address instead, they would share one 30-write budget.
+    const one = withHeaders({ 'x-auth-token': 'one' }, '10.0.0.1');
+    const two = withHeaders({ 'x-auth-token': 'two' }, '10.0.0.1');
+    expect(integrationsRateLimitKey(one)).not.toBe(integrationsRateLimitKey(two));
+    expect(integrationsRateLimitKey(one)).toMatch(/^tok:/);
+
+    // `auth` reads Authorization first (auth.ts:34-35), so the key does too.
+    const both = withHeaders({ authorization: 'Bearer one', 'x-auth-token': 'two' }, '10.0.0.1');
+    expect(integrationsRateLimitKey(both))
+      .toBe(integrationsRateLimitKey(withHeaders({ authorization: 'Bearer one' }, '10.0.0.1')));
+  });
+
+  it('gives one token one bucket in every form auth accepts', () => {
+    const withHeaders = (headers) => ({ get: (name) => headers[name], headers, ip: '10.0.0.1' });
+    const key = integrationsRateLimitKey(withHeaders({ authorization: 'Bearer one' }));
+
+    // `auth` strips `Bearer ` and falls back to x-auth-token, so these are the
+    // same credential to it. Keyed by the raw header, they were three budgets.
+    expect(integrationsRateLimitKey(withHeaders({ authorization: 'one' }))).toBe(key);
+    expect(integrationsRateLimitKey(withHeaders({ 'x-auth-token': 'one' }))).toBe(key);
+  });
 });

backend/__tests__/unit/routes/integrations.connectRateLimit.test.js

@@ -0,0 +1,109 @@
+// TASK-261 — POST /:id/connect and POST /:id/disconnect carry the connector
+// write limiter, ahead of auth, and spend the same bucket as every other
+// connector write.
+//
+// `routeRateLimitGuard` reads NAMES, so it witnesses presence and mount order
+// and nothing else: a route-local `rateLimit()` given a `*RateLimit*` name, or a
+// bound nobody chose, satisfies it exactly as well as the shared limiter does.
+// This suite loads the real `middleware/integrationRateLimit` and drives the
+// real router, so what it measures is the count.
+//
+// No database. `Integration.findById` answers null, so every request the
+// limiter lets through is the handler's own 404 — cheap, and distinguishable
+// from the limiter's 429 by status and by body. `auth` counts its calls: a
+// refusal that ran auth first would still be a 429 here, and the DB lookup auth
+// does is the cost the limiter exists to spare.
+//
+// The budget and the 429 body are restated from the module (30 writes/60s), as
+// its own suite does, so a change to either reddens this file too. The store is
+// module-level and persists across tests, so each test presents its own token.
+const request = require('supertest');
+const express = require('express');
+
+jest.mock('../../../middleware/auth', () => jest.fn((req, _res, next) => {
+  req.user = { id: 'user-1' };
+  next();
+}));
+jest.mock('../../../middleware/adminAuth', () => (req, res, next) => next());
+jest.mock('../../../models/Pod', () => ({ findById: jest.fn() }));
+jest.mock('../../../models/User', () => ({ findById: jest.fn() }));
+jest.mock('../../../models/DiscordIntegration', () => function DiscordIntegration(data) {
+  Object.assign(this, data);
+  this.save = jest.fn().mockResolvedValue(this);
+});
+jest.mock('../../../services/discordService', () => jest.fn().mockImplementation(() => ({
+  connect: jest.fn().mockResolvedValue(true),
+  disconnect: jest.fn().mockResolvedValue(true),
+})));
+jest.mock('../../../models/Integration', () => {
+  function Integration(data) {
+    Object.assign(this, data);
+  }
+  Integration.findById = jest.fn().mockResolvedValue(null);
+  return Integration;
+});
+
+const auth = require('../../../middleware/auth');
+const integrationRoutes = require('../../../routes/integrations');
+
+const app = express();
+app.use(express.json());
+app.use('/api/integrations', integrationRoutes);
+
+const WRITE_BUDGET = 30;
+const WRITE_MSG = 'rate limit exceeded: 30 writes per 60s';
+
+const token = (name) => ({ authorization: `Bearer ${name}` });
+
+// Sequential on purpose: the limiter counts on arrival, so a concurrent burst
+// would decide which request is refused by timing rather than by order.
+const send = (paths, headers) => paths.reduce(
+  (prior, path) => prior.then(async (seen) => {
+    const res = await request(app).post(path).set(headers).send({});
+    return seen.concat({ status: res.status, msg: res.body && res.body.msg });
+  }),
+  Promise.resolve([]),
+);
+
+const times = (n, path) => Array.from({ length: n }, () => path);
+
+describe('the connector connect and disconnect routes are rate-limited (TASK-261)', () => {
+  beforeEach(() => {
+    auth.mockClear();
+  });
+
+  it.each(['connect', 'disconnect'])(
+    '%s reaches its handler 30 times, then the limiter refuses before auth',
+    async (verb) => {
+      const path = `/api/integrations/i-261/${verb}`;
+      const seen = await send(times(WRITE_BUDGET + 1, path), token(`t261-${verb}`));
+
+      // Up to the bound, every answer is the handler's own 404, so a limiter
+      // that refused early would show here rather than read as a passing loop.
+      expect(seen.slice(0, WRITE_BUDGET).every((entry) => entry.status === 404)).toBe(true);
+      expect(seen[WRITE_BUDGET]).toEqual({ status: 429, msg: WRITE_MSG });
+      // 31 requests, 30 auth calls: the refused one never reached auth.
+      expect(auth).toHaveBeenCalledTimes(WRITE_BUDGET);
+    },
+  );
+
+  it('spends one bucket with the other connector writes, and a fresh credential has its own', async () => {
+    const half = WRITE_BUDGET / 2;
+    const spent = await send([
+      ...times(half, '/api/integrations/i-261/connect'),
+      ...times(half, '/api/integrations/i-261/disconnect'),
+    ], token('t261-shared'));
+    expect(spent.every((entry) => entry.status === 404)).toBe(true);
+
+    // The create route has carried the shared limiter since #1297. A
+    // route-local limiter on either verb would leave this bucket at 15 and let
+    // the create through to its handler.
+    const [create] = await send(['/api/integrations'], token('t261-shared'));
+    expect(create).toEqual({ status: 429, msg: WRITE_MSG });
+
+    // The control: the refusal above is this credential's spent bucket, not a
+    // blanket one.
+    const [fresh] = await send(['/api/integrations/i-261/connect'], token('t261-fresh'));
+    expect(fresh.status).toBe(404);
+  });
+});

backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json

@@ -84,8 +84,6 @@
   "routes/integrations.ts GET /:id/stats [unlimited]",
   "routes/integrations.ts GET /admin/all [unlimited]",
   "routes/integrations.ts GET /catalog [unlimited]",
-  "routes/integrations.ts POST /:id/connect [unlimited]",
-  "routes/integrations.ts POST /:id/disconnect [unlimited]",
   "routes/integrations.ts POST /:id/ingest-tokens [unlimited]",
   "routes/integrations.ts POST /:id/send [unlimited]",
   "routes/integrations.ts POST /ingest [unlimited]",

backend/middleware/integrationRateLimit.ts

@@ -16,14 +16,20 @@ type RateLimitRequest = {
 // 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
+// credential is the token `auth` extracts (`auth.ts:34-35`): `Authorization`
+// without its `Bearer ` prefix, else `x-auth-token`. Hash a raw header instead
+// and one token sent three ways (`Bearer t`, bare `t`, `x-auth-token: t`) holds
+// three budgets; read `Authorization` alone and an `x-auth-token` caller shares
+// its address's budget with every caller behind that address.
+// 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)}`;
+  const credential = req.get?.('authorization')?.replace('Bearer ', '')
+    || req.get?.('x-auth-token');
+  if (credential) {
+    return `tok:${createHash('sha256').update(credential).digest('hex').slice(0, 16)}`;
   }
   // Structural type → the helper's express `Request`: this module keeps its own
   // type so the key stays unit-testable without an express request.

backend/routes/integrations.ts

@@ -552,7 +552,7 @@ router.post('/', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res
   }
 });
 
-router.post('/:id/connect', auth, async (req: AuthReq, res: Res) => {
+router.post('/:id/connect', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res) => {
   try {
     const { id } = req.params || {};
     const integration = await Integration.findById(id) as { type?: string; podId?: unknown } | null;
@@ -572,7 +572,7 @@ router.post('/:id/connect', auth, async (req: AuthReq, res: Res) => {
   }
 });
 
-router.post('/:id/disconnect', auth, async (req: AuthReq, res: Res) => {
+router.post('/:id/disconnect', writeIntegrationsRateLimit, auth, async (req: AuthReq, res: Res) => {
   try {
     const { id } = req.params || {};
     const integration = await Integration.findById(id) as { type?: string; podId?: unknown } | null;