diff --git a/src/McpServer/autopilot-monitor-mcp/package-lock.json b/src/McpServer/autopilot-monitor-mcp/package-lock.json index e35f4526..cec83e18 100644 --- a/src/McpServer/autopilot-monitor-mcp/package-lock.json +++ b/src/McpServer/autopilot-monitor-mcp/package-lock.json @@ -1,12 +1,12 @@ { "name": "autopilot-monitor-mcp", - "version": "1.4.5", + "version": "1.4.6", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "autopilot-monitor-mcp", - "version": "1.4.5", + "version": "1.4.6", "dependencies": { "@huggingface/transformers": "^3.8.1", "@modelcontextprotocol/sdk": "^1.28.0", diff --git a/src/McpServer/autopilot-monitor-mcp/package.json b/src/McpServer/autopilot-monitor-mcp/package.json index c3a22f79..19af53e8 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.5", + "version": "1.4.6", "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__/oauth-rate-limit.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-rate-limit.test.ts new file mode 100644 index 00000000..3aa7a206 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-rate-limit.test.ts @@ -0,0 +1,91 @@ +/** + * Regression tests for the 2026-07-02 review finding R-3: the unauthenticated + * /oauth/* routes had NO rate limit (accessGuard covers /mcp only), making + * /oauth/token an anonymous amplifier for outbound Entra calls carrying the + * client secret — a flood would get the app registration itself throttled by + * Entra (org-wide auth DoS with zero credentials). + * + * Both budgets are pinned tiny BEFORE oauth.js loads (access-guard reads the + * env at module init). Own file so the tiny limits cannot bleed into other + * suites. No network is reached: every request fails validation (unsupported + * grant / missing code) before the handler would call Entra, and the limiter + * runs before the handler anyway. + */ +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import express from 'express'; +import type { Server } from 'node:http'; +import type { AddressInfo } from 'node:net'; + +process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +process.env.MCP_OAUTH_RATE_LIMIT_PER_MINUTE = '3'; +process.env.MCP_OAUTH_TOKEN_RATE_LIMIT_PER_MINUTE = '2'; +const { createOAuthRouter } = await import('../oauth.js'); + +let server: Server; +let baseUrl: string; + +beforeAll(async () => { + const app = express(); + app.use(express.json()); + app.use(express.urlencoded({ extended: true })); + app.use(createOAuthRouter()); + await new Promise((resolve) => { + server = app.listen(0, '127.0.0.1', () => resolve()); + }); + const { port } = server.address() as AddressInfo; + baseUrl = `http://127.0.0.1:${port}`; +}); + +afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); +}); + +/** POST /oauth/token with an unsupported grant — 400s in the handler, never reaches Entra. */ +async function token() { + const res = await fetch(`${baseUrl}/oauth/token`, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams({ grant_type: 'client_credentials' }).toString(), + }); + return res; +} + +/** GET /oauth/callback with no params — 400s in the handler (missing code). */ +async function callback() { + return fetch(`${baseUrl}/oauth/callback`); +} + +describe('/oauth/* per-source-IP rate limiting (R-3)', () => { + it('throttles /oauth/token after its budget, independently of the general /oauth budget', async () => { + // Budget pinned to 2: the first two reach the handler (400), the third is throttled. + const statuses = [(await token()).status, (await token()).status]; + const third = await token(); + statuses.push(third.status); + expect(statuses).toEqual([400, 400, 429]); + + expect(third.headers.get('Retry-After')).toBe('60'); + const body = (await third.json()) as Record; + expect(body.error).toBe('Rate limit exceeded'); + expect(body.retryAfterSeconds).toBe(60); + expect(String(body.message)).toMatch(/authentication requests/); + + // Separate bucket: exhausting the token budget must NOT consume the + // general /oauth budget — callback still reaches its handler (400). + expect((await callback()).status).toBe(400); + }); + + it('throttles the general /oauth routes after their shared budget', async () => { + // Budget pinned to 3; the independence probe above already spent 1. + expect((await callback()).status).toBe(400); + expect((await callback()).status).toBe(400); + const throttled = await callback(); + expect(throttled.status).toBe(429); + expect(throttled.headers.get('Retry-After')).toBe('60'); + + // And the throttled general budget must not leak back into the token + // bucket's key space: token stays at ITS 429 (already exhausted above), + // proving the two 429s come from distinct buckets rather than one shared + // counter (which would have tripped far earlier). + expect((await token()).status).toBe(429); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts index cf8eef3f..c88ebbff 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-register.test.ts @@ -18,6 +18,12 @@ import type { AddressInfo } from 'node:net'; // createOAuthRouter() throws at import unless the Entra client id is present. process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +// The /oauth/* routes now carry per-source-IP throttles (read at module init). +// This suite fires far more than the production budgets from one loopback IP +// within a minute, so pin them out of the way — the limiter itself is covered +// by oauth-rate-limit.test.ts with tiny pinned budgets in its own module. +process.env.MCP_OAUTH_RATE_LIMIT_PER_MINUTE = '100000'; +process.env.MCP_OAUTH_TOKEN_RATE_LIMIT_PER_MINUTE = '100000'; const { createOAuthRouter, signClientId, verifyClientId, signState, redirectUriMatches, MAX_REDIRECT_URIS_PER_CLIENT, MAX_REDIRECT_URI_LENGTH, MAX_CLIENT_NAME_LENGTH } = await import('../oauth.js'); @@ -393,3 +399,44 @@ describe('/oauth/token — PKCE finishing move', () => { expect(String(json.error_description)).toMatch(/code_verifier is required/); }); }); + +describe('/oauth/token — grant_type allowlist', () => { + // Every request to this endpoint is forwarded to Entra with the app's + // client_id + client_secret attached. Unlisted grants (client_credentials, + // ROPC, device-code) previously failed only by accident of the pinned scope + // and which fields the proxy copies — these tests pin the allowlist as + // explicit proxy policy. All rejection paths return before any fetch, so no + // network is reached. + async function token(params: Record) { + const res = await fetch(`${baseUrl}/oauth/token`, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: new URLSearchParams(params).toString(), + }); + const json = (await res.json().catch(() => ({}))) as Record; + return { status: res.status, json }; + } + + it('rejects a missing grant_type as invalid_request', async () => { + const res = await token({ code: 'abc' }); + expect(res.status).toBe(400); + expect(res.json.error).toBe('invalid_request'); + expect(String(res.json.error_description)).toMatch(/grant_type is required/); + }); + + it.each(['client_credentials', 'password', 'urn:ietf:params:oauth:grant-type:device_code'])( + 'rejects grant_type=%s as unsupported_grant_type', + async (grantType) => { + const res = await token({ grant_type: grantType }); + expect(res.status).toBe(400); + expect(res.json.error).toBe('unsupported_grant_type'); + }, + ); + + it('lets an authorization_code grant pass the allowlist (fails later on PKCE, not on grant_type)', async () => { + const res = await token({ grant_type: 'authorization_code', code: 'abc' }); + expect(res.status).toBe(400); + // Reached the PKCE gate — i.e. the allowlist did not reject the listed grant. + expect(String(res.json.error_description)).toMatch(/code_verifier is required/); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts index 8b9803d5..07b733b6 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts @@ -23,6 +23,17 @@ import { extractTenantList, TENANT_SAFE_FIELDS } from '../tools/admin.js'; // Importing the OAuth helper requires the env var that gates module load. // Set a dummy value before the import resolves so the throw doesn't fire. process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +// access-guard reads ALL its env tunables at module init, and oauth.js now +// imports access-guard (the /oauth/* throttles live there) — so every pin must +// land BEFORE this first import, not next to the later access-guard import. +// Pin a small pre-auth budget so the throttle tests are cheap and +// limit-independent, and a tiny rate-bucket cap so the size-cap test can flood +// a handful of distinct keys instead of 10_000 (well above the per-UPN +// throttle tests' key count). +const PRE_AUTH_TEST_LIMIT = 5; +process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE = String(PRE_AUTH_TEST_LIMIT); +const RATE_BUCKET_TEST_CAP = 10; +process.env.MCP_RATE_BUCKET_MAX_ENTRIES = String(RATE_BUCKET_TEST_CAP); const { isAllowedRedirectUri, isAllowedRedirectUriWith, @@ -294,14 +305,9 @@ describe('L3 — error-handler 5xx body redaction', () => { // 2026-05-08 security review fixes // --------------------------------------------------------------------------- -// Pin a small pre-auth budget BEFORE access-guard loads (it reads the env at -// module init) so the throttle tests are cheap and limit-independent. -const PRE_AUTH_TEST_LIMIT = 5; -process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE = String(PRE_AUTH_TEST_LIMIT); -// Pin a tiny rate-bucket cap (also read at module init) so the size-cap test can flood a -// handful of distinct keys instead of 10_000. Well above the per-UPN throttle tests' key count. -const RATE_BUCKET_TEST_CAP = 10; -process.env.MCP_RATE_BUCKET_MAX_ENTRIES = String(RATE_BUCKET_TEST_CAP); +// NOTE: the env pins for these tests (PRE_AUTH_TEST_LIMIT / RATE_BUCKET_TEST_CAP) +// live at the top of the file — access-guard is already loaded transitively by +// the oauth.js import there, so pinning here would be too late. const { buildCacheKey, boundedSet, isPreAuthRateLimited, getRateBucketSizes } = await import('../access-guard.js'); const { parsePositiveInt, getPublicBaseUrl } = await import('../config.js'); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index f4706943..fb199860 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -35,13 +35,25 @@ const RATE_LIMIT = parsePositiveInt(process.env.MCP_RATE_LIMIT_PER_MINUTE, 60); // if a very large shared-IP org reports false 429s. const PRE_AUTH_RATE_LIMIT = parsePositiveInt(process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE, 120); +// Per-source-IP budgets for the UNauthenticated /oauth/* endpoints (sliding 60s +// window). These routes sit OUTSIDE accessGuard (they exist to obtain the token +// accessGuard demands), so without their own throttle they are open amplifiers: +// every anonymous POST /oauth/token triggers an outbound Entra call carrying the +// app's client_secret — a flood makes Entra throttle the app REGISTRATION, +// breaking token refresh for every legitimate MCP user (org-wide auth DoS with +// zero credentials). The other /oauth routes are local-only (HMAC/allowlist +// work) but remain CPU/log-spam vectors. Budgets are far above a legitimate +// flow's needs: a full OAuth dance is ~4 requests, token refresh ~1/hour/client. +const OAUTH_RATE_LIMIT = parsePositiveInt(process.env.MCP_OAUTH_RATE_LIMIT_PER_MINUTE, 60); +const OAUTH_TOKEN_RATE_LIMIT = parsePositiveInt(process.env.MCP_OAUTH_TOKEN_RATE_LIMIT_PER_MINUTE, 20); + // Hard ceiling on access-cache cardinality. The key is UPN+tokenHash, so a flood // of distinct (valid-signature) tokens would otherwise grow the map unbounded // between the 5-min reaper passes — a memory-exhaustion vector on the 0.5 GiB // container. Oldest entries are evicted on write once the cap is hit. const MAX_ACCESS_CACHE_ENTRIES = parsePositiveInt(process.env.MCP_ACCESS_CACHE_MAX_ENTRIES, 10_000); -// Hard ceiling on rate-bucket cardinality (both the per-UPN and per-source-IP maps). +// Hard ceiling on rate-bucket cardinality (the per-UPN, per-source-IP and /oauth/* maps). // A bucket is created per distinct key on first sight and only removed by the 5-min // reaper (empty-window prune), so a distinct-key flood — spoof-resistant IPs are hard, // but forged UPNs and NAT churn are not — could grow either map unbounded between @@ -232,6 +244,10 @@ const rateBuckets = new Map(); // space from rateBuckets so throttling unvalidated floods never affects a // validated user's per-UPN budget. const preAuthBuckets = new Map(); +// Per-source-IP budgets for the unauthenticated /oauth/* endpoints. Own map +// (keys are `${kind}:${ip}`) so an OAuth flood never burns an IP's /mcp +// pre-auth budget and vice versa. +const oauthBuckets = new Map(); /** Drop timestamps outside the 60s window from a bucket map; delete empty buckets. */ function pruneBuckets(buckets: Map, cutoff: number): void { @@ -251,6 +267,7 @@ setInterval(() => { const cutoff = now - 60_000; pruneBuckets(rateBuckets, cutoff); pruneBuckets(preAuthBuckets, cutoff); + pruneBuckets(oauthBuckets, cutoff); for (const [key, entry] of accessCache) { if (entry.expiresAt <= now) accessCache.delete(key); } @@ -287,9 +304,9 @@ export function isPreAuthRateLimited(clientIp: string): boolean { return isWindowExceeded(preAuthBuckets, clientIp, PRE_AUTH_RATE_LIMIT); } -/** Current cardinality of the two rate-bucket maps. Exported for unit testing the size cap. */ -export function getRateBucketSizes(): { rate: number; preAuth: number } { - return { rate: rateBuckets.size, preAuth: preAuthBuckets.size }; +/** Current cardinality of the rate-bucket maps. Exported for unit testing the size cap. */ +export function getRateBucketSizes(): { rate: number; preAuth: number; oauth: number } { + return { rate: rateBuckets.size, preAuth: preAuthBuckets.size, oauth: oauthBuckets.size }; } /** @@ -308,27 +325,64 @@ function isRateLimited(upn: string): boolean { /** * Emit a 429 with the conventional Retry-After header, a structured + greppable - * log line, and an actionable message. `kind` distinguishes the per-source-IP - * pre-auth throttle from the per-UPN throttle so a Log Analytics alert can break - * the two out and so an unexpected false-positive surfaces to the user (who is - * told to contact the administrator). + * log line, and an actionable message. `kind` distinguishes the throttles + * (per-source-IP pre-auth, per-UPN, per-IP /oauth/*) so a Log Analytics alert + * can break them out and so an unexpected false-positive surfaces to the user + * (who is told to contact the administrator). */ -function respondRateLimited(res: Response, kind: 'pre-auth' | 'upn', key: string, rpcMethod: string): void { +type RateLimitKind = 'pre-auth' | 'upn' | 'oauth' | 'oauth-token'; + +function rateLimitMessage(kind: RateLimitKind): string { + switch (kind) { + case 'upn': + return 'You are sending requests faster than the per-user limit. Wait ~60s and retry. If this keeps ' + + 'happening unexpectedly, contact the MCP server administrator.'; + case 'oauth': + case 'oauth-token': + return 'Too many authentication requests from your network in a short window. Wait ~60s and retry. ' + + 'If this keeps happening unexpectedly, you may be sharing an IP via a corporate proxy/NAT — ' + + 'contact the MCP server administrator.'; + default: + return 'Too many requests from your network in a short window. Wait ~60s and retry. If this keeps ' + + 'happening unexpectedly, you may be sharing an IP via a corporate proxy/NAT — contact the MCP ' + + 'server administrator.'; + } +} + +function respondRateLimited(res: Response, kind: RateLimitKind, key: string, rpcMethod: string): void { console.error(`[mcp-auth] 429 kind=${kind} key=${key} method=${rpcMethod}`); res.setHeader('Retry-After', '60'); res.status(429).json({ error: 'Rate limit exceeded', retryAfterSeconds: 60, - message: - kind === 'pre-auth' - ? 'Too many requests from your network in a short window. Wait ~60s and retry. If this keeps ' + - 'happening unexpectedly, you may be sharing an IP via a corporate proxy/NAT — contact the MCP ' + - 'server administrator.' - : 'You are sending requests faster than the per-user limit. Wait ~60s and retry. If this keeps ' + - 'happening unexpectedly, contact the MCP server administrator.', + message: rateLimitMessage(kind), }); } +/** + * Per-source-IP throttle middleware for the unauthenticated /oauth/* routes. + * Same sliding-window machinery as the /mcp pre-auth limiter, separate bucket + * map keyed `${kind}:${ip}` so the two surfaces cannot starve each other and + * the strict token budget stays independent of the general one. Applied + * per-route inside createOAuthRouter so any future /oauth endpoint must opt in + * consciously rather than inherit silence. + */ +function makeOAuthIpLimiter(kind: 'oauth' | 'oauth-token', limit: number) { + return (req: Request, res: Response, next: NextFunction): void => { + const clientIp = getClientIp(req); + if (isWindowExceeded(oauthBuckets, `${kind}:${clientIp}`, limit)) { + respondRateLimited(res, kind, clientIp, req.path); + return; + } + next(); + }; +} + +/** General /oauth/* budget (register/authorize/callback — local HMAC/allowlist work only). */ +export const oauthRateLimit = makeOAuthIpLimiter('oauth', OAUTH_RATE_LIMIT); +/** Strict /oauth/token budget — every request triggers an outbound Entra call with the client secret. */ +export const oauthTokenRateLimit = makeOAuthIpLimiter('oauth-token', OAUTH_TOKEN_RATE_LIMIT); + // --- Express middleware --- export function accessGuard(req: Request, res: Response, next: NextFunction): void { diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index ff3e44ec..50bf02c7 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -18,6 +18,7 @@ */ import { Router } from 'express'; import crypto from 'node:crypto'; +import { oauthRateLimit, oauthTokenRateLimit } from './access-guard.js'; import { getPublicBaseUrl } from './config.js'; const CLIENT_ID: string = (() => { @@ -388,7 +389,13 @@ export function createOAuthRouter(): Router { }); // --- Dynamic Client Registration (RFC 7591) --- - router.post('/oauth/register', (req, res) => { + // All /oauth/* routes below carry a per-source-IP throttle: they are the only + // unauthenticated surface (accessGuard covers /mcp only) and /oauth/token in + // particular triggers an outbound Entra call with the client secret per + // request — unthrottled, a flood would make Entra throttle the app + // registration itself, an org-wide auth DoS. The .well-known discovery docs + // stay unthrottled (static JSON, no outbound work, no logging). + router.post('/oauth/register', oauthRateLimit, (req, res) => { const { client_name, redirect_uris, grant_types, response_types, token_endpoint_auth_method } = req.body ?? {}; // Field-level bounds — backstop the route-level body-size limit. The @@ -460,7 +467,7 @@ export function createOAuthRouter(): Router { }); // --- Authorize: redirect to Entra ID --- - router.get('/oauth/authorize', (req, res) => { + router.get('/oauth/authorize', oauthRateLimit, (req, res) => { const baseUrl = getPublicBaseUrl(req); const { client_id, @@ -586,7 +593,7 @@ export function createOAuthRouter(): Router { }); // --- Callback: receive code from Entra ID, forward to Claude Code --- - router.get('/oauth/callback', (req, res) => { + router.get('/oauth/callback', oauthRateLimit, (req, res) => { const { code, state, error, error_description } = req.query as Record; if (error) { @@ -654,11 +661,34 @@ export function createOAuthRouter(): Router { }); // --- Token: proxy token exchange to Entra ID --- - router.post('/oauth/token', async (req, res) => { + router.post('/oauth/token', oauthTokenRateLimit, async (req, res) => { const baseUrl = getPublicBaseUrl(req); const params = req.body as Record; + // Allowlist the grant before anything is forwarded. Every request to this + // endpoint goes out with the app's client_id + client_secret attached, so + // an unlisted grant_type would make the proxy an unauthenticated + // confidential-client oracle for whatever grants Entra happens to accept + // (client_credentials/ROPC/device-code currently fail only by accident of + // the pinned scope and which fields we copy — policy must live here, not + // in Entra-side coincidences). The discovery doc advertises exactly these + // two grants, so no conforming client loses anything. + if (!params.grant_type) { + res.status(400).json({ + error: 'invalid_request', + error_description: 'grant_type is required', + }); + return; + } + if (params.grant_type !== 'authorization_code' && params.grant_type !== 'refresh_token') { + res.status(400).json({ + error: 'unsupported_grant_type', + error_description: 'grant_type must be authorization_code or refresh_token', + }); + return; + } + // Enforce PKCE finishing move on the authorization_code grant. /oauth/ // authorize already required code_challenge — accepting a token request // without code_verifier here would defeat that gate. @@ -677,7 +707,7 @@ export function createOAuthRouter(): Router { if (CLIENT_SECRET) body.set('client_secret', CLIENT_SECRET); body.set('redirect_uri', `${baseUrl}/oauth/callback`); - if (params.grant_type) body.set('grant_type', params.grant_type); + body.set('grant_type', params.grant_type); if (params.code) body.set('code', params.code); if (params.refresh_token) body.set('refresh_token', params.refresh_token); if (params.code_verifier) body.set('code_verifier', params.code_verifier);