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

.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

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"

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)

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.

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{}

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")
+		}
+	})
+}

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,
 	}
 }
 

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)