fix: enforce tenant scope on webhooks (#65)

* fix(webhooks): enforce tenant dispatch scope

Carry tenant_id through webhook dispatch jobs and re-check scope in the worker so tenant-scoped webhooks cannot receive another tenant's payload.

Allow worker and embedding backfill commands to use an explicitly configured local DATABASE_URL while still rejecting implicit defaults.

Refs ENG-796

* fix(webhooks): enforce tenant dispatch scope

Carry tenant_id through webhook dispatch jobs and re-check scope in the worker so tenant-scoped webhooks cannot receive another tenant's payload.

Allow worker and embedding backfill commands to use an explicitly configured local DATABASE_URL while still rejecting implicit defaults.

Refs ENG-796

* docs: fix AGENTS markdown spacing

* test(webhooks): keep repository scope test out of unit CI

* chore: restore tests workflow

* fix: tighten webhook tenant isolation

* chore: revert delete api

* chore: allow deleting specific tenant id

* chore: add gdpr exception

* chore: address pr comments

* chore: lint fix

* fix: harden webhook delete payload dispatch

Author: xernobyl

AGENTS.md

@@ -25,6 +25,19 @@
 - Prefer Go naming conventions (CamelCase for exported, lowerCamel for unexported).
 - Keep package names short and domain-focused (e.g., `repository`, `service`).
 
+## Multi-Tenancy & Data Isolation
+
+- Treat `tenant_id` as a security boundary, not a convenience filter. Tenant-owned data must never be read, enqueued, dispatched, cached, searched, embedded, or deleted across tenants.
+- Exception: GDPR/right-to-erasure flows may intentionally delete all records for a data subject across tenants when that is the documented API contract. Make that all-tenant behavior explicit in the API docs, service/repository names or comments, logs, and tests; do not reuse it for normal tenant-owned workflows.
+- When making a model, migration, API request, or repository change involving tenant-owned data, audit every downstream path that carries or derives from that data: handlers, services, repositories, message publishers, River job args, workers, webhook payloads, search, embeddings, bulk operations, logs, and metrics.
+- Tenant access rules must be consistent across every path that can observe, mutate, derive from, or act on the same resource. If one API endpoint, repository method, search path, webhook dispatch path, worker, backfill, bulk operation, or export path requires tenant scope, every alternate path for that resource must enforce the same tenant boundary.
+- Do not model `tenant_id` as an optional filter for tenant-owned resources. Prefer required tenant parameters in service/repository method signatures (`tenantID string`, not `*string`) unless the domain explicitly supports global resources and documents that behavior.
+- Prefer tenant-aware repository/service methods for tenant-owned workflows. Avoid adding broad helpers that return all enabled/all matching resources when the caller is dispatching, processing, deriving, exporting, or exposing tenant data.
+- Async jobs must carry the tenant boundary when the source data has one, and workers must re-check tenant scope before doing side effects. Do not rely only on enqueue-time filtering.
+- Global resources may intentionally have `tenant_id = NULL` only when the domain explicitly documents them as non-tenant-owned. Webhooks are tenant-owned and must require a non-empty `tenant_id`; a missing tenant on event data must not match any webhook.
+- When changing access rules for a tenant-owned model, search for all alternate access paths by resource name and by derived side effects: list, get, update, delete, bulk delete, webhook fan-out, River jobs, workers, embeddings, search, exports, cache invalidation, logs, and metrics.
+- For any tenant-scoping change, include verification at the boundary where the leak could happen: database query behavior, service fan-out, worker execution, and API behavior when relevant. Include at least one alternate-path regression test proving that data allowed through the primary path cannot leak through async dispatch, bulk operations, derived indexes, exports, or background workers. Tests are the evidence; the invariant belongs in the architecture.
+
 ## Testing Guidelines
 - Tests live under `tests/` and are run with `go test ./tests/...`.
 - Name test files `*_test.go` and test functions `TestXxx`.

internal/api/handlers/feedback_records_handler.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"log/slog"
 	"net/http"
 
 	"github.com/google/uuid"
@@ -24,7 +25,7 @@ type FeedbackRecordsService interface {
 	ListFeedbackRecords(ctx context.Context, filters *models.ListFeedbackRecordsFilters) (*models.ListFeedbackRecordsResponse, error)
 	UpdateFeedbackRecord(ctx context.Context, id uuid.UUID, req *models.UpdateFeedbackRecordRequest) (*models.FeedbackRecord, error)
 	DeleteFeedbackRecord(ctx context.Context, id uuid.UUID) error
-	BulkDeleteFeedbackRecords(ctx context.Context, userID string, tenantID *string) (int, error)
+	BulkDeleteFeedbackRecords(ctx context.Context, filters *models.BulkDeleteFilters) (int, error)
 }
 
 // FeedbackRecordsHandler handles HTTP requests for feedback records.
@@ -59,6 +60,12 @@ func (h *FeedbackRecordsHandler) Create(w http.ResponseWriter, r *http.Request)
 
 	record, err := h.service.CreateFeedbackRecord(r.Context(), &req)
 	if err != nil {
+		if errors.Is(err, huberrors.ErrValidation) {
+			validation.RespondValidationError(w, err)
+
+			return
+		}
+
 		if errors.Is(err, huberrors.ErrNotFound) {
 			response.RespondNotFound(w, "Feedback record not found")
 
@@ -219,7 +226,7 @@ func (h *FeedbackRecordsHandler) Delete(w http.ResponseWriter, r *http.Request)
 	w.WriteHeader(http.StatusNoContent)
 }
 
-// BulkDelete handles DELETE /v1/feedback-records?user_id=<id>.
+// BulkDelete handles DELETE /v1/feedback-records?user_id=<id>[&tenant_id=<id>].
 func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Request) {
 	filters := &models.BulkDeleteFilters{}
 
@@ -230,8 +237,27 @@ func (h *FeedbackRecordsHandler) BulkDelete(w http.ResponseWriter, r *http.Reque
 		return
 	}
 
-	deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters.UserID, filters.TenantID)
+	deletedCount, err := h.service.BulkDeleteFeedbackRecords(r.Context(), filters)
 	if err != nil {
+		if errors.Is(err, huberrors.ErrValidation) {
+			validation.RespondValidationError(w, err)
+
+			return
+		}
+
+		var tenantID string
+		if filters.TenantID != nil {
+			tenantID = *filters.TenantID
+		}
+
+		slog.Error("Failed to bulk delete feedback records", // #nosec G706 -- slog key-values
+			"method", r.Method,
+			"path", r.URL.Path,
+			"user_id", filters.UserID,
+			"tenant_id", tenantID,
+			"error", err,
+		)
+
 		response.RespondInternalServerError(w, "An unexpected error occurred")
 
 		return

internal/api/handlers/feedback_records_handler_test.go

@@ -1,6 +1,7 @@
 package handlers
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"net/http"
@@ -11,17 +12,23 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/formbricks/hub/internal/huberrors"
 	"github.com/formbricks/hub/internal/models"
 )
 
 // mockFeedbackRecordsService mocks FeedbackRecordsService for handler tests.
 type mockFeedbackRecordsService struct {
-	bulkDeleteFunc func(ctx context.Context, userID string, tenantID *string) (int, error)
+	createFunc     func(ctx context.Context, req *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error)
+	bulkDeleteFunc func(ctx context.Context, filters *models.BulkDeleteFilters) (int, error)
 }
 
 func (m *mockFeedbackRecordsService) CreateFeedbackRecord(
-	context.Context, *models.CreateFeedbackRecordRequest,
+	ctx context.Context, req *models.CreateFeedbackRecordRequest,
 ) (*models.FeedbackRecord, error) {
+	if m.createFunc != nil {
+		return m.createFunc(ctx, req)
+	}
+
 	return nil, nil
 }
 
@@ -45,9 +52,11 @@ func (m *mockFeedbackRecordsService) DeleteFeedbackRecord(context.Context, uuid.
 	return nil
 }
 
-func (m *mockFeedbackRecordsService) BulkDeleteFeedbackRecords(ctx context.Context, userID string, tenantID *string) (int, error) {
+func (m *mockFeedbackRecordsService) BulkDeleteFeedbackRecords(
+	ctx context.Context, filters *models.BulkDeleteFilters,
+) (int, error) {
 	if m.bulkDeleteFunc != nil {
-		return m.bulkDeleteFunc(ctx, userID, tenantID)
+		return m.bulkDeleteFunc(ctx, filters)
 	}
 
 	return 0, nil
@@ -67,11 +76,101 @@ func TestFeedbackRecordsHandler_List(t *testing.T) {
 	})
 }
 
+func TestFeedbackRecordsHandler_Create(t *testing.T) {
+	t.Run("success returns created record", func(t *testing.T) {
+		recordID := uuid.Must(uuid.NewV7())
+		mock := &mockFeedbackRecordsService{
+			createFunc: func(_ context.Context, req *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) {
+				assert.Equal(t, "org-123", req.TenantID)
+
+				return &models.FeedbackRecord{
+					ID:           recordID,
+					SourceType:   req.SourceType,
+					FieldID:      req.FieldID,
+					FieldType:    req.FieldType,
+					TenantID:     req.TenantID,
+					SubmissionID: req.SubmissionID,
+				}, nil
+			},
+		}
+		handler := NewFeedbackRecordsHandler(mock)
+
+		req := httptest.NewRequestWithContext(
+			context.Background(), http.MethodPost, "http://test/v1/feedback-records", feedbackRecordCreateBody(t, "org-123"),
+		)
+		rec := httptest.NewRecorder()
+
+		handler.Create(rec, req)
+
+		assert.Equal(t, http.StatusCreated, rec.Code)
+
+		var got models.FeedbackRecord
+
+		err := json.Unmarshal(rec.Body.Bytes(), &got)
+		require.NoError(t, err)
+		assert.Equal(t, recordID, got.ID)
+		assert.Equal(t, "org-123", got.TenantID)
+	})
+
+	t.Run("service validation error returns bad request", func(t *testing.T) {
+		mock := &mockFeedbackRecordsService{
+			createFunc: func(_ context.Context, _ *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) {
+				return nil, huberrors.NewValidationError("tenant_id", "tenant_id is required and cannot be empty")
+			},
+		}
+		handler := NewFeedbackRecordsHandler(mock)
+
+		req := httptest.NewRequestWithContext(
+			context.Background(), http.MethodPost, "http://test/v1/feedback-records", feedbackRecordCreateBody(t, "   "),
+		)
+		rec := httptest.NewRecorder()
+
+		handler.Create(rec, req)
+
+		assert.Equal(t, http.StatusBadRequest, rec.Code)
+		assert.Contains(t, rec.Header().Get("Content-Type"), "application/problem+json")
+	})
+
+	t.Run("service conflict returns conflict", func(t *testing.T) {
+		mock := &mockFeedbackRecordsService{
+			createFunc: func(_ context.Context, _ *models.CreateFeedbackRecordRequest) (*models.FeedbackRecord, error) {
+				return nil, huberrors.NewConflictError("duplicate feedback record")
+			},
+		}
+		handler := NewFeedbackRecordsHandler(mock)
+
+		req := httptest.NewRequestWithContext(
+			context.Background(), http.MethodPost, "http://test/v1/feedback-records", feedbackRecordCreateBody(t, "org-123"),
+		)
+		rec := httptest.NewRecorder()
+
+		handler.Create(rec, req)
+
+		assert.Equal(t, http.StatusConflict, rec.Code)
+	})
+}
+
+func feedbackRecordCreateBody(t *testing.T, tenantID string) *bytes.Reader {
+	t.Helper()
+
+	body, err := json.Marshal(map[string]any{
+		"source_type":   "formbricks",
+		"submission_id": "submission-1",
+		"tenant_id":     tenantID,
+		"field_id":      "feedback",
+		"field_type":    "text",
+	})
+	require.NoError(t, err)
+
+	return bytes.NewReader(body)
+}
+
 func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) {
 	t.Run("success returns 200 with deleted_count and message", func(t *testing.T) {
 		mock := &mockFeedbackRecordsService{
-			bulkDeleteFunc: func(_ context.Context, userID string, _ *string) (int, error) {
-				assert.Equal(t, "user-123", userID)
+			bulkDeleteFunc: func(_ context.Context, filters *models.BulkDeleteFilters) (int, error) {
+				assert.Equal(t, "user-123", filters.UserID)
+				assert.Nil(t, filters.TenantID)
 
 				return 3, nil
 			},
@@ -94,14 +193,12 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) {
 		assert.Equal(t, "Successfully deleted 3 feedback records", resp.Message)
 	})
 
-	t.Run("success with tenant_id passes tenant to service", func(t *testing.T) {
-		var capturedTenantID *string
-
+	t.Run("optional tenant_id query parameter is passed to service", func(t *testing.T) {
 		mock := &mockFeedbackRecordsService{
-			bulkDeleteFunc: func(_ context.Context, userID string, tenantID *string) (int, error) {
-				assert.Equal(t, "user-456", userID)
-
-				capturedTenantID = tenantID
+			bulkDeleteFunc: func(_ context.Context, filters *models.BulkDeleteFilters) (int, error) {
+				assert.Equal(t, "user-456", filters.UserID)
+				require.NotNil(t, filters.TenantID)
+				assert.Equal(t, "tenant-a", *filters.TenantID)
 
 				return 1, nil
 			},
@@ -115,8 +212,19 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) {
 		handler.BulkDelete(rec, req)
 
 		assert.Equal(t, http.StatusOK, rec.Code)
-		require.NotNil(t, capturedTenantID)
-		assert.Equal(t, "tenant-a", *capturedTenantID)
+	})
+
+	t.Run("empty tenant_id returns bad request", func(t *testing.T) {
+		mock := &mockFeedbackRecordsService{}
+		handler := NewFeedbackRecordsHandler(mock)
+
+		req := httptest.NewRequestWithContext(context.Background(),
+			http.MethodDelete, "http://test/v1/feedback-records?user_id=user-123&tenant_id=", http.NoBody)
+		rec := httptest.NewRecorder()
+
+		handler.BulkDelete(rec, req)
+
+		assert.Equal(t, http.StatusBadRequest, rec.Code)
 	})
 
 	t.Run("missing user_id returns bad request", func(t *testing.T) {
@@ -146,7 +254,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) {
 
 	t.Run("service error returns 500", func(t *testing.T) {
 		mock := &mockFeedbackRecordsService{
-			bulkDeleteFunc: func(_ context.Context, _ string, _ *string) (int, error) {
+			bulkDeleteFunc: func(_ context.Context, _ *models.BulkDeleteFilters) (int, error) {
 				return 0, assert.AnError
 			},
 		}
@@ -163,7 +271,7 @@ func TestFeedbackRecordsHandler_BulkDelete(t *testing.T) {
 
 	t.Run("zero deleted returns 200 with deleted_count 0", func(t *testing.T) {
 		mock := &mockFeedbackRecordsService{
-			bulkDeleteFunc: func(_ context.Context, _ string, _ *string) (int, error) {
+			bulkDeleteFunc: func(_ context.Context, _ *models.BulkDeleteFilters) (int, error) {
 				return 0, nil
 			},
 		}

internal/models/events.go

@@ -0,0 +1,9 @@
+package models
+
+import "github.com/google/uuid"
+
+// DeletedIDsEventData is the tenant-aware payload for resource deletion events.
+type DeletedIDsEventData struct {
+	TenantID string      `json:"tenant_id"`
+	IDs      []uuid.UUID `json:"ids"`
+}

internal/models/feedback_records.go

@@ -199,11 +199,17 @@ type ListFeedbackRecordsResponse struct {
 // BulkDeleteFilters represents query parameters for bulk delete operation.
 type BulkDeleteFilters struct {
 	UserID   string  `form:"user_id"   validate:"required,no_null_bytes,min=1"`
-	TenantID *string `form:"tenant_id" validate:"omitempty,no_null_bytes"`
+	TenantID *string `form:"tenant_id" validate:"omitempty,no_null_bytes,min=1"`
 }
 
 // BulkDeleteResponse represents the response for bulk delete operation.
 type BulkDeleteResponse struct {
 	DeletedCount int64  `json:"deleted_count"`
 	Message      string `json:"message"`
 }
+
+// DeletedFeedbackRecordsByTenant groups deleted feedback record IDs by tenant.
+type DeletedFeedbackRecordsByTenant struct {
+	TenantID string
+	IDs      []uuid.UUID
+}

internal/models/webhooks.go

@@ -34,6 +34,12 @@ type Webhook struct {
 	DisabledAt     *time.Time            `json:"disabled_at,omitempty"`
 }
 
+// DeletedWebhook is the minimal data returned after deleting a webhook.
+type DeletedWebhook struct {
+	ID       uuid.UUID
+	TenantID *string
+}
+
 // MarshalJSON converts []datatypes.EventType to JSON string array.
 func (w *Webhook) MarshalJSON() ([]byte, error) {
 	type Alias Webhook
@@ -183,7 +189,7 @@ type CreateWebhookRequest struct {
 	URL        string                `json:"url"                   validate:"required,no_null_bytes,http_url,min=1,max=2048"`
 	SigningKey string                `json:"signing_key,omitempty" validate:"omitempty,max=255"`
 	Enabled    *bool                 `json:"enabled,omitempty"`
-	TenantID   *string               `json:"tenant_id,omitempty"   validate:"omitempty,no_null_bytes,max=255"`
+	TenantID   *string               `json:"tenant_id"             validate:"required,no_null_bytes,min=1,max=255"`
 	EventTypes []datatypes.EventType `json:"event_types,omitempty"`
 }
 
@@ -219,7 +225,7 @@ type UpdateWebhookRequest struct {
 	URL            *string                `json:"url,omitempty"         validate:"omitempty,no_null_bytes,http_url,min=1,max=2048"`
 	SigningKey     *string                `json:"signing_key,omitempty" validate:"omitempty,no_null_bytes,min=1,max=255"`
 	Enabled        *bool                  `json:"enabled,omitempty"`
-	TenantID       *string                `json:"tenant_id,omitempty"   validate:"omitempty,no_null_bytes,max=255"`
+	TenantID       *string                `json:"tenant_id,omitempty"   validate:"omitempty,no_null_bytes,min=1,max=255"`
 	EventTypes     *[]datatypes.EventType `json:"event_types,omitempty"`
 	DisabledReason *string                `json:"-"` // read-only; set by system when disabling
 	DisabledAt     *time.Time             `json:"-"` // read-only; set by system when disabling

internal/observability/names.go

@@ -43,8 +43,9 @@ func AllowedEventTypes() []string {
 
 // allowedProviderReasons for hub_webhook_provider_errors_total (bounded cardinality).
 var allowedProviderReasons = map[string]bool{
-	"list_failed":    true,
-	"enqueue_failed": true,
+	"list_failed":       true,
+	"enqueue_failed":    true,
+	"missing_tenant_id": true,
 }
 
 // allowedDeliveryStatuses for hub_webhook_deliveries_total and hub_webhook_delivery_duration_seconds.
@@ -63,6 +64,8 @@ var allowedDisabledReasons = map[string]bool{
 // allowedDispatchReasons for hub_webhook_dispatch_errors_total.
 var allowedDispatchReasons = map[string]bool{
 	"get_webhook_failed": true,
+	"missing_tenant_id":  true,
+	"tenant_mismatch":    true,
 }
 
 // allowedEmbeddingProviderReasons for hub_embedding_provider_errors_total.