mcp: OAuth-proxy hardening — Entra token timeout, https boot guards, replay note

- /oauth/token: AbortSignal.timeout(15s) on the outbound Entra fetch;
  TimeoutError maps to 504 token_exchange_timeout (502 stays for hard
  failures). Undici defaults previously allowed ~300s per stalled request.
- Boot guards (fail-fast, matching config.ts posture): AUTOPILOT_ENTRA_AUTHORITY
  must be https (sovereign clouds allowed, host not pinned); MCP_PUBLIC_URL pin
  must be https (http tolerated for loopback dev only), trailing slashes
  stripped; production log hint when AUTOPILOT_ENTRA_CLIENT_SECRET is unset
  (public-client vs AADSTS7000218 misconfig).
- State one-time-use: documented as accepted residual risk (a nonce store would
  reintroduce the shared state the stateless multi-replica design removed;
  impact bounded by single-use PKCE-bound Entra codes + callback re-validation).
- Tests: oauth-boot-guards.test.ts (validators) + token-timeout route tests
  with Entra-host-scoped fetch stub. Suite: 559 passed, tsc clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

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/);
+  });
+});

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<string, string>) {
+    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<string, unknown>;
+    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');
+  });
+});

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 / ' +

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' });
     }