From 218186156e10a3bb44fb26d10f4be780a3ba19c4 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Fri, 21 Aug 2026 15:09:59 +0000 Subject: [PATCH 1/4] fix: close the webhook SSRF denylist gaps (ENG-2310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The webhook URL classifier 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, most notably CGNAT 100.64.0.0/10 — Tailscale addresses tailnets out of it, and it is a range the Formbricks side already blocks and has a test for. Add a CIDR table for the ranges netip has no predicate for, alongside the existing predicates rather than replacing them: IsLinkLocalUnicast covers the whole of fe80::/10, which a string-prefix classifier would not, so swapping the predicates out for a prefix table would open a gap instead of closing one. IsLinkLocalMulticast also becomes IsMulticast, which covers 224/4 and ff00::/8 in full — the old check leaked every multicast scope above ff02::. Newly rejected: 0.0.0.0/8 (IsUnspecified matches only 0.0.0.0 itself), 100.64.0.0/10, 192.0.0.0/24, the three TEST-NETs, 198.18.0.0/15, 240.0.0.0/4, 255.255.255.255, 168.63.129.16 (Azure WireServer, outside 169.254/16), 64:ff9b::/96 and 64:ff9b:1::/48 (NAT64), 2002::/16 (6to4), fec0::/10, and the remaining multicast scopes. Blocking CGNAT would otherwise be a silent breaking change for operators whose receivers sit on a tailnet, and there was no way to opt out, so add WEBHOOK_ALLOWED_CIDRS to re-permit specific ranges. It is scoped to the range check and does not override WEBHOOK_BLACKLIST, which stays an explicit deny. A malformed entry fails startup rather than being skipped: this list widens what is reachable, so a typo must not be silently absorbed. validateWebhookHost becomes 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; now they are provably the same check. --- .env.example | 7 + charts/hub/values.yaml | 4 + cmd/api/app.go | 5 +- cmd/worker/app.go | 3 +- internal/config/config.go | 43 ++++++ internal/config/config_test.go | 95 ++++++++++++ internal/service/webhook_sender.go | 22 +-- internal/service/webhook_sender_test.go | 6 +- internal/service/webhook_ssrf.go | 121 ++++++++++++++++ internal/service/webhook_ssrf_test.go | 169 ++++++++++++++++++++++ internal/service/webhooks_service.go | 152 +++++-------------- internal/service/webhooks_service_test.go | 104 ++++++++++++- tests/integration_test.go | 5 +- 13 files changed, 596 insertions(+), 140 deletions(-) create mode 100644 internal/service/webhook_ssrf.go create mode 100644 internal/service/webhook_ssrf_test.go diff --git a/.env.example b/.env.example index 342b84e4..26f65a9a 100644 --- a/.env.example +++ b/.env.example @@ -97,6 +97,13 @@ WEBHOOK_MAX_COUNT=500 # Default: localhost,127.0.0.1,::1,169.254.169.254 (includes AWS metadata endpoint) # WEBHOOK_BLACKLIST=localhost,127.0.0.1,::1,169.254.169.254 +# Webhook allowed CIDRs (optional). Comma-separated CIDR ranges that re-permit webhook targets the +# SSRF classifier would otherwise reject as private/reserved. Use this when your webhook receivers +# legitimately live on internal addresses -- most commonly a Tailscale tailnet, which is addressed +# out of 100.64.0.0/10. Entries in WEBHOOK_BLACKLIST still win. A malformed entry fails startup. +# Default: empty (no private/reserved range is reachable). +# WEBHOOK_ALLOWED_CIDRS=100.64.0.0/10 + # Webhook enqueue retries (optional). When River InsertMany fails, retry with exponential backoff + jitter. # Defaults: 3 retries, 100ms initial backoff, 2s max backoff. # WEBHOOK_ENQUEUE_MAX_RETRIES=3 diff --git a/charts/hub/values.yaml b/charts/hub/values.yaml index 600dc830..8817d97e 100644 --- a/charts/hub/values.yaml +++ b/charts/hub/values.yaml @@ -306,6 +306,10 @@ config: WEBHOOK_MAX_COUNT: "500" WEBHOOK_HTTP_TIMEOUT_SECONDS: "15" WEBHOOK_BLACKLIST: "localhost,127.0.0.1,::1,169.254.169.254" + # Re-permits webhook targets the SSRF classifier rejects as private/reserved. Set this only if + # your receivers live on internal addresses (e.g. a Tailscale tailnet in 100.64.0.0/10); + # WEBHOOK_BLACKLIST entries still win, and a malformed value fails startup. + WEBHOOK_ALLOWED_CIDRS: "" WEBHOOK_ENQUEUE_MAX_RETRIES: "3" WEBHOOK_ENQUEUE_INITIAL_BACKOFF_MS: "100" WEBHOOK_ENQUEUE_MAX_BACKOFF_MS: "2000" diff --git a/cmd/api/app.go b/cmd/api/app.go index 21bf6272..9202e591 100644 --- a/cmd/api/app.go +++ b/cmd/api/app.go @@ -365,7 +365,10 @@ func NewApp(cfg *config.Config, db *pgxpool.Pool) (*App, error) { } } - webhooksService := service.NewWebhooksService(webhooksRepo, messageManager, cfg.Webhook.MaxCount, cfg.Webhook.URLBlacklist) + webhooksService := service.NewWebhooksService( + webhooksRepo, messageManager, cfg.Webhook.MaxCount, + service.NewSSRFPolicy(cfg.Webhook.URLBlacklist, cfg.Webhook.AllowedCIDRs), + ) webhooksHandler := handlers.NewWebhooksHandler(webhooksService) tenantDataService := service.NewTenantDataService(tenantDataRepo) tenantDataHandler := handlers.NewTenantDataHandler(tenantDataService) diff --git a/cmd/worker/app.go b/cmd/worker/app.go index b47b1c02..b26b3f16 100644 --- a/cmd/worker/app.go +++ b/cmd/worker/app.go @@ -85,7 +85,8 @@ func NewWorkerApp(cfg *config.Config, db *pgxpool.Pool) (*WorkerApp, error) { } webhookSender := service.NewWebhookSenderImpl( - webhooksRepo, webhookMetrics, cfg.Webhook.URLBlacklist, cfg.Webhook.HTTPTimeout.Duration(), nil) + webhooksRepo, webhookMetrics, service.NewSSRFPolicy(cfg.Webhook.URLBlacklist, cfg.Webhook.AllowedCIDRs), + cfg.Webhook.HTTPTimeout.Duration(), nil) // hub-worker performs feedback-records purges; it never enqueues them (the API does), so the // service is built without an inserter. diff --git a/internal/config/config.go b/internal/config/config.go index 4b8b1c39..aab2af31 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -5,6 +5,7 @@ package config import ( "errors" "fmt" + "net/netip" "net/url" "os" "strconv" @@ -122,6 +123,7 @@ type WebhookConfig struct { EnqueueInitialBackoffMs int `env:"WEBHOOK_ENQUEUE_INITIAL_BACKOFF_MS" env-default:"100"` EnqueueMaxBackoffMs int `env:"WEBHOOK_ENQUEUE_MAX_BACKOFF_MS" env-default:"2000"` URLBlacklist BlacklistSet `env:"WEBHOOK_BLACKLIST" env-default:"localhost,127.0.0.1,::1,169.254.169.254"` + AllowedCIDRs CIDRSet `env:"WEBHOOK_ALLOWED_CIDRS"` } // MessagePublisherConfig holds event channel and timeout settings. @@ -268,6 +270,47 @@ func (d *DurationSec) Duration() time.Duration { return time.Duration(*d) } +// CIDRSet is a list of CIDR ranges that re-permit otherwise-blocked private/reserved webhook +// targets (e.g. a tailnet in 100.64.0.0/10). It implements cleanenv.Setter by parsing a +// comma-separated list of prefixes. +type CIDRSet []netip.Prefix + +// SetValue implements cleanenv.Setter. +// +// Unlike parseBlacklist, an unparseable entry is a hard error rather than a skipped one: this list +// widens what the SSRF classifier permits, so a typo must fail startup instead of silently leaving +// a range blocked (or, worse, being read as a different range than intended). +func (c *CIDRSet) SetValue(s string) error { + out, err := parseCIDRSet(s) + if err != nil { + return err + } + + *c = out + + return nil +} + +func parseCIDRSet(s string) (CIDRSet, error) { + var out CIDRSet + + for part := range strings.SplitSeq(s, ",") { + entry := strings.TrimSpace(part) + if entry == "" { + continue + } + + prefix, err := netip.ParsePrefix(entry) + if err != nil { + return nil, fmt.Errorf("parse webhook allowed CIDR %q: %w", entry, err) + } + + out = append(out, prefix.Masked()) + } + + return out, nil +} + // BlacklistSet is a set of normalized hostnames (e.g. for SSRF mitigation). // It implements cleanenv.Setter by parsing a comma-separated list. type BlacklistSet map[string]struct{} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 8beb4270..86ba4ee5 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "errors" + "net/netip" "os" "strings" "testing" @@ -989,3 +990,97 @@ func TestLoad_SurfacesEnvCoercionError(t *testing.T) { t.Fatalf("Load() error = %v, want it to name the offending SENTIMENT_MAX_ATTEMPTS variable", err) } } + +func TestCIDRSetSetValue(t *testing.T) { + var set CIDRSet + + if err := set.SetValue("100.64.0.0/10, 64:ff9b::/96 ,"); err != nil { + t.Fatalf("SetValue() error = %v, want nil", err) + } + + if len(set) != 2 { + t.Fatalf("len(set) = %d, want 2 (blank entries skipped)", len(set)) + } + + if got := set[0].String(); got != "100.64.0.0/10" { + t.Errorf("set[0] = %q, want %q", got, "100.64.0.0/10") + } + + if !set[0].Contains(netip.MustParseAddr("100.100.100.100")) { + t.Error("parsed prefix does not contain an address inside its range") + } + + var empty CIDRSet + if err := empty.SetValue(""); err != nil { + t.Fatalf("SetValue(\"\") error = %v, want nil", err) + } + + if len(empty) != 0 { + t.Errorf("len(empty) = %d, want 0", len(empty)) + } +} + +// A typo in an SSRF allowlist must fail startup rather than being silently skipped: skipping it +// would leave the operator believing a range is permitted when it is not, and a mistyped prefix +// could name a wider range than intended. +func TestCIDRSetSetValue_RejectsMalformedEntry(t *testing.T) { + for _, raw := range []string{"not-a-cidr", "100.64.0.0", "100.64.0.0/99", "100.64.0.0/10,garbage"} { + var set CIDRSet + if err := set.SetValue(raw); err == nil { + t.Errorf("SetValue(%q) error = nil, want error", raw) + } + } +} + +// A host address (e.g. 100.64.0.1/10) is normalized to its network so Contains behaves as the +// operator expects rather than depending on which host they happened to type. +func TestCIDRSetSetValue_MasksHostBits(t *testing.T) { + var set CIDRSet + if err := set.SetValue("100.64.0.5/10"); err != nil { + t.Fatalf("SetValue() error = %v, want nil", err) + } + + if got := set[0].String(); got != "100.64.0.0/10" { + t.Errorf("set[0] = %q, want %q", got, "100.64.0.0/10") + } +} + +func TestLoad_WebhookAllowedCIDRs(t *testing.T) { + t.Run("empty when unset", func(t *testing.T) { + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + if len(cfg.Webhook.AllowedCIDRs) != 0 { + t.Errorf("Webhook.AllowedCIDRs = %v, want empty by default", cfg.Webhook.AllowedCIDRs) + } + }) + + t.Run("parsed from WEBHOOK_ALLOWED_CIDRS", func(t *testing.T) { + t.Setenv("WEBHOOK_ALLOWED_CIDRS", "100.64.0.0/10,64:ff9b::/96") + + cfg, err := Load() + if err != nil { + t.Fatalf("Load() error = %v", err) + } + + if len(cfg.Webhook.AllowedCIDRs) != 2 { + t.Fatalf("len(Webhook.AllowedCIDRs) = %d, want 2", len(cfg.Webhook.AllowedCIDRs)) + } + + if !cfg.Webhook.AllowedCIDRs[0].Contains(netip.MustParseAddr("100.64.0.1")) { + t.Error("configured tailnet prefix does not contain 100.64.0.1") + } + }) + + // Fail closed: a malformed allowlist must stop the process, not start it with a policy the + // operator did not write. + t.Run("malformed value fails startup", func(t *testing.T) { + t.Setenv("WEBHOOK_ALLOWED_CIDRS", "100.64.0.0/10,not-a-cidr") + + if _, err := Load(); err == nil { + t.Fatal("Load() error = nil, want error for a malformed WEBHOOK_ALLOWED_CIDRS") + } + }) +} diff --git a/internal/service/webhook_sender.go b/internal/service/webhook_sender.go index 66224f3b..970701a6 100644 --- a/internal/service/webhook_sender.go +++ b/internal/service/webhook_sender.go @@ -37,20 +37,20 @@ type WebhookSenderRepository interface { // WebhookSenderImpl implements WebhookSender with Standard Webhooks conformance. type WebhookSenderImpl struct { - repo WebhookSenderRepository - httpClient *http.Client - metrics observability.WebhookMetrics - urlHostBlacklist map[string]struct{} + repo WebhookSenderRepository + httpClient *http.Client + metrics observability.WebhookMetrics + ssrfPolicy SSRFPolicy } // NewWebhookSenderImpl creates a sender that uses the given repo. -// urlHostBlacklist is the SSRF blacklist (hosts/IPs); may be nil (address checks still run). +// ssrfPolicy restricts which hosts may be dialed; its zero value still rejects private/reserved ranges. // httpTimeout is the HTTP client timeout; job timeout should be httpTimeout + buffer (e.g. 5s). // Client does not follow redirects and validates resolved IPs at dial time (DNS rebinding protection). // metrics may be nil when metrics are disabled. // If httpClient is non-nil, it is used as-is (e.g. for tests that hit loopback); otherwise a secured client is built. func NewWebhookSenderImpl( - repo WebhookSenderRepository, metrics observability.WebhookMetrics, urlHostBlacklist map[string]struct{}, + repo WebhookSenderRepository, metrics observability.WebhookMetrics, ssrfPolicy SSRFPolicy, httpTimeout time.Duration, httpClient *http.Client, ) *WebhookSenderImpl { if httpClient == nil { @@ -62,7 +62,7 @@ func NewWebhookSenderImpl( return nil, fmt.Errorf("invalid address %q: %w", addr, err) } - allowed, err := resolveWebhookHost(ctx, host, urlHostBlacklist) + allowed, err := resolveWebhookHost(ctx, host, ssrfPolicy) if err != nil { return nil, err } @@ -95,10 +95,10 @@ func NewWebhookSenderImpl( } return &WebhookSenderImpl{ - repo: repo, - httpClient: httpClient, - metrics: metrics, - urlHostBlacklist: urlHostBlacklist, + repo: repo, + httpClient: httpClient, + metrics: metrics, + ssrfPolicy: ssrfPolicy, } } diff --git a/internal/service/webhook_sender_test.go b/internal/service/webhook_sender_test.go index 1e576135..4d93d0b6 100644 --- a/internal/service/webhook_sender_test.go +++ b/internal/service/webhook_sender_test.go @@ -66,7 +66,7 @@ func TestWebhookSenderImpl_Send(t *testing.T) { repo := &mockSenderRepo{} // Use default client for tests (hits loopback httptest server). client := &http.Client{Timeout: 5 * time.Second} - sender := NewWebhookSenderImpl(repo, nil, nil, 5*time.Second, client) + sender := NewWebhookSenderImpl(repo, nil, SSRFPolicy{}, 5*time.Second, client) payload := &WebhookPayload{ ID: uuid.Must(uuid.NewV7()), Type: "feedback_record.created", @@ -95,7 +95,7 @@ func TestWebhookSenderImpl_Send(t *testing.T) { repo := &mockSenderRepo{} // Use default client for tests (hits loopback httptest server). client := &http.Client{Timeout: 5 * time.Second} - sender := NewWebhookSenderImpl(repo, nil, nil, 5*time.Second, client) + sender := NewWebhookSenderImpl(repo, nil, SSRFPolicy{}, 5*time.Second, client) payload := &WebhookPayload{ID: uuid.Must(uuid.NewV7()), Type: "test", Timestamp: time.Now(), Data: nil} err := sender.Send(ctx, webhook, payload) @@ -119,7 +119,7 @@ func TestWebhookSenderImpl_Send(t *testing.T) { repo := &mockSenderRepo{} // Use default client for tests (hits loopback httptest server). client := &http.Client{Timeout: 5 * time.Second} - sender := NewWebhookSenderImpl(repo, nil, nil, 5*time.Second, client) + sender := NewWebhookSenderImpl(repo, nil, SSRFPolicy{}, 5*time.Second, client) payload := &WebhookPayload{ID: uuid.Must(uuid.NewV7()), Type: "test", Timestamp: time.Now(), Data: nil} err := sender.Send(ctx, webhook, payload) diff --git a/internal/service/webhook_ssrf.go b/internal/service/webhook_ssrf.go new file mode 100644 index 00000000..8387d19b --- /dev/null +++ b/internal/service/webhook_ssrf.go @@ -0,0 +1,121 @@ +package service + +import ( + "net/netip" + "strings" +) + +// SSRFPolicy holds the webhook URL restrictions applied at create/update time and again at dial time. +// The zero value still rejects private/reserved ranges; Blacklist and AllowedCIDRs are both optional. +type SSRFPolicy struct { + // Blacklist is a set of normalized hostnames/IPs that can never be used as webhook URLs. + // An entry here is an explicit deny and wins over AllowedCIDRs. + Blacklist map[string]struct{} + + // AllowedCIDRs re-permits specific private/reserved ranges that isPrivateOrReserved would + // otherwise block, for operators whose webhook receivers legitimately live on internal + // addresses (e.g. a tailnet in 100.64.0.0/10). It does not override Blacklist. + AllowedCIDRs []netip.Prefix +} + +// NewSSRFPolicy builds a policy from config primitives. Both arguments may be nil/empty; the +// resulting policy still rejects private/reserved ranges. +func NewSSRFPolicy(blacklist map[string]struct{}, allowedCIDRs []netip.Prefix) SSRFPolicy { + return SSRFPolicy{Blacklist: blacklist, AllowedCIDRs: allowedCIDRs} +} + +// blocked returns true if host (a canonicalized hostname or IP string) is explicitly denied. +func (p SSRFPolicy) blocked(host string) bool { + if p.Blacklist == nil { + return false + } + + _, found := p.Blacklist[host] + + return found +} + +// allows returns true if addr falls in an operator-configured allowlist range. +func (p SSRFPolicy) allows(addr netip.Addr) bool { + addr = addr.Unmap() + + for _, prefix := range p.AllowedCIDRs { + if prefix.Contains(addr) { + return true + } + } + + return false +} + +// permits reports whether addr may be used as a webhook target under this policy. +// The explicit blacklist is checked first so it always wins, then the allowlist, then the ranges. +func (p SSRFPolicy) permits(addr netip.Addr) bool { + addr = addr.Unmap() + + if p.blocked(addr.String()) { + return false + } + + return p.allows(addr) || !isPrivateOrReserved(addr) +} + +// blockedPrefixes are private/reserved ranges that Go's netip predicates do not model. +// +// netip covers RFC1918 + fc00::/7 (IsPrivate), 127/8 + ::1 (IsLoopback), 169.254/16 + fe80::/10 +// (IsLinkLocalUnicast), 224/4 + ff00::/8 (IsMulticast) and the single address 0.0.0.0 / :: +// (IsUnspecified) — so everything below has to be matched as a CIDR instead. Kept as prefixes +// rather than predicates because a string-prefix or regex classifier is what let these leak in +// the first place (see ENG-2310, ENG-1326). +var blockedPrefixes = []netip.Prefix{ + // IPv4 + netip.MustParsePrefix("0.0.0.0/8"), // "this network" — IsUnspecified only matches 0.0.0.0 itself + netip.MustParsePrefix("100.64.0.0/10"), // CGNAT / shared address space (RFC 6598) — Tailscale tailnets + netip.MustParsePrefix("192.0.0.0/24"), // IETF protocol assignments + netip.MustParsePrefix("192.0.2.0/24"), // TEST-NET-1 (documentation) + netip.MustParsePrefix("198.51.100.0/24"), // TEST-NET-2 (documentation) + netip.MustParsePrefix("203.0.113.0/24"), // TEST-NET-3 (documentation) + netip.MustParsePrefix("198.18.0.0/15"), // benchmarking (RFC 2544) + netip.MustParsePrefix("240.0.0.0/4"), // reserved for future use + netip.MustParsePrefix("255.255.255.255/32"), // limited broadcast + netip.MustParsePrefix("168.63.129.16/32"), // Azure WireServer — sibling of IMDS, outside 169.254/16 + + // IPv6 transition ranges: these encode an IPv4 destination the predicates never see. + netip.MustParsePrefix("64:ff9b::/96"), // NAT64 well-known (64:ff9b::a9fe:a9fe == 169.254.169.254) + netip.MustParsePrefix("64:ff9b:1::/48"), // NAT64 local-use (RFC 8215) + netip.MustParsePrefix("2002::/16"), // 6to4 (2002:7f00:1::1 == 127.0.0.1) + netip.MustParsePrefix("fec0::/10"), // deprecated site-local, just above fe80::/10 +} + +// isPrivateOrReserved returns true if the IP is loopback, private, link-local, multicast, +// unspecified, or falls in one of the reserved ranges netip has no predicate for. +// +// The netip predicates are kept and the CIDR list only adds to them: IsLinkLocalUnicast covers +// the whole of fe80::/10, which a "fe80:" string-prefix check would not, so replacing the +// predicates with a prefix table would open a gap rather than close one. +func isPrivateOrReserved(addr netip.Addr) bool { + addr = addr.Unmap() + + // IsMulticast is a superset of IsLinkLocalMulticast (224/4 and ff00::/8 in full), which is why + // the previous IsLinkLocalMulticast-only check leaked every multicast scope above ff02::. + if addr.IsLoopback() || addr.IsPrivate() || addr.IsLinkLocalUnicast() || + addr.IsMulticast() || addr.IsUnspecified() { + return true + } + + for _, prefix := range blockedPrefixes { + if prefix.Contains(addr) { + return true + } + } + + return false +} + +// canonicalizeHost normalizes host for blacklist lookup (trim trailing dots, lowercase). +func canonicalizeHost(host string) string { + h := strings.TrimSpace(strings.ToLower(host)) + h = strings.TrimRight(h, ".") + + return h +} diff --git a/internal/service/webhook_ssrf_test.go b/internal/service/webhook_ssrf_test.go new file mode 100644 index 00000000..22ef2fbb --- /dev/null +++ b/internal/service/webhook_ssrf_test.go @@ -0,0 +1,169 @@ +package service + +import ( + "net/netip" + "testing" +) + +// TestIsPrivateOrReserved covers the classifier directly, including every range that Go's netip +// predicates do not model (ENG-2310) and the public controls that must keep working — an +// over-broad prefix is as much a bug as a missing one. +func TestIsPrivateOrReserved(t *testing.T) { + tests := []struct { + addr string + want bool + why string + }{ + // Covered by the netip predicates. These are regression cases: the predicates must be + // kept alongside the CIDR list, not replaced by it. + {"127.0.0.1", true, "loopback"}, + {"127.5.5.5", true, "loopback, whole /8"}, + {"10.0.0.5", true, "RFC1918 10/8"}, + {"172.16.0.1", true, "RFC1918 172.16/12"}, + {"192.168.1.1", true, "RFC1918 192.168/16"}, + {"169.254.169.254", true, "link-local, cloud metadata (IMDS)"}, + {"0.0.0.0", true, "unspecified"}, + {"::1", true, "IPv6 loopback"}, + {"fd00::1", true, "IPv6 ULA fc00::/7"}, + {"fe80::1", true, "IPv6 link-local, bottom of fe80::/10"}, + // fe80::/10 spans fe80:: through febf::. A "fe80:" string-prefix classifier (as used on the + // Formbricks side) covers only fe80::/16 and would let these two through; IsLinkLocalUnicast + // covers the full range. Guards against "porting" that classifier over here. + {"fe9f::1", true, "IPv6 link-local, middle of fe80::/10"}, + {"febf::1", true, "IPv6 link-local, top of fe80::/10"}, + {"::ffff:127.0.0.1", true, "IPv4-mapped loopback"}, + {"::ffff:10.0.0.1", true, "IPv4-mapped RFC1918"}, + + // Multicast: IsLinkLocalMulticast alone only caught ff02:: and 224.0.0.0/24, so every + // other scope leaked. IsMulticast covers 224/4 and ff00::/8 in full. + {"224.0.0.1", true, "IPv4 multicast, link-local scope"}, + {"224.0.1.1", true, "IPv4 multicast, outside 224.0.0.0/24"}, + {"225.0.0.1", true, "IPv4 multicast, outside 224/8"}, + {"239.255.255.250", true, "IPv4 multicast, SSDP"}, + {"ff01::1", true, "IPv6 multicast, interface-local scope"}, + {"ff02::1", true, "IPv6 multicast, link-local scope"}, + {"ff05::1", true, "IPv6 multicast, site-local scope"}, + {"ff0e::1", true, "IPv6 multicast, global scope"}, + + // The CIDR list: ranges netip has no predicate for. + {"0.1.2.3", true, "0.0.0.0/8 — IsUnspecified only matches 0.0.0.0 itself"}, + {"100.64.0.1", true, "CGNAT bottom — Tailscale tailnets"}, + {"100.100.100.100", true, "CGNAT — Tailscale MagicDNS"}, + {"100.127.255.254", true, "CGNAT top"}, + {"192.0.0.1", true, "IETF protocol assignments"}, + {"192.0.2.5", true, "TEST-NET-1"}, + {"198.51.100.5", true, "TEST-NET-2"}, + {"203.0.113.5", true, "TEST-NET-3"}, + {"198.18.0.1", true, "benchmarking 198.18/15"}, + {"198.19.255.254", true, "benchmarking, top of /15"}, + {"240.0.0.1", true, "reserved 240/4"}, + {"250.1.2.3", true, "reserved 240/4, mid-range"}, + {"255.255.255.255", true, "limited broadcast"}, + {"168.63.129.16", true, "Azure WireServer, outside 169.254/16"}, + {"64:ff9b::a9fe:a9fe", true, "NAT64 encoding of 169.254.169.254"}, + {"64:ff9b::7f00:1", true, "NAT64 encoding of 127.0.0.1"}, + {"64:ff9b:1::1", true, "NAT64 local-use (RFC 8215)"}, + {"2002:7f00:1::1", true, "6to4 encoding of 127.0.0.1"}, + {"fec0::1", true, "deprecated IPv6 site-local"}, + + // Must stay reachable. These catch a prefix written one bit too wide. + {"8.8.8.8", false, "public DNS"}, + {"93.184.216.34", false, "public (example.com)"}, + {"100.128.0.1", false, "public, immediately above CGNAT"}, + {"100.63.255.255", false, "public, immediately below CGNAT"}, + {"172.32.0.1", false, "public, immediately above RFC1918 172.16/12"}, + {"192.0.3.1", false, "public, immediately above TEST-NET-1"}, + {"198.20.0.1", false, "public, immediately above benchmarking"}, + {"239.255.255.249", true, "still multicast (boundary sanity)"}, + {"2606:2800:220:1:248:1893:25c8:1946", false, "public IPv6 (example.com)"}, + {"2003::1", false, "public IPv6, immediately above 6to4"}, + {"64:ff9c::1", false, "public IPv6, immediately above NAT64 well-known"}, + } + + for _, tt := range tests { + t.Run(tt.addr, func(t *testing.T) { + addr, err := netip.ParseAddr(tt.addr) + if err != nil { + t.Fatalf("bad test address %q: %v", tt.addr, err) + } + + if got := isPrivateOrReserved(addr); got != tt.want { + t.Errorf("isPrivateOrReserved(%s) = %v, want %v (%s)", tt.addr, got, tt.want, tt.why) + } + }) + } +} + +// TestSSRFPolicy_Permits covers how the blacklist, the allowlist and the range check compose. +func TestSSRFPolicy_Permits(t *testing.T) { + tailnet := netip.MustParsePrefix("100.64.0.0/10") + + tests := []struct { + name string + policy SSRFPolicy + addr string + want bool + }{ + { + name: "zero policy still blocks reserved ranges", + policy: SSRFPolicy{}, + addr: "100.64.0.1", + want: false, + }, + { + name: "zero policy allows public", + policy: SSRFPolicy{}, + addr: "8.8.8.8", + want: true, + }, + { + name: "allowlist re-permits a configured range", + policy: SSRFPolicy{AllowedCIDRs: []netip.Prefix{tailnet}}, + addr: "100.64.0.1", + want: true, + }, + { + name: "allowlist does not leak beyond its range", + policy: SSRFPolicy{AllowedCIDRs: []netip.Prefix{tailnet}}, + addr: "10.0.0.1", + want: false, + }, + { + // An operator-set denylist entry is an explicit deny and must win, otherwise a broad + // allowlist would silently re-open a host the operator named. + name: "blacklist beats allowlist", + policy: SSRFPolicy{ + Blacklist: map[string]struct{}{"100.64.0.1": {}}, + AllowedCIDRs: []netip.Prefix{tailnet}, + }, + addr: "100.64.0.1", + want: false, + }, + { + name: "blacklist blocks an otherwise-public address", + policy: SSRFPolicy{ + Blacklist: map[string]struct{}{"8.8.8.8": {}}, + }, + addr: "8.8.8.8", + want: false, + }, + { + // The allowlist is matched after Unmap, so a mapped form of an allowed address resolves + // the same way its IPv4 form does. + name: "allowlist matches IPv4-mapped form", + policy: SSRFPolicy{AllowedCIDRs: []netip.Prefix{tailnet}}, + addr: "::ffff:100.64.0.1", + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + addr := netip.MustParseAddr(tt.addr) + + if got := tt.policy.permits(addr); got != tt.want { + t.Errorf("permits(%s) = %v, want %v", tt.addr, got, tt.want) + } + }) + } +} diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index 74288de5..de187941 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -37,22 +37,23 @@ type WebhooksRepository interface { // WebhooksService handles business logic for webhooks. type WebhooksService struct { - repo WebhooksRepository - publisher MessagePublisher - maxWebhooks int - urlHostBlacklist map[string]struct{} + repo WebhooksRepository + publisher MessagePublisher + maxWebhooks int + ssrfPolicy SSRFPolicy } // NewWebhooksService creates a new webhooks service. -// urlHostBlacklist is a set of hostnames/IPs that cannot be used as webhook URLs (SSRF mitigation); may be nil for no restriction. +// ssrfPolicy restricts which hosts may be used as webhook URLs (SSRF mitigation); its zero value +// still rejects private/reserved ranges. func NewWebhooksService( - repo WebhooksRepository, publisher MessagePublisher, maxWebhooks int, urlHostBlacklist map[string]struct{}, + repo WebhooksRepository, publisher MessagePublisher, maxWebhooks int, ssrfPolicy SSRFPolicy, ) *WebhooksService { return &WebhooksService{ - repo: repo, - publisher: publisher, - maxWebhooks: maxWebhooks, - urlHostBlacklist: urlHostBlacklist, + repo: repo, + publisher: publisher, + maxWebhooks: maxWebhooks, + ssrfPolicy: ssrfPolicy, } } @@ -71,7 +72,7 @@ func (s *WebhooksService) CreateWebhook(ctx context.Context, req *models.CreateW return nil, huberrors.NewLimitExceededError(fmt.Sprintf("webhook limit reached (max %d)", s.maxWebhooks)) } - if err := validateWebhookURLHost(ctx, req.URL, s.urlHostBlacklist); err != nil { + if err := validateWebhookURLHost(ctx, req.URL, s.ssrfPolicy); err != nil { return nil, err } @@ -114,107 +115,28 @@ func validateSigningKey(key string) error { // SigningKeySize is the number of random bytes for Standard Webhooks signing keys. const SigningKeySize = 32 -// canonicalizeHost normalizes host for blacklist lookup (trim trailing dots, lowercase). -func canonicalizeHost(host string) string { - h := strings.TrimSpace(strings.ToLower(host)) - h = strings.TrimRight(h, ".") - - return h -} - -// isPrivateOrReserved returns true if the IP is loopback, private, link-local, or unspecified. -func isPrivateOrReserved(addr netip.Addr) bool { - addr = addr.Unmap() - - return addr.IsLoopback() || addr.IsPrivate() || addr.IsLinkLocalUnicast() || - addr.IsLinkLocalMulticast() || addr.IsUnspecified() -} - -// validateWebhookHost checks that the host (IP or hostname) is allowed for webhook URLs (SSRF mitigation). -// For literal IPs: rejects private/reserved ranges. For hostnames: resolves and rejects if any returned IP is disallowed. -// Always runs address checks; blacklist is applied when non-nil. -func validateWebhookHost(ctx context.Context, host string, blacklist map[string]struct{}) error { - host = canonicalizeHost(host) - if host == "" { - return huberrors.NewValidationError("url", "webhook URL host is empty") - } - - if blacklist != nil { - if _, blocked := blacklist[host]; blocked { - return huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") - } - } - - if addr, parseErr := netip.ParseAddr(host); parseErr == nil { - if isPrivateOrReserved(addr) { - return huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") - } - - if blacklist != nil { - if _, blocked := blacklist[addr.String()]; blocked { - return huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") - } - } - - return nil - } - - ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) - if err != nil { - return huberrors.NewValidationError("url", "cannot resolve webhook URL host: "+err.Error()) - } - - if len(ips) == 0 { - return huberrors.NewValidationError("url", "webhook URL host resolves to no addresses") - } - - for _, ipa := range ips { - addr, ok := netip.AddrFromSlice(ipa.IP) - if !ok { - continue - } - - addr = addr.Unmap() - if isPrivateOrReserved(addr) { - return huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") - } - - if blacklist != nil { - if _, blocked := blacklist[addr.String()]; blocked { - return huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") - } - } - } - - return nil -} - -// resolveWebhookHost resolves the host to allowed IPs for connection (DNS rebinding protection). -// Returns the list of IPs that pass validation, or an error if any resolved IP is disallowed. -func resolveWebhookHost(ctx context.Context, host string, blacklist map[string]struct{}) ([]netip.Addr, error) { +// resolveWebhookHost resolves the host to the IPs allowed for connection (SSRF mitigation). +// For a literal IP: rejects private/reserved ranges. For a hostname: resolves and rejects if ANY +// returned IP is disallowed, so a name that mixes public and internal answers cannot be used. +// +// The returned addresses are what the dialer must connect to — pinning them is what closes the +// DNS-rebinding window between validation and the request (see webhook_sender.go). +func resolveWebhookHost(ctx context.Context, host string, policy SSRFPolicy) ([]netip.Addr, error) { host = canonicalizeHost(host) if host == "" { return nil, huberrors.NewValidationError("url", "webhook URL host is empty") } - if blacklist != nil { - if _, blocked := blacklist[host]; blocked { - return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") - } + if policy.blocked(host) { + return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") } if addr, parseErr := netip.ParseAddr(host); parseErr == nil { - if isPrivateOrReserved(addr) { + if !policy.permits(addr) { return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") } - if blacklist != nil { - if _, blocked := blacklist[addr.String()]; blocked { - return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") - } - } - - return []netip.Addr{addr}, nil + return []netip.Addr{addr.Unmap()}, nil } ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) @@ -234,18 +156,11 @@ func resolveWebhookHost(ctx context.Context, host string, blacklist map[string]s continue } - addr = addr.Unmap() - if isPrivateOrReserved(addr) { + if !policy.permits(addr) { return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") } - if blacklist != nil { - if _, blocked := blacklist[addr.String()]; blocked { - return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") - } - } - - allowed = append(allowed, addr) + allowed = append(allowed, addr.Unmap()) } if len(allowed) == 0 { @@ -255,16 +170,23 @@ func resolveWebhookHost(ctx context.Context, host string, blacklist map[string]s return allowed, nil } +// validateWebhookHost checks that the host (IP or hostname) is allowed for webhook URLs. +// Thin wrapper over resolveWebhookHost that discards the addresses, so create/update-time +// validation and dial-time validation are provably the same check and cannot drift apart. +func validateWebhookHost(ctx context.Context, host string, policy SSRFPolicy) error { + _, err := resolveWebhookHost(ctx, host, policy) + + return err +} + // validateWebhookURLHost checks that the URL's host is allowed for webhooks (SSRF mitigation). -func validateWebhookURLHost(ctx context.Context, urlStr string, blacklist map[string]struct{}) error { +func validateWebhookURLHost(ctx context.Context, urlStr string, policy SSRFPolicy) error { u, err := url.Parse(urlStr) if err != nil { return huberrors.NewValidationError("url", "invalid URL: "+err.Error()) } - host := u.Hostname() - - return validateWebhookHost(ctx, host, blacklist) + return validateWebhookHost(ctx, u.Hostname(), policy) } // generateSigningKey generates a cryptographically secure signing key @@ -355,7 +277,7 @@ func (s *WebhooksService) UpdateWebhook(ctx context.Context, id uuid.UUID, req * } if req.URL != nil { - if err := validateWebhookURLHost(ctx, *req.URL, s.urlHostBlacklist); err != nil { + if err := validateWebhookURLHost(ctx, *req.URL, s.ssrfPolicy); err != nil { return nil, err } } diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index f29f58b1..ef60aed2 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -3,6 +3,7 @@ package service import ( "context" "errors" + "net/netip" "strings" "testing" "time" @@ -23,7 +24,9 @@ type mockWebhooksRepo struct { } func (m *mockWebhooksRepo) Create(_ context.Context, _ *models.CreateWebhookRequest) (*models.Webhook, error) { - return nil, nil + // Returns the seeded webhook when set, so tests that exercise a *successful* create have + // something to publish; a real repository never returns (nil, nil). + return m.webhook, nil } func (m *mockWebhooksRepo) GetByID(_ context.Context, _ uuid.UUID) (*models.Webhook, error) { @@ -100,7 +103,7 @@ func (p *capturePublisher) PublishEventWithChangedFields( func TestWebhooksService_CreateWebhook_InvalidSigningKey(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{}) tenantID := "org-123" req := &models.CreateWebhookRequest{ @@ -118,7 +121,7 @@ func TestWebhooksService_CreateWebhook_InvalidSigningKey(t *testing.T) { func TestWebhooksService_UpdateWebhook_InvalidSigningKey(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{}) id := uuid.Must(uuid.NewV7()) badKey := "bad_key" req := &models.UpdateWebhookRequest{ @@ -142,7 +145,7 @@ var ssrfBlacklist = map[string]struct{}{ func TestWebhooksService_CreateWebhook_RejectsSSRFHosts(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, ssrfBlacklist) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{Blacklist: ssrfBlacklist}) validKey := "whsec_" + "abcdefghijklmnopqrstuvwxyz123456" tenantID := "org-123" @@ -182,7 +185,7 @@ func TestWebhooksService_CreateWebhook_RejectsSSRFHosts(t *testing.T) { func TestWebhooksService_CreateWebhook_RequiresTenantID(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{}) req := &models.CreateWebhookRequest{ URL: "https://example.com/webhook", @@ -198,7 +201,7 @@ func TestWebhooksService_CreateWebhook_RequiresTenantID(t *testing.T) { func TestWebhooksService_UpdateWebhook_RejectsEmptyTenantID(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, nil) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{}) id := uuid.Must(uuid.NewV7()) tenantID := " " @@ -216,7 +219,7 @@ func TestWebhooksService_DeleteWebhook_PublishesTenantAwareDeletedEvent(t *testi tenantID := "org-123" repo := &mockWebhooksRepo{deleted: &models.DeletedWebhook{ID: webhookID, TenantID: &tenantID}} publisher := &capturePublisher{} - svc := NewWebhooksService(repo, publisher, 10, nil) + svc := NewWebhooksService(repo, publisher, 10, SSRFPolicy{}) err := svc.DeleteWebhook(ctx, webhookID) if err != nil { @@ -251,7 +254,7 @@ func TestWebhooksService_DeleteWebhook_PublishesTenantAwareDeletedEvent(t *testi func TestWebhooksService_UpdateWebhook_RejectsSSRFHosts(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, ssrfBlacklist) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{Blacklist: ssrfBlacklist}) id := uuid.Must(uuid.NewV7()) tests := []struct { @@ -276,3 +279,88 @@ func TestWebhooksService_UpdateWebhook_RejectsSSRFHosts(t *testing.T) { }) } } + +// TestWebhooksService_CreateWebhook_SSRFRangeCoverage drives the gap ranges through the real +// create path, so the classifier is proven where it is actually enforced and not just in isolation. +func TestWebhooksService_CreateWebhook_SSRFRangeCoverage(t *testing.T) { + ctx := context.Background() + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{Blacklist: ssrfBlacklist}) + validKey := "whsec_" + "abcdefghijklmnopqrstuvwxyz123456" + tenantID := "org-123" + + tests := []struct { + name string + url string + }{ + {"CGNAT / Tailscale", "https://100.64.0.1/webhook"}, + {"CGNAT MagicDNS", "https://100.100.100.100/webhook"}, + {"0.0.0.0/8", "https://0.1.2.3/webhook"}, + {"benchmarking", "https://198.18.0.1/webhook"}, + {"Azure WireServer", "https://168.63.129.16/webhook"}, + {"broadcast", "https://255.255.255.255/webhook"}, + {"multicast beyond 224/8", "https://239.255.255.250/webhook"}, + {"NAT64 to IMDS", "https://[64:ff9b::a9fe:a9fe]/webhook"}, + {"NAT64 to loopback", "https://[64:ff9b::7f00:1]/webhook"}, + {"NAT64 local-use", "https://[64:ff9b:1::1]/webhook"}, + {"6to4 to loopback", "https://[2002:7f00:1::1]/webhook"}, + {"IPv6 site-local", "https://[fec0::1]/webhook"}, + {"IPv6 multicast, site scope", "https://[ff05::1]/webhook"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := &models.CreateWebhookRequest{ + URL: tt.url, + SigningKey: validKey, + TenantID: &tenantID, + EventTypes: []datatypes.EventType{datatypes.FeedbackRecordCreated}, + } + + _, err := svc.CreateWebhook(ctx, req) + if !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("expected ErrValidation for %s, got %v", tt.url, err) + } + + var verr *huberrors.ValidationError + if errors.As(err, &verr) && !strings.Contains(verr.Message, "private/internal") { + t.Errorf("error message %q does not contain %q", verr.Message, "private/internal") + } + }) + } +} + +// TestWebhooksService_CreateWebhook_AllowedCIDR is the upgrade-break escape hatch: an operator +// whose receiver lives on a tailnet can re-permit that range without disabling SSRF defense. +func TestWebhooksService_CreateWebhook_AllowedCIDR(t *testing.T) { + ctx := context.Background() + validKey := "whsec_" + "abcdefghijklmnopqrstuvwxyz123456" + tenantID := "org-123" + + newReq := func(url string) *models.CreateWebhookRequest { + return &models.CreateWebhookRequest{ + URL: url, + SigningKey: validKey, + TenantID: &tenantID, + EventTypes: []datatypes.EventType{datatypes.FeedbackRecordCreated}, + } + } + + // Without the allowlist the tailnet address is rejected... + svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{}) + if _, err := svc.CreateWebhook(ctx, newReq("https://100.64.0.1/webhook")); !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("expected ErrValidation without allowlist, got %v", err) + } + + // ...and with it configured, the same URL is accepted. + allowed := SSRFPolicy{AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("100.64.0.0/10")}} + svc = NewWebhooksService(&mockWebhooksRepo{count: 0, webhook: &models.Webhook{}}, noopPublisher{}, 10, allowed) + + if _, err := svc.CreateWebhook(ctx, newReq("https://100.64.0.1/webhook")); err != nil { + t.Fatalf("expected the allowlisted range to be accepted, got %v", err) + } + + // The allowlist is scoped: other private ranges stay blocked. + if _, err := svc.CreateWebhook(ctx, newReq("https://10.0.0.1/webhook")); !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("expected RFC1918 to stay blocked with a CGNAT allowlist, got %v", err) + } +} diff --git a/tests/integration_test.go b/tests/integration_test.go index 22f53171..43c4be82 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -94,7 +94,10 @@ func setupTestServerWithEventProviders( // Webhooks webhooksRepo := repository.NewWebhooksRepository(db) - webhooksService := service.NewWebhooksService(webhooksRepo, messageManager, cfg.Webhook.MaxCount, cfg.Webhook.URLBlacklist) + webhooksService := service.NewWebhooksService( + webhooksRepo, messageManager, cfg.Webhook.MaxCount, + service.NewSSRFPolicy(cfg.Webhook.URLBlacklist, cfg.Webhook.AllowedCIDRs), + ) webhooksHandler := handlers.NewWebhooksHandler(webhooksService) // Initialize repository, service, and handler layers From 41644df0804ad2ac5c10acca884e0951719ac36e Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Fri, 21 Aug 2026 15:14:52 +0000 Subject: [PATCH 2/4] test: allowlist TEST-NET-1 for the webhook integration fixtures The integration fixtures target 192.0.2.1, which the widened SSRF classifier now correctly rejects as a reserved documentation range. Allowlist just that range in the harness rather than repointing the fixtures at a hostname: the literal keeps them hermetic (no DNS in the test path) and it exercises the new WEBHOOK_ALLOWED_CIDRS plumbing end to end. --- tests/integration_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/integration_test.go b/tests/integration_test.go index 43c4be82..15dd31cb 100644 --- a/tests/integration_test.go +++ b/tests/integration_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/netip" "net/url" "os" "sync" @@ -94,10 +95,16 @@ func setupTestServerWithEventProviders( // Webhooks webhooksRepo := repository.NewWebhooksRepository(db) - webhooksService := service.NewWebhooksService( - webhooksRepo, messageManager, cfg.Webhook.MaxCount, - service.NewSSRFPolicy(cfg.Webhook.URLBlacklist, cfg.Webhook.AllowedCIDRs), + // Webhook fixtures target 192.0.2.0/24 (TEST-NET-1), which the SSRF classifier rejects as a + // reserved range. Allowlisting just that range keeps the fixtures hermetic — literal IPs need + // no DNS — and exercises the WEBHOOK_ALLOWED_CIDRS path end to end. Every other reserved range + // stays blocked here; the classifier itself is covered in internal/service/webhook_ssrf_test.go. + ssrfPolicy := service.NewSSRFPolicy( + cfg.Webhook.URLBlacklist, + append(cfg.Webhook.AllowedCIDRs, netip.MustParsePrefix("192.0.2.0/24")), ) + + webhooksService := service.NewWebhooksService(webhooksRepo, messageManager, cfg.Webhook.MaxCount, ssrfPolicy) webhooksHandler := handlers.NewWebhooksHandler(webhooksService) // Initialize repository, service, and handler layers From b26d5534cdaba2c5ebd483b76a10b0ac811c8ae4 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Fri, 21 Aug 2026 17:32:29 +0000 Subject: [PATCH 3/4] fix: close an IPv6-zone bypass of the SSRF CIDR list, and 4 more ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review findings on the CIDR list added earlier in this branch. netip.Prefix.Contains is documented to return false for an address carrying an IPv6 zone, and url.Parse preserves the zone through u.Hostname() — so appending %25eth0 skipped every entry in blockedPrefixes: [64:ff9b::a9fe:a9fe%25eth0] reached IMDS. The netip predicates evaluate zoned addresses correctly, so loopback/RFC1918/link-local never leaked; only the CIDR-matched ranges did, which is to say the half this branch added. classify now rejects zoned addresses outright (a zone means "via this interface", never a valid webhook target) and isPrivateOrReserved strips the zone before the CIDR walk so it cannot fail open for any other caller. Also newly blocked, all the same class of IPv6-wrapped IPv4 destination the branch already targets: - ::/96 IPv4-compatible IPv6, deprecated (::7f00:1 == 127.0.0.1, ::a9fe:a9fe == IMDS). Unmap() does not collapse this, only ::ffff:0:0/96, and mapped public addresses stay reachable. - 2001::/32 Teredo, which tunnels IPv4 exactly as the 6to4 we already block - 100::/64 discard-only (RFC 6666) - 2001:db8::/32 documentation, the IPv6 analogue of the TEST-NETs above Two more review fixes: - The rejection reason was collapsed into one message when the two validators were merged, so a host the operator had put in WEBHOOK_BLACKLIST was reported as "private/internal". Verified against a live stack: a hostname resolving to a blacklisted public address now reports "blacklisted" again. - WEBHOOK_ALLOWED_CIDRS was silent. An allowlist re-opens internal ranges to anyone who can create a webhook, so NewSSRFPolicy logs a warning naming the ranges when one is in effect. Dropped the redundant 255.255.255.255/32 (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. --- internal/service/webhook_sender.go | 6 +- internal/service/webhook_ssrf.go | 105 ++++++++++++++--- internal/service/webhook_ssrf_test.go | 159 ++++++++++++++++++++++++++ internal/service/webhooks_service.go | 8 +- 4 files changed, 255 insertions(+), 23 deletions(-) diff --git a/internal/service/webhook_sender.go b/internal/service/webhook_sender.go index 970701a6..885129b0 100644 --- a/internal/service/webhook_sender.go +++ b/internal/service/webhook_sender.go @@ -40,11 +40,12 @@ type WebhookSenderImpl struct { repo WebhookSenderRepository httpClient *http.Client metrics observability.WebhookMetrics - ssrfPolicy SSRFPolicy } // NewWebhookSenderImpl creates a sender that uses the given repo. -// ssrfPolicy restricts which hosts may be dialed; its zero value still rejects private/reserved ranges. +// ssrfPolicy restricts which hosts may be dialed; its zero value still rejects private/reserved +// ranges. It is enforced in the transport's DialContext, so it is not retained on the struct — an +// injected httpClient (below) is expected to carry its own dialer. // httpTimeout is the HTTP client timeout; job timeout should be httpTimeout + buffer (e.g. 5s). // Client does not follow redirects and validates resolved IPs at dial time (DNS rebinding protection). // metrics may be nil when metrics are disabled. @@ -98,7 +99,6 @@ func NewWebhookSenderImpl( repo: repo, httpClient: httpClient, metrics: metrics, - ssrfPolicy: ssrfPolicy, } } diff --git a/internal/service/webhook_ssrf.go b/internal/service/webhook_ssrf.go index 8387d19b..0184fa5f 100644 --- a/internal/service/webhook_ssrf.go +++ b/internal/service/webhook_ssrf.go @@ -1,8 +1,11 @@ package service import ( + "log/slog" "net/netip" "strings" + + "github.com/formbricks/hub/internal/huberrors" ) // SSRFPolicy holds the webhook URL restrictions applied at create/update time and again at dial time. @@ -21,6 +24,18 @@ type SSRFPolicy struct { // NewSSRFPolicy builds a policy from config primitives. Both arguments may be nil/empty; the // resulting policy still rejects private/reserved ranges. func NewSSRFPolicy(blacklist map[string]struct{}, allowedCIDRs []netip.Prefix) SSRFPolicy { + // An allowlist re-opens internal ranges to anyone who can create a webhook, so it should never + // be in effect without being visible in the logs of the process enforcing it. + if len(allowedCIDRs) > 0 { + ranges := make([]string, 0, len(allowedCIDRs)) + for _, prefix := range allowedCIDRs { + ranges = append(ranges, prefix.String()) + } + + slog.Warn("webhook SSRF allowlist active: private/reserved ranges are reachable as webhook targets", + "allowed_cidrs", strings.Join(ranges, ",")) + } + return SSRFPolicy{Blacklist: blacklist, AllowedCIDRs: allowedCIDRs} } @@ -48,16 +63,63 @@ func (p SSRFPolicy) allows(addr netip.Addr) bool { return false } -// permits reports whether addr may be used as a webhook target under this policy. -// The explicit blacklist is checked first so it always wins, then the allowlist, then the ranges. -func (p SSRFPolicy) permits(addr netip.Addr) bool { +// ssrfRejection is why an address may not be used as a webhook target. +type ssrfRejection int + +const ( + ssrfAllowed ssrfRejection = iota + ssrfBlacklisted + ssrfPrivateOrReserved +) + +// validationError renders the rejection as the client-facing error, or nil when allowed. +// The two reasons stay distinct because they need different operator actions: a blacklist hit is +// something they configured, a range hit is not. +func (r ssrfRejection) validationError() error { + switch r { + case ssrfBlacklisted: + return huberrors.NewValidationError("url", "webhook URL host is not allowed (blacklisted)") + case ssrfPrivateOrReserved: + return huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") + case ssrfAllowed: + return nil + } + + return nil +} + +// classify reports whether addr may be used as a webhook target under this policy, and why not. +// The explicit blacklist is checked first so it always wins over the allowlist. +func (p SSRFPolicy) classify(addr netip.Addr) ssrfRejection { + // An IPv6 zone identifier ("fe80::1%eth0") is rejected outright. netip.Prefix.Contains is + // documented to return false for any zoned address, so a zone would silently skip every CIDR + // in blockedPrefixes — url.Parse preserves the zone, so `[64:ff9b::a9fe:a9fe%25eth0]` would + // otherwise reach IMDS. A zone means "via this specific interface", which is never a legitimate + // webhook target anyway. + if addr.Zone() != "" { + return ssrfPrivateOrReserved + } + addr = addr.Unmap() if p.blocked(addr.String()) { - return false + return ssrfBlacklisted + } + + if p.allows(addr) { + return ssrfAllowed + } + + if isPrivateOrReserved(addr) { + return ssrfPrivateOrReserved } - return p.allows(addr) || !isPrivateOrReserved(addr) + return ssrfAllowed +} + +// permits reports whether addr may be used as a webhook target under this policy. +func (p SSRFPolicy) permits(addr netip.Addr) bool { + return p.classify(addr) == ssrfAllowed } // blockedPrefixes are private/reserved ranges that Go's netip predicates do not model. @@ -67,24 +129,32 @@ func (p SSRFPolicy) permits(addr netip.Addr) bool { // (IsUnspecified) — so everything below has to be matched as a CIDR instead. Kept as prefixes // rather than predicates because a string-prefix or regex classifier is what let these leak in // the first place (see ENG-2310, ENG-1326). +// +// Note ::/96 (IPv4-compatible) does not collide with the IPv4-mapped range ::ffff:0:0/96, whose +// 6th group is ffff — Unmap() handles mapped addresses, and mapped public addresses stay allowed. var blockedPrefixes = []netip.Prefix{ // IPv4 - netip.MustParsePrefix("0.0.0.0/8"), // "this network" — IsUnspecified only matches 0.0.0.0 itself - netip.MustParsePrefix("100.64.0.0/10"), // CGNAT / shared address space (RFC 6598) — Tailscale tailnets - netip.MustParsePrefix("192.0.0.0/24"), // IETF protocol assignments - netip.MustParsePrefix("192.0.2.0/24"), // TEST-NET-1 (documentation) - netip.MustParsePrefix("198.51.100.0/24"), // TEST-NET-2 (documentation) - netip.MustParsePrefix("203.0.113.0/24"), // TEST-NET-3 (documentation) - netip.MustParsePrefix("198.18.0.0/15"), // benchmarking (RFC 2544) - netip.MustParsePrefix("240.0.0.0/4"), // reserved for future use - netip.MustParsePrefix("255.255.255.255/32"), // limited broadcast - netip.MustParsePrefix("168.63.129.16/32"), // Azure WireServer — sibling of IMDS, outside 169.254/16 + netip.MustParsePrefix("0.0.0.0/8"), // "this network" — IsUnspecified only matches 0.0.0.0 itself + netip.MustParsePrefix("100.64.0.0/10"), // CGNAT / shared address space (RFC 6598) — Tailscale tailnets + netip.MustParsePrefix("192.0.0.0/24"), // IETF protocol assignments + netip.MustParsePrefix("192.0.2.0/24"), // TEST-NET-1 (documentation) + netip.MustParsePrefix("198.51.100.0/24"), // TEST-NET-2 (documentation) + netip.MustParsePrefix("203.0.113.0/24"), // TEST-NET-3 (documentation) + netip.MustParsePrefix("198.18.0.0/15"), // benchmarking (RFC 2544) + netip.MustParsePrefix("240.0.0.0/4"), // reserved for future use, incl. 255.255.255.255 broadcast + netip.MustParsePrefix("168.63.129.16/32"), // Azure WireServer — sibling of IMDS, outside 169.254/16 // IPv6 transition ranges: these encode an IPv4 destination the predicates never see. + netip.MustParsePrefix("::/96"), // IPv4-compatible IPv6, deprecated (::7f00:1 == 127.0.0.1) netip.MustParsePrefix("64:ff9b::/96"), // NAT64 well-known (64:ff9b::a9fe:a9fe == 169.254.169.254) netip.MustParsePrefix("64:ff9b:1::/48"), // NAT64 local-use (RFC 8215) + netip.MustParsePrefix("2001::/32"), // Teredo — tunnels IPv4 the same way 6to4 does netip.MustParsePrefix("2002::/16"), // 6to4 (2002:7f00:1::1 == 127.0.0.1) netip.MustParsePrefix("fec0::/10"), // deprecated site-local, just above fe80::/10 + + // Reserved IPv6 ranges with no routable host, mirroring the IPv4 documentation ranges above. + netip.MustParsePrefix("100::/64"), // discard-only (RFC 6666) + netip.MustParsePrefix("2001:db8::/32"), // documentation (RFC 3849) } // isPrivateOrReserved returns true if the IP is loopback, private, link-local, multicast, @@ -94,7 +164,10 @@ var blockedPrefixes = []netip.Prefix{ // the whole of fe80::/10, which a "fe80:" string-prefix check would not, so replacing the // predicates with a prefix table would open a gap rather than close one. func isPrivateOrReserved(addr netip.Addr) bool { - addr = addr.Unmap() + // Strip any IPv6 zone before the CIDR walk: Prefix.Contains returns false for a zoned address, + // which would fail open. classify rejects zoned addresses before reaching here; this keeps the + // classifier correct on its own terms for any other caller. + addr = addr.Unmap().WithZone("") // IsMulticast is a superset of IsLinkLocalMulticast (224/4 and ff00::/8 in full), which is why // the previous IsLinkLocalMulticast-only check leaked every multicast scope above ff02::. diff --git a/internal/service/webhook_ssrf_test.go b/internal/service/webhook_ssrf_test.go index 22ef2fbb..58807bc1 100644 --- a/internal/service/webhook_ssrf_test.go +++ b/internal/service/webhook_ssrf_test.go @@ -1,8 +1,13 @@ package service import ( + "context" + "errors" "net/netip" + "strings" "testing" + + "github.com/formbricks/hub/internal/huberrors" ) // TestIsPrivateOrReserved covers the classifier directly, including every range that Go's netip @@ -65,6 +70,15 @@ func TestIsPrivateOrReserved(t *testing.T) { {"64:ff9b:1::1", true, "NAT64 local-use (RFC 8215)"}, {"2002:7f00:1::1", true, "6to4 encoding of 127.0.0.1"}, {"fec0::1", true, "deprecated IPv6 site-local"}, + // IPv4-compatible IPv6 (::/96) is the same trick as NAT64/6to4 — an IPv6 wrapper around an + // IPv4 destination — and Unmap() does not touch it, since it only collapses ::ffff:0:0/96. + {"::7f00:1", true, "IPv4-compatible ::127.0.0.1"}, + {"::127.0.0.1", true, "IPv4-compatible, dotted form"}, + {"::a9fe:a9fe", true, "IPv4-compatible ::169.254.169.254 (IMDS)"}, + {"::a00:1", true, "IPv4-compatible ::10.0.0.1"}, + {"2001:0:1234::1", true, "Teredo — tunnels IPv4 like 6to4"}, + {"2001:db8::1", true, "IPv6 documentation range"}, + {"100::1", true, "IPv6 discard-only prefix"}, // Must stay reachable. These catch a prefix written one bit too wide. {"8.8.8.8", false, "public DNS"}, @@ -78,6 +92,12 @@ func TestIsPrivateOrReserved(t *testing.T) { {"2606:2800:220:1:248:1893:25c8:1946", false, "public IPv6 (example.com)"}, {"2003::1", false, "public IPv6, immediately above 6to4"}, {"64:ff9c::1", false, "public IPv6, immediately above NAT64 well-known"}, + // ::/96 must not swallow the IPv4-mapped range, whose 6th group is ffff. + {"::ffff:93.184.216.34", false, "IPv4-mapped PUBLIC address"}, + // 2001::/32 is Teredo; the rest of 2001::/16 is ordinary global unicast. + {"2001:4860:4860::8888", false, "public IPv6 in 2001::/16 but outside Teredo"}, + {"2001:db9::1", false, "public IPv6, immediately above the documentation range"}, + {"101::1", false, "public IPv6, immediately above the discard prefix"}, } for _, tt := range tests { @@ -167,3 +187,142 @@ func TestSSRFPolicy_Permits(t *testing.T) { }) } } + +// TestSSRFPolicy_RejectionReason pins the two rejection reasons apart. They were briefly collapsed +// into one message while refactoring, which told an operator "private/internal" for a host they had +// themselves put in WEBHOOK_BLACKLIST — the opposite of actionable. +func TestSSRFPolicy_RejectionReason(t *testing.T) { + tests := []struct { + name string + policy SSRFPolicy + addr string + want ssrfRejection + wantMsg string + }{ + { + name: "reserved range", + policy: SSRFPolicy{}, + addr: "100.64.0.1", + want: ssrfPrivateOrReserved, + wantMsg: "private/internal", + }, + { + name: "blacklisted public address", + policy: SSRFPolicy{Blacklist: map[string]struct{}{"8.8.8.8": {}}}, + addr: "8.8.8.8", + want: ssrfBlacklisted, + wantMsg: "blacklisted", + }, + { + // The reason must survive the allowlist: the operator denied this address explicitly. + name: "blacklisted inside an allowlisted range", + policy: SSRFPolicy{ + Blacklist: map[string]struct{}{"100.64.0.1": {}}, + AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("100.64.0.0/10")}, + }, + addr: "100.64.0.1", + want: ssrfBlacklisted, + wantMsg: "blacklisted", + }, + { + name: "allowed", + policy: SSRFPolicy{}, + addr: "8.8.8.8", + want: ssrfAllowed, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.policy.classify(netip.MustParseAddr(tt.addr)) + if got != tt.want { + t.Fatalf("classify(%s) = %v, want %v", tt.addr, got, tt.want) + } + + err := got.validationError() + if tt.wantMsg == "" { + if err != nil { + t.Fatalf("validationError() = %v, want nil", err) + } + + return + } + + var verr *huberrors.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("validationError() = %v, want a *huberrors.ValidationError", err) + } + + if !strings.Contains(verr.Message, tt.wantMsg) { + t.Errorf("message %q does not contain %q", verr.Message, tt.wantMsg) + } + }) + } +} + +// TestSSRFPolicy_RejectsZonedAddresses covers a bypass of the CIDR list specifically. +// +// netip.Prefix.Contains is documented to return false for an address carrying an IPv6 zone, and +// Go's url.Parse preserves the zone through u.Hostname() — so `[64:ff9b::a9fe:a9fe%25eth0]` +// skipped every entry in blockedPrefixes and reached IMDS, while the netip predicates (loopback, +// RFC1918, link-local) kept working. Only the ranges added as CIDRs were affected. +func TestSSRFPolicy_RejectsZonedAddresses(t *testing.T) { + zoned := []struct { + addr string + // reservedWithoutZone is whether the address is still private/reserved once the zone is + // stripped. Only those must also be caught by isPrivateOrReserved on its own; a *public* + // address with a zone is rejected by classify, not by the range classifier. + reservedWithoutZone bool + }{ + {"::7f00:1%eth0", true}, // IPv4-compatible loopback + {"64:ff9b::a9fe:a9fe%eth0", true}, // NAT64 -> IMDS + {"64:ff9b::7f00:1%eth0", true}, // NAT64 -> loopback + {"2002:7f00:1::1%eth0", true}, // 6to4 -> loopback + {"fec0::1%eth0", true}, // site-local + {"2001:db8::1%eth0", true}, // documentation + {"fe80::1%eth0", true}, // link-local (already blocked by the predicate) + {"2606:2800:220:1:248:1893:25c8:1946%eth0", false}, // public; the zone alone disqualifies it + } + + for _, tt := range zoned { + t.Run(tt.addr, func(t *testing.T) { + addr, err := netip.ParseAddr(tt.addr) + if err != nil { + t.Fatalf("bad test address %q: %v", tt.addr, err) + } + + if addr.Zone() == "" { + t.Fatalf("test address %q lost its zone; the case no longer covers what it claims", tt.addr) + } + + if (SSRFPolicy{}).permits(addr) { + t.Errorf("permits(%s) = true, want false: a zoned address bypasses Prefix.Contains", tt.addr) + } + + // The range classifier must also hold on its own terms, so stripping the zone cannot + // fail open for any other caller. + if got := isPrivateOrReserved(addr); got != tt.reservedWithoutZone { + t.Errorf("isPrivateOrReserved(%s) = %v, want %v", tt.addr, got, tt.reservedWithoutZone) + } + }) + } +} + +// TestValidateWebhookURLHost_ZonedIPv6 drives the bypass through the real entry point, since it +// depends on url.Parse keeping the zone. +func TestValidateWebhookURLHost_ZonedIPv6(t *testing.T) { + ctx := context.Background() + + for _, raw := range []string{ + "https://[64:ff9b::a9fe:a9fe%25eth0]/webhook", + "https://[::7f00:1%25eth0]/webhook", + "https://[fec0::1%25eth0]/webhook", + } { + t.Run(raw, func(t *testing.T) { + err := validateWebhookURLHost(ctx, raw, SSRFPolicy{}) + if !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("validateWebhookURLHost(%q) = %v, want a validation error", raw, err) + } + }) + } +} diff --git a/internal/service/webhooks_service.go b/internal/service/webhooks_service.go index de187941..30765af2 100644 --- a/internal/service/webhooks_service.go +++ b/internal/service/webhooks_service.go @@ -132,8 +132,8 @@ func resolveWebhookHost(ctx context.Context, host string, policy SSRFPolicy) ([] } if addr, parseErr := netip.ParseAddr(host); parseErr == nil { - if !policy.permits(addr) { - return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") + if err := policy.classify(addr).validationError(); err != nil { + return nil, err } return []netip.Addr{addr.Unmap()}, nil @@ -156,8 +156,8 @@ func resolveWebhookHost(ctx context.Context, host string, policy SSRFPolicy) ([] continue } - if !policy.permits(addr) { - return nil, huberrors.NewValidationError("url", "webhook URL host is not allowed (private/internal)") + if err := policy.classify(addr).validationError(); err != nil { + return nil, err } allowed = append(allowed, addr.Unmap()) From c7b4b28391e41a28bb7e4b4c4c921fcde775b341 Mon Sep 17 00:00:00 2001 From: Tiago Farto Date: Wed, 26 Aug 2026 14:55:03 +0000 Subject: [PATCH 4/4] fix: block the remaining not-globally-reachable IPv6 ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review: blockedPrefixes omitted 5f00::/16 (SRv6 SIDs) and 3fff::/20 (documentation). IANA marks 5f00::/16 forwardable but not globally reachable, so an SRv6-enabled deployment kept an internal-destination path; both were accepted by isPrivateOrReserved and by resolveWebhookHost, and the secured dialer then treated the address as eligible. Adds four prefixes: - 5f00::/16 SRv6 SIDs (RFC 9602) — the reported gap - 3fff::/20 documentation (RFC 9637) — the reported gap - 2001:20::/28 ORCHIDv2 (RFC 7343) — same class, found by sweeping the registry rather than reading the list - ::ffff:0:0:0/96 IPv4-translated (RFC 2765 / SIIT), the sixth IPv4-wrapper format after IPv4-mapped, IPv4-compatible, NAT64, 6to4 and Teredo. Unmap() only collapses ::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 IPv6 list is now the IANA special-purpose registry filtered to Globally Reachable = False, minus what the netip predicates cover. That derivation is recorded above blockedPrefixes, along with why the Globally-Reachable = True entries are deliberately absent — 2001:30::/28 (DRIP) sits immediately above ORCHIDv2, so the /28 boundary is load-bearing and now has a control asserting it stays public. Tests: default rejection for every new range, top-of-range and either-side boundary controls, and allowlist round-trips through the real service path (blocked by default, reachable when named, one range's entry not re-opening another, blacklist still winning). 10 unit cases and 5 service cases go red without the prefix additions. Also gives two test fixtures a non-nil webhook. A URL that was wrongly *accepted* previously panicked on a nil result, which aborts the test binary and hides every remaining subtest — precisely when you need to see them. They now fail as clean assertions. --- internal/service/webhook_ssrf.go | 29 ++++++++--- internal/service/webhook_ssrf_test.go | 59 +++++++++++++++++++++++ internal/service/webhooks_service_test.go | 29 ++++++++++- 3 files changed, 107 insertions(+), 10 deletions(-) diff --git a/internal/service/webhook_ssrf.go b/internal/service/webhook_ssrf.go index 0184fa5f..9630e4f3 100644 --- a/internal/service/webhook_ssrf.go +++ b/internal/service/webhook_ssrf.go @@ -132,6 +132,13 @@ func (p SSRFPolicy) permits(addr netip.Addr) bool { // // Note ::/96 (IPv4-compatible) does not collide with the IPv4-mapped range ::ffff:0:0/96, whose // 6th group is ffff — Unmap() handles mapped addresses, and mapped public addresses stay allowed. +// +// The IPv6 entries are the IANA special-purpose registry filtered to Globally Reachable = False, +// minus what the 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. Note 2001:20::/28 (ORCHIDv2) stops short of +// 2001:30::/28 for exactly that reason. var blockedPrefixes = []netip.Prefix{ // IPv4 netip.MustParsePrefix("0.0.0.0/8"), // "this network" — IsUnspecified only matches 0.0.0.0 itself @@ -145,16 +152,22 @@ var blockedPrefixes = []netip.Prefix{ netip.MustParsePrefix("168.63.129.16/32"), // Azure WireServer — sibling of IMDS, outside 169.254/16 // IPv6 transition ranges: these encode an IPv4 destination the predicates never see. - netip.MustParsePrefix("::/96"), // IPv4-compatible IPv6, deprecated (::7f00:1 == 127.0.0.1) - netip.MustParsePrefix("64:ff9b::/96"), // NAT64 well-known (64:ff9b::a9fe:a9fe == 169.254.169.254) - netip.MustParsePrefix("64:ff9b:1::/48"), // NAT64 local-use (RFC 8215) - netip.MustParsePrefix("2001::/32"), // Teredo — tunnels IPv4 the same way 6to4 does - netip.MustParsePrefix("2002::/16"), // 6to4 (2002:7f00:1::1 == 127.0.0.1) - netip.MustParsePrefix("fec0::/10"), // deprecated site-local, just above fe80::/10 - - // Reserved IPv6 ranges with no routable host, mirroring the IPv4 documentation ranges above. + netip.MustParsePrefix("::/96"), // IPv4-compatible IPv6, deprecated (::7f00:1 == 127.0.0.1) + netip.MustParsePrefix("64:ff9b::/96"), // NAT64 well-known (64:ff9b::a9fe:a9fe == 169.254.169.254) + netip.MustParsePrefix("64:ff9b:1::/48"), // NAT64 local-use (RFC 8215) + netip.MustParsePrefix("2001::/32"), // Teredo — tunnels IPv4 the same way 6to4 does + netip.MustParsePrefix("2002::/16"), // 6to4 (2002:7f00:1::1 == 127.0.0.1) + netip.MustParsePrefix("::ffff:0:0:0/96"), // IPv4-translated, deprecated (RFC 2765/SIIT; ::ffff:0:7f00:1 == 127.0.0.1) + netip.MustParsePrefix("fec0::/10"), // deprecated site-local, just above fe80::/10 + + // Reserved IPv6 ranges that are not globally reachable, mirroring the IPv4 documentation ranges + // above. Most have no routable host at all; 5f00::/16 is the exception — IANA marks it + // forwardable, so an SRv6 deployment can route it to an internal destination. netip.MustParsePrefix("100::/64"), // discard-only (RFC 6666) + netip.MustParsePrefix("2001:20::/28"), // ORCHIDv2 (RFC 7343) netip.MustParsePrefix("2001:db8::/32"), // documentation (RFC 3849) + netip.MustParsePrefix("3fff::/20"), // documentation (RFC 9637) + netip.MustParsePrefix("5f00::/16"), // SRv6 SIDs (RFC 9602) — forwardable, so routable inside an SRv6 deployment } // isPrivateOrReserved returns true if the IP is loopback, private, link-local, multicast, diff --git a/internal/service/webhook_ssrf_test.go b/internal/service/webhook_ssrf_test.go index 58807bc1..90260b8a 100644 --- a/internal/service/webhook_ssrf_test.go +++ b/internal/service/webhook_ssrf_test.go @@ -79,6 +79,18 @@ func TestIsPrivateOrReserved(t *testing.T) { {"2001:0:1234::1", true, "Teredo — tunnels IPv4 like 6to4"}, {"2001:db8::1", true, "IPv6 documentation range"}, {"100::1", true, "IPv6 discard-only prefix"}, + // IPv4-translated (::ffff:0:0:0/96, RFC 2765/SIIT) is the sixth IPv4-wrapper format, after + // IPv4-mapped, IPv4-compatible, NAT64, 6to4 and Teredo. Unmap() only collapses ::ffff:0:0/96, + // so this one reaches the CIDR walk as ordinary global unicast. + {"::ffff:0:7f00:1", true, "IPv4-translated ::127.0.0.1"}, + {"::ffff:0:a9fe:a9fe", true, "IPv4-translated ::169.254.169.254 (IMDS)"}, + // The rest of the IANA special-purpose registry that is not globally reachable. + {"2001:20::1", true, "ORCHIDv2 (RFC 7343)"}, + {"2001:2f:ffff:ffff:ffff:ffff:ffff:ffff", true, "ORCHIDv2, top of the /28"}, + {"3fff::1", true, "documentation (RFC 9637)"}, + {"3fff:fff:ffff:ffff:ffff:ffff:ffff:ffff", true, "documentation, top of the /20"}, + {"5f00::1", true, "SRv6 SIDs (RFC 9602) — forwardable, so internally routable"}, + {"5f00:ffff::1", true, "SRv6 SIDs, top of the /16"}, // Must stay reachable. These catch a prefix written one bit too wide. {"8.8.8.8", false, "public DNS"}, @@ -98,6 +110,16 @@ func TestIsPrivateOrReserved(t *testing.T) { {"2001:4860:4860::8888", false, "public IPv6 in 2001::/16 but outside Teredo"}, {"2001:db9::1", false, "public IPv6, immediately above the documentation range"}, {"101::1", false, "public IPv6, immediately above the discard prefix"}, + // ::ffff:0:0:0/96 must not reach into the mapped range or past its own end. + {"::ffff:1:0:0", false, "public IPv6, immediately above IPv4-translated"}, + // The registry marks these Globally Reachable = True: public infrastructure, not internal. + // 2001:30::/28 in particular sits immediately above ORCHIDv2, so a /24 there would eat it. + {"2001:30::1", false, "DRIP (RFC 9153) — globally reachable"}, + {"2001:1f::1", false, "public IPv6, immediately below ORCHIDv2"}, + {"3ffe::1", false, "public IPv6, immediately below the 3fff::/20 documentation range"}, + {"4000::1", false, "public IPv6, immediately above the 3fff::/20 documentation range"}, + {"5eff::1", false, "public IPv6, immediately below SRv6 SIDs"}, + {"6000::1", false, "public IPv6, immediately above SRv6 SIDs"}, } for _, tt := range tests { @@ -175,6 +197,43 @@ func TestSSRFPolicy_Permits(t *testing.T) { addr: "::ffff:100.64.0.1", want: true, }, + { + // An SRv6 deployment whose webhook receiver genuinely sits on a SID needs a way back in. + // Blocked by default, reachable only when the operator names the range. + name: "allowlist re-permits SRv6 SIDs", + policy: SSRFPolicy{AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("5f00::/16")}}, + addr: "5f00::1", + want: true, + }, + { + name: "SRv6 SIDs blocked without an allowlist entry", + policy: SSRFPolicy{}, + addr: "5f00::1", + want: false, + }, + { + name: "allowlist re-permits the 3fff::/20 documentation range", + policy: SSRFPolicy{AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("3fff::/20")}}, + addr: "3fff::1", + want: true, + }, + { + // An allowlist entry for one new range must not re-open the others. + name: "allowlisting SRv6 does not re-open ORCHIDv2", + policy: SSRFPolicy{AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("5f00::/16")}}, + addr: "2001:20::1", + want: false, + }, + { + // The blacklist still wins for the newly-blocked ranges, same as everywhere else. + name: "blacklist beats an allowlist covering SRv6", + policy: SSRFPolicy{ + Blacklist: map[string]struct{}{"5f00::1": {}}, + AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("5f00::/16")}, + }, + addr: "5f00::1", + want: false, + }, } for _, tt := range tests { diff --git a/internal/service/webhooks_service_test.go b/internal/service/webhooks_service_test.go index ef60aed2..1a919f21 100644 --- a/internal/service/webhooks_service_test.go +++ b/internal/service/webhooks_service_test.go @@ -284,7 +284,13 @@ func TestWebhooksService_UpdateWebhook_RejectsSSRFHosts(t *testing.T) { // create path, so the classifier is proven where it is actually enforced and not just in isolation. func TestWebhooksService_CreateWebhook_SSRFRangeCoverage(t *testing.T) { ctx := context.Background() - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{Blacklist: ssrfBlacklist}) + // The repo returns a webhook so a URL that is wrongly *accepted* fails as a clean assertion + // rather than panicking on a nil result. A panic aborts the whole test binary and hides every + // remaining subtest — exactly when you most want to see them. + svc := NewWebhooksService( + &mockWebhooksRepo{count: 0, webhook: &models.Webhook{}}, + noopPublisher{}, 10, SSRFPolicy{Blacklist: ssrfBlacklist}, + ) validKey := "whsec_" + "abcdefghijklmnopqrstuvwxyz123456" tenantID := "org-123" @@ -305,6 +311,11 @@ func TestWebhooksService_CreateWebhook_SSRFRangeCoverage(t *testing.T) { {"6to4 to loopback", "https://[2002:7f00:1::1]/webhook"}, {"IPv6 site-local", "https://[fec0::1]/webhook"}, {"IPv6 multicast, site scope", "https://[ff05::1]/webhook"}, + {"IPv4-translated to loopback", "https://[::ffff:0:7f00:1]/webhook"}, + {"IPv4-translated to IMDS", "https://[::ffff:0:a9fe:a9fe]/webhook"}, + {"ORCHIDv2", "https://[2001:20::1]/webhook"}, + {"documentation (RFC 9637)", "https://[3fff::1]/webhook"}, + {"SRv6 SIDs", "https://[5f00::1]/webhook"}, } for _, tt := range tests { @@ -346,7 +357,7 @@ func TestWebhooksService_CreateWebhook_AllowedCIDR(t *testing.T) { } // Without the allowlist the tailnet address is rejected... - svc := NewWebhooksService(&mockWebhooksRepo{count: 0}, noopPublisher{}, 10, SSRFPolicy{}) + svc := NewWebhooksService(&mockWebhooksRepo{count: 0, webhook: &models.Webhook{}}, noopPublisher{}, 10, SSRFPolicy{}) if _, err := svc.CreateWebhook(ctx, newReq("https://100.64.0.1/webhook")); !errors.Is(err, huberrors.ErrValidation) { t.Fatalf("expected ErrValidation without allowlist, got %v", err) } @@ -359,6 +370,20 @@ func TestWebhooksService_CreateWebhook_AllowedCIDR(t *testing.T) { t.Fatalf("expected the allowlisted range to be accepted, got %v", err) } + // The same escape hatch has to work for the ranges added after review — an SRv6 deployment + // whose receiver sits on a SID is blocked by default and reachable only when named. + svc = NewWebhooksService(&mockWebhooksRepo{count: 0, webhook: &models.Webhook{}}, noopPublisher{}, 10, SSRFPolicy{}) + if _, err := svc.CreateWebhook(ctx, newReq("https://[5f00::1]/webhook")); !errors.Is(err, huberrors.ErrValidation) { + t.Fatalf("expected ErrValidation for an SRv6 SID without an allowlist, got %v", err) + } + + srv6 := SSRFPolicy{AllowedCIDRs: []netip.Prefix{netip.MustParsePrefix("5f00::/16")}} + svc = NewWebhooksService(&mockWebhooksRepo{count: 0, webhook: &models.Webhook{}}, noopPublisher{}, 10, srv6) + + if _, err := svc.CreateWebhook(ctx, newReq("https://[5f00::1]/webhook")); err != nil { + t.Fatalf("expected the allowlisted SRv6 range to be accepted, got %v", err) + } + // The allowlist is scoped: other private ranges stay blocked. if _, err := svc.CreateWebhook(ctx, newReq("https://10.0.0.1/webhook")); !errors.Is(err, huberrors.ErrValidation) { t.Fatalf("expected RFC1918 to stay blocked with a CGNAT allowlist, got %v", err)