fix: allowlist order_by and direction on list queries (EN-229)

internal/database/class_enrollments_store.go

@@ -498,7 +498,7 @@ func (db *DB) GetProgramClassEnrollmentsForProgram(args *models.QueryContext, cl
 		return nil, newNotFoundDBError(err, "program class enrollments")
 	}
 	if err := tx.Limit(args.PerPage).
-		Offset(args.CalcOffset()).Order(adjustUserOrderBy(args.OrderClause("pse.created_at desc"))).
+		Offset(args.CalcOffset()).Order(adjustUserOrderBy(args.OrderClause("pse.created_at desc", enrollmentOrderBy...))).
 		Find(&content).Error; err != nil {
 		return nil, newNotFoundDBError(err, "program class enrollments")
 	}

internal/database/events_attendance_store.go

@@ -31,7 +31,7 @@ func (db *DB) GetAttendees(queryParams *models.QueryContext, params url.Values,
 	if err := tx.Count(&queryParams.Total).Error; err != nil {
 		return nil, newGetRecordsDBError(err, "class_event_attendance")
 	}
-	err := tx.Order(queryParams.OrderClause("program_class_event_attendance.created_at DESC")).Limit(queryParams.PerPage).Offset(queryParams.CalcOffset()).Find(&attendance).Error
+	err := tx.Order(queryParams.OrderClause("program_class_event_attendance.created_at DESC", attendanceOrderBy...)).Limit(queryParams.PerPage).Offset(queryParams.CalcOffset()).Find(&attendance).Error
 	if err != nil {
 		return nil, newGetRecordsDBError(err, "class_event_attendance")
 	}
@@ -202,11 +202,7 @@ func (db *DB) GetEnrollmentsWithAttendanceForEvent(qryCtx *models.QueryContext,
 			a.minutes_attended AS minutes_attended,
 			a.scheduled_minutes AS scheduled_minutes`
 	finalQuery := selectClause + baseQuery
-	if slices.Contains([]string{"name_first", "name_last"}, qryCtx.OrderBy) {
-		finalQuery += " ORDER BY " + adjustUserOrderBy(qryCtx.OrderClause("e.created_at DESC"))
-	} else {
-		finalQuery += " ORDER BY e.id ASC"
-	}
+	finalQuery += " ORDER BY " + rosterOrder(qryCtx)
 
 	finalQuery += " LIMIT ? OFFSET ?"
 	args = append(args, qryCtx.PerPage, qryCtx.CalcOffset())
@@ -689,3 +685,12 @@ func (db *DB) GetCumulativeAttendanceRatesForClasses(ctx context.Context, classI
 	}
 	return out, nil
 }
+
+// rosterOrder is the roster's ORDER BY. The query is raw SQL, so this is what
+// keeps request text out of it: name sorts only, else the stable id order.
+func rosterOrder(qryCtx *models.QueryContext) string {
+	if !slices.Contains(attendanceRosterOrderBy, qryCtx.OrderBy) {
+		return "e.id ASC"
+	}
+	return adjustUserOrderBy(qryCtx.OrderClause("e.created_at DESC", attendanceRosterOrderBy...))
+}

internal/database/order_clause_test.go

@@ -0,0 +1,96 @@
+package database
+
+import (
+	"context"
+	"slices"
+	"strings"
+	"testing"
+
+	"github.com/UnlockedLabs/unlocked-programs/internal/models"
+	"github.com/UnlockedLabs/unlocked-programs/internal/testutil"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/gorm"
+)
+
+// Sort keys the frontend and core's internal caller send.
+func TestOrderByListsKeepClientKeys(t *testing.T) {
+	assert.Subset(t, programOverviewOrderBy, []string{"programs.name", "mr.total_active_enrollments", "completion_rate"})
+	assert.Subset(t, programOrderBy, []string{"name"})
+	assert.Subset(t, classDetailsOrderBy, []string{"ps.start_dt"})
+	assert.Subset(t, enrollmentOrderBy, []string{"name_last", "doc_id", "start_dt"})
+	assert.Subset(t, userProgramOrderBy, []string{"start_dt"})
+}
+
+// captureSQL is a store whose queries are built but not run, and the SQL each
+// one produced. It lets the Postgres-only queries be checked on SQLite.
+func captureSQL(t *testing.T) (*DB, *[]string) {
+	t.Helper()
+	db := testutil.NewDB(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 Find would reuse the Count's 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 NewDB(db.Session(&gorm.Session{DryRun: true})), &sqls
+}
+
+func hostile(orderBy, order string) *models.QueryContext {
+	return &models.QueryContext{Ctx: context.Background(), PerPage: 10, Page: 1, OrderBy: orderBy, Order: order}
+}
+
+func TestStoreQueriesIgnoreHostileOrder(t *testing.T) {
+	const payload = "(1/(select/**/0))"
+	cases := []struct {
+		name string
+		run  func(s *DB, q *models.QueryContext)
+		want string
+	}{
+		{"programs", func(s *DB, q *models.QueryContext) { _, _ = s.GetPrograms(q) }, "programs.created_at desc"},
+		{"programs overview", func(s *DB, q *models.QueryContext) {
+			_, _ = s.GetProgramsOverviewTable(q, 30, false, "", nil)
+		}, "programs.created_at desc"},
+		{"class details", func(s *DB, q *models.QueryContext) { _, _ = s.GetProgramClassDetailsByID(1, q) }, "ps.created_at desc"},
+		{"class outcomes", func(s *DB, q *models.QueryContext) { _, _ = s.GetProgramClassOutcomes(1, q) }, "months.month asc"},
+		{"class history", func(s *DB, q *models.QueryContext) { _, _ = s.GetProgramClassesHistory(1, "t", q) }, "created_at desc"},
+		{"user programs", func(s *DB, q *models.QueryContext) { _, _ = s.GetUserProgramInfo(q, 1) }, "pce.created_at desc"},
+		{"enrollments", func(s *DB, q *models.QueryContext) { _, _ = s.GetProgramClassEnrollmentsForProgram(q, 1, "all") }, "pse.created_at desc"},
+		{"attendees", func(s *DB, q *models.QueryContext) { _, _ = s.GetAttendees(q, nil, 1) }, "program_class_event_attendance.created_at DESC"},
+	}
+	for _, c := range cases {
+		for _, in := range [][2]string{{payload, ""}, {"name_last", "desc," + payload}} {
+			t.Run(c.name+"/"+in[0]+" "+in[1], func(t *testing.T) {
+				s, sqls := captureSQL(t)
+				c.run(s, hostile(in[0], in[1]))
+				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 "+c.want) }),
+					"no query ordered by %q", c.want)
+			})
+		}
+	}
+}
+
+// The roster query builds its ORDER BY into a raw SQL string.
+func TestRosterOrderIgnoresHostileOrder(t *testing.T) {
+	const payload = "(1/(select/**/0))"
+	cases := []struct{ orderBy, order, want string }{
+		{"name_last", "asc", "name_last asc, name_first"},
+		{"name_first", "desc", "name_first desc"},
+		{"name_last", "desc," + payload, "e.created_at DESC"},
+		{"name_last", "desc;" + payload, "e.created_at DESC"},
+		{payload, "", "e.id ASC"},
+		{"name_last, " + payload, "asc", "e.id ASC"},
+	}
+	for _, c := range cases {
+		t.Run(c.orderBy+" "+c.order, func(t *testing.T) {
+			assert.Equal(t, c.want, rosterOrder(hostile(c.orderBy, c.order)))
+		})
+	}
+}

internal/database/order_columns.go

@@ -0,0 +1,15 @@
+package database
+
+// The order_by values each list query accepts. A value outside its list falls
+// back to the query's default order, so request text never reaches ORDER BY.
+var (
+	programOrderBy          = []string{"name", "programs.created_at"}
+	programOverviewOrderBy  = []string{"programs.name", "mr.total_active_enrollments", "completion_rate", "programs.created_at"}
+	classDetailsOrderBy     = []string{"ps.start_dt", "ps.created_at"}
+	classOutcomesOrderBy    = []string{"months.month", "month", "completions", "drops"}
+	classHistoryOrderBy     = []string{"created_at"}
+	attendanceOrderBy       = []string{"program_class_event_attendance.created_at"}
+	attendanceRosterOrderBy = []string{"name_first", "name_last"}
+	userProgramOrderBy      = []string{"pce.created_at", "start_dt"}
+	enrollmentOrderBy       = []string{"name_last", "doc_id", "start_dt", "pse.created_at"}
+)

internal/database/program_classes_store.go

@@ -412,7 +412,7 @@ func (db *DB) GetProgramClassDetailsByID(id int, args *models.QueryContext) ([]m
 	if err := query.Count(&args.Total).Error; err != nil {
 		return nil, newGetRecordsDBError(err, "programs")
 	}
-	if err := query.Limit(args.PerPage).Offset(args.CalcOffset()).Order(args.OrderClause("ps.created_at desc")).Find(&classDetails).Error; err != nil {
+	if err := query.Limit(args.PerPage).Offset(args.CalcOffset()).Order(args.OrderClause("ps.created_at desc", classDetailsOrderBy...)).Find(&classDetails).Error; err != nil {
 		return nil, newGetRecordsDBError(err, "programs")
 	}
 
@@ -484,7 +484,7 @@ func (db *DB) GetProgramClassOutcomes(id int, args *models.QueryContext) ([]Prog
 			ON TO_CHAR(DATE_TRUNC('month', pce.updated_at), 'YYYY-MM') = months.month
 		`, enrollmentsSubquery), id, facilityID).
 		Group("months.month").
