fix(mcp): harden OAuth proxy and access-guard against external review findings

External security review (2026-05-08) flagged 5 issues against the MCP
server's auth surface. All five are addressed in code; one (Bicep param
additions) lives on disk only because infra/ is gitignored — the operator
needs to redeploy with the new mcpPublicUrl + stateHmacKey parameters
documented in the bicep header to put F3 fully into effect.

#1 access-guard cache-key collision (Medium)
  Cache was keyed on UPN only. A forged JWT with a victim's UPN would
  hit a legitimate user's cached `allowed: true` and bypass the backend
  signature check for any MCP-local operation (tools/list, resources/
  list etc.) plus burn the user's rate-limit budget.
  Fix: cache key is now `${upn}:${sha256(token)}`, so distinct tokens
  always require a fresh backend roundtrip. accessCache joins the 5-min
  cleanup tick to evict expired entries (cardinality went up).

#2 /oauth/register DoS surface (Medium)
  Endpoint is unauthenticated per RFC 7591 and the registry was an
  unbounded in-memory Map at 0.5 GiB container budget. Per-IP rate-
  limit was rejected (cloud-proxy egress collisions — Zscaler etc.).
  Fix: route-level express.json({ limit: '8kb' }) registered before
  the global parser; map-size cap at 10_000 entries (~3 MB worst-case)
  → 503 when full; field-level caps on redirect_uris.length and
  client_name. TTL/LRU eviction is intentionally NOT enabled —
  documented as TODO(oauth-map-ttl) because it interacts with the
  manual /mcp reconnect flow.

#3 OAuth base URL trusted X-Forwarded-* headers (Medium)
  Discovery, issuer, WWW-Authenticate, and `${baseUrl}/oauth/callback`
  were all derived from caller-supplied headers. Container Apps Ingress
  appends but does not strip X-Forwarded-Host, so a hostile client could
  spoof the issuer in /.well-known/oauth-authorization-server.
  Fix: startup warning when MCP_PUBLIC_URL is unset. The bicep file
  (gitignored) was also extended with a `mcpPublicUrl` param — operator
  must redeploy with that pinned to the Container App FQDN after the
  first deploy emits it as an output.

#4 PKCE/state hardening (Medium)
  code_challenge was optional and only forwarded conditionally;
  code_verifier was never required at /oauth/token; state was bare
  base64url-JSON without integrity protection.
  Fix: /oauth/authorize now rejects requests without code_challenge
  or with code_challenge_method != S256 (the discovery doc already
  advertised S256-only). /oauth/token requires code_verifier for
  grant_type=authorization_code. state is HMAC-SHA256 signed with
  STATE_HMAC_KEY (env, fallback per-instance random), payload includes
  iat, /oauth/callback rejects tampered or >10-min-old state in
  constant-time compare. Bicep adds a secure `stateHmacKey` param;
  unset is acceptable for single-replica deployments.

#5 Structured 5xx leak in error-handler (Low)
  ApiError.parsed branch flowed all 5xx detail (error / hint /
  exceptionType) through to the MCP client. exceptionType in
  particular leaks CLR class names that have no business reaching a
  downstream caller.
  Fix: status>=500 branch moved before the parsed-detail branch and
  applies generic-message sanitization to structured 5xx too. Only
  correlationId and errorCode survive — those are operational
  handles for an operator to pivot on, not internals.

Tests: src/__tests__/security-guards.test.ts. The pre-existing
"preserves structured 5xx" case is inverted to assert sanitization.
16 new tests cover the cache-key fix, the registry caps, and the
state HMAC roundtrip / tamper / expiry / replay paths. 47/47 green
under `vitest run src/__tests__/security-guards.test.ts`. The
tools-*.test.ts integration suites still require AUTOPILOT_API_TOKEN
to run (pre-existing).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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

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<string, AccessCacheEntry>();
 
 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<AccessCheckResult> {
-  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<AccessCheckResul
       isGlobalAdmin: data.isGlobalAdmin === true,
     };
 
-    accessCache.set(upn, { ...result, expiresAt: Date.now() + ACCESS_CACHE_TTL_MS });
+    accessCache.set(cacheKey, { ...result, expiresAt: Date.now() + ACCESS_CACHE_TTL_MS });
     return result;
   } catch (err) {
     console.error(`[access-guard] Backend check failed for ${upn}:`, err);
@@ -88,13 +101,21 @@ interface RateEntry {
 
 const rateBuckets = new Map<string, RateEntry>();
 
-// 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) {

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

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<string, { name: string; redirectUris: string[] }>();
 
+// ---- 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<ProxyStatePayload, 'iat'>): 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<string, string>;
 
+    // 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<string, string>;
+
+    // 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<string, string>;
 
     body.set('client_id', CLIENT_ID);
     if (CLIENT_SECRET) body.set('client_secret', CLIENT_SECRET);

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;