Skip to content

Commit cc225eb

Browse files
ma-hillma-hill
committed
HYPERFLEET-1259 - fix: SQL injection protection and allowlist for orderBy
1 parent 177f746 commit cc225eb

5 files changed

Lines changed: 500 additions & 37 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
4848

4949
### Fixed
5050

51+
- Restricted `orderBy` query parameter to only allow specific whitelisted fields ([#244](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/244))
5152
- Validated adapter status conditions in handler layer ([#88](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/88))
5253
- Removed org prefix from image.repository default ([#86](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/86))
5354
- Addressed revive linter violations from enabled linting standard ([#85](https://github.com/openshift-hyperfleet/hyperfleet-api/pull/85))

‎pkg/db/sql_helpers.go‎

Lines changed: 59 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -553,49 +553,74 @@ func IdentWalk(n *tsl.Node, check func(string) (string, error)) (*tsl.Node, erro
553553
}
554554
}
555555

556-
// cleanOrderBy takes the orderBy arg and cleans it.
557-
func cleanOrderBy(userArg string, disallowedFields map[string]string) (orderBy string, err *errors.ServiceError) {
558-
var orderField string
556+
// orderAllowedFields defines the whitelist of fields that are allowed to be ordered.
557+
// This prevents SQL injection and restricts invalid order queries.
558+
var orderAllowedFields = map[string]bool{
559+
"id": true,
560+
"name": true,
561+
"created_time": true,
562+
"updated_time": true,
563+
"deleted_time": true,
564+
"kind": true,
565+
"created_by": true,
566+
"updated_by": true,
567+
"deleted_by": true,
568+
"generation": true,
569+
"href": true,
570+
}
559571

560-
trimedName := strings.Trim(userArg, " ")
572+
// orderPattern matches valid order syntax: field name (letters, digits, underscore) followed by optional asc/desc.
573+
// This regex rejects SQL injection attempts (semicolons, parentheses, dashes, comments, etc).
574+
var orderPattern = regexp.MustCompile(`^[a-z_][a-z_]*(\s+(asc|desc))?$`)
575+
576+
// ArgsToOrder validates and cleans order arguments against the allowed fields whitelist.
577+
// Returns a cleaned list of order clauses in the format ["field direction", ...]
578+
// Empty or whitespace-only strings are silently skipped.
579+
func ArgsToOrder(args []string) (cleanedOrderList []string, err *errors.ServiceError) {
580+
for _, val := range args {
581+
// Accept args with trailing and leading spaces
582+
trimVal := strings.TrimSpace(val)
583+
584+
// Skip empty strings silently
585+
if trimVal == "" {
586+
continue
587+
}
561588

562-
order := strings.Split(trimedName, " ")
563-
direction := "none valid"
589+
// Check for SQL injection attempts before parsing
590+
if !orderPattern.MatchString(trimVal) {
591+
return nil, errors.BadRequest("invalid order format '%s': expected 'field' or 'field asc|desc'", val)
592+
}
564593

565-
if len(order) == 1 {
566-
orderField, err = getField(order[0], disallowedFields)
567-
direction = "asc"
568-
} else if len(order) == 2 {
569-
orderField, err = getField(order[0], disallowedFields)
570-
direction = order[1]
571-
}
572-
if err != nil || (direction != "asc" && direction != "desc") {
573-
err = errors.BadRequest("bad order value '%s'", userArg)
574-
return
575-
}
594+
// Each value should be "<field-name>" or "<field-name> asc|desc"
595+
splitVal := strings.Fields(trimVal)
596+
lenVal := len(splitVal)
576597

577-
orderBy = fmt.Sprintf("%s %s", orderField, direction)
578-
return
579-
}
598+
var field, direction string
580599

581-
// ArgsToOrderBy returns cleaned orderBy list.
582-
func ArgsToOrderBy(
583-
orderByArgs []string,
584-
disallowedFields map[string]string,
585-
) (orderBy []string, err *errors.ServiceError) {
586-
var order string
587-
if len(orderByArgs) != 0 {
588-
orderBy = []string{}
589-
for _, o := range orderByArgs {
590-
order, err = cleanOrderBy(o, disallowedFields)
591-
if err != nil {
592-
return
600+
switch lenVal {
601+
case 2:
602+
field = splitVal[0]
603+
direction = splitVal[1]
604+
if direction != "asc" && direction != "desc" {
605+
return nil, errors.BadRequest("invalid sort direction '%s': must be 'asc' or 'desc'", direction)
593606
}
607+
case 1:
608+
field = splitVal[0]
609+
direction = "asc"
610+
default:
611+
return nil, errors.BadRequest("invalid order format '%s': expected 'field' or 'field asc|desc'", val)
612+
}
594613

595-
orderBy = append(orderBy, order)
614+
// Validate field against orderAllowedFields
615+
if !orderAllowedFields[field] {
616+
return nil, errors.BadRequest("field '%s' is not allowed for ordering", field)
596617
}
618+
619+
cleanedValue := fmt.Sprintf("%s %s", field, direction)
620+
cleanedOrderList = append(cleanedOrderList, cleanedValue)
597621
}
598-
return
622+
623+
return cleanedOrderList, nil
599624
}
600625

601626
func GetTableName(g2 *gorm.DB) string {

‎pkg/db/sql_helpers_test.go‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -834,3 +834,165 @@ func TestConditionStatusValidation(t *testing.T) {
834834
})
835835
}
836836
}
837+
838+
func TestArgsToOrder(t *testing.T) {
839+
tests := []struct {
840+
name string
841+
errorContains string
842+
input []string
843+
expected []string
844+
expectError bool
845+
}{
846+
{
847+
name: "single field with asc",
848+
input: []string{"name asc"},
849+
expected: []string{"name asc"},
850+
},
851+
{
852+
name: "single field with desc",
853+
input: []string{"created_time desc"},
854+
expected: []string{"created_time desc"},
855+
},
856+
{
857+
name: "single field without direction defaults to asc",
858+
input: []string{"created_time"},
859+
expected: []string{"created_time asc"},
860+
},
861+
{
862+
name: "multiple fields",
863+
input: []string{"name asc", "created_time desc"},
864+
expected: []string{"name asc", "created_time desc"},
865+
},
866+
{
867+
name: "field with extra spaces",
868+
input: []string{" name asc "},
869+
expected: []string{"name asc"},
870+
},
871+
{
872+
name: "field with tabs and spaces",
873+
input: []string{"name \t desc"},
874+
expected: []string{"name desc"},
875+
},
876+
{
877+
name: "all allowed fields",
878+
input: []string{"id", "name", "created_time", "updated_time", "kind"},
879+
expected: []string{"id asc", "name asc", "created_time asc", "updated_time asc", "kind asc"},
880+
},
881+
{
882+
name: "invalid direction",
883+
input: []string{"name ascending"},
884+
expectError: true,
885+
errorContains: "invalid order format",
886+
},
887+
{
888+
name: "SQL injection attempt - semicolon",
889+
input: []string{"name; DROP TABLE resources"},
890+
expectError: true,
891+
errorContains: "invalid order format",
892+
},
893+
{
894+
name: "SQL injection attempt - comment",
895+
input: []string{"name-- asc"},
896+
expectError: true,
897+
errorContains: "invalid order format",
898+
},
899+
{
900+
name: "empty string in array is skipped",
901+
input: []string{""},
902+
expected: nil,
903+
},
904+
{
905+
name: "empty string in array with field at end",
906+
input: []string{"", "", "", "", "", "kind asc", "href desc"},
907+
expected: []string{"kind asc", "href desc"},
908+
},
909+
{
910+
name: "whitespace only string is skipped",
911+
input: []string{" "},
912+
expected: nil,
913+
},
914+
{
915+
name: "mixed valid and empty strings and tabs",
916+
input: []string{"name asc", "", "created_time desc", " ", "\t"},
917+
expected: []string{"name asc", "created_time desc"},
918+
},
919+
{
920+
name: "mixed valid and invalid field",
921+
input: []string{"created_time desc", "name", "wrong_field"},
922+
expectError: true,
923+
errorContains: "not allowed for ordering",
924+
},
925+
{
926+
name: "field not in whitelist",
927+
input: []string{"custom_field asc"},
928+
expectError: true,
929+
errorContains: "not allowed for ordering",
930+
},
931+
{
932+
name: "deleted_time field",
933+
input: []string{"deleted_time desc"},
934+
expected: []string{"deleted_time desc"},
935+
},
936+
{
937+
name: "generation field",
938+
input: []string{"generation asc"},
939+
expected: []string{"generation asc"},
940+
},
941+
{
942+
name: "too many parts",
943+
input: []string{"name asc extra"},
944+
expectError: true,
945+
errorContains: "invalid order format",
946+
},
947+
{
948+
name: "empty array",
949+
input: []string{},
950+
expected: nil,
951+
},
952+
}
953+
954+
for _, tt := range tests {
955+
t.Run(tt.name, func(t *testing.T) {
956+
RegisterTestingT(t)
957+
958+
result, err := ArgsToOrder(tt.input)
959+
960+
if tt.expectError {
961+
Expect(err).ToNot(BeNil(), "expected error but got nil")
962+
if tt.errorContains != "" {
963+
Expect(err.Reason).To(ContainSubstring(tt.errorContains))
964+
}
965+
} else {
966+
Expect(err).To(BeNil(), "unexpected error: %v", err)
967+
Expect(result).To(Equal(tt.expected))
968+
}
969+
})
970+
}
971+
}
972+
973+
func TestArgsToOrder_SecurityValidation(t *testing.T) {
974+
RegisterTestingT(t)
975+
976+
// SQL injection attempts that should all fail
977+
injectionAttempts := []struct {
978+
name string
979+
input string
980+
}{
981+
{"union injection", "name UNION SELECT password FROM users"},
982+
{"comment injection", "name--"},
983+
{"semicolon terminator", "name; DROP TABLE resources;"},
984+
{"quote escape", "name' OR '1'='1"},
985+
{"parentheses", "name) OR (1=1"},
986+
{"wildcard", "name*"},
987+
{"backtick", "name`"},
988+
}
989+
990+
for _, tt := range injectionAttempts {
991+
t.Run(tt.name, func(t *testing.T) {
992+
RegisterTestingT(t)
993+
994+
_, err := ArgsToOrder([]string{tt.input})
995+
Expect(err).ToNot(BeNil(), "injection attempt '%s' should be rejected", tt.input)
996+
})
997+
}
998+
}

‎pkg/services/generic.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,12 +142,12 @@ func (s *sqlGenericService) buildPreload(listCtx *listContext, d *dao.GenericDao
142142

143143
func (s *sqlGenericService) buildOrderBy(listCtx *listContext, d *dao.GenericDao) (bool, *errors.ServiceError) {
144144
if len(listCtx.args.OrderBy) != 0 {
145-
orderByArgs, serviceErr := db.ArgsToOrderBy(listCtx.args.OrderBy, *listCtx.disallowedFields)
145+
cleanedOrderList, serviceErr := db.ArgsToOrder(listCtx.args.OrderBy)
146146
if serviceErr != nil {
147147
return false, serviceErr
148148
}
149-
for _, orderByArg := range orderByArgs {
150-
(*d).OrderBy(orderByArg)
149+
for _, orderArg := range cleanedOrderList {
150+
(*d).OrderBy(orderArg)
151151
}
152152
}
153153
return false, nil

0 commit comments

Comments
 (0)