fix(webhooks): block private DNS targets#2200
Conversation
📝 WalkthroughWalkthroughWebhook URL validation now performs async DNS A/AAAA resolution and private-IP checks. A new async validator is added and awaited across webhook create/update/test/retry handlers, the dispatcher, delivery path, and website preview; tests exercise DNS outcomes and redirect behavior. ChangesAsync DNS-Based Webhook URL Validation
Sequence DiagramsequenceDiagram
participant Handler as webhook handler
participant Validator as getWebhookUrlValidationErrorAsync
participant DNS as resolveHostnameIps (DoH)
participant Classifier as isPrivateIp
participant Queue as queueWebhookDelivery
Handler->>Validator: validate URL (await)
Validator->>DNS: request A and AAAA records
DNS-->>Validator: list of IPs
Validator->>Classifier: check each IP
Classifier-->>Validator: private/public verdict
Validator-->>Handler: validation result (null or error)
Handler->>Queue: queue delivery (when valid)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Merging this PR will not alter performance
Comparing 
Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b767ba9098
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const response = await fetch(dnsUrl.toString(), { | ||
| headers: { Accept: 'application/dns-json' }, | ||
| }) |
There was a problem hiding this comment.
Wrap the DNS-over-HTTPS fetch in resolveHostnameIps with error handling (and ideally return a validation error string) instead of letting rejections bubble. As written, any transient network/DNS failure to cloudflare-dns.com throws out of getWebhookUrlValidationErrorAsync, which turns webhook create/update/test/retry into 500s and can leave queued deliveries stuck in pending when dispatcher/delivery flows abort before updateDeliveryResult runs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
supabase/functions/_backend/utils/webhook.ts (1)
157-160: 💤 Low valueResolve A and AAAA concurrently.
The two DoH lookups are awaited sequentially, doubling the latency on every validation call (and validation runs on webhook create/update/test, every dispatch, and every delivery). They're independent, so
Promise.allhalves the added latency.♻️ Suggested change
- const ips = [ - ...await resolveHostnameIps(hostname, 'A'), - ...await resolveHostnameIps(hostname, 'AAAA'), - ] + const [aRecords, aaaaRecords] = await Promise.all([ + resolveHostnameIps(hostname, 'A'), + resolveHostnameIps(hostname, 'AAAA'), + ]) + const ips = [...aRecords, ...aaaaRecords]🤖 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 `@supabase/functions/_backend/utils/webhook.ts` around lines 157 - 160, The current code builds ips by awaiting resolveHostnameIps(hostname, 'A') and then resolveHostnameIps(hostname, 'AAAA') sequentially; change this to run both DNS-over-HTTPS lookups concurrently with Promise.all (e.g., call both resolveHostnameIps promises in an array and await Promise.all), then flatten/concat the two results into the ips array so resolveHostnameIps and the ips const remain the same but with parallel resolution to halve latency.tests/webhook-url-validation.test.ts (1)
21-49: 💤 Low valueConsider adding a few high-value negative cases.
The three cases here exercise the happy paths well. Two cheap additions would meaningfully increase confidence in the new validator:
- DNS resolution failure → ensure
'Webhook URL host could not be resolved'is returned (mockfetchto throw or return!response.ok). This also locks in the fix for unhandled DNS fetch errors.- Mixed A + AAAA where AAAA is private → ensure
some(isPrivateIp)still rejects (catches regressions if a future change accidentally short-circuits on a public A record).🤖 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 `@tests/webhook-url-validation.test.ts` around lines 21 - 49, Add two negative tests for webhook URL validation: (1) simulate DNS/fetch failure for getWebhookUrlValidationErrorAsync (mock fetch to throw or return a non-ok response) and assert it resolves to 'Webhook URL host could not be resolved' to cover unhandled DNS fetch errors; (2) add a test where mockDnsAnswers returns mixed records (e.g., an A public address plus an AAAA private address) and assert getWebhookUrlValidationErrorAsync still rejects with 'Webhook URL must point to a public host' to ensure some(isPrivateIp) logic doesn't short-circuit on the public A record; reference getWebhookUrlValidationErrorAsync, getWebhookUrlValidationError and mockDnsAnswers when adding these cases.supabase/functions/_backend/triggers/webhook_dispatcher.ts (1)
80-146: Dispatcher integration looks correct.Per-webhook validation runs inside the
Promise.all(map(...))so multiple webhooks for the same event still validate in parallel; on failure the delivery row is marked failed with the validation message and the queue is bypassed cleanly. Note that the same hostname will now be DoH-resolved once here and again insidedeliverWebhook— that's intentional defense-in-depth, but if you see DNS resolution become a hot path, a short-lived in-process cache (e.g., 30–60s LRU keyed by hostname) would dedupe both layers.🤖 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 `@supabase/functions/_backend/triggers/webhook_dispatcher.ts` around lines 80 - 146, Per the review, validation and delivery both perform DoH resolution causing duplicate lookups; add a small in-process LRU cache (30–60s TTL, keyed by hostname) and consult it in the DNS resolution path used by getWebhookUrlValidationErrorAsync and deliverWebhook so both functions check the cache before performing DoH resolution, falling back to actual DoH and populating the cache on miss; keep cache lifecycle short-lived and shared module-scoped so the concurrent Promise.all in webhook_dispatcher.ts benefits without changing the existing queueWebhookDelivery or delivery flow.
🤖 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 `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 147-168: Add an explicit inline comment in
getWebhookUrlValidationErrorAsync explaining the DNS-rebinding window between
resolving the hostname via resolveHostnameIps and the later network call in
deliverWebhook (i.e., fetch does its own DNS resolution), reference the
functions resolveHostnameIps, isPrivateIp and deliverWebhook, and state this
residual SSRF risk is not fully mitigated by this check; additionally, either
(1) move or repeat the same hostname/IP validation immediately inside
deliverWebhook just before its fetch call, or (2) note that outbound requests
should be enforced via an egress proxy/WAF that blocks private IPs — choose one
mitigation and document it in the code.
- Around line 68-85: The isPrivateIpv4 function needs to also treat IPv4
multicast/reserved/broadcast ranges as private; update the return expression in
isPrivateIpv4 (which uses octets and [a,b]) to additionally return true when the
first octet a is between 224 and 239 (multicast), when a is between 240 and 254
(reserved), and when the full address equals 255.255.255.255 (limited broadcast)
— you can check the exact broadcast by comparing octets to [255,255,255,255] or
join the parts, and keep these checks alongside the existing
RFC1918/loopback/link-local conditions.
- Around line 102-117: The resolveHostnameIps function lacks a timeout and error
handling around the fetch to DNS_LOOKUP_URL; wrap the fetch in a try/catch and
use an AbortController with a short timeout (e.g. few hundred ms) so stalled DoH
requests abort, and if any error/timeout occurs return an empty array (so
callers treat the host as unresolved). In practice, create an AbortController,
set a timer to call controller.abort(), pass controller.signal to
fetch(dnsUrl.toString(), { headers: {...}, signal }), await the response inside
try, fallback to returning [] on non-ok responses or any thrown error, and keep
the existing mapping/filtering logic (isIpLiteral) for successful responses.
- Around line 87-100: The IPv6 link-local check in isPrivateIpv6 is too narrow
(only matches 'fe80:'); replace the startsWith('fe80:') branch with a regex test
that matches the full fe80::/10 range (use the suggested /^fe[89ab][0-9a-f]?:/i)
so addresses from fe80:: through febf:: are detected as private; keep the
existing ::1/:: and fc/fd (ULA) checks, and ensure isPrivateIp continues to
dispatch to isPrivateIpv6 for colon-containing addresses.
---
Nitpick comments:
In `@supabase/functions/_backend/triggers/webhook_dispatcher.ts`:
- Around line 80-146: Per the review, validation and delivery both perform DoH
resolution causing duplicate lookups; add a small in-process LRU cache (30–60s
TTL, keyed by hostname) and consult it in the DNS resolution path used by
getWebhookUrlValidationErrorAsync and deliverWebhook so both functions check the
cache before performing DoH resolution, falling back to actual DoH and
populating the cache on miss; keep cache lifecycle short-lived and shared
module-scoped so the concurrent Promise.all in webhook_dispatcher.ts benefits
without changing the existing queueWebhookDelivery or delivery flow.
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 157-160: The current code builds ips by awaiting
resolveHostnameIps(hostname, 'A') and then resolveHostnameIps(hostname, 'AAAA')
sequentially; change this to run both DNS-over-HTTPS lookups concurrently with
Promise.all (e.g., call both resolveHostnameIps promises in an array and await
Promise.all), then flatten/concat the two results into the ips array so
resolveHostnameIps and the ips const remain the same but with parallel
resolution to halve latency.
In `@tests/webhook-url-validation.test.ts`:
- Around line 21-49: Add two negative tests for webhook URL validation: (1)
simulate DNS/fetch failure for getWebhookUrlValidationErrorAsync (mock fetch to
throw or return a non-ok response) and assert it resolves to 'Webhook URL host
could not be resolved' to cover unhandled DNS fetch errors; (2) add a test where
mockDnsAnswers returns mixed records (e.g., an A public address plus an AAAA
private address) and assert getWebhookUrlValidationErrorAsync still rejects with
'Webhook URL must point to a public host' to ensure some(isPrivateIp) logic
doesn't short-circuit on the public A record; reference
getWebhookUrlValidationErrorAsync, getWebhookUrlValidationError and
mockDnsAnswers when adding these cases.
🪄 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: 397eafec-8e35-43c1-929b-c4db52d34026
📒 Files selected for processing (7)
supabase/functions/_backend/public/webhooks/deliveries.tssupabase/functions/_backend/public/webhooks/post.tssupabase/functions/_backend/public/webhooks/put.tssupabase/functions/_backend/public/webhooks/test.tssupabase/functions/_backend/triggers/webhook_dispatcher.tssupabase/functions/_backend/utils/webhook.tstests/webhook-url-validation.test.ts
|
Nice direction — closing the DNS-resolution gap in webhook validation is real protection. Few things I'd push back on before this merges, in roughly decreasing severity. 1. TOCTOU / DNS-rebinding bypass (highest impact)
Two ways to close it:
PR #2106 (Harden website preview DNS IP validation) explicitly acknowledges the same residual risk in its body, so it's worth coordinating with that PR on a shared fix rather than punting it twice. 2.
|
|
Review note: this still allows an SSRF bypass through redirects.
A safer shape is to set |
|
Thanks for the detailed reviews. I pushed a follow-up commit (
Verification run locally: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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)
tests/webhook-url-validation.test.ts (1)
1-159:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd test coverage for IPv6 special-use ranges with abbreviated forms, and use
it.concurrent()for parallel execution.The current webhook validation (ip.ts lines 85-89) has a real vulnerability: string prefix matching fails for IPv6 addresses with leading zeros. Addresses like
0064:ff9b::1234:5678and0100::bypass the private IP checks because they don't match the string prefixes64:ff9b:and100::. This allows webhook URLs to resolve to reserved IPv6 ranges.Add these test cases to expose and verify fixes for the vulnerability:
🧪 Suggested test cases
it.concurrent('blocks IPv6 discard-only prefix 100::/64 in abbreviated forms', async () => { mockDnsAnswers(['100::1', '0100::']) await expect( getWebhookUrlValidationErrorAsync(context, 'https://discard.example.com/webhook'), ) .resolves .toBe('Webhook URL must point to a public host') }) it.concurrent('blocks IPv6 NAT64 prefix 64:ff9b::/96 with leading zeros', async () => { mockDnsAnswers(['64:ff9b::1234:5678', '0064:ff9b::8888:8888']) await expect( getWebhookUrlValidationErrorAsync(context, 'https://nat64.example.com/webhook'), ) .resolves .toBe('Webhook URL must point to a public host') }) it.concurrent('blocks IPv6 documentation prefix 2001:db8::/32 with leading zeros', async () => { mockDnsAnswers(['2001:db8::1', '2001:0db8::']) await expect( getWebhookUrlValidationErrorAsync(context, 'https://docs.example.com/webhook'), ) .resolves .toBe('Webhook URL must point to a public host') }) it.concurrent('blocks IPv6 multicast addresses ff00::/8', async () => { mockDnsAnswers(['ff02::1', 'ff00::']) await expect( getWebhookUrlValidationErrorAsync(context, 'https://multicast.example.com/webhook'), ) .resolves .toBe('Webhook URL must point to a public host') })Also update all existing tests to use
it.concurrent()instead ofit()per guidelines for parallel test execution.🤖 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 `@tests/webhook-url-validation.test.ts` around lines 1 - 159, Update the webhook URL validation tests to cover IPv6 special-use ranges with abbreviated/zero-padded forms and run tests in parallel: replace all occurrences of it(...) with it.concurrent(...) in tests/webhook-url-validation.test.ts and add the four new test cases that call getWebhookUrlValidationErrorAsync(context, ...) using mockDnsAnswers(...) for the following scenarios — discard-only prefix (100::/64) with values like "100::1" and "0100::"; NAT64 prefix (64:ff9b::/96) including "64:ff9b::1234:5678" and "0064:ff9b::8888:8888"; documentation prefix (2001:db8::/32) with "2001:db8::1" and "2001:0db8::"; and multicast ff00::/8 with "ff02::1" and "ff00::"; ensure each new test asserts getWebhookUrlValidationErrorAsync resolves to 'Webhook URL must point to a public host'.
🤖 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 `@supabase/functions/_backend/utils/ip.ts`:
- Around line 85-89: The current string-prefix checks against normalized
('100::' and '64:ff9b::') miss valid abbreviated IPv6 forms; replace them with
numeric prefix checks by parsing the IPv6 address into hextets (e.g., split into
an array of up to 8 16-bit words) and then test the prefix bits: for 100::/64
verify the first four hextets match [0x0100, 0x0000, 0x0000, 0x0000] (or
equivalently check first 64 bits), and for 64:ff9b::/96 verify the first six
hextets match [0x0064, 0xff9b, 0x0000, 0x0000, 0x0000, 0x0000] (or check first
96 bits); update the logic that currently checks normalized.startsWith('100::')
/ normalized.startsWith('64:ff9b:') to use this hextet/prefix comparison
(referencing the normalized variable and the IPv6 parsing helper you already use
or add one) so abbreviated forms are correctly detected.
- Line 89: The check using normalized.startsWith('2001:db8:') fails for
abbreviated forms like 2001:0db8::; update the same IPv6 prefix logic used for
the other reserved ranges to compare the first 32 bits (first four hextets) of
the normalized address instead of a raw startsWith string. In practice, in the
function where the variable normalized is tested (the same place that checks
other IPv6 prefixes), normalize each hextet by removing leading zeros or use the
existing normalization helper, then compare the first four hextets (or use a
regex that matches the 2001:db8::/32 pattern robustly) rather than relying on
normalized.startsWith('2001:db8:').
---
Outside diff comments:
In `@tests/webhook-url-validation.test.ts`:
- Around line 1-159: Update the webhook URL validation tests to cover IPv6
special-use ranges with abbreviated/zero-padded forms and run tests in parallel:
replace all occurrences of it(...) with it.concurrent(...) in
tests/webhook-url-validation.test.ts and add the four new test cases that call
getWebhookUrlValidationErrorAsync(context, ...) using mockDnsAnswers(...) for
the following scenarios — discard-only prefix (100::/64) with values like
"100::1" and "0100::"; NAT64 prefix (64:ff9b::/96) including
"64:ff9b::1234:5678" and "0064:ff9b::8888:8888"; documentation prefix
(2001:db8::/32) with "2001:db8::1" and "2001:0db8::"; and multicast ff00::/8
with "ff02::1" and "ff00::"; ensure each new test asserts
getWebhookUrlValidationErrorAsync resolves to 'Webhook URL must point to a
public host'.
🪄 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: c3ba0254-fa1a-4689-af99-7d9df40a8ae8
📒 Files selected for processing (4)
supabase/functions/_backend/private/website_preview.tssupabase/functions/_backend/utils/ip.tssupabase/functions/_backend/utils/webhook.tstests/webhook-url-validation.test.ts
|
Thanks for the follow-up, this addresses most of the concerns I raised, especially the shared IP utilities, DoH timeout/error handling, The remaining CodeRabbit finding looks valid to me: the A small helper that expands IPv6 into 8 hextets and compares prefix bits would close that cleanly. After that and the merge conflict resolution, this looks much closer. |
|
Pushed another follow-up commit ( Changes:
Verification run locally: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/webhook-url-validation.test.ts (1)
6-19: ⚡ Quick winIsolate shared DNS/delivery maps per test run
dnsAnswersanddeliveryResponsesare shared across allit.concurrentcases, so test state can bleed between cases as this file grows. Please clear both maps in abeforeEachto keep cases independent.Suggested change
-import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest' +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' @@ describe('webhook URL validation', () => { + beforeEach(() => { + dnsAnswers.clear() + deliveryResponses.clear() + }) + beforeAll(() => {As per coding guidelines, tests in
tests/**/*.test.tsshould be designed for parallel execution.🤖 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 `@tests/webhook-url-validation.test.ts` around lines 6 - 19, The shared maps dnsAnswers and deliveryResponses must be cleared before each test to avoid cross-test state; add a beforeEach hook that calls dnsAnswers.clear() and deliveryResponses.clear(), placing it near the top of the test file alongside the existing mock helpers (mockDnsAnswers and mockDnsThenDelivery) so each it.concurrent run starts with empty maps.
🤖 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.
Nitpick comments:
In `@tests/webhook-url-validation.test.ts`:
- Around line 6-19: The shared maps dnsAnswers and deliveryResponses must be
cleared before each test to avoid cross-test state; add a beforeEach hook that
calls dnsAnswers.clear() and deliveryResponses.clear(), placing it near the top
of the test file alongside the existing mock helpers (mockDnsAnswers and
mockDnsThenDelivery) so each it.concurrent run starts with empty maps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 301bdeec-76f8-40a2-bc5e-9982b95c0d24
📒 Files selected for processing (2)
supabase/functions/_backend/utils/ip.tstests/webhook-url-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/functions/_backend/utils/ip.ts
|
|
This still has a DNS TOCTOU gap. Since redirects are already |
|
I pushed a follow-up branch with the remaining DNS TOCTOU fix and opened a PR into this PR's source branch:
Summary of the fix:
Coverage added/updated:
Ran locally:
|
|
Fix conflict |
|
P2: IPv4-mapped IPv6 is only detected when the address string starts with |
|
P2: Partial DNS lookup failures can still pass validation when the other family returns a public address. I would make lookup failure explicit instead of encoding it as an empty answer set, then reject if either A or AAAA lookup errors. Empty |
KCDaemon
left a comment
KCDaemon
left a comment
There was a problem hiding this comment.
Rechecked the current head (e44cd81).
This is much better than the earlier version, but I still see two SSRF validation gaps that should stay blocking before merge:
-
IPv4-mapped IPv6 is only handled when the string starts with
::ffff:. Equivalent expanded forms inside::ffff:0:0/96, such as0:0:0:0:0:ffff:7f00:1for loopback or0000:0000:0000:0000:0000:ffff:0a00:0001for RFC1918, are valid AAAA answers but do not hit that shortcut. Those should be parsed as IPv6 hextets first, detect the[0,0,0,0,0,0xffff]prefix, and then run the embedded last 32 bits through the IPv4 private-range classifier. -
resolveHostnameIps()still turns resolver/network failures into[], andgetWebhookUrlValidationErrorAsync()only fails closed when the combined A+AAAA list is empty. So A can return a public address while AAAA times out/503s, and validation still passes without ever classifying the failed address family. For an SSRF guard, I would make resolver failure explicit and reject if either A or AAAA lookup fails; an empty successful answer can remain distinct from a failed lookup.
The PR also remains merge-conflicted (mergeStateStatus is DIRTY). I would keep this blocked until those edge cases and the conflict are fixed.
9 participants
Summary
Security context
The existing webhook guard blocked direct IP literals and localhost hostnames, but hostnames resolving to private network addresses were not checked. This hardens webhook delivery against SSRF-style private DNS targets while preserving the existing
CAPGO_ALLOW_LOCAL_WEBHOOK_URLSlocal-development override.Tests
npx vitest run tests/webhook-url-validation.test.tsnpx eslint "supabase/functions/_backend/utils/webhook.ts" "supabase/functions/_backend/public/webhooks/post.ts" "supabase/functions/_backend/public/webhooks/put.ts" "supabase/functions/_backend/public/webhooks/test.ts" "supabase/functions/_backend/public/webhooks/deliveries.ts" "supabase/functions/_backend/triggers/webhook_dispatcher.ts" "tests/webhook-url-validation.test.ts"Summary by CodeRabbit
Bug Fixes
Tests