fix(webhooks): validate webhook URLs on delivery#1571
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces centralized URL validation for webhooks by creating a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: daf8804786
ℹ️ 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".
| if (isIpLiteral(hostname)) | ||
| return 'Webhook URL must use a hostname, not an IP address' |
There was a problem hiding this comment.
This new validation hard-rejects any IP-literal hostname (e.g., https://203.0.113.10/webhook), but those URLs were previously accepted as long as they were HTTPS. Because the same check now runs on create/update/test/retry/dispatcher, existing customers who configured webhooks with a public IP will see deliveries fail and be unable to retry without changing their URL. That is a backward‑compatibility regression for a public API, and the change is not version‑gated or flagged.
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: 1
🤖 Fix all issues with AI agents
In `@supabase/functions/_backend/utils/webhook.ts`:
- Line 5: The webhook URL hostname validation currently only checks literal IPs
and "localhost" strings; update the validation logic (the function that
validates webhook endpoints in this file — e.g., the webhook URL validation
helper around lines ~48-90) to resolve hostnames and reject any addresses in
private/loopback/link-local/reserved ranges for both IPv4 and IPv6: parse the
URL to get the hostname, if it's an IP validate against
RFC1918/loopback/link-local CIDRs, otherwise perform a DNS resolution
(dns.promises.lookup with {all:true}) and check every returned address against
the same private/reserved CIDRs; use a reliable IP/CIDR library (e.g., ipaddr.js
or a small utility using net.isIP + CIDR checks) to perform the range checks and
fail validation if any resolved address is private, or alternatively enforce a
strict hostname allowlist. Ensure the check runs wherever the current
hostname-only checks live (the existing literal IP/localhost branch) so SSRF via
hostnames is blocked.
| import { cloudlog, cloudlogErr, serializeError } from './logging.ts' | ||
| import { closeClient, getPgClient } from './pg.ts' | ||
| import { supabaseAdmin } from './supabase.ts' | ||
| import { getEnv } from './utils.ts' |
There was a problem hiding this comment.
Block hostnames that resolve to private/loopback ranges.
Current checks only reject literal IPs and localhost/.localhost, so hostnames that resolve to private RFC1918/loopback/link‑local ranges (or common local suffixes like .local, localhost.localdomain) can still pass and enable SSRF. Consider resolving the hostname and rejecting private/reserved ranges (IPv4 + IPv6), or adopting a strict allowlist.
Also applies to: 48-90
🤖 Prompt for AI Agents
In `@supabase/functions/_backend/utils/webhook.ts` at line 5, The webhook URL
hostname validation currently only checks literal IPs and "localhost" strings;
update the validation logic (the function that validates webhook endpoints in
this file — e.g., the webhook URL validation helper around lines ~48-90) to
resolve hostnames and reject any addresses in
private/loopback/link-local/reserved ranges for both IPv4 and IPv6: parse the
URL to get the hostname, if it's an IP validate against
RFC1918/loopback/link-local CIDRs, otherwise perform a DNS resolution
(dns.promises.lookup with {all:true}) and check every returned address against
the same private/reserved CIDRs; use a reliable IP/CIDR library (e.g., ipaddr.js
or a small utility using net.isIP + CIDR checks) to perform the range checks and
fail validation if any resolved address is private, or alternatively enforce a
strict hostname allowlist. Ensure the check runs wherever the current
hostname-only checks live (the existing literal IP/localhost branch) so SSRF via
hostnames is blocked.
There was a problem hiding this comment.
Pull request overview
This PR centralizes webhook URL validation and applies it across webhook creation/update, test/retry, dispatcher queuing, and delivery-time execution to reduce SSRF risk and improve consistency.
Changes:
- Added a shared
getWebhookUrlValidationError()helper and used it across webhook endpoints and trigger handlers. - Blocked deliveries early (dispatcher + delivery) when URL validation fails, and persisted failure results to
webhook_deliveries. - Adjusted stored delivery failure messaging to be more generic.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| supabase/functions/_backend/utils/webhook.ts | Introduces centralized URL validation and enforces it at delivery-time; adjusts delivery failure response body. |
| supabase/functions/_backend/triggers/webhook_dispatcher.ts | Validates webhook URLs before queueing deliveries and marks invalid deliveries as failed immediately. |
| supabase/functions/_backend/public/webhooks/test.ts | Validates webhook URL before attempting a test delivery. |
| supabase/functions/_backend/public/webhooks/put.ts | Replaces inline URL validation with the shared validator for webhook updates. |
| supabase/functions/_backend/public/webhooks/post.ts | Replaces inline URL validation with the shared validator for webhook creation. |
| supabase/functions/_backend/public/webhooks/deliveries.ts | Validates webhook URL before allowing a retry to be queued. |
| if (isLocalWebhookEnv(c)) | ||
| return null | ||
|
|
There was a problem hiding this comment.
getWebhookUrlValidationError fully bypasses all validation when SUPABASE_URL contains localhost/127.0.0.1. In local/dev/test environments this will allow non-HTTPS public URLs (e.g. http://example.com/...), which breaks existing API expectations/tests (see tests/webhooks.test.ts cases that require HTTP non-local URLs to be rejected) and weakens SSRF protections during development.
Instead of returning null for every URL in local env, keep enforcing the general rules and only relax them for explicitly local targets (e.g. allow http://localhost / loopback only), while still rejecting non-HTTPS for non-local hosts.
| const urlValidationError = getWebhookUrlValidationError(c, url) | ||
| if (urlValidationError) { | ||
| const duration = Date.now() - startTime | ||
| cloudlogErr({ | ||
| requestId: c.get('requestId'), |
There was a problem hiding this comment.
The new URL validation blocks disallowed initial URLs, but fetch() will still follow redirects by default. A webhook URL can pass this validation and then redirect to a disallowed target (IP literal / localhost / non-HTTPS), effectively bypassing the protection and reintroducing SSRF risk.
To make this validation effective, consider disabling automatic redirects (redirect: 'manual') and failing on 3xx, or manually following redirects while re-validating each Location (with a hop limit).
| return { | ||
| success: false, | ||
| body: `Error: ${errorMessage}`, | ||
| body: 'Error: Webhook delivery failed', | ||
| duration, |
There was a problem hiding this comment.
This changes the stored/displayed delivery error from the actual exception message to a fully generic string ('Error: Webhook delivery failed'). Since response_body is surfaced in the UI (e.g. src/components/WebhookDeliveryLog.vue), this removes most actionable diagnostics for users and makes it hard to distinguish timeouts vs DNS vs TLS failures.
Consider returning/storing a safe, categorized error (e.g. timeout vs network vs non-2xx) or a sanitized subset of the original message, while keeping detailed errors only in logs if needed.
riderx
force-pushed
the
riderx/webhook-ssrf-fix
branch
from
415a3cd to
63dc913
Compare
|
|
/tip @Judel777 $150 thanks for the report |
|
🎉🎈 @Judel777 has been awarded $150 by Capgo! 🎈🎊 |
|
I’ve re-tested this on production. PUT /webhooks now correctly rejects:
|
Summary (AI generated)
Motivation (AI generated)
Business Impact (AI generated)
Test plan (AI generated)
bun run lint:backendScreenshots (AI generated)
Checklist (AI generated)
bun run lint:backend && bun run lintSummary by CodeRabbit