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) <noreply@anthropic.com>

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": {

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, unknown>): 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<Outcome> {
+  return new Promise<Outcome>((resolve) => {
+    let statusCode: number | null = null;
+    let settled = false;
+    const done = (o: Outcome) => { if (!settled) { settled = true; resolve(o); } };
+    const res: Partial<Response> = {
+      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<typeof vi.fn> {
+  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
+  });
+});

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,