-		Order(args.OrderBy)
+		Order(args.OrderClause("months.month asc", classOutcomesOrderBy...))
 	if err := query.Find(&outcomes).Error; err != nil {
 		return nil, newGetRecordsDBError(err, "program_class_enrollments")
 	}
@@ -493,7 +493,7 @@ func (db *DB) GetProgramClassOutcomes(id int, args *models.QueryContext) ([]Prog
 
 func (db *DB) GetProgramClassesHistory(id int, tableName string, args *models.QueryContext) ([]models.ProgramClassesHistory, error) {
 	history := []models.ProgramClassesHistory{}
-	if err := db.WithContext(args.Ctx).Order(args.OrderClause("created_at desc")).
+	if err := db.WithContext(args.Ctx).Order(args.OrderClause("created_at desc", classHistoryOrderBy...)).
 		Find(&history, "parent_ref_id = ? and table_name = ?", id, tableName).
 		Error; err != nil {
 		return nil, newGetRecordsDBError(err, "program_classes_history")

internal/database/programs_store.go

@@ -128,7 +128,7 @@ func (db *DB) GetPrograms(args *models.QueryContext) ([]models.Program, error) {
 	if len(args.Tags) > 0 {
 		tx = tx.Joins("JOIN program_types t ON t.program_id = programs.id").Where("t.id IN (?)", args.Tags)
 	}
-	tx = tx.Order(args.OrderClause("programs.created_at desc"))
+	tx = tx.Order(args.OrderClause("programs.created_at desc", programOrderBy...))
 
 	if args.Search != "" {
 		tx = tx.Where("LOWER(name) LIKE ? OR LOWER(description) LIKE ? ", args.SearchQuery(), args.SearchQuery())
@@ -935,11 +935,7 @@ func (db *DB) GetProgramsOverviewTable(args *models.QueryContext, timeFilter int
 		return nil, newGetRecordsDBError(err, "programs table count")
 	}
 
-	if args.OrderBy != "" && args.Order != "" {
-		tx = tx.Order(args.OrderBy + " " + args.Order)
-	} else {
-		tx = tx.Order(args.OrderClause("programs.created_at desc"))
-	}
+	tx = tx.Order(args.OrderClause("programs.created_at desc", programOverviewOrderBy...))
 
 	if !args.All {
 		tx = tx.Limit(args.PerPage).Offset(args.CalcOffset())

internal/database/users_store.go

@@ -170,7 +170,7 @@ func (db *DB) GetUserProgramInfo(args *models.QueryContext, userId int) ([]model
             pce.id, pce.enrollment_status,
             p.name, p.id,
             cl.name, pc.status, pc.start_dt, pc.end_dt, pc.id, pce.updated_at,
-			pce.created_at, pce.change_reason`).Order(args.OrderClause("pce.created_at desc"))
+			pce.created_at, pce.change_reason`).Order(args.OrderClause("pce.created_at desc", userProgramOrderBy...))
 
 	if !args.All {
 		if err := base.Count(&args.Total).Error; err != nil {

internal/models/order_clause_test.go

@@ -0,0 +1,52 @@
+package models
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestOrderClauseDirection(t *testing.T) {
+	cases := []struct{ name, orderBy, order, want string }{
+		{"asc", "name_last", "asc", "name_last asc"},
+		{"desc", "name_last", "desc", "name_last desc"},
+		{"upper case direction", "name_last", "DESC", "name_last desc"},
+		{"empty direction defaults to asc", "name_last", "", "name_last asc"},
+		{"subselect in direction", "name_last", "desc,(select 1/0)", "fb"},
+		{"statement in direction", "name_last", "desc; drop table users", "fb"},
+		{"unknown direction", "name_last", "sideways", "fb"},
+		{"whitespace direction", "name_last", " ", "fb"},
+	}
+	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("fb", "name_last"))
+		})
+	}
+}
+
+func TestOrderClauseAllowlist(t *testing.T) {
+	allowed := []string{"name_last", "ps.start_dt"}
+	cases := []struct{ name, orderBy, order, want string }{
+		{"allowed column", "name_last", "asc", "name_last asc"},
+		{"qualified allowed column", "ps.start_dt", "desc", "ps.start_dt desc"},
+		{"column not listed", "password", "asc", "fb"},
+		{"empty column", "", "asc", "fb"},
+		{"empty column and direction", "", "", "fb"},
+		{"subselect as column", "(1/(select/**/0))", "", "fb"},
+		{"column with direction smuggled in", "name_last desc", "", "fb"},
+		{"comma separated columns", "name_last,password", "asc", "fb"},
+		{"comment suffix", "name_last--", "asc", "fb"},
+		{"upper case column is not a member", "NAME_LAST", "asc", "fb"},
+		{"over long column", strings.Repeat("a", 10000), "asc", "fb"},
+		{"NUL byte", "name_last\x00", "asc", "fb"},
+	}
+	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("fb", allowed...))
+		})
+	}
+	assert.Equal(t, "fb", QueryContext{OrderBy: "name_last", Order: "asc"}.OrderClause("fb"))
+}

internal/models/query_context.go

@@ -6,6 +6,7 @@ import (
 	"net/url"
 	"slices"
 	"strconv"
+	"strings"
 
 	"github.com/UnlockedLabs/unlocked-contracts/features"
 )
@@ -73,13 +74,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 {
-	val := q.OrderBy + " " + q.Order
-	if val == " " || 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
 	}
-	return val
+	switch dir := strings.ToLower(q.Order); dir {
+	case "":
+		return q.OrderBy + " asc"
+	case "asc", "desc":
+		return q.OrderBy + " " + dir
+	}
+	return fallback
 }
 
 func (q QueryContext) SearchQuery() string {