Skip to content

Security Fixes #2300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Security Fixes #2300

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 53 additions & 35 deletions app/controllers/submissions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ class SubmissionsController < ApplicationController
tweak_total]
before_action :get_submission_file, only: %i[download view]

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?

Comment on lines +18 to +20
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 21, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 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: :exception

This 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.

Suggested 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
🤖 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.

action_auth_level :index, :instructor
def index
# cache ids instead of entire entries
Expand Down Expand Up @@ -79,7 +82,8 @@ def score_details
scores: submission_id_to_score_data,
tweaks: }, status: :ok
rescue StandardError => e
render json: { error: e.message }, status: :not_found
Rails.logger.error("Score details error: #{e.message}")
render json: { error: "Unable to retrieve submission details" }, status: :not_found
nil
end

Expand Down Expand Up @@ -221,45 +225,49 @@ def destroy_batch
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|
if s.nil?
next
end
next if !s.nil? && s.assessment == @assessment

unless @cud.instructor || @cud.course_assistant || s.course_user_datum_id == @cud.id
flash[:error] =
"You do not have permission to delete #{s.course_user_datum.user.email}'s submission."
redirect_to(course_assessment_submissions_path(submissions[0].course_user_datum.course,
submissions[0].assessment)) && return
flash[:error] = "You don't have permission to delete these submissions."
allowed = false
break
end

if allowed
submissions.each do |s|
if s.destroy
scount += 1
else
fcount += 1
end
end
if s.destroy
scount += 1
if fcount == 0
flash[:success] =
"#{ActionController::Base.helpers.pluralize(scount,
'submission')} destroyed.
#{ActionController::Base.helpers.pluralize(
fcount, 'submission'
)} failed."
else
fcount += 1
flash[:error] =
"#{ActionController::Base.helpers.pluralize(scount,
'submission')} destroyed.
#{ActionController::Base.helpers.pluralize(
fcount, 'submission'
)} failed."
end
end
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
respond_to do |format|
format.html { redirect_to [@course, @assessment, :submissions] }
format.json { render json: { redirect: url_for([@course, @assessment, :submissions]) } }
end
end

# this is good
# page to show to instructor to confirm that they would like to
# remove a given submission for a student
action_auth_level :destroyConfirm, :instructor
def destroyConfirm; end

Expand Down Expand Up @@ -364,13 +372,19 @@ def download_batch
return
end

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

filedata = submissions.collect do |s|
unless @cud.instructor || @cud.course_assistant || s.course_user_datum_id == @cud.id
flash[:error] =
"You do not have permission to download #{s.course_user_datum.user.email}'s submission."
redirect_to(course_assessment_submissions_path(submissions[0].course_user_datum.course,
submissions[0].assessment)) && return
end
p = s.handin_file_path
email = s.course_user_datum.user.email
[p, download_filename(p, email)] if !p.nil? && File.exist?(p) && File.readable?(p)
Expand All @@ -389,7 +403,7 @@ def download_batch
filename: "#{@course.name}_#{@course.semester}_#{@assessment.name}_submissions.zip")
end

action_auth_level :submission_info, :instructor
action_auth_level :tweak_total, :instructor
def tweak_total
tweak =
@submission.global_annotations.empty? ? nil : @submission.global_annotations.sum(:value)
Expand Down Expand Up @@ -889,6 +903,10 @@ def unrelease_student_grade

private

def json_request?
request.format.json?
end

def appropriate_redirect_path
if @cud.course_assistant
course_assessment_path(@course, @assessment)
Expand Down