fix(mcp-security): rate-limit /oauth/* and allowlist grant_type (review R-3)

The unauthenticated /oauth/* routes carried no throttle (accessGuard covers
/mcp only), making /oauth/token an anonymous amplifier: every POST triggers
an outbound Entra call with the app's client_secret attached, so a flood
would get the app registration itself throttled by Entra — an org-wide auth
DoS requiring zero credentials.

- Per-source-IP sliding-window throttles as per-route middleware inside
  createOAuthRouter: 20/min on /oauth/token, 60/min on register/authorize/
  callback (MCP_OAUTH_TOKEN_RATE_LIMIT_PER_MINUTE / MCP_OAUTH_RATE_LIMIT_
  PER_MINUTE). Own bucket map keyed `${kind}:${ip}` so OAuth floods never
  burn the /mcp pre-auth budget and vice versa; pruned by the shared reaper
  and capped by MAX_RATE_BUCKET_ENTRIES. 429s reuse respondRateLimited, so
  the existing `[mcp-auth] 429 kind=` log/KQL pattern covers the new kinds.
  .well-known discovery docs stay unthrottled (static JSON, no outbound work).
- grant_type allowlist on /oauth/token: missing -> invalid_request, anything
  but authorization_code/refresh_token -> unsupported_grant_type, rejected
  before the client secret leaves the building. Policy now lives in the
  proxy instead of relying on Entra-side accidents (pinned scope, uncopied
  fields) to reject client_credentials/ROPC/device-code.
- Tests: oauth-rate-limit.test.ts (tiny pinned budgets, bucket independence
  in both directions) + grant_type allowlist block in oauth-register.test.ts.
  Env pins in security-guards.test.ts hoisted above the first oauth.js
  import — oauth.ts now (transitively) loads access-guard.ts at import time.

MCP server v1.4.5 -> 1.4.6. vitest 345 passed / 41 skipped, tsc clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-07-03 11:27:42 +02:00
parent aa9adfbac0
commit 6f8ce556b7
7 changed files with 260 additions and 32 deletions
+2 -2
View File
@@ -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",
@@ -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": {
@@ -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<void>((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<void>((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<string, unknown>;
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);
});
});
@@ -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<string, string>) {
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<string, unknown>;
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/);
});
});
@@ -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');
@@ -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<string, RateEntry>();
// space from rateBuckets so throttling unvalidated floods never affects a
// validated user's per-UPN budget.
const preAuthBuckets = new Map<string, RateEntry>();
// 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<string, RateEntry>();
/** Drop timestamps outside the 60s window from a bucket map; delete empty buckets. */
function pruneBuckets(buckets: Map<string, RateEntry>, 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 {
@@ -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<string, string>;
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<string, string>;
// 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);