From 6e7b974e8c9427feab09465a0d5d037afb2f4fcd Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Thu, 4 Apr 2024 15:08:26 +0200 Subject: [PATCH 01/12] Reapply "api: Enable local IP verification on webhooks (#2118)" (#2123) This reverts commit 5d924747074a90ca4df0ac7b82bfc9beb5f6d28c. --- packages/api/src/app-router.ts | 10 +++--- packages/api/src/webhooks/cannon.ts | 51 ++++++++++------------------- 2 files changed, 23 insertions(+), 38 deletions(-) diff --git a/packages/api/src/app-router.ts b/packages/api/src/app-router.ts index dada3b60a0..d9b5c73c65 100644 --- a/packages/api/src/app-router.ts +++ b/packages/api/src/app-router.ts @@ -53,6 +53,9 @@ const PROM_BUNDLE_OPTS: promBundle.Opts = { }, }; +const isTest = + process.env.NODE_ENV === "test" || process.env.NODE_ENV === "development"; + export default async function makeApp(params: CliArgs) { const { httpPrefix, @@ -122,15 +125,12 @@ export default async function makeApp(params: CliArgs) { recordCatalystObjectStoreId, secondaryRecordObjectStoreId, supportAddr, - verifyUrls: true, + skipUrlVerification: isTest, queue, }); await webhookCannon.start(); - if ( - process.env.NODE_ENV === "test" || - process.env.NODE_ENV === "development" - ) { + if (isTest) { await setupTestTus(); } else if (vodObjectStoreId) { await setupTus(vodObjectStoreId); diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index e1b77a77ac..3fb42dcc8a 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -42,7 +42,7 @@ function isRuntimeError(err: any): boolean { export default class WebhookCannon { running: boolean; - verifyUrls: boolean; + skipUrlVerification: boolean; frontendDomain: string; sendgridTemplateId: string; sendgridApiKey: string; @@ -51,7 +51,7 @@ export default class WebhookCannon { secondaryVodObjectStoreId: string; recordCatalystObjectStoreId: string; secondaryRecordObjectStoreId: string; - resolver: any; + resolver: dns.Resolver; queue: Queue; constructor({ frontendDomain, @@ -62,11 +62,11 @@ export default class WebhookCannon { secondaryVodObjectStoreId, recordCatalystObjectStoreId, secondaryRecordObjectStoreId, - verifyUrls, + skipUrlVerification, queue, }) { this.running = true; - this.verifyUrls = verifyUrls; + this.skipUrlVerification = skipUrlVerification; this.frontendDomain = frontendDomain; this.sendgridTemplateId = sendgridTemplateId; this.sendgridApiKey = sendgridApiKey; @@ -208,8 +208,7 @@ export default class WebhookCannon { return; } try { - // TODO Activate URL Verification - await this._fireHook(trigger, false); + await this._fireHook(trigger); } catch (err) { console.log("_fireHook error", err); await this.retry(trigger, null, err); @@ -223,10 +222,6 @@ export default class WebhookCannon { this.running = false; } - disableUrlVerify() { - this.verifyUrls = false; - } - public calcBackoff = (lastInterval?: number): number => { if (!lastInterval || lastInterval < 1000) { return 5000; @@ -328,7 +323,7 @@ export default class WebhookCannon { ); } - async _fireHook(trigger: messages.WebhookTrigger, verifyUrl = true) { + async _fireHook(trigger: messages.WebhookTrigger) { const { event, webhook, stream, user } = trigger; if (!event || !webhook || !user) { console.error( @@ -338,34 +333,24 @@ export default class WebhookCannon { return; } console.log(`trying webhook ${webhook.name}: ${webhook.url}`); - let ips, urlObj, isLocal; - if (verifyUrl) { - try { - urlObj = parseUrl(webhook.url); - if (urlObj.host) { - ips = await this.resolver.resolve4(urlObj.hostname); - } - } catch (e) { - console.error("error: ", e); - throw e; - } - } - // This is mainly useful for local testing - if (user.admin || verifyUrl === false) { - isLocal = false; - } else { + let ips: string[]; + let isLocal = false; + // These conditions are mainly useful for local testing + if (!user.admin && !this.skipUrlVerification) { try { - if (ips && ips.length) { - isLocal = isLocalIP(ips[0]); - } else { - isLocal = true; - } + const urlObj = parseUrl(webhook.url); + ips = await Promise.all([ + this.resolver.resolve4(urlObj.hostname), + this.resolver.resolve6(urlObj.hostname), + ]).then((ipsArrs) => ipsArrs.flat()); + isLocal = ips.some(isLocalIP); } catch (e) { - console.error("isLocal Error", isLocal, e); + console.error("error checking if is local IP: ", e); throw e; } } + if (isLocal) { // don't fire this webhook. console.log( From 9fbb94fe0965e9f4dd8f2858eece7ad8dc3b9028 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Thu, 4 Apr 2024 15:40:24 +0200 Subject: [PATCH 02/12] api/cannon: Handle not found DNSs --- packages/api/src/webhooks/cannon.ts | 55 ++++++++++++++++++----------- 1 file changed, 34 insertions(+), 21 deletions(-) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index 3fb42dcc8a..a4e3add783 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -1,17 +1,16 @@ import { ConsumeMessage } from "amqplib"; -import { promises as dns } from "dns"; +import dns from "dns"; import isLocalIP from "is-local-ip"; import { Response } from "node-fetch"; import { v4 as uuid } from "uuid"; import { parse as parseUrl } from "url"; -import { DB } from "../store/db"; import { DBSession } from "../store/session-table"; import messages from "../store/messages"; import Queue from "../store/queue"; import { DBWebhook } from "../store/webhook-table"; import { fetchWithTimeout, RequestInitWithTimeout, sleep } from "../util"; import logger from "../logger"; -import { sign, sendgridEmail, pathJoin } from "../controllers/helpers"; +import { sign, sendgridEmail } from "../controllers/helpers"; import { taskScheduler } from "../task/scheduler"; import { generateUniquePlaybackId } from "../controllers/generate-keys"; import { createAsset, primaryStorageExperiment } from "../controllers/asset"; @@ -51,7 +50,7 @@ export default class WebhookCannon { secondaryVodObjectStoreId: string; recordCatalystObjectStoreId: string; secondaryRecordObjectStoreId: string; - resolver: dns.Resolver; + resolver: dns.promises.Resolver; queue: Queue; constructor({ frontendDomain, @@ -75,7 +74,7 @@ export default class WebhookCannon { this.secondaryVodObjectStoreId = secondaryVodObjectStoreId; this.recordCatalystObjectStoreId = recordCatalystObjectStoreId; this.secondaryRecordObjectStoreId = secondaryRecordObjectStoreId; - this.resolver = new dns.Resolver(); + this.resolver = new dns.promises.Resolver(); this.queue = queue; // this.start(); } @@ -334,22 +333,13 @@ export default class WebhookCannon { } console.log(`trying webhook ${webhook.name}: ${webhook.url}`); - let ips: string[]; - let isLocal = false; - // These conditions are mainly useful for local testing - if (!user.admin && !this.skipUrlVerification) { - try { - const urlObj = parseUrl(webhook.url); - ips = await Promise.all([ - this.resolver.resolve4(urlObj.hostname), - this.resolver.resolve6(urlObj.hostname), - ]).then((ipsArrs) => ipsArrs.flat()); - isLocal = ips.some(isLocalIP); - } catch (e) { - console.error("error checking if is local IP: ", e); - throw e; - } - } + const { ips, isLocal } = await this.checkIsLocalIp( + webhook.url, + user.admin + ).catch((e) => { + console.error("error checking if is local IP: ", e); + throw e; + }); if (isLocal) { // don't fire this webhook. @@ -449,6 +439,29 @@ export default class WebhookCannon { } } + private async checkIsLocalIp(url: string, isAdmin: boolean) { + if (isAdmin || this.skipUrlVerification) { + // this is mainly useful for local testing + return { ips: [], isLocal: false }; + } + + const emptyIfNotFound = (err) => { + if ([dns.NODATA, dns.NOTFOUND, dns.BADFAMILY].includes(err.code)) { + return [] as string[]; + } + throw err; + }; + + const urlObj = parseUrl(url); + const ips = await Promise.all([ + this.resolver.resolve4(urlObj.hostname).catch(emptyIfNotFound), + this.resolver.resolve6(urlObj.hostname).catch(emptyIfNotFound), + ]).then((ipsArrs) => ipsArrs.flat()); + + const isLocal = ips.some(isLocalIP); + return { ips, isLocal }; + } + async storeTriggerStatus( webhook: DBWebhook, triggerTime: number, From 0140a8477393f60a8df6c42777f073b1d7437d0a Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Thu, 4 Apr 2024 15:45:05 +0200 Subject: [PATCH 03/12] api/cannon: Make DNS resolution errors non fatal --- packages/api/src/webhooks/cannon.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index a4e3add783..eea62333cb 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -338,7 +338,7 @@ export default class WebhookCannon { user.admin ).catch((e) => { console.error("error checking if is local IP: ", e); - throw e; + return { ips: [], isLocal: false }; }); if (isLocal) { From 1e15d9140d229bafa9836a62a0535e75c691d65b Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Fri, 5 Apr 2024 16:46:05 +0200 Subject: [PATCH 04/12] api/cannon: Handle IP hostnames gracefully --- packages/api/src/webhooks/cannon.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index eea62333cb..3edac3ae51 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -1,4 +1,5 @@ import { ConsumeMessage } from "amqplib"; +import { isIP } from "net"; import dns from "dns"; import isLocalIP from "is-local-ip"; import { Response } from "node-fetch"; @@ -452,11 +453,13 @@ export default class WebhookCannon { throw err; }; - const urlObj = parseUrl(url); - const ips = await Promise.all([ - this.resolver.resolve4(urlObj.hostname).catch(emptyIfNotFound), - this.resolver.resolve6(urlObj.hostname).catch(emptyIfNotFound), - ]).then((ipsArrs) => ipsArrs.flat()); + const { hostname } = parseUrl(url); + const ips = isIP(hostname) + ? [hostname] + : await Promise.all([ + this.resolver.resolve4(hostname).catch(emptyIfNotFound), + this.resolver.resolve6(hostname).catch(emptyIfNotFound), + ]).then((ipsArrs) => ipsArrs.flat()); const isLocal = ips.some(isLocalIP); return { ips, isLocal }; From e00130d77bfae70e3aeb97534c51ed1ce74ffc81 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 23 Apr 2024 18:04:53 +0200 Subject: [PATCH 05/12] api/store: Avoid sql injection on webhook status update --- packages/api/src/store/webhook-table.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/src/store/webhook-table.ts b/packages/api/src/store/webhook-table.ts index c8ab96ae44..766fa8bfcf 100644 --- a/packages/api/src/store/webhook-table.ts +++ b/packages/api/src/store/webhook-table.ts @@ -50,7 +50,7 @@ export default class WebhookTable extends Table { async updateStatus(id: string, status: DBWebhook["status"]) { const res = await this.db.query( - `UPDATE ${ + sql`UPDATE ${ this.name } SET data = jsonb_set(data, '{status}', case when data->'status' is null then '{}' else data->'status' end || '${JSON.stringify( status From 39e9b140b9fc0c9f2fd921dd2d4194178de816e9 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 23 Apr 2024 18:09:15 +0200 Subject: [PATCH 06/12] [DEV] Add some debug logging --- packages/api/src/webhooks/cannon.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index 3edac3ae51..0f8c904632 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -441,6 +441,9 @@ export default class WebhookCannon { } private async checkIsLocalIp(url: string, isAdmin: boolean) { + console.log( + `checkIsLocalIp(${url}): admin ${isAdmin} skip ${this.skipUrlVerification}` + ); if (isAdmin || this.skipUrlVerification) { // this is mainly useful for local testing return { ips: [], isLocal: false }; @@ -448,6 +451,9 @@ export default class WebhookCannon { const emptyIfNotFound = (err) => { if ([dns.NODATA, dns.NOTFOUND, dns.BADFAMILY].includes(err.code)) { + console.log( + `checkIsLocalIp(${url}): returning empty array for ${err.code} error` + ); return [] as string[]; } throw err; @@ -460,8 +466,10 @@ export default class WebhookCannon { this.resolver.resolve4(hostname).catch(emptyIfNotFound), this.resolver.resolve6(hostname).catch(emptyIfNotFound), ]).then((ipsArrs) => ipsArrs.flat()); + console.log(`checkIsLocalIp(${url}): resolved IPs for ${hostname}: ${ips}`); const isLocal = ips.some(isLocalIP); + console.log(`checkIsLocalIp(${url}): isLocal=${isLocal}`); return { ips, isLocal }; } From feb1bb0c57e7a53f83220d85a991db50e59b1957 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 23 Apr 2024 20:23:37 +0200 Subject: [PATCH 07/12] api/store: Fix updateStatus query Can't parametrize table name --- packages/api/src/store/webhook-table.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/api/src/store/webhook-table.ts b/packages/api/src/store/webhook-table.ts index 766fa8bfcf..68eb66523c 100644 --- a/packages/api/src/store/webhook-table.ts +++ b/packages/api/src/store/webhook-table.ts @@ -49,12 +49,14 @@ export default class WebhookTable extends Table { } async updateStatus(id: string, status: DBWebhook["status"]) { + const statusStr = JSON.stringify(status); const res = await this.db.query( - sql`UPDATE ${ - this.name - } SET data = jsonb_set(data, '{status}', case when data->'status' is null then '{}' else data->'status' end || '${JSON.stringify( - status - )}') WHERE id = '${id}'` + sql`` + .append(`UPDATE ${this.name} `) // table name can't be parameterized, append a raw string + .append( + sql`SET data = jsonb_set(data, '{status}', case when data->'status' is null then '{}' else data->'status' end || '${statusStr}') ` + ) + .append(sql`WHERE id = '${id}'`) ); if (res.rowCount < 1) { From f854d4b4932ecf40812aa81ff65fa7588724e655 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 23 Apr 2024 20:32:46 +0200 Subject: [PATCH 08/12] Revert "[DEV] Add some debug logging" This reverts commit 39e9b140b9fc0c9f2fd921dd2d4194178de816e9. --- packages/api/src/webhooks/cannon.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index 0f8c904632..3edac3ae51 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -441,9 +441,6 @@ export default class WebhookCannon { } private async checkIsLocalIp(url: string, isAdmin: boolean) { - console.log( - `checkIsLocalIp(${url}): admin ${isAdmin} skip ${this.skipUrlVerification}` - ); if (isAdmin || this.skipUrlVerification) { // this is mainly useful for local testing return { ips: [], isLocal: false }; @@ -451,9 +448,6 @@ export default class WebhookCannon { const emptyIfNotFound = (err) => { if ([dns.NODATA, dns.NOTFOUND, dns.BADFAMILY].includes(err.code)) { - console.log( - `checkIsLocalIp(${url}): returning empty array for ${err.code} error` - ); return [] as string[]; } throw err; @@ -466,10 +460,8 @@ export default class WebhookCannon { this.resolver.resolve4(hostname).catch(emptyIfNotFound), this.resolver.resolve6(hostname).catch(emptyIfNotFound), ]).then((ipsArrs) => ipsArrs.flat()); - console.log(`checkIsLocalIp(${url}): resolved IPs for ${hostname}: ${ips}`); const isLocal = ips.some(isLocalIP); - console.log(`checkIsLocalIp(${url}): isLocal=${isLocal}`); return { ips, isLocal }; } From c84dd1e79a18ffec0ac9140d4d3d11c08de12fe6 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 23 Apr 2024 20:35:31 +0200 Subject: [PATCH 09/12] api: Handle localhost explicitly --- packages/api/src/webhooks/cannon.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index 3edac3ae51..a2e88ff2c3 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -454,6 +454,11 @@ export default class WebhookCannon { }; const { hostname } = parseUrl(url); + if (["localhost", "ip6-localhost", "ip6-loopback"].includes(hostname)) { + // dns.resolve functions do not take /etc/hosts into account, so we need to handle these separately + return { ips: ["127.0.0.1"], isLocal: true }; + } + const ips = isIP(hostname) ? [hostname] : await Promise.all([ From 67cf535f994cff4854d14aadc5be6f05d19a8823 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 23 Apr 2024 20:45:35 +0200 Subject: [PATCH 10/12] api/store/updateStatus: Remove surrounding quotes on params --- packages/api/src/store/webhook-table.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/api/src/store/webhook-table.ts b/packages/api/src/store/webhook-table.ts index 68eb66523c..0060f7a572 100644 --- a/packages/api/src/store/webhook-table.ts +++ b/packages/api/src/store/webhook-table.ts @@ -54,9 +54,9 @@ export default class WebhookTable extends Table { sql`` .append(`UPDATE ${this.name} `) // table name can't be parameterized, append a raw string .append( - sql`SET data = jsonb_set(data, '{status}', case when data->'status' is null then '{}' else data->'status' end || '${statusStr}') ` + sql`SET data = jsonb_set(data, '{status}', case when data->'status' is null then '{}' else data->'status' end || ${statusStr}) ` ) - .append(sql`WHERE id = '${id}'`) + .append(sql`WHERE id = ${id}`) ); if (res.rowCount < 1) { From 63ea0c6a5e701461a75da856bc7456d4ec19c239 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Wed, 24 Apr 2024 19:56:25 +0200 Subject: [PATCH 11/12] api/store: Add tests to local webhook logic --- packages/api/src/webhooks/cannon.test.ts | 162 +++++++++++++++++++++++ packages/api/src/webhooks/cannon.ts | 5 +- 2 files changed, 165 insertions(+), 2 deletions(-) diff --git a/packages/api/src/webhooks/cannon.test.ts b/packages/api/src/webhooks/cannon.test.ts index 629b1345f5..d31d9aeef9 100644 --- a/packages/api/src/webhooks/cannon.test.ts +++ b/packages/api/src/webhooks/cannon.test.ts @@ -219,6 +219,47 @@ describe("webhook cannon", () => { expect(called).toBe(true); }); + describe("local webhook", () => { + beforeAll(() => { + server.webhook.skipUrlVerification = false; + }); + + afterAll(() => { + server.webhook.skipUrlVerification = true; + }); + + it("should not call local webhooks", async () => { + // we create the same mock webhook, but given url verification is enabled it should not be called + const res = await client.post("/webhook", { + ...mockWebhook, + name: "test non admin", + }); + const resJson = await res.json(); + console.log("webhook body: ", resJson); + expect(res.status).toBe(201); + expect(resJson.name).toBe("test non admin"); + + const sem = semaphore(); + let called = false; + webhookCallback = () => { + called = true; + sem.release(); + }; + + await server.queue.publishWebhook("events.stream.started", { + type: "webhook_event", + id: "webhook_test_12", + timestamp: Date.now(), + streamId: "streamid", + event: "stream.started", + userId: nonAdminUser.id, + }); + + await sem.wait(3000); + expect(called).toBe(false); + }); + }); + it("should call multiple webhooks", async () => { let res = await client.post("/webhook", { ...mockWebhook, @@ -355,4 +396,125 @@ describe("webhook cannon", () => { expect(calledCounts).toEqual([4, 2]); }); }); + + describe("local IP check", () => { + beforeAll(() => { + server.webhook.skipUrlVerification = false; + }); + + afterAll(() => { + server.webhook.skipUrlVerification = true; + }); + + const expectIsLocal = async ( + url: string, + isLocal: boolean, + ips?: string[] + ) => { + expect(await server.webhook.checkIsLocalIp(url, false)).toMatchObject({ + isLocal, + ips, + }); + }; + + it("should flag local IPs", async () => { + await expectIsLocal("http://127.0.0.1/test", true, ["127.0.0.1"]); + await expectIsLocal("http://[::1]/test", true, ["::1"]); + }); + + it("should flag private IPs", async () => { + await expectIsLocal("http://10.42.0.1/test", true, ["10.42.0.1"]); + await expectIsLocal("http://172.16.3.4/test", true, ["172.16.3.4"]); + await expectIsLocal("http://[fd12:3456:789a:1::1]/test", true, [ + "fd12:3456:789a:1::1", + ]); + }); + + it("should flag loopback addresses", async () => { + await expectIsLocal("http://localhost:1234/test", true, ["127.0.0.1"]); + await expectIsLocal("http://ip6-localhost:1234/test", true, ["::1"]); + await expectIsLocal("http://ip6-loopback:1234/test", true, ["::1"]); + }); + + it("should not flag public IPs", async () => { + await expectIsLocal("http://172.67.149.35/test", false, [ + "172.67.149.35", + ]); + await expectIsLocal("http://[2606:4700:3037::ac43:9523]/test", false, [ + "2606:4700:3037::ac43:9523", + ]); + }); + + describe("domain resolution", () => { + let prevResolver; + let resolverMock: ReturnType; + + const createResolverMock = () => ({ + resolve4: jest.fn, any, any>(), + resolve6: jest.fn, any, any>(), + }); + + beforeEach(() => { + prevResolver = server.webhook.resolver; + server.webhook.resolver = resolverMock = createResolverMock() as any; + }); + + afterEach(() => { + server.webhook.resolver = prevResolver; + }); + + it("should not flag domains that resolve to public IPs", async () => { + resolverMock.resolve4.mockReturnValueOnce( + Promise.resolve(["172.67.149.35"]) + ); + resolverMock.resolve6.mockReturnValueOnce( + Promise.resolve(["2606:4700:3037::ac43:9523"]) + ); + + await expectIsLocal("http://livepeer.studio/mock", false, [ + "172.67.149.35", + "2606:4700:3037::ac43:9523", + ]); + expect(resolverMock.resolve4.mock.calls).toHaveLength(1); + expect(resolverMock.resolve4.mock.calls[0][0]).toEqual( + "livepeer.studio" + ); + expect(resolverMock.resolve6.mock.calls).toHaveLength(1); + expect(resolverMock.resolve6.mock.calls[0][0]).toEqual( + "livepeer.studio" + ); + }); + + const privateTestCases = [ + { name: "IPv4-only", ipv4: ["10.42.0.10"], ipv6: [] }, + { name: "IPv6-only", ipv4: [], ipv6: ["::1"] }, + { name: "IPv4 and IPv6", ipv4: ["172.0.0.1"], ipv6: ["::1"] }, + { + name: "mixed private and public IPs", + ipv4: ["172.67.149.35", "172.16.34.123"], + ipv6: ["2606:4700:3037::ac43:9523", "fd12:3456:789a:1::1"], + }, + ]; + + for (const { name, ipv4, ipv6 } of privateTestCases) { + it(`should flag domains that resolve to private IPs (${name})`, async () => { + resolverMock.resolve4.mockReturnValueOnce(Promise.resolve(ipv4)); + resolverMock.resolve6.mockReturnValueOnce(Promise.resolve(ipv6)); + + await expectIsLocal("http://local.mydomain.com/test", true, [ + ...ipv4, + ...ipv6, + ]); + expect(resolverMock.resolve4.mock.calls).toHaveLength(1); + expect(resolverMock.resolve4.mock.calls[0][0]).toEqual( + "local.mydomain.com" + ); + expect(resolverMock.resolve6.mock.calls).toHaveLength(1); + expect(resolverMock.resolve6.mock.calls[0][0]).toEqual( + "local.mydomain.com" + ); + }); + } + }); + }); }); diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index a2e88ff2c3..42beaa05f9 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -440,7 +440,7 @@ export default class WebhookCannon { } } - private async checkIsLocalIp(url: string, isAdmin: boolean) { + public async checkIsLocalIp(url: string, isAdmin: boolean) { if (isAdmin || this.skipUrlVerification) { // this is mainly useful for local testing return { ips: [], isLocal: false }; @@ -456,7 +456,8 @@ export default class WebhookCannon { const { hostname } = parseUrl(url); if (["localhost", "ip6-localhost", "ip6-loopback"].includes(hostname)) { // dns.resolve functions do not take /etc/hosts into account, so we need to handle these separately - return { ips: ["127.0.0.1"], isLocal: true }; + const ips = hostname === "localhost" ? ["127.0.0.1"] : ["::1"]; + return { ips, isLocal: true }; } const ips = isIP(hostname) From b7059e22ba5d43f36e5c6c8da7e5be7ef12dee33 Mon Sep 17 00:00:00 2001 From: Victor Elias Date: Tue, 7 May 2024 12:18:44 +0100 Subject: [PATCH 12/12] Optimize imports --- packages/api/src/webhooks/cannon.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/api/src/webhooks/cannon.ts b/packages/api/src/webhooks/cannon.ts index 819ac6095f..ad95619c5e 100644 --- a/packages/api/src/webhooks/cannon.ts +++ b/packages/api/src/webhooks/cannon.ts @@ -1,8 +1,8 @@ import { ConsumeMessage } from "amqplib"; -import { isIP } from "net"; import dns from "dns"; import isLocalIP from "is-local-ip"; import _ from "lodash"; +import { isIP } from "net"; import { Response } from "node-fetch"; import { parse as parseUrl } from "url"; import { v4 as uuid } from "uuid";