[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

If you are a coding agent acting for the author, read this first.
Nothing below blocks merge automatically; a maintainer decides. What they will expect:

• 🔴 critical or 🟠 major in code this PR adds or changes: fix before asking for review.
• 🔵 minor in code this PR changes: your call. Fix if small, otherwise reply.
• Anything about surrounding code, or asking you to widen the change (hoist a helper,
  dedupe with another file, fix other call sites): never fix here, whatever the
  severity. Reply with one sentence; if it is critical, say so plainly so a human sees it.

One commit per review round. After two rounds, stop and ask a maintainer to review
scope rather than addressing more automated findings. Full rule: AGENTS.md.

2 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 2 minor

1 posted as inline comment(s) on the changed lines. 1 listed below.

Findings outside the changed lines

1 minor

• 🔵 packages/api/src/utils/__tests__/validators.test.ts:298 — Allowlisting by resolved IP address is never tested → webhookLookup has a second allowlist branch, !isAllowlistedWebhookHostname(address), which lets an allowlisted IP (e.g. WEBHOOK_HOSTNAME_ALLOWLIST=10.0.0.5) pass for any hostname that resolves to it. The only allowlist test covers the hostname branch with localhost, so a regression that drops or inverts the IP branch would still pass. Fix: add a case that sets hostnameAllowlist: '10.0.0.5' and mocks dns.lookup to return [{address:'10.0.0.5',family:4}] for internal.example, and expect it to resolve. Also mock a mix of 10.0.0.5 and 10.0.0.6 and expect it to be blocked.

---

_{Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides.}

[github-actions]

Deep Review

✅ No critical issues found. The SSRF/DNS-rebinding hardening is sound: webhookLookup checks every resolved address on the connection actually dialed, WebhookUrlValidationError.status = 400 correctly stops withRetry from retrying a blocked URL, redirects remain blocked via redirect: 'manual', and both generic and incident.io transports route through the hardened dispatcher. The items below are recommended follow-ups, not merge blockers.

🟡 P2 — recommended

• packages/api/src/tasks/checkAlerts/transports/generic.ts:271 — the for (let c = e; c; c = c.cause) cause-chain walk has no depth cap or visited-set, so a self-referential or cyclic cause would spin forever and wedge the background alert worker, diverging from the repo's own guarded walk in errors.ts (MAX_CAUSE_DEPTH + seen).
  • Fix: Bound the walk with a depth limit or a visited-set, mirroring the existing errors.ts cause-chain guard.
  • _{correctness, adversarial, reliability, maintainability, kieran-typescript}
• packages/api/src/tasks/checkAlerts/__tests__/checkAlerts.int.test.ts:5040 — dispatcher: expect.anything() passes for any truthy dispatcher, so a regression that drops connect.lookup (the exact thing this change guards against) would not fail, and the global undici.fetch mock in jest.setup.ts means only one test drives the real Agent→webhookLookup connect path.
  • Fix: Export webhookDispatcher and assert the fetch call receives it (or assert the dispatcher carries the hardened connect.lookup).
  • _{testing, maintainability, adversarial}
• packages/api/src/utils/validators.ts:195 — the per-resolved-address allowlist branch isPrivateIp(address) && !isAllowlistedWebhookHostname(address) is never exercised, because the only allowlist test allowlists the hostname and short-circuits before the address check.
  • Fix: Add a case where the hostname is not allowlisted but resolves to an allowlisted private IP literal, and assert it resolves.
  • _testing

🔵 P3 nitpicks (6)

• packages/api/src/utils/validators.ts:59 — isPrivateIp extracts the embedded IPv4 only for mapped, compatible, and NAT64 (64:ff9b::/96, 64:ff9b:1::/48) forms, so 6to4 (2002::/16), Teredo (2001::/32), and operator NAT64 prefixes embedding a private IPv4 fall through unblocked (reachable only on hosts with such tunneling configured).
  • Fix: Decode and re-check the embedded IPv4 for 6to4 and Teredo prefixes, and consider blocking the broader NAT64 space.
• packages/api/src/utils/validators.ts:189 — dns.lookup (getaddrinfo) is not cancelable by the fetch AbortSignal, so a hung resolver blocks a libuv threadpool thread; under concurrent deliveries this can starve the shared threadpool used by other getaddrinfo calls (largely a pre-existing property of resolution, now explicit here).
  • Fix: Bound the resolution phase with its own timeout, or use the cancelable c-ares dns.resolve.
• packages/api/src/utils/validators.ts:204 — the options.all === false branch dereferences addresses[0].address with no empty-array guard, throwing a TypeError if dns.lookup ever calls back (null, []); this branch is also untested.
  • Fix: Guard the empty case (return an ENODATA-style error) and add a mockLookup(null, []) test.
  • _{correctness, testing}
• packages/api/src/utils/validators.ts:192 — webhookLookup checks the allowlist against both the hostname and each resolved IP using the hostname-named predicate, which obscures that allowlist entries may be hostnames or IP literals.
  • Fix: Add a one-line comment explaining the per-address allowlist check permits a specific private IP literal.
• packages/api/src/utils/__tests__/validators.test.ts:400 — resolves.toEqual(expect.any(String)) proves only that some string was returned, not that the allowlisted private address passed through.
  • Fix: Mock a known private IP and assert the exact returned value (or assert isPrivateIp(result)).
• packages/api/src/utils/__tests__/validators.test.ts:337 — the lookup() test helper drops the third family callback argument, so the all=false single-return shape that undici's connector relies on is never asserted.
  • Fix: Capture and assert the family value in the single-address path.

---

Reviewers (9): correctness, security, adversarial, reliability, testing, maintainability, kieran-typescript, project-standards, previous-comments.

Testing gaps:

• The real Agent→webhookLookup connect path is exercised end-to-end by only one test (loopback block); the global undici.fetch mock in jest.setup.ts bypasses the hardened dispatcher for every other webhook test, so the NAT64/IPv4-mapped/mixed-record block paths are proven only in webhookLookup isolation.
• No test asserts a blocked URL results in exactly one delivery attempt (retry suppression) through undici's nested cause chain at the integration level.

_{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 • 465 passed • 1 skipped • 1428s

Status	Count
✅ Passed	465
❌ Failed	0
⚠️ Flaky	1
⏭️ 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 9, 2026 2:34pm UTC
clickstack-storybook	Ready	Preview	Nov 9, 2026 2:34pm UTC
hyperdx-oss	Ready	Preview	Nov 9, 2026 2:34pm UTC
hyperdx-storybook	Ready	Preview	Nov 9, 2026 2:34pm UTC

[github-actions]

🔵 minor — Save and /validate still accept hostnames that every delivery will now reject

The new resolved-address check runs only at send time, through the undici dispatcher in generic.ts. WEBHOOK_CONFIG_CHECKS (validators.ts:219), which create and POST /api/v2/webhooks/validate both use, checks only the hostname string. So a URL like http://127-0-0-1.nip.io/ or an internal name that isn't allowlisted saves without error, then every alert firing fails and the problem shows up only in logs and alert history. This also breaks the invariant stated in the comment above that list. Fix: add an async resolve-and-isPrivateIp check (the same logic as webhookLookup) to the create/validate path, so these configs are rejected with a 400 when saved. Keep the connect-time check as the rebinding guard.

_{Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond}