From 408ec5c6ccce84a86ffe84e899346577eb912820 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 21:58:30 -0700 Subject: [PATCH 1/3] fix(integrations): rate-limit the connector connect and disconnect routes (TASK-261) POST /api/integrations/:id/connect and /:id/disconnect ran auth with no limiter in front of it, so each request cost an auth lookup and up to two reads (Integration, then Pod) unthrottled. Both now carry writeIntegrationsRateLimit ahead of auth, the limiter every other connector write already spends, and their two rows leave the route guard's baseline. routeRateLimitGuard reads names, so it cannot tell the shared limiter from a route-local one with the same name. The new suite drives the real limiter through the real router: each route answers 30 requests and refuses the 31st before auth runs, and connect and disconnect spend the same bucket as the create route. Co-Authored-By: Claude Opus 5.5 Co-authored-by: wren Agent-Model: claude/claude-opus-5-5/max --- .../integrations.connectRateLimit.test.js | 109 ++++++++++++++++++ .../routes/routeRateLimitGuard.baseline.json | 2 - backend/routes/integrations.ts | 4 +- 3 files changed, 111 insertions(+), 4 deletions(-) create mode 100644 backend/__tests__/unit/routes/integrations.connectRateLimit.test.js diff --git a/backend/__tests__/unit/routes/integrations.connectRateLimit.test.js b/backend/__tests__/unit/routes/integrations.connectRateLimit.test.js new file mode 100644 index 0000000000..e1805d9ad1 --- /dev/null +++ b/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); + }); +}); diff --git a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json index 1264d5cb59..470d1c4bfd 100644 --- a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json +++ b/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]", diff --git a/backend/routes/integrations.ts b/backend/routes/integrations.ts index 96635ac9fc..fa3bc74016 100644 --- a/backend/routes/integrations.ts +++ b/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; From 3103f9c8ebd0b4b26809383fa08cf70dbe69494b Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 22:14:14 -0700 Subject: [PATCH 2/3] fix(integrations): key the connector limiter on x-auth-token too (TASK-261) `auth` reads Authorization, then x-auth-token (auth.ts:34-35). The shared limiter key read only Authorization, so a caller that sent x-auth-token alone fell to the address branch and spent one 30-write budget with every caller behind its address. The key now reads both headers in auth's order. Raised by CodeRabbit on #2775. Two mutants each fail the new key test: a key that reads Authorization only, and one where x-auth-token wins. Co-Authored-By: Claude Opus 5.5 Co-authored-by: wren Agent-Model: claude/claude-opus-5-5/max --- .../middleware/integrationRateLimit.test.js | 19 ++++++++++++++++++- backend/middleware/integrationRateLimit.ts | 7 +++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/backend/__tests__/unit/middleware/integrationRateLimit.test.js b/backend/__tests__/unit/middleware/integrationRateLimit.test.js index 4c35d8bd01..673ee1bca0 100644 --- a/backend/__tests__/unit/middleware/integrationRateLimit.test.js +++ b/backend/__tests__/unit/middleware/integrationRateLimit.test.js @@ -8,7 +8,8 @@ // // 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. 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 +116,20 @@ 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'))); + }); }); diff --git a/backend/middleware/integrationRateLimit.ts b/backend/middleware/integrationRateLimit.ts index 4635d6e4f6..b2dc28d105 100644 --- a/backend/middleware/integrationRateLimit.ts +++ b/backend/middleware/integrationRateLimit.ts @@ -16,12 +16,15 @@ 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 read the way `auth` reads it (`auth.ts:34-35`): `Authorization`, +// else `x-auth-token`. Read only the first, an `x-auth-token` caller fell to the +// address branch and shared one budget with every caller behind its 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'); + const authHeader = req.get?.('authorization') || req.get?.('x-auth-token'); if (authHeader) { return `tok:${createHash('sha256').update(authHeader).digest('hex').slice(0, 16)}`; } From 9abac36d85f0287f2aeadd2f3760b99316a3350f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 25 Oct 2026 22:22:58 -0700 Subject: [PATCH 3/3] fix(integrations): key the connector limiter on the token auth extracts (TASK-261) `auth` takes `Authorization` without its `Bearer ` prefix, else `x-auth-token` (auth.ts:34-35). The key hashed the raw header, so one token sent as `Bearer t`, bare `t` and `x-auth-token: t` held three write budgets. The key now hashes the token `auth` extracts, so all three forms share one bucket. CodeRabbit raised it on 3103f9c8e. A new case in integrationRateLimit.test.js covers it; a mutant that hashes the raw Authorization header fails that case. Co-Authored-By: Claude Opus 5.5 Co-authored-by: wren Agent-Model: claude/claude-opus-5-5/max --- .../unit/middleware/integrationRateLimit.test.js | 13 ++++++++++++- backend/middleware/integrationRateLimit.ts | 15 +++++++++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/backend/__tests__/unit/middleware/integrationRateLimit.test.js b/backend/__tests__/unit/middleware/integrationRateLimit.test.js index 673ee1bca0..e026cd62b9 100644 --- a/backend/__tests__/unit/middleware/integrationRateLimit.test.js +++ b/backend/__tests__/unit/middleware/integrationRateLimit.test.js @@ -9,7 +9,8 @@ // 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 so are two `x-auth-token` values, the header `auth` falls -// back to. And the no-header branch buckets by the Cloudflare-recorded +// 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. // @@ -132,4 +133,14 @@ describe('integrationsRateLimitKey', () => { 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); + }); }); diff --git a/backend/middleware/integrationRateLimit.ts b/backend/middleware/integrationRateLimit.ts index b2dc28d105..fea57adcc3 100644 --- a/backend/middleware/integrationRateLimit.ts +++ b/backend/middleware/integrationRateLimit.ts @@ -16,17 +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 -// credential is read the way `auth` reads it (`auth.ts:34-35`): `Authorization`, -// else `x-auth-token`. Read only the first, an `x-auth-token` caller fell to the -// address branch and shared one budget with every caller behind its address. +// 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') || req.get?.('x-auth-token'); - 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.