diff --git a/backend/__tests__/unit/routes/registry.catalog-rate-limit-scope.test.js b/backend/__tests__/unit/routes/registry.catalog-rate-limit-scope.test.js new file mode 100644 index 0000000000..c4dd450305 --- /dev/null +++ b/backend/__tests__/unit/routes/registry.catalog-rate-limit-scope.test.js @@ -0,0 +1,44 @@ +/** + * D175 — the catalog's IP limiter must stay on the catalog's own routes. + * + * registry.ts mounts catalogRouter ahead of pod-agents, files, install and + * the rest. A `catalogRouter.use(limiter)` therefore ran for EVERY + * /api/registry/* request that reached it, and /v2/agents (one + * `/pods/:podId/agents` read per pod) hit 429 on all 62 calls from its third + * load on a 62-pod account (prod, 10-28). The limiter skips itself under + * NODE_ENV=test, so this guards the shape instead: catalogRouter carries no + * router-level middleware, and every route it declares runs the limiter. + */ + +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => next())); +jest.mock('../../../models/Pod', () => ({})); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentRegistry: { search: jest.fn(), getByName: jest.fn() }, + AgentInstallation: { find: jest.fn(), getInstalledAgents: jest.fn() }, +})); + +const catalogRouter = require('../../../routes/registry/catalog'); + +describe('catalog rate limiter scope (D175)', () => { + const layers = catalogRouter.stack; + + it('has no router-level middleware that would run for later registry routers', () => { + const routerLevel = layers.filter((layer) => !layer.route); + expect(routerLevel).toHaveLength(0); + }); + + it('runs the same limiter on every catalog route, ahead of auth', () => { + const routes = layers.filter((layer) => layer.route); + expect(routes.length).toBeGreaterThan(0); + const firstHandlers = routes.map((layer) => layer.route.stack[0].handle); + const limiter = firstHandlers[0]; + expect(firstHandlers.every((handle) => handle === limiter)).toBe(true); + // express-rate-limit's middleware exposes resetKey; auth does not. + expect(typeof limiter.resetKey).toBe('function'); + }); + + it('does not declare /pods/:podId/agents, which belongs to pod-agents', () => { + const paths = layers.filter((layer) => layer.route).map((layer) => layer.route.path); + expect(paths).not.toContain('/pods/:podId/agents'); + }); +}); diff --git a/backend/__tests__/unit/routes/registry.rate-limit-coverage.test.js b/backend/__tests__/unit/routes/registry.rate-limit-coverage.test.js new file mode 100644 index 0000000000..8ba76e2b9a --- /dev/null +++ b/backend/__tests__/unit/routes/registry.rate-limit-coverage.test.js @@ -0,0 +1,73 @@ +/** + * D175 follow-up: scoping the catalog limiter to catalog routes must not strip + * rate limiting from the routers mounted after it. They used to inherit the + * catalog's router-level limiter; they now share a registry-wide IP limiter + * mounted right after the catalog, and generate-avatar (which calls a paid + * image provider per request) also carries its own per-caller limiter. + * Limiters skip themselves under NODE_ENV=test, so these guard the shape. + */ +const fs = require('fs'); +const path = require('path'); + +jest.mock('../../../middleware/auth', () => jest.fn((req, res, next) => next())); +jest.mock('../../../models/AgentRegistry', () => ({ AgentRegistry: {} })); +jest.mock('../../../services/agentIdentityService', () => ({})); +jest.mock('../../../routes/registry/helpers', () => ({ getUserId: jest.fn(), resolveUsername: jest.fn() })); +jest.mock('../../../utils/agentManifestRegistry', () => ({ + ManifestValidationError: class extends Error {}, + normalizePublishPayload: jest.fn(), +})); + +describe('registry rate-limit coverage (D175)', () => { + it('mounts a registry-wide limiter directly after the catalog router', () => { + const src = fs.readFileSync(path.join(__dirname, '../../../routes/registry.ts'), 'utf8'); + const mounts = [...src.matchAll(/router\.use\((require\('\.\/registry\/([a-z-]+)'\)|rateLimit\()/g)] + .map((m) => m[2] || 'LIMITER'); + const catalogAt = mounts.indexOf('catalog'); + expect(catalogAt).toBeGreaterThan(-1); + expect(mounts[catalogAt + 1]).toBe('LIMITER'); + expect(mounts.filter((m) => m === 'LIMITER')).toHaveLength(1); + }); + + it('runs a limiter before auth on POST /generate-avatar', () => { + const publishRouter = require('../../../routes/registry/publish'); + const layer = publishRouter.stack.find((l) => l.route && l.route.path === '/generate-avatar'); + expect(layer).toBeDefined(); + expect(typeof layer.route.stack[0].handle.resetKey).toBe('function'); + }); + + it('gives every spelling of one token one shared avatar budget, through the route', async () => { + // The limiter skips itself under NODE_ENV=test and reads it per request, + // so lift that for this test only. auth is mocked to pass; the limiter + // runs ahead of it, so the budget it enforces is the one under test. + const express = require('express'); + const request = require('supertest'); + const publishRouter = require('../../../routes/registry/publish'); + const app = express(); + app.use(express.json()); + app.use(publishRouter); + const spellings = [ + { Authorization: 'Bearer cm_avatar_budget' }, + { Authorization: 'cm_avatar_budget' }, + { 'x-auth-token': 'cm_avatar_budget' }, + ]; + const saved = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + try { + const statuses = []; + for (let i = 0; i < 11; i += 1) { + // An empty body is answered 400 by the handler, after the limiter has counted it. + // eslint-disable-next-line no-await-in-loop + const res = await request(app).post('/generate-avatar').set(spellings[i % 3]).send({}); + statuses.push(res.status); + } + expect(statuses.slice(0, 10).every((status) => status === 400)).toBe(true); + expect(statuses[10]).toBe(429); + + const other = await request(app).post('/generate-avatar').set({ Authorization: 'Bearer cm_someone_else' }).send({}); + expect(other.status).toBe(400); + } finally { + process.env.NODE_ENV = saved; + } + }); +}); diff --git a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json index a7c8fb7599..74411092e0 100644 --- a/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json +++ b/backend/__tests__/unit/routes/routeRateLimitGuard.baseline.json @@ -110,7 +110,6 @@ "routes/registry/pod-agents.ts GET /pods/:podId/agents [unlimited]", "routes/registry/pod-agents.ts GET /pods/:podId/agents/:name [unlimited]", "routes/registry/presets-router.ts GET /presets [unlimited]", - "routes/registry/publish.ts POST /generate-avatar [unlimited]", "routes/registry/publish.ts POST /publish [unlimited]", "routes/registry/publish.ts POST /seed [unlimited]", "routes/registry/templates.ts DELETE /templates/:id [unlimited]", diff --git a/backend/routes/registry.ts b/backend/routes/registry.ts index f7ec3802d6..34aeeaffa6 100644 --- a/backend/routes/registry.ts +++ b/backend/routes/registry.ts @@ -8,6 +8,11 @@ // eslint-disable-next-line global-require const express = require('express'); +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { rateLimit } = require('express-rate-limit'); +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { cloudflareIpRateLimitKeyGenerator } = require('../middleware/ipRateLimit'); + const router: ReturnType = express.Router(); // eslint-disable-next-line global-require @@ -18,6 +23,20 @@ router.use(require('./registry/manage-grants')); router.use(require('./registry/templates')); // eslint-disable-next-line global-require router.use(require('./registry/catalog')); +// Every router mounted below shares this per-IP budget (D175). It used to +// share the catalog's 120/min, which a per-pod read like /v2/agents spends in +// two page loads. Routers above it (runtime, grants, templates) are not +// covered, as before. Sized for fan-out: a 62-pod account is ~125 requests +// per load. +router.use(rateLimit({ + windowMs: 60_000, + max: 1200, + standardHeaders: true, + legacyHeaders: false, + skip: () => process.env.NODE_ENV === 'test', + keyGenerator: (req: any) => cloudflareIpRateLimitKeyGenerator(req), + handler: (_req: any, res: any) => res.status(429).json({ code: 'rate_limited' }), +})); // eslint-disable-next-line global-require router.use(require('./registry/admin')); // eslint-disable-next-line global-require diff --git a/backend/routes/registry/catalog.ts b/backend/routes/registry/catalog.ts index 8bb1a1aa2e..4ecd21fe0b 100644 --- a/backend/routes/registry/catalog.ts +++ b/backend/routes/registry/catalog.ts @@ -25,7 +25,13 @@ const catalogRouter = express.Router(); // ~120 req/min/IP, same shape as agentProfile's limiter: generous for a // human browsing the catalog, low enough to blunt scrapers now that every // listing request may also cost a role lookup. Skipped in tests. -catalogRouter.use(rateLimit({ +// +// Attached per route, never via `catalogRouter.use`: registry.ts mounts this +// router ahead of pod-agents, files, install and the rest, so a router-level +// limiter ran for every /api/registry/* request and spent the catalog budget +// on them. /v2/agents reads `/pods/:podId/agents` once per pod, and a 62-pod +// account hit 429 on every call from its third load (D175, 10-28). +const catalogRateLimit = rateLimit({ windowMs: 60_000, max: 120, standardHeaders: true, @@ -33,10 +39,10 @@ catalogRouter.use(rateLimit({ skip: () => process.env.NODE_ENV === 'test', keyGenerator: (req: any) => cloudflareIpRateLimitKeyGenerator(req), handler: (_req: any, res: any) => res.status(429).json({ code: 'rate_limited' }), -})); +}); /** Suggest a free BYO name across the caller's installable pods. */ -catalogRouter.get('/agents/next-free-name', auth, async (req: any, res: any) => { +catalogRouter.get('/agents/next-free-name', catalogRateLimit, auth, async (req: any, res: any) => { try { const userId = getUserId(req); if (!userId || !mongoose.Types.ObjectId.isValid(String(userId))) { @@ -96,7 +102,7 @@ const findExistingAgentInstance = async (agentName: any, instanceId: any) => { * GET /api/registry/agents * List available agents in the registry */ -catalogRouter.get('/agents', auth, async (req: any, res: any) => { +catalogRouter.get('/agents', catalogRateLimit, auth, async (req: any, res: any) => { try { const { q, category, verified, registry, limit = 20, offset = 0, @@ -161,7 +167,7 @@ catalogRouter.get('/agents', auth, async (req: any, res: any) => { * GET /api/registry/agents/:name * Get agent details */ -catalogRouter.get('/agents/:name', auth, async (req: any, res: any) => { +catalogRouter.get('/agents/:name', catalogRateLimit, auth, async (req: any, res: any) => { try { const agent = await AgentRegistry.getByName(req.params.name); @@ -201,7 +207,7 @@ catalogRouter.get('/agents/:name', auth, async (req: any, res: any) => { * GET /api/registry/agents/:name/instances/:instanceId * Check if an agent instance exists globally (across all pods). */ -catalogRouter.get('/agents/:name/instances/:instanceId', auth, async (req: any, res: any) => { +catalogRouter.get('/agents/:name/instances/:instanceId', catalogRateLimit, auth, async (req: any, res: any) => { try { const { name, instanceId } = req.params; const userId = getUserId(req); @@ -242,7 +248,7 @@ catalogRouter.get('/agents/:name/instances/:instanceId', auth, async (req: any, * GET /api/registry/agents/:name/instances * List all instances of an agent type (for discovery). */ -catalogRouter.get('/agents/:name/instances', auth, async (req: any, res: any) => { +catalogRouter.get('/agents/:name/instances', catalogRateLimit, auth, async (req: any, res: any) => { try { const { name } = req.params; const userId = getUserId(req); @@ -291,7 +297,7 @@ catalogRouter.get('/agents/:name/instances', auth, async (req: any, res: any) => * GET /api/registry/categories * List agent categories */ -catalogRouter.get('/categories', auth, async (req: any, res: any) => { +catalogRouter.get('/categories', catalogRateLimit, auth, async (req: any, res: any) => { try { const categories = await AgentRegistry.distinct('categories'); res.json({ categories }); @@ -305,7 +311,7 @@ catalogRouter.get('/categories', auth, async (req: any, res: any) => { * GET /api/registry/openclaw/bundled-skills * List bundled gateway skills available under /app/skills. */ -catalogRouter.get('/openclaw/bundled-skills', auth, async (req: any, res: any) => { +catalogRouter.get('/openclaw/bundled-skills', catalogRateLimit, auth, async (req: any, res: any) => { try { const userId = getUserId(req); if (!userId) { @@ -329,7 +335,7 @@ catalogRouter.get('/openclaw/bundled-skills', auth, async (req: any, res: any) = * GET /api/registry/agents/:name/installations * List pods where an agent instance is installed (user-visible only) */ -catalogRouter.get('/agents/:name/installations', auth, async (req: any, res: any) => { +catalogRouter.get('/agents/:name/installations', catalogRateLimit, auth, async (req: any, res: any) => { try { const userId = getUserId(req); if (!userId) { diff --git a/backend/routes/registry/publish.ts b/backend/routes/registry/publish.ts index ebee06c4d0..30fb7b2806 100644 --- a/backend/routes/registry/publish.ts +++ b/backend/routes/registry/publish.ts @@ -1,7 +1,12 @@ // Agent publish/seed/avatar routes — extracted from registry.js (GH#112) // Handles: POST /publish, POST /seed, POST /generate-avatar +const { createHash } = require('crypto'); const express = require('express'); +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { rateLimit } = require('express-rate-limit'); const auth = require('../../middleware/auth'); +// eslint-disable-next-line @typescript-eslint/no-require-imports, global-require +const { cloudflareIpRateLimitKeyGenerator } = require('../../middleware/ipRateLimit'); const { AgentRegistry } = require('../../models/AgentRegistry'); const AgentIdentityService = require('../../services/agentIdentityService'); const { getUserId, resolveUsername } = require('./helpers'); @@ -12,6 +17,28 @@ const { const publishRouter = express.Router(); +// generate-avatar calls the configured image provider (Gemini or OpenAI) on +// every request, so it gets its own per-caller budget on top of the +// registry-wide IP limiter. It runs ahead of `auth` (which does a Mongo +// lookup), so it derives the token exactly as `auth` does: every spelling that +// `auth` accepts as one token (`Bearer cm_x`, `cm_x`, `x-auth-token: cm_x`) +// shares one budget. Only a hash of the token is kept in limiter state. +const avatarCallerKey = (req: any) => { + const token = req.header?.('Authorization')?.replace('Bearer ', '') || req.header?.('x-auth-token'); + if (token) return `tok:${createHash('sha256').update(token).digest('hex').slice(0, 16)}`; + return cloudflareIpRateLimitKeyGenerator(req); +}; + +const avatarRateLimit = rateLimit({ + windowMs: 60_000, + max: 10, + standardHeaders: true, + legacyHeaders: false, + skip: () => process.env.NODE_ENV === 'test', + keyGenerator: avatarCallerKey, + handler: (_req: any, res: any) => res.status(429).json({ code: 'rate_limited' }), +}); + /** * POST /api/registry/publish * Publish a new agent to the registry (for developers) @@ -199,7 +226,7 @@ publishRouter.post('/seed', auth, async (req: any, res: any) => { * POST /api/registry/generate-avatar * Generate AI avatar for an agent */ -publishRouter.post('/generate-avatar', auth, async (req: any, res: any) => { +publishRouter.post('/generate-avatar', avatarRateLimit, auth, async (req: any, res: any) => { try { // eslint-disable-next-line global-require const AgentAvatarService = require('../../services/agentAvatarService');