Security Fixes #2300
Security Fixes #2300
Conversation
…njmyers@buffalo.edu
KesterTan
requested review from
a team and
dwang3851
and removed request for
a team
📝 WalkthroughWalkthroughThe SubmissionsController is updated to add CSRF protection with JSON-specific exceptions, improve error handling in score_details, rework batch operation permission checks, update flash messaging for batch operations, rename submission_info authorization to tweak_total, and introduce a json_request? helper method. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Single-file scope simplifies review, but the changes span multiple distinct concerns (CSRF security, authorization logic, error handling, and action renaming) requiring careful verification of each subsystem's correctness and interaction. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/submissions_controller.rb (1)
217-221: Batch delete: handle invalid JSON, scope strictly to assessment, and return explicit responses.Risks:
- JSON.parse can raise; current code 500s.
- Early return on empty set yields no response.
- Global Submission.where allows cross‑assessment IDs; you later check, but error/response handling is inconsistent.
Refactor to validate input, scope by @Assessment, and report missing IDs.
-def destroy_batch - request_body = request.body.read - submission_ids = JSON.parse(request_body)['submission_ids'] - submission_ids = Array(submission_ids) - submissions = Submission.where(id: submission_ids) +def destroy_batch + begin + payload = JSON.parse(request.body.read.presence || "{}") + rescue JSON::ParserError + respond_to do |format| + format.json { render json: { error: "Invalid JSON" }, status: :bad_request } + format.html { redirect_to [@course, @assessment, :submissions], alert: "Invalid request." } + end + return + end + submission_ids = Array(payload["submission_ids"]).map(&:to_i).uniq + rel = @assessment.submissions.where(id: submission_ids) + submissions = rel.to_a + missing_ids = submission_ids - rel.pluck(:id) scount = 0 fcount = 0 - if submissions.empty? || submissions[0].nil? - return - end + if submissions.empty? + respond_to do |format| + format.json { render json: { error: "No valid submissions." }, status: :unprocessable_entity } + format.html { redirect_to [@course, @assessment, :submissions], alert: "No valid submissions." } + end + return + end - # Verify all submissions belong to the current assessment (and therefore course), so that - # we know the user is an instructor for the course that the submissions belong to. - allowed = true - submissions.each do |s| - next if !s.nil? && s.assessment == @assessment - - flash[:error] = "You don't have permission to delete these submissions." - allowed = false - break - end - - if allowed + if missing_ids.any? + flash[:error] = "Some submissions do not belong to this assessment." + else submissions.each do |s| if s.destroy scount += 1 else fcount += 1 end end - if fcount == 0 + if fcount == 0 flash[:success] = "#{ActionController::Base.helpers.pluralize(scount, 'submission')} destroyed. #{ActionController::Base.helpers.pluralize( fcount, 'submission' )} failed." else flash[:error] = "#{ActionController::Base.helpers.pluralize(scount, 'submission')} destroyed. #{ActionController::Base.helpers.pluralize( fcount, 'submission' )} failed." end endAlso mirror the JSON parsing/error handling in excuse_batch for consistency.
Also applies to: 228-237, 239-261
🧹 Nitpick comments (2)
app/controllers/submissions_controller.rb (2)
375-386: Redundant check; instead, detect IDs outside current assessment.submissions is already scoped via @assessment.submissions; the s.assessment == @Assessment loop is a no‑op. Replace with a missing_ids check to surface filtered IDs.
-allowed = true -# Check permissions for all submissions before processing any -submissions.each do |s| - next if !s.nil? && s.assessment == @assessment - - flash[:error] = "You don't have permission to download these submissions." - allowed = false - break -end - -return unless allowed +found_ids = @assessment.submissions.where(id: submission_ids).pluck(:id) +missing_ids = Array(submission_ids).map(&:to_i).uniq - found_ids +if missing_ids.any? + flash[:error] = "Some submissions do not belong to this assessment." + return +end
906-909: json_request? predicate may miss fetch/axios calls.request.format.json? often returns html for fetch unless Accept is set. If you keep conditional CSRF, prefer content‑type:
-def json_request? - request.format.json? -end +def json_request? + request.content_mime_type == Mime[:json] +end(If you adopt the earlier CSRF refactor, this helper becomes unnecessary.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/submissions_controller.rb(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
PR: autolab/Autolab#2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/submissions_controller.rb
🧬 Code graph analysis (1)
app/controllers/submissions_controller.rb (1)
app/controllers/application_controller.rb (1)
action_auth_level(53-71)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
app/controllers/submissions_controller.rb (2)
85-86: Good: generic error response avoids info disclosure.Hides exception details and logs the message internally. LGTM.
406-411: Auth for tweak_total looks right; confirm intended role.tweak_total is instructor‑only. Confirm that course assistants should not call it from UI. If assistants need read‑only access, consider a separate action with assistant scope.
Based on learnings.
| protect_from_forgery with: :exception, except: %i[destroy_batch excuse_batch] | ||
| before_action :verify_authenticity_token, only: %i[destroy_batch excuse_batch], if: :json_request? | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
CSRF config is fragile; prefer default CSRF for all non‑GET requests (JSON included).
You’re skipping CSRF on destroy_batch/excuse_batch and re‑adding it only when request.format.json? is true. Two issues:
- HTML/form requests to these actions are left unprotected.
- request.format.json? is often false for fetch/axios (Accept: /), so CSRF may not run.
Safer: keep default CSRF for both HTML and JSON; clients should send X-CSRF-Token. Remove the exception and the conditional before_action.
Run to locate callers and confirm they set the CSRF header:
Apply this minimal change:
-protect_from_forgery with: :exception, except: %i[destroy_batch excuse_batch]
-before_action :verify_authenticity_token, only: %i[destroy_batch excuse_batch], if: :json_request?
+protect_from_forgery with: :exception🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites of destroy_batch/excuse_batch:"
rg -nC2 -g '!**/vendor/**' '(destroy_batch|excuse_batch)'
echo -e "\nPotential AJAX calls with CSRF header nearby:"
rg -nC2 -g '!**/vendor/**' -S 'fetch\(|axios|\.ajax\(' | rg -nC2 -S 'X-CSRF-Token|csrf-token|Authenticity-Token'Length of output: 2671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== manage_submissions.js content ==="
cat app/assets/javascripts/manage_submissions.js 2>/dev/null || echo "File not found"
echo -e "\n=== Search for fetch/axios/XMLHttpRequest in app/assets/javascripts ==="
rg -nC3 'fetch\(|axios\.|XMLHttpRequest|\.ajax\(' app/assets/javascripts/ || echo "No matches found"
echo -e "\n=== Search for CSRF token meta tags in views/layouts ==="
rg -nC2 'csrf.*meta|Authenticity.*Token|X-CSRF' app/views/layouts/ 2>/dev/null || echo "No layouts found or no matches"
echo -e "\n=== Check for global CSRF token setup in application.js or initializers ==="
rg -nC3 'csrf|token' app/assets/javascripts/application.js 2>/dev/null || echo "No app/assets/javascripts/application.js"Length of output: 173188
Remove the fragile CSRF bypass; use default Rails CSRF protection.
The current config is indeed vulnerable. Although your JavaScript correctly sends X-CSRF-Token headers, the except clause bypasses CSRF checks entirely, leaving HTML form submissions unprotected. The conditional if: :json_request? is unreliable because request.format.json? depends on the Accept header, not Content-Type, and AJAX calls may not consistently trigger it.
Fix:
-protect_from_forgery with: :exception, except: %i[destroy_batch excuse_batch]
-before_action :verify_authenticity_token, only: %i[destroy_batch excuse_batch], if: :json_request?
+protect_from_forgery with: :exceptionThis works because your JavaScript clients already send the token in the X-CSRF-Token header (as confirmed in manage_submissions.js), and Rails will validate it automatically. HTML forms will also be protected by the default CSRF filter.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protect_from_forgery with: :exception, except: %i[destroy_batch excuse_batch] | |
| before_action :verify_authenticity_token, only: %i[destroy_batch excuse_batch], if: :json_request? | |
| protect_from_forgery with: :exception | |
🤖 Prompt for AI Agents
In app/controllers/submissions_controller.rb around lines 18 to 20, remove the
fragile CSRF bypass by deleting the "except: %i[destroy_batch excuse_batch]"
from the protect_from_forgery call and remove the explicit before_action
:verify_authenticity_token line (and its json_request? condition); restore the
default Rails CSRF protection (protect_from_forgery with: :exception) so Rails
will validate X-CSRF-Token headers for AJAX and protect HTML form submissions as
intended.
Description
This PR builds on the work by @NicholasMy . Original work is found here which verifies that the requested submissions belong to the current assessment/course and here which adds a auth check for tweak_total.
Thanks for the original implementation!
Additionally, this PR verifies that submissions belong to the same assessment in download batch, adds some CSRF protection for endpoints, and also enforces error handling without information disclosure.
How Has This Been Tested?
Test that everything works as expected in manage submissions.
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.