fix: reject unstorable feedback-record metadata as 400 instead of 500

internal/api/validation/json_storable.go

@@ -0,0 +1,165 @@
+package validation
+
+import (
+	"reflect"
+	"unicode/utf8"
+
+	"github.com/go-playground/validator/v10"
+)
+
+// Postgres refuses two character-level things when text is written into a jsonb column, and both
+// arrive as escapes inside otherwise-valid JSON:
+//
+//	SELECT '{"note":"bad\u0000value"}'::jsonb  -- ERROR: unsupported Unicode escape sequence
+//	SELECT '{"note":"bad\ud83dvalue"}'::jsonb  -- ERROR: invalid input syntax for type json
+//
+// A raw-JSON request field (json.RawMessage) reaches the driver byte-for-byte, so neither is caught
+// by anything upstream: the request parses, the struct validates, and the failure lands as an
+// unmapped driver error — a 500 on input the API documents as invalid. `no_null_bytes` cannot cover
+// it either, because it skips any field that is not a string kind and json.RawMessage is a []byte.
+//
+// (Postgres also refuses a third, non-character thing: numbers that do not fit numeric, e.g.
+// 1e1000000. That is a range property of the parsed value, invisible to a byte scan, so it is
+// handled where it surfaces — the feedback-records repository maps SQLSTATE 22003 to a metadata
+// validation error.)
+//
+// The check runs over the raw bytes rather than the unmarshalled value on purpose. Unmarshalling
+// rewrites both cases — encoding/json turns a \u0000 escape into a real NUL and replaces an
+// unpaired surrogate with U+FFFD — so a value read back through a Go string looks storable while
+// the bytes actually sent to Postgres are not.
+const storableJSONTag = "storable_json"
+
+const (
+	highSurrogateStart = 0xd800
+	highSurrogateEnd   = 0xdbff
+	lowSurrogateStart  = 0xdc00
+	lowSurrogateEnd    = 0xdfff
+
+	escapeLen       = 6  // \uXXXX
+	hexLetterOffset = 10 // 'a'/'A' stands for 0xa, so a letter digit's value is offset by ten
+	hexDigits       = 4
+	pairLen         = escapeLen * 2
+	nibbleBits      = 4
+)
+
+// validateStorableJSON is the `storable_json` tag: it accepts anything that is not a raw JSON field
+// so the tag is inert if applied to the wrong kind, matching how no_null_bytes behaves.
+func validateStorableJSON(fl validator.FieldLevel) bool {
+	field := fl.Field()
+
+	if field.Kind() == reflect.Ptr {
+		if field.IsNil() {
+			return true // nil pointer is valid (handled by omitempty)
+		}
+
+		field = field.Elem()
+	}
+
+	if field.Kind() != reflect.Slice || field.Type().Elem().Kind() != reflect.Uint8 {
+		return true // Not raw JSON, skip validation
+	}
+
+	return IsStorableJSON(field.Bytes())
+}
+
+// IsStorableJSON reports whether raw JSON can be written to a Postgres jsonb column.
+//
+// It assumes the bytes are already syntactically valid JSON, which the request decoder has
+// established by the time a struct is validated. That is what makes a plain scan sound: a backslash
+// escape can only appear inside a JSON string, so keys and values are both covered without tracking
+// string context — and a NULL byte in a *key* is rejected by Postgres just as one in a value is.
+func IsStorableJSON(raw []byte) bool {
+	if len(raw) == 0 {
+		return true
+	}
+
+	// Catches raw invalid bytes, including a surrogate smuggled in as CESU-8, which Postgres
+	// rejects as an encoding error rather than a JSON one.
+	if !utf8.Valid(raw) {
+		return false
+	}
+
+	for i := 0; i < len(raw); i++ {
+		if raw[i] != '\\' {
+			continue
+		}
+
+		if i+1 >= len(raw) {
+			return false // trailing backslash: not valid JSON, and not storable
+		}
+
+		// Any other escape (\\, \", \n, ...) consumes its own second byte, which is what keeps a
+		// literal "\\u0000" — an escaped backslash followed by text — from reading as an escape.
+		if raw[i+1] != 'u' {
+			i++
+
+			continue
+		}
+
+		code, ok := parseHex4(raw[i+2:])
+		if !ok {
+			return false // malformed \u escape; Postgres would reject it too
+		}
+
+		if code == 0 {
+			return false // NULL byte
+		}
+
+		if code >= lowSurrogateStart && code <= lowSurrogateEnd {
+			return false // low surrogate with no high surrogate before it
+		}
+
+		if code >= highSurrogateStart && code <= highSurrogateEnd {
+			if !hasLowSurrogateAt(raw, i+escapeLen) {
+				return false // high surrogate never completed into a pair
+			}
+
+			i += pairLen - 1 // consume both halves
+
+			continue
+		}
+
+		i += escapeLen - 1
+	}
+
+	return true
+}
+
+// hasLowSurrogateAt reports whether a \uDC00-\uDFFF escape starts exactly at offset i.
+func hasLowSurrogateAt(raw []byte, i int) bool {
+	if i+escapeLen > len(raw) || raw[i] != '\\' || raw[i+1] != 'u' {
+		return false
+	}
+
+	code, ok := parseHex4(raw[i+2:])
+
+	return ok && code >= lowSurrogateStart && code <= lowSurrogateEnd
+}
+
+// parseHex4 reads the four hex digits of a \uXXXX escape.
+func parseHex4(digits []byte) (int, bool) {
+	if len(digits) < hexDigits {
+		return 0, false
+	}
+
+	code := 0
+
+	for _, digit := range digits[:hexDigits] {
+		var nibble int
+
+		switch {
+		case digit >= '0' && digit <= '9':
+			nibble = int(digit - '0')
+		case digit >= 'a' && digit <= 'f':
+			nibble = int(digit-'a') + hexLetterOffset
+		case digit >= 'A' && digit <= 'F':
+			nibble = int(digit-'A') + hexLetterOffset
+		default:
+			return 0, false
+		}
+
+		code = code<<nibbleBits | nibble
+	}
+
+	return code, true
+}

