[knudtty]

Summary

Hardens the webhook logic by doing an actual lookup of the DNS record and determining whether to proceed/error.

References

• Linear Issue: Closes HDX-5966

[github-actions]

🔴 Tier 4 — Critical

Touches authentication, tenancy data models, the public API or shipped database config — or substantially changes the query rendering engine, background tasks, the OTel pipeline, image build, or release CI.

Why this tier:

• Security-critical files (1) — auth or input validation, escalated at any size:
  • packages/api/src/utils/validators.ts

Additional context: touches background tasks or the delivery pipeline lightly (11 lines, under the 30-line bar for Tier 4)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats

• Production files changed: 4
• Production lines changed: 67 (+ 165 in test files, excluded from tier calculation)
• Critical-path lines changed: 48
• Branch: aaron/webhook-harden-dns-check
• Author: knudtty

To override this classification, remove the review/tier-4 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

Duplicate code analysis

No new duplicate code introduced by this PR.

---

jscpd compared this branch against origin/main
across app, api and common-utils. Only new clones are listed; the codebase
currently holds 110 in total (1.21%).
Run yarn dupes locally to reproduce, or yarn dupes:all for the full picture.

[github-actions]

PR Review

✅ No issues found.

[github-actions]

Deep Review

✅ No critical issues found.

This SSRF-hardening change is well-constructed. The core defense — checking the resolved address at connect time via an undici Agent connect.lookup hook so the dialed address is the validated one — correctly closes the DNS-rebinding and public-name-to-private-IP gaps that validateWebhookUrl (hostname-only) misses. Independent verification confirmed: WebhookUrlValidationError.status = 400 is read by withRetry's status extraction so blocked URLs are not retried; the .catch cause-chain unwrap surfaces connect-time blocks as the validation error; NAT64 handling is internally consistent (64:ff9b::/96 recurses on the embedded IPv4, 64:ff9b:1::/48 is blanket-blocked); and both Generic and IncidentIO transports share the hardened dispatcher.

No P2-level findings. The items below are low-impact nits.

🔵 P3 nitpicks (3)

• packages/api/src/utils/validators.ts:204 — webhookLookup indexes addresses[0].address on the non-all branch with no empty-array guard, so a successful resolution to zero addresses would throw a TypeError inside the dns.lookup callback; dns.lookup with all:true returns ENOTFOUND instead of an empty array, so the path is effectively unreached today.
  • Fix: Handle the zero-address case explicitly (call back with an error) before dereferencing addresses[0].
  • _{correctness, testing, reliability}
• packages/api/src/utils/validators.ts:177 — PRIVATE_ADDRESS_MESSAGE is referenced inside validateWebhookUrl but declared lower in the file; it is initialized at module-eval time so this is not a temporal-dead-zone bug, only a backwards read.
  • Fix: Move the const PRIVATE_ADDRESS_MESSAGE declaration above its first use.
  • _{maintainability}
• packages/api/src/tasks/checkAlerts/transports/generic.ts:271 — the for (let c = e; c; c = c.cause) cause-chain walk has no cycle guard, so a self-referential error.cause chain would loop forever; real undici/Node errors are acyclic, so this is theoretical.
  • Fix: Bound the walk with a depth counter or a visited set.
  • _{correctness, reliability}

---

Reviewers (5): correctness, testing, maintainability, reliability, previous-comments.

Testing gaps:

• No test asserts the rethrow-original branch in generic.ts:274 preserves a non-WebhookUrlValidationError error's identity when the cause chain contains no validation error.
• No test exercises webhookLookup resolving to zero addresses on the all:false path.

Coverage note: The security, adversarial, and kieran-typescript sub-agents were dispatched but did not return before synthesis; the SSRF-critical paths (connect-time address check, retry/status behavior, NAT64 coverage, allowlist trust model) were verified directly against the diff and retry.ts instead. Residual (non-blocking) risks worth operator awareness: an allowlisted hostname short-circuits the per-address check for all its resolved addresses (intended opt-in); the module-level new Agent(...) is constructed at import time (total blast radius if it ever throws); and addresses.some(isPrivateIp) is fail-closed, so a transient poisoned DNS record can non-retryably drop a legitimate alert dispatch.

_{No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond}

[github-actions]

E2E Test Results

✅ All tests passed • 461 passed • 1 skipped • 1189s

Status	Count
✅ Passed	461
❌ Failed	0
⚠️ Flaky	3
⏭️ Skipped	1

Tests ran across 4 shards in parallel.

View full report →

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
clickstack-oss	Ready	Preview	Nov 6, 2026 10:21pm UTC
clickstack-storybook	Ready	Preview	Nov 6, 2026 10:21pm UTC