diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-boot-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-boot-guards.test.ts new file mode 100644 index 00000000..1fb7fe73 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/oauth-boot-guards.test.ts @@ -0,0 +1,58 @@ +/** + * Boot guards for operator-supplied OAuth origins (review findings 08-15). + * + * validateAuthority / validatePublicUrl run at module scope against env values, + * so a bad AUTOPILOT_ENTRA_AUTHORITY or MCP_PUBLIC_URL fails the boot instead + * of shipping a proxy that sends the client secret / advertises an issuer over + * plain http. These are misconfiguration guards (env is operator-controlled), + * matching the fail-fast posture of AUTOPILOT_API_URL and the MCP_PUBLIC_URL + * production pin in config.ts. + */ +import { describe, it, expect } from 'vitest'; + +// oauth.ts throws at import unless the Entra client id is present. +process.env.AUTOPILOT_ENTRA_CLIENT_ID ??= '00000000-0000-0000-0000-000000000000'; +const { validateAuthority } = await import('../oauth.js'); +const { validatePublicUrl } = await import('../config.js'); + +describe('validateAuthority — Entra authority must be https', () => { + it('accepts the default public-cloud authority and strips trailing slashes', () => { + expect(validateAuthority('https://login.microsoftonline.com/organizations/')) + .toBe('https://login.microsoftonline.com/organizations'); + }); + + it('accepts sovereign-cloud authorities (deliberately not host-pinned)', () => { + expect(validateAuthority('https://login.microsoftonline.us/organizations')) + .toBe('https://login.microsoftonline.us/organizations'); + }); + + it('rejects a plain-http authority (secret + auth codes would transit unencrypted)', () => { + expect(() => validateAuthority('http://login.microsoftonline.com/organizations')) + .toThrow(/must be an https:\/\/ URL/); + }); + + it('rejects an unparseable value', () => { + expect(() => validateAuthority('not a url')).toThrow(/not a valid URL/); + }); +}); + +describe('validatePublicUrl — MCP_PUBLIC_URL pin must be https (loopback-http tolerated)', () => { + it('accepts an https pin and strips trailing slashes (consumers do `${base}/mcp` joins)', () => { + expect(validatePublicUrl('https://mcp.example.net/')).toBe('https://mcp.example.net'); + }); + + it.each(['http://localhost:3000', 'http://127.0.0.1:8080'])( + 'accepts %s (local dev loopback)', + (uri) => { + expect(validatePublicUrl(uri)).toBe(uri); + }, + ); + + it('rejects a plain-http non-loopback pin (would advertise a downgradeable issuer)', () => { + expect(() => validatePublicUrl('http://mcp.example.net')).toThrow(/must be an https:\/\/ URL/); + }); + + it('rejects an unparseable value', () => { + expect(() => validatePublicUrl('not a url')).toThrow(/not a valid URL/); + }); +}); 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 08878d94..72a9816c 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 @@ -11,7 +11,7 @@ * would call Entra ID (the /oauth/authorize success case yields a 302 we read * with redirect:'manual'; the token error case returns before fetch). */ -import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, afterEach, vi } from 'vitest'; import express from 'express'; import type { Server } from 'node:http'; import type { AddressInfo } from 'node:net'; @@ -456,3 +456,56 @@ describe('/oauth/token — grant_type allowlist', () => { expect(String(res.json.error_description)).toMatch(/code_verifier is required/); }); }); + +describe('/oauth/token — outbound Entra timeout', () => { + // The route passes AbortSignal.timeout() to the Entra fetch; a stalled + // identity provider must surface as 504 (distinguishable from a hard 502 + // exchange failure) instead of pinning the handler for undici's ~300 s + // defaults. Stub global fetch for the Entra host only — the test's own + // request to the ephemeral server must keep using the real implementation. + const realFetch = globalThis.fetch; + afterEach(() => vi.unstubAllGlobals()); + + function stubEntraFetch(rejection: unknown, onCall?: (init?: RequestInit) => void) { + vi.stubGlobal('fetch', ((input: RequestInfo | URL, init?: RequestInit) => { + if (String(input).includes('login.microsoftonline.com')) { + onCall?.(init); + return Promise.reject(rejection); + } + return realFetch(input, init); + }) as typeof fetch); + } + + async function token(params: Record) { + const res = await realFetch(`${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, cacheControl: res.headers.get('cache-control') }; + } + + it('maps a TimeoutError from the Entra fetch to 504 and passes an abort signal', async () => { + let sawAbortSignal = false; + // Exactly what fetch rejects with under AbortSignal.timeout() (see + // error-handler.ts): DOMException with NAME 'TimeoutError'. + stubEntraFetch( + new DOMException('The operation was aborted due to timeout', 'TimeoutError'), + (init) => { sawAbortSignal = init?.signal instanceof AbortSignal; }, + ); + const res = await token({ grant_type: 'authorization_code', code: 'abc', code_verifier: 'v'.repeat(43) }); + expect(res.status).toBe(504); + expect(res.json.error).toBe('token_exchange_timeout'); + // RFC 6749 §5.1 no-store must hold on the timeout path too. + expect(res.cacheControl).toBe('no-store'); + expect(sawAbortSignal).toBe(true); + }); + + it('maps a non-timeout network failure to 502 token_exchange_failed (unchanged path)', async () => { + stubEntraFetch(new TypeError('fetch failed')); + const res = await token({ grant_type: 'authorization_code', code: 'abc', code_verifier: 'v'.repeat(43) }); + expect(res.status).toBe(502); + expect(res.json.error).toBe('token_exchange_failed'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/config.ts b/src/McpServer/autopilot-monitor-mcp/src/config.ts index e60533da..826375b0 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/config.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/config.ts @@ -64,7 +64,34 @@ export function parsePositiveInt(raw: string | undefined, fallback: number): num * minReplicas=0, so no container boots and this check never fires; the operator * then re-deploys with mcpPublicUrl pinned before any traffic arrives. */ -const MCP_PUBLIC_URL = process.env.MCP_PUBLIC_URL?.trim() || undefined; +/** + * Validates a pinned public base URL. It becomes the OAuth issuer, the RFC 9207 + * `iss` value, and the Entra redirect_uri — an http:// pin (a typo away) would + * advertise a downgrade-able issuer and get rejected by Entra's redirect_uri + * registration anyway. https is required; http is tolerated only for loopback + * hosts so a local dev pin like http://localhost:3000 keeps working. Trailing + * slashes are stripped — consumers do `${baseUrl}/mcp` string joins. + */ +export function validatePublicUrl(raw: string): string { + let parsed: URL; + try { + parsed = new URL(raw); + } catch { + throw new Error(`MCP_PUBLIC_URL is not a valid URL: ${raw}`); + } + const host = parsed.hostname.toLowerCase(); + const isLoopback = host === 'localhost' || host === '127.0.0.1' || host === '[::1]' || host === '::1'; + if (parsed.protocol !== 'https:' && !(parsed.protocol === 'http:' && isLoopback)) { + throw new Error( + `MCP_PUBLIC_URL must be an https:// URL (got ${raw}) — it is published as the OAuth ` + + 'issuer and redirect_uri base. http:// is allowed for loopback hosts only (local dev).', + ); + } + return raw.replace(/\/+$/, ''); +} + +const rawPublicUrl = process.env.MCP_PUBLIC_URL?.trim() || undefined; +const MCP_PUBLIC_URL = rawPublicUrl ? validatePublicUrl(rawPublicUrl) : undefined; if (!MCP_PUBLIC_URL && process.env.NODE_ENV === 'production') { throw new Error( 'MCP_PUBLIC_URL must be set in production — refusing to derive the OAuth issuer / ' + diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index 4b5580d3..b44a35ba 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -32,9 +32,58 @@ const CLIENT_ID: string = (() => { return v; })(); const CLIENT_SECRET = process.env.AUTOPILOT_ENTRA_CLIENT_SECRET ?? ''; -const AUTHORITY = process.env.AUTOPILOT_ENTRA_AUTHORITY ?? `${ENTRA_LOGIN_BASE_URL}/organizations`; +// Intentionally optional: a public-client app registration (PKCE only) needs no +// secret. But if the Entra app is configured as a confidential client, a missing +// secret fails EVERY token exchange (AADSTS7000218) — surface that at boot +// instead of leaving it to be discovered one failed login at a time. +if (!CLIENT_SECRET && process.env.NODE_ENV === 'production') { + console.error( + '[oauth] AUTOPILOT_ENTRA_CLIENT_SECRET is not set — token exchange runs as a ' + + 'PUBLIC client (PKCE only). Correct only if the Entra app registration allows ' + + 'public client flows; a confidential-client registration will reject every ' + + 'token exchange with AADSTS7000218.', + ); +} + +/** + * Validates the Entra authority URL at boot. The authority receives the client + * secret and every authorization code, so a plain-http or unparseable value is + * a misconfiguration we refuse to run with (fail-fast, matching the + * AUTOPILOT_API_URL / MCP_PUBLIC_URL guards in config.ts). Deliberately NOT + * pinned to login.microsoftonline.com — sovereign clouds (.us, .cn) are + * legitimate authorities. Trailing slashes are stripped so the + * `${AUTHORITY}/oauth2/v2.0/...` joins below cannot produce double slashes. + */ +export function validateAuthority(raw: string): string { + let parsed: URL; + try { + parsed = new URL(raw); + } catch { + throw new Error(`AUTOPILOT_ENTRA_AUTHORITY is not a valid URL: ${sanitizeForLog(raw)}`); + } + if (parsed.protocol !== 'https:') { + throw new Error( + `AUTOPILOT_ENTRA_AUTHORITY must be an https:// URL (got ${sanitizeForLog(raw)}) — ` + + 'the authority receives the client secret and all authorization codes.', + ); + } + return raw.replace(/\/+$/, ''); +} + +const AUTHORITY = validateAuthority( + process.env.AUTOPILOT_ENTRA_AUTHORITY ?? `${ENTRA_LOGIN_BASE_URL}/organizations`, +); const SCOPES = `api://${CLIENT_ID}/access_as_user openid profile offline_access`; +/** + * Outbound budget for the Entra token exchange. Undici's defaults allow ~300 s + * of headers/body time — a hung Entra endpoint would pin sockets and Express + * handlers for 5 minutes each. 15 s is generous for a token endpoint (normally + * well under 1 s) while keeping a stall bounded. Same pattern as client.ts + * (API_TIMEOUT_MS) and access-guard.ts. + */ +const ENTRA_TOKEN_TIMEOUT_MS = 15_000; + // Dynamic client registration (RFC 7591) is STATELESS: instead of a // server-side Map, the issued client_id is itself an HMAC-signed token that // encodes the client's registered redirect_uris (see signClientId below). @@ -57,6 +106,16 @@ const SCOPES = `api://${CLIENT_ID}/access_as_user openid profile offline_access` // hostile destinations, but signing the state is a cheap belt-and-suspenders // that also catches replay (via iat/exp) and detects accidental client bugs. // +// ACCEPTED RESIDUAL RISK — no one-time-use guarantee: within the 10-minute +// window the same signed state verifies more than once. True single-use would +// need a shared nonce store (Redis/Table), reintroducing exactly the shared +// state this design removed for scale-to-zero multi-replica operation. The +// impact is bounded independently: /oauth/callback requires an authorization +// code, Entra codes are single-use and bound to the original flow's PKCE +// challenge, the redirectUri is re-validated against allowlist + client +// binding on every callback, and the client validates its own originalState +// exactly once. Decision reviewed 2026-08-15. +// // HMAC key: prefer OAuthStateSigningKey from the environment so all replicas // agree on the signature; fall back to a per-instance random key when unset // (state has a 10 min lifetime, so per-instance is acceptable for single- @@ -785,6 +844,10 @@ export function createOAuthRouter(): Router { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, body: body.toString(), + // Bounded outbound budget — covers headers AND body consumption below. + // Without it, undici's ~300 s defaults let a hung Entra endpoint pin a + // socket and an Express handler for 5 minutes per request. + signal: AbortSignal.timeout(ENTRA_TOKEN_TIMEOUT_MS), }); const data = await tokenResponse.json(); @@ -803,6 +866,17 @@ export function createOAuthRouter(): Router { } res.status(tokenResponse.status).json(data); } catch (err) { + // AbortSignal.timeout() rejects with a DOMException named 'TimeoutError' + // (see error-handler.ts) — report that as 504 so a stalled identity + // provider is distinguishable from a hard exchange failure (502). + if (err instanceof Error && err.name === 'TimeoutError') { + console.error(`[oauth/token] Entra token endpoint timed out after ${ENTRA_TOKEN_TIMEOUT_MS}ms`); + res.status(504).json({ + error: 'token_exchange_timeout', + error_description: 'Identity provider did not respond in time', + }); + return; + } console.error('[oauth] Token exchange failed:', err); res.status(502).json({ error: 'token_exchange_failed', error_description: 'Failed to exchange token with identity provider' }); }