internal/api/validation/json_storable_test.go

@@ -0,0 +1,150 @@
+package validation
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/formbricks/hub/internal/models"
+)
+
+func TestIsStorableJSON(t *testing.T) {
+	// Every rejected case here was confirmed against Postgres 18 by casting the same text to
+	// jsonb; every accepted case casts cleanly.
+	tests := []struct {
+		name     string
+		raw      string
+		storable bool
+	}{
+		{name: "empty", raw: "", storable: true},
+		{name: "plain object", raw: `{"source":"link","country":"PT"}`, storable: true},
+		{name: "nested and arrays", raw: `{"a":{"b":["c",1,true,null]}}`, storable: true},
+		{name: "non-object scalar", raw: `42`, storable: true},
+
+		{name: "null byte in a value", raw: `{"note":"bad\u0000value"}`, storable: false},
+		{name: "null byte in a key", raw: `{"ba\u0000d":"v"}`, storable: false},
+		{name: "null byte in a nested object key", raw: `{"a":{"b\u0000c":1}}`, storable: false},
+		{name: "null byte nested in an array", raw: `{"tags":["ok","b\u0000d"]}`, storable: false},
+
+		{name: "lone high surrogate", raw: `{"note":"bad\ud83dvalue"}`, storable: false},
+		{name: "lone low surrogate", raw: `{"note":"bad\ude00value"}`, storable: false},
+		{name: "high surrogate at end of input", raw: `{"note":"bad\ud83d"}`, storable: false},
+		{name: "high surrogate followed by a plain character", raw: `{"note":"\ud83dA"}`, storable: false},
+		{name: "valid surrogate pair", raw: `{"note":"ok\ud83d\ude00"}`, storable: true},
+		{name: "valid pair uppercase hex", raw: `{"note":"ok\uD83D\uDE00"}`, storable: true},
+		{name: "two valid pairs back to back", raw: `{"note":"\ud83d\ude00\ud83d\ude01"}`, storable: true},
+
+		// An escaped backslash is not the start of an escape. Without consuming its second byte the
+		// scanner would read the following "u0000" as a NUL escape and reject a legitimate value.
+		{name: "escaped backslash before u0000 text", raw: `{"note":"path\\u0000notanescape"}`, storable: true},
+		{name: "escaped backslash then a real null escape", raw: `{"note":"a\\\u0000"}`, storable: false},
+		{name: "other escapes are ignored", raw: `{"note":"line\nquote\"tab\t"}`, storable: true},
+
+		{name: "malformed short escape", raw: `{"note":"\u00"}`, storable: false},
+		{name: "malformed non-hex escape", raw: `{"note":"\uZZZZ"}`, storable: false},
+		{name: "trailing backslash", raw: `{"note":"a\`, storable: false},
+
+		// Emoji as real UTF-8 rather than escapes: valid, and the common case for survey text.
+		{name: "literal multi-byte utf8", raw: `{"note":"feedback 😀"}`, storable: true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.storable, IsStorableJSON([]byte(tt.raw)))
+		})
+	}
+}
+
+func TestIsStorableJSONRejectsInvalidUTF8(t *testing.T) {
+	// A surrogate encoded directly as CESU-8 rather than as a \u escape. Postgres rejects it as an
+	// encoding error, so the scan has to catch it before the escape walk.
+	raw := []byte(`{"note":"`)
+	raw = append(raw, 0xed, 0xa0, 0xbd) // U+D83D encoded directly, which UTF-8 forbids
+	raw = append(raw, `"}`...)
+
+	assert.False(t, IsStorableJSON(raw))
+}
+
+// The tag is what actually protects the endpoint, so drive it through ValidateStruct rather than
+// calling the predicate directly.
+func TestStorableJSONTagOnRequestStruct(t *testing.T) {
+	type request struct {
+		Metadata json.RawMessage `json:"metadata,omitempty" validate:"omitempty,storable_json"`
+	}
+
+	t.Run("accepts storable metadata", func(t *testing.T) {
+		require.NoError(t, ValidateStruct(request{Metadata: json.RawMessage(`{"source":"link"}`)}))
+	})
+
+	t.Run("accepts an absent metadata field", func(t *testing.T) {
+		require.NoError(t, ValidateStruct(request{}))
+	})
+
+	t.Run("rejects a null byte with a self-correcting reason", func(t *testing.T) {
+		err := ValidateStruct(request{Metadata: json.RawMessage(`{"note":"bad\u0000value"}`)})
+
+		require.Error(t, err)
+		require.ErrorIs(t, err, ErrValidationFailed)
+		assert.Contains(t, err.Error(), "metadata must contain valid UTF-8 and no NULL bytes or unpaired UTF-16 surrogates")
+	})
+
+	t.Run("rejects an unpaired surrogate", func(t *testing.T) {
+		err := ValidateStruct(request{Metadata: json.RawMessage(`{"note":"bad\ud83dvalue"}`)})
+
+		require.Error(t, err)
+		require.ErrorIs(t, err, ErrValidationFailed)
+	})
+}
+
+// The tag is deliberately inert on non-raw-JSON fields, mirroring no_null_bytes, so misapplying it
+// cannot start rejecting unrelated input.
+func TestStorableJSONTagIgnoresNonRawJSONFields(t *testing.T) {
+	type request struct {
+		Name string `json:"name" validate:"omitempty,storable_json"`
+	}
+
+	require.NoError(t, ValidateStruct(request{Name: "anything at all"}))
+}
+
+// The tests above pin the tag's behavior on a local struct; these pin that the tag is actually
+// PRESENT on the two request structs the endpoint decodes into. Reverting only the models hunk of
+// the ENG-2745 fix — the part that protects the endpoint — left the whole suite green before this.
+func TestRealRequestStructsCarryStorableJSONTag(t *testing.T) {
+	badMetadata := json.RawMessage(`{"note":"bad\u0000value"}`)
+
+	t.Run("CreateFeedbackRecordRequest rejects unstorable metadata", func(t *testing.T) {
+		err := ValidateStruct(models.CreateFeedbackRecordRequest{
+			SourceType:   "survey",
+			FieldID:      "q1",
+			FieldType:    models.FieldTypeText,
+			TenantID:     "tenant-1",
+			SubmissionID: "s1",
+			Metadata:     badMetadata,
+		})
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "metadata must contain valid UTF-8 and no NULL bytes or unpaired UTF-16 surrogates")
+	})
+
+	t.Run("UpdateFeedbackRecordRequest rejects unstorable metadata", func(t *testing.T) {
+		err := ValidateStruct(models.UpdateFeedbackRecordRequest{Metadata: badMetadata})
+
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "metadata must contain valid UTF-8 and no NULL bytes or unpaired UTF-16 surrogates")
+	})
+
+	t.Run("both accept storable metadata", func(t *testing.T) {
+		fine := json.RawMessage(`{"source":"link"}`)
+		require.NoError(t, ValidateStruct(models.CreateFeedbackRecordRequest{
+			SourceType:   "survey",
+			FieldID:      "q1",
+			FieldType:    models.FieldTypeText,
+			TenantID:     "tenant-1",
+			SubmissionID: "s1",
+			Metadata:     fine,
+		}))
+		require.NoError(t, ValidateStruct(models.UpdateFeedbackRecordRequest{Metadata: fine}))
+	})
+}

internal/api/validation/validation.go

@@ -104,6 +104,10 @@ func init() {
 		slog.Error("Failed to register no_null_bytes validator", "error", err)
 	}
 
+	if err := validate.RegisterValidation(storableJSONTag, validateStorableJSON); err != nil {
+		slog.Error("Failed to register storable_json validator", "error", err)
+	}
+
 	// Element validators for the repeatable enum filters, applied via `dive`. They are a second
 	// gate behind registerEnumSliceTypes, not a replacement: see the comment there for why the
 	// decoder alone cannot be trusted to have run.
@@ -390,6 +394,8 @@ func FormatFieldError(fieldErr validator.FieldError) string {
 		return "must be in RFC3339 (ISO 8601) format"
 	case "no_null_bytes":
 		return "must not contain NULL bytes"
+	case storableJSONTag:
+		return "must contain valid UTF-8 and no NULL bytes or unpaired UTF-16 surrogates"
 	case "http_url":
 		return "must be a valid HTTP or HTTPS URL"
 	case "url":

internal/models/feedback_records.go

@@ -494,7 +494,7 @@ type CreateFeedbackRecordRequest struct {
 	ValueNumber     *float64        `json:"value_number,omitempty"`
 	ValueBoolean    *bool           `json:"value_boolean,omitempty"`
 	ValueDate       *time.Time      `json:"value_date,omitempty"`
-	Metadata        json.RawMessage `json:"metadata,omitempty"`
+	Metadata        json.RawMessage `json:"metadata,omitempty"          validate:"omitempty,storable_json"`
 	Language        *string         `json:"language,omitempty"          validate:"omitempty,no_null_bytes,max=10"`
 	UserID          *string         `json:"user_id,omitempty"           validate:"omitempty,no_null_bytes,max=255"`
 	TenantID        string          `json:"tenant_id"                   validate:"required,no_null_bytes,max=255"`
@@ -516,7 +516,7 @@ type UpdateFeedbackRecordRequest struct {
 	ValueNumber  *float64        `json:"value_number,omitempty"`
 	ValueBoolean *bool           `json:"value_boolean,omitempty"`
 	ValueDate    *time.Time      `json:"value_date,omitempty"`
-	Metadata     json.RawMessage `json:"metadata,omitempty"`
+	Metadata     json.RawMessage `json:"metadata,omitempty"      validate:"omitempty,storable_json"`
 	Language     *string         `json:"language,omitempty"      validate:"omitempty,no_null_bytes,max=10"`
 	UserID       *string         `json:"user_id,omitempty"       validate:"omitempty,no_null_bytes,max=255"`
 }

internal/repository/feedback_records_repository.go

@@ -21,6 +21,17 @@ import (
 // constraint violations (23505 unique_violation).
 const uniqueViolationSQLState = "23505"
 
+// numericOverflowSQLState (22003 numeric_value_out_of_range) fires when jsonb metadata carries a
+// number that does not fit Postgres numeric — e.g. `{"n":1e1000000}`, fifteen bytes of perfectly
+// valid JSON. The storable_json request validator cannot see this class: it is a range property of
+// the parsed number, not of the bytes. Metadata is the only place it can originate on these
+// queries — value_number is float8, and every Go float64 fits float8 — so the error is mapped to a
+// metadata validation failure rather than surfacing as an unmapped 500 (ENG-2745).
+const numericOverflowSQLState = "22003"
+
+// numericOverflowMessage is the invalid_params reason for the mapping above.
+const numericOverflowMessage = "contains a number outside the storable range"
+
 // FeedbackRecordsRepository handles data access for feedback records.
 type FeedbackRecordsRepository struct {
 	db *pgxpool.Pool
@@ -139,6 +150,10 @@ func (r *FeedbackRecordsRepository) Create(ctx context.Context, req *models.Crea
 			return nil, huberrors.NewConflictError("a feedback record with this tenant_id, submission_id, and field_id already exists")
 		}
 
+		if errors.As(err, &pgErr) && pgErr.Code == numericOverflowSQLState {
+			return nil, huberrors.NewValidationError("metadata", numericOverflowMessage)
+		}
+
 		if errors.Is(err, pgx.ErrNoRows) {
 			return nil, huberrors.NewTenantWriteConflictError("tenant data purge in progress for this tenant; retry later")
 		}
@@ -947,6 +962,11 @@ func (r *FeedbackRecordsRepository) Update(
 				return huberrors.NewNotFoundError("feedback record", "feedback record not found")
 			}
 
+			var pgErr *pgconn.PgError
+			if errors.As(scanErr, &pgErr) && pgErr.Code == numericOverflowSQLState {
+				return huberrors.NewValidationError("metadata", numericOverflowMessage)
+			}
+
 			return fmt.Errorf("failed to update feedback record: %w", scanErr)
 		}