Skip to content

Commit 56369d7

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

4 files changed

Lines changed: 542 additions & 36 deletions

File tree

‎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+
// orderByAllowedFields defines the whitelist of fields that are allowed to be ordered.
557+
// This prevents SQL injection and restricts invalid orderBy queries.
558+
var orderByAllowedFields = 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+
// orderByPattern matches valid orderBy 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 orderByPattern = regexp.MustCompile(`^[a-z_][a-z_]*(\s+(asc|desc))?$`)
575+
576+
// ArgsToOrderBy validates and cleans orderBy arguments against the allowed fields whitelist.
577+
// Returns a cleaned list of orderBy clauses in the format ["field direction", ...]
578+
// Empty or whitespace-only strings are silently skipped.
579+
func ArgsToOrderBy(args []string) (cleanedOrderByList []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 !orderByPattern.MatchString(trimVal) {
591+
return nil, errors.BadRequest("invalid orderBy 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 OrderBy can 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 orderBy format '%s': expected 'field' or 'field asc|desc'", val)
612+
}
594613

595-
orderBy = append(orderBy, order)
614+
// Validate field against allowedFields
615+
if !orderByAllowedFields[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+
cleanedOrderByList = append(cleanedOrderByList, cleanedValue)
597621
}
598-
return
622+
623+
return cleanedOrderByList, nil
599624
}
600625

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

‎pkg/db/sql_helpers_test.go‎

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

‎pkg/services/generic.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,11 +142,11 @@ 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+
cleanedOrderByList, serviceErr := db.ArgsToOrderBy(listCtx.args.OrderBy)
146146
if serviceErr != nil {
147147
return false, serviceErr
148148
}
149-
for _, orderByArg := range orderByArgs {
149+
for _, orderByArg := range cleanedOrderByList {
150150
(*d).OrderBy(orderByArg)
151151
}
152152
}

0 commit comments

Comments
 (0)