fix: resolve mass-assignment and IDOR in RequestsController

- Remove :user_id from permitted params to prevent ownership hijacking
- Scope assignment lookups to the current course to prevent cross-course IDOR
- Verify student enrollment before allowing instructors to file requests
- Offset User factory sequence to prevent UID collisions in tests

Co-authored-by: Claude Code <noreply@anthropic.com>

Author: 2 people

app/controllers/requests_controller.rb

@@ -73,11 +73,18 @@ def edit
 
   def create
     Request.merge_date_and_time!(params[:request])
+    assignment_id = request_params[:assignment_id]
+
+    if assignment_id.present? && !assignment_in_course?(assignment_id)
+      redirect_to course_requests_path(@course), alert: 'Assignment not found for this course.'
+      return
+    end
+
     @request = @course.requests.new(request_params.merge(user: @user))
 
     # Check if the assignment already has a pending request, but only if assignment_id exists
-    if request_params[:assignment_id].present? &&
-       Assignment.find_by(id: request_params[:assignment_id])&.has_pending_request_for_user?(@user, @course)
+    if assignment_id.present? &&
+       Assignment.find_by(id: assignment_id)&.has_pending_request_for_user?(@user, @course)
       redirect_to course_requests_path(@course), alert: 'You already have a pending request for this assignment.'
       return
     end
@@ -95,8 +102,13 @@ def create_for_student
 
     student = User.find_by(id: params[:request][:user_id])
     return redirect_to new_course_request_path(@course), alert: 'Student not found.' unless student
+    return redirect_to new_course_request_path(@course), alert: 'Student is not enrolled in this course.' unless student_enrolled_in_course?(student)
 
     assignment_id = params[:request][:assignment_id]
+    if assignment_id.present? && !assignment_in_course?(assignment_id)
+      return redirect_to new_course_request_path(@course), alert: 'Assignment not found for this course.'
+    end
+
     reject_other_student_requests(student, assignment_id) if assignment_id.present?
 
     Request.merge_date_and_time!(params[:request])
@@ -116,6 +128,12 @@ def update
 
     Request.merge_date_and_time!(params[:request])
 
+    assignment_id = request_params[:assignment_id]
+    if assignment_id.present? && !assignment_in_course?(assignment_id)
+      flash.now[:alert] = 'There was a problem updating the request.'
+      return render :edit
+    end
+
     if @request.update(request_params)
       result = @request.process_update(@user)
       redirect_to result[:redirect_to], notice: result[:notice]
@@ -222,7 +240,21 @@ def set_course_role_from_settings
   end
 
   def request_params
-    params.require(:request).permit(:assignment_id, :reason, :documentation, :custom_q1, :custom_q2, :requested_due_date, :user_id)
+    params.require(:request).permit(:assignment_id, :reason, :documentation, :custom_q1, :custom_q2, :requested_due_date)
+  end
+
+  # Confirms the assignment belongs to one of this course's linked LMSs,
+  # preventing a request from referencing an assignment in another course.
+  def assignment_in_course?(assignment_id)
+    return false if assignment_id.blank?
+
+    Assignment.joins(:course_to_lms).exists?(id: assignment_id, course_to_lms: { course_id: @course.id })
+  end
+
+  # Confirms the target student is actually enrolled in this course before an
+  # instructor can file a request on their behalf.
+  def student_enrolled_in_course?(student)
+    UserToCourse.exists?(user_id: student.id, course_id: @course.id, role: UserToCourse::STUDENT_ROLE)
   end
 
   def authenticate_user

spec/controllers/requests_controller_spec.rb

@@ -720,4 +720,128 @@
       expect(flash[:notice]).not_to match(/has been approved/)
     end
   end
