fix(mcp): security hardening - 7 review findings

H1 OAuth redirect_uri host allowlist (loopback always; hosted clients
   via MCP_ALLOWED_REDIRECT_HOSTS, default Anthropic/OpenAI/Google).
   Closes auth-code-leak through unvalidated /oauth/callback.
M1 followNextLink rejects continuation paths that don't equal the
   tool's own basePath - closes cross-tool path substitution.
M2 strict UUID schema on every sessionId input - blocks WHATWG-URL
   path-normalization attacks via /api/sessions/{id}.
M3 rate-limit moved after access-check (Step 1) - forged JWTs cannot
   drain a real user's per-minute budget.
L1 get_rule_stats uses pickGlobalOrTenantPath like every other tool.
L3 5xx response bodies no longer relayed to the caller; structured
   4xx errors keep their detail.
L4 hardcoded CLIENT_ID fallback removed - module throws on boot if
   AUTOPILOT_ENTRA_CLIENT_ID is unset.

Adds security-guards.test.ts (32 unit tests) covering all guards.

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

src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts

@@ -0,0 +1,256 @@
+/**
+ * Unit tests for the security guards added in the MCP review:
+ *   H1 — OAuth redirect_uri allowlist
+ *   M1 — followNextLink basePath validation
+ *   M2 — SessionIdSchema UUID enforcement
+ *   L3 — error-handler 5xx body redaction
+ *
+ * These tests are pure functions; no live backend or token needed.
+ */
+import { describe, it, expect } from 'vitest';
+import { followNextLink } from '../client.js';
+import { SessionIdSchema } from '../tools/shared.js';
+import { ApiError } from '../client.js';
+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');
+
+describe('M1 — followNextLink basePath validation', () => {
+  const basePath = '/api/global/audit/logs';
+
+  it('accepts a nextLink that targets the same basePath', () => {
+    const result = followNextLink(
+      basePath,
+      {},
+      `${basePath}?pageSize=200&continuation=abc&dateFrom=2026-01-01`,
+    );
+    expect(result).toBe(`${basePath}?pageSize=200&continuation=abc&dateFrom=2026-01-01`);
+  });
+
+  it('accepts a nextLink with no query string', () => {
+    const result = followNextLink(basePath, {}, basePath);
+    expect(result).toBe(basePath);
+  });
+
+  it('rejects a nextLink that targets a different /api/ endpoint', () => {
+    expect(() =>
+      followNextLink(
+        basePath,
+        {},
+        '/api/global/raw/logs?somequery=evil',
+      ),
+    ).toThrow(/does not match the tool's expected base path/);
+  });
+
+  it('rejects a fully different /api/ path even when the prefix overlaps', () => {
+    // /api/global/audit/logs-extra would naively pass startsWith(basePath); ensure
+    // exact-equal path matching, not prefix.
+    expect(() =>
+      followNextLink(
+        basePath,
+        {},
+        '/api/global/audit/logs-extra?continuation=x',
+      ),
+    ).toThrow(/does not match the tool's expected base path/);
+  });
+
+  it('falls back to legacy buildQuery when continuation is opaque (no /api/ prefix)', () => {
+    const result = followNextLink(
+      basePath,
+      { pageSize: 200 },
+      'opaque-token-abc',
+    );
+    expect(result).toBe(`${basePath}?pageSize=200&continuation=opaque-token-abc`);
+  });
+
+  it('falls back to buildQuery when no continuation is supplied', () => {
+    const result = followNextLink(basePath, { pageSize: 100 }, undefined);
+    expect(result).toBe(`${basePath}?pageSize=100`);
+  });
+});
+
+describe('M2 — SessionIdSchema UUID enforcement', () => {
+  it('accepts a valid lowercase UUID', () => {
+    expect(SessionIdSchema.safeParse('e259c121-1234-4abc-9def-0123456789ab').success).toBe(true);
+  });
+
+  it('accepts a valid uppercase UUID', () => {
+    expect(SessionIdSchema.safeParse('E259C121-1234-4ABC-9DEF-0123456789AB').success).toBe(true);
+  });
+
+  it('rejects path-traversal payloads', () => {
+    // The reason we added this guard: WHATWG-URL normalizes "../admin/foo" in
+    // the path segment, which would route the request to a different backend
+    // endpoint than the tool intended.
+    expect(SessionIdSchema.safeParse('../admin/secrets').success).toBe(false);
+    expect(SessionIdSchema.safeParse('..%2Fadmin%2Fsecrets').success).toBe(false);
+    expect(SessionIdSchema.safeParse('foo/bar').success).toBe(false);
+  });
+
+  it('rejects values that are nearly-UUID but malformed', () => {
+    expect(SessionIdSchema.safeParse('e259c121-1234-4abc-9def').success).toBe(false); // too short
+    expect(SessionIdSchema.safeParse('e259c121-1234-4abc-9def-0123456789abcd').success).toBe(false); // too long
+    expect(SessionIdSchema.safeParse('zzzzzzzz-1234-4abc-9def-0123456789ab').success).toBe(false); // non-hex
+  });
+
+  it('rejects empty strings and obvious garbage', () => {
+    expect(SessionIdSchema.safeParse('').success).toBe(false);
+    expect(SessionIdSchema.safeParse('abc').success).toBe(false);
+  });
+});
+
+describe('H1 — OAuth redirect_uri allowlist (default Multi-Vendor)', () => {
+  it('accepts loopback over http with arbitrary port', () => {
+    expect(isAllowedRedirectUri('http://localhost:1234/callback')).toBe(true);
+    expect(isAllowedRedirectUri('http://127.0.0.1:54321/cb')).toBe(true);
+  });
+
+  it('accepts loopback even over https (some clients use TLS-loopback)', () => {
+    expect(isAllowedRedirectUri('https://localhost:8443/cb')).toBe(true);
+  });
+
+  it('accepts Anthropic hosts over https (claude.ai, *.claude.ai, *.anthropic.com)', () => {
+    expect(isAllowedRedirectUri('https://claude.ai/api/oauth/cb')).toBe(true);
+    expect(isAllowedRedirectUri('https://api.claude.ai/cb')).toBe(true);
+    expect(isAllowedRedirectUri('https://accounts.anthropic.com/cb')).toBe(true);
+  });
+
+  it('accepts OpenAI/ChatGPT hosts over https', () => {
+    expect(isAllowedRedirectUri('https://chatgpt.com/connector/oauth/cb')).toBe(true);
+    expect(isAllowedRedirectUri('https://app.chatgpt.com/cb')).toBe(true);
+    expect(isAllowedRedirectUri('https://platform.openai.com/cb')).toBe(true);
+    expect(isAllowedRedirectUri('https://chat.openai.com/cb')).toBe(true);
+  });
+
+  it('accepts Google Gemini host over https', () => {
+    expect(isAllowedRedirectUri('https://gemini.google.com/oauth/cb')).toBe(true);
+  });
+
+  it('rejects allowed hosts over plain http (downgrade attempt)', () => {
+    expect(isAllowedRedirectUri('http://claude.ai/cb')).toBe(false);
+    expect(isAllowedRedirectUri('http://chatgpt.com/cb')).toBe(false);
+    expect(isAllowedRedirectUri('http://gemini.google.com/cb')).toBe(false);
+  });
+
+  it('rejects arbitrary hosts', () => {
+    expect(isAllowedRedirectUri('https://attacker.tld/cb')).toBe(false);
+    expect(isAllowedRedirectUri('https://evil.example.com/cb')).toBe(false);
+  });
+
+  it('rejects look-alike hosts (suffix match must be on the dot boundary)', () => {
+    // "evilclaude.ai" ends with "claude.ai" naïvely, but the suffix list
+    // stores ".claude.ai" with a leading dot, anchoring matches to a real
+    // subdomain boundary.
+    expect(isAllowedRedirectUri('https://evilclaude.ai/cb')).toBe(false);
+    expect(isAllowedRedirectUri('https://notanthropic.com/cb')).toBe(false);
+    expect(isAllowedRedirectUri('https://fakechatgpt.com/cb')).toBe(false);
+    expect(isAllowedRedirectUri('https://notopenai.com/cb')).toBe(false);
+    // gemini.google.com is exact-match only — google.com itself is NOT allowed.
+    expect(isAllowedRedirectUri('https://google.com/cb')).toBe(false);
+    expect(isAllowedRedirectUri('https://accounts.google.com/cb')).toBe(false);
+  });
+
+  it('rejects javascript:, data:, file:, ftp: schemes', () => {
+    expect(isAllowedRedirectUri('javascript:alert(1)')).toBe(false);
+    expect(isAllowedRedirectUri('data:text/html,<script>alert(1)</script>')).toBe(false);
+    expect(isAllowedRedirectUri('file:///etc/passwd')).toBe(false);
+    expect(isAllowedRedirectUri('ftp://localhost/x')).toBe(false);
+  });
+
+  it('rejects malformed URIs', () => {
+    expect(isAllowedRedirectUri('not a url')).toBe(false);
+    expect(isAllowedRedirectUri('')).toBe(false);
+    expect(isAllowedRedirectUri(null)).toBe(false);
+    expect(isAllowedRedirectUri(undefined)).toBe(false);
+  });
+});
+
+describe('H1 — OAuth allowlist via MCP_ALLOWED_REDIRECT_HOSTS env override', () => {
+  it('parseAllowedHosts splits exact hosts and *.wildcards', () => {
+    const r = parseAllowedHosts('foo.example.com,*.bar.example.com,baz.org');
+    expect(r.exact.has('foo.example.com')).toBe(true);
+    expect(r.exact.has('baz.org')).toBe(true);
+    expect(r.exact.has('bar.example.com')).toBe(false); // wildcard, not exact
+    expect(r.suffixes).toContain('.bar.example.com');
+  });
+
+  it('parseAllowedHosts normalizes case', () => {
+    const r = parseAllowedHosts('Foo.EXAMPLE.com');
+    expect(r.exact.has('foo.example.com')).toBe(true);
+  });
+
+  it('parseAllowedHosts trims whitespace and drops empty entries', () => {
+    const r = parseAllowedHosts(' a.com ,  , b.com ');
+    expect(r.exact.size).toBe(2);
+    expect(r.exact.has('a.com')).toBe(true);
+    expect(r.exact.has('b.com')).toBe(true);
+  });
+
+  it('parseAllowedHosts falls back to defaults when env is undefined', () => {
+    const r = parseAllowedHosts(undefined);
+    // Default baseline includes all three vendors.
+    expect(r.exact.has('claude.ai')).toBe(true);
+    expect(r.exact.has('chatgpt.com')).toBe(true);
+    expect(r.exact.has('gemini.google.com')).toBe(true);
+  });
+
+  it('custom allowlist accepts only the configured hosts (defaults are NOT additive)', () => {
+    const customOnly = parseAllowedHosts('mycompany.example.com,*.partner.example.org');
+    expect(isAllowedRedirectUriWith('https://mycompany.example.com/cb', customOnly)).toBe(true);
+    expect(isAllowedRedirectUriWith('https://api.partner.example.org/cb', customOnly)).toBe(true);
+    // Defaults must NOT leak through — operator-supplied list fully replaces them.
+    expect(isAllowedRedirectUriWith('https://claude.ai/cb', customOnly)).toBe(false);
+    expect(isAllowedRedirectUriWith('https://chatgpt.com/cb', customOnly)).toBe(false);
+  });
+
+  it('custom allowlist still requires HTTPS for non-loopback', () => {
+    const r = parseAllowedHosts('mycompany.example.com');
+    expect(isAllowedRedirectUriWith('http://mycompany.example.com/cb', r)).toBe(false);
+  });
+
+  it('loopback is always allowed even with empty allowlist', () => {
+    const empty = parseAllowedHosts('');
+    expect(isAllowedRedirectUriWith('http://localhost:9000/cb', empty)).toBe(true);
+    expect(isAllowedRedirectUriWith('http://127.0.0.1:9000/cb', empty)).toBe(true);
+    // But arbitrary hosts are not.
+    expect(isAllowedRedirectUriWith('https://anywhere.com/cb', empty)).toBe(false);
+  });
+});
+
+describe('L3 — error-handler 5xx body redaction', () => {
+  it('does not leak the response body on 500', () => {
+    const apiErr = new ApiError(500, 'Stack frame at /usr/src/app/secrets.js:42 with token sk-leak');
+    const result = toolError('test_tool', { foo: 'bar' }, apiErr);
+    const text = result.content[0].text;
+    expect(text).not.toContain('sk-leak');
+    expect(text).not.toContain('/usr/src/app');
+    expect(text).toContain('HTTP 500');
+  });
+
+  it('does not leak the response body on 502/503/504', () => {
+    for (const status of [502, 503, 504]) {
+      const apiErr = new ApiError(status, 'sensitive backend internal');
+      const text = toolError('test_tool', {}, apiErr).content[0].text;
+      expect(text).not.toContain('sensitive backend internal');
+      expect(text).toContain(`HTTP ${status}`);
+    }
+  });
+
+  it('still surfaces 4xx body content (caller can act on it)', () => {
+    const apiErr = new ApiError(400, 'invalid filter syntax: bad_field');
+    const text = toolError('test_tool', {}, apiErr).content[0].text;
+    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' }));
+    const text = toolError('test_tool', {}, structured).content[0].text;
+    expect(text).toContain('service unavailable');
+    expect(text).toContain('retry in 30s');
+  });
+});

src/McpServer/autopilot-monitor-mcp/src/access-guard.ts

@@ -147,13 +147,14 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo
 
   const upn = claims.upn.toLowerCase();
 
-  // Rate limit check (sync — no backend call needed)
-  if (isRateLimited(upn)) {
-    res.status(429).json({ error: 'Rate limit exceeded', retryAfterSeconds: 60 });
-    return;
-  }
-
-  // Access check (async — calls backend, cached)
+  // Access check (async — calls backend, cached). Must run BEFORE rate-limit
+  // accounting: the JWT signature is not verified here (only the backend has
+  // JWKS access), so an attacker can forge an unsigned token with a victim's
+  // 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.
   checkAccess(upn, token)
     .then((result) => {
       if (!result.allowed) {
@@ -161,6 +162,14 @@ export function accessGuard(req: Request, res: Response, next: NextFunction): vo
         return;
       }
 
+      // Rate-limit only validated identities. A forged JWT with someone
+      // else's UPN was already rejected above; only a real, allow-listed
+      // user reaches this counter.
+      if (isRateLimited(upn)) {
+        res.status(429).json({ error: 'Rate limit exceeded', retryAfterSeconds: 60 });
+        return;
+      }
+
       // Scope the caller context (token + GA status) to this async context
       // so concurrent sessions cannot overwrite each other on the event loop,
       // and so tools can route based on role without re-checking the JWT.

src/McpServer/autopilot-monitor-mcp/src/client.ts

@@ -142,6 +142,16 @@ function buildQuery(params: Record<string, string | number | boolean | undefined
  * binds the resolved tenantId into the token's fingerprint, so page 2 must
  * re-send `?tenantId=` to validate. Tools that strip everything except
  * `continuation` would silently fail token validation on follow-up calls.
+ *
+ * Security: the path of the supplied nextLink must equal the tool's
+ * <paramref name="basePath"/>. Without this, a caller could pass any
+ * `/api/...` value via the `continuation` argument and bend the request
+ * to a different backend endpoint — bypassing the tool's declared READ_ONLY
+ * + closed-world contract. Backend RBAC still gates cross-tenant access, but
+ * cross-tool path substitution is a defense-in-depth gap. Continuation-
+ * tokens are HMAC-bound to their endpoint scope; this guard prevents callers
+ * from bypassing them entirely by issuing a fresh, token-less request to a
+ * different path.
  */
 function followNextLink(
   basePath: string,
@@ -149,6 +159,16 @@ function followNextLink(
   continuationOrNextLink?: string,
 ): string {
   if (continuationOrNextLink && continuationOrNextLink.startsWith('/api/')) {
+    const queryStart = continuationOrNextLink.indexOf('?');
+    const pathOnly = queryStart === -1
+      ? continuationOrNextLink
+      : continuationOrNextLink.slice(0, queryStart);
+    if (pathOnly !== basePath) {
+      throw new Error(
+        `Continuation nextLink path "${pathOnly}" does not match the tool's expected base path "${basePath}". ` +
+        'Pass the full nextLink string from the SAME tool\'s prior response, not a synthesized or third-party path.',
+      );
+    }
     return continuationOrNextLink;
   }
   return `${basePath}${buildQuery({ ...extraParams, continuation: continuationOrNextLink })}`;

src/McpServer/autopilot-monitor-mcp/src/oauth.ts

@@ -19,7 +19,16 @@
 import { Router } from 'express';
 import crypto from 'node:crypto';
 
-const CLIENT_ID = process.env.AUTOPILOT_ENTRA_CLIENT_ID ?? '1a400946-62c1-4ab4-aa37-f730ac89704d';
+const CLIENT_ID: string = (() => {
+  const v = process.env.AUTOPILOT_ENTRA_CLIENT_ID;
+  if (!v) {
+    throw new Error(
+      'AUTOPILOT_ENTRA_CLIENT_ID environment variable is required. ' +
+      'Set it to the Entra ID app registration client_id.',
+    );
+  }
+  return v;
+})();
 const CLIENT_SECRET = process.env.AUTOPILOT_ENTRA_CLIENT_SECRET ?? '';
 const AUTHORITY = process.env.AUTOPILOT_ENTRA_AUTHORITY ?? 'https://login.microsoftonline.com/organizations';
 const SCOPES = `api://${CLIENT_ID}/access_as_user openid profile offline_access`;
@@ -31,6 +40,106 @@ const SCOPES = `api://${CLIENT_ID}/access_as_user openid profile offline_access`
  */
 const registeredClients = new Map<string, { name: string; redirectUris: string[] }>();
 
+/**
+ * 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
+ * attacker calls the unauthenticated /oauth/register endpoint to register a
+ * client with a hostile redirect_uri, the URI must still pass this filter.
+ *
+ * Loopback (localhost / 127.0.0.1 / [::1]) is always allowed for native CLI
+ * flows (RFC 8252 §7.3). Hosted-AI-client hosts are configurable via the
+ * MCP_ALLOWED_REDIRECT_HOSTS env var (comma-separated). When unset, a
+ * baseline of widely-deployed AI vendors is used.
+ *
+ * Entry syntax: bare host (`claude.ai`) for exact match, or `*.example.com`
+ * for any subdomain. The leading-dot suffix check anchors the match to a
+ * real subdomain boundary so `evilclaude.ai` does not slip past `claude.ai`.
+ */
+const DEFAULT_ALLOWED_HOSTS: readonly string[] = [
+  // Anthropic
+  'claude.ai',
+  '*.claude.ai',
+  '*.anthropic.com',
+  // OpenAI / ChatGPT
+  'chatgpt.com',
+  '*.chatgpt.com',
+  '*.openai.com',
+  // Google / Gemini
+  'gemini.google.com',
+];
+
+export function parseAllowedHosts(envValue: string | undefined): { exact: Set<string>; suffixes: string[] } {
+  const exact = new Set<string>();
+  const suffixes: string[] = [];
+  const entries = envValue
+    ? envValue.split(',').map((s) => s.trim()).filter(Boolean)
+    : DEFAULT_ALLOWED_HOSTS;
+  for (const entry of entries) {
+    const lower = entry.toLowerCase();
+    if (lower.startsWith('*.')) {
+      // Store with the leading dot so the suffix check is anchored to a real
+      // subdomain boundary — `*.claude.ai` matches `api.claude.ai` but not
+      // `evilclaude.ai`.
+      suffixes.push(lower.slice(1));
+    } else {
+      exact.add(lower);
+    }
+  }
+  return { exact, suffixes };
+}
+
+const ALLOWED_HOSTS = parseAllowedHosts(process.env.MCP_ALLOWED_REDIRECT_HOSTS);
+
+if (process.env.MCP_ALLOWED_REDIRECT_HOSTS) {
+  console.error(
+    `[oauth] MCP_ALLOWED_REDIRECT_HOSTS set — using ${ALLOWED_HOSTS.exact.size} exact + ` +
+    `${ALLOWED_HOSTS.suffixes.length} wildcard host(s) (loopback always allowed)`,
+  );
+}
+
+/**
+ * Internal worker that applies the host check against an arbitrary allowlist.
+ * Exposed for tests so the ENV-override path can be exercised without
+ * reloading the module (ENV is captured at import time).
+ */
+export function isAllowedRedirectUriWith(
+  uri: string | undefined | null,
+  allowed: { exact: Set<string>; suffixes: string[] },
+): boolean {
+  if (!uri) return false;
+  let parsed: URL;
+  try {
+    parsed = new URL(uri);
+  } catch {
+    return false;
+  }
+  // Reject anything other than http/https. Blocks file:, javascript:, data:, etc.
+  if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return false;
+
+  const host = parsed.hostname.toLowerCase();
+
+  // Loopback (any port, http or https) — RFC 8252 §7.3 native-app pattern.
+  // Cannot be disabled via env; CLI/desktop AI clients depend on it.
+  if (host === 'localhost' || host === '127.0.0.1' || host === '[::1]' || host === '::1') {
+    return true;
+  }
+
+  // Configured hosted AI clients must use HTTPS — no point allow-listing a
+  // production AI vendor over plain HTTP, and refusing this blocks downgrade
+  // tricks where an attacker gets the user to follow `http://claude.ai/...`.
+  if (parsed.protocol !== 'https:') return false;
+
+  if (allowed.exact.has(host)) return true;
+  for (const suffix of allowed.suffixes) {
+    if (host.endsWith(suffix)) return true;
+  }
+  return false;
+}
+
+export function isAllowedRedirectUri(uri: string | undefined | null): boolean {
+  return isAllowedRedirectUriWith(uri, ALLOWED_HOSTS);
+}
+
 /**
  * Derives the public base URL of this MCP server.
  * In production: from X-Forwarded-Host / X-Forwarded-Proto (set by Container Apps ingress).
@@ -79,10 +188,31 @@ export function createOAuthRouter(): Router {
   router.post('/oauth/register', (req, res) => {
     const { client_name, redirect_uris, grant_types, response_types, token_endpoint_auth_method } = req.body ?? {};
 
+    // 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(
+        `[oauth/register] Rejected client ${client_name ?? 'unknown'}: ` +
+        `${invalidUris.length} redirect_uri(s) outside allowlist`,
+      );
+      res.status(400).json({
+        error: 'invalid_redirect_uri',
+        error_description:
+          'One or more redirect_uris are not in the allowlist. Loopback (localhost / 127.0.0.1) ' +
+          'is always accepted; hosted AI client hosts are governed by the ' +
+          'MCP_ALLOWED_REDIRECT_HOSTS env var (default: Anthropic, OpenAI, Google).',
+      });
+      return;
+    }
+
     const clientId = crypto.randomUUID();
     registeredClients.set(clientId, {
       name: client_name ?? 'unknown',
-      redirectUris: Array.isArray(redirect_uris) ? redirect_uris : [],
+      redirectUris: uris,
     });
 
     console.error(`[oauth/register] Registered dynamic client ${clientId} (${client_name ?? 'unknown'})`);
@@ -90,7 +220,7 @@ export function createOAuthRouter(): Router {
     res.status(201).json({
       client_id: clientId,
       client_name: client_name ?? 'unknown',
-      redirect_uris: redirect_uris ?? [],
+      redirect_uris: uris,
       grant_types: grant_types ?? ['authorization_code', 'refresh_token'],
       response_types: response_types ?? ['code'],
       token_endpoint_auth_method: token_endpoint_auth_method ?? 'none',
@@ -101,16 +231,71 @@ export function createOAuthRouter(): Router {
   router.get('/oauth/authorize', (req, res) => {
     const baseUrl = getPublicBaseUrl(req);
     const {
+      client_id,
       redirect_uri,
       state,
       code_challenge,
       code_challenge_method,
     } = req.query as Record<string, string>;
 
-    // Store Claude Code's redirect_uri in state so we can forward the code back
+    // 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
+    // (the attacker generates their own PKCE pair, so PKCE does not protect).
+    //
+    // Two checks:
+    //   1. URI must be in the per-client redirect list (RFC 6749 §3.1.2.4).
+    //   2. URI host must be in the static allowlist (defense against the
+    //      unauthenticated /oauth/register endpoint being abused to whitelist
+    //      a hostile URI on the fly).
+    if (!redirect_uri) {
+      res.status(400).json({ error: 'invalid_request', error_description: 'redirect_uri is required' });
+      return;
+    }
+    if (!isAllowedRedirectUri(redirect_uri)) {
+      console.error(`[oauth/authorize] Rejected redirect_uri (host not in allowlist): ${redirect_uri}`);
+      res.status(400).json({
+        error: 'invalid_request',
+        error_description: 'redirect_uri host is not in the allowlist',
+      });
+      return;
+    }
+    if (client_id) {
+      const registered = registeredClients.get(client_id);
+      if (!registered) {
+        res.status(400).json({
+          error: 'invalid_client',
+          error_description: 'Unknown client_id — register via /oauth/register first',
+        });
+        return;
+      }
+      if (!registered.redirectUris.includes(redirect_uri)) {
+        console.error(
+          `[oauth/authorize] redirect_uri not registered for client ${client_id}: ${redirect_uri}`,
+        );
+        res.status(400).json({
+          error: 'invalid_request',
+          error_description: 'redirect_uri does not match any URI registered for this client_id',
+        });
+        return;
+      }
+    }
+    // If client_id is missing we still allow the flow (the host allowlist
+    // already gates which destinations are reachable), but emit a warning so
+    // misbehaving clients are noticed.
+    if (!client_id) {
+      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({
       originalState: state,
       redirectUri: redirect_uri,
+      clientId: client_id ?? null,
     })).toString('base64url');
 
     // Always use server-defined SCOPES — Claude Code may send scope values
@@ -142,15 +327,46 @@ export function createOAuthRouter(): Router {
     // 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' });
       return;
     }
 
+    // Re-validate the redirectUri at callback time too. Belt-and-suspenders:
+    // /oauth/authorize already gated the URI, but the state value transits
+    // the user-agent and could in principle be replayed against a server
+    // whose registeredClients map was wiped (cold start) — re-running the
+    // host allowlist + registry check here closes that gap.
+    if (!isAllowedRedirectUri(redirectUri)) {
+      console.error(`[oauth/callback] Rejected redirectUri from state (host not in allowlist): ${redirectUri}`);
+      res.status(400).json({
+        error: 'invalid_request',
+        error_description: 'redirect_uri host is not in the allowlist',
+      });
+      return;
+    }
+    if (clientId) {
+      const registered = registeredClients.get(clientId);
+      // Cold-start may have wiped the registry — fall back to host allowlist
+      // alone (already validated above) rather than fail the user's flow.
+      if (registered && !registered.redirectUris.includes(redirectUri)) {
+        console.error(
+          `[oauth/callback] redirectUri not registered for client ${clientId}: ${redirectUri}`,
+        );
+        res.status(400).json({
+          error: 'invalid_request',
+          error_description: 'redirect_uri does not match any URI registered for this client_id',
+        });
+        return;
+      }
+    }
+
     // Redirect back to Claude Code with the authorization code
     const callbackParams = new URLSearchParams({
       code,

src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts

@@ -3,7 +3,7 @@ import { z } from 'zod';
 import { apiFetch, buildQuery, followNextLink, pickGlobalOrTenantPath } from '../client.js';
 import { withToolTelemetry } from '../telemetry.js';
 import { getResourceContent } from '../resource-catalog.js';
-import { READ_ONLY, READ_ONLY_OPEN, MAX_RESULT_SIZE_CHARS, toolResultText } from './shared.js';
+import { READ_ONLY, READ_ONLY_OPEN, MAX_RESULT_SIZE_CHARS, toolResultText, SessionIdSchema } from './shared.js';
 import { toolError } from './error-handler.js';
 
 export function registerAdminTools(server: McpServer): void {
@@ -334,7 +334,7 @@ export function registerAdminTools(server: McpServer): void {
     'indexed session can contribute multiple events, so total events per page may exceed pageSize.',
     {
       tenantId: z.string().optional().describe('Tenant ID. Omit for cross-tenant search (Global Admin only).'),
-      sessionId: z.string().optional().describe('Filter to a specific session'),
+      sessionId: SessionIdSchema.optional().describe('Filter to a specific session'),
       eventType: z.string().optional().describe('Event type filter (e.g. "app_install_failed", "error_detected")'),
       severity: z.enum(['Info', 'Warning', 'Error', 'Critical']).optional(),
       source: z.string().optional().describe('Filter by event source/app name (substring match)'),
@@ -534,15 +534,14 @@ export function registerAdminTools(server: McpServer): void {
     READ_ONLY,
     async (args) => withToolTelemetry('get_rule_stats', async () => {
       try {
-        const params = buildQuery({
+        const params: Record<string, string | undefined> = {
           startDate: args.startDate,
           endDate: args.endDate,
           ruleType: args.ruleType,
-        });
-        const path = args.tenantId
-          ? `/api/metrics/rule-stats${params}`
-          : `/api/global/metrics/rule-stats${params}`;
-        const data = await apiFetch(path);
+        };
+        if (args.tenantId) params.tenantId = args.tenantId;
+        const prefix = pickGlobalOrTenantPath('/api/global/metrics', '/api/metrics');
+        const data = await apiFetch(`${prefix}/rule-stats${buildQuery(params)}`);
         return toolResultText(data, MAX_RESULT_SIZE_CHARS.small);
       } catch (error: unknown) {
         return toolError('get_rule_stats', args, error);

src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts

@@ -40,6 +40,15 @@ 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;

src/McpServer/autopilot-monitor-mcp/src/tools/search.ts

@@ -3,7 +3,7 @@ import { z } from 'zod';
 import { apiFetch, buildQuery, pickGlobalOrTenantPath } from '../client.js';
 import { withToolTelemetry } from '../telemetry.js';
 import type { SearchProvider } from '../search-provider.js';
-import { READ_ONLY, MAX_RESULT_SIZE_CHARS, toolResultText } from './shared.js';
+import { READ_ONLY, MAX_RESULT_SIZE_CHARS, toolResultText, SessionIdSchema } from './shared.js';
 import { toolError } from './error-handler.js';
 
 // ── Helpers ─────────────────────────────────────────────────────────────
@@ -241,7 +241,7 @@ export function registerSearchTools(server: McpServer, knowledgeBase?: SearchPro
     'If results seem incomplete, escalate to deep_search_events for exhaustive coverage.',
     {
       query: z.string().describe('Natural language description of what to find (e.g. "app download stuck", "certificate error", "disk space low")'),
-      sessionId: z.string().optional().describe('Search within a specific session. If omitted, searches across recent failed sessions.'),
+      sessionId: SessionIdSchema.optional().describe('Search within a specific session. If omitted, searches across recent failed sessions.'),
       tenantId: z.string().optional().describe('Tenant ID. Required for non-Global Admin users; Global Admin can omit to search across tenants.'),
       topK: z.coerce.number().min(1).max(30).optional().default(10).describe('Number of matching events to return (1-30, default 10)'),
       minScore: z.coerce.number().min(0).max(1).optional().default(0.1)
@@ -374,7 +374,7 @@ export function registerSearchTools(server: McpServer, knowledgeBase?: SearchPro
     'Omit tenantId for cross-tenant search (Global Admin), or specify tenantId for single-tenant.',
     {
       query: z.string().describe('Natural language description of what to find (e.g. "app download stuck", "certificate error", "disk space low")'),
-      sessionId: z.string().optional().describe('Search within a specific session. If omitted, searches across recent failed sessions.'),
+      sessionId: SessionIdSchema.optional().describe('Search within a specific session. If omitted, searches across recent failed sessions.'),
       tenantId: z.string().optional().describe('Tenant ID. Required for non-Global Admin users; Global Admin can omit to search across tenants.'),
       topK: z.coerce.number().min(1).max(50).optional().default(20)
         .describe('Max results to return (1-50, default 20). Higher default for thoroughness.'),

src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts

@@ -2,7 +2,7 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
 import { z } from 'zod';
 import { apiFetch, buildQuery, followNextLink, pickGlobalOrTenantPath } from '../client.js';
 import { withToolTelemetry } from '../telemetry.js';
-import { READ_ONLY, MAX_RESULT_SIZE_CHARS, toolResultText } from './shared.js';
+import { READ_ONLY, MAX_RESULT_SIZE_CHARS, toolResultText, SessionIdSchema } from './shared.js';
 import { toolError } from './error-handler.js';
 
 // ── Session summary constants ───────────────────────────────────────────
@@ -153,7 +153,7 @@ export function registerSessionTools(server: McpServer): void {
     'get_session',
     'Get full details of a single enrollment session including all device metadata. Set includeAnalysis=true to also get AI rule analysis results explaining why the session failed and remediation suggestions.',
     {
-      sessionId: z.string().describe('Session UUID'),
+      sessionId: SessionIdSchema.describe('Session UUID'),
       tenantId: z.string().optional().describe('Tenant ID. If omitted, auto-resolved from the session (Global Admin can access any tenant).'),
       includeAnalysis: z.boolean().optional().default(false).describe('Include rule analysis results (failure explanations and remediation steps)'),
     },
@@ -192,7 +192,7 @@ export function registerSessionTools(server: McpServer): void {
     'filters, etc.) round-trip correctly. Stop when the response no longer contains a nextLink. Sessions with ' +
     'thousands of events are fully reachable across multiple calls.',
     {
-      sessionId: z.string().describe('Session UUID'),
+      sessionId: SessionIdSchema.describe('Session UUID'),
       tenantId: z.string().optional().describe('Tenant ID. If omitted, auto-resolved from the session.'),
       eventType: z.string().optional().describe('Filter to only events of this type'),
       severity: z.enum(['Info', 'Warning', 'Error', 'Critical']).optional(),
@@ -234,7 +234,7 @@ export function registerSessionTools(server: McpServer): void {
     'Use this as the FIRST tool when investigating a session. ' +
     'For raw unfiltered events use get_session_events. For full metadata use get_session.',
     {
-      sessionId: z.string().describe('Session UUID'),
+      sessionId: SessionIdSchema.describe('Session UUID'),
       tenantId: z.string().optional().describe('Tenant ID. If omitted, auto-resolved from the session (Global Admin can access any tenant).'),
     },
     READ_ONLY,

src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts

@@ -1,4 +1,20 @@
 import type { ToolAnnotations } from '@modelcontextprotocol/sdk/types.js';
+import { z } from 'zod';
+
+/**
+ * Zod validator for session IDs. Sessions are UUIDs and the value is
+ * interpolated unencoded into backend URL paths (`/api/sessions/{id}/...`).
+ * WHATWG-URL normalization collapses `..` segments before fetch sends the
+ * request, so an unvalidated value like `../admin/foo` would silently route
+ * to a different endpoint. The strict GUID pattern blocks both that path-
+ * traversal vector and accidental garbage inputs.
+ */
+export const SessionIdSchema = z
+  .string()
+  .regex(
+    /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i,
+    'sessionId must be a UUID (e.g. "e259c121-1234-4abc-9def-0123456789ab")',
+  );
 
 /** Read-only query tool — no side effects, idempotent, closed-world (our backend only). */
 export const READ_ONLY: ToolAnnotations = {