From 5186706db15bb6feabfd707cca696e9c7a3478b0 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Thu, 15 Oct 2026 21:10:41 -0700 Subject: [PATCH] test(hosted): the hosted suite runs without the rate limiter, which gets its own suite routes/hosted.test.js makes more requests than the route limiter's 30 a minute, all on one shared bucket, so adding two requests made an unrelated deprovision arm 429, and only on a full-file run (sprint-review on #2256, via Growth). Two arms had worked around it with private Authorization buckets. - hosted.test.js passes express-rate-limit through (test-only; the route and production config are unchanged), drops the two workarounds, and pins the pass-through: 35 requests in a row are never 429. - hosted.rateLimit.test.js tests the real limiter on its own: 30 requests a minute per bearer then 429 with the stated message, another bearer has its own budget, and with no bearer the key is the address Cloudflare reports, each address its own budget. Mutations caught: dropping the pass-through, one key for every bearer, max 31, ignoring CF-Connecting-IP. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wkz5axAzCvoLEFkZ5KtkKG --- .../unit/routes/hosted.rateLimit.test.js | 63 +++++++++++++++++++ backend/__tests__/unit/routes/hosted.test.js | 42 ++++++++----- 2 files changed, 88 insertions(+), 17 deletions(-) create mode 100644 backend/__tests__/unit/routes/hosted.rateLimit.test.js diff --git a/backend/__tests__/unit/routes/hosted.rateLimit.test.js b/backend/__tests__/unit/routes/hosted.rateLimit.test.js new file mode 100644 index 0000000000..7f31f7a04d --- /dev/null +++ b/backend/__tests__/unit/routes/hosted.rateLimit.test.js @@ -0,0 +1,63 @@ +/** + * /api/hosted's rate limiter, on its own (hosted.test.js passes it through, + * because that file makes more requests than one minute's budget). + * + * Contract under test: + * - 30 requests a minute per bearer, then 429 with the stated message + * - a different bearer has its own budget + * - with no bearer, the key falls back to the client address Cloudflare reports + */ +const request = require('supertest'); +const express = require('express'); + +jest.mock('../../../middleware/auth', () => (req, _res, next) => { + req.user = { id: 'owner-1' }; + next(); +}); +jest.mock('../../../services/hostedRuntimeService', () => ({ + HOSTED_RUNTIME_TYPE: 'hosted', + isConfigured: () => false, + isHostedInstallation: () => false, + hostedCaps: () => ({ agentsPerUser: 1, turnsPerDay: 30 }), + hostedCapsForOwner: async () => ({ agentsPerUser: 1, turnsPerDay: 30 }), +})); +jest.mock('../../../services/nativeRuntimeService', () => ({ + isNativeRuntimeAvailable: () => false, + hostedEngineAvailable: () => false, +})); +jest.mock('../../../services/onboardingMarks', () => ({ markByoOpened: jest.fn() })); + +const hostedRouter = require('../../../routes/hosted'); + +const app = express(); +app.use(express.json()); +app.use('/api/hosted', hostedRouter); + +const availability = (headers = {}) => request(app).get('/api/hosted/availability').set(headers); + +describe('/api/hosted rate limiter', () => { + it('allows 30 requests a minute per bearer, then answers 429, while another bearer still gets through', async () => { + const statuses = []; + for (let i = 0; i < 31; i += 1) { + // eslint-disable-next-line no-await-in-loop + statuses.push((await availability({ Authorization: 'Bearer owner-a' })).status); + } + expect(statuses.slice(0, 30).filter((status) => status === 429)).toEqual([]); + const limited = await availability({ Authorization: 'Bearer owner-a' }); + expect(limited.status).toBe(429); + expect(limited.body.message).toMatch(/30 hosted-runtime requests per 60s/); + + expect((await availability({ Authorization: 'Bearer owner-b' })).status).not.toBe(429); + }); + + it('with no bearer, keys on the client address Cloudflare reports', async () => { + const statuses = []; + for (let i = 0; i < 31; i += 1) { + // eslint-disable-next-line no-await-in-loop + statuses.push((await availability({ 'CF-Connecting-IP': '203.0.113.9' })).status); + } + expect(statuses.slice(0, 30).filter((status) => status === 429)).toEqual([]); + expect(statuses[30]).toBe(429); + expect((await availability({ 'CF-Connecting-IP': '203.0.113.10' })).status).not.toBe(429); + }); +}); diff --git a/backend/__tests__/unit/routes/hosted.test.js b/backend/__tests__/unit/routes/hosted.test.js index 21c4908049..3e80fff053 100644 --- a/backend/__tests__/unit/routes/hosted.test.js +++ b/backend/__tests__/unit/routes/hosted.test.js @@ -25,6 +25,12 @@ const mockHosted = { }, }; +// The route's limiter (30 a minute, module-level) is passed through here: this +// file makes more requests than one minute's budget, and a shared bucket made +// unrelated arms 429 on a full-file run (sprint-review on #2256). The limiter +// and its key are tested on their own in hosted.rateLimit.test.js. +jest.mock('express-rate-limit', () => ({ __esModule: true, default: () => (_req, _res, next) => next() })); + jest.mock('../../../middleware/auth', () => (req, _res, next) => { req.user = { id: 'owner-1' }; next(); @@ -86,12 +92,6 @@ const app = express(); app.use(express.json()); app.use('/api/hosted', hostedRouter); -// The route's limiter (30 a minute) is module-level and shared by this file; -// the migration arms use their own key so they do not spend the others' budget. -// The auth middleware is mocked, so the header is read only by the limiter. -let armSeq = 0; -const asMigrationArm = (req) => { armSeq += 1; return req.set('Authorization', `Bearer migration-arm-${armSeq}`); }; - let installationSeq = 0; const makeInstallation = (overrides = {}) => { installationSeq += 1; @@ -108,6 +108,18 @@ const makeInstallation = (overrides = {}) => { return installation; }; + +// Pins the pass-through above: if the limiter mock stopped applying (an import +// change), this file's later arms would start to 429 again on a full run. +it('this suite runs without the rate limiter: 35 requests in a row are never 429', async () => { + const statuses = []; + for (let i = 0; i < 35; i += 1) { + // eslint-disable-next-line no-await-in-loop + statuses.push((await request(app).get('/api/hosted/availability')).status); + } + expect(statuses.filter((status) => status === 429)).toEqual([]); +}); + describe('/api/hosted', () => { beforeEach(() => { jest.clearAllMocks(); @@ -366,7 +378,7 @@ describe('/api/hosted', () => { }); mockHosted.deprovisionAgent.mockResolvedValue({ deprovisioned: true }); - const res = await asMigrationArm(request(app).post('/api/hosted/provision')).send({ agentName: 'scout' }); + const res = await request(app).post('/api/hosted/provision').send({ agentName: 'scout' }); expect(res.status).toBe(409); expect(res.body.code).toBe('hosted_engine_migrated'); @@ -377,7 +389,7 @@ describe('/api/hosted', () => { expect(JSON.stringify(res.body)).not.toContain('cm_agent_secret'); // The next provision finds the native record and mints nothing. - const again = await asMigrationArm(request(app).post('/api/hosted/provision')).send({ agentName: 'scout' }); + const again = await request(app).post('/api/hosted/provision').send({ agentName: 'scout' }); expect(again.status).toBe(200); expect(again.body.engine).toBe('native'); expect(mockIssueToken).toHaveBeenCalledTimes(1); @@ -395,7 +407,7 @@ describe('/api/hosted', () => { mockHosted.deprovisionAgent.mockRejectedValue(new Error('worker unreachable')); jest.spyOn(console, 'warn').mockImplementation(() => {}); - const res = await asMigrationArm(request(app).post('/api/hosted/provision')).send({ agentName: 'scout' }); + const res = await request(app).post('/api/hosted/provision').send({ agentName: 'scout' }); expect(res.status).toBe(409); expect(mockRevokeRuntimeTokens).toHaveBeenCalledTimes(2); @@ -410,7 +422,7 @@ describe('/api/hosted', () => { return { deprovisioned: true }; }); - const res = await asMigrationArm(request(app).post('/api/hosted/deprovision')).send({ agentName: 'scout' }); + const res = await request(app).post('/api/hosted/deprovision').send({ agentName: 'scout' }); expect(res.status).toBe(200); expect(installation.config.get('hosted')).toEqual(MIGRATED); @@ -495,15 +507,11 @@ describe('/api/hosted on the native engine', () => { }); it('names the agent on the card by its display name first', async () => { - // Its own rate-limit bucket: the limiter keys on Authorization (auth is - // mocked), and this file's other requests already sit at its 30-a-minute - // ceiling, so two more from the shared key 429 a later test. - const ownBucket = { Authorization: 'Bearer hosted-label-test' }; mockUserFindOne.mockResolvedValue({ _id: 'bot-1', username: 'scout', agentRuntimeTokens: [], botMetadata: { displayName: 'Research Scout' }, }); mockFindOne.mockResolvedValue(nativeInstallation()); - await request(app).post('/api/hosted/provision').set(ownBucket).send({ agentName: 'scout' }); + await request(app).post('/api/hosted/provision').send({ agentName: 'scout' }); expect(mockCompleteConnectAgent.mock.calls[0][0].agentLabel).toBe('Research Scout'); mockCompleteConnectAgent.mockClear(); @@ -512,7 +520,7 @@ describe('/api/hosted on the native engine', () => { displayName: 'Lab Notes', config: new Map([['runtime', { runtimeType: 'hosted' }], ['hosted', { engine: 'native', provisionedAt: PROVISIONED }]]), })); - await request(app).post('/api/hosted/provision').set(ownBucket).send({ agentName: 'scout' }); + await request(app).post('/api/hosted/provision').send({ agentName: 'scout' }); expect(mockCompleteConnectAgent.mock.calls[0][0].agentLabel).toBe('Lab Notes'); }); @@ -571,7 +579,7 @@ describe('/api/hosted on the native engine', () => { return read; }); - const res = await asMigrationArm(request(app).post(`/api/hosted/${route}`)).send({ agentName: 'scout' }); + const res = await request(app).post(`/api/hosted/${route}`).send({ agentName: 'scout' }); expect(res.status).toBe(200); const hosted = installation.config.get('hosted');