fix(providerplatforms): allowlist order_by in OrderClause (EN-229)

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 {

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

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

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