diff --git a/backend/__tests__/unit/middleware/integrationRateLimit.test.js b/backend/__tests__/unit/middleware/integrationRateLimit.test.js index 4c35d8bd01..e026cd62b9 100644 --- a/backend/__tests__/unit/middleware/integrationRateLimit.test.js +++ b/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); + }); }); 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/middleware/integrationRateLimit.ts b/backend/middleware/integrationRateLimit.ts index 4635d6e4f6..fea57adcc3 100644 --- a/backend/middleware/integrationRateLimit.ts +++ b/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. 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;