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..885129b0 100644 --- a/internal/service/webhook_sender.go +++ b/internal/service/webhook_sender.go @@ -37,20 +37,21 @@ 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 } // 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. 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. // 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 +63,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 +96,9 @@ func NewWebhookSenderImpl( } return &WebhookSenderImpl{ - repo: repo, - httpClient: httpClient, - metrics: metrics, - urlHostBlacklist: urlHostBlacklist, + repo: repo, + httpClient: httpClient, + metrics: metrics, } } 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..9630e4f3 --- /dev/null +++ b/internal/service/webhook_ssrf.go @@ -0,0 +1,207 @@ +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. +// 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 { + // 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} +} + +// 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 +} + +// 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 ssrfBlacklisted + } + + if p.allows(addr) { + return ssrfAllowed + } + + if isPrivateOrReserved(addr) { + return ssrfPrivateOrReserved + } + + 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. +// +// 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). +// +// 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 + 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("::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, +// 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 { + // 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::. + 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..90260b8a --- /dev/null +++ b/internal/service/webhook_ssrf_test.go @@ -0,0 +1,387 @@ +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 +// 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"}, + // 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"}, + // 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"}, + {"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"}, + // ::/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"}, + // ::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 { + 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, + }, + { + // 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 { + 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) + } + }) + } +} + +// 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 74288de5..30765af2 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) { - 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)") - } + if err := policy.classify(addr).validationError(); err != nil { + return nil, err } - 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) { - 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)") - } + if err := policy.classify(addr).validationError(); err != nil { + return nil, err } - 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..1a919f21 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,113 @@ 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() + // 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" + + 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"}, + {"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 { + 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, 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) + } + + // ...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 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) + } +} diff --git a/tests/integration_test.go b/tests/integration_test.go index 22f53171..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,7 +95,16 @@ func setupTestServerWithEventProviders( // Webhooks webhooksRepo := repository.NewWebhooksRepository(db) - webhooksService := service.NewWebhooksService(webhooksRepo, messageManager, cfg.Webhook.MaxCount, cfg.Webhook.URLBlacklist) + // 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