From 65ed90b603306dc9b018003e0c55e58e1c981d4f Mon Sep 17 00:00:00 2001 From: Matt Date: Tue, 2 Jun 2026 12:40:41 +0200 Subject: [PATCH] =?UTF-8?q?fix(audit):=20webhook=20cluster=20=E2=80=94=20M?= =?UTF-8?q?21=20(test-send=20isActive),=20M22=20(cross-tenant=20dead-lette?= =?UTF-8?q?r),=20L28=20(ipv6=20SSRF),=20L29=20(rebind=20doc),=20L30=20(rep?= =?UTF-8?q?lay=20event-time)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/queue/workers/webhooks.ts | 57 ++++++++++++++++++++++++---- src/lib/services/webhooks.service.ts | 18 +++++++++ src/lib/validators/webhooks.ts | 35 +++++++++++++++-- 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/src/lib/queue/workers/webhooks.ts b/src/lib/queue/workers/webhooks.ts index 87610dda..0c5cd79b 100644 --- a/src/lib/queue/workers/webhooks.ts +++ b/src/lib/queue/workers/webhooks.ts @@ -14,10 +14,29 @@ import { isLocalOrPrivateHost } from '@/lib/validators/webhooks'; * disallowed range. Defends against DNS rebinding where the validator-time * resolution returned a public address but dispatch-time resolution * returns a private one. + * + * L29 — residual TOCTOU / DNS-rebind window: this function resolves the host + * and validates every returned IP, but the subsequent `fetch` does its OWN + * independent DNS resolution. An attacker controlling an authoritative server + * with a very short TTL could return a public IP here and a private IP to + * `fetch` microseconds later (classic DNS rebinding). Fully closing this + * requires PINNING the validated IP — i.e. connecting by the exact address we + * checked while preserving Host header + TLS SNI. That needs a custom undici + * dispatcher (`undici.Agent({ connect: { lookup } })`); `undici` is not a + * direct dependency and `node:undici` is not an importable built-in here, and + * rewriting delivery onto `node:https` would discard the redirect/SSRF + * handling built around `fetch`. So we do NOT yet pin. What we DO tighten: + * - the resolved/validated addresses are returned to the caller so a future + * pin can connect to one of them without re-resolving; + * - the check runs as LATE as possible (immediately before `fetch`), making + * the rebind window as narrow as the time between this lookup and fetch's; + * - the redirect SSRF path (the easy route to the same target) is already + * closed via `redirect: 'manual'` (audit H1). + * The remaining gap is a short-TTL rebind race; tracked as audit L29. */ async function resolveAndCheckHost( rawUrl: string, -): Promise<{ ok: true } | { ok: false; reason: string }> { +): Promise<{ ok: true; addresses: string[] } | { ok: false; reason: string }> { if (isLocalOrPrivateHost(rawUrl)) { return { ok: false, reason: 'webhook URL host blocked by static check' }; } @@ -35,13 +54,13 @@ async function resolveAndCheckHost( return { ok: false, reason: `resolved address ${a.address} is in a blocked range` }; } } + return { ok: true, addresses: addresses.map((a) => a.address) }; } catch (err) { return { ok: false, reason: `DNS resolution failed: ${err instanceof Error ? err.message : 'unknown'}`, }; } - return { ok: true }; } // ─── Job Payload ───────────────────────────────────────────────────────────── @@ -70,7 +89,7 @@ export const webhooksWorker = new Worker( const { db } = await import('@/lib/db'); const { webhooks, webhookDeliveries } = await import('@/lib/db/schema/system'); - const { userProfiles } = await import('@/lib/db/schema/users'); + const { userProfiles, userPortRoles } = await import('@/lib/db/schema/users'); const { decrypt } = await import('@/lib/utils/encryption'); const { createNotification } = await import('@/lib/services/notifications.service'); const { eq, and } = await import('drizzle-orm'); @@ -221,6 +240,13 @@ export const webhooksWorker = new Worker( const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 10_000); + // L29: `fetch` re-resolves `webhook.url`'s hostname independently of the + // `resolveAndCheckHost` check just above, so a short-TTL DNS-rebind race + // remains (validated public IP here, private IP at connect). We do not + // pin `hostCheck.addresses` because that needs a custom undici dispatcher + // that isn't importable in this runtime — see resolveAndCheckHost for the + // full rationale and the mitigations that are in place. Residual gap is a + // narrow rebind window; the easier redirect route is closed below. const response = await fetch(webhook.url, { method: 'POST', // SSRF guard (audit H1): never follow redirects. resolveAndCheckHost @@ -322,14 +348,29 @@ export const webhooksWorker = new Worker( severity: 'error', }); - // Notify all super admins + // M22: notify only super-admins who actually operate in the + // originating port. The webhook's `description` embeds the + // admin-controlled `webhook.name` and a `/admin/webhooks/{id}` link + // scoped to THIS tenant; the old query selected every super-admin of + // every tenant, leaking Port A's webhook name (and a minor injection + // vector) into unrelated tenants' notification feeds. Requiring a + // `userPortRoles` row for `portId` keeps the alert inside the tenant. + // `select` with the inner join can fan out one row per role, so we + // dedupe userIds before notifying. try { - const superAdmins = await db - .select({ userId: userProfiles.userId }) + const superAdminRows = await db + .selectDistinct({ userId: userProfiles.userId }) .from(userProfiles) - .where(and(eq(userProfiles.isSuperAdmin, true), eq(userProfiles.isActive, true))); + .innerJoin(userPortRoles, eq(userPortRoles.userId, userProfiles.userId)) + .where( + and( + eq(userProfiles.isSuperAdmin, true), + eq(userProfiles.isActive, true), + eq(userPortRoles.portId, portId), + ), + ); - for (const admin of superAdmins) { + for (const admin of superAdminRows) { void createNotification({ portId, userId: admin.userId, diff --git a/src/lib/services/webhooks.service.ts b/src/lib/services/webhooks.service.ts index a9ee91d1..96774b2c 100644 --- a/src/lib/services/webhooks.service.ts +++ b/src/lib/services/webhooks.service.ts @@ -309,10 +309,20 @@ export async function redeliverWebhookDelivery( .limit(1); if (!source) throw new NotFoundError('Delivery'); + // L30: redeliver intentionally RE-SIGNS the original captured payload with a + // FRESH signature + fresh `X-Webhook-Timestamp` at dispatch time (see worker + // `finalPayload`). A receiver that judges freshness solely from the transport + // timestamp / delivery id would therefore accept arbitrarily-old event data + // as if it were new. This is by design (replaying a missed delivery), but the + // business-level event age must travel inside `data` so receivers can apply + // an event-time freshness check independent of the transport envelope. We + // surface the ORIGINAL delivery's `createdAt` as `original_event_at` (and keep + // the existing `retried_from` / `retried_at` replay markers). const replayPayload = { ...(source.payload as Record), retried_from: deliveryId, retried_at: new Date().toISOString(), + original_event_at: source.createdAt?.toISOString() ?? null, }; const [next] = await db @@ -363,6 +373,14 @@ export async function sendTestWebhook(portId: string, webhookId: string, eventTy throw new NotFoundError('Webhook'); } + // M21: mirror redeliverWebhookDelivery — refuse to fire a live signed POST + // for a webhook an admin has explicitly disabled (e.g. because its endpoint + // was flagged). Without this, the test button is a convenient operator- + // controlled trigger for an otherwise-inert webhook. + if (!webhook.isActive) { + throw new NotFoundError('Webhook is inactive'); + } + // Create a pending delivery record const [delivery] = await db .insert(webhookDeliveries) diff --git a/src/lib/validators/webhooks.ts b/src/lib/validators/webhooks.ts index d1a3b81f..a38f17c1 100644 --- a/src/lib/validators/webhooks.ts +++ b/src/lib/validators/webhooks.ts @@ -45,6 +45,32 @@ function isBlockedIpv4(host: string): boolean { return false; } +/** + * Extracts the embedded IPv4 dotted-quad from an IPv4-mapped IPv6 literal. + * + * L28: Node's URL parser normalizes `[::ffff:127.0.0.1]` to the hex form + * `[::ffff:7f00:1]`, so the old `h.slice(7)` (which assumed a dotted-quad + * tail like `::ffff:127.0.0.1`) produced `7f00:1` and never matched + * `isBlockedIpv4`'s dotted-quad regex — the mapped-range block was dead. + * We now handle BOTH encodings: a literal dotted-quad tail, and the + * two-hextet hex tail Node actually emits, reconstructing the dotted-quad + * from the hextets before range-checking. + */ +function extractMappedIpv4(h: string): string | null { + if (!h.startsWith('::ffff:')) return null; + const tail = h.slice(7); + // Form 1: `::ffff:127.0.0.1` (dotted-quad tail survived parsing) + if (/^\d{1,3}(\.\d{1,3}){3}$/.test(tail)) return tail; + // Form 2: `::ffff:7f00:1` (Node-normalized two-hextet hex tail). + const hextets = tail.split(':'); + if (hextets.length !== 2) return null; + if (!hextets.every((g) => /^[0-9a-f]{1,4}$/.test(g))) return null; + const high = parseInt(hextets[0]!, 16); + const low = parseInt(hextets[1]!, 16); + if (Number.isNaN(high) || Number.isNaN(low)) return null; + return `${(high >> 8) & 0xff}.${high & 0xff}.${(low >> 8) & 0xff}.${low & 0xff}`; +} + /** Returns true if `host` is an IPv6 literal in any disallowed range. */ function isBlockedIpv6(host: string): boolean { // strip brackets if present @@ -53,10 +79,11 @@ function isBlockedIpv6(host: string): boolean { if (h === '::') return true; // unspecified if (h.startsWith('fe80:') || h.startsWith('fe80::')) return true; // link-local if (/^f[cd][0-9a-f]{2}:/.test(h)) return true; // fc00::/7 unique-local - if (h.startsWith('::ffff:')) { - // IPv4-mapped - unwrap and check - const ipv4 = h.slice(7); - return isBlockedIpv4(ipv4); + // IPv4-mapped (::ffff:0:0/96) — block when the embedded v4 is private/ + // reserved. Handles both the dotted-quad and Node's normalized hex form. + const mapped = extractMappedIpv4(h); + if (mapped !== null) { + return isBlockedIpv4(mapped); } return false; }