diff --git a/internal/api/handlers/decode_json_handlers_test.go b/internal/api/handlers/decode_json_handlers_test.go index 09dcd7e5..3195dfcd 100644 --- a/internal/api/handlers/decode_json_handlers_test.go +++ b/internal/api/handlers/decode_json_handlers_test.go @@ -297,6 +297,50 @@ func TestNewlyCappedBodiesAreBounded(t *testing.T) { } } +// Both taxonomy labels are free text with no limit of their own before ENG-1652; an over-long one +// must be refused with 400 naming it, before the (panicking) stub service is reached. +func TestTaxonomyLabelsAreBounded(t *testing.T) { + long := strings.Repeat("l", 2049) + id := uuid.Must(uuid.NewV7()).String() + + cases := map[string]struct { + member string + body string + call func(http.ResponseWriter, *http.Request) + }{ + "taxonomy run create": { + member: "field_label", + body: `{"tenant_id":"tenant-a","source_type":"survey","source_id":"s","field_id":"q1",` + + `"actor_id":"a","field_label":"` + long + `"}`, + call: NewTaxonomyHandler(&stubTaxonomyService{}).CreateRun, + }, + "taxonomy node rename": { + member: "label", + body: `{"tenant_id":"tenant-a","actor_id":"a","label":"` + long + `"}`, + call: NewTaxonomyHandler(&stubTaxonomyService{}).RenameNode, + }, + } + + for name, testCase := range cases { + t.Run(name, func(t *testing.T) { + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "http://test/x", + strings.NewReader(testCase.body)) + req.SetPathValue("node_id", id) + + rec := httptest.NewRecorder() + + require.NotPanics(t, func() { testCase.call(rec, req) }, "the body must be refused before any service call") + + assert.Equal(t, http.StatusBadRequest, rec.Code) + + var problem response.ProblemDetails + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &problem)) + require.Len(t, problem.InvalidParams, 1) + assert.Equal(t, testCase.member, problem.InvalidParams[0].Name) + }) + } +} + type recordingWebhooksService struct{ called bool } func (s *recordingWebhooksService) CreateWebhook(context.Context, *models.CreateWebhookRequest) (*models.Webhook, error) { diff --git a/internal/api/handlers/search_handler.go b/internal/api/handlers/search_handler.go index b1b5fd81..b7564f14 100644 --- a/internal/api/handlers/search_handler.go +++ b/internal/api/handlers/search_handler.go @@ -34,9 +34,11 @@ func NewSearchHandler(service SearchService) *SearchHandler { } // SemanticSearchRequest is the body for POST /v1/feedback-records/search/semantic (snake_case for consistency with data model). +// The query is sent to the paid embedding provider on every uncached call, so it is capped; 4,096 +// characters is twice what the Formbricks gateway already allows, so the gateway's error comes first. type SemanticSearchRequest struct { - Query string `json:"query"` - TenantID string `json:"tenant_id" validate:"required,no_replacement_char"` + Query string `json:"query" validate:"max=4096"` + TenantID string `json:"tenant_id" validate:"required,no_replacement_char,max=255"` } // SemanticSearchResponse is the response for semantic search and similar feedback (consistent with list endpoints: data, limit). diff --git a/internal/api/handlers/search_handler_test.go b/internal/api/handlers/search_handler_test.go index 31efd400..99a54a18 100644 --- a/internal/api/handlers/search_handler_test.go +++ b/internal/api/handlers/search_handler_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/google/uuid" @@ -81,6 +82,62 @@ func TestSearchHandler_SemanticSearch(t *testing.T) { assert.Equal(t, http.StatusBadRequest, rec.Code) }) + t.Run("over-long query or tenant_id returns 400 before the provider is called", func(t *testing.T) { + for name, body := range map[string]map[string]string{ + "query": {"query": strings.Repeat("a", 4097), "tenant_id": "env-1"}, + "tenant_id": {"query": "login is slow", "tenant_id": strings.Repeat("t", 256)}, + } { + t.Run(name, func(t *testing.T) { + called := false + mock := &mockSearchService{ + semanticFunc: func(_ context.Context, _, _ string, _ int, _ float64, _ string) (service.SearchResult, error) { + called = true + + return service.SearchResult{}, nil + }, + } + raw, err := json.Marshal(body) + require.NoError(t, err) + + req := httptest.NewRequestWithContext(context.Background(), + http.MethodPost, "http://test/v1/feedback-records/search/semantic", bytes.NewReader(raw)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + + NewSearchHandler(mock).SemanticSearch(rec, req) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + assert.Contains(t, rec.Body.String(), `"name":"`+name+`"`) + assert.False(t, called) + }) + } + }) + + t.Run("query at the limit counts characters, not bytes", func(t *testing.T) { + called := false + mock := &mockSearchService{ + semanticFunc: func(_ context.Context, _, _ string, _ int, _ float64, _ string) (service.SearchResult, error) { + called = true + + return service.SearchResult{}, nil + }, + } + raw, err := json.Marshal(map[string]string{"query": strings.Repeat("ü", 4096), "tenant_id": "env-1"}) + require.NoError(t, err) + + req := httptest.NewRequestWithContext(context.Background(), + http.MethodPost, "http://test/v1/feedback-records/search/semantic", bytes.NewReader(raw)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + + NewSearchHandler(mock).SemanticSearch(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.True(t, called) + }) + t.Run("success returns 200 with data and value", func(t *testing.T) { id1 := uuid.MustParse("018e1234-5678-9abc-def0-111111111111") id2 := uuid.MustParse("018e1234-5678-9abc-def0-222222222222") diff --git a/internal/api/response/errors.go b/internal/api/response/errors.go index a9be6850..22bd0abf 100644 --- a/internal/api/response/errors.go +++ b/internal/api/response/errors.go @@ -154,11 +154,13 @@ const ( ReasonJSONMemberRepeated = "appears more than once; each member may appear only once" ) -// maxReportedJSONNameRunes bounds how much of a caller-chosen member path is echoed back, and -// maxReportedJSONReasonRunes how much of a decoder's reason, which can quote the caller's value. +// maxReportedJSONNameRunes bounds how much of a caller-chosen member path is echoed back, +// maxReportedJSONReasonRunes how much of a decoder's reason, which can quote the caller's value, +// and maxReportedValueRunes how much of a rejected value a reason quotes itself. const ( maxReportedJSONNameRunes = 64 maxReportedJSONReasonRunes = 256 + maxReportedValueRunes = 64 ) // problemFromJSONDecodeError recognizes errors from decoding a JSON request body with @@ -363,7 +365,7 @@ func invalidFieldTypeParam(err error) (InvalidParam, bool) { Name: "field_type", Reason: fmt.Sprintf( "has invalid value %q; must be one of: %s", - invalidFieldType.Value, + truncateRunes(invalidFieldType.Value, maxReportedValueRunes), models.ValidFieldTypeValuesString(), ), }, true diff --git a/internal/api/response/response_test.go b/internal/api/response/response_test.go index cc1d9f67..498c0496 100644 --- a/internal/api/response/response_test.go +++ b/internal/api/response/response_test.go @@ -158,6 +158,21 @@ func TestRespondErrorInvalidFieldTypeReason(t *testing.T) { assert.Contains(t, problem.InvalidParams[0].Reason, "date") } +func TestRespondErrorInvalidFieldTypeReasonTruncatesLongValue(t *testing.T) { + rec := httptest.NewRecorder() + value := strings.Repeat("x", 512<<10) + + RespondError(rec, newReq(t, http.MethodPost, "/v1/feedback-records"), &models.InvalidFieldTypeError{Value: value}) + + problem := decodeProblem(t, rec) + require.Len(t, problem.InvalidParams, 1) + + reason := problem.InvalidParams[0].Reason + assert.Contains(t, reason, `"`+strings.Repeat("x", maxReportedValueRunes)+`…"`) + assert.NotContains(t, reason, strings.Repeat("x", maxReportedValueRunes+1)) + assert.Contains(t, reason, "must be one of") +} + func TestRespondErrorQueryDecodeErrorIsValidationProblem(t *testing.T) { var filters struct { Since *time.Time `form:"since"` diff --git a/internal/models/feedback_records_filters_openapi_test.go b/internal/models/feedback_records_filters_openapi_test.go index 42ccf9e4..fc964726 100644 --- a/internal/models/feedback_records_filters_openapi_test.go +++ b/internal/models/feedback_records_filters_openapi_test.go @@ -127,6 +127,20 @@ func TestOpenAPICapsMatchTheFilterStruct(t *testing.T) { func loadOpenAPIParameters(t *testing.T) openAPIParameters { t.Helper() + var spec openAPIParameters + loadOpenAPISpec(t, &spec) + + if len(spec.Components.Parameters) == 0 { + t.Fatal("openapi.yaml parsed to zero parameters; this test would assert nothing") + } + + return spec +} + +// loadOpenAPISpec decodes the repository's openapi.yaml into dst, which names only the parts a test needs. +func loadOpenAPISpec(t *testing.T, dst any) { + t.Helper() + _, filename, _, ok := runtime.Caller(0) if !ok { t.Fatal("cannot resolve this test file's path") @@ -139,16 +153,9 @@ func loadOpenAPIParameters(t *testing.T) openAPIParameters { t.Fatalf("read openapi.yaml: %v", err) } - var spec openAPIParameters - if err := yaml.Unmarshal(contents, &spec); err != nil { + if err := yaml.Unmarshal(contents, dst); err != nil { t.Fatalf("parse openapi.yaml: %v", err) } - - if len(spec.Components.Parameters) == 0 { - t.Fatal("openapi.yaml parsed to zero parameters; this test would assert nothing") - } - - return spec } // openAPIOperationParameters is the slice of openapi.yaml naming each list operation's query diff --git a/internal/models/request_arrays_openapi_test.go b/internal/models/request_arrays_openapi_test.go new file mode 100644 index 00000000..65abd8e5 --- /dev/null +++ b/internal/models/request_arrays_openapi_test.go @@ -0,0 +1,66 @@ +package models + +import ( + "testing" + + "github.com/formbricks/hub/internal/datatypes" +) + +type openAPIRequestSchemas struct { + Components struct { + Schemas map[string]struct { + Properties map[string]struct { + MaxItems *int `yaml:"maxItems"` + Items struct { + Enum []string `yaml:"enum"` + } `yaml:"items"` + } `yaml:"properties"` + } `yaml:"schemas"` + } `yaml:"components"` +} + +// TestOpenAPIRequestArrayCapsMatchTheirValues pins each request array whose values come from a fixed +// set to a maxItems equal to that set's size. The server refuses a repeated or unknown value, so the +// set's size is the real bound; a spec that falls behind it (a seventh event type added, maxItems left +// at six) would make generated clients refuse a list the server accepts. +func TestOpenAPIRequestArrayCapsMatchTheirValues(t *testing.T) { + t.Parallel() + + var spec openAPIRequestSchemas + loadOpenAPISpec(t, &spec) + + retry := spec.Components.Schemas["EnrichmentRetryInputBody"].Properties["enrichments"] + + cases := map[string]struct { + maxItems *int + want int + }{ + "CreateWebhookInputBody.event_types": { + spec.Components.Schemas["CreateWebhookInputBody"].Properties["event_types"].MaxItems, + len(datatypes.GetAllEventTypes()), + }, + "UpdateWebhookInputBody.event_types": { + spec.Components.Schemas["UpdateWebhookInputBody"].Properties["event_types"].MaxItems, + len(datatypes.GetAllEventTypes()), + }, + "EnrichmentRetryInputBody.enrichments": {retry.MaxItems, len(retry.Items.Enum)}, + } + + for name, testCase := range cases { + if testCase.maxItems == nil { + t.Errorf("%s has no maxItems in openapi.yaml", name) + + continue + } + + if testCase.want == 0 { + t.Errorf("%s: found no allowed values to compare against; this check would assert nothing", name) + + continue + } + + if *testCase.maxItems != testCase.want { + t.Errorf("%s: maxItems is %d, but there are %d allowed values", name, *testCase.maxItems, testCase.want) + } + } +} diff --git a/internal/models/taxonomy.go b/internal/models/taxonomy.go index 7416fda5..f74fd339 100644 --- a/internal/models/taxonomy.go +++ b/internal/models/taxonomy.go @@ -131,7 +131,7 @@ type TaxonomyRun struct { type CreateTaxonomyRunRequest struct { TaxonomyScope - FieldLabel *string `json:"field_label,omitempty" validate:"omitempty,no_null_bytes"` + FieldLabel *string `json:"field_label,omitempty" validate:"omitempty,no_null_bytes,max=2048"` ActorID *string `json:"actor_id,omitempty" validate:"omitempty,no_null_bytes,min=1,max=255"` } @@ -339,7 +339,7 @@ type TaxonomyRunPartialMetrics struct { type RenameTaxonomyNodeRequest struct { TenantID string `json:"tenant_id" validate:"required,no_null_bytes,no_replacement_char,min=1,max=255"` ActorID string `json:"actor_id" validate:"required,no_null_bytes,min=1,max=255"` - Label string `json:"label" validate:"required,no_null_bytes,min=1"` + Label string `json:"label" validate:"required,no_null_bytes,min=1,max=2048"` } // TaxonomyNodeRecordsFilters scopes taxonomy node feedback record drilldown. diff --git a/internal/service/enrichment_retry_service.go b/internal/service/enrichment_retry_service.go index 7e1f3100..5e97b73b 100644 --- a/internal/service/enrichment_retry_service.go +++ b/internal/service/enrichment_retry_service.go @@ -255,6 +255,13 @@ func normalizeRequestedEnrichments(requested []string) ([]string, error) { return all, nil } + // There are only three names, so a longer list can only repeat them. The cap matches the spec's + // maxItems, which lets a generated client refuse such a list before sending it. + if len(requested) > len(all) { + return nil, huberrors.NewValidationError("enrichments", + fmt.Sprintf("must list at most %d enrichments", len(all))) + } + known := make(map[string]struct{}, len(all)) for _, name := range all { known[name] = struct{}{} diff --git a/internal/service/enrichment_retry_service_test.go b/internal/service/enrichment_retry_service_test.go index 6a282043..d31545ea 100644 --- a/internal/service/enrichment_retry_service_test.go +++ b/internal/service/enrichment_retry_service_test.go @@ -85,6 +85,25 @@ func TestRetryReportsTheCeilingOfTheWait(t *testing.T) { "90.4s remaining must report 91, not 90") } +// TestRetryRefusesMoreNamesThanThereAreEnrichments: a repeated name is still folded away, but a +// list longer than the three enrichments is refused before anything is cleared. +func TestRetryRefusesMoreNamesThanThereAreEnrichments(t *testing.T) { + repo := &fakeRetryRepo{claimed: true, cleared: 1} + svc := newRetryService(repo, true) + + resp, err := svc.Retry(context.Background(), "tenant-1", []string{"sentiment", "sentiment"}) + require.NoError(t, err) + assert.Len(t, resp.Results, 1) + + _, err = svc.Retry(context.Background(), "tenant-1", []string{"sentiment", "emotions", "translation", "sentiment"}) + require.Error(t, err) + + var validation *huberrors.ValidationError + require.ErrorAs(t, err, &validation) + assert.Equal(t, "enrichments", validation.Field) + assert.Equal(t, 1, repo.calls, "only the valid request reached the repository") +} + // TestRetryWindowDecisionBelongsToTheClear: the service must not pre-check the cooldown and then // clear — the repository claims the window atomically with the delete, and the service's only job // on refusal is to report the wait. One repo call on the success path is the proof. diff --git a/openapi.yaml b/openapi.yaml index c9dfdfcc..d340bf80 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -22,7 +22,7 @@ info: license: name: Apache-2.0 url: https://www.apache.org/licenses/LICENSE-2.0 - version: 1.1.0 + version: 1.2.0 servers: - url: "{scheme}://{host}{basePath}" description: Deployed Formbricks Hub instance (self-hosted or Formbricks Cloud) @@ -3182,11 +3182,13 @@ components: query: type: string minLength: 1 + maxLength: 4096 description: Search query text (embedded and compared via cosine similarity) example: "What do users think about login speed?" tenant_id: type: string minLength: 1 + maxLength: 255 description: Tenant ID (required for isolation; must match feedback record tenant_id). U+FFFD is not allowed (it stands in for invalid UTF-8 or an unpaired surrogate). default: "org-123" example: "org-123" @@ -3692,8 +3694,10 @@ components: event_types: type: array description: | - Event types this webhook subscribes to. Each value must be one of WebhookEventType. - If empty, the webhook receives all event types. + Event types this webhook subscribes to. Each value must be one of WebhookEventType, + listed at most once. If empty, the webhook receives all event types. + maxItems: 6 + uniqueItems: true items: $ref: '#/components/schemas/WebhookEventType' required: @@ -3756,7 +3760,9 @@ components: example: "org-123" event_types: type: array - description: New list of event types (use empty array to clear). Each value must be one of WebhookEventType. + description: New list of event types (use empty array to clear). Each value must be one of WebhookEventType, listed at most once. + maxItems: 6 + uniqueItems: true items: $ref: '#/components/schemas/WebhookEventType' WebhookPublicData: @@ -4291,6 +4297,7 @@ components: properties: enrichments: type: array + maxItems: 3 items: type: string enum: @@ -4300,7 +4307,7 @@ components: description: | Which enrichments to clear. Omit or leave empty for all of them, which is the usual call. An unknown name is rejected rather than ignored, so a typo cannot come back as - a successful no-op. + a successful no-op. A list longer than the three names is rejected too. EnrichmentRetryResult: type: object additionalProperties: false @@ -4419,6 +4426,7 @@ components: description: Required for field scope; omit for directory scope. field_label: type: string + maxLength: 2048 pattern: '^[^\x00]*$' description: Optional human-readable field label. actor_id: @@ -4525,6 +4533,7 @@ components: label: type: string minLength: 1 + maxLength: 2048 pattern: '^[^\x00]*$' description: New node label. required: