feat(security): mcp tier + handler rate-limit sweep

[simonholmes]

Summary

Follow-up to PR #211. Two related changes:

1. Adds a dedicated mcp tier (300/min, api-key keyed) to the rate-limit policy. MCP is server-to-server LLM-agent traffic with its own authentication model and bursty session shape — the api tier's 100/min IP-keyed cap was the wrong default. Two customers sharing a NAT'd egress now get independent buckets; the per-customer McpRateLimiter sub-cap inside the handler stays in place as the tunable knob.
2. Sweeps the redundant per-handler adminLimiter.check / apiLimiter.check blocks from ~150 handlers. The middleware enforces every section cap centrally since PR feat(security): middleware-driven rate limiting with policy table #211 merged; the in-handler calls were defense-in-depth that's now duplicate work. Codemod-driven, deterministic — .claude/tmp/sweep-{admin,api}-{limiter,tests}.mjs script is gitignored but documented in commit messages.

Behaviour changes worth reviewer attention

1. MCP rate limiting: was IP-keyed at 100/min via apiLimiter. Now api-key keyed at 300/min via the new mcp tier. Two distinct customers behind one egress IP no longer fight for one bucket. One API key abused across many IPs is now correctly bucketed under a single 300/min cap.
2. Orchestration tier cap raise: 30→120 per minute for /api/v1/admin/orchestration/**. This is the orchestration tier's documented design (chatty admin UI); the prior 30/min adminLimiter handler call was tighter than what PR feat(security): middleware-driven rate limiting with policy table #211 established for the orchestration tier. Section-level enforcement now comes solely from the middleware.
3. Keying shift on admin/api routes: handler-level limiter calls were IP-keyed (getClientIP). The middleware tier keys on session.user.id with IP fallback. Better behaviour for CGNAT/office shared IPs; same identity model PR feat(security): middleware-driven rate limiting with policy table #211 established for the catch-all.

Out of scope (intentionally)

• The conversations/export/route.ts route keeps its adminLimiter.check(\export:${ip}`)` call — that's a deliberate per-flow custom-token sub-cap on the admin bucket, not a section-level redundancy. Codemod detected the template-literal token shape and skipped the file.
• 11 source files dropped below 80% branch coverage post-sweep — all pre-existing error-path gaps (NotFoundError, ForbiddenError, nullish-coalescing fallbacks) that the deleted rate-limit tests were incidentally padding through their happy-path `mockReturnValue({ success: true })` setups. The gaps were already there; removing the dead tests just made them visible. Tracked for a separate test-hygiene PR rather than padding this diff.

Test plan

• All 4 internal gates clean: `/pre-pr`, `/test-review lib/security`, `/test-fix --findings=2`, `/security-review`
• Full suite: 850/850 files passing, 17,718 tests, 6 skipped, 10 todo
• All 3 changed source files in commit 1 (mcp tier) at 100/100/100/100 coverage
• Type-check, lint, format clean
• Net diff: 319 files changed, +281 / -4,857 lines (mostly mechanical deletions)
• Manual smoke against dev DB to confirm MCP traffic routes to the new tier (check X-RateLimit-Limit: 300 on a /api/v1/mcp/* 429 response)

🤖 Generated with Claude Code

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
sunrise	Ready	Preview, Comment	May 23, 2026 12:07am

[simonholmes]

Code review

Found 3 issues:

1. The vi.mock('@/lib/security/rate-limit', ...) block was deleted from the export route's test by the codemod, but the handler still calls adminLimiter.check(\export:${ip}`)` directly as an intentional per-flow sub-cap. The 15 tests in this file now hit the real LRU bucket; under high parallelism or repeat runs within the sliding window, the 30/min cap could trip and cause spurious 429s on unrelated tests sharing the process.

   sunrise/tests/unit/app/api/v1/admin/orchestration/conversations/export/route.test.ts

   Lines 10 to 42 in b49e817

   	// ─── Module mocks ───────────────────────────────────────────────────────
   	vi.mock('@/lib/auth/config', () => ({
   	auth: { api: { getSession: vi.fn() } },
   	}));
   	vi.mock('next/headers', () => ({
   	headers: vi.fn(() => Promise.resolve(new Headers())),
   	}));
   	vi.mock('@/lib/db/client', () => ({
   	prisma: {
   	aiConversation: {
   	findMany: vi.fn(),
   	count: vi.fn().mockResolvedValue(0),
   	},
   	},
   	}));
   	vi.mock('@/lib/security/ip', () => ({
   	getClientIP: vi.fn(() => '127.0.0.1'),
   	}));
   	vi.mock('@/lib/api/context', () => ({
   	getRouteLogger: vi.fn().mockResolvedValue({
   	info: vi.fn(),
   	warn: vi.fn(),
   	error: vi.fn(),
   	debug: vi.fn(),
   	}),
   	}));
   	// ─── Imports ────────────────────────────────────────────────────────────

   Suggested fix: add vi.mock('@/lib/security/rate-limit', () => ({ adminLimiter: { check: vi.fn(() => ({ success: true })) }, createRateLimitResponse: vi.fn() })) to the mock block at the top of the file, OR reset adminLimiter for the export: token in a beforeEach.

2. app/api/v1/mcp/route.ts POST handler still has numbered step comments // 3. through // 7. after commit 587e20b4 removed the original step 1 ("IP-level rate limit") and replaced step 2 ("Authenticate bearer token") with an unnumbered prose comment. The sequence now opens at 3 with no preceding 1 or 2 — a reader following the numbers gets a non-sequitur.

   sunrise/app/api/v1/mcp/route.ts

   Lines 58 to 75 in b49e817

   	);
   	}
   	// 3. Check MCP server is enabled
   	const serverState = await getMcpServerConfig();
   	if (!serverState.isEnabled) {
   	return Response.json(
   	{
   	jsonrpc: '2.0',
   	id: null,
   	error: { code: JsonRpcErrorCode.SERVER_DISABLED, message: 'MCP server is disabled' },
   	},
   	{ status: 503 }
   	);
   	}
   	// 4. Parse request body with size limit
   	const contentLength = request.headers.get('content-length');

   Suggested fix: renumber the remaining steps to // 1. – // 5., or drop the numbering entirely (per the project's "default to writing no comments" convention).

3. lib/security/rate-limit.ts:226-228 JSDoc on adminLimiter blanket-forbids direct handler calls ("Route handlers should NOT call .check() directly"), and .context/security/rate-limiting.md explicitly flags the adminLimiter.check(ip) pattern as an anti-pattern. But conversations/export/route.ts:34 deliberately calls adminLimiter.check(\export:${ip}`)` with a custom token — a per-resource sub-cap that the codemod intentionally preserves. JSDoc + project docs + code disagree.

   sunrise/lib/security/rate-limit.ts

   Lines 220 to 232 in b49e817

   	/**
   	* Rate limiter for core admin endpoints (user management, logs, invitations, etc.)
   	* Limit: 30 requests per minute. Override with `RATE_LIMIT_ADMIN`.
   	*
   	* Wired into the middleware via `RATE_LIMIT_TIERS['admin']` and the
   	* `/api/v1/admin/` rule in `rate-limit-policy.ts`. Route handlers should
   	* NOT call `.check()` directly — the dispatcher already applied this cap.
   	* See `.context/security/rate-limiting.md` for the layered model.
   	*/
   	export const adminLimiter = createRateLimiter({
   	interval: SECURITY_CONSTANTS.RATE_LIMIT.DEFAULT_INTERVAL,
   	maxRequests: SECURITY_CONSTANTS.RATE_LIMIT.LIMITS.ADMIN,

   sunrise/app/api/v1/admin/orchestration/conversations/export/route.ts

   Lines 19 to 37 in b49e817

   	import { csvEscape } from '@/lib/api/csv';
   	import { getRouteLogger } from '@/lib/api/context';
   	import { adminLimiter, createRateLimitResponse } from '@/lib/security/rate-limit';
   	import { getClientIP } from '@/lib/security/ip';
   	import { conversationExportQuerySchema } from '@/lib/validations/orchestration';
   	/** Maximum conversations per export to prevent memory issues. */
   	const MAX_EXPORT_CONVERSATIONS = 500;
   	/** Maximum messages per conversation in exports to bound memory usage. */
   	const MAX_MESSAGES_PER_CONVERSATION = 500;
   	export const GET = withAdminAuth(async (request, session) => {
   	// Extra rate limit for exports — 1/min per admin IP
   	const ip = getClientIP(request);
   	const rl = adminLimiter.check(`export:${ip}`);
   	if (!rl.success) return createRateLimitResponse(rl);
   	const log = await getRouteLogger(request);

   Suggested fix (architecturally cleaner): introduce a dedicated exportLimiter as a per-flow sub-cap in lib/security/rate-limit.ts (mirroring contactLimiter / uploadLimiter), have conversations/export/route.ts use it, and drop the codemod's special-case skip. The documented "section limiters never called from handlers" invariant then holds without an exception.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}