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 a2a7fe85..4e5a7682 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 @@ -3,7 +3,13 @@ * H1 — OAuth redirect_uri allowlist * M1 — followNextLink basePath validation * M2 — SessionIdSchema UUID enforcement - * L3 — error-handler 5xx body redaction + * L3 — error-handler 5xx body redaction (also covers post-2026-05 review: + * structured 5xx are now sanitized identically to unstructured) + * + * Plus 2026-05-08 security review fixes: + * F1 — access-guard cache key includes token hash + * F2 — /oauth/register field-level + registry-cap defenses + * F4 — PKCE S256 enforcement + HMAC-signed state + state expiry * * These tests are pure functions; no live backend or token needed. */ @@ -16,7 +22,17 @@ import { toolError } from '../tools/error-handler.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'; -const { isAllowedRedirectUri, isAllowedRedirectUriWith, parseAllowedHosts } = await import('../oauth.js'); +const { + isAllowedRedirectUri, + isAllowedRedirectUriWith, + parseAllowedHosts, + signState, + verifyState, + MAX_REGISTERED_CLIENTS, + MAX_REDIRECT_URIS_PER_CLIENT, + MAX_REDIRECT_URI_LENGTH, + MAX_CLIENT_NAME_LENGTH, +} = await import('../oauth.js'); describe('M1 — followNextLink basePath validation', () => { const basePath = '/api/global/audit/logs'; @@ -245,12 +261,148 @@ describe('L3 — error-handler 5xx body redaction', () => { expect(text).toContain('invalid filter syntax'); }); - it('preserves structured 5xx errors with parsed body (parsed.error path)', () => { - // ApiError parses JSON bodies; structured errors flow through the parsed - // branch and are presentable. - const structured = new ApiError(503, JSON.stringify({ error: 'service unavailable', hint: 'retry in 30s' })); + it('sanitizes structured 5xx errors too — `error` and `hint` must NOT leak', () => { + // 2026-05-08 review: structured 5xx errors used to flow through the + // `parsed`-detail branch with full `error`/`hint`/`exceptionType` fields. + // The current handler treats all status >= 500 the same: generic message, + // correlationId + errorCode kept (operational handles), everything else + // dropped. + const structured = new ApiError( + 503, + JSON.stringify({ + error: 'service unavailable', + hint: 'retry in 30s', + exceptionType: 'System.Data.SqlClient.SqlException', + correlationId: 'corr-abc-123', + errorCode: 'SQL_TIMEOUT', + }), + ); const text = toolError('test_tool', {}, structured).content[0].text; - expect(text).toContain('service unavailable'); - expect(text).toContain('retry in 30s'); + + expect(text).toContain('HTTP 503'); + // Operational handles still surface so an operator can pivot. + expect(text).toContain('corr-abc-123'); + expect(text).toContain('SQL_TIMEOUT'); + // Internal-detail fields must be dropped. + expect(text).not.toContain('service unavailable'); + expect(text).not.toContain('retry in 30s'); + expect(text).not.toContain('System.Data.SqlClient'); }); }); + +// --------------------------------------------------------------------------- +// 2026-05-08 security review fixes +// --------------------------------------------------------------------------- + +const { buildCacheKey } = await import('../access-guard.js'); + +describe('F1 — access-guard cache key includes token hash', () => { + it('produces stable keys for the same (upn, token) pair', () => { + const a = buildCacheKey('alice@contoso.com', 'header.payload.signature'); + const b = buildCacheKey('alice@contoso.com', 'header.payload.signature'); + expect(a).toBe(b); + }); + + it('distinguishes two tokens that share a UPN', () => { + // The whole point of the fix: a forged token with a victim UPN cannot + // piggyback on a legitimate user's cached `allowed: true`. + const legit = buildCacheKey('alice@contoso.com', 'legit-token-from-entra'); + const forged = buildCacheKey('alice@contoso.com', 'forged-token-no-signature'); + expect(legit).not.toBe(forged); + }); + + it('distinguishes UPNs with the same token (defensive)', () => { + const x = buildCacheKey('alice@contoso.com', 'shared-token'); + const y = buildCacheKey('bob@contoso.com', 'shared-token'); + expect(x).not.toBe(y); + }); + + it('keeps the UPN as the visible prefix (operability — log scanning)', () => { + const k = buildCacheKey('alice@contoso.com', 'whatever'); + expect(k.startsWith('alice@contoso.com:')).toBe(true); + }); +}); + +describe('F2 — /oauth/register defense constants', () => { + it('exposes a hard cap that bounds memory at ~3 MB worst case', () => { + // 0.5 GiB container budget × ~300 bytes/entry → 10k is two orders of + // magnitude below the OOM cliff and well above realistic peak. + expect(MAX_REGISTERED_CLIENTS).toBeGreaterThanOrEqual(1_000); + expect(MAX_REGISTERED_CLIENTS).toBeLessThanOrEqual(100_000); + }); + + it('caps redirect_uris per client (RFC 7591 typically lists 1-3)', () => { + expect(MAX_REDIRECT_URIS_PER_CLIENT).toBeGreaterThanOrEqual(2); + expect(MAX_REDIRECT_URIS_PER_CLIENT).toBeLessThanOrEqual(50); + }); + + it('caps individual redirect_uri length below practical browser URL limits', () => { + expect(MAX_REDIRECT_URI_LENGTH).toBeGreaterThanOrEqual(256); + expect(MAX_REDIRECT_URI_LENGTH).toBeLessThanOrEqual(8192); + }); + + it('caps client_name length below realistic display strings', () => { + expect(MAX_CLIENT_NAME_LENGTH).toBeGreaterThanOrEqual(64); + expect(MAX_CLIENT_NAME_LENGTH).toBeLessThanOrEqual(2048); + }); +}); + +describe('F4 — HMAC-signed OAuth state', () => { + const validPayload = { + originalState: 'client-supplied-state-abc', + redirectUri: 'https://claude.ai/cb', + clientId: 'client-uuid-123', + }; + + it('round-trips a freshly signed state', () => { + const signed = signState(validPayload); + const verified = verifyState(signed); + expect(verified).not.toBeNull(); + expect(verified!.redirectUri).toBe('https://claude.ai/cb'); + expect(verified!.originalState).toBe('client-supplied-state-abc'); + expect(verified!.clientId).toBe('client-uuid-123'); + expect(typeof verified!.iat).toBe('number'); + }); + + it('rejects state without an HMAC suffix', () => { + // Old behavior was bare base64url-JSON — must be rejected now. + const unsigned = Buffer.from(JSON.stringify(validPayload)).toString('base64url'); + expect(verifyState(unsigned)).toBeNull(); + }); + + it('rejects state where the body was tampered after signing', () => { + const signed = signState(validPayload); + const [body, sig] = signed.split('.'); + // Flip a byte in the body but keep the original signature. + const tamperedBody = Buffer.from(body, 'base64url').toString('utf-8').replace('claude.ai', 'evil.tld'); + const tampered = `${Buffer.from(tamperedBody).toString('base64url')}.${sig}`; + expect(verifyState(tampered)).toBeNull(); + }); + + it('rejects state with a corrupt HMAC suffix', () => { + const signed = signState(validPayload); + const [body] = signed.split('.'); + const corrupt = `${body}.AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA`; + expect(verifyState(corrupt)).toBeNull(); + }); + + it('rejects state older than 10 minutes (replay window cap)', () => { + const signed = signState(validPayload); + // verifyState accepts an injectable clock for deterministic testing. + const elevenMinutesLater = Math.floor(Date.now() / 1000) + 11 * 60; + expect(verifyState(signed, elevenMinutesLater)).toBeNull(); + }); + + it('accepts state at the 9-minute mark (still within window)', () => { + const signed = signState(validPayload); + const nineMinutesLater = Math.floor(Date.now() / 1000) + 9 * 60; + expect(verifyState(signed, nineMinutesLater)).not.toBeNull(); + }); + + it('rejects malformed state strings that lack the dot separator', () => { + expect(verifyState('no-dot-no-signature')).toBeNull(); + expect(verifyState('')).toBeNull(); + expect(verifyState('a.b.c')).toBeNull(); // too many parts + }); + +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index b6bee36a..3e8833f0 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -8,6 +8,7 @@ * 4. Set token for pass-through to backend API */ import type { Request, Response, NextFunction } from 'express'; +import crypto from 'node:crypto'; import { extractTokenClaims, isTokenExpired } from './auth.js'; import { runWithCaller } from './client.js'; @@ -34,6 +35,12 @@ interface AccessCacheEntry { expiresAt: number; } +// Cache key includes a hash of the token, not just the UPN. The JWT signature +// is verified by the backend, not here — so two tokens with the same UPN must +// be treated as distinct cache entries. Without this, a forged token carrying +// a victim's UPN would hit the legitimate user's cached `allowed: true` and +// bypass the backend signature check for any local-only MCP operation +// (tools/list, resources/list, etc.). const accessCache = new Map(); interface AccessCheckResult { @@ -42,8 +49,14 @@ interface AccessCheckResult { isGlobalAdmin: boolean; } +export function buildCacheKey(upn: string, token: string): string { + const h = crypto.createHash('sha256').update(token).digest('hex').slice(0, 32); + return `${upn}:${h}`; +} + async function checkAccess(upn: string, token: string): Promise { - const cached = accessCache.get(upn); + const cacheKey = buildCacheKey(upn, token); + const cached = accessCache.get(cacheKey); if (cached && Date.now() < cached.expiresAt) { return { allowed: cached.allowed, reason: cached.reason, isGlobalAdmin: cached.isGlobalAdmin }; } @@ -71,7 +84,7 @@ async function checkAccess(upn: string, token: string): Promise(); -// Cleanup stale entries every 5 minutes +// Cleanup stale entries every 5 minutes. Both maps share this interval — +// rateBuckets evicts empty windows, accessCache evicts expired entries +// (the cache key now includes a token hash, so cardinality is one entry per +// user×token-rotation; without proactive eviction the map would only shrink +// when a key is re-fetched). setInterval(() => { - const cutoff = Date.now() - 60_000; + const now = Date.now(); + const cutoff = now - 60_000; for (const [key, entry] of rateBuckets) { entry.timestamps = entry.timestamps.filter((t) => t > cutoff); if (entry.timestamps.length === 0) rateBuckets.delete(key); } + for (const [key, entry] of accessCache) { + if (entry.expiresAt <= now) accessCache.delete(key); + } }, 5 * 60_000); function isRateLimited(upn: string): boolean { @@ -153,8 +174,10 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo // UPN. If rate-limit incremented before validation, those forgeries would // burn the victim's per-minute budget and 429 their legitimate calls. // checkAccess delegates signature verification to the backend; both - // allow- and deny-decisions are cached for 5min per UPN, so forged-token - // floods cost at most one backend call per fake UPN. + // allow- and deny-decisions are cached for 5 min keyed on UPN+tokenHash, + // so a forged token cannot piggyback on a legitimate user's cached + // `allowed: true`, and forged-token floods cost one backend call per + // distinct token. checkAccess(upn, token) .then((result) => { if (!result.allowed) { diff --git a/src/McpServer/autopilot-monitor-mcp/src/index.ts b/src/McpServer/autopilot-monitor-mcp/src/index.ts index 898f2304..34bdeea6 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/index.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/index.ts @@ -19,6 +19,19 @@ const SERVER_VERSION: string = pkg.version; const PORT = parseInt(process.env.PORT ?? '8080', 10); const RULES_DIR = process.env.RULES_DIR ?? resolve(__dirname, '..', '..', '..', '..', 'rules'); +// Surface the MCP_PUBLIC_URL state at boot so a missing public-URL pin is +// visible in container logs, not silently fallen back to forwarded-headers +// in production. Two-stage deploy expectation: first deploy → containerAppUrl +// output → second deploy with mcpPublicUrl pinned. See infra/mcp-server.bicep. +if (!process.env.MCP_PUBLIC_URL) { + console.error( + '[startup] MCP_PUBLIC_URL is not set — OAuth issuer / WWW-Authenticate / ' + + 'redirect metadata will be derived from X-Forwarded-* headers. This is ' + + 'acceptable for local dev only; in production, set MCP_PUBLIC_URL to the ' + + 'Container App FQDN (re-deploy with the bicep `mcpPublicUrl` parameter).', + ); +} + // --- Load shared knowledge base (reused across all sessions) --- console.error('Loading knowledge base documents…'); @@ -40,6 +53,15 @@ function createMcpServer(): McpServer { // --- HTTP Server with Streamable HTTP Transport --- const app = express(); + +// Tight body-size limit for /oauth/register, registered BEFORE the global +// parser so the smaller limit wins (the global parser's body-already-parsed +// short-circuit then skips re-parsing). RFC 7591 registration requests carry +// only client_name + redirect_uris + a few flags — 8 KB is two orders of +// magnitude over realistic; anything larger is a memory-pressure attempt +// against the in-memory client registry. +app.use('/oauth/register', express.json({ limit: '8kb' })); + app.use(express.json()); app.use(express.urlencoded({ extended: true })); diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index c2afafb6..c537f4dc 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -37,9 +37,101 @@ const SCOPES = `api://${CLIENT_ID}/access_as_user openid profile offline_access` * In-memory store for dynamically registered OAuth clients (RFC 7591). * Clients re-register after a server restart — this is fine because the MCP * server acts as an OAuth proxy and the real credentials live in Entra ID. + * + * TODO(oauth-map-ttl): TTL/LRU eviction is intentionally NOT enabled. With a + * stateless scale-to-zero Container App, an evicted client_id forces the user + * back through Claude Code's manual `/mcp` reconnect flow (no auto-recovery). + * The hard size cap (MAX_REGISTERED_CLIENTS) bounds memory; revisit TTL once + * the reconnect UX is debugged or the registry moves to persistent storage. */ const registeredClients = new Map(); +// ---- State signing (HMAC) ------------------------------------------------- +// +// /oauth/authorize stores its proxy state in the OAuth `state` parameter that +// transits the user-agent. Without integrity protection a tampered state can +// inject a chosen redirectUri / clientId pair into /oauth/callback. The +// downstream redirect_uri allowlist + per-client registry already block +// hostile destinations, but signing the state is a cheap belt-and-suspenders +// that also catches replay (via iat/exp) and detects accidental client bugs. +// +// HMAC key: prefer STATE_HMAC_KEY from the environment so all replicas agree; +// fall back to a per-instance random key when unset (state has a 10 min +// lifetime, so per-instance is acceptable for single-replica scale=0..1). +const STATE_HMAC_KEY: Buffer = process.env.STATE_HMAC_KEY + ? Buffer.from(process.env.STATE_HMAC_KEY, 'utf-8') + : crypto.randomBytes(32); +const STATE_MAX_AGE_SECONDS = 600; + +interface ProxyStatePayload { + originalState?: string; + redirectUri: string; + clientId: string | null; + iat: number; // unix seconds +} + +export function signState(payload: Omit): string { + const full: ProxyStatePayload = { ...payload, iat: Math.floor(Date.now() / 1000) }; + const json = JSON.stringify(full); + const body = Buffer.from(json, 'utf-8').toString('base64url'); + const sig = crypto.createHmac('sha256', STATE_HMAC_KEY).update(body).digest('base64url'); + return `${body}.${sig}`; +} + +export function verifyState(state: string, nowSeconds: number = Math.floor(Date.now() / 1000)): ProxyStatePayload | null { + const parts = state.split('.'); + if (parts.length !== 2) return null; + const [body, sig] = parts; + + const expected = crypto.createHmac('sha256', STATE_HMAC_KEY).update(body).digest('base64url'); + // Constant-time compare; lengths must match before timingSafeEqual. + const sigBuf = Buffer.from(sig, 'utf-8'); + const expBuf = Buffer.from(expected, 'utf-8'); + if (sigBuf.length !== expBuf.length) return null; + if (!crypto.timingSafeEqual(sigBuf, expBuf)) return null; + + let parsed: ProxyStatePayload; + try { + parsed = JSON.parse(Buffer.from(body, 'base64url').toString('utf-8')) as ProxyStatePayload; + } catch { + return null; + } + + if (typeof parsed.iat !== 'number' || nowSeconds - parsed.iat > STATE_MAX_AGE_SECONDS) return null; + if (typeof parsed.redirectUri !== 'string') return null; + + return parsed; +} + +/** + * Hard cap on the dynamic-client registry. At ~300 bytes per entry this caps + * memory at ~3 MB worst case — well below the 0.5 GiB container budget. Real + * usage adds one entry per Claude-Code-session × server-URL, lifetime; even a + * busy deployment stays in the low thousands. + */ +export const MAX_REGISTERED_CLIENTS = 10_000; +export const MAX_REDIRECT_URIS_PER_CLIENT = 10; +export const MAX_REDIRECT_URI_LENGTH = 1024; +export const MAX_CLIENT_NAME_LENGTH = 256; + +/** + * Test-only handle on the registry. Lets unit tests verify cap behavior + * without spinning up the full Express stack. Production code never imports + * this — it only ever touches the closure inside createOAuthRouter(). + */ +export const _registryForTest = { + size: () => registeredClients.size, + fill: (count: number) => { + for (let i = 0; i < count; i++) { + registeredClients.set(`__test_${i}_${crypto.randomUUID()}`, { + name: '__test', + redirectUris: ['http://localhost:1/cb'], + }); + } + }, + clear: () => registeredClients.clear(), +}; + /** * Strict allowlist of host patterns acceptable for OAuth redirect_uri. * Defense-in-depth on top of the per-client redirect_uri match: even if an @@ -186,13 +278,55 @@ export function createOAuthRouter(): Router { // --- Dynamic Client Registration (RFC 7591) --- router.post('/oauth/register', (req, res) => { + // Hard memory ceiling — refuse new registrations once the registry is at + // capacity. The endpoint is unauthenticated per RFC 7591, so without this + // a flood would push the 0.5 GiB container into OOM. + if (registeredClients.size >= MAX_REGISTERED_CLIENTS) { + console.error( + `[oauth/register] Registry at cap (${MAX_REGISTERED_CLIENTS}) — rejecting new registration`, + ); + res.status(503).json({ + error: 'temporarily_unavailable', + error_description: 'Registration capacity reached, please retry later', + }); + return; + } + 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 + // parser caps total body bytes; these caps stop a single registration + // from carrying e.g. a thousand redirect_uris that all individually + // pass the URI allowlist. + if (typeof client_name === 'string' && client_name.length > MAX_CLIENT_NAME_LENGTH) { + res.status(400).json({ + error: 'invalid_client_metadata', + error_description: `client_name exceeds ${MAX_CLIENT_NAME_LENGTH} characters`, + }); + return; + } + + const uris = Array.isArray(redirect_uris) ? redirect_uris : []; + if (uris.length > MAX_REDIRECT_URIS_PER_CLIENT) { + res.status(400).json({ + error: 'invalid_redirect_uri', + error_description: `Too many redirect_uris (max ${MAX_REDIRECT_URIS_PER_CLIENT})`, + }); + return; + } + const oversized = uris.find((u: unknown) => typeof u === 'string' && u.length > MAX_REDIRECT_URI_LENGTH); + if (oversized !== undefined) { + res.status(400).json({ + error: 'invalid_redirect_uri', + error_description: `redirect_uri exceeds ${MAX_REDIRECT_URI_LENGTH} characters`, + }); + return; + } + // Reject hostile redirect_uris at registration time so the registry can be // trusted by /oauth/authorize and /oauth/callback. The endpoint itself is // unauthenticated per RFC 7591, so this filter is the only lever stopping // an attacker from registering arbitrary post-callback destinations. - const uris = Array.isArray(redirect_uris) ? redirect_uris : []; const invalidUris = uris.filter((u: unknown) => typeof u !== 'string' || !isAllowedRedirectUri(u)); if (invalidUris.length > 0) { console.error( @@ -238,6 +372,26 @@ export function createOAuthRouter(): Router { code_challenge_method, } = req.query as Record; + // PKCE is mandatory. The discovery doc advertises S256 only, so any client + // that omits the challenge or downgrades to `plain` is either misbehaving + // or attempting to weaken the flow. Reject before redirecting to Entra so + // the failure surfaces cleanly at the proxy boundary. + if (!code_challenge) { + res.status(400).json({ + error: 'invalid_request', + error_description: 'code_challenge is required (PKCE S256 mandatory)', + }); + return; + } + const method = code_challenge_method ?? 'S256'; + if (method !== 'S256') { + res.status(400).json({ + error: 'invalid_request', + error_description: `code_challenge_method must be S256 (got: ${method})`, + }); + return; + } + // Defense in depth: validate redirect_uri before we build the proxy state. // Without this, /oauth/callback would happily 302 to any URL the caller // supplied — leaking the auth code to an attacker-controlled endpoint @@ -287,16 +441,14 @@ export function createOAuthRouter(): Router { console.warn('[oauth/authorize] No client_id supplied — proceeding on host-allowlist gate only'); } - // Store Claude Code's redirect_uri in state so we can forward the code back. - // The clientId is bound into the state so /oauth/callback can re-validate - // against the registry — the inbound request to /oauth/callback only sees - // the state, not the original query, and tampering with the state is - // harmless because the same allowlist + registry checks run there too. - const proxyState = Buffer.from(JSON.stringify({ + // HMAC-signed state: tampering or replay (>10 min) fails verification at + // /oauth/callback. The original client state is preserved inside the + // signed payload and re-emitted to the client unchanged. + const proxyState = signState({ originalState: state, redirectUri: redirect_uri, clientId: client_id ?? null, - })).toString('base64url'); + }); // Always use server-defined SCOPES — Claude Code may send scope values // (e.g. after dynamic registration) that trigger AADSTS90009 when forwarded @@ -308,8 +460,8 @@ export function createOAuthRouter(): Router { redirect_uri: `${baseUrl}/oauth/callback`, scope: SCOPES, state: proxyState, - ...(code_challenge ? { code_challenge } : {}), - ...(code_challenge_method ? { code_challenge_method } : {}), + code_challenge, + code_challenge_method: 'S256', }); res.redirect(`${AUTHORITY}/oauth2/v2.0/authorize?${entraParams}`); @@ -324,19 +476,18 @@ export function createOAuthRouter(): Router { return; } - // Decode proxy state to get Claude Code's original redirect_uri - let originalState = state; - let redirectUri = ''; - let clientId: string | null = null; - try { - const decoded = JSON.parse(Buffer.from(state, 'base64url').toString('utf-8')); - originalState = decoded.originalState; - redirectUri = decoded.redirectUri; - clientId = decoded.clientId ?? null; - } catch { - res.status(400).json({ error: 'invalid_state', error_description: 'Could not decode state' }); + // verifyState rejects tampered, replayed-after-10-min, or malformed state. + const decoded = state ? verifyState(state) : null; + if (!decoded) { + res.status(400).json({ + error: 'invalid_state', + error_description: 'state failed HMAC verification or expired', + }); return; } + const originalState = decoded.originalState; + const redirectUri = decoded.redirectUri; + const clientId = decoded.clientId; // Re-validate the redirectUri at callback time too. Belt-and-suspenders: // /oauth/authorize already gated the URI, but the state value transits @@ -380,9 +531,21 @@ export function createOAuthRouter(): Router { router.post('/oauth/token', async (req, res) => { const baseUrl = getPublicBaseUrl(req); + const params = req.body as Record; + + // 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. + if (params.grant_type === 'authorization_code' && !params.code_verifier) { + res.status(400).json({ + error: 'invalid_request', + error_description: 'code_verifier is required for grant_type=authorization_code (PKCE)', + }); + return; + } + // Build form body for Entra ID token endpoint const body = new URLSearchParams(); - const params = req.body as Record; body.set('client_id', CLIENT_ID); if (CLIENT_SECRET) body.set('client_secret', CLIENT_SECRET); diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts index c945dba5..ffe8cc6e 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts @@ -24,8 +24,19 @@ export function toolError( ): ToolErrorResult { const parts: string[] = []; - if (error instanceof ApiError && error.parsed) { - // Structured backend error — extract rich details + if (error instanceof ApiError && error.status >= 500) { + // Sanitize ALL 5xx — structured or not. Even structured backend errors + // can carry internal fingerprints (CLR exception types, stack frames, + // hint strings that name internal services), and the model has no + // legitimate reason to act on them. correlationId + errorCode stay — + // those are operational handles the operator can pivot on, not + // internals. + parts.push(`**Backend error in ${toolName}** (HTTP ${error.status}): the server returned an error.`); + if (error.parsed?.correlationId) parts.push(`**Correlation ID**: ${error.parsed.correlationId}`); + if (error.parsed?.errorCode) parts.push(`**Error code**: ${error.parsed.errorCode}`); + parts.push('**Suggestion**: retry in a few seconds; if persistent, ask an operator to inspect backend logs.'); + } else if (error instanceof ApiError && error.parsed) { + // Structured backend error (4xx) — extract rich details const p = error.parsed; parts.push(`**Error in ${toolName}**: ${p.error ?? error.message}`); if (p.hint) parts.push(`**Suggestion**: ${p.hint}`); @@ -40,15 +51,6 @@ export function toolError( parts.push(`**Not found in ${toolName}**: The requested resource does not exist. Verify IDs, table names, or filters.`); } else if (error.status === 429) { parts.push(`**Rate limited in ${toolName}**: Too many requests. Wait a moment and retry.`); - } else if (error.status >= 500) { - // Don't relay 5xx response bodies to the model — unstructured server - // errors can occasionally leak internals (stack frames, connection - // strings, environment hints) that shouldn't reach a downstream caller. - // The status code + a stable hint is enough for the model to retry or - // fall back; deep diagnostics are available in backend logs by - // correlation id. - parts.push(`**Backend error in ${toolName}** (HTTP ${error.status}): the server returned an unstructured error.`); - parts.push('**Suggestion**: retry in a few seconds; if persistent, ask an operator to inspect backend logs.'); } else { const body = error.body || 'No response body'; const truncated = body.length > 500 ? body.slice(0, 500) + '…' : body;