fix(node): guard webhook delivery against SSRF targets - #491
Merged
pratyush618 merged 5 commits intoJul 21, 2026
Conversation
Validates the destination at registration and before every delivery attempt, and stops following redirects, so a rebound name or a 302 cannot reach loopback, private, or metadata addresses.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughWebhook URLs now undergo SSRF validation at registration and before delivery attempts. Unsafe destinations and redirects are not retried, delivery status includes ChangesWebhook SSRF protection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Deliverer
participant URLSafety
participant WebhookEndpoint
Deliverer->>URLSafety: Validate destination before each attempt
URLSafety-->>Deliverer: Allow or reject unsafe URL
Deliverer->>WebhookEndpoint: POST with redirects disabled
WebhookEndpoint-->>Deliverer: Response or 3xx redirect
Deliverer-->>WebhookEndpoint: Record failed or retry outcome
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/content/docs/node/guides/operations/security.mdx (1)
44-60: 🔒 Security & Privacy | 🔵 TrivialEliminate the resolve-to-connect race using a custom dispatcher.
The documentation correctly identifies the residual DNS rebinding race condition between URL validation and the socket connection. If you wish to completely eliminate this TOCTOU in Node.js in the future, you can supply a custom
undici.AgentorDispatchertofetch. By overriding theconnectorlookupfunction, you can validate the exact IP address immediately before the socket connects (or force the socket to use the already-validated IP fromassertSafeWebhookUrl).This eliminates the need for a secondary network egress allowlist just for the webhook worker.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/content/docs/node/guides/operations/security.mdx` around lines 44 - 60, Update the webhook delivery documentation to describe eliminating the resolve-to-connect race with a custom undici.Agent or Dispatcher supplied to fetch, using its connect or lookup hook to validate or reuse the IP resolved by assertSafeWebhookUrl immediately before socket connection. Remove the statement that a separate egress allowlist is required solely for this race, while retaining the strict delivery timeout guidance.sdks/node/src/webhooks/urlSafety.ts (1)
104-109: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on the DNS lookup used for per-attempt validation.
lookup(hostname, { all: true, verbatim: true })has no bound; an attacker-controlled or unresponsive resolver for the webhook host can stall this await well beyond typical resolver defaults, delaying (or effectively blocking) each delivery attempt before thefetch()timeout even gets a chance to apply.♻️ Suggested fix: bound the resolution
- let resolved: Array<{ address: string }>; - try { - resolved = await lookup(hostname, { all: true, verbatim: true }); - } catch { - throw new UnsafeWebhookUrlError(`could not resolve webhook host ${hostname}`); - } + let resolved: Array<{ address: string }>; + try { + resolved = await withTimeout(lookup(hostname, { all: true, verbatim: true }), DNS_TIMEOUT_MS); + } catch { + throw new UnsafeWebhookUrlError(`could not resolve webhook host ${hostname}`); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/node/src/webhooks/urlSafety.ts` around lines 104 - 109, Bound the DNS resolution await in the webhook URL validation flow around lookup by applying the existing per-attempt timeout or an appropriate resolver-specific timeout. Ensure stalled lookups reject within that bound and continue to produce UnsafeWebhookUrlError, while preserving the current resolved-address handling for successful lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdks/node/src/webhooks/deliverer.ts`:
- Around line 72-76: Update the redirect-handling condition in the webhook
delivery flow to also detect opaque manual redirects by checking response.type
=== "opaqueredirect" or response.status === 0. Preserve the existing
nonRetryable behavior and redirect error handling for both standard 3xx
responses and opaque redirects.
In `@sdks/node/src/webhooks/urlSafety.ts`:
- Around line 82-113: Update the delivery transport flow using
assertSafeWebhookUrl so it connects to the validated resolved address rather
than allowing fetch(webhook.url, ...) to perform a second hostname lookup.
Propagate the selected validated address and preserve the original hostname for
HTTP Host/TLS SNI handling, ensuring private rebinding targets cannot be reached
between validation and connection.
---
Nitpick comments:
In `@docs/content/docs/node/guides/operations/security.mdx`:
- Around line 44-60: Update the webhook delivery documentation to describe
eliminating the resolve-to-connect race with a custom undici.Agent or Dispatcher
supplied to fetch, using its connect or lookup hook to validate or reuse the IP
resolved by assertSafeWebhookUrl immediately before socket connection. Remove
the statement that a separate egress allowlist is required solely for this race,
while retaining the strict delivery timeout guidance.
In `@sdks/node/src/webhooks/urlSafety.ts`:
- Around line 104-109: Bound the DNS resolution await in the webhook URL
validation flow around lookup by applying the existing per-attempt timeout or an
appropriate resolver-specific timeout. Ensure stalled lookups reject within that
bound and continue to produce UnsafeWebhookUrlError, while preserving the
current resolved-address handling for successful lookups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2da8a985-2f91-42f9-8e5c-2ffcc13e9a3a
📒 Files selected for processing (11)
docs/content/docs/node/guides/extensibility/webhooks.mdxdocs/content/docs/node/guides/operations/security.mdxsdks/node/src/index.tssdks/node/src/webhooks/deliverer.tssdks/node/src/webhooks/errors.tssdks/node/src/webhooks/index.tssdks/node/src/webhooks/manager.tssdks/node/src/webhooks/urlSafety.tssdks/node/test/core/webhookUrlSafety.test.tssdks/node/test/core/webhooks.test.tssdks/node/test/dashboard/webhookDeliveries.test.ts
Delivery now goes through node:http(s) with a validating lookup, so the socket dials the address the guard checked and a 3xx never reads as a success on runtimes that return an opaque redirect.
Collaborator
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdks/node/src/webhooks/urlSafety.ts`:
- Around line 93-113: Update assertSafeWebhookUrl and the preflight handling in
deliver to preserve retryability for transient DNS failures: only convert
explicit policy refusals into UnsafeWebhookUrlError, let lookup failures
propagate as ordinary errors, and handle those plain errors in deliver as
retryable attempts with backoff while retaining the existing non-retryable path
for UnsafeWebhookUrlError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c65d473-e5cf-4aae-8f54-302a673b97d3
📒 Files selected for processing (11)
docs/content/docs/node/guides/extensibility/webhooks.mdxdocs/content/docs/node/guides/operations/security.mdxsdks/node/src/index.tssdks/node/src/webhooks/deliverer.tssdks/node/src/webhooks/errors.tssdks/node/src/webhooks/index.tssdks/node/src/webhooks/manager.tssdks/node/src/webhooks/urlSafety.tssdks/node/test/core/webhookUrlSafety.test.tssdks/node/test/core/webhooks.test.tssdks/node/test/dashboard/webhookDeliveries.test.ts
The pre-attempt check resolved DNS and reported every failure as an SSRF refusal, so one transient resolver hiccup killed the delivery with zero attempts. The connect-time lookup already decides addresses, and cannot be raced, so the pre-attempt check now rules only on the URL itself.
A request timeout only starts once a socket is assigned, so an unresponsive resolver stalled delivery indefinitely. Resolution now shares the subscription's timeout budget.
pratyush618
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #481.
Webhook URLs registered through the dashboard or the API were fetched as-is, so anyone able to write a subscription could point the worker at
http://169.254.169.254/..., a loopback admin port, or the RFC1918 intranet.What changed
src/webhooks/urlSafety.ts(new) —assertSafeWebhookUrl(resolves the host) andassertSafeWebhookUrlSync(no DNS). Rejects non-http(s)schemes, hostless URLs, local-only names (localhost,*.internal,*.local, …), and addresses in loopback / unspecified / RFC1918 / CGNAT / link-local / multicast / reserved / IPv6 unique-local ranges. IPv4-mapped IPv6 is unwrapped and judged as its v4 address.WebhookManagerruns the DNS-free checks on create/update, so a literal private target is rejected with a 400 (UnsafeWebhookUrlErrorextendsWebhookValidationError).Delivererre-validates before every attempt, closing the registration-to-delivery DNS-rebinding window, and now usesredirect: "manual"so a3xxcannot walk past the guard. A refused delivery is recorded asfailedand is not retried.TASKITO_WEBHOOKS_ALLOW_PRIVATE(truthy) disables the address checks for local development, matching the cross-SDK contract. Scheme and host are still validated.Tests
test/core/webhookUrlSafety.test.tscovers the blocked ranges and mapped IPv6 forms, local-only names, scheme/hostless rejection under bypass, falsy env values, registration refusal, a delivery blocked after the target becomes unsafe, and a refused redirect. Existing loopback-targeting webhook tests opt into the bypass.Node suite: 72 files / 449 tests green; tsc, biome, and the docs typecheck/lint all pass.
Summary by CodeRabbit
TASKITO_WEBHOOKS_ALLOW_PRIVATE=1) for local testing.