fix(node): guard webhook delivery against SSRF targets - #490
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughWebhook URLs now undergo SSRF checks during registration and before delivery attempts. Unsafe destinations and redirects stop delivery without retries and are recorded as ChangesWebhook SSRF protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WebhookManager
participant Deliverer
participant URLSafety
participant WebhookReceiver
participant DeliveryLog
WebhookManager->>Deliverer: provide registered webhook
Deliverer->>URLSafety: revalidate URL before attempt
URLSafety-->>Deliverer: safe URL or refusal error
Deliverer->>WebhookReceiver: send webhook request
WebhookReceiver-->>Deliverer: HTTP response
Deliverer->>DeliveryLog: record delivered, failed, or dead status
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
6e57eaf to
695cf57
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/docs/node/guides/extensibility/webhooks.mdx (1)
69-69: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument errors for refused deliveries.
Blocked URLs and redirects produce
status: "failed"with an error message, not only"dead"deliveries.Proposed fix
-| `error` | The last error message, if `status` is `"dead"`. | +| `error` | The last error message, if `status` is not `"delivered"`. |🤖 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/extensibility/webhooks.mdx` at line 69, Update the webhook delivery table’s `error` field description to state that it contains the last error message for failed deliveries, including refused blocked URLs and redirects, as well as dead deliveries; do not limit the description to status "dead".
🧹 Nitpick comments (1)
sdks/node/test/core/webhooks.test.ts (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the process-wide SSRF bypass after these suites. Both assignments persist for the worker process and can mask default-guard behavior in later tests when workers are reused or isolation is disabled.
sdks/node/test/core/webhooks.test.ts#L10-L12: snapshotTASKITO_WEBHOOKS_ALLOW_PRIVATEand restore/delete it in suite teardown.sdks/node/test/dashboard/webhookDeliveries.test.ts#L13-L15: apply the same scoped setup and teardown.🤖 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/test/core/webhooks.test.ts` around lines 10 - 12, Scope the TASKITO_WEBHOOKS_ALLOW_PRIVATE override to each test suite by snapshotting its initial value during setup and restoring it, or deleting it when originally unset, during teardown. Apply this in sdks/node/test/core/webhooks.test.ts lines 10-12 and sdks/node/test/dashboard/webhookDeliveries.test.ts lines 13-15; both sites require the same setup and teardown behavior.
🤖 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 `@docs/content/docs/node/guides/extensibility/webhooks.mdx`:
- Around line 41-45: Update the webhook SSRF guard description to distinguish
registration-time synchronous hostname/literal-IP checks from delivery-time DNS
validation. State that named hosts resolving to loopback, private, link-local,
or cloud-metadata addresses are rejected before delivery, while preserving the
existing redirect and development override details.
In `@sdks/node/src/webhooks/urlSafety.ts`:
- Around line 104-112: Update the webhook URL safety flow around lookup and
assertAddressAllowed in urlSafety.ts to return the vetted address and address
family instead of discarding DNS results, then update deliverer.ts at the fetch
call to use a custom dispatcher/agent that connects to that approved address
while preserving the original hostname for Host and TLS SNI. Ensure every
resolved address remains validated before selecting the pinned endpoint.
---
Outside diff comments:
In `@docs/content/docs/node/guides/extensibility/webhooks.mdx`:
- Line 69: Update the webhook delivery table’s `error` field description to
state that it contains the last error message for failed deliveries, including
refused blocked URLs and redirects, as well as dead deliveries; do not limit the
description to status "dead".
---
Nitpick comments:
In `@sdks/node/test/core/webhooks.test.ts`:
- Around line 10-12: Scope the TASKITO_WEBHOOKS_ALLOW_PRIVATE override to each
test suite by snapshotting its initial value during setup and restoring it, or
deleting it when originally unset, during teardown. Apply this in
sdks/node/test/core/webhooks.test.ts lines 10-12 and
sdks/node/test/dashboard/webhookDeliveries.test.ts lines 13-15; both sites
require the same setup and teardown behavior.
🪄 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: 049faef6-f2e4-462c-b04d-ae691c1bccb2
📒 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
| let resolved: Array<{ address: string }>; | ||
| try { | ||
| resolved = await lookup(hostname, { all: true, verbatim: true }); | ||
| } catch { | ||
| throw new UnsafeWebhookUrlError(`could not resolve webhook host ${hostname}`); | ||
| } | ||
| for (const { address } of resolved) { | ||
| assertAddressAllowed(address, hostname); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File map ==\n'
git ls-files 'sdks/node/src/webhooks/*' 'sdks/node/test/**/*webhook*' 'sdks/node/test/**/*webhooks*'
printf '\n== Outline urlSafety.ts ==\n'
ast-grep outline sdks/node/src/webhooks/urlSafety.ts --view expanded || true
printf '\n== Outline deliverer.ts ==\n'
ast-grep outline sdks/node/src/webhooks/deliverer.ts --view expanded || true
printf '\n== Relevant lines urlSafety.ts ==\n'
sed -n '1,220p' sdks/node/src/webhooks/urlSafety.ts | cat -n
printf '\n== Relevant lines deliverer.ts ==\n'
sed -n '1,220p' sdks/node/src/webhooks/deliverer.ts | cat -n
printf '\n== Search for URL safety helpers/usages ==\n'
rg -n "assertAddressAllowed|lookup\\(|UnsafeWebhookUrlError|fetch\\(webhook.url|redirect: \"manual\"|dispatcher|agent|undici|connect" sdks/node/src/webhooks sdks/node/test || trueRepository: ByteVeda/taskito
Length of output: 21586
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' sdks/node/src/webhooks/urlSafety.ts | cat -n
printf '\n---\n'
sed -n '1,220p' sdks/node/src/webhooks/deliverer.ts | cat -nRepository: ByteVeda/taskito
Length of output: 15353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path('sdks/node/src/webhooks/urlSafety.ts'), Path('sdks/node/src/webhooks/deliverer.ts')]:
print(f'===== {p} =====')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 1 <= i <= 220:
print(f'{i:4d}: {line}')
PYRepository: ByteVeda/taskito
Length of output: 15073
Bind webhook delivery to the vetted DNS answer. lookup() checks the host, then discards the result; fetch(webhook.url) can resolve it again and be steered to a different IP on connect. Return the approved address/family and pin the request to it with a custom dispatcher/agent while preserving Host/SNI.
📍 Affects 2 files
sdks/node/src/webhooks/urlSafety.ts#L104-L112(this comment)sdks/node/src/webhooks/deliverer.ts#L58-L65
🤖 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 - 112, Update the
webhook URL safety flow around lookup and assertAddressAllowed in urlSafety.ts
to return the vetted address and address family instead of discarding DNS
results, then update deliverer.ts at the fetch call to use a custom
dispatcher/agent that connects to that approved address while preserving the
original hostname for Host and TLS SNI. Ensure every resolved address remains
validated before selecting the pinned endpoint.
695cf57 to
8a35aab
Compare
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.
8a35aab to
d76396b
Compare
|
Superseded by #491, same commits from the fork. |
1 similar comment
|
Superseded by #491, same commits from the fork. |
2 participants
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
New Features
TASKITO_WEBHOOKS_ALLOW_PRIVATE=1for local development.Bug Fixes