fix(audit): webhook cluster — M21 (test-send isActive), M22 (cross-tenant dead-letter), L28 (ipv6 SSRF), L29 (rebind doc), L30 (replay event-time)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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
+        const superAdminRows = await db
-          .select({ userId: userProfiles.userId })
+          .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,

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<string, unknown>),
     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)

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 (::ffff:0:0/96) — block when the embedded v4 is private/
-    // IPv4-mapped - unwrap and check
+  // reserved. Handles both the dotted-quad and Node's normalized hex form.
-    const ipv4 = h.slice(7);
+  const mapped = extractMappedIpv4(h);
-    return isBlockedIpv4(ipv4);
+  if (mapped !== null) {
+    return isBlockedIpv4(mapped);
   }
   return false;
 }