refactor(core): Unify SSRF bridge result contract (#26660)

2026-09-24 23:22:38 +08:00 · 2026-03-09 12:39:32 +00:00
parent f2a65da361
commit 1e8e7ee018
6 changed files with 133 additions and 186 deletions
@@ -3,6 +3,7 @@ import { SsrfProtectionConfig } from '@n8n/config';
 import { mock } from 'jest-mock-extended';

 import type { DnsResolver } from '../dns-resolver';
+import { SsrfBlockedIpError } from '../ssrf-blocked-ip.error';
 import { SsrfProtectionService } from '../ssrf-protection.service';

 function createConfig(overrides: Partial<SsrfProtectionConfig> = {}): SsrfProtectionConfig {
@@ -32,6 +33,14 @@ function createService(
 	};
 }

+const expectBlocked = (result: unknown) => {
+	expect(result).toEqual({ ok: false, error: expect.any(SsrfBlockedIpError) });
+};
+
+const expectAllowed = (result: unknown) => {
+	expect(result).toEqual({ ok: true, result: undefined });
+};
+
 describe('SsrfProtectionService', () => {
 	beforeEach(() => {
 		jest.clearAllMocks();
@@ -49,7 +58,7 @@ describe('SsrfProtectionService', () => {
 			])('should block %s (in %s)', (ip) => {
 				const { service } = createService();
 				const result = service.validateIp(ip);
-				expect(result).toEqual({ allowed: false, reason: 'IP address is blocked', ip });
+				expectBlocked(result);
 			});
 		});

@@ -58,41 +67,25 @@ describe('SsrfProtectionService', () => {
 				'should block IPv4 loopback %s',
 				(ip) => {
 					const { service } = createService();
-					expect(service.validateIp(ip)).toEqual({
-						allowed: false,
-						reason: 'IP address is blocked',
-						ip,
-					});
+					expectBlocked(service.validateIp(ip));
 				},
 			);

 			it('should block IPv6 loopback ::1', () => {
 				const { service } = createService();
-				expect(service.validateIp('::1')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '::1',
-				});
+				expectBlocked(service.validateIp('::1'));
 			});
 		});

 		describe('blocked link-local addresses', () => {
 			it('should block IPv4 link-local 169.254.1.1', () => {
 				const { service } = createService();
-				expect(service.validateIp('169.254.1.1')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '169.254.1.1',
-				});
+				expectBlocked(service.validateIp('169.254.1.1'));
 			});

 			it('should block IPv6 link-local fe80::1', () => {
 				const { service } = createService();
-				expect(service.validateIp('fe80::1')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: 'fe80::1',
-				});
+				expectBlocked(service.validateIp('fe80::1'));
 			});
 		});

@@ -101,11 +94,7 @@ describe('SsrfProtectionService', () => {
 				'should block special address %s',
 				(ip) => {
 					const { service } = createService();
-					expect(service.validateIp(ip)).toEqual({
-						allowed: false,
-						reason: 'IP address is blocked',
-						ip,
-					});
+					expectBlocked(service.validateIp(ip));
 				},
 			);
 		});
@@ -115,7 +104,7 @@ describe('SsrfProtectionService', () => {
 				'should allow public IP %s',
 				(ip) => {
 					const { service } = createService();
-					expect(service.validateIp(ip)).toEqual({ allowed: true });
+					expectAllowed(service.validateIp(ip));
 				},
 			);
 		});
@@ -126,7 +115,7 @@ describe('SsrfProtectionService', () => {
 					allowedIpRanges: ['10.0.0.0/8'] as unknown as SsrfProtectionConfig['allowedIpRanges'],
 				});

-				expect(service.validateIp('10.0.0.1')).toEqual({ allowed: true });
+				expectAllowed(service.validateIp('10.0.0.1'));
 			});

 			it('should allow a specific blocked IP in the allowlist', () => {
@@ -134,23 +123,15 @@ describe('SsrfProtectionService', () => {
 					allowedIpRanges: ['127.0.0.1/32'] as unknown as SsrfProtectionConfig['allowedIpRanges'],
 				});

-				expect(service.validateIp('127.0.0.1')).toEqual({ allowed: true });
+				expectAllowed(service.validateIp('127.0.0.1'));
 				// Other loopback IPs should still be blocked
-				expect(service.validateIp('127.0.0.2')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '127.0.0.2',
-				});
+				expectBlocked(service.validateIp('127.0.0.2'));
 			});
 		});

