Restrict API index sorting to the resource's own columns - #2228
Merged
Conversation
The sort allowlist in PreparedParams checks permitted_query, which for several resources unions in nested-resource fields so presenters can sort a parent's efforts. When one of those foreign fields reached the generic API index ORDER BY, Postgres raised UndefinedColumn and the request 500ed (e.g. /api/v1/event_groups?sort=-scheduled_start_time). Slice the sort hash to the controller class's column names in the API base controller index and the raw times index, so foreign fields are silently ignored like non-permitted fields already are. Also clear pre-existing rubocop offenses in the touched files (response.parsed_body, hash literal spacing, let! setup, and a duplicate let in the raw times spec). Resolves #2225 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the production 500 captured in Scout error group 123484:
GET /api/v1/event_groups?sort=-scheduled_start_timeraisedPG::UndefinedColumnbecausescheduled_start_timeis not anevent_groupscolumn.Why not tighten the allowlist in the parameters classes
PreparedParams#sortvalidates againstpermitted_query, and several resources (event groups, events, courses, lotteries) deliberately union nested-resource fields intopermitted_query— the HTML presenters rely on that to sort a parent's efforts (e.g. the spread'slast_name,first_namecolumn sort runs underEventParameters). Restrictingpermitted_queryper class would break those views.The fix
The bug only bites where the sort is applied to the resource's own table: the generic
Api::V1::BaseController#indexand the raw times index. Those now order bycolumn_sort— the prepared sort hash sliced tocontroller_class.column_names— so a permitted-but-foreign sort field is silently ignored, consistent with how non-permitted sort fields are already silently dropped.Rubocop cleanup
CI runs rubocop on whole changed files, so the pre-existing offenses in the two touched specs are cleared as well:
response.parsed_bodyconversions, layout fixes, a duplicatelet(:event_group), anif-worded context, andlet!setup converted tobefore(or removed where the fixture reads were dead).Resolves #2225
Testing
sort=-scheduled_start_timeand raw_times index withsort=effort_last_nameboth return 200 and ignore the sort; all four new examples (login + JWT strategies) verified to fail withPG::UndefinedColumnagainst the pre-fix codespec/controllers/api/v1suite: 391 examples, 0 failures🤖 Generated with Claude Code