diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts new file mode 100644 index 00000000..a2a7fe85 --- /dev/null +++ b/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,')).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'); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index f98034be..b6bee36a 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/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. diff --git a/src/McpServer/autopilot-monitor-mcp/src/client.ts b/src/McpServer/autopilot-monitor-mcp/src/client.ts index 1b68ce31..d7004854 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/client.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/client.ts @@ -142,6 +142,16 @@ function buildQuery(params: Record. 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 })}`; diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index 5099e93d..c2afafb6 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/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(); +/** + * 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; suffixes: string[] } { + const exact = new Set(); + 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; 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; - // 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, diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts index 2317ebb8..5fddc8cd 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/admin.ts +++ b/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 = { 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); diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts index a7fc8572..c945dba5 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/error-handler.ts +++ b/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; diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/search.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/search.ts index 5dc61e2b..50c2a60c 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/search.ts +++ b/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.'), diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts index aea7eb5a..3767a4b0 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/sessions.ts +++ b/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, diff --git a/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts b/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts index 8a30d546..a5527442 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/tools/shared.ts +++ b/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 = {