diff --git a/backend/src/database/order_fields_test.go b/backend/src/database/order_fields_test.go new file mode 100644 index 000000000..5e24869f7 --- /dev/null +++ b/backend/src/database/order_fields_test.go @@ -0,0 +1,73 @@ +package database + +import ( + "context" + "slices" + "strings" + "testing" + + "github.com/UnlockedLabs/unlocked-core/backend/src/models" + "github.com/glebarez/sqlite" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gorm.io/gorm" +) + +// Sort keys the frontend sends to the users list and the resident filters +// (StudentManagement.tsx, AddClassEnrollments.tsx, EnrollResidentsModal.tsx). +func TestOrderByFieldsKeepClientKeys(t *testing.T) { + assert.Subset(t, validUserOrderByFields, []string{"name_last", "username", "doc_id", "last_login", "created_at"}) + assert.Subset(t, residentOrderByFields, []string{"name_last", "doc_id", "created_at"}) +} + +// captureUserQueries is a DB whose queries are built but not run, and the SQL +// each one produced. +func captureUserQueries(t *testing.T) (*DB, *[]string) { + t.Helper() + gdb, err := gorm.Open(sqlite.Open("file:"+t.Name()+"?mode=memory"), &gorm.Config{DryRun: true}) + require.NoError(t, err) + 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, gdb.Callback().Query().After("gorm:query").Register("capture", record)) + return NewDB(gdb), &sqls +} + +func TestUserListsIgnoreHostileOrder(t *testing.T) { + const payload = "(1/(select/**/0))" + args := func(orderBy, order string) *models.QueryContext { + return &models.QueryContext{Ctx: context.Background(), PerPage: 10, Page: 1, OrderBy: orderBy, Order: order} + } + cases := []struct { + name, orderBy, order, want string + run func(db *DB, q *models.QueryContext) + }{ + {"users list direction", "name_last", "desc," + payload, "ORDER BY users.name_last desc", func(db *DB, q *models.QueryContext) { _, _ = db.GetCurrentUsers(q, "student") }}, + {"users list column", payload, "", "ORDER BY name_last asc", func(db *DB, q *models.QueryContext) { _, _ = db.GetCurrentUsers(q, "student") }}, + {"users list legit", "last_login", "desc", "login_metrics.last_login desc", func(db *DB, q *models.QueryContext) { _, _ = db.GetCurrentUsers(q, "student") }}, + {"residents direction", "name_last", "asc," + payload, "ORDER BY users.created_at desc", func(db *DB, q *models.QueryContext) { + _, _ = db.GetResidentsByIDs(q, []uint{1}, "users.created_at desc") + }}, + {"residents column", payload, "asc", "ORDER BY users.created_at desc", func(db *DB, q *models.QueryContext) { + _, _ = db.GetResidentsByIDs(q, []uint{1}, "users.created_at desc") + }}, + {"residents joinless column", "last_login", "asc", "ORDER BY users.created_at desc", func(db *DB, q *models.QueryContext) { + _, _ = db.GetResidentsByIDs(q, []uint{1}, "users.created_at desc") + }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + db, sqls := captureUserQueries(t) + c.run(db, args(c.orderBy, c.order)) + require.NotEmpty(t, *sqls, "query was not built") + assert.True(t, slices.ContainsFunc(*sqls, func(q string) bool { return strings.Contains(q, c.want) }), "no query with %q in %v", c.want, *sqls) + for _, q := range *sqls { + assert.NotContains(t, q, "select/**/0") + } + }) + } +} diff --git a/backend/src/database/users.go b/backend/src/database/users.go index d316237b2..cd7ac2b04 100644 --- a/backend/src/database/users.go +++ b/backend/src/database/users.go @@ -20,6 +20,12 @@ func calcOffset(page, perPage int) int { return (page - 1) * perPage } +var ( + validUserOrderByFields = []string{"name_last", "created_at", "last_login", "doc_id", "username", "role", "facility_name"} + // residentOrderByFields has no joins, so last_login and facility_name are not sortable here. + residentOrderByFields = []string{"name_last", "created_at", "doc_id", "username"} +) + func (db *DB) GetCurrentUsers(args *models.QueryContext, role string) ([]models.User, error) { tx := db.WithContext(args.Ctx).Model(models.User{}). Preload("LoginMetrics"). @@ -59,11 +65,10 @@ func (db *DB) GetCurrentUsers(args *models.QueryContext, role string) ([]models. return nil, newGetRecordsDBError(err, "users") } users := make([]models.User, 0, args.PerPage) - validUserOrderByFields := []string{"name_last", "created_at", "last_login", "doc_id", "username", "role", "facility_name"} if !slices.Contains(validUserOrderByFields, args.OrderBy) { args.OrderBy = "name_last" } - orderClause := adjustUserOrderBy(args.OrderClause("users.name_last desc")) + orderClause := adjustUserOrderBy(args.OrderClause("users.name_last desc", validUserOrderByFields...)) if args.OrderBy == "last_login" { tx = tx.Joins("LEFT JOIN login_metrics ON login_metrics.user_id = users.id") orderClause = adjustLastLoginOrderBy(orderClause) @@ -163,7 +168,7 @@ func (db *DB) GetResidentsByIDs(args *models.QueryContext, ids []uint, defaultOr return nil, newGetRecordsDBError(err, "users") } users := make([]models.User, 0, args.PerPage) - if err := tx.Order(adjustUserOrderBy(args.OrderClause(defaultOrder))). + if err := tx.Order(adjustUserOrderBy(args.OrderClause(defaultOrder, residentOrderByFields...))). Offset(args.CalcOffset()). Limit(args.PerPage). Find(&users). diff --git a/backend/src/handlers/query_context_order_test.go b/backend/src/handlers/query_context_order_test.go new file mode 100644 index 000000000..13f2c978d --- /dev/null +++ b/backend/src/handlers/query_context_order_test.go @@ -0,0 +1,32 @@ +package handlers + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +// order_by= in one param is split into the two fields the +// database layer validates, so it keeps working after the allowlist. +func TestGetQueryContextSplitsOrderBy(t *testing.T) { + cases := []struct{ query, orderBy, order string }{ + {"order_by=name_last%20desc", "name_last", "desc"}, + {"order_by=NAME_LAST%20DESC", "name_last", "desc"}, + {"order_by=name_last&order=asc", "name_last", "asc"}, + {"order_by=ps.start_dt%20asc", "ps.start_dt", "asc"}, + } + srv := &Server{} + for _, c := range cases { + t.Run(c.query, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/api/users?"+c.query, nil) + r = r.WithContext(context.WithValue(r.Context(), ClaimsKey, &Claims{})) + qc := srv.getQueryContext(r) + assert.Equal(t, c.orderBy, qc.OrderBy) + assert.Equal(t, c.order, qc.Order) + assert.Equal(t, c.orderBy+" "+c.order, qc.OrderClause("fb", c.orderBy)) + }) + } +} diff --git a/backend/src/models/resource.go b/backend/src/models/resource.go index e15d1892e..763fc0a0e 100644 --- a/backend/src/models/resource.go +++ b/backend/src/models/resource.go @@ -149,13 +149,20 @@ func (q QueryContext) CalcOffset() int { return int(math.Abs(float64((q.Page - 1) * q.PerPage))) } -// fallback column is used if there is no order by in the query string -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 { diff --git a/backend/src/models/resource_test.go b/backend/src/models/resource_test.go new file mode 100644 index 000000000..4474fd2d3 --- /dev/null +++ b/backend/src/models/resource_test.go @@ -0,0 +1,55 @@ +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", "(select 1/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...)) + }) + } +} + +func TestOrderClauseNoAllowlistFallsBack(t *testing.T) { + assert.Equal(t, "fb", QueryContext{OrderBy: "name_last", Order: "asc"}.OrderClause("fb")) +}