From c20d9df8dcf188bc5b6edb03436793a82e95209a Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Sat, 11 Jul 2026 10:17:09 +0200 Subject: [PATCH] fix(mcp): pin OAuth redirect allowlist to exact vendor callbacks + token/log hygiene MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Medium finding: the anonymous dynamic-client-registration allowlist accepted any path on whole vendor domains plus wildcard subdomains, so an open redirect (or controllable subdomain) on an allowed domain could exfiltrate authorization codes — PKCE does not help when the attacker runs the flow with their own verifier. Defaults are now the documented callback URIs only (claude.ai + claude.com /api/mcp/auth_callback, chatgpt.com /connector_platform_oauth_redirect, vscode.dev + insiders.vscode.dev /redirect); gemini.google.com dropped (Gemini CLI uses loopback). MCP_ALLOWED_REDIRECT_HOSTS gains optional /path templates (host-only entries still work). redirect_uris with userinfo or fragments are rejected outright, loopback included; localhost stays allowed because Claude Code and Gemini CLI register http://localhost callbacks. Low finding: /oauth/token now sets Cache-Control: no-store + Pragma: no-cache on every response; Entra error logging is reduced to status/error/error_codes/ correlation_id; all user-controlled log fields (client_name, redirect_uri) pass through sanitizeForLog (control-char strip + length cap). Co-Authored-By: Claude Fable 5 --- .../src/__tests__/security-guards.test.ts | 187 ++++++++++++++---- .../autopilot-monitor-mcp/src/oauth.ts | 173 ++++++++++------ 2 files changed, 264 insertions(+), 96 deletions(-) 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 index 07b733b6..e675f112 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts @@ -14,6 +14,8 @@ * These tests are pure functions; no live backend or token needed. */ import { describe, it, expect } from 'vitest'; +import express from 'express'; +import type { AddressInfo } from 'node:net'; import { followNextLink } from '../client.js'; import { SessionIdSchema } from '../tools/shared.js'; import { ApiError } from '../client.js'; @@ -38,6 +40,8 @@ const { isAllowedRedirectUri, isAllowedRedirectUriWith, parseAllowedHosts, + sanitizeForLog, + createOAuthRouter, signState, verifyState, MAX_REDIRECT_URIS_PER_CLIENT, @@ -129,7 +133,7 @@ describe('M2 — SessionIdSchema UUID enforcement', () => { }); }); -describe('H1 — OAuth redirect_uri allowlist (default Multi-Vendor)', () => { +describe('H1 — OAuth redirect_uri allowlist (default = exact vendor callback URIs)', () => { 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); @@ -139,27 +143,50 @@ describe('H1 — OAuth redirect_uri allowlist (default Multi-Vendor)', () => { 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 the documented Claude web/desktop callback (claude.ai + claude.com)', () => { + expect(isAllowedRedirectUri('https://claude.ai/api/mcp/auth_callback')).toBe(true); + expect(isAllowedRedirectUri('https://claude.com/api/mcp/auth_callback')).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 the documented ChatGPT connector callback', () => { + expect(isAllowedRedirectUri('https://chatgpt.com/connector_platform_oauth_redirect')).toBe(true); }); - it('accepts Google Gemini host over https', () => { - expect(isAllowedRedirectUri('https://gemini.google.com/oauth/cb')).toBe(true); + it('accepts the documented VS Code stable/insiders callbacks', () => { + expect(isAllowedRedirectUri('https://vscode.dev/redirect')).toBe(true); + expect(isAllowedRedirectUri('https://insiders.vscode.dev/redirect')).toBe(true); + }); + + it('tolerates a trailing slash and a query string on a path-template match', () => { + expect(isAllowedRedirectUri('https://vscode.dev/redirect/')).toBe(true); + expect(isAllowedRedirectUri('https://vscode.dev/redirect?quality=stable')).toBe(true); + }); + + it('rejects other paths on allowlisted hosts (open-redirect containment)', () => { + expect(isAllowedRedirectUri('https://claude.ai/api/oauth/cb')).toBe(false); + expect(isAllowedRedirectUri('https://claude.ai/')).toBe(false); + expect(isAllowedRedirectUri('https://chatgpt.com/connector/oauth/cb')).toBe(false); + expect(isAllowedRedirectUri('https://vscode.dev/redirect/extra')).toBe(false); + }); + + it('rejects dot-segment traversal into a template path (URL normalizes it away)', () => { + // new URL() collapses "/api/mcp/auth_callback/../evil" to "/api/mcp/evil", + // which must then fail the exact-path compare. + expect(isAllowedRedirectUri('https://claude.ai/api/mcp/auth_callback/../evil')).toBe(false); + }); + + it('rejects subdomains and unrelated vendor hosts no longer in the defaults', () => { + expect(isAllowedRedirectUri('https://api.claude.ai/cb')).toBe(false); + expect(isAllowedRedirectUri('https://accounts.anthropic.com/cb')).toBe(false); + expect(isAllowedRedirectUri('https://app.chatgpt.com/cb')).toBe(false); + expect(isAllowedRedirectUri('https://platform.openai.com/cb')).toBe(false); + expect(isAllowedRedirectUri('https://chat.openai.com/cb')).toBe(false); + expect(isAllowedRedirectUri('https://gemini.google.com/oauth/cb')).toBe(false); }); 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); + expect(isAllowedRedirectUri('http://claude.ai/api/mcp/auth_callback')).toBe(false); + expect(isAllowedRedirectUri('http://chatgpt.com/connector_platform_oauth_redirect')).toBe(false); }); it('rejects arbitrary hosts', () => { @@ -167,17 +194,21 @@ describe('H1 — OAuth redirect_uri allowlist (default Multi-Vendor)', () => { 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. + it('rejects look-alike hosts', () => { + expect(isAllowedRedirectUri('https://evilclaude.ai/api/mcp/auth_callback')).toBe(false); + expect(isAllowedRedirectUri('https://fakechatgpt.com/connector_platform_oauth_redirect')).toBe(false); expect(isAllowedRedirectUri('https://google.com/cb')).toBe(false); - expect(isAllowedRedirectUri('https://accounts.google.com/cb')).toBe(false); + }); + + it('rejects userinfo in the URI (host-confusion primitive), including loopback', () => { + expect(isAllowedRedirectUri('https://user@claude.ai/api/mcp/auth_callback')).toBe(false); + expect(isAllowedRedirectUri('https://claude.ai:pw@evil.tld/cb')).toBe(false); + expect(isAllowedRedirectUri('http://user@localhost:1234/callback')).toBe(false); + }); + + it('rejects fragments (forbidden by RFC 6749 §3.1.2), including loopback', () => { + expect(isAllowedRedirectUri('https://claude.ai/api/mcp/auth_callback#frag')).toBe(false); + expect(isAllowedRedirectUri('http://localhost:1234/callback#frag')).toBe(false); }); it('rejects javascript:, data:, file:, ftp: schemes', () => { @@ -196,50 +227,77 @@ describe('H1 — OAuth redirect_uri allowlist (default Multi-Vendor)', () => { }); describe('H1 — OAuth allowlist via MCP_ALLOWED_REDIRECT_HOSTS env override', () => { + const find = (r: { entries: { host: string; wildcard: boolean; path: string | null }[] }, host: string) => + r.entries.find((e) => e.host === host); + 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'); + expect(find(r, 'foo.example.com')).toMatchObject({ wildcard: false, path: null }); + expect(find(r, 'baz.org')).toMatchObject({ wildcard: false, path: null }); + expect(find(r, 'bar.example.com')).toMatchObject({ wildcard: true, path: null }); + }); + + it('parseAllowedHosts parses optional path templates', () => { + const r = parseAllowedHosts('a.example.com/oauth/cb,*.b.example.com/redirect,c.example.com'); + expect(find(r, 'a.example.com')).toMatchObject({ wildcard: false, path: '/oauth/cb' }); + expect(find(r, 'b.example.com')).toMatchObject({ wildcard: true, path: '/redirect' }); + expect(find(r, 'c.example.com')).toMatchObject({ wildcard: false, path: null }); }); it('parseAllowedHosts normalizes case', () => { const r = parseAllowedHosts('Foo.EXAMPLE.com'); - expect(r.exact.has('foo.example.com')).toBe(true); + expect(find(r, 'foo.example.com')).toBeDefined(); }); 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); + expect(r.entries).toHaveLength(2); + expect(find(r, 'a.com')).toBeDefined(); + expect(find(r, 'b.com')).toBeDefined(); }); - it('parseAllowedHosts falls back to defaults when env is undefined', () => { + it('parseAllowedHosts falls back to path-pinned 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); + expect(find(r, 'claude.ai')).toMatchObject({ path: '/api/mcp/auth_callback' }); + expect(find(r, 'claude.com')).toMatchObject({ path: '/api/mcp/auth_callback' }); + expect(find(r, 'chatgpt.com')).toMatchObject({ path: '/connector_platform_oauth_redirect' }); + expect(find(r, 'vscode.dev')).toMatchObject({ path: '/redirect' }); + expect(find(r, 'insiders.vscode.dev')).toMatchObject({ path: '/redirect' }); + // No host-wide or wildcard entries remain in the defaults. + expect(r.entries.every((e) => e.path !== null && !e.wildcard)).toBe(true); }); - it('custom allowlist accepts only the configured hosts (defaults are NOT additive)', () => { + it('custom allowlist accepts only the configured entries (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://claude.ai/api/mcp/auth_callback', customOnly)).toBe(false); expect(isAllowedRedirectUriWith('https://chatgpt.com/cb', customOnly)).toBe(false); }); + it('custom path-template entries pin the path; host-only entries allow any path', () => { + const r = parseAllowedHosts('pinned.example.com/oauth/cb,open.example.com'); + expect(isAllowedRedirectUriWith('https://pinned.example.com/oauth/cb', r)).toBe(true); + expect(isAllowedRedirectUriWith('https://pinned.example.com/other', r)).toBe(false); + expect(isAllowedRedirectUriWith('https://open.example.com/anything/goes', r)).toBe(true); + }); + + it('wildcard entries match subdomains on the dot boundary only', () => { + const r = parseAllowedHosts('*.partner.example.org/cb'); + expect(isAllowedRedirectUriWith('https://api.partner.example.org/cb', r)).toBe(true); + // Bare apex does not match a `*.` entry, and look-alikes stay out. + expect(isAllowedRedirectUriWith('https://partner.example.org/cb', r)).toBe(false); + expect(isAllowedRedirectUriWith('https://evilpartner.example.org.attacker.tld/cb', r)).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(''); + const empty = { entries: [] }; 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. @@ -247,6 +305,53 @@ describe('H1 — OAuth allowlist via MCP_ALLOWED_REDIRECT_HOSTS env override', ( }); }); +describe('sanitizeForLog — user-controlled log-field hygiene', () => { + it('strips CR/LF so a hostile value cannot forge log lines', () => { + expect(sanitizeForLog('evil\r\n[oauth] fake line')).toBe('evil[oauth] fake line'); + }); + + it('strips other control characters', () => { + expect(sanitizeForLog('a\x00b\x1bc\x7fd')).toBe('abcd'); + }); + + it('caps the length', () => { + const out = sanitizeForLog('x'.repeat(500), 200); + expect(out).toHaveLength(203); // 200 chars + '...' + expect(out.endsWith('...')).toBe(true); + }); + + it('stringifies null/undefined without throwing', () => { + expect(sanitizeForLog(undefined)).toBe(''); + expect(sanitizeForLog(null)).toBe(''); + }); +}); + +describe('/oauth/token response hygiene', () => { + it('sets Cache-Control: no-store and Pragma: no-cache on token responses', async () => { + const app = express(); + app.use(express.urlencoded({ extended: false })); + app.use(createOAuthRouter()); + const server = app.listen(0, '127.0.0.1'); + await new Promise((resolve) => server.once('listening', resolve)); + const { port } = server.address() as AddressInfo; + try { + // Missing grant_type → the proxy's own 400, no outbound Entra call. + // The anti-caching headers are set route-wide at handler entry, so this + // cheap path proves they are present on every response shape. + const res = await fetch(`http://127.0.0.1:${port}/oauth/token`, { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: '', + }); + expect(res.status).toBe(400); + expect(res.headers.get('cache-control')).toBe('no-store'); + expect(res.headers.get('pragma')).toBe('no-cache'); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }); +}); + 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'); diff --git a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts index 50bf02c7..d03c030a 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/oauth.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/oauth.ts @@ -128,6 +128,19 @@ export const MAX_REDIRECT_URIS_PER_CLIENT = 10; export const MAX_REDIRECT_URI_LENGTH = 1024; export const MAX_CLIENT_NAME_LENGTH = 256; +/** + * Makes a caller-controlled value safe for single-line text logs: strips + * control characters (CR/LF would let a hostile client_name or redirect_uri + * forge additional log lines) and caps the length. Everything user-supplied + * that reaches console.error in this module must pass through here. + */ +export function sanitizeForLog(value: unknown, maxLength = 200): string { + const s = String(value ?? ''); + // eslint-disable-next-line no-control-regex + const cleaned = s.replace(/[\x00-\x1f\x7f]/g, ''); + return cleaned.length > maxLength ? `${cleaned.slice(0, maxLength)}...` : cleaned; +} + // ---- Client-id signing (HMAC) --------------------------------------------- // // The dynamic-registration client_id is a self-describing signed token rather @@ -196,69 +209,86 @@ export function verifyClientId(clientId: string | undefined | null): ClientIdPay } /** - * 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. + * Strict allowlist of redirect_uri templates. 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. + * flows (RFC 8252 §7.3). `localhost` is deliberately kept alongside the IP + * literals — Claude Code and Gemini CLI both register `http://localhost:...` + * callbacks, so an IP-literal-only rule would break real clients. * - * 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`. + * Hosted-AI-client entries are configurable via the MCP_ALLOWED_REDIRECT_HOSTS + * env var (comma-separated). When unset, the baseline below allows exactly the + * documented callback URIs of widely-deployed AI vendors — full host+path, not + * whole domains. Scoping to the vendor's dedicated OAuth callback path keeps an + * open redirect elsewhere on the same domain (or a compromised sibling + * subdomain) from being usable to exfiltrate authorization codes; PKCE does not + * help in that scenario because the attacker runs the whole flow with their own + * verifier. + * + * Entry syntax: `host` (any path — operator escape hatch), `host/path` + * (exact path, trailing-slash tolerant), and `*.host` / `*.host/path` for + * subdomain wildcards. The dot-anchored suffix check keeps `evilclaude.ai` + * from slipping 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', - // Microsoft / VS Code (GitHub Copilot, Continue, etc.) - // Stable uses https://vscode.dev/redirect; Insiders uses - // https://insiders.vscode.dev/redirect. The loopback URI VS Code also - // registers (http://127.0.0.1:) is already accepted by the - // RFC 8252 loopback rule below. - 'vscode.dev', - '*.vscode.dev', + // Anthropic — Claude web / Desktop / mobile connector callback (both the + // claude.ai and claude.com front doors). Claude Code uses RFC 8252 loopback, + // which the loopback rule below already covers. + 'claude.ai/api/mcp/auth_callback', + 'claude.com/api/mcp/auth_callback', + // OpenAI — ChatGPT connector-platform callback. + 'chatgpt.com/connector_platform_oauth_redirect', + // Microsoft / VS Code (GitHub Copilot, Continue, etc.) — Stable and Insiders + // web redirect endpoints. The loopback URI VS Code also registers + // (http://127.0.0.1:) is accepted by the loopback rule below. + 'vscode.dev/redirect', + 'insiders.vscode.dev/redirect', + // Google Gemini has no documented hosted-web MCP callback; Gemini CLI uses + // loopback redirects, which are always allowed. No entry needed. ]; -export function parseAllowedHosts(envValue: string | undefined): { exact: Set; suffixes: string[] } { - const exact = new Set(); - const suffixes: string[] = []; - const entries = envValue +export interface AllowedRedirectEntry { + host: string; // lowercase, no leading `*.` + wildcard: boolean; // true → match any subdomain of host (dot-anchored) + path: string | null; // null → any path; else exact pathname (trailing-slash tolerant) +} + +export function parseAllowedHosts(envValue: string | undefined): { entries: AllowedRedirectEntry[] } { + const entries: AllowedRedirectEntry[] = []; + const raw = 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); - } + for (const item of raw) { + let rest = item.toLowerCase(); + const wildcard = rest.startsWith('*.'); + if (wildcard) rest = rest.slice(2); + const slash = rest.indexOf('/'); + const host = slash >= 0 ? rest.slice(0, slash) : rest; + const path = slash >= 0 ? rest.slice(slash) : null; + if (!host) continue; + entries.push({ host, wildcard, path }); } - return { exact, suffixes }; + return { entries }; } 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)`, + `[oauth] MCP_ALLOWED_REDIRECT_HOSTS set — using ${ALLOWED_HOSTS.entries.length} ` + + 'allowlist entrie(s) (loopback always allowed)', ); } +/** Exact pathname compare, tolerant of a single trailing slash on either side. */ +function pathMatchesTemplate(templatePath: string, pathname: string): boolean { + const norm = (p: string) => (p.length > 1 && p.endsWith('/') ? p.slice(0, -1) : p); + return norm(templatePath) === norm(pathname); +} + /** * Internal worker that applies the host check against an arbitrary allowlist. * Exposed for tests so the ENV-override path can be exercised without @@ -266,7 +296,7 @@ if (process.env.MCP_ALLOWED_REDIRECT_HOSTS) { */ export function isAllowedRedirectUriWith( uri: string | undefined | null, - allowed: { exact: Set; suffixes: string[] }, + allowed: { entries: AllowedRedirectEntry[] }, ): boolean { if (!uri) return false; let parsed: URL; @@ -278,6 +308,14 @@ export function isAllowedRedirectUriWith( // Reject anything other than http/https. Blocks file:, javascript:, data:, etc. if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return false; + // Userinfo and fragments have no legitimate place in a redirect_uri + // (RFC 6749 §3.1.2 forbids fragments outright) and both are classic + // confusion primitives — `https://claude.ai@evil.tld/` reads as claude.ai + // to a human, and a fragment can smuggle state past server-side parsers. + // Applies to loopback URIs too. + if (parsed.username !== '' || parsed.password !== '') return false; + if (parsed.hash !== '') return false; + const host = parsed.hostname.toLowerCase(); // Loopback (any port, http or https) — RFC 8252 §7.3 native-app pattern. @@ -291,9 +329,17 @@ export function isAllowedRedirectUriWith( // 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; + // Host must match (exact, or dot-anchored subdomain for wildcards) AND the + // pathname must match the entry's path template when one is set. `new URL()` + // has already collapsed dot-segments, so `/cb/../evil` cannot masquerade as + // a template path. Query strings are permitted — the host+path pair is what + // pins the vendor's callback handler. + for (const entry of allowed.entries) { + const hostMatches = entry.wildcard + ? host.endsWith(`.${entry.host}`) + : host === entry.host; + if (!hostMatches) continue; + if (entry.path === null || pathMatchesTemplate(entry.path, parsed.pathname)) return true; } return false; } @@ -436,7 +482,7 @@ export function createOAuthRouter(): Router { const invalidUris = uris.filter((u: unknown) => typeof u !== 'string' || !isAllowedRedirectUri(u)); if (invalidUris.length > 0) { console.error( - `[oauth/register] Rejected client ${client_name ?? 'unknown'}: ` + + `[oauth/register] Rejected client ${sanitizeForLog(client_name ?? 'unknown')}: ` + `${invalidUris.length} redirect_uri(s) outside allowlist`, ); res.status(400).json({ @@ -454,7 +500,7 @@ export function createOAuthRouter(): Router { // on any replica (see signClientId). const clientId = signClientId(uris, client_name ?? 'unknown'); - console.error(`[oauth/register] Registered dynamic client (${client_name ?? 'unknown'}), ${uris.length} redirect_uri(s)`); + console.error(`[oauth/register] Registered dynamic client (${sanitizeForLog(client_name ?? 'unknown')}), ${uris.length} redirect_uri(s)`); res.status(201).json({ client_id: clientId, @@ -512,7 +558,7 @@ export function createOAuthRouter(): Router { return; } if (!isAllowedRedirectUri(redirect_uri)) { - console.error(`[oauth/authorize] Rejected redirect_uri (host not in allowlist): ${redirect_uri}`); + console.error(`[oauth/authorize] Rejected redirect_uri (not in allowlist): ${sanitizeForLog(redirect_uri)}`); res.status(400).json({ error: 'invalid_request', error_description: 'redirect_uri host is not in the allowlist', @@ -547,7 +593,7 @@ export function createOAuthRouter(): Router { } if (!redirectUriMatches(registered.redirectUris, redirect_uri)) { console.error( - `[oauth/authorize] redirect_uri not registered for this client_id: ${redirect_uri}`, + `[oauth/authorize] redirect_uri not registered for this client_id: ${sanitizeForLog(redirect_uri)}`, ); res.status(400).json({ error: 'invalid_request', @@ -628,7 +674,7 @@ export function createOAuthRouter(): Router { // /oauth/authorize already gated the URI, but the state value transits the // user-agent — re-running the host allowlist here closes any replay gap. if (!isAllowedRedirectUri(redirectUri)) { - console.error(`[oauth/callback] Rejected redirectUri from state (host not in allowlist): ${redirectUri}`); + console.error(`[oauth/callback] Rejected redirectUri from state (not in allowlist): ${sanitizeForLog(redirectUri)}`); res.status(400).json({ error: 'invalid_request', error_description: 'redirect_uri host is not in the allowlist', @@ -642,7 +688,7 @@ export function createOAuthRouter(): Router { // redirectUri. A forged client_id fails closed. const registered = verifyClientId(clientId); if (!registered || !redirectUriMatches(registered.redirectUris, redirectUri)) { - console.error(`[oauth/callback] client_id invalid or redirectUri not registered: ${redirectUri}`); + console.error(`[oauth/callback] client_id invalid or redirectUri not registered: ${sanitizeForLog(redirectUri)}`); res.status(400).json({ error: 'invalid_request', error_description: 'redirect_uri does not match any URI registered for this client_id', @@ -664,6 +710,13 @@ export function createOAuthRouter(): Router { router.post('/oauth/token', oauthTokenRateLimit, async (req, res) => { const baseUrl = getPublicBaseUrl(req); + // RFC 6749 §5.1: responses carrying tokens MUST NOT be cached. Set on + // every response from this route (success, Entra error passthrough, our + // own 400s/502) — Entra sends these headers but the proxy's res.json() + // does not copy upstream headers. + res.set('Cache-Control', 'no-store'); + res.set('Pragma', 'no-cache'); + const params = req.body as Record; // Allowlist the grant before anything is forwarded. Every request to this @@ -723,7 +776,17 @@ export function createOAuthRouter(): Router { const data = await tokenResponse.json(); if (tokenResponse.status !== 200) { - console.error(`[oauth/token] Entra error: status=${tokenResponse.status}`, JSON.stringify(data)); + // Log only stable identifiers — never the full body. error_description + // can carry user/tenant details, and a full dump would also capture any + // token material Entra chooses to include in edge-case errors. The + // correlation_id is what Microsoft support needs to trace the request. + const err = (typeof data === 'object' && data !== null ? data : {}) as Record; + const codes = Array.isArray(err.error_codes) ? err.error_codes.join(',') : err.error_codes; + console.error( + `[oauth/token] Entra error: status=${tokenResponse.status} ` + + `error=${sanitizeForLog(err.error)} error_codes=${sanitizeForLog(codes)} ` + + `correlation_id=${sanitizeForLog(err.correlation_id)}`, + ); } res.status(tokenResponse.status).json(data); } catch (err) {