Skip to content

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

Merged
CK-7vn merged 1 commit into
mainfrom
EN-229-sql-injection-order-by
Nov 9, 2026
Merged

CK-7vn merged 1 commit into
mainfrom
EN-229-sql-injection-order-by

Conversation

@CK-7vn

@CK-7vn CK-7vn commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Pre-Submission PR Checklist

  • No debug/console/fmt.Println statements
  • Unnecessary development comments removed
  • All acceptance criteria verified
  • Functions according to ticket specifications
  • Tested manually where applicable (covered by automated tests only; no manual or Postgres run)
  • Branch rebased with latest main
  • No business logic exists within the database layer
  • DB migrations (if any) are backward-compatible and online-safe (no migrations)
  • PR title and every commit are conventional commits (feat:, fix:, docs:...); a squash merge lands under the PR title, and both become the changelog

Description of the change

OrderClause joined the request's order_by and order into ORDER BY unchecked, and three queries bypassed it entirely. This ports

  • Tested manually where applicable (covered by automated tests only; no manual or Postgres run)
  • Branch rebased with latest main
  • No business logic exists within the database layer
  • DB migrations (if any) are backward-compatible and online-safe (no migrations)
  • PR title and every commit are conventional commits (feat:, fix:, docs:...); a squash merge lands under the PR title, and both become the changelog

Description of the change

OrderClause joined the request's order_by and order into ORDER BY unchecked, and three queries bypassed it entirely. This ports the fix from unlocked-core ea045a9e, which was dropped when EN-207 moved Programs out of core.

  • OrderClause(fallback, allowed ...string) in internal/models/query_context.go returns <order_by> <dir> only when order_by is in allowed and the direction is asc or desc (case-insensitive; asc when absent). Anything else returns fallback.
  • internal/database/order_columns.go holds each list query's allowed order_by values, and every OrderClause call site passes its list (programs, overview, class details, class history, user programs, enrollments, attendees).
  • The two queries that bypassed the helper now go through it: the programs overview (raw OrderBy + " " + Order) and the class outcomes (Order(args.OrderBy), now defaulting to months.month asc).
  • The raw-SQL attendance roster in GetEnrollmentsWithAttendanceForEvent builds its ORDER BY with rosterOrder: name sorts only, else e.id ASC.
  • Tests: internal/models/order_clause_test.go (direction and allowlist cases) and internal/database/order_clause_test.go (each store query with a hostile order_by or order is built with its default order and no payload; the roster's order; the sort keys the frontend sends stay allowed).

Behavior change: an order_by outside a query's list falls back to that query's default instead of reaching the database. Class outcomes now honor order and default to months.month asc.

EN-229 also needs UnlockedLabs/unlocked-core#1294 (core users lists) and UnlockedLabs/unlocked-provider-platforms#4 (getOutcomesForUser).

Release

  • Changelog type: fix (patch bump).
  • Breaking change?: no.

OrderClause joined order_by and order into ORDER BY unchecked. It now takes
the columns a query may sort by and accepts only asc or desc, falling back to
the query's default otherwise. The programs overview and class outcomes
queries, which bypassed it, and the raw-SQL attendance roster now go through
it. Ported from unlocked-core ea045a9e, which EN-207's move dropped.

@carddev81 carddev81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to go, tested good.

@CK-7vn
CK-7vn merged commit bf809f1 into main Nov 9, 2026
11 checks passed
carddev81 added a commit that referenced this pull request Nov 11, 2026
…consistency update- #5

fix: build with Go 1.26.11 for GO-2026-6859 (EN-251) and govulncheck consistency update- #5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants