fix: close the webhook SSRF denylist gaps (ENG-2310)

[xernobyl]

What does this PR do?

Closes the webhook SSRF denylist gaps in ENG-2310.

isPrivateOrReserved relied entirely on net/netip's predicates, which model only RFC1918 + fc00::/7, 127/8 + ::1, 169.254/16 + fe80::/10, ff02:: and the single unspecified address. Every other private or reserved range passed both the create/update check and the dial-time check.

The one that motivated the ticket: CGNAT 100.64.0.0/10. Tailscale addresses tailnets out of it, so on a tailnet deployment a webhook could reach internal services — and it is a range the Formbricks side already blocks and has a test for, so this was a plain inconsistency between the two products rather than an open question.

Newly rejected:

Range	Why it matters
100.64.0.0/10	CGNAT — Tailscale tailnets
0.0.0.0/8	IsUnspecified() matches only 0.0.0.0 itself, so 0.1.2.3 passed
64:ff9b::/96, 64:ff9b:1::/48	NAT64 — 64:ff9b::a9fe:a9fe is IMDS
2002::/16	6to4 — 2002:7f00:1::1 is 127.0.0.1
168.63.129.16	Azure WireServer, the IMDS sibling outside 169.254/16
fec0::/10	deprecated IPv6 site-local
198.18.0.0/15, 240.0.0.0/4, 255.255.255.255	benchmarking, reserved, broadcast
192.0.0.0/24, TEST-NET-1/2/3	IETF assignments and documentation ranges
remaining multicast scopes	IsLinkLocalMulticast() caught only ff02:: and 224.0.0.0/24

Approach

Two things worth flagging for review, because both are the opposite of what the ticket suggested:

1. CIDRs are added alongside the netip predicates, not in place of them. The ticket proposed porting Formbricks' range list over. That would have been a regression: Formbricks matches IPv6 by string prefix "fe80:", covering only fe80::/16, whereas IsLinkLocalUnicast() covers the full fe80::/10. fe9f::1 and febf::1 are blocked here today and allowed on the Formbricks side. There are explicit no-regression tests for those two addresses.
2. IsLinkLocalMulticast() becomes IsMulticast(), which covers 224/4 and ff00::/8 in full — so multicast needs no CIDR entries at all.

validateWebhookHost is now a thin wrapper over resolveWebhookHost. The two were ~90% duplicated, which is how create-time and dial-time validation could drift apart in the first place; they are now provably the same check.

Breaking change + escape hatch

Blocking CGNAT is potentially breaking: an operator whose webhook receiver sits on a tailnet would start getting 400s on upgrade, and there was no way to opt out — Hub has no equivalent of Formbricks' DANGEROUSLY_ALLOW_WEBHOOK_INTERNAL_URLS.

So this adds WEBHOOK_ALLOWED_CIDRS, a comma-separated allowlist that re-permits specific ranges. It is deliberately scoped:

• it only bypasses the range classifier, not WEBHOOK_BLACKLIST, which stays an explicit deny that wins;
• a malformed entry fails startup rather than being skipped (unlike parseBlacklist) — this list widens what is reachable, so a typo must not be silently absorbed;
• default is empty, so nothing is reachable that wasn't intended.

Documented in .env.example and charts/hub/values.yaml.

How should this be tested?

Automated:

• go test ./internal/... — TestIsPrivateOrReserved covers ~50 vectors: every newly-blocked range, the predicate-covered ranges as regression cases, and public controls (8.8.8.8, 100.128.0.1 immediately above CGNAT, 172.32.0.1 immediately above RFC1918, 64:ff9c::1 immediately above NAT64) that catch a prefix written one bit too wide. TestSSRFPolicy_Permits covers blacklist/allowlist precedence.
• TestWebhooksService_CreateWebhook_SSRFRangeCoverage drives the ranges through the real create path, and ..._AllowedCIDR proves the allowlist toggles behaviour.
• TestLoad_WebhookAllowedCIDRs proves a malformed value fails Load(), not just the setter.
• make tests (integration) — needs a DB; the harness allowlists TEST-NET-1 because the fixtures target 192.0.2.1.

I also confirmed the new tests can fail: reverting isPrivateOrReserved to the four-predicate version turns 26 cases red, and no public-address case fails, so they aren't just asserting "block everything".

Manual, against a local stack (DATABASE_URL on a scratch DB, API_KEY=…, PORT=8477) — this is the config→classifier wiring a unit test can't prove:

# no allowlist
curl -X POST localhost:8477/v1/webhooks -H 'Authorization: Bearer $KEY' -H 'content-type: application/json' \
  -d '{"url":"https://100.64.0.1/webhook","tenant_id":"org-x","event_types":["feedback_record.created"]}'

Case	Result
100.64.0.1, no allowlist	400 webhook URL host is not allowed (private/internal)
[64:ff9b::a9fe:a9fe], no allowlist	400 same
example.com (control)	201
100.64.0.1 with WEBHOOK_ALLOWED_CIDRS=100.64.0.0/10	201
10.0.0.1 with that same allowlist	400 — allowlist is scoped
[64:ff9b::a9fe:a9fe] with that allowlist	400
100.64.0.1 with allowlist and WEBHOOK_BLACKLIST=100.64.0.1	400 (blacklisted) — denylist wins
WEBHOOK_ALLOWED_CIDRS=100.64.0.0/10,oops	process refuses to start, error names the bad entry

Checklist

Required

• Filled out the "How to test" section in this PR
• Read Repository Guidelines
• Self-reviewed my own code
• Commented on my code in hard-to-understand bits
• Ran make build
• Ran make tests (integration tests in tests/)
• Ran make fmt and make lint; no new warnings
• Removed debug prints / temporary logging
• Merged the latest changes from main onto my branch with git pull origin main
• If database schema changed: added migration in migrations/ with goose annotations and ran make migrate-validate — n/a, no schema change

Appreciated

• If API changed: added or updated OpenAPI spec and ran contract tests — n/a, no API surface change; only which URLs are accepted
• If API behavior changed: added request/response examples to this PR
• Updated docs in docs/ if changes were necessary — .env.example and the Helm values carry the new setting
• Ran make tests-coverage for meaningful logic changes

Notes for the reviewer

• Release exposure: the gap has been present since the classifier landed (chore: production hardening — Go 1.26, security (SSRF, hide signing key), config, pagination #46, 2026-03-09) and is in every release 0.2.0 → 0.8.5. Not a fresh regression, and Medium severity, so per policy no backport — flag if you disagree.
• ENG-1326 is the precedent worth knowing about. It reported the NAT64/6to4/site-local half of this for Formbricks and was Canceled with no rationale recorded (10 days in Triage, zero comments). I verified those five ranges still bypass Formbricks main and release 5.3.4, so that half is genuinely unfixed rather than deliberately declined. ENG-2218 closed the same NAT64 class as Done, but that was a transitive ip-address bump and ip-address has no first-party import anywhere in the monorepo — it fixed nothing in this path. A companion PR covers the Formbricks side.

---

Self-review findings (addressed in b26d553)

I reviewed my own diff adversarially rather than re-reading it, and the CIDR mechanism I'd added turned out to be trivially bypassable.

1. IPv6 zone identifiers bypassed the entire CIDR list. netip.Prefix.Contains is documented to return false for an address carrying a zone, and Go's url.Parse preserves the zone through u.Hostname(). So https://[64:ff9b::a9fe:a9fe%25eth0]/ skipped every entry in blockedPrefixes and reached IMDS.

Scope: the netip predicates evaluate zoned addresses correctly (IsLinkLocalUnicast("fe80::1%eth0") is true), so loopback / RFC1918 / link-local never leaked — only the CIDR-matched ranges did, which is exactly the half this branch adds. Not a regression against main, but it would have shipped a fix that a %25eth0 suffix defeats.

Fix: classify rejects zoned addresses outright (a zone means "reach this via that interface" — never a valid webhook target), and isPrivateOrReserved strips the zone before the CIDR walk so it cannot fail open for any other caller. Both are tested, and 10 cases go red without the fix.

2. Four more ranges of the same class. ::/96 IPv4-compatible IPv6 (::7f00:1 is 127.0.0.1, ::a9fe:a9fe is IMDS — Unmap() only collapses ::ffff:0:0/96, so this was classified as global unicast), Teredo 2001::/32 (tunnels IPv4 exactly as the 6to4 we already block), plus 2001:db8::/32 documentation and 100::/64 discard-only. Boundary controls assert ::ffff:93.184.216.34 and 2001:4860:4860::8888 stay reachable.

3. The rejection reason regressed when I merged the two validators. permits() collapsed "blacklisted" and "private/internal" into one message, so a host the operator had themselves put in WEBHOOK_BLACKLIST was reported as private/internal — the opposite of actionable. Split back into a classify → reason → message chain and verified live: https://dns.google/ with WEBHOOK_BLACKLIST=8.8.8.8 now reports (blacklisted) through the DNS path.

4. WEBHOOK_ALLOWED_CIDRS was silent. An allowlist re-opens internal ranges to anyone who can create a webhook, so it should never be in effect without being visible. NewSSRFPolicy now logs a warning naming the ranges.

5. Two cleanups. Dropped the redundant 255.255.255.255/32 (already inside 240.0.0.0/4) and the never-read ssrfPolicy field on WebhookSenderImpl — the policy is enforced in the transport's DialContext, and a field of that name implied Send consulted it.

Smoke test against a live stack

Fresh DB + API on a scratch port, hitting POST /v1/webhooks for real:

Target	Result
[64:ff9b::a9fe:a9fe%25eth0], [::7f00:1%25eth0], [fec0::1%25eth0]	400 private/internal — zone bypass closed
[::7f00:1], [::a9fe:a9fe], [2001:0:1234::1], [2001:db8::1], [100::1]	400 private/internal
evil@127.0.0.1	400 — rejected by the URL pattern before the classifier
127.0.0.1. (trailing dot)	400 blacklisted — canonicalizeHost trims the dot, so the blacklist entry still matches
dns.google with WEBHOOK_BLACKLIST=8.8.8.8	400 blacklisted — the reason survives the DNS path
100.64.0.1 with WEBHOOK_ALLOWED_CIDRS=100.64.0.0/10	201, and startup logged the allowlist warning
10.0.0.1 and the zoned NAT64 address, same allowlist	400 — allowlist stays scoped
example.com	201

Also confirmed no unvalidated egress path exists: webhook_sender.go:126 is the only place a webhook URL is fetched, and it uses the client whose DialContext re-validates.

golangci-lint run ./... clean, full unit suite and make tests (integration) green.

---

Review round 2 (addressed in c7b4b28)

@BhagyaAmarasinghe caught that blockedPrefixes still omitted 5f00::/16 (SRv6 SIDs) and 3fff::/20 (documentation) — correct, and the 5f00::/16 one matters most: IANA marks it forwardable but not globally reachable, so an SRv6-enabled deployment kept a real internal-destination path. Both were accepted by isPrivateOrReserved and by resolveWebhookHost, so the secured dialer treated them as eligible.

Rather than add just those two, I swept the registry. Four prefixes added:

Prefix	What	How found
5f00::/16	SRv6 SIDs (RFC 9602) — forwardable, so internally routable	reported in review
3fff::/20	documentation (RFC 9637)	reported in review
2001:20::/28	ORCHIDv2 (RFC 7343)	registry sweep
::ffff:0:0:0/96	IPv4-translated (RFC 2765 / SIIT)	registry sweep

The last one is the same wrapper class as the transition ranges already here, and the sixth IPv4-wrapper format after IPv4-mapped, IPv4-compatible, NAT64, 6to4 and Teredo. Unmap() collapses only ::ffff:0:0/96, so ::ffff:0:7f00:1 (127.0.0.1) and ::ffff:0:a9fe:a9fe (IMDS) reached the CIDR walk as ordinary global unicast. The Formbricks side had the identical gap — fixed there in #8946.

The derivation is now recorded in the file, so the next addition is a lookup rather than a guess: the IPv6 entries are the IANA special-purpose registry filtered to Globally Reachable = False, minus what the netip predicates already cover. The registry's Globally Reachable = True entries are deliberately absent — 2001:3::/32 (AMT), 2001:4:112::/48 and 2620:4f:8000::/48 (AS112), 2001:30::/28 (DRIP) and 192.88.99.0/24 (6to4 relay anycast) are public infrastructure, and blocking them would reject legitimate targets.

That last exclusion makes the ORCHIDv2 boundary load-bearing: 2001:30::/28 (DRIP) sits immediately above 2001:20::/28, so a prefix written one bit too wide there would blackhole globally-reachable space. There is now a control asserting 2001:30::1 stays public.

Tests

As requested — default rejection and explicit allowlisting for the new ranges:

• Default rejection for every new range, plus top-of-range cases (2001:2f:ffff:…, 3fff:fff:ffff:…, 5f00:ffff::1).
• Boundary controls either side of each: 2001:1f::1, 2001:30::1, 3ffe::1, 4000::1, 5eff::1, 6000::1, ::ffff:1:0:0 all stay reachable.
• Allowlist round-trips through the real service path (CreateWebhook, so resolveWebhookHost is exercised, not just classify): blocked by default → reachable when the operator names the range → an entry for one new range does not re-open another → WEBHOOK_BLACKLIST still wins over an allowlist covering it.

10 unit cases and 5 service cases go red without the prefix additions.

One test-harness fix worth calling out

Two fixtures built the mock repo without a webhook, so a URL that was wrongly accepted hit a nil dereference. A panic aborts the whole test binary — which meant exactly one of my five new service cases reported, and the other four never ran. They now fail as clean assertions instead, so the red state is legible.

golangci-lint run ./... clean, full unit suite green.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 9ef5039d-37a8-4257-8244-36574dc0cfef

📥 Commits

Reviewing files that changed from the base of the PR and between 0dd2db1 and b26d553.

📒 Files selected for processing (13)

• .env.example
• charts/hub/values.yaml
• cmd/api/app.go
• cmd/worker/app.go
• internal/config/config.go
• internal/config/config_test.go
• internal/service/webhook_sender.go
• internal/service/webhook_sender_test.go
• internal/service/webhook_ssrf.go
• internal/service/webhook_ssrf_test.go
• internal/service/webhooks_service.go
• internal/service/webhooks_service_test.go
• tests/integration_test.go

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

The change adds WEBHOOK_ALLOWED_CIDRS configuration with strict CIDR parsing and startup errors for malformed values. It introduces SSRFPolicy with blacklist precedence, CIDR allowlisting, hostname canonicalization, zoned IPv6 rejection, and expanded private or reserved range detection. Webhook creation, updates, host resolution, and sending now use the policy. API, worker, chart, environment, and integration test wiring passes blacklist and CIDR settings into the policy.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 11 files. (2 skipped:…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the primary change: fixing webhook SSRF denylist gaps. It uses the Conventional Commits fix: prefix and includes the issue reference.
Description check	✅ Passed	The description is complete and directly related to the changes. It explains the SSRF gaps, implementation approach, allowlist behavior, configuration failures, testing steps, manual verification, che…

Full details: Docstring Coverage

Explanation

Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 32 functions across 11 files. (2 skipped: 2 unsupported.)

Full details: Description check

Explanation

The description is complete and directly related to the changes. It explains the SSRF gaps, implementation approach, allowlist behavior, configuration failures, testing steps, manual verification, checklist status, and review findings.

• Fix all pre-merge checks with AI

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[BhagyaAmarasinghe]

Requesting changes for one low-severity but reproducible SSRF-policy gap. The new classifier still admits IANA special-purpose IPv6 ranges that can be internally routed; details inline.