From 1e8e7ee018f4e06dee96fb934000cc564a15425d Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Mon, 9 Mar 2026 14:39:32 +0200 Subject: [PATCH] refactor(core): Unify SSRF bridge result contract (#26660) --- .../__tests__/ssrf-protection.service.test.ts | 172 +++++------------- .../services/ssrf/ssrf-protection.service.ts | 103 ++++++----- packages/core/src/execution-engine/index.ts | 13 +- .../request-helper-functions.test.ts | 22 +-- .../utils/request-helper-functions.ts | 5 +- .../nodes-base/test/nodes/TriggerHelpers.ts | 4 +- 6 files changed, 133 insertions(+), 186 deletions(-) diff --git a/packages/cli/src/services/ssrf/__tests__/ssrf-protection.service.test.ts b/packages/cli/src/services/ssrf/__tests__/ssrf-protection.service.test.ts index f4cad17e51b..bfdade1cf54 100644 --- a/packages/cli/src/services/ssrf/__tests__/ssrf-protection.service.test.ts +++ b/packages/cli/src/services/ssrf/__tests__/ssrf-protection.service.test.ts @@ -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 { @@ -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')); }); }); }); diff --git a/packages/cli/src/services/ssrf/ssrf-protection.service.ts b/packages/cli/src/services/ssrf/ssrf-protection.service.ts index 5ad92278b79..cbbd98f5d7f 100644 --- a/packages/cli/src/services/ssrf/ssrf-protection.service.ts +++ b/packages/cli/src/services/ssrf/ssrf-protection.service.ts @@ -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; /** * 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 { 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 { + 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 { + 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 { diff --git a/packages/core/src/execution-engine/index.ts b/packages/core/src/execution-engine/index.ts index 9de8886bc80..754fb7381f6 100644 --- a/packages/core/src/execution-engine/index.ts +++ b/packages/core/src/execution-engine/index.ts @@ -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; + /** * 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; validateRedirectSync(url: string): void; createSecureLookup(): LookupFunction; } diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/request-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/request-helper-functions.test.ts index ba72e8b70b7..72f0969840f 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/request-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/request-helper-functions.test.ts @@ -1444,8 +1444,8 @@ describe('Request Helper Functions', () => { const node = mock(); const createSsrfBridge = (overrides?: Partial): 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({ 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({ 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')); }); diff --git a/packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts b/packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts index 87735b747a0..433c68dbe52 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/request-helper-functions.ts @@ -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; } } diff --git a/packages/nodes-base/test/nodes/TriggerHelpers.ts b/packages/nodes-base/test/nodes/TriggerHelpers.ts index e2266a068f4..7ac1e5e8708 100644 --- a/packages/nodes-base/test/nodes/TriggerHelpers.ts +++ b/packages/nodes-base/test/nodes/TriggerHelpers.ts @@ -264,8 +264,8 @@ export async function testPollingTriggerNode( }), hooks: mock(), 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,