diff --git a/ApiCore-Backend/src/webhooks/webhook-url-guard.spec.ts b/ApiCore-Backend/src/webhooks/webhook-url-guard.spec.ts index b318865..593c8d6 100644 --- a/ApiCore-Backend/src/webhooks/webhook-url-guard.spec.ts +++ b/ApiCore-Backend/src/webhooks/webhook-url-guard.spec.ts @@ -1,35 +1,47 @@ import { BadRequestException } from '@nestjs/common'; import { assertPublicWebhookUrl } from './webhook-url-guard'; +function lookupResolving(...addresses: string[]) { + return () => + Promise.resolve(addresses.map((address) => ({ address, family: 4 }))); +} + +function lookupFailing() { + return () => Promise.reject(new Error('ENOTFOUND')); +} + describe('assertPublicWebhookUrl', () => { - it('accepts a normal public https URL', () => { - expect(() => - assertPublicWebhookUrl('https://example.com/hook'), - ).not.toThrow(); + it('accepts a hostname that resolves only to public addresses', async () => { + await expect( + assertPublicWebhookUrl( + 'https://example.com/hook', + lookupResolving('93.184.216.34'), + ), + ).resolves.toBeUndefined(); }); - it('rejects an invalid URL', () => { - expect(() => assertPublicWebhookUrl('not-a-url')).toThrow( + it('rejects an invalid URL', async () => { + await expect(assertPublicWebhookUrl('not-a-url')).rejects.toBeInstanceOf( BadRequestException, ); }); - it('rejects non-http(s) schemes', () => { - expect(() => assertPublicWebhookUrl('ftp://example.com/hook')).toThrow( - BadRequestException, - ); - expect(() => assertPublicWebhookUrl('file:///etc/passwd')).toThrow( - BadRequestException, - ); + it('rejects non-http(s) schemes', async () => { + await expect( + assertPublicWebhookUrl('ftp://example.com/hook'), + ).rejects.toBeInstanceOf(BadRequestException); + await expect( + assertPublicWebhookUrl('file:///etc/passwd'), + ).rejects.toBeInstanceOf(BadRequestException); }); - it('rejects localhost by hostname', () => { - expect(() => assertPublicWebhookUrl('http://localhost/hook')).toThrow( - BadRequestException, - ); + it('rejects localhost by hostname', async () => { + await expect( + assertPublicWebhookUrl('http://localhost/hook'), + ).rejects.toBeInstanceOf(BadRequestException); }); - it('rejects IPv4 loopback and its shorthand/decimal/hex/octal encodings', () => { + it('rejects IPv4 loopback and its shorthand/decimal/hex/octal encodings', async () => { for (const url of [ 'http://127.0.0.1/hook', 'http://127.1/hook', @@ -37,37 +49,79 @@ describe('assertPublicWebhookUrl', () => { 'http://2130706433/hook', 'http://017700000001/hook', ]) { - expect(() => assertPublicWebhookUrl(url)).toThrow(BadRequestException); + await expect(assertPublicWebhookUrl(url)).rejects.toBeInstanceOf( + BadRequestException, + ); } }); - it('rejects RFC 1918 private ranges', () => { + it('rejects RFC 1918 private ranges', async () => { for (const url of [ 'http://10.0.0.5/hook', 'http://172.16.0.5/hook', 'http://172.31.255.255/hook', 'http://192.168.1.1/hook', ]) { - expect(() => assertPublicWebhookUrl(url)).toThrow(BadRequestException); + await expect(assertPublicWebhookUrl(url)).rejects.toBeInstanceOf( + BadRequestException, + ); } }); - it('rejects link-local addresses including the cloud metadata endpoint', () => { - expect(() => + it('rejects link-local addresses including the cloud metadata endpoint', async () => { + await expect( assertPublicWebhookUrl('http://169.254.169.254/latest/meta-data/'), - ).toThrow(BadRequestException); + ).rejects.toBeInstanceOf(BadRequestException); }); - it('does not reject a public IPv4 literal outside reserved ranges', () => { - expect(() => assertPublicWebhookUrl('http://8.8.8.8/hook')).not.toThrow(); + it('does not reject a public IPv4 literal outside reserved ranges', async () => { + await expect( + assertPublicWebhookUrl('http://8.8.8.8/hook'), + ).resolves.toBeUndefined(); }); - it('rejects IPv6 loopback and link-local addresses', () => { - expect(() => assertPublicWebhookUrl('http://[::1]/hook')).toThrow( - BadRequestException, - ); - expect(() => assertPublicWebhookUrl('http://[fe80::1]/hook')).toThrow( - BadRequestException, - ); + it('rejects IPv6 loopback and link-local addresses', async () => { + await expect( + assertPublicWebhookUrl('http://[::1]/hook'), + ).rejects.toBeInstanceOf(BadRequestException); + await expect( + assertPublicWebhookUrl('http://[fe80::1]/hook'), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('rejects a hostname that resolves to a private address (DNS-based bypass)', async () => { + await expect( + assertPublicWebhookUrl( + 'https://attacker.example/hook', + lookupResolving('10.0.0.5'), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('rejects a hostname that resolves to the cloud metadata address', async () => { + await expect( + assertPublicWebhookUrl( + 'https://attacker.example/hook', + lookupResolving('169.254.169.254'), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('rejects a hostname where any resolved address is private, even if others are public', async () => { + await expect( + assertPublicWebhookUrl( + 'https://attacker.example/hook', + lookupResolving('93.184.216.34', '10.0.0.5'), + ), + ).rejects.toBeInstanceOf(BadRequestException); + }); + + it('rejects a hostname that fails to resolve', async () => { + await expect( + assertPublicWebhookUrl( + 'https://does-not-exist.invalid/hook', + lookupFailing(), + ), + ).rejects.toBeInstanceOf(BadRequestException); }); }); diff --git a/ApiCore-Backend/src/webhooks/webhook-url-guard.ts b/ApiCore-Backend/src/webhooks/webhook-url-guard.ts index f35c868..bb18783 100644 --- a/ApiCore-Backend/src/webhooks/webhook-url-guard.ts +++ b/ApiCore-Backend/src/webhooks/webhook-url-guard.ts @@ -1,46 +1,56 @@ import { BadRequestException } from '@nestjs/common'; +import * as dns from 'node:dns/promises'; import * as net from 'node:net'; -const BLOCKED_HOSTNAMES = new Set(['localhost', '0.0.0.0']); +const BLOCKED_HOSTNAMES = new Set(['localhost']); -function isPrivateOrReservedIpv4(octets: number[]): boolean { - const [a, b] = octets; - if (a === 127) return true; // loopback - if (a === 10) return true; // private (RFC 1918) - if (a === 172 && b >= 16 && b <= 31) return true; // private (RFC 1918) - if (a === 192 && b === 168) return true; // private (RFC 1918) - if (a === 169 && b === 254) return true; // link-local, incl. cloud metadata (169.254.169.254) - if (a === 0) return true; // "this network" +const blockedRanges = new net.BlockList(); +blockedRanges.addSubnet('10.0.0.0', 8, 'ipv4'); // RFC 1918 +blockedRanges.addSubnet('172.16.0.0', 12, 'ipv4'); // RFC 1918 +blockedRanges.addSubnet('192.168.0.0', 16, 'ipv4'); // RFC 1918 +blockedRanges.addSubnet('127.0.0.0', 8, 'ipv4'); // loopback +blockedRanges.addSubnet('169.254.0.0', 16, 'ipv4'); // link-local, incl. cloud metadata +blockedRanges.addSubnet('0.0.0.0', 8, 'ipv4'); // "this network" +blockedRanges.addAddress('::1', 'ipv6'); // loopback +blockedRanges.addAddress('::', 'ipv6'); // unspecified +blockedRanges.addSubnet('fe80::', 10, 'ipv6'); // link-local +blockedRanges.addSubnet('fc00::', 7, 'ipv6'); // unique local + +function isBlockedIp(ip: string): boolean { + const version = net.isIP(ip); + if (version === 4) return blockedRanges.check(ip, 'ipv4'); + if (version === 6) return blockedRanges.check(ip, 'ipv6'); return false; } -function isPrivateOrReservedIp(ip: string): boolean { - if (net.isIPv4(ip)) { - return isPrivateOrReservedIpv4(ip.split('.').map(Number)); - } - if (net.isIPv6(ip)) { - const normalized = ip.toLowerCase(); - if (normalized === '::1' || normalized === '::') return true; // loopback / unspecified - if (normalized.startsWith('fe80:')) return true; // link-local - if (normalized.startsWith('fc') || normalized.startsWith('fd')) return true; // unique local - const mapped = /^::ffff:(\d+\.\d+\.\d+\.\d+)$/.exec(normalized); - if (mapped && net.isIPv4(mapped[1])) { - return isPrivateOrReservedIpv4(mapped[1].split('.').map(Number)); - } - return false; - } - return false; +interface LookupAddress { + address: string; + family: number; } +type LookupFn = (hostname: string) => Promise; + +const defaultLookup: LookupFn = (hostname) => + dns.lookup(hostname, { all: true }); + /** * Blocks webhook targets that would let an admin turn the backend into an * SSRF proxy against internal services or the cloud metadata endpoint. - * Checks are syntactic only (no DNS lookups), relying on the WHATWG URL - * parser to normalize decimal/octal/hex IPv4 shorthand into dotted form - * before the range check runs; DNS-rebinding (a hostname resolving to a - * private address only at delivery time) is not covered. + * + * IP literals (including decimal/octal/hex shorthand, which the WHATWG URL + * parser normalizes before this runs) are checked directly against + * `net.BlockList`. Plain hostnames are resolved via DNS and every returned + * address is checked too, so an attacker-controlled domain pointed at an + * internal IP is rejected rather than silently passing the literal-only + * check. This still does not defend against DNS rebinding (the same + * hostname resolving to a different, private address only later, at actual + * delivery time) — closing that would require re-resolving on every + * delivery, which the caller intentionally does not do (see deliver()). */ -export function assertPublicWebhookUrl(rawUrl: string): void { +export async function assertPublicWebhookUrl( + rawUrl: string, + lookup: LookupFn = defaultLookup, +): Promise { let parsed: URL; try { parsed = new URL(rawUrl); @@ -54,13 +64,30 @@ export function assertPublicWebhookUrl(rawUrl: string): void { const hostname = parsed.hostname.toLowerCase().replace(/^\[|\]$/g, ''); if (BLOCKED_HOSTNAMES.has(hostname)) { - throw new BadRequestException( - 'Webhook URL must not target localhost or a reserved address', - ); + throw new BadRequestException('Webhook URL must not target localhost'); } - if (net.isIP(hostname) && isPrivateOrReservedIp(hostname)) { + + if (net.isIP(hostname)) { + if (isBlockedIp(hostname)) { + throw new BadRequestException( + 'Webhook URL must not target a private, loopback, or link-local address', + ); + } + return; + } + + let addresses: LookupAddress[]; + try { + addresses = await lookup(hostname); + } catch { + throw new BadRequestException('Webhook URL host could not be resolved'); + } + if ( + addresses.length === 0 || + addresses.some((record) => isBlockedIp(record.address)) + ) { throw new BadRequestException( - 'Webhook URL must not target a private, loopback, or link-local address', + 'Webhook URL must not resolve to a private, loopback, or link-local address', ); } } diff --git a/ApiCore-Backend/src/webhooks/webhooks.service.spec.ts b/ApiCore-Backend/src/webhooks/webhooks.service.spec.ts index 6e490ec..18f50ab 100644 --- a/ApiCore-Backend/src/webhooks/webhooks.service.spec.ts +++ b/ApiCore-Backend/src/webhooks/webhooks.service.spec.ts @@ -45,7 +45,7 @@ describe('WebhooksService', () => { ); const subscription = await service.create( - { url: 'https://example.test/hook', events: ['colony.created'] }, + { url: 'https://203.0.113.10/hook', events: ['colony.created'] }, 'user-1', ); @@ -53,7 +53,7 @@ describe('WebhooksService', () => { const [[createArg]] = prisma.webhookSubscription.create.mock.calls as [ [{ data: { url: string; events: string[]; createdByUserId: string } }], ]; - expect(createArg.data.url).toBe('https://example.test/hook'); + expect(createArg.data.url).toBe('https://203.0.113.10/hook'); expect(createArg.data.events).toEqual(['colony.created']); expect(createArg.data.createdByUserId).toBe('user-1'); }); diff --git a/ApiCore-Backend/src/webhooks/webhooks.service.ts b/ApiCore-Backend/src/webhooks/webhooks.service.ts index 07e4288..510102a 100644 --- a/ApiCore-Backend/src/webhooks/webhooks.service.ts +++ b/ApiCore-Backend/src/webhooks/webhooks.service.ts @@ -37,7 +37,7 @@ export class WebhooksService { dto: CreateWebhookDto, actingUserId: string, ): Promise { - assertPublicWebhookUrl(dto.url); + await assertPublicWebhookUrl(dto.url); const secret = crypto.randomBytes(SECRET_BYTES).toString('hex'); const subscription = await this.prisma.webhookSubscription.create({ data: { @@ -104,6 +104,11 @@ export class WebhooksService { 'X-ApiCore-Signature': `sha256=${signature}`, }, body, + // A redirect target is unvalidated (assertPublicWebhookUrl only + // checked the registered URL), so it must not be auto-followed — + // otherwise a subscription pointed at an attacker-controlled public + // URL could redirect delivery into internal infrastructure. + redirect: 'manual', }); } catch (error: unknown) { const message = error instanceof Error ? error.message : String(error);