Skip to content

Commit 7f4f829

Browse files
ma-hillma-hill
committed
HYPERFLEET-1259 - fix: SQL injection protection and allowlist for orderBy
1 parent 43c0474 commit 7f4f829

5 files changed

Lines changed: 514 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+
// ArgsToOrderBy 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 ArgsToOrderBy(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: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -834,3 +834,177 @@ func TestConditionStatusValidation(t *testing.T) {
834834
})
835835
}
836836
}
837+
838+
func TestArgsToOrderBy(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: "uppercase field name",
901+
input: []string{"NAME asc"},
902+
expectError: true,
903+
errorContains: "invalid order format",
904+
},
905+
{
906+
name: "uppercase direction",
907+
input: []string{"name ASC"},
908+
expectError: true,
909+
errorContains: "invalid order format",
910+
},
911+
{
912+
name: "empty string in array is skipped",
913+
input: []string{""},
914+
expected: nil,
915+
},
916+
{
917+
name: "empty string in array with field at end",
918+
input: []string{"", "", "", "", "", "kind asc", "href desc"},
919+
expected: []string{"kind asc", "href desc"},
920+
},
921+
{
922+
name: "whitespace only string is skipped",
923+
input: []string{" "},
924+
expected: nil,
925+
},
926+
{
927+
name: "mixed valid and empty strings and tabs",
928+
input: []string{"name asc", "", "created_time desc", " ", "\t"},
929+
expected: []string{"name asc", "created_time desc"},
930+
},
931+
{
932+
name: "mixed valid and invalid field",
933+
input: []string{"created_time desc", "name", "wrong_field"},
934+
expectError: true,
935+
errorContains: "not allowed for ordering",
936+
},
937+
{
938+
name: "field not in whitelist",
939+
input: []string{"custom_field asc"},
940+
expectError: true,
941+
errorContains: "not allowed for ordering",
942+
},
943+
{
944+
name: "deleted_time field",
945+
input: []string{"deleted_time desc"},
946+
expected: []string{"deleted_time desc"},
947+
},
948+
{
949+
name: "generation field",
950+
input: []string{"generation asc"},
951+
expected: []string{"generation asc"},
952+
},
953+
{
954+
name: "too many parts",
955+
input: []string{"name asc extra"},
956+
expectError: true,
957+
errorContains: "invalid order format",
958+
},
959+
{
960+
name: "empty array",
961+
input: []string{},
962+
expected: nil,
963+
},
964+
}
965+
966+
for _, tt := range tests {
967+
t.Run(tt.name, func(t *testing.T) {
968+
RegisterTestingT(t)
969+
970+
result, err := ArgsToOrderBy(tt.input)
971+
972+
if tt.expectError {
973+
Expect(err).ToNot(BeNil(), "expected error but got nil")
974+
if tt.errorContains != "" {
975+
Expect(err.Reason).To(ContainSubstring(tt.errorContains))
976+
}
977+
} else {
978+
Expect(err).To(BeNil(), "unexpected error: %v", err)
979+
Expect(result).To(Equal(tt.expected))
980+
}
981+
})
982+
}
983+
}
984+
985+
func TestArgsToOrder_SecurityValidation(t *testing.T) {
986+
RegisterTestingT(t)
987+
988+
// SQL injection attempts that should all fail
989+
injectionAttempts := []struct {
990+
name string
991+
input string
992+
}{
993+
{"union injection", "name UNION SELECT password FROM users"},
994+
{"comment injection", "name--"},
995+
{"semicolon terminator", "name; DROP TABLE resources;"},
996+
{"quote escape", "name' OR '1'='1"},
997+
{"parentheses", "name) OR (1=1"},
998+
{"wildcard", "name*"},
999+
{"backtick", "name`"},
1000+
}
1001+
1002+
for _, tt := range injectionAttempts {
1003+
t.Run(tt.name, func(t *testing.T) {
1004+
RegisterTestingT(t)
1005+
1006+
_, err := ArgsToOrderBy([]string{tt.input})
1007+
Expect(err).ToNot(BeNil(), "injection attempt '%s' should be rejected", tt.input)
1008+
})
1009+
}
1010+
}

‎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.Order) != 0 {
145-
orderByArgs, serviceErr := db.ArgsToOrderBy(listCtx.args.Order, *listCtx.disallowedFields)
145+
cleanedOrderList, serviceErr := db.ArgsToOrderBy(listCtx.args.Order)
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)