fix(security): block SSRF bypass vectors in webhook URL validation#1726
fix(security): block SSRF bypass vectors in webhook URL validation#1726artylobos wants to merge 1 commit intoCap-go:mainfrom
Conversation
📝 WalkthroughWalkthroughAdded SSRF hardening to webhook URL validation by introducing blocked-domain lists and helper predicates to prevent requests to localhost, DNS rebinding services, and cloud metadata endpoints, with checks running after hostname normalization but before HTTPS enforcement. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
supabase/functions/_backend/utils/webhook.ts (2)
68-74: Verify the baremetadatahostname doesn't cause false positives.The entry
'metadata'without a domain suffix could theoretically block legitimate webhook endpoints if a user's internal service happens to be namedmetadata. While this provides defense-in-depth, consider whether the risk of false positives outweighs the security benefit for this specific entry.Additionally, cloud providers primarily use IPs (blocked by
isIpLiteral) or specific FQDNs, so the bare hostname may be unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/webhook.ts` around lines 68 - 74, Remove the bare 'metadata' entry from the BLOCKED_METADATA_HOSTNAMES set to avoid false positives; update the constant (BLOCKED_METADATA_HOSTNAMES) to only include explicit metadata FQDNs ('metadata.google.internal', 'metadata.goog', 'instance-data') and adjust any tests or callers that assumed 'metadata' was blocked, or alternatively implement a stricter check (e.g., only block when host resolves to known metadata IPs via isIpLiteral or explicit DNS resolution) if you need defense-in-depth.
110-112: Consider adding subdomain checks for consistency withisBlockedLocalhostDomain.Unlike
isBlockedLocalhostDomain, this function only performs exact matches. While cloud metadata services typically only respond to exact hostnames, adding subdomain blocking would provide defense-in-depth and maintain consistency across the helper functions.🛡️ Suggested enhancement
function isCloudMetadataHostname(hostname: string): boolean { - return BLOCKED_METADATA_HOSTNAMES.has(hostname) + if (BLOCKED_METADATA_HOSTNAMES.has(hostname)) + return true + for (const blocked of BLOCKED_METADATA_HOSTNAMES) { + if (hostname.endsWith(`.${blocked}`)) + return true + } + return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/webhook.ts` around lines 110 - 112, The isCloudMetadataHostname function currently only checks exact matches against BLOCKED_METADATA_HOSTNAMES; change it to also block subdomains for defense-in-depth and consistency with isBlockedLocalhostDomain by comparing a normalized hostname (e.g., lowercased/trimmed) and returning true if it equals any entry in BLOCKED_METADATA_HOSTNAMES or endsWith("." + entry); reference the isCloudMetadataHostname function and the BLOCKED_METADATA_HOSTNAMES set and mirror the subdomain logic used in isBlockedLocalhostDomain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/_backend/utils/webhook.ts`:
- Around line 68-74: Remove the bare 'metadata' entry from the
BLOCKED_METADATA_HOSTNAMES set to avoid false positives; update the constant
(BLOCKED_METADATA_HOSTNAMES) to only include explicit metadata FQDNs
('metadata.google.internal', 'metadata.goog', 'instance-data') and adjust any
tests or callers that assumed 'metadata' was blocked, or alternatively implement
a stricter check (e.g., only block when host resolves to known metadata IPs via
isIpLiteral or explicit DNS resolution) if you need defense-in-depth.
- Around line 110-112: The isCloudMetadataHostname function currently only
checks exact matches against BLOCKED_METADATA_HOSTNAMES; change it to also block
subdomains for defense-in-depth and consistency with isBlockedLocalhostDomain by
comparing a normalized hostname (e.g., lowercased/trimmed) and returning true if
it equals any entry in BLOCKED_METADATA_HOSTNAMES or endsWith("." + entry);
reference the isCloudMetadataHostname function and the
BLOCKED_METADATA_HOSTNAMES set and mirror the subdomain logic used in
isBlockedLocalhostDomain.
|
|
do not make security fix on PR use proper security disclosure |
|
2 participants
Summary
This PR fixes SSRF (Server-Side Request Forgery) bypass vulnerabilities in the webhook URL validation.
Security Issue
The existing validation in
getWebhookUrlValidationError()could be bypassed using:169.254.169.254.nip.ioresolves to AWS/GCP metadata endpointlocaltest.me,vcap.meresolve to127.0.0.1metadata.google.internalImpact
Changes
Added blocklists for:
localtest.me,vcap.me,lvh.me, etc.).nip.io,.xip.io,.sslip.io, etc.)metadata.google.internal, etc.)Testing
The following URLs are now correctly blocked:
https://169.254.169.254.nip.io/- DNS rebinding to metadatahttps://localtest.me/- Resolves to 127.0.0.1https://metadata.google.internal/- GCP metadataSummary by CodeRabbit