From f688257b9281785e18802aea2b6959efa3e237e0 Mon Sep 17 00:00:00 2001 From: mliptak Date: Thu, 23 Jul 2026 11:43:19 +0200 Subject: [PATCH 1/3] HYPERFLEET-1375 - fix: validate typed search fields and close DB column leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Search values for generation/created_time/updated_time/deleted_time reached Postgres without type validation, leaking raw pq: driver errors and SQLSTATE codes as 500s instead of clean 400s. Add type validation to FieldNameWalk, the shared search-to-SQL translation layer used by both /resources and typed entity search. Also remove the dead "properties." field mapping, a leftover alias from before the resources table's JSONB column was renamed to "spec" — it pointed at a nonexistent column and produced the same class of raw pq: leak. Also cap the label-key length check at the actual resource_labels.key DB column size (255) instead of the Kubernetes spec's own limit (317), so a spec-valid-but-oversized key is rejected with a clean 400 during request validation instead of a 500 during resource conversion. Co-Authored-By: Claude --- pkg/db/sql_helpers.go | 98 ++++++++++-- pkg/db/sql_helpers_test.go | 86 +++++++++++ pkg/handlers/validation.go | 9 +- pkg/handlers/validation_test.go | 24 +++ pkg/services/generic_test.go | 13 +- test/integration/search_field_mapping_test.go | 142 ++++++++++++++++++ 6 files changed, 355 insertions(+), 17 deletions(-) diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index 9a5e260a..e9044e1d 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -37,16 +37,6 @@ func validateJSONBKey(key, fieldType string) *errors.ServiceError { func getField(name string) (field string, err *errors.ServiceError) { trimmedName := strings.Trim(name, " ") - if strings.HasPrefix(trimmedName, "properties.") { - key := strings.TrimPrefix(trimmedName, "properties.") - if validationErr := validateJSONBKey(key, "property key"); validationErr != nil { - err = validationErr - return - } - field = fmt.Sprintf("properties ->> '%s'", key) - return - } - // Map user-friendly spec.xxx (and nested spec.xxx.yyy...) syntax to JSONB query. // v6 gives us the full dotted path directly: // spec.region → spec->>'region' @@ -575,9 +565,97 @@ func FieldNameWalk(n *tsl.Node) (*tsl.Node, *errors.ServiceError) { } return n, errors.BadRequest("%s", err.Error()) } + if svcErr := validateTypedFieldValues(mapped); svcErr != nil { + return n, svcErr + } return wrapSpecNumericCasts(mapped), nil } +// searchTypedFields declares the expected TSL literal kind for top-level search +// columns whose underlying SQL type does not accept arbitrary text (INTEGER, +// TIMESTAMPTZ). Fields not listed here are VARCHAR and accept any string +// literal without a Postgres type-mismatch risk. +var searchTypedFields = map[string]tsl.Kind{ + "generation": tsl.KindNumericLiteral, + "created_time": tsl.KindTimestampLiteral, + "updated_time": tsl.KindTimestampLiteral, + "deleted_time": tsl.KindTimestampLiteral, +} + +// validateTypedFieldValues walks binary comparisons against a searchTypedFields +// column and rejects values whose TSL literal kind doesn't match the column's +// SQL type, before the query reaches Postgres as a raw pq: error. +func validateTypedFieldValues(n *tsl.Node) *errors.ServiceError { + if n == nil { + return nil + } + if n.Kind == tsl.KindBinaryExpr && n.Left != nil && n.Left.Kind == tsl.KindIdentifier { + if field, ok := n.Left.Value.(string); ok { + if wantKind, tracked := searchTypedFields[field]; tracked { + if err := validateTypedValue(field, wantKind, n.Right); err != nil { + return err + } + } + } + } + if err := validateTypedFieldValues(n.Left); err != nil { + return err + } + if err := validateTypedFieldValues(n.Right); err != nil { + return err + } + for _, child := range n.Children { + if err := validateTypedFieldValues(child); err != nil { + return err + } + } + return nil +} + +// validateTypedValue checks a single value node (or, for IN queries, each +// element of an array literal) against the expected TSL literal kind. +func validateTypedValue(field string, wantKind tsl.Kind, right *tsl.Node) *errors.ServiceError { + if right == nil { + return errors.BadRequest("missing value for field '%s'", field) + } + + if right.Kind == tsl.KindArrayLiteral { + for _, child := range right.Children { + if !matchesTypedKind(wantKind, child.Kind) { + return typedFieldError(field, wantKind, child.Value) + } + } + return nil + } + + if !matchesTypedKind(wantKind, right.Kind) { + return typedFieldError(field, wantKind, right.Value) + } + return nil +} + +// matchesTypedKind reports whether a literal kind is acceptable for wantKind. +// A date literal (no time component) is accepted wherever a timestamp is expected. +func matchesTypedKind(wantKind, gotKind tsl.Kind) bool { + if gotKind == wantKind { + return true + } + return wantKind == tsl.KindTimestampLiteral && gotKind == tsl.KindDateLiteral +} + +func typedFieldError(field string, wantKind tsl.Kind, got any) *errors.ServiceError { + switch wantKind { + case tsl.KindNumericLiteral: + return errors.BadRequest("field '%s' expects an integer value, got %v", field, got) + case tsl.KindTimestampLiteral: + return errors.BadRequest( + "field '%s' expects an RFC3339 timestamp value (e.g. 2026-01-01T00:00:00Z), got %v", field, got, + ) + default: + return errors.BadRequest("field '%s' has an unsupported value type", field) + } +} + // wrapSpecNumericCasts wraps spec JSONB fields in CAST(... AS numeric) when // compared against a number, so multi-digit values sort correctly. func wrapSpecNumericCasts(n *tsl.Node) *tsl.Node { diff --git a/pkg/db/sql_helpers_test.go b/pkg/db/sql_helpers_test.go index d3ceccdc..00970726 100644 --- a/pkg/db/sql_helpers_test.go +++ b/pkg/db/sql_helpers_test.go @@ -797,6 +797,92 @@ func TestFieldNameWalk_NumericCast(t *testing.T) { }) } +// TestFieldNameWalk_TypedFieldValidation verifies that typed top-level columns +// (generation: INTEGER; created_time/updated_time/deleted_time: TIMESTAMPTZ) +// reject mismatched values with a clean 400 before ever reaching Postgres. +func TestFieldNameWalk_TypedFieldValidation(t *testing.T) { + tests := []struct { + name string + searchQuery string + errorContains string + expectError bool + }{ + { + name: "generation with valid integer", + searchQuery: "generation = 5", + }, + { + name: "generation with non-integer string value", + searchQuery: "generation = 'abc'", + expectError: true, + errorContains: "field 'generation' expects an integer value", + }, + { + name: "created_time with valid RFC3339 timestamp", + searchQuery: "created_time > '2026-01-01T00:00:00Z'", + }, + { + name: "created_time with non-timestamp string value", + searchQuery: "created_time = 'not-a-date'", + expectError: true, + errorContains: "field 'created_time' expects an RFC3339 timestamp value", + }, + { + name: "updated_time with non-timestamp string value", + searchQuery: "updated_time = 'not-a-date'", + expectError: true, + errorContains: "field 'updated_time' expects an RFC3339 timestamp value", + }, + { + name: "deleted_time with non-timestamp string value", + searchQuery: "deleted_time = 'not-a-date'", + expectError: true, + errorContains: "field 'deleted_time' expects an RFC3339 timestamp value", + }, + { + name: "generation IN list with a non-integer element", + searchQuery: "generation IN [1, 'abc']", + expectError: true, + errorContains: "field 'generation' expects an integer value", + }, + { + name: "generation IN list with all integers", + searchQuery: "generation IN [1, 2, 3]", + }, + { + name: "combined query: typed field valid, text field untouched", + searchQuery: "generation = 1 AND name = 'anything-goes'", + }, + { + name: "combined query: error surfaces even when other predicate is valid", + searchQuery: "name = 'ok' AND generation = 'abc'", + expectError: true, + errorContains: "field 'generation' expects an integer value", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + + tree, err := tsl.ParseTSL(tt.searchQuery) + Expect(err).ToNot(HaveOccurred()) + + _, serviceErr := FieldNameWalk(tree.Node) + + if tt.expectError { + Expect(serviceErr).ToNot(BeNil()) + Expect(serviceErr.Error()).To(ContainSubstring(tt.errorContains)) + // No raw driver details should ever leak into the message. + Expect(serviceErr.Error()).ToNot(ContainSubstring("pq:")) + Expect(serviceErr.Error()).ToNot(ContainSubstring("SQLSTATE")) + return + } + Expect(serviceErr).To(BeNil()) + }) + } +} + func TestConditionStatusValidation(t *testing.T) { tests := []struct { status string diff --git a/pkg/handlers/validation.go b/pkg/handlers/validation.go index 1433752f..5d50532f 100755 --- a/pkg/handlers/validation.go +++ b/pkg/handlers/validation.go @@ -8,13 +8,18 @@ import ( "time" "unicode/utf8" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) const ( - maxReasonLength = 1024 - maxLabelKeyLen = 317 // 253-char prefix + "/" + 63-char name + maxReasonLength = 1024 + // The Kubernetes label key spec allows up to 317 chars (253-char prefix + "/" + 63-char + // name), but resource_labels.key is VARCHAR(255) — cap at the DB limit so an + // oversized-but-K8s-valid key fails here with a clean 400 instead of during resource + // conversion with a 500 (api.ValidateLabel / api.MaxLabelKeyLen enforce the same bound). + maxLabelKeyLen = api.MaxLabelKeyLen maxLabelValueLen = 63 maxObservedTimeSkew = 5 * time.Minute // tolerance for clock skew between adapter pods and API server maxObservedTimeAge = 30 * time.Minute // matches Sentinel staleness health check window diff --git a/pkg/handlers/validation_test.go b/pkg/handlers/validation_test.go index d7b44fe1..fc467bb2 100644 --- a/pkg/handlers/validation_test.go +++ b/pkg/handlers/validation_test.go @@ -639,6 +639,30 @@ func TestValidateLabels_InvalidValues(t *testing.T) { } } +// TestValidateLabels_KeyExceedsDBColumnLimit verifies that a label key which is +// valid per the Kubernetes label-key spec (max 317 chars: 253-char DNS prefix + +// "/" + 63-char name) but exceeds the resource_labels.key VARCHAR(255) column +// is rejected here with a clean 400, instead of passing this check and later +// blowing up as a 500 during resource conversion (api.ValidateLabel). +func TestValidateLabels_KeyExceedsDBColumnLimit(t *testing.T) { + RegisterTestingT(t) + + seg := strings.Repeat("a", 62) + prefix := strings.Join([]string{seg, seg, seg, seg}, ".") // 4*62 + 3 = 251 chars, each segment valid DNS-1123 + name := strings.Repeat("b", 63) + key := prefix + "/" + name // 251 + 1 + 63 = 315 chars: valid K8s key, exceeds VARCHAR(255) + Expect(len(key)).To(Equal(315)) + Expect(isValidLabelKeyPrefix(prefix)).To(BeTrue(), "prefix must be spec-valid for this test to be meaningful") + + labels := map[string]string{key: "valid-value"} + req := openapi.ClusterCreateRequest{Labels: &labels} + validator := validateLabels(&req, "Labels") + err := validator() + + Expect(err).ToNot(BeNil(), "expected an oversized-but-K8s-valid key to fail validation") + Expect(err.Reason).To(ContainSubstring("label validation failed")) +} + func TestValidateLabels_NilLabels(t *testing.T) { RegisterTestingT(t) diff --git a/pkg/services/generic_test.go b/pkg/services/generic_test.go index 8a6f4548..d29b1b14 100755 --- a/pkg/services/generic_test.go +++ b/pkg/services/generic_test.go @@ -33,6 +33,14 @@ func TestSQLTranslation(t *testing.T) { "search": "= = =", "error": errors.CodeBadRequest + ": Failed to parse search query: = = =", }, + // "properties." was a leftover alias from before the resources table's JSONB + // column was renamed to "spec" — there is no "properties" column, so this must + // be rejected as an unknown field rather than translated into a query against a + // nonexistent column (which would surface as a raw pq: error at execution time). + { + "search": "properties.owner = 'team_a'", + "error": errors.CodeBadRequest + ": properties.owner is not a valid field name", + }, } for _, test := range errorTests { var list []api.Resource @@ -132,11 +140,6 @@ func TestSQLTranslation(t *testing.T) { "sql": "spec->'release'->>'version' = ?", "values": ConsistOf("2"), }, - { - "search": "properties.owner = 'team_a'", - "sql": "properties ->> 'owner' = ?", - "values": ConsistOf("team_a"), - }, } for _, test := range jsonbRelatedTableTests { var list []api.Resource diff --git a/test/integration/search_field_mapping_test.go b/test/integration/search_field_mapping_test.go index f21c9196..8c3a33ed 100644 --- a/test/integration/search_field_mapping_test.go +++ b/test/integration/search_field_mapping_test.go @@ -904,6 +904,148 @@ func TestSearchConditionSubfieldInvalidSubfield(t *testing.T) { Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) } +// TestSearchTypedFieldValidation verifies that malformed values for typed +// top-level columns (generation: INTEGER; created_time/updated_time/deleted_time: +// TIMESTAMPTZ) are rejected with a clean 400 HYPERFLEET-VAL-* error — never a +// raw Postgres pq: driver error or SQLSTATE code — on both the kind-agnostic +// /resources endpoint and typed entity endpoints like /clusters. +func TestSearchTypedFieldValidation(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + assertCleanBadRequest := func(body []byte, statusCode int) { + Expect(statusCode).To(Equal(http.StatusBadRequest)) + bodyStr := string(body) + Expect(bodyStr).ToNot(ContainSubstring("pq:")) + Expect(bodyStr).ToNot(ContainSubstring("SQLSTATE")) + Expect(bodyStr).To(ContainSubstring("HYPERFLEET-VAL-")) + } + + t.Run("GET /resources?search=generation='abc' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("generation='abc'") + params := &openapi.GetResourcesParams{Search: &search} + resp, err := client.GetResourcesWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + t.Run("GET /resources?search=created_time='not-a-date' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("created_time='not-a-date'") + params := &openapi.GetResourcesParams{Search: &search} + resp, err := client.GetResourcesWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + t.Run("GET /clusters?search=created_time='not-a-date' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("created_time='not-a-date'") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + t.Run("GET /clusters?search=generation='abc' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("generation='abc'") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + t.Run("GET /clusters?search=updated_time='not-a-date' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("updated_time='not-a-date'") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + t.Run("GET /clusters?search=deleted_time='not-a-date' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("deleted_time='not-a-date'") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + // Regression guard: valid searches on the same typed fields must keep working. + t.Run("GET /clusters?search=generation=1 still returns 200", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("generation=1") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusOK)) + }) + + t.Run("GET /clusters?search=created_time>'2020-01-01T00:00:00Z' still returns 200", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("created_time>'2020-01-01T00:00:00Z'") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusOK)) + }) +} + +// TestSearchPropertiesFieldRejected verifies that "properties.xxx" — a leftover alias +// from before the resources table's JSONB column was renamed to "spec" — is rejected +// as an unknown field with a clean 400, instead of being translated into a query +// against the nonexistent "properties" column and leaking a raw pq: 42703 error as a 500. +func TestSearchPropertiesFieldRejected(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + assertCleanBadRequest := func(body []byte, statusCode int) { + Expect(statusCode).To(Equal(http.StatusBadRequest)) + bodyStr := string(body) + Expect(bodyStr).ToNot(ContainSubstring("pq:")) + Expect(bodyStr).ToNot(ContainSubstring("SQLSTATE")) + Expect(bodyStr).To(ContainSubstring("HYPERFLEET-VAL-")) + } + + t.Run("GET /resources?search=properties.foo='bar' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("properties.foo='bar'") + params := &openapi.GetResourcesParams{Search: &search} + resp, err := client.GetResourcesWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) + + t.Run("GET /clusters?search=properties.foo='bar' returns clean 400", func(t *testing.T) { + RegisterTestingT(t) + search := openapi.SearchParams("properties.foo='bar'") + params := &openapi.GetClustersParams{Search: &search} + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + assertCleanBadRequest(resp.Body, resp.StatusCode()) + }) +} + // TestSearchNodePoolConditionSubfieldLastUpdatedTime verifies that condition subfield queries // work for NodePools — same code path as Clusters but validates the full end-to-end for NodePools. func TestSearchNodePoolConditionSubfieldLastUpdatedTime(t *testing.T) { From 608c7f8943f15f2ff334b79176febadba74a84ec Mon Sep 17 00:00:00 2001 From: mliptak Date: Thu, 23 Jul 2026 12:51:08 +0200 Subject: [PATCH 2/3] HYPERFLEET-1375 - fix: validate typed search fields on either operand side validateTypedFieldValues only checked the field-on-left form (field = value), so field-on-right comparisons like 'not-a-date' < created_time or 'abc' = generation skipped validation and reached Postgres unvalidated, reintroducing the pq:/SQLSTATE leak this PR is meant to close. Also collapses the per-type error-message switch into a single lookup map for a simpler, still field-type-aware error message. Co-Authored-By: Claude --- pkg/db/sql_helpers.go | 89 ++++++++++++++++++++------------------ pkg/db/sql_helpers_test.go | 28 +++++++++--- 2 files changed, 68 insertions(+), 49 deletions(-) diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index e9044e1d..8cd24d06 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -582,20 +582,28 @@ var searchTypedFields = map[string]tsl.Kind{ "deleted_time": tsl.KindTimestampLiteral, } +// typedKindHints gives a human-readable description of each tsl.Kind used in +// searchTypedFields, for use in validation error messages. +var typedKindHints = map[tsl.Kind]string{ + tsl.KindNumericLiteral: "an integer", + tsl.KindTimestampLiteral: "an RFC3339 timestamp (e.g. 2026-01-01T00:00:00Z)", +} + // validateTypedFieldValues walks binary comparisons against a searchTypedFields -// column and rejects values whose TSL literal kind doesn't match the column's -// SQL type, before the query reaches Postgres as a raw pq: error. +// column — on either side of the operator, since TSL allows both orders (see +// wrapSpecNumericCasts below) — and rejects values whose TSL literal kind +// doesn't match the column's SQL type, before the query reaches Postgres as a +// raw pq: error. func validateTypedFieldValues(n *tsl.Node) *errors.ServiceError { if n == nil { return nil } - if n.Kind == tsl.KindBinaryExpr && n.Left != nil && n.Left.Kind == tsl.KindIdentifier { - if field, ok := n.Left.Value.(string); ok { - if wantKind, tracked := searchTypedFields[field]; tracked { - if err := validateTypedValue(field, wantKind, n.Right); err != nil { - return err - } - } + if n.Kind == tsl.KindBinaryExpr { + if err := validateTypedOperand(n.Left, n.Right); err != nil { + return err + } + if err := validateTypedOperand(n.Right, n.Left); err != nil { + return err } } if err := validateTypedFieldValues(n.Left); err != nil { @@ -612,48 +620,43 @@ func validateTypedFieldValues(n *tsl.Node) *errors.ServiceError { return nil } -// validateTypedValue checks a single value node (or, for IN queries, each -// element of an array literal) against the expected TSL literal kind. -func validateTypedValue(field string, wantKind tsl.Kind, right *tsl.Node) *errors.ServiceError { - if right == nil { - return errors.BadRequest("missing value for field '%s'", field) +// validateTypedOperand checks whether ident is a tracked typed-field identifier +// and, if so, validates value against that field's expected TSL literal kind. +// It's a no-op when ident isn't a tracked identifier (e.g. it's the value side). +func validateTypedOperand(ident, value *tsl.Node) *errors.ServiceError { + if ident == nil || ident.Kind != tsl.KindIdentifier { + return nil } - - if right.Kind == tsl.KindArrayLiteral { - for _, child := range right.Children { - if !matchesTypedKind(wantKind, child.Kind) { - return typedFieldError(field, wantKind, child.Value) - } - } + field, ok := ident.Value.(string) + if !ok { return nil } - - if !matchesTypedKind(wantKind, right.Kind) { - return typedFieldError(field, wantKind, right.Value) + wantKind, tracked := searchTypedFields[field] + if !tracked { + return nil } - return nil + return validateTypedValue(field, wantKind, value) } -// matchesTypedKind reports whether a literal kind is acceptable for wantKind. -// A date literal (no time component) is accepted wherever a timestamp is expected. -func matchesTypedKind(wantKind, gotKind tsl.Kind) bool { - if gotKind == wantKind { - return true +// validateTypedValue checks a single value node (or, for IN queries, each +// element of an array literal) against the expected TSL literal kind. A date +// literal (no time component) is accepted wherever a timestamp is expected. +func validateTypedValue(field string, wantKind tsl.Kind, value *tsl.Node) *errors.ServiceError { + if value == nil { + return errors.BadRequest("missing value for field '%s'", field) } - return wantKind == tsl.KindTimestampLiteral && gotKind == tsl.KindDateLiteral -} - -func typedFieldError(field string, wantKind tsl.Kind, got any) *errors.ServiceError { - switch wantKind { - case tsl.KindNumericLiteral: - return errors.BadRequest("field '%s' expects an integer value, got %v", field, got) - case tsl.KindTimestampLiteral: - return errors.BadRequest( - "field '%s' expects an RFC3339 timestamp value (e.g. 2026-01-01T00:00:00Z), got %v", field, got, - ) - default: - return errors.BadRequest("field '%s' has an unsupported value type", field) + if value.Kind == tsl.KindArrayLiteral { + for _, child := range value.Children { + if err := validateTypedValue(field, wantKind, child); err != nil { + return err + } + } + return nil + } + if value.Kind == wantKind || (wantKind == tsl.KindTimestampLiteral && value.Kind == tsl.KindDateLiteral) { + return nil } + return errors.BadRequest("field '%s' expects %s", field, typedKindHints[wantKind]) } // wrapSpecNumericCasts wraps spec JSONB fields in CAST(... AS numeric) when diff --git a/pkg/db/sql_helpers_test.go b/pkg/db/sql_helpers_test.go index 00970726..b668361a 100644 --- a/pkg/db/sql_helpers_test.go +++ b/pkg/db/sql_helpers_test.go @@ -815,7 +815,7 @@ func TestFieldNameWalk_TypedFieldValidation(t *testing.T) { name: "generation with non-integer string value", searchQuery: "generation = 'abc'", expectError: true, - errorContains: "field 'generation' expects an integer value", + errorContains: "field 'generation' expects an integer", }, { name: "created_time with valid RFC3339 timestamp", @@ -825,25 +825,25 @@ func TestFieldNameWalk_TypedFieldValidation(t *testing.T) { name: "created_time with non-timestamp string value", searchQuery: "created_time = 'not-a-date'", expectError: true, - errorContains: "field 'created_time' expects an RFC3339 timestamp value", + errorContains: "field 'created_time' expects an RFC3339 timestamp", }, { name: "updated_time with non-timestamp string value", searchQuery: "updated_time = 'not-a-date'", expectError: true, - errorContains: "field 'updated_time' expects an RFC3339 timestamp value", + errorContains: "field 'updated_time' expects an RFC3339 timestamp", }, { name: "deleted_time with non-timestamp string value", searchQuery: "deleted_time = 'not-a-date'", expectError: true, - errorContains: "field 'deleted_time' expects an RFC3339 timestamp value", + errorContains: "field 'deleted_time' expects an RFC3339 timestamp", }, { name: "generation IN list with a non-integer element", searchQuery: "generation IN [1, 'abc']", expectError: true, - errorContains: "field 'generation' expects an integer value", + errorContains: "field 'generation' expects an integer", }, { name: "generation IN list with all integers", @@ -857,7 +857,23 @@ func TestFieldNameWalk_TypedFieldValidation(t *testing.T) { name: "combined query: error surfaces even when other predicate is valid", searchQuery: "name = 'ok' AND generation = 'abc'", expectError: true, - errorContains: "field 'generation' expects an integer value", + errorContains: "field 'generation' expects an integer", + }, + { + name: "generation with literal-first comparison (field on right)", + searchQuery: "'abc' = generation", + expectError: true, + errorContains: "field 'generation' expects an integer", + }, + { + name: "created_time with literal-first comparison (field on right)", + searchQuery: "'not-a-date' < created_time", + expectError: true, + errorContains: "field 'created_time' expects an RFC3339 timestamp", + }, + { + name: "generation with literal-first comparison and valid value", + searchQuery: "1 < generation", }, } From b030838dcb6ec44e2a54c9aa86c73222024cbc34 Mon Sep 17 00:00:00 2001 From: mliptak Date: Mon, 27 Jul 2026 08:58:01 +0200 Subject: [PATCH 3/3] HYPERFLEET-1375 - fix: accept null in typed search field validation deleted_time IS NULL / IS NOT NULL is a standard soft-delete filter and was being incorrectly rejected by the new typed-column search validation. Also hardens the error hint for unmapped tsl.Kind values, converts TestSearchTypedFieldValidation to table-driven (closing a NodePool coverage gap in the same pass), and trims two duplicated doc comments. Co-Authored-By: Claude --- pkg/db/sql_helpers.go | 11 +- pkg/db/sql_helpers_test.go | 12 ++ pkg/handlers/validation_test.go | 7 +- test/integration/search_field_mapping_test.go | 135 +++++++----------- 4 files changed, 77 insertions(+), 88 deletions(-) diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index 8cd24d06..09578d3b 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -641,6 +641,8 @@ func validateTypedOperand(ident, value *tsl.Node) *errors.ServiceError { // validateTypedValue checks a single value node (or, for IN queries, each // element of an array literal) against the expected TSL literal kind. A date // literal (no time component) is accepted wherever a timestamp is expected. +// null is always accepted, since typed columns here are nullable (e.g. +// deleted_time IS NULL / IS NOT NULL is a standard soft-delete filter). func validateTypedValue(field string, wantKind tsl.Kind, value *tsl.Node) *errors.ServiceError { if value == nil { return errors.BadRequest("missing value for field '%s'", field) @@ -653,10 +655,17 @@ func validateTypedValue(field string, wantKind tsl.Kind, value *tsl.Node) *error } return nil } + if value.Kind == tsl.KindNullLiteral { + return nil + } if value.Kind == wantKind || (wantKind == tsl.KindTimestampLiteral && value.Kind == tsl.KindDateLiteral) { return nil } - return errors.BadRequest("field '%s' expects %s", field, typedKindHints[wantKind]) + hint, ok := typedKindHints[wantKind] + if !ok { + hint = fmt.Sprintf("a value of kind %s", wantKind) + } + return errors.BadRequest("field '%s' expects %s", field, hint) } // wrapSpecNumericCasts wraps spec JSONB fields in CAST(... AS numeric) when diff --git a/pkg/db/sql_helpers_test.go b/pkg/db/sql_helpers_test.go index b668361a..1c25cdcb 100644 --- a/pkg/db/sql_helpers_test.go +++ b/pkg/db/sql_helpers_test.go @@ -875,6 +875,18 @@ func TestFieldNameWalk_TypedFieldValidation(t *testing.T) { name: "generation with literal-first comparison and valid value", searchQuery: "1 < generation", }, + { + name: "deleted_time IS NULL is accepted (soft-delete filter)", + searchQuery: "deleted_time IS NULL", + }, + { + name: "deleted_time IS NOT NULL is accepted (soft-delete filter)", + searchQuery: "deleted_time IS NOT NULL", + }, + { + name: "created_time with valid date literal (no time component)", + searchQuery: "created_time > '2026-01-01'", + }, } for _, tt := range tests { diff --git a/pkg/handlers/validation_test.go b/pkg/handlers/validation_test.go index fc467bb2..fd294c08 100644 --- a/pkg/handlers/validation_test.go +++ b/pkg/handlers/validation_test.go @@ -639,11 +639,8 @@ func TestValidateLabels_InvalidValues(t *testing.T) { } } -// TestValidateLabels_KeyExceedsDBColumnLimit verifies that a label key which is -// valid per the Kubernetes label-key spec (max 317 chars: 253-char DNS prefix + -// "/" + 63-char name) but exceeds the resource_labels.key VARCHAR(255) column -// is rejected here with a clean 400, instead of passing this check and later -// blowing up as a 500 during resource conversion (api.ValidateLabel). +// TestValidateLabels_KeyExceedsDBColumnLimit exercises the maxLabelKeyLen bound +// (see its doc comment) with a key that is K8s-spec-valid but DB-column-oversized. func TestValidateLabels_KeyExceedsDBColumnLimit(t *testing.T) { RegisterTestingT(t) diff --git a/test/integration/search_field_mapping_test.go b/test/integration/search_field_mapping_test.go index 8c3a33ed..aac99546 100644 --- a/test/integration/search_field_mapping_test.go +++ b/test/integration/search_field_mapping_test.go @@ -924,92 +924,63 @@ func TestSearchTypedFieldValidation(t *testing.T) { Expect(bodyStr).To(ContainSubstring("HYPERFLEET-VAL-")) } - t.Run("GET /resources?search=generation='abc' returns clean 400", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("generation='abc'") - params := &openapi.GetResourcesParams{Search: &search} - resp, err := client.GetResourcesWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - assertCleanBadRequest(resp.Body, resp.StatusCode()) - }) - - t.Run("GET /resources?search=created_time='not-a-date' returns clean 400", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("created_time='not-a-date'") - params := &openapi.GetResourcesParams{Search: &search} - resp, err := client.GetResourcesWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - assertCleanBadRequest(resp.Body, resp.StatusCode()) - }) - - t.Run("GET /clusters?search=created_time='not-a-date' returns clean 400", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("created_time='not-a-date'") - params := &openapi.GetClustersParams{Search: &search} - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - assertCleanBadRequest(resp.Body, resp.StatusCode()) - }) - - t.Run("GET /clusters?search=generation='abc' returns clean 400", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("generation='abc'") - params := &openapi.GetClustersParams{Search: &search} - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - assertCleanBadRequest(resp.Body, resp.StatusCode()) - }) - - t.Run("GET /clusters?search=updated_time='not-a-date' returns clean 400", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("updated_time='not-a-date'") - params := &openapi.GetClustersParams{Search: &search} - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - assertCleanBadRequest(resp.Body, resp.StatusCode()) - }) - - t.Run("GET /clusters?search=deleted_time='not-a-date' returns clean 400", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("deleted_time='not-a-date'") - params := &openapi.GetClustersParams{Search: &search} - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - assertCleanBadRequest(resp.Body, resp.StatusCode()) - }) - - // Regression guard: valid searches on the same typed fields must keep working. - t.Run("GET /clusters?search=generation=1 still returns 200", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("generation=1") - params := &openapi.GetClustersParams{Search: &search} - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) - - Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusOK)) - }) + // search performs the GET against the given kind-agnostic/typed entity endpoint + // and returns the raw response body and status code for the table below to assert on. + search := func(endpoint, query string) ([]byte, int) { + params := openapi.SearchParams(query) + authToken := test.WithAuthToken(ctx) + switch endpoint { + case "resources": + resp, err := client.GetResourcesWithResponse(ctx, &openapi.GetResourcesParams{Search: ¶ms}, authToken) + Expect(err).NotTo(HaveOccurred()) + return resp.Body, resp.StatusCode() + case "clusters": + resp, err := client.GetClustersWithResponse(ctx, &openapi.GetClustersParams{Search: ¶ms}, authToken) + Expect(err).NotTo(HaveOccurred()) + return resp.Body, resp.StatusCode() + case "nodepools": + resp, err := client.GetNodePoolsWithResponse(ctx, &openapi.GetNodePoolsParams{Search: ¶ms}, authToken) + Expect(err).NotTo(HaveOccurred()) + return resp.Body, resp.StatusCode() + default: + t.Fatalf("unknown endpoint %q", endpoint) + return nil, 0 + } + } - t.Run("GET /clusters?search=created_time>'2020-01-01T00:00:00Z' still returns 200", func(t *testing.T) { - RegisterTestingT(t) - search := openapi.SearchParams("created_time>'2020-01-01T00:00:00Z'") - params := &openapi.GetClustersParams{Search: &search} - resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + cases := []struct { + endpoint string + query string + wantOK bool + }{ + {"resources", "generation='abc'", false}, + {"resources", "created_time='not-a-date'", false}, + {"clusters", "created_time='not-a-date'", false}, + {"clusters", "generation='abc'", false}, + {"clusters", "updated_time='not-a-date'", false}, + {"clusters", "deleted_time='not-a-date'", false}, + {"nodepools", "generation='abc'", false}, + // Regression guard: valid searches on the same typed fields must keep working. + {"clusters", "generation=1", true}, + {"clusters", "created_time>'2020-01-01T00:00:00Z'", true}, + } - Expect(err).NotTo(HaveOccurred()) - Expect(resp.StatusCode()).To(Equal(http.StatusOK)) - }) + for _, tc := range cases { + t.Run(fmt.Sprintf("GET /%s?search=%s", tc.endpoint, tc.query), func(t *testing.T) { + RegisterTestingT(t) + body, statusCode := search(tc.endpoint, tc.query) + if !tc.wantOK { + assertCleanBadRequest(body, statusCode) + return + } + Expect(statusCode).To(Equal(http.StatusOK)) + }) + } } -// TestSearchPropertiesFieldRejected verifies that "properties.xxx" — a leftover alias -// from before the resources table's JSONB column was renamed to "spec" — is rejected -// as an unknown field with a clean 400, instead of being translated into a query -// against the nonexistent "properties" column and leaking a raw pq: 42703 error as a 500. +// TestSearchPropertiesFieldRejected is the end-to-end counterpart of +// TestSQLTranslation's "properties.owner" case (generic_test.go) — same alias, +// asserting the clean-400 behavior over HTTP instead of at the SQL-translation layer. func TestSearchPropertiesFieldRejected(t *testing.T) { RegisterTestingT(t) h, client := test.RegisterIntegration(t)