fix(security): 2 improvements across 2 files#11712
Conversation
- Security: Potential SQL injection via unsanitized `order` parameter - Security: Missing object-level authorization check when loading repository for uploads Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
- Security: Potential SQL injection via unsanitized `order` parameter - Security: Missing object-level authorization check when loading repository for uploads Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
📝 WalkthroughWalkthroughTwo controller actions were modified: one now restricts repository lookups to editable instances using a scope chain, and the other simplifies sort parameter normalization by always sanitizing and reconstructing the order parameter regardless of input state. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/katello/api/v2/host_bootc_images_controller.rb`:
- Around line 14-15: The current call passes raw params[:sort_order] into
sanitize_sort_order which assumes a String and calls downcase, causing
exceptions for Array/Hash inputs; change the caller to ensure a safe String is
provided (e.g., coerce with to_s or guard with is_a?(String)) before calling
sanitize_sort_order, and have sanitize_sort_order validate against allowed
values ('asc'/'desc') and return 'asc' as a safe default for non-String or
invalid inputs; update references in this method to use the sanitized value for
params[:order] construction (params[:sort_order], sanitize_sort_order,
params[:order]).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88673e96-9865-434c-9661-823158021e5a
📒 Files selected for processing (2)
app/controllers/katello/api/v2/content_uploads_controller.rbapp/controllers/katello/api/v2/host_bootc_images_controller.rb
| params[:sort_order] = sanitize_sort_order(params[:sort_order]) | ||
| params[:order] = "#{params[:sort_by]} #{params[:sort_order]}" |
There was a problem hiding this comment.
Harden sort order sanitization against non-String params
On Line 14, sanitize_sort_order is now always called with raw request input. If a client sends sort_order as an Array/Hash, downcase in the helper will raise and return 500 instead of safely defaulting to 'asc'.
Suggested fix
def sanitize_sort_order(sort_order)
- if sort_order.present? && ['asc', 'desc'].include?(sort_order.downcase)
- sort_order.downcase
+ normalized = sort_order.is_a?(String) ? sort_order.downcase : nil
+ if normalized.present? && ['asc', 'desc'].include?(normalized)
+ normalized
else
'asc'
end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/katello/api/v2/host_bootc_images_controller.rb` around lines
14 - 15, The current call passes raw params[:sort_order] into
sanitize_sort_order which assumes a String and calls downcase, causing
exceptions for Array/Hash inputs; change the caller to ensure a safe String is
provided (e.g., coerce with to_s or guard with is_a?(String)) before calling
sanitize_sort_order, and have sanitize_sort_order validate against allowed
values ('asc'/'desc') and return 'asc' as a safe default for non-String or
invalid inputs; update references in this method to use the sanitized value for
params[:order] construction (params[:sort_order], sanitize_sort_order,
params[:order]).
|
Do you mind creating a redmine issue here. https://projects.theforeman.org/projects/katello, if you need any help on fields or creating the issue feel free to ask. After that please squash your commits into one and make the commit message. This will fix those two failing checks |
| params[:order] = "#{params[:sort_by]} #{sanitize_sort_order(params[:sort_order])}" | ||
| end | ||
| params[:sort_order] = sanitize_sort_order(params[:sort_order]) | ||
| params[:order] = "#{params[:sort_by]} #{params[:sort_order]}" |
There was a problem hiding this comment.
There is a change in API behavior here, the user can no longer modify the order param directly. Granted with how the endpoints are setup it may not be necessary to include order and the sort arguments, but we need to double check if the UI uses them in a special way.
|
To cover the security part of the sort order token, I haven't found a way to expose secure data. The most I've seen SQL injection get is raw SQL errors with the order being incorrect. For most injection attempts, rails flagged the query as dangerous and didn't allow it. |
|
|
||
| def find_repository | ||
| @repository = Repository.find(params[:repository_id]) | ||
| @repository = Repository.editable.find(params[:repository_id]) |
There was a problem hiding this comment.
Is there a unit test that can be added or improved here?
ianballou
left a comment
ianballou
left a comment
There was a problem hiding this comment.
Thanks for the contribution, I've added a suggestion for the bootc images controller.
| params[:order] = "#{params[:sort_by]} #{sanitize_sort_order(params[:sort_order])}" | ||
| end | ||
| params[:sort_order] = sanitize_sort_order(params[:sort_order]) | ||
| params[:order] = "#{params[:sort_by]} #{params[:sort_order]}" |
There was a problem hiding this comment.
How about the following logic:
If order, which actually changes the order, is passed in, sort_by and sort_order (which are purely informational in the API return) are set to equal order. If order is not passed in, order is set by sort_by and sort_order.
The only change here would be that sort_by and sort_order are kept up to date with what is passed in by order, which is an overall improvement.
| params[:order] = "#{params[:sort_by]} #{params[:sort_order]}" | |
| if params[:order] | |
| parts = params[:order].split(' ') | |
| params[:sort_by] = sanitize_sort_column(parts[0]) | |
| params[:sort_order] = sanitize_sort_order(parts[1]) | |
| else | |
| params[:order] = "#{params[:sort_by]} #{params[:sort_order]}" | |
| end |
|
@ianballou I believe if we want this changed we should just close this PR and have someone else make these changes. If you take a look at his github profile he created 100+ PRs to multiple repos in the span of a day. With many of the PRs being very spam like I do not mind opening a PR under my name that commit these changes. |
|
@aidenfine that sounds good to me, it's been long enough without a reply. |
|
Closing in favor of #11733 |
3 participants
Summary
fix(security): 2 improvements across 2 files
Problem
Severity:
High| File:app/controllers/katello/api/v2/host_bootc_images_controller.rb:L16The
bootc_imagesaction partially sanitizes sorting, but whenparams[:order]is provided it keeps the first token unsanitized and later passes it directly into ActiveRecord.order(params[:order]). An attacker can supply crafted SQL in the column portion oforder, which may result in SQL injection depending on adapter/rails protections and query context.Solution
Do not accept raw
orderinput. Ignoreparams[:order]entirely and build ordering only from an allowlist of known columns + allowlisted direction (asc/desc), e.g.order(Arel.sql("#{allowed_column} #{allowed_dir}"))where both parts are strictly validated against constants.Changes
app/controllers/katello/api/v2/host_bootc_images_controller.rb(modified)app/controllers/katello/api/v2/content_uploads_controller.rb(modified)Summary by CodeRabbit
Bug Fixes
Improvements