diff --git a/internal/database/class_enrollments_store.go b/internal/database/class_enrollments_store.go index 4fff1ef..73d3251 100644 --- a/internal/database/class_enrollments_store.go +++ b/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") } diff --git a/internal/database/events_attendance_store.go b/internal/database/events_attendance_store.go index 3a3f967..c33e76b 100644 --- a/internal/database/events_attendance_store.go +++ b/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...)) +} diff --git a/internal/database/order_clause_test.go b/internal/database/order_clause_test.go new file mode 100644 index 0000000..66b1d1c --- /dev/null +++ b/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))) + }) + } +} diff --git a/internal/database/order_columns.go b/internal/database/order_columns.go new file mode 100644 index 0000000..d9db742 --- /dev/null +++ b/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"} +) diff --git a/internal/database/program_classes_store.go b/internal/database/program_classes_store.go index 0d4e27c..8322937 100644 --- a/internal/database/program_classes_store.go +++ b/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") diff --git a/internal/database/programs_store.go b/internal/database/programs_store.go index 8348e65..d9f7634 100644 --- a/internal/database/programs_store.go +++ b/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()) diff --git a/internal/database/users_store.go b/internal/database/users_store.go index 27876a5..0239d13 100644 --- a/internal/database/users_store.go +++ b/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 { diff --git a/internal/models/order_clause_test.go b/internal/models/order_clause_test.go new file mode 100644 index 0000000..cc52d7f --- /dev/null +++ b/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")) +} diff --git a/internal/models/query_context.go b/internal/models/query_context.go index e617186..779c246 100644 --- a/internal/models/query_context.go +++ b/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 {