From bc6bac62423487ae8825366f69ffb93e27ea4977 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 30 Jun 2026 14:22:55 +0200 Subject: [PATCH] fix(mcp-security): count cached deny verdicts against the pre-auth limiter Follow-up to 84fe7575 (v1.4.4 -> 1.4.5). Deny verdicts (allowed:false) are cached for 5 min, and the cache-hit path returned the cached 403 BEFORE the per-source-IP pre-auth limiter ran. So an attacker could repeat one forged or denied token and farm unlimited cheap cached 403s for the whole TTL without ever tripping the 429 throttle (verified: MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE=1 gave fetch=1 and 403,403,403,403). Fix: on a cache hit, count a cached allowed:false against isPreAuthRateLimited (clientIp) and return 429 once the budget is exhausted. Cached allowed:true stays free so legitimate users never burn the pre-auth budget on their cached fast-path. New regression test access-guard-preauth.test.ts: with the limit pinned to 1, a repeated denied token yields [403, 429, 429, 429] with a single backend call, and a repeated allowed token rides the cache 4x with no throttle. vitest 329 passed / 41 skipped; tsc clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../autopilot-monitor-mcp/package.json | 2 +- .../__tests__/access-guard-preauth.test.ts | 90 +++++++++++++++++++ .../autopilot-monitor-mcp/src/access-guard.ts | 10 +++ 3 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts diff --git a/src/McpServer/autopilot-monitor-mcp/package.json b/src/McpServer/autopilot-monitor-mcp/package.json index a77a117b..46d8e224 100644 --- a/src/McpServer/autopilot-monitor-mcp/package.json +++ b/src/McpServer/autopilot-monitor-mcp/package.json @@ -1,6 +1,6 @@ { "name": "autopilot-monitor-mcp", - "version": "1.4.4", + "version": "1.4.5", "description": "MCP server for Autopilot-Monitor — enables AI agents to query enrollment sessions, device properties, events and vulnerabilities", "type": "module", "bin": { diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts new file mode 100644 index 00000000..13761c9d --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/access-guard-preauth.test.ts @@ -0,0 +1,90 @@ +/** + * Regression test for the 2026-06-30 review finding: cached DENY verdicts must + * also count against the per-source-IP pre-auth limiter. + * + * Deny verdicts are cached for 5 min. Before the fix, the cache-hit path + * returned the cached 403 BEFORE the pre-auth check ran, so an attacker could + * repeat one forged/denied token and farm unlimited cheap 403s for the whole + * TTL without ever being throttled. The fix counts cached denials against the + * per-IP budget (→ 429 once exhausted) while keeping cached ALLOWs free. + * + * Pre-auth limit is pinned to 1/min BEFORE access-guard loads (it reads the env + * at module init). Own file so this tiny limit cannot bleed into other suites. + */ +import { describe, it, expect, vi, afterEach } from 'vitest'; +import type { Request, Response, NextFunction } from 'express'; + +process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE = '1'; +const { accessGuard } = await import('../access-guard.js'); + +function makeToken(claims: Record): string { + const header = Buffer.from(JSON.stringify({ alg: 'none', typ: 'JWT' })).toString('base64url'); + const payload = Buffer.from(JSON.stringify(claims)).toString('base64url'); + return `${header}.${payload}.sig`; +} +const validToken = (upn: string) => makeToken({ upn, exp: Math.floor(Date.now() / 1000) + 3600 }); + +/** Mock request carrying a Bearer token and a fixed client IP (→ getClientIp). */ +function mockReq(token: string, ip: string): Request { + return { + headers: { host: 'mcp.example.com', authorization: `Bearer ${token}` }, + protocol: 'https', + ip, + socket: { remoteAddress: ip }, + } as unknown as Request; +} + +interface Outcome { nextCalled: boolean; status: number | null; } + +function runGuard(req: Request): Promise { + return new Promise((resolve) => { + let statusCode: number | null = null; + let settled = false; + const done = (o: Outcome) => { if (!settled) { settled = true; resolve(o); } }; + const res: Partial = { + setHeader() { return this as Response; }, + status(code: number) { statusCode = code; return this as Response; }, + json() { done({ nextCalled: false, status: statusCode }); return this as Response; }, + }; + const next: NextFunction = () => done({ nextCalled: true, status: statusCode }); + accessGuard(req, res as Response, next); + setTimeout(() => done({ nextCalled: false, status: statusCode }), 2_000); + }); +} + +function stubBackend(body: unknown): ReturnType { + const fn = vi.fn(async () => ({ status: 200, text: async () => JSON.stringify(body) } as unknown as Response)); + vi.stubGlobal('fetch', fn); + return fn; +} + +afterEach(() => vi.unstubAllGlobals()); + +describe('pre-auth limiter — cached deny path', () => { + it('throttles repeated cached DENY to 429 after the budget (one backend call)', async () => { + const fetchFn = stubBackend({ allowed: false, reason: 'not on the MCP whitelist' }); + const req = mockReq(validToken('attacker@evil.example'), '198.51.100.7'); + + const r1 = await runGuard(req); // miss → backend → cached deny + const r2 = await runGuard(req); // cached deny, budget exhausted → 429 + const r3 = await runGuard(req); + const r4 = await runGuard(req); + + expect(fetchFn).toHaveBeenCalledTimes(1); // backend hit once; rest served from cache + expect(r1.status).toBe(403); // first denial surfaces the real 403 + expect([r2.status, r3.status, r4.status]).toEqual([429, 429, 429]); + }); + + it('does NOT charge cached ALLOW against the pre-auth budget', async () => { + // limit=1: if a cached allow consumed pre-auth budget, the 2nd request would + // 429. It must not — legitimate users ride the cache fast-path for free. + const fetchFn = stubBackend({ allowed: true, accessGrant: 'whitelisted' }); + const req = mockReq(validToken('legit@contoso.com'), '198.51.100.8'); + + const outs = [await runGuard(req), await runGuard(req), await runGuard(req), await runGuard(req)]; + + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(outs.every((o) => o.nextCalled)).toBe(true); + expect(outs.every((o) => o.status === null)).toBe(true); // no 429/403 sent + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index 9ef2284a..b716b9e6 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -116,6 +116,16 @@ async function checkAccess(upn: string, token: string, clientIp: string): Promis const cacheKey = buildCacheKey(upn, token); const cached = accessCache.get(cacheKey); if (cached && Date.now() < cached.expiresAt) { + // A cached DENY is cheap to serve but still costs request processing — and + // deny verdicts ARE cached, so without this an attacker could repeat one + // forged/denied token and farm unlimited cached 403s for the whole 5-min TTL + // while never tripping the pre-auth limiter (it only ran on the miss path). + // Count cached denials against the per-source-IP budget too, returning 429 + // once it is exhausted. Cached ALLOWs stay free, so a legitimate user never + // burns the pre-auth budget on their cached fast-path. + if (!cached.allowed && isPreAuthRateLimited(clientIp)) { + return { allowed: false, reason: 'Pre-auth rate limit exceeded', isGlobalAdmin: false, isGlobalReader: false, infraError: false, rateLimited: true }; + } return { allowed: cached.allowed, reason: cached.reason,