-		it('should return invalid for non-IP strings', () => {
+		it('should throw for non-IP strings', () => {
 			const { service } = createService();
-			expect(service.validateIp('not-an-ip')).toEqual({
-				allowed: false,
-				reason: 'Invalid IP address',
-				ip: 'not-an-ip',
-			});
+			expect(() => service.validateIp('not-an-ip')).toThrow('Invalid IP address');
 		});
 	});

@@ -158,7 +139,10 @@ describe('SsrfProtectionService', () => {
 		it('should reject invalid URLs', async () => {
 			const { service } = createService();
 			const result = await service.validateUrl('not-a-url');
-			expect(result).toEqual({ allowed: false, reason: 'Invalid URL', url: 'not-a-url' });
+			expect(result).toEqual({
+				ok: false,
+				error: expect.objectContaining({ message: 'Invalid URL: not-a-url' }),
+			});
 		});

 		it('should validate direct IPv4 addresses in URLs', async () => {
@@ -166,11 +150,7 @@ describe('SsrfProtectionService', () => {

 			const result = await service.validateUrl('http://127.0.0.1/admin');

-			expect(result).toEqual({
-				allowed: false,
-				reason: 'IP address is blocked',
-				ip: '127.0.0.1',
-			});
+			expectBlocked(result);
 		});

 		it('should validate direct IPv6 addresses in URLs', async () => {
@@ -178,11 +158,7 @@ describe('SsrfProtectionService', () => {

 			const result = await service.validateUrl('http://[::1]/admin');

-			expect(result).toEqual({
-				allowed: false,
-				reason: 'IP address is blocked',
-				ip: '::1',
-			});
+			expectBlocked(result);
 		});

 		it('should allow public IPs in URLs', async () => {
@@ -190,7 +166,7 @@ describe('SsrfProtectionService', () => {

 			const result = await service.validateUrl('http://8.8.8.8/');

-			expect(result).toEqual({ allowed: true });
+			expectAllowed(result);
 		});

 		it('should resolve hostnames and validate resolved IPs', async () => {
@@ -200,7 +176,7 @@ describe('SsrfProtectionService', () => {
 			const { service } = createService({}, dnsResolver);
 			const result = await service.validateUrl('http://example.com/api');

-			expect(result).toEqual({ allowed: true });
+			expectAllowed(result);
 		});

 		it('should block if hostname resolves to blocked IP', async () => {
@@ -210,11 +186,7 @@ describe('SsrfProtectionService', () => {
 			const { service } = createService({}, dnsResolver);
 			const result = await service.validateUrl('http://malicious.com/');

-			expect(result).toEqual({
-				allowed: false,
-				reason: 'IP address is blocked',
-				ip: '10.0.0.1',
-			});
+			expectBlocked(result);
 		});

 		it('should block if any resolved IP is blocked', async () => {
@@ -227,11 +199,7 @@ describe('SsrfProtectionService', () => {
 			const { service } = createService({}, dnsResolver);
 			const result = await service.validateUrl('http://multi-ip.example.com/');

-			expect(result).toEqual({
-				allowed: false,
-				reason: 'IP address is blocked',
-				ip: '127.0.0.1',
-			});
+			expectBlocked(result);
 		});

 		it('should block if any resolved IP is blocked across IPv4/IPv6', async () => {
@@ -244,25 +212,18 @@ describe('SsrfProtectionService', () => {
 			const { service } = createService({}, dnsResolver);
 			const result = await service.validateUrl('http://mixed-family.example.com/');

-			expect(result).toEqual({
-				allowed: false,
-				reason: 'IP address is blocked',
-				ip: '::1',
-			});
+			expectBlocked(result);
 		});

-		it('should fail when DNS resolution returns no results', async () => {
+		it('should throw when DNS resolution returns no results', async () => {
 			const dnsResolver = createMockDnsResolver();
 			dnsResolver.lookup.mockResolvedValue([]);

 			const { service } = createService({}, dnsResolver);
-			const result = await service.validateUrl('http://nonexistent.example.com/');

-			expect(result).toEqual({
-				allowed: false,
-				reason: 'DNS resolution failed',
-				hostname: 'nonexistent.example.com',
-			});
+			await expect(service.validateUrl('http://nonexistent.example.com/')).rejects.toThrow(
+				'DNS lookup for nonexistent.example.com returned no results',
+			);
 		});

 		it('should bubble up DNS resolver errors', async () => {
@@ -291,19 +252,13 @@ describe('SsrfProtectionService', () => {

 			const result = await service.validateUrl('http://api.internal.n8n.io/health');

-			expect(result).toEqual({ allowed: true });
-			// DNS should not have been called since hostname matched
-			expect(dnsResolver.lookup).not.toHaveBeenCalled();
+			expectAllowed(result);
 		});

 		it('should accept URL objects', async () => {
 			const { service } = createService();
 			const result = await service.validateUrl(new URL('http://127.0.0.1'));
-			expect(result).toEqual({
-				allowed: false,
-				reason: 'IP address is blocked',
-				ip: '127.0.0.1',
-			});
+			expectBlocked(result);
 		});

 		it('should use DNS resolver for hostname lookups', async () => {
@@ -313,7 +268,7 @@ describe('SsrfProtectionService', () => {
 			const { service } = createService({}, dnsResolver);
 			const result = await service.validateUrl('http://cached.example.com/');

-			expect(result).toEqual({ allowed: true });
+			expectAllowed(result);
 			expect(dnsResolver.lookup).toHaveBeenCalledWith('cached.example.com', { all: true });
 		});
 	});
@@ -335,7 +290,7 @@ describe('SsrfProtectionService', () => {

 			// Initial URL is public (allowed)
 			const initial = await service.validateUrl('http://public.example.com');
-			expect(initial).toEqual({ allowed: true });
+			expectAllowed(initial);

 			// Redirect target is private (blocked)
 			expect(() => service.validateRedirectSync('http://192.168.1.1/admin')).toThrow(
@@ -498,22 +453,14 @@ describe('SsrfProtectionService', () => {
 				// %31%32%37%2e%30%2e%30%2e%31 = 127.0.0.1 percent-encoded
 				// URL constructor normalizes this back to 127.0.0.1
 				const result = await service.validateUrl('http://%31%32%37%2e%30%2e%30%2e%31/');
-				expect(result).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '127.0.0.1',
-				});
+				expectBlocked(result);
 			});

 			it('should handle URLs with encoded path components', async () => {
 				const { service } = createService();

 				const result = await service.validateUrl('http://127.0.0.1/%61%64%6d%69%6e');
-				expect(result).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '127.0.0.1',
-				});
+				expectBlocked(result);
 			});
 		});

@@ -526,39 +473,24 @@ describe('SsrfProtectionService', () => {
 				const { service } = createService({}, dnsResolver);
 				const result = await service.validateUrl('http://2130706433/');

-				expect(result).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '127.0.0.1',
-				});
+				expectBlocked(result);
 			});
 		});

 		describe('IPv6-mapped IPv4 addresses', () => {
 			it('should block ::ffff:127.0.0.1', () => {
 				const { service } = createService();
-
-				const result = service.validateIp('::ffff:127.0.0.1');
-
-				expect(result).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '::ffff:127.0.0.1',
-				});
+				expectBlocked(service.validateIp('::ffff:127.0.0.1'));
 			});

 			it('should block ::ffff:10.0.0.1', () => {
 				const { service } = createService();
-				expect(service.validateIp('::ffff:10.0.0.1')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: '::ffff:10.0.0.1',
-				});
+				expectBlocked(service.validateIp('::ffff:10.0.0.1'));
 			});

 			it('should allow ::ffff: with public IP', () => {
 				const { service } = createService();
-				expect(service.validateIp('::ffff:8.8.8.8')).toEqual({ allowed: true });
+				expectAllowed(service.validateIp('::ffff:8.8.8.8'));
 			});
 		});

@@ -599,20 +531,12 @@ describe('SsrfProtectionService', () => {
 		describe('IPv6 unique local addresses', () => {
 			it('should block fc00:: addresses', () => {
 				const { service } = createService();
-				expect(service.validateIp('fc00::1')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: 'fc00::1',
-				});
+				expectBlocked(service.validateIp('fc00::1'));
 			});

 			it('should block fd00:: addresses', () => {
 				const { service } = createService();
-				expect(service.validateIp('fd00::1')).toEqual({
-					allowed: false,
-					reason: 'IP address is blocked',
-					ip: 'fd00::1',
-				});
+				expectBlocked(service.validateIp('fd00::1'));
 			});
 		});
 	});
@@ -1,7 +1,9 @@
 import { Logger } from '@n8n/backend-common';
 import { SsrfProtectionConfig } from '@n8n/config';
 import { Service } from '@n8n/di';
-import { ensureError } from 'n8n-workflow';
+import { SsrfBridge, SsrfCheckResult } from 'n8n-core';
+import { createResultError, ensureError, Result, createResultOk } from 'n8n-workflow';
+import assert from 'node:assert';
 import type { LookupAddress, LookupOptions } from 'node:dns';
 import { isIP } from 'node:net';
 import type { BlockList, LookupFunction } from 'node:net';
@@ -11,9 +13,7 @@ import { HostnameMatcher } from './hostname-matcher';
 import { buildIpRangeList } from './ip-range-builder';
 import { SsrfBlockedIpError } from './ssrf-blocked-ip.error';

-export type SsrfCheckResult =
-	| { allowed: true }
-	| { allowed: false; reason: string; ip?: string; hostname?: string; url?: string };
+export type LookAndValidateResult = Result<LookupAddress[], Error>;

 /**
 * Validates outbound HTTP requests against configurable blocklists and allowlists
@@ -26,7 +26,7 @@ export type SsrfCheckResult =
 * 4. Otherwise — request is allowed
 */
@Service()
-export class SsrfProtectionService {
+export class SsrfProtectionService implements SsrfBridge {
 	private readonly logger: Logger;

 	private readonly blockedIps: BlockList;
@@ -68,37 +68,17 @@ export class SsrfProtectionService {
 	async validateUrl(url: string | URL): Promise<SsrfCheckResult> {
 		const parsed = this.tryParseUrl(url);
 		if (!parsed) {
-			this.logger.debug('Failed to parse URL for SSRF validation', {
-				url,
-			});
-			return { allowed: false, reason: 'Invalid URL', url: url ? String(url) : undefined };
+			return createResultError(new Error(`Invalid URL: ${url}`));
 		}

 		const { hostname } = parsed;

-		if (this.allowedHostnameMatcher.matches(hostname)) {
-			return { allowed: true };
+		const result = await this.lookupAndValidate(hostname, { all: true });
+		if (!result.ok) {
+			return result;
 		}

-		const cleanIp = this.normalizeIpInHostname(hostname);
-		if (isIP(cleanIp)) {
-			return this.validateIp(cleanIp);
-		}
-
-		// Resolve hostname via DNS and validate all IPs
-		const ips = await this.dnsResolver.lookup(hostname, { all: true });
-		if (ips.length === 0) {
-			return { allowed: false, reason: 'DNS resolution failed', hostname };
-		}
-
-		for (const ip of ips) {
-			const result = this.validateIp(ip.address);
-			if (!result.allowed) {
-				return result;
-			}
-		}
-
-		return { allowed: true };
+		return createResultOk(undefined);
 	}

 	/**
@@ -106,19 +86,17 @@ export class SsrfProtectionService {
 	 */
 	validateIp(ip: string): SsrfCheckResult {
 		const family = this.getIpFamily(ip);
-		if (family === null) {
-			return { allowed: false, reason: 'Invalid IP address', ip };
-		}
+		assert(family !== null, `Invalid IP address: ${ip}`);

 		if (this.allowedIps.check(ip, family)) {
-			return { allowed: true };
+			return createResultOk(undefined);
 		}

 		if (this.blockedIps.check(ip, family)) {
-			return { allowed: false, reason: 'IP address is blocked', ip };
+			return createResultError(new SsrfBlockedIpError(ip));
 		}

-		return { allowed: true };
+		return createResultOk(undefined);
 	}

 	/**
@@ -167,8 +145,8 @@ export class SsrfProtectionService {
 		const cleanIp = this.normalizeIpInHostname(hostname);
 		if (isIP(cleanIp)) {
 			const result = this.validateIp(cleanIp);
-			if (!result.allowed) {
-				throw new SsrfBlockedIpError(cleanIp, hostname);
+			if (!result.ok) {
+				throw result.error;
 			}
 		}
 	}
@@ -181,24 +159,65 @@ export class SsrfProtectionService {
 		return hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname;
 	}

+	/**
+	 * @throws {SsrfBlockedIpError} if any resolved IP is blocked
+	 */
 	private async secureLookupAsync(
 		hostname: string,
 		options: LookupOptions,
 	): Promise<LookupAddress[]> {
+		const result = await this.lookupAndValidate(hostname, options);
+		if (!result.ok) {
+			throw result.error;
+		}
+
+		return result.result;
+	}
+
+	/**
+	 * Lookup a hostname and validate the resulting IP addresses. Direct IPs
+	 * are validated without DNS resolution.
+	 */
+	private async lookupAndValidate(
+		hostname: string,
+		options: LookupOptions,
+	): Promise<LookAndValidateResult> {
+		const cleanIp = this.normalizeIpInHostname(hostname);
+		const ipFamily = isIP(cleanIp);
+
+		if (ipFamily) {
+			// Direct IP, we don't need to lookup, just validate
+
+			const result = this.validateIp(cleanIp);
+			if (!result.ok) {
+				return result;
+			}
+
+			return createResultOk([
+				{
+					address: cleanIp,
+					family: ipFamily,
+				},
+			]);
+		}
+
+		// Hostname, we need to lookup first and then validate the IP(s)
 		const resolved = await this.dnsResolver.lookup(hostname, options);
+		// The resolves must always return result(s) or throw
+		assert(resolved.length > 0, `DNS lookup for ${hostname} returned no results`);

 		if (this.allowedHostnameMatcher.matches(hostname)) {
-			return resolved;
+			return createResultOk(resolved);
 		}

 		for (const ip of resolved) {
 			const result = this.validateIp(ip.address);
-			if (!result.allowed) {
-				throw new SsrfBlockedIpError(ip.address, hostname);
+			if (!result.ok) {
+				return result;
 			}
 		}

-		return resolved;
+		return createResultOk(resolved);
 	}

 	private tryParseUrl(url: string | URL): URL | null {
@@ -1,16 +1,23 @@
-import type { DataTableProxyProvider, IExecutionContext, IWorkflowSettings } from 'n8n-workflow';
+import type {
+	DataTableProxyProvider,
+	IExecutionContext,
+	IWorkflowSettings,
+	Result,
+} from 'n8n-workflow';
 import type { LookupFunction } from 'node:net';

 import type { ExecutionLifecycleHooks } from './execution-lifecycle-hooks';
 import type { ExternalSecretsProxy } from './external-secrets-proxy';

+export type SsrfCheckResult = Result<void, Error>;
+
 /**
 * Narrow interface for SSRF protection, satisfied structurally by SsrfProtectionService.
 * Defined here so packages/core can use it without importing from packages/cli.
 */
 export interface SsrfBridge {
-	validateIp(ip: string): { allowed: boolean; reason?: string };
-	validateUrl(url: string | URL): Promise<{ allowed: boolean; reason?: string }>;
+	validateIp(ip: string): SsrfCheckResult;
+	validateUrl(url: string | URL): Promise<SsrfCheckResult>;
 	validateRedirectSync(url: string): void;
 	createSecureLookup(): LookupFunction;
 }
@@ -1444,8 +1444,8 @@ describe('Request Helper Functions', () => {
 		const node = mock<INode>();

 		const createSsrfBridge = (overrides?: Partial<SsrfBridge>): SsrfBridge => ({
-			validateIp: jest.fn().mockReturnValue({ allowed: true }),
-			validateUrl: jest.fn().mockResolvedValue({ allowed: true }),
+			validateIp: jest.fn().mockReturnValue({ ok: true, result: undefined }),
+			validateUrl: jest.fn().mockResolvedValue({ ok: true, result: undefined }),
 			validateRedirectSync: jest.fn(),
 			createSecureLookup: jest.fn().mockReturnValue(jest.fn()),
 			...overrides,
@@ -1468,11 +1468,10 @@ describe('Request Helper Functions', () => {
 				expect(response).toEqual({ ok: true });
 			});

-			test('should throw UserError when validateUrl blocks a direct IP request', async () => {
+			test('should throw when validateUrl blocks a direct IP request', async () => {
+				const blockedError = new UserError('IP address is blocked');
 				const ssrfBridge = createSsrfBridge({
-					validateUrl: jest
-						.fn()
-						.mockResolvedValue({ allowed: false, reason: 'IP address is blocked' }),
+					validateUrl: jest.fn().mockResolvedValue({ ok: false, error: blockedError }),
 				});
 				const additionalData = mock<IWorkflowExecuteAdditionalData>({
 					hooks,
@@ -1487,7 +1486,7 @@ describe('Request Helper Functions', () => {
 						method: 'GET',
 						url: 'http://127.0.0.1/secret',
 					}),
-				).rejects.toThrow(UserError);
+				).rejects.toThrow('IP address is blocked');

 				expect(ssrfBridge.validateUrl).toHaveBeenCalledWith(new URL('http://127.0.0.1/secret'));
 			});
@@ -1559,11 +1558,10 @@ describe('Request Helper Functions', () => {
 		});

 		describe('proxyRequestToAxios (legacy path)', () => {
-			test('should throw UserError when validateUrl blocks a direct IP request', async () => {
+			test('should throw when validateUrl blocks a direct IP request', async () => {
+				const blockedError = new UserError('IP address is blocked');
 				const ssrfBridge = createSsrfBridge({
-					validateUrl: jest
-						.fn()
-						.mockResolvedValue({ allowed: false, reason: 'IP address is blocked' }),
+					validateUrl: jest.fn().mockResolvedValue({ ok: false, error: blockedError }),
 				});
 				const additionalData = mock<IWorkflowExecuteAdditionalData>({
 					hooks,
@@ -1572,7 +1570,7 @@ describe('Request Helper Functions', () => {

 				await expect(
 					proxyRequestToAxios(workflow, additionalData, node, 'http://10.0.0.1/internal'),
-				).rejects.toThrow(UserError);
+				).rejects.toThrow('IP address is blocked');

 				expect(ssrfBridge.validateUrl).toHaveBeenCalledWith(new URL('http://10.0.0.1/internal'));
 			});
@@ -31,7 +31,6 @@ import {
 	NodeApiError,
 	NodeOperationError,
 	NodeSslError,
-	UserError,
 	isObjectEmpty,
 	ExecutionBaseError,
 	jsonParse,
@@ -915,8 +914,8 @@ async function validateUrlSsrf(url: string | undefined, ssrfBridge?: SsrfBridge)
 	if (!parsed) return;

 	const result = await ssrfBridge.validateUrl(parsed);
-	if (!result.allowed) {
-		throw new UserError(`SSRF protection blocked request to ${url}: ${result.reason}`);
+	if (!result.ok) {
+		throw result.error;
 	}
 }

@@ -264,8 +264,8 @@ export async function testPollingTriggerNode(
 			}),
 			hooks: mock<ExecutionLifecycleHooks>(),
 			ssrfBridge: {
-				validateIp: jest.fn().mockReturnValue({ allowed: true }),
-				validateUrl: jest.fn().mockResolvedValue({ allowed: true }),
+				validateIp: jest.fn().mockReturnValue({ ok: true, result: undefined }),
+				validateUrl: jest.fn().mockResolvedValue({ ok: true, result: undefined }),
 				validateRedirectSync: jest.fn(),
 				createSecureLookup: jest.fn().mockReturnValue(jest.fn()),
 			} as SsrfBridge,