From 07d0ce4ce9885281633800d7a14d7b2abab5c76f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 15 Mar 2026 17:08:43 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=A4=20fix:=20Fail-Closed=20MCP=20Domai?= =?UTF-8?q?n=20Validation=20for=20Unparseable=20URLs=20(#12245)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🛡️ fix: Fail-closed MCP domain validation for unparseable URLs `isMCPDomainAllowed` returned true (allow) when `extractMCPServerDomain` could not parse the URL, treating it identically to a stdio transport. A URL containing template placeholders or invalid syntax bypassed the domain allowlist, then `processMCPEnv` resolved it to a valid—and potentially disallowed—host at connection time. Distinguish "no URL" (stdio, allowed) from "has URL but unparseable" (rejected when an allowlist is active) by checking whether `config.url` is an explicit non-empty string before falling through to the stdio path. When no allowlist is configured the guard does not fire—unparseable URLs fall through to connection-level SSRF protection via `createSSRFSafeUndiciConnect`, preserving legitimate `customUserVars` template-URL configs. * test: Expand MCP domain validation coverage for invalid/templated URLs Cover all branches of the fail-closed guard: - Invalid/templated URLs rejected when allowlist is configured - Invalid/templated URLs allowed when no allowlist (null/undefined/[]) - Whitespace-only and empty-string URLs treated as absent across all allowedDomains configurations - Stdio configs (no url property) remain allowed --- packages/api/src/auth/domain.spec.ts | 31 +++++++++++++++++++++++++++- packages/api/src/auth/domain.ts | 20 ++++++++++++++++-- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/packages/api/src/auth/domain.spec.ts b/packages/api/src/auth/domain.spec.ts index 8ba72d82a2..76f50213db 100644 --- a/packages/api/src/auth/domain.spec.ts +++ b/packages/api/src/auth/domain.spec.ts @@ -1046,8 +1046,37 @@ describe('isMCPDomainAllowed', () => { }); describe('invalid URL handling', () => { - it('should allow config with invalid URL (treated as stdio)', async () => { + it('should reject invalid URL when allowlist is configured', async () => { const config = { url: 'not-a-valid-url' }; + expect(await isMCPDomainAllowed(config, ['example.com'])).toBe(false); + }); + + it('should reject templated URL when allowlist is configured', async () => { + const config = { url: 'http://{{CUSTOM_HOST}}/mcp' }; + expect(await isMCPDomainAllowed(config, ['example.com'])).toBe(false); + }); + + it('should allow invalid URL when no allowlist is configured (defers to connection-level SSRF)', async () => { + const config = { url: 'http://{{CUSTOM_HOST}}/mcp' }; + expect(await isMCPDomainAllowed(config, null)).toBe(true); + expect(await isMCPDomainAllowed(config, undefined)).toBe(true); + expect(await isMCPDomainAllowed(config, [])).toBe(true); + }); + + it('should allow config with whitespace-only URL (treated as absent)', async () => { + const config = { url: ' ' }; + expect(await isMCPDomainAllowed(config, [])).toBe(true); + expect(await isMCPDomainAllowed(config, ['example.com'])).toBe(true); + expect(await isMCPDomainAllowed(config, null)).toBe(true); + }); + + it('should allow config with empty string URL (treated as absent)', async () => { + const config = { url: '' }; + expect(await isMCPDomainAllowed(config, ['example.com'])).toBe(true); + }); + + it('should allow config with no url property (stdio)', async () => { + const config = { command: 'node', args: ['server.js'] }; expect(await isMCPDomainAllowed(config, ['example.com'])).toBe(true); }); }); diff --git a/packages/api/src/auth/domain.ts b/packages/api/src/auth/domain.ts index 37510f5e9b..3babb09aa6 100644 --- a/packages/api/src/auth/domain.ts +++ b/packages/api/src/auth/domain.ts @@ -442,7 +442,10 @@ export async function isActionDomainAllowed( /** * Extracts full domain spec (protocol://hostname:port) from MCP server config URL. * Returns the full origin for proper protocol/port matching against allowedDomains. - * Returns null for stdio transports (no URL) or invalid URLs. + * @returns The full origin string, or null when: + * - No `url` property, non-string, or empty (stdio transport — always allowed upstream) + * - URL string present but cannot be parsed (rejected fail-closed upstream when allowlist active) + * Callers must distinguish these two null cases; see {@link isMCPDomainAllowed}. * @param config - MCP server configuration (accepts any config with optional url field) */ export function extractMCPServerDomain(config: Record): string | null { @@ -466,6 +469,11 @@ export function extractMCPServerDomain(config: Record): string * Validates MCP server domain against allowedDomains. * Supports HTTP, HTTPS, WS, and WSS protocols (per MCP specification). * Stdio transports (no URL) are always allowed. + * Configs with a non-empty URL that cannot be parsed are rejected fail-closed when an + * allowlist is active, preventing template placeholders (e.g. `{{HOST}}`) from bypassing + * domain validation after `processMCPEnv` resolves them at connection time. + * When no allowlist is configured, unparseable URLs fall through to connection-level + * SSRF protection (`createSSRFSafeUndiciConnect`). * @param config - MCP server configuration with optional url field * @param allowedDomains - List of allowed domains (with wildcard support) */ @@ -474,8 +482,16 @@ export async function isMCPDomainAllowed( allowedDomains?: string[] | null, ): Promise { const domain = extractMCPServerDomain(config); + const hasAllowlist = Array.isArray(allowedDomains) && allowedDomains.length > 0; - // Stdio transports don't have domains - always allowed + const hasExplicitUrl = + Object.hasOwn(config, 'url') && typeof config.url === 'string' && config.url.trim().length > 0; + + if (!domain && hasExplicitUrl && hasAllowlist) { + return false; + } + + // Stdio transports (no URL) are always allowed if (!domain) { return true; }