diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index 09578d3b..5a320e8f 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -69,34 +69,10 @@ func getField(name string) (field string, err *errors.ServiceError) { return } - baseName := fieldParts[0] - if !searchAllowedFields[baseName] { - err = errors.BadRequest("%s is not a valid field name", name) - return - } - field = trimmedName return } -var searchAllowedFields = map[string]bool{ - "id": true, - "name": true, - "kind": true, - "created_time": true, - "updated_time": true, - "deleted_time": true, - "created_by": true, - "updated_by": true, - "deleted_by": true, - "generation": true, - "href": true, - "labels": true, - "conditions": true, - "owner_id": true, - "owner_kind": true, -} - // Condition type validation pattern: PascalCase condition types (e.g., Reconciled, Available, Progressing) var conditionTypePattern = regexp.MustCompile(`^[A-Z][a-zA-Z0-9]*$`) @@ -751,27 +727,11 @@ func IdentWalk(n *tsl.Node, check func(string) (string, error)) (*tsl.Node, erro } } -// orderAllowedFields defines the whitelist of fields that are allowed to be ordered. -// This prevents SQL injection and restricts invalid order queries. -var orderAllowedFields = map[string]bool{ - "id": true, - "name": true, - "created_time": true, - "updated_time": true, - "deleted_time": true, - "kind": true, - "created_by": true, - "updated_by": true, - "deleted_by": true, - "generation": true, - "href": true, -} - // orderPattern matches valid order syntax: field name (letters, digits, underscore) followed by optional asc/desc. // This regex rejects SQL injection attempts (semicolons, parentheses, dashes, comments, etc). var orderPattern = regexp.MustCompile(`^[a-z_][a-z_]*(\s+(asc|desc))?$`) -// ArgsToOrder validates and cleans order arguments against the allowed fields whitelist. +// ArgsToOrder validates and cleans order arguments. // Returns a cleaned list of order clauses in the format ["field direction", ...] // Empty or whitespace-only strings are silently skipped. func ArgsToOrder(args []string) (cleanedOrderList []string, err *errors.ServiceError) { @@ -809,11 +769,6 @@ func ArgsToOrder(args []string) (cleanedOrderList []string, err *errors.ServiceE return nil, errors.BadRequest("invalid order format '%s': expected 'field' or 'field asc|desc'", val) } - // Validate field against orderAllowedFields - if !orderAllowedFields[field] { - return nil, errors.BadRequest("field '%s' is not allowed for ordering", field) - } - cleanedValue := fmt.Sprintf("%s %s", field, direction) cleanedOrderList = append(cleanedOrderList, cleanedValue) } diff --git a/pkg/db/sql_helpers_test.go b/pkg/db/sql_helpers_test.go index 1c25cdcb..89858a6e 100644 --- a/pkg/db/sql_helpers_test.go +++ b/pkg/db/sql_helpers_test.go @@ -1030,18 +1030,6 @@ func TestArgsToOrder(t *testing.T) { input: []string{"name asc", "", "created_time desc", " ", "\t"}, expected: []string{"name asc", "created_time desc"}, }, - { - name: "mixed valid and invalid field", - input: []string{"created_time desc", "name", "wrong_field"}, - expectError: true, - errorContains: "not allowed for ordering", - }, - { - name: "field not in whitelist", - input: []string{"custom_field asc"}, - expectError: true, - errorContains: "not allowed for ordering", - }, { name: "deleted_time field", input: []string{"deleted_time desc"}, diff --git a/pkg/db/transaction_middleware.go b/pkg/db/transaction_middleware.go index fa04cda0..1d3752df 100755 --- a/pkg/db/transaction_middleware.go +++ b/pkg/db/transaction_middleware.go @@ -8,6 +8,8 @@ import ( "net/http" "time" + "github.com/lib/pq" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/response" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" @@ -74,6 +76,13 @@ func TransactionMiddleware(next http.Handler, connection SessionFactory, request }) } +// IsUndefinedColumnError reports whether err is "undefined_column" error, +// this occurs when a query references a column that does not exist in the schema +func IsUndefinedColumnError(err error) bool { + var pqErr *pq.Error + return stderrors.As(err, &pqErr) && pqErr.Code == "42703" +} + // IsDBConnectionError indicates whether err is an infrastructure failure // (network unreachable, connection refused, connection dropped) func IsDBConnectionError(err error) bool { diff --git a/pkg/services/generic.go b/pkg/services/generic.go index 301836e2..13b9077b 100755 --- a/pkg/services/generic.go +++ b/pkg/services/generic.go @@ -278,6 +278,11 @@ func (s *sqlGenericService) loadList(listCtx *listContext, d dao.GenericDao) *er listCtx.pagingMeta.Size = 0 case db.IsDBConnectionError(err): return errors.ServiceUnavailable("Database connection unavailable") + case db.IsUndefinedColumnError(err): + if listCtx.args.Search != "" || len(listCtx.args.Order) > 0 { + return errors.BadRequest("invalid field in search or order query") + } + return errors.GeneralError("Unable to list resources: %s", err) default: return errors.GeneralError("Unable to list resources: %s", err) } diff --git a/pkg/services/generic_test.go b/pkg/services/generic_test.go index dae7dd02..75a930ff 100755 --- a/pkg/services/generic_test.go +++ b/pkg/services/generic_test.go @@ -33,14 +33,6 @@ 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 diff --git a/test/integration/order_field_mapping_test.go b/test/integration/order_field_mapping_test.go index 497fc585..c7267b10 100644 --- a/test/integration/order_field_mapping_test.go +++ b/test/integration/order_field_mapping_test.go @@ -150,9 +150,9 @@ func TestOrderFieldValidation(t *testing.T) { expectedError string }{ { - name: "InvalidFieldName", + name: "NonexistentField", order: "nonexistent_field asc", - expectedError: "not allowed for ordering", + expectedError: "invalid field in search or order query", }, { name: "InvalidDirection", @@ -203,7 +203,7 @@ func TestOrderFieldValidation(t *testing.T) { } } -// TestOrderAllowedFields verifies that all whitelisted fields work correctly +// TestOrderAllowedFields verifies that standard fields work correctly for ordering func TestOrderAllowedFields(t *testing.T) { RegisterTestingT(t) h, client := test.RegisterIntegration(t) diff --git a/test/integration/search_field_mapping_test.go b/test/integration/search_field_mapping_test.go index aac99546..5c606c78 100644 --- a/test/integration/search_field_mapping_test.go +++ b/test/integration/search_field_mapping_test.go @@ -1070,3 +1070,23 @@ func TestSearchNodePoolConditionSubfieldLastUpdatedTime(t *testing.T) { } Expect(foundStale).To(BeTrue(), "Expected to find the stale node pool") } + +// TestSearchNonexistentField verifies that searching on a field that does not exist +// in the database schema returns a 400 Bad Request +func TestSearchNonexistentField(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + search := openapi.SearchParams("nonexistent_column = 'foo'") + params := &openapi.GetClustersParams{ + Search: &search, + } + resp, err := client.GetClustersWithResponse(ctx, params, test.WithAuthToken(ctx)) + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest)) + Expect(string(resp.Body)).To(ContainSubstring("invalid field in search or order query")) +}