From 944d05bab40859628daf99bbd0b8121310862ed3 Mon Sep 17 00:00:00 2001 From: Clyde Kallahan Date: Fri, 6 Nov 2026 15:30:23 -0500 Subject: [PATCH] fix(providerplatforms): allowlist order_by in OrderClause (EN-229) OrderClause returned a bare order_by when no direction was sent and coerced a bad direction to desc. It now takes the columns a query may sort by and falls back to the query's default for anything else, matching core and programs. --- internal/providerplatforms/http.go | 18 ++-- internal/providerplatforms/http_test.go | 9 -- .../providerplatforms/order_clause_test.go | 97 +++++++++++++++++++ internal/providerplatforms/store.go | 7 +- 4 files changed, 110 insertions(+), 21 deletions(-) create mode 100644 internal/providerplatforms/order_clause_test.go diff --git a/internal/providerplatforms/http.go b/internal/providerplatforms/http.go index 52100ca..e803380 100644 --- a/internal/providerplatforms/http.go +++ b/internal/providerplatforms/http.go @@ -9,6 +9,7 @@ import ( "net/http" "net/url" "reflect" + "slices" "strconv" "strings" @@ -284,15 +285,20 @@ func (q queryContext) CalcOffset() int { return int(math.Abs(float64((q.Page - 1) * q.PerPage))) } -// OrderClause is the request's order, or fallback when it has none. -func (q queryContext) OrderClause(fallback string) string { - if q.OrderBy == "" { +// OrderClause is the request's order when order_by is one of allowed and the +// direction is asc or desc (asc when absent), otherwise fallback. allowed is +// the exact set of order_by values the query can sort by. +func (q queryContext) OrderClause(fallback string, allowed ...string) string { + if !slices.Contains(allowed, q.OrderBy) { return fallback } - if q.Order == "" { - return q.OrderBy + switch dir := strings.ToLower(q.Order); dir { + case "": + return q.OrderBy + " asc" + case "asc", "desc": + return q.OrderBy + " " + dir } - return q.OrderBy + " " + validOrder(q.Order) + return fallback } func (q queryContext) SearchQuery() string { diff --git a/internal/providerplatforms/http_test.go b/internal/providerplatforms/http_test.go index 8b19365..2a02108 100644 --- a/internal/providerplatforms/http_test.go +++ b/internal/providerplatforms/http_test.go @@ -32,12 +32,3 @@ func TestHandleErrorLogFields(t *testing.T) { assert.Equal(t, http.MethodPut, entry.Data["method"]) assert.Equal(t, "/api/things/1", entry.Data["path"]) } - -func TestOrderClauseOnlyAllowsAscOrDesc(t *testing.T) { - q := queryContext{OrderBy: "created_at", Order: "desc; drop table users"} - assert.Equal(t, "created_at desc", q.OrderClause("fallback")) - q.Order = "asc" - assert.Equal(t, "created_at asc", q.OrderClause("fallback")) - assert.Equal(t, "created_at", queryContext{OrderBy: "created_at"}.OrderClause("fallback")) - assert.Equal(t, "fallback", queryContext{}.OrderClause("fallback")) -} diff --git a/internal/providerplatforms/order_clause_test.go b/internal/providerplatforms/order_clause_test.go new file mode 100644 index 0000000..1ab9e2f --- /dev/null +++ b/internal/providerplatforms/order_clause_test.go @@ -0,0 +1,97 @@ +package providerplatforms + +import ( + "context" + "slices" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +func TestOrderClauseFallsBack(t *testing.T) { + cases := []struct{ name, orderBy, order, want string }{ + {"asc", "created_at", "asc", "created_at asc"}, + {"desc", "created_at", "desc", "created_at desc"}, + {"upper case direction", "created_at", "DESC", "created_at desc"}, + {"empty direction defaults to asc", "created_at", "", "created_at asc"}, + {"subselect in direction", "created_at", "desc,(select 1/0)", "fallback"}, + {"statement in direction", "created_at", "desc; drop table users", "fallback"}, + {"unknown direction", "created_at", "sideways", "fallback"}, + {"whitespace direction", "created_at", " ", "fallback"}, + {"hostile column without direction", "(1/(select/**/0))", "", "fallback"}, + {"hostile column with direction", "(1/(select/**/0))", "asc", "fallback"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + q := queryContext{OrderBy: c.orderBy, Order: c.order} + assert.Equal(t, c.want, q.OrderClause("fallback", "created_at")) + }) + } +} + +// capturedSQL is a store whose queries are built but not run, and the SQL each +// one produced. +func capturedSQL(t *testing.T) (*store, *[]string) { + t.Helper() + db := newTestDB(t) + var sqls []string + record := func(tx *gorm.DB) { + sqls = append(sqls, tx.Statement.SQL.String()) + // gorm skips this reset in DryRun, so a following query would reuse this SQL. + tx.Statement.SQL.Reset() + tx.Statement.Vars = nil + } + require.NoError(t, db.Callback().Query().After("gorm:query").Register("capture", record)) + require.NoError(t, db.Callback().Row().After("gorm:row").Register("capture_row", record)) + return &store{db: db.Session(&gorm.Session{DryRun: true})}, &sqls +} + +// The two queries that already validated their order keep doing so. +func TestUserCoursesAndCatalogIgnoreHostileOrder(t *testing.T) { + const payload = "(1/(select/**/0))" + t.Run("courses by order_by", func(t *testing.T) { + s, sqls := capturedSQL(t) + _, _ = s.getUserCourses(&queryContext{Ctx: context.Background(), PerPage: 10, Page: 1, UserID: 1, OrderBy: payload, Order: "asc"}) + assertOrderedBy(t, *sqls, "c.name asc") + }) + t.Run("courses by order", func(t *testing.T) { + s, sqls := capturedSQL(t) + _, _ = s.getUserCourses(&queryContext{Ctx: context.Background(), PerPage: 10, Page: 1, UserID: 1, OrderBy: "course_name", Order: "desc," + payload}) + assertOrderedBy(t, *sqls, "c.name desc") + }) + t.Run("catalog by order", func(t *testing.T) { + s, sqls := capturedSQL(t) + _, _ = s.getUserCatalog(1, nil, "", "desc,"+payload) + assertOrderedBy(t, *sqls, "c.name desc") + }) +} + +func assertOrderedBy(t *testing.T, sqls []string, want string) { + t.Helper() + require.NotEmpty(t, sqls, "query was not built") + for _, q := range sqls { + assert.NotContains(t, q, "select/**/0") + } + assert.True(t, slices.ContainsFunc(sqls, func(q string) bool { return strings.Contains(q, "ORDER BY "+want) }), "no query ordered by %q", want) +} + +func TestOutcomesIgnoreHostileOrder(t *testing.T) { + const payload = "(1/(select/**/0))" + cases := []struct{ orderBy, order, want string }{ + {"type", "desc", "type desc"}, + {"created_at", "asc", "created_at asc"}, + {payload, "", "outcomes.created_at DESC"}, + {payload, "asc", "outcomes.created_at DESC"}, + {"type", "desc," + payload, "outcomes.created_at DESC"}, + } + for _, c := range cases { + t.Run(c.orderBy+" "+c.order, func(t *testing.T) { + s, sqls := capturedSQL(t) + _, _ = s.getOutcomesForUser(&queryContext{Ctx: context.Background(), PerPage: 10, Page: 1, UserID: 1, OrderBy: c.orderBy, Order: c.order}, "") + assertOrderedBy(t, *sqls, c.want) + }) + } +} diff --git a/internal/providerplatforms/store.go b/internal/providerplatforms/store.go index 87b7843..e197926 100644 --- a/internal/providerplatforms/store.go +++ b/internal/providerplatforms/store.go @@ -299,11 +299,6 @@ func (s *store) getCourses(args *queryContext) ([]Course, error) { func (s *store) getOutcomesForUser(args *queryContext, outcomeType OutcomeType) ([]Outcome, error) { var outcomes []Outcome - fields := []string{"created_at", "type"} - if !slices.Contains(fields, args.OrderBy) { - args.OrderBy = "created_at" - } - query := s.db.WithContext(args.Ctx).Model(&Outcome{}).Where("user_id = ?", args.UserID) if outcomeType != "" { @@ -313,7 +308,7 @@ func (s *store) getOutcomesForUser(args *queryContext, outcomeType OutcomeType) if err := query.Count(&args.Total).Error; err != nil { return nil, newGetRecordsDBError(err, "outcomes") } - return outcomes, query.Order(args.OrderClause("outcomes.created_at DESC")).Offset(args.CalcOffset()).Limit(args.PerPage).Find(&outcomes).Error + return outcomes, query.Order(args.OrderClause("outcomes.created_at DESC", "created_at", "type")).Offset(args.CalcOffset()).Limit(args.PerPage).Find(&outcomes).Error } func (s *store) getDailyActivityByUserID(userID int, startDate time.Time, endDate time.Time) ([]DailyActivity, error) {