Repository navigation
fix(webhooks): reject internal and metadata addresses in webhook delivery - #467
polylane[bot] wants to merge 1 commit into
Conversation
…very Co-authored-by: polylane[bot] <277585245+polylane[bot]@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Deploying usesend with
|
| Latest commit: |
1358dc5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://86e96378.usesend.pages.dev |
| Branch Preview URL: | https://polylane-autofix-4p0jdtvo2jc.usesend.pages.dev |
| try { | ||
| await assertWebhookUrlAllowed(url); | ||
| } catch (error) { | ||
| if (error instanceof WebhookUrlNotAllowedError) { |
There was a problem hiding this comment.
Log each refused non-public webhook URL so an operator can count SSRF attempts at the save path.
| if (error instanceof WebhookUrlNotAllowedError) { | |
| if (error instanceof WebhookUrlNotAllowedError) { | |
| logger.warn( | |
| { reason: error.message }, | |
| "[WebhookService]: Rejected non-public webhook URL", | |
| ); |
|
Tip Expected to resolve the linked issue. Linked issue: Webhook delivery calls tenant URLs without an SSRF check Checked the unsend production service. 330 webhook deliveries in 72h, all HTTP 200, no failures. The guard adds one DNS resolve before the existing fetch, and CPU stayed under 0.05 vCPU of a 32 vCPU limit. Also considered · 1 plausible, 3 refuted
Analysed against 1 Service and 1 repository
Polylane analysed Rate this review: 👍 helpful · 👎 not helpful · 😕 confusing |
Fixes: Webhook delivery calls tenant URLs without an SSRF check
Any team member could save a webhook destination on an internal address. The delivery worker sent the signed payload to that address, and the tenant could read the response back from the webhook call record. Once this merges, an internal, loopback, link-local, or metadata destination is no longer usable for a webhook.
Detection evidence
Affected:
int_13ee3c91c001c2c8xfdj06a9Why this fix
A unit test drives the real delivery worker at a loopback webhook target, and the worker contacted it, while the repository docs already require a publicly accessible endpoint. The worker now resolves the host and refuses a private, loopback, link-local, or metadata address before it sends, and the save path refuses the same addresses so the tenant gets a clear error instead of a silently failing webhook. The residual risk is a DNS answer that changes between the save check and the send check, because the resolved address is not pinned.
Fix walkthrough
Watch the fix explained (2:33). Recorded for commit
1358dc55696f684d948c8e55b429249404c0dcce.Out of scope
SesSettingsService.createSesSettingfetches an admin-supplied URL without this guard. Its caller is the platform-admin procedure, a different boundary, and it is recorded on the incident timeline.Causal chain
4 files changed (+624/-0)
apps/web/src/server/service/webhook-service.ts: modified, +26/-0apps/web/src/server/service/webhook-service.unit.test.ts: modified, +6/-0apps/web/src/server/service/webhook-url-guard.ts: added, +250/-0apps/web/src/server/service/webhook-url-guard.unit.test.ts: added, +342/-0Repository lint:
pnpm run lint(declared in package.json) fails over the whole repository (the declared target takes no file arguments), so the failure is not attributed to this change and did not block it; run it before merging.Lint output
Generated by Polylane. You can ask follow-ups by mentioning @polylane in a comment.
Summary by cubic
Rejects webhook destinations that resolve to internal, loopback, link-local, or cloud metadata addresses so the delivery worker no longer sends signed payloads to tenant controlled internal hosts.
BAD_REQUESTto the tenant instead of storing a silently failing webhook.assertWebhookUrlAllowedguard that resolves hostnames and rejects if any returned address is non-public.Written for commit 1358dc5. Summary will update on new commits.