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