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

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")
+			}
+		})
+	}
+}

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).

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=<col> <dir> 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))
+		})
+	}
+}

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 {

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"))
+}