+
+  describe 'Mass-assignment and IDOR protections' do
+    let(:other_user) { User.create!(email: 'other@example.com', canvas_uid: '777', name: 'Other Student') }
+    let(:foreign_course) { create(:course, course_name: 'Foreign Course', canvas_id: '888', course_code: 'FOR101') }
+    let(:foreign_assignment) { foreign_course.assignments.first }
+
+    describe 'POST #create' do
+      it 'ignores user_id in params and assigns the request to the current user' do
+        post :create, params: {
+          course_id: course.id,
+          request: {
+            assignment_id: assignment.id,
+            reason: 'Sick',
+            requested_due_date: Date.tomorrow.to_s,
+            due_time: '10:00',
+            user_id: other_user.id
+          }
+        }
+
+        expect(Request.last.user).to eq(user)
+        expect(Request.last.user).not_to eq(other_user)
+      end
+
+      it 'rejects an assignment that belongs to another course' do
+        post :create, params: {
+          course_id: course.id,
+          request: {
+            assignment_id: foreign_assignment.id,
+            reason: 'Sick',
+            requested_due_date: Date.tomorrow.to_s,
+            due_time: '10:00'
+          }
+        }
+
+        expect(response).to redirect_to(course_requests_path(course))
+        expect(flash[:alert]).to match(/Assignment not found for this course/)
+        expect(Request.last).to be_nil
+      end
+    end
+
+    describe 'PATCH #update' do
+      it 'rejects reassigning to an assignment from another course' do
+        original_assignment_id = request.assignment_id
+
+        patch :update, params: {
+          course_id: course.id,
+          id: request.id,
+          request: {
+            assignment_id: foreign_assignment.id,
+            reason: 'Updated reason',
+            requested_due_date: Date.tomorrow.to_s,
+            due_time: '12:00'
+          }
+        }
+
+        expect(response).to render_template(:edit)
+        expect(flash[:alert]).to match(/problem updating the request/)
+        expect(request.reload.assignment_id).to eq(original_assignment_id)
+      end
+    end
+
+    describe 'POST #create_for_student' do
+      let(:enrolled_student) { User.create!(email: 'enrolled@example.com', canvas_uid: '901', name: 'Enrolled Student') }
+      let(:unenrolled_student) { User.create!(email: 'unenrolled@example.com', canvas_uid: '902', name: 'Unenrolled Student') }
+
+      before do
+        session[:user_id] = instructor.canvas_uid
+        UserToCourse.create!(user: instructor, course: course, role: 'teacher')
+      end
+
+      it 'rejects filing on behalf of a student who is not enrolled in the course' do
+        post :create_for_student, params: {
+          course_id: course.id,
+          request: {
+            user_id: unenrolled_student.id,
+            assignment_id: assignment.id,
+            reason: 'Sick',
+            requested_due_date: Date.tomorrow.to_s,
+            due_time: '10:00'
+          }
+        }
+
+        expect(response).to redirect_to(new_course_request_path(course))
+        expect(flash[:alert]).to match(/not enrolled/)
+        expect(Request.where(user: unenrolled_student)).to be_empty
+      end
+
+      it 'creates a request for an enrolled student' do
+        UserToCourse.create!(user: enrolled_student, course: course, role: 'student')
+
+        post :create_for_student, params: {
+          course_id: course.id,
+          request: {
+            user_id: enrolled_student.id,
+            assignment_id: assignment.id,
+            reason: 'Sick',
+            requested_due_date: Date.tomorrow.to_s,
+            due_time: '10:00'
+          }
+        }
+
+        expect(Request.last.user).to eq(enrolled_student)
+      end
+
+      it 'rejects an assignment that belongs to another course' do
+        UserToCourse.create!(user: enrolled_student, course: course, role: 'student')
+
+        post :create_for_student, params: {
+          course_id: course.id,
+          request: {
+            user_id: enrolled_student.id,
+            assignment_id: foreign_assignment.id,
+            reason: 'Sick',
+            requested_due_date: Date.tomorrow.to_s,
+            due_time: '10:00'
+          }
+        }
+
+        expect(response).to redirect_to(new_course_request_path(course))
+        expect(flash[:alert]).to match(/Assignment not found for this course/)
+        expect(Request.where(user: enrolled_student)).to be_empty
+      end
+    end
+  end
 end

spec/factories/users.rb

@@ -19,7 +19,9 @@
 FactoryBot.define do
   factory :user do
     sequence(:email) { |n| "user#{n}@example.com" }
-    sequence(:canvas_uid, &:to_s)
+    # Offset so generated UIDs never collide with the low, hand-picked
+    # canvas_uid literals (e.g. '123', '566') used directly in specs.
+    sequence(:canvas_uid) { |n| (n + 100_000).to_s }
     sequence(:name) { |n| "User #{n}" }
 
     factory :admin do