From ccd6c633f2c700c73d88dd5cacb6221f2cf2d6c8 Mon Sep 17 00:00:00 2001
From: David Liu <david@cs.toronto.edu>
Date: Fri, 5 Apr 2024 14:16:21 -0400
Subject: [PATCH 1/2] Security updates (constantize and path injection) (#7026)

* Remove use of constantize for notes controller method

* Validate user-provided paths

* Validate user-provided paths

* Additional checks

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
---
 Changelog.md                                  |  1 +
 .../api/starter_file_groups_controller.rb     | 68 +++++++++++------
 app/controllers/automated_tests_controller.rb | 48 ++++++++----
 app/controllers/exam_templates_controller.rb  | 22 ++++--
 app/controllers/notes_controller.rb           |  2 +-
 .../starter_file_groups_controller.rb         | 43 ++++++++---
 app/controllers/submissions_controller.rb     | 36 ++++++---
 app/helpers/repository_helper.rb              | 23 ++++++
 app/helpers/submissions_helper.rb             | 12 ++-
 app/lib/file_helper.rb                        |  9 +++
 app/models/note.rb                            | 13 ++++
 config/locales/common/en.yml                  |  2 +
 .../api/assignments_controller_spec.rb        | 17 ++++-
 .../starter_file_groups_controller_spec.rb    | 73 +++++++++++++++++++
 .../automated_tests_controller_spec.rb        | 69 +++++++++++++++++-
 .../exam_templates_controller_spec.rb         | 28 +++++++
 .../starter_file_groups_controller_spec.rb    | 71 ++++++++++++++++++
 .../submissions_controller_spec.rb            | 60 +++++++++++++++
 18 files changed, 525 insertions(+), 72 deletions(-)

diff --git a/Changelog.md b/Changelog.md
index 06ca86c2c0..60d43116cd 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -4,6 +4,7 @@
 - Support Jupyter notebooks for results printing (#6993)
 - Enable bulk download of print PDFs for an assignments (#6998)
 - Fixed long annotations being cut off in the annotation table (#7001)
+- Validate user-provided paths (#7025)
 
 ## [v2.4.6]
 - Disallow students from uploading .git file and .git folder in their repository (#6963)
diff --git a/app/controllers/api/starter_file_groups_controller.rb b/app/controllers/api/starter_file_groups_controller.rb
index bc7767df1f..80675f58f8 100644
--- a/app/controllers/api/starter_file_groups_controller.rb
+++ b/app/controllers/api/starter_file_groups_controller.rb
@@ -70,12 +70,17 @@ def create_file
       else
         content = params[:file_content]
       end
-      file_path = File.join(starter_file_group.path, params[:filename])
-      File.write(file_path, content, mode: 'wb')
-      update_entries_and_warn(starter_file_group)
-      render 'shared/http_status',
-             locals: { code: '201', message: HttpStatusHelper::ERROR_CODE['message']['201'] },
-             status: :created
+      file_path = FileHelper.checked_join(starter_file_group.path, params[:filename])
+      if file_path.nil?
+        render 'shared/http_status', locals: { code: '422', message:
+          HttpStatusHelper::ERROR_CODE['message']['422'] }, status: :unprocessable_entity
+      else
+        File.write(file_path, content, mode: 'wb')
+        update_entries_and_warn(starter_file_group)
+        render 'shared/http_status',
+               locals: { code: '201', message: HttpStatusHelper::ERROR_CODE['message']['201'] },
+               status: :created
+      end
     rescue StandardError => e
       message = "#{HttpStatusHelper::ERROR_CODE['message']['500']}\n\n#{e.message}"
       render 'shared/http_status', locals: { code: '500', message: message }, status: :internal_server_error
@@ -90,12 +95,17 @@ def create_folder
         return
       end
 
-      folder_path = File.join(starter_file_group.path, params[:folder_path])
-      FileUtils.mkdir_p(folder_path)
-      update_entries_and_warn(starter_file_group)
-      render 'shared/http_status',
-             locals: { code: '201', message: HttpStatusHelper::ERROR_CODE['message']['201'] },
-             status: :created
+      folder_path = FileHelper.checked_join(starter_file_group.path, params[:folder_path])
+      if folder_path.nil?
+        render 'shared/http_status', locals: { code: '422', message:
+          HttpStatusHelper::ERROR_CODE['message']['422'] }, status: :unprocessable_entity
+      else
+        FileUtils.mkdir_p(folder_path)
+        update_entries_and_warn(starter_file_group)
+        render 'shared/http_status',
+               locals: { code: '201', message: HttpStatusHelper::ERROR_CODE['message']['201'] },
+               status: :created
+      end
     rescue StandardError => e
       message = "#{HttpStatusHelper::ERROR_CODE['message']['500']}\n\n#{e.message}"
       render 'shared/http_status', locals: { code: '500', message: message }, status: :internal_server_error
@@ -109,12 +119,17 @@ def remove_file
             HttpStatusHelper::ERROR_CODE['message']['422'] }, status: :unprocessable_entity
         return
       end
-      file_path = File.join(starter_file_group.path, params[:filename])
-      File.delete(file_path)
-      update_entries_and_warn(starter_file_group)
-      render 'shared/http_status',
-             locals: { code: '200', message: HttpStatusHelper::ERROR_CODE['message']['200'] },
-             status: :ok
+      file_path = FileHelper.checked_join(starter_file_group.path, params[:filename])
+      if file_path.nil?
+        render 'shared/http_status', locals: { code: '422', message:
+          HttpStatusHelper::ERROR_CODE['message']['422'] }, status: :unprocessable_entity
+      else
+        File.delete(file_path)
+        update_entries_and_warn(starter_file_group)
+        render 'shared/http_status',
+               locals: { code: '200', message: HttpStatusHelper::ERROR_CODE['message']['200'] },
+               status: :ok
+      end
     rescue StandardError => e
       message = "#{HttpStatusHelper::ERROR_CODE['message']['500']}\n\n#{e.message}"
       render 'shared/http_status', locals: { code: '500', message: message }, status: :internal_server_error
@@ -129,12 +144,17 @@ def remove_folder
         return
       end
 
-      folder_path = File.join(starter_file_group.path, params[:folder_path])
-      FileUtils.rm_rf(folder_path)
-      update_entries_and_warn(starter_file_group)
-      render 'shared/http_status',
-             locals: { code: '200', message: HttpStatusHelper::ERROR_CODE['message']['200'] },
-             status: :ok
+      folder_path = FileHelper.checked_join(starter_file_group.path, params[:folder_path])
+      if folder_path.nil?
+        render 'shared/http_status', locals: { code: '422', message:
+          HttpStatusHelper::ERROR_CODE['message']['422'] }, status: :unprocessable_entity
+      else
+        FileUtils.rm_rf(folder_path)
+        update_entries_and_warn(starter_file_group)
+        render 'shared/http_status',
+               locals: { code: '200', message: HttpStatusHelper::ERROR_CODE['message']['200'] },
+               status: :ok
+      end
     rescue StandardError => e
       message = "#{HttpStatusHelper::ERROR_CODE['message']['500']}\n\n#{e.message}"
       render 'shared/http_status', locals: { code: '500', message: message }, status: :internal_server_error
diff --git a/app/controllers/automated_tests_controller.rb b/app/controllers/automated_tests_controller.rb
index 47a478c807..c7def48605 100644
--- a/app/controllers/automated_tests_controller.rb
+++ b/app/controllers/automated_tests_controller.rb
@@ -104,12 +104,12 @@ def populate_autotest_manager
 
   def download_file
     assignment = Assignment.find(params[:assignment_id])
-    file_path = File.join(assignment.autotest_files_dir, params[:file_name])
+    file_path = FileHelper.checked_join(assignment.autotest_files_dir, params[:file_name])
     filename = File.basename params[:file_name]
-    if File.exist?(file_path)
+    if file_path.present? && File.exist?(file_path)
       send_file_download file_path, filename: filename
     else
-      render plain: t('student.submission.missing_file', file_name: filename)
+      render plain: t('student.submission.missing_file', file_name: params[:file_name])
     end
   end
 
@@ -129,11 +129,21 @@ def upload_files
     delete_files = params[:delete_files] || []
     new_files = params[:new_files] || []
     unzip = params[:unzip] == 'true'
+    autotest_files_path = FileHelper.checked_join(assignment.autotest_files_dir, params[:path])
+    if autotest_files_path.nil?
+      flash_now(:error, I18n.t('errors.invalid_path'))
+      render partial: 'update_files'
+      return
+    end
 
     upload_files_helper(new_folders, new_files, unzip: unzip) do |f|
       if f.is_a?(String) # is a directory
-        folder_path = File.join(assignment.autotest_files_dir, params[:path], f)
-        FileUtils.mkdir_p(folder_path)
+        folder_path = FileHelper.checked_join(autotest_files_path, f)
+        if folder_path.nil?
+          flash_now(:error, I18n.t('errors.invalid_path'))
+        else
+          FileUtils.mkdir_p(folder_path)
+        end
       else
         if f.size > assignment.course.max_file_size
           flash_now(:error, t('student.submission.file_too_large',
@@ -143,19 +153,31 @@ def upload_files
         elsif f.size == 0
           flash_now(:warning, t('student.submission.empty_file_warning', file_name: f.original_filename))
         end
-        file_path = File.join(assignment.autotest_files_dir, params[:path], f.original_filename)
-        FileUtils.mkdir_p(File.dirname(file_path))
-        file_content = f.read
-        File.write(file_path, file_content, mode: 'wb')
+        file_path = FileHelper.checked_join(autotest_files_path, f.original_filename)
+        if file_path.nil?
+          flash_now(:error, I18n.t('errors.invalid_path'))
+        else
+          FileUtils.mkdir_p(File.dirname(file_path))
+          file_content = f.read
+          File.write(file_path, file_content, mode: 'wb')
+        end
       end
     end
     delete_folders.each do |f|
-      folder_path = File.join(assignment.autotest_files_dir, f)
-      FileUtils.rm_rf(folder_path)
+      folder_path = FileHelper.checked_join(assignment.autotest_files_dir, f)
+      if folder_path.nil?
+        flash_now(:error, I18n.t('errors.invalid_path'))
+      else
+        FileUtils.rm_rf(folder_path)
+      end
     end
     delete_files.each do |f|
-      file_path = File.join(assignment.autotest_files_dir, f)
-      File.delete(file_path)
+      file_path = FileHelper.checked_join(assignment.autotest_files_dir, f)
+      if file_path.nil?
+        flash_now(:error, I18n.t('errors.invalid_path'))
+      else
+        File.delete(file_path)
+      end
     end
     render partial: 'update_files'
   end
diff --git a/app/controllers/exam_templates_controller.rb b/app/controllers/exam_templates_controller.rb
index e04c031d01..8b5d00701e 100644
--- a/app/controllers/exam_templates_controller.rb
+++ b/app/controllers/exam_templates_controller.rb
@@ -104,9 +104,14 @@ def generate
 
   def download_generate
     exam_template = record
-    send_file(File.join(exam_template.tmp_path, params[:file_name]),
-              filename: params[:file_name],
-              type: 'application/pdf')
+    path = FileHelper.checked_join(exam_template.tmp_path, params[:file_name])
+    if path.nil?
+      head :unprocessable_entity
+    else
+      send_file(path,
+                filename: params[:file_name],
+                type: 'application/pdf')
+    end
   end
 
   def show_cover
@@ -265,9 +270,14 @@ def download_raw_split_file
   def download_error_file
     exam_template = record
     @assignment = record.assignment
-    send_file(File.join(exam_template.base_path, 'error', params[:file_name]),
-              filename: params[:file_name],
-              type: 'application/pdf')
+    path = FileHelper.checked_join(exam_template.base_path, 'error', params[:file_name])
+    if path.nil?
+      head :unprocessable_entity
+    else
+      send_file(path,
+                filename: params[:file_name],
+                type: 'application/pdf')
+    end
   end
 
   def fix_error
diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index 73aeb0d52a..5124ced468 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -151,7 +151,7 @@ def noteable
     return @noteable if defined?(@noteable)
     noteable_id = params[:noteable_id] || params[:note]&.[](:noteable_id)
     return @noteable = nil unless Note::NOTEABLES.include?(params[:noteable_type])
-    @noteable = params[:noteable_type]&.constantize&.find_by(id: noteable_id)
+    @noteable = Note.get_noteable(params[:noteable_type]).find_by(id: noteable_id)
   end
 
   protected
diff --git a/app/controllers/starter_file_groups_controller.rb b/app/controllers/starter_file_groups_controller.rb
index bd4eb677ba..b9bf137571 100644
--- a/app/controllers/starter_file_groups_controller.rb
+++ b/app/controllers/starter_file_groups_controller.rb
@@ -13,9 +13,9 @@ def create
 
   def download_file
     starter_file_group = record
-    file_path = File.join starter_file_group.path, params[:file_name]
+    file_path = FileHelper.checked_join starter_file_group.path, params[:file_name]
     filename = File.basename params[:file_name]
-    if File.exist?(file_path)
+    if file_path.present? && File.exist?(file_path)
       send_file_download file_path, filename: filename
     else
       render plain: t('student.submission.missing_file', file_name: filename)
@@ -43,11 +43,20 @@ def update_files
     delete_folders = params[:delete_folders] || []
     delete_files = params[:delete_files] || []
     new_files = params[:new_files] || []
+    target_path = FileHelper.checked_join(starter_file_group.path, params[:path].to_s)
+    if target_path.nil?
+      flash_now(:error, I18n.t('errors.invalid_path'))
+      return
+    end
 
     upload_files_helper(new_folders, new_files, unzip: unzip) do |f|
       if f.is_a?(String) # is a directory
-        folder_path = File.join(starter_file_group.path, params[:path].to_s, f)
-        FileUtils.mkdir_p(folder_path)
+        folder_path = FileHelper.checked_join(target_path, f)
+        if folder_path.nil?
+          flash_now(:error, I18n.t('errors.invalid_path'))
+        else
+          FileUtils.mkdir_p(folder_path)
+        end
       else
         if f.size > assignment.course.max_file_size
           flash_now(:error, t('student.submission.file_too_large',
@@ -57,19 +66,31 @@ def update_files
         elsif f.size == 0
           flash_now(:warning, t('student.submission.empty_file_warning', file_name: f.original_filename))
         end
-        file_path = File.join(starter_file_group.path, params[:path].to_s, f.original_filename)
-        file_content = f.read
-        File.write(file_path, file_content, mode: 'wb')
+        file_path = FileHelper.checked_join(target_path, f.original_filename)
+        if file_path.nil?
+          flash_now(:error, I18n.t('errors.invalid_path'))
+        else
+          file_content = f.read
+          File.write(file_path, file_content, mode: 'wb')
+        end
       end
     end
 
     delete_folders.each do |f|
-      folder_path = File.join(starter_file_group.path, f)
-      FileUtils.rm_rf(folder_path)
+      folder_path = FileHelper.checked_join(starter_file_group.path, f)
+      if folder_path.nil?
+        flash_now(:error, I18n.t('errors.invalid_path'))
+      else
+        FileUtils.rm_rf(folder_path)
+      end
     end
     delete_files.each do |f|
-      file_path = File.join(starter_file_group.path, f)
-      File.delete(file_path)
+      file_path = FileHelper.checked_join(starter_file_group.path, f)
+      if file_path.nil?
+        flash_now(:error, I18n.t('errors.invalid_path'))
+      else
+        File.delete(file_path)
+      end
     end
     if params[:path].blank?
       all_paths = [new_folders, new_files.map(&:original_filename), delete_files, delete_folders].flatten
diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb
index 4d8c79563e..ac657120a1 100644
--- a/app/controllers/submissions_controller.rb
+++ b/app/controllers/submissions_controller.rb
@@ -361,11 +361,15 @@ def update_files
       flash_message(:warning, I18n.t('student.submission.no_action_detected'))
     else
       messages = []
+      path = FileHelper.checked_join(@grouping.assignment.repository_folder, @path.gsub(%r{^/}, ''))
+      if path.nil?
+        raise I18n.t('errors.invalid_path')
+      end
+      path = Pathname.new(path)
       @grouping.access_repo do |repo|
         # Create transaction, setting the author.  Timestamp is implicit.
         txn = repo.get_transaction(current_user.user_name)
         should_commit = true
-        path = Pathname.new(@grouping.assignment.repository_folder).join(@path.gsub(%r{^/}, ''))
 
         if current_role.student? && @grouping.assignment.only_required_files
           required_files = @grouping.assignment.assignment_files.pluck(:filename)
@@ -652,23 +656,31 @@ def notebook_content
       grouping = find_appropriate_grouping(assignment.id, params)
       revision_identifier = params[:revision_identifier]
 
-      path = params[:path] || '/'
-      file_contents = grouping.access_repo do |repo|
-        if revision_identifier.nil?
-          revision = repo.get_latest_revision
-        else
-          revision = repo.get_revision(revision_identifier)
+      path = FileHelper.checked_join(assignment.repository_folder, params[:path] || '/')
+      if path.nil?
+        flash_message(:error, I18n.t('errors.invalid_path'))
+      else
+        file_contents = grouping.access_repo do |repo|
+          if revision_identifier.nil?
+            revision = repo.get_latest_revision
+          else
+            revision = repo.get_revision(revision_identifier)
+          end
+          file = revision.files_at_path(path)[params[:file_name]]
+          repo.download_as_string(file)
         end
-        file = revision.files_at_path(File.join(assignment.repository_folder, path))[params[:file_name]]
-        repo.download_as_string(file)
       end
       filename = params[:file_name]
     end
 
-    file_path = "#{assignment.repository_folder}/#{path}/#{filename}"
-    unique_path = "#{grouping.group.repo_name}/#{file_path}.#{revision_identifier}"
     @notebook_type = FileHelper.get_file_type(filename)
-    @notebook_content = notebook_to_html(file_contents, unique_path, @notebook_type)
+    if path.nil?
+      @notebook_content = ''
+    else
+      sanitized_filename = ActiveStorage::Filename.new("#{filename}.#{revision_identifier}").sanitized
+      unique_path = File.join(grouping.group.repo_name, path, sanitized_filename)
+      @notebook_content = notebook_to_html(file_contents, unique_path, @notebook_type)
+    end
     render layout: 'notebook'
   end
 
diff --git a/app/helpers/repository_helper.rb b/app/helpers/repository_helper.rb
index 13a9d60dcb..ad62cabca3 100644
--- a/app/helpers/repository_helper.rb
+++ b/app/helpers/repository_helper.rb
@@ -52,6 +52,12 @@ def add_file(f, role, repo, path: '/', txn: nil, check_size: true, required_file
     subdir_path, filename = File.split(filename)
     filename = FileHelper.sanitize_file_name(filename)
     file_path = current_path.join(subdir_path).join(filename).to_s
+
+    unless File.expand_path(file_path).start_with?(File.expand_path(path))
+      messages << [:invalid_filename, f.original_filename]
+      return false, messages
+    end
+
     new_files << file_path
     # Sometimes the file pointer of file_object is at the end of the file.
     # In order to avoid empty uploaded files, rewind it to be safe.
@@ -113,6 +119,12 @@ def remove_files(files, user, repo, path: '/', txn: nil, keep_folder: true)
       basename = FileHelper.sanitize_file_name(basename)
       file_path = current_path.join(subdir_path).join(basename)
       file_path = file_path.to_s
+
+      unless File.expand_path(file_path).start_with?(File.expand_path(path))
+        messages << [:invalid_filename, File.join(subdir_path, basename)]
+        return false, messages
+      end
+
       txn.remove(file_path, current_revision.to_s, keep_folder: keep_folder)
     end
 
@@ -139,6 +151,12 @@ def add_folder(folder_path, user, repo, path: '/', txn: nil, required_files: nil
     folder_path = current_path.join(folder_path)
     folder_path = folder_path.to_s
 
+    unless File.expand_path(folder_path).start_with?(File.expand_path(path))
+      folder_path = format_folder_path folder_path
+      messages << [:invalid_folder_name, folder_path]
+      return false, messages
+    end
+
     # check if only required files are allowed for a submission
     # allowed folders = paths in required files
     if required_files.present? && required_files.none? { |file| file.starts_with?(folder_path) }
@@ -189,6 +207,11 @@ def remove_folders(folders, user, repo, path: '/', txn: nil)
     files = []
     folders.each do |folder_path|
       folder_path = current_path.join(folder_path).to_s
+      unless File.expand_path(folder_path).start_with?(File.expand_path(path))
+        messages << [:invalid_folder_name, folder_path]
+        return false, messages
+      end
+
       next if dirs.include? folder_path
 
       repo.get_revision(current_revision.to_s).tree_at_path(folder_path, with_attrs: false).each do |_, obj|
diff --git a/app/helpers/submissions_helper.rb b/app/helpers/submissions_helper.rb
index f6b41429e1..665de26003 100644
--- a/app/helpers/submissions_helper.rb
+++ b/app/helpers/submissions_helper.rb
@@ -61,9 +61,18 @@ def upload_file(grouping, only_required_files: false)
       return
     end
 
+    path = Pathname.new(grouping.assignment.repository_folder)
+    filename = params[:filename]
+
+    if FileHelper.checked_join(path.to_s, filename).nil?
+      message = I18n.t('errors.invalid_path')
+      render 'shared/http_status', locals: { code: '422', message: message }, status: :unprocessable_entity
+      return
+    end
+
     # Only allow required files to be uploaded if +only_required_files+ is true
     required_files = grouping.assignment.assignment_files.pluck(:filename)
-    if only_required_files && required_files.exclude?(params[:filename])
+    if only_required_files && required_files.exclude?(filename)
       message = t('assignments.upload_file_requirement', file_name: params[:filename]) +
         "\n#{Assignment.human_attribute_name(:assignment_files)}: #{required_files.join(', ')}"
       render 'shared/http_status', locals: { code: '422', message: message }, status: :unprocessable_entity
@@ -84,7 +93,6 @@ def upload_file(grouping, only_required_files: false)
                                                     filename: params[:filename],
                                                     type: params[:mime_type])
       success, messages = grouping.access_repo do |repo|
-        path = Pathname.new(grouping.assignment.repository_folder)
         add_file(file, current_role, repo, path: path)
       end
     ensure
diff --git a/app/lib/file_helper.rb b/app/lib/file_helper.rb
index af49ad4c8b..d22072982f 100644
--- a/app/lib/file_helper.rb
+++ b/app/lib/file_helper.rb
@@ -82,4 +82,13 @@ def self.get_comment_syntax(filename)
       %w[## ##]
     end
   end
+
+  # Join +path+ and +paths+, where +paths+ are potentially untrusted inputs.
+  # Returns +nil+ if the resulting path is outside +path+.
+  def self.checked_join(path, *paths)
+    new_path = File.join(path, *paths)
+    if File.expand_path(new_path).start_with?(File.expand_path(path))
+      new_path
+    end
+  end
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index 8784cdb455..340f81486b 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -23,4 +23,17 @@ def self.noteables_exist?(course_id)
     end
     false
   end
+
+  def self.get_noteable(class_name)
+    case class_name
+    when 'Grouping'
+      Grouping
+    when 'Student'
+      Student
+    when 'Assignment'
+      Assignment
+    else
+      raise "Invalid noteable class #{class_name}"
+    end
+  end
 end
diff --git a/config/locales/common/en.yml b/config/locales/common/en.yml
index cdbf65bdb0..d004b4afec 100644
--- a/config/locales/common/en.yml
+++ b/config/locales/common/en.yml
@@ -12,6 +12,8 @@ en:
   delete: Delete
   edit: Edit
   encoding: Encoding
+  errors:
+    invalid_path: Invalid path provided.
   file: File
   filter_by: Filter by %{name}
   forbidden: Forbidden
diff --git a/spec/controllers/api/assignments_controller_spec.rb b/spec/controllers/api/assignments_controller_spec.rb
index 73a8992055..a3a490a663 100644
--- a/spec/controllers/api/assignments_controller_spec.rb
+++ b/spec/controllers/api/assignments_controller_spec.rb
@@ -533,12 +533,13 @@
 
     context 'POST submit_file' do
       let(:student) { create(:grouping_with_inviter, assignment: assignment).inviter }
+      let(:filename) { 'v1/x/y/test.txt' }
       before do
         assignment.update(api_submit: true)
       end
 
       subject do
-        post :submit_file, params: { id: assignment.id, filename: 'v1/x/y/test.txt', mime_type: 'text',
+        post :submit_file, params: { id: assignment.id, filename: filename, mime_type: 'text',
                                      file_content: 'This is a test file', course_id: course.id }
       end
 
@@ -668,6 +669,20 @@
             include_examples 'submits successfully'
           end
         end
+
+        context 'when the filename is invalid' do
+          let(:filename) { '../hello' }
+
+          it 'responds with 422' do
+            subject
+            expect(response).to have_http_status(422)
+          end
+
+          it 'does not create a temporary file' do
+            expect(Tempfile).not_to receive(:new)
+            subject
+          end
+        end
       end
     end
   end
diff --git a/spec/controllers/api/starter_file_groups_controller_spec.rb b/spec/controllers/api/starter_file_groups_controller_spec.rb
index de92658aa8..9e4eb32838 100644
--- a/spec/controllers/api/starter_file_groups_controller_spec.rb
+++ b/spec/controllers/api/starter_file_groups_controller_spec.rb
@@ -245,6 +245,10 @@
       context 'for this assignment' do
         let(:starter_file_group) { create :starter_file_group_with_entries, assignment: assignment }
         include_examples 'unauthenticated request'
+        it 'should respond with a success code' do
+          subject
+          expect(response).to have_http_status(:created)
+        end
         it 'should add a file to the group' do
           subject
           expect(starter_file_group.files_and_dirs).to include 'a'
@@ -271,6 +275,24 @@
         expect(response).to have_http_status(404)
       end
     end
+    context 'when the path given is invalid' do
+      let(:starter_file_group) { create :starter_file_group_with_entries, assignment: assignment }
+
+      before do
+        post :create_file, params: { filename: '../../../a',
+                                     file_content: 'a',
+                                     course_id: course.id,
+                                     id: starter_file_group.id }
+      end
+
+      it 'returns a 422 status code' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+
+      it 'does not create the file' do
+        expect(File).not_to exist(File.expand_path(File.join(starter_file_group.path, '../../../a')))
+      end
+    end
   end
   describe '#create_folder' do
     subject do
@@ -307,6 +329,23 @@
         expect(response).to have_http_status(404)
       end
     end
+    context 'when the path given is invalid' do
+      let(:starter_file_group) { create :starter_file_group_with_entries, assignment: assignment }
+
+      before do
+        post :create_folder, params: { folder_path: '../../../a',
+                                       course_id: course.id,
+                                       id: starter_file_group.id }
+      end
+
+      it 'returns a 422 status code' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+
+      it 'does not create the folder' do
+        expect(Dir).not_to exist(File.expand_path(File.join(starter_file_group.path, '../../../a')))
+      end
+    end
   end
   describe '#remove_file' do
     subject do
@@ -343,6 +382,23 @@
         expect(response).to have_http_status(404)
       end
     end
+    context 'when the path given is invalid' do
+      let(:starter_file_group) { create :starter_file_group_with_entries, assignment: assignment }
+
+      before do
+        post :remove_file, params: { filename: '../../../../../LICENSE',
+                                     course_id: course.id,
+                                     id: starter_file_group.id }
+      end
+
+      it 'returns a 422 status code' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+
+      it 'does not delete the file' do
+        expect(File).to exist(File.expand_path(File.join(starter_file_group.path, '../../../../../LICENSE')))
+      end
+    end
   end
   describe '#remove_folder' do
     subject do
@@ -382,6 +438,23 @@
         expect(response).to have_http_status(404)
       end
     end
+    context 'when the path given is invalid' do
+      let(:starter_file_group) { create :starter_file_group_with_entries, assignment: assignment }
+
+      before do
+        post :remove_folder, params: { folder_path: '../../../../../doc',
+                                       course_id: course.id,
+                                       id: starter_file_group.id }
+      end
+
+      it 'returns a 422 status code' do
+        expect(response).to have_http_status(:unprocessable_entity)
+      end
+
+      it 'does not delete the folder' do
+        expect(Dir).to exist(File.expand_path(File.join(starter_file_group.path, '../../../../../doc')))
+      end
+    end
   end
   describe '#entries' do
     subject do
diff --git a/spec/controllers/automated_tests_controller_spec.rb b/spec/controllers/automated_tests_controller_spec.rb
index 0d7c68478d..0fbf851397 100644
--- a/spec/controllers/automated_tests_controller_spec.rb
+++ b/spec/controllers/automated_tests_controller_spec.rb
@@ -122,6 +122,15 @@
     context 'GET download_file' do
       before { get_as role, :download_file, params: params }
       # TODO: write tests
+
+      context 'when given an invalid path' do
+        let(:filename) { '../../../a.txt' }
+        let(:params) { { course_id: assignment.course.id, assignment_id: assignment.id, file_name: filename } }
+
+        it 'returns an error message' do
+          expect(response.body).to eq I18n.t('student.submission.missing_file', file_name: filename)
+        end
+      end
     end
     context 'GET download_files' do
       subject { get_as role, :download_files, params: params }
@@ -166,10 +175,10 @@
     end
     context 'POST upload_files' do
       before do
-        FileUtils.rm_r assignment.autotest_files_dir
+        FileUtils.rm_rf assignment.autotest_files_dir
         post_as role, :upload_files, params: params
       end
-      after { FileUtils.rm_r assignment.autotest_files_dir }
+      after { FileUtils.rm_rf assignment.autotest_files_dir }
       context 'uploading a zip file' do
         let(:params) do
           { course_id: assignment.course.id, assignment_id: assignment.id,
@@ -219,6 +228,62 @@
           end
         end
       end
+      context 'when the provided path is invalid' do
+        let(:params) do
+          { course_id: assignment.course.id, assignment_id: assignment.id,
+            unzip: false, new_files: [fixture_file_upload('TestShapes.java')], path: '../../' }
+        end
+
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+
+        it 'does not save the file' do
+          expect(File).to_not exist(File.join(assignment.autotest_files_dir, '../..', 'TestShapes.java'))
+        end
+      end
+      context 'when the provided new folder to be created is invalid' do
+        let(:params) do
+          { course_id: assignment.course.id, assignment_id: assignment.id,
+            unzip: false, new_folders: ['../../hello'], path: '/' }
+        end
+
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+
+        it 'does not create the folder' do
+          expect(Dir).to_not exist(File.expand_path(File.join(assignment.autotest_files_dir, '../../hello')))
+        end
+      end
+      context 'when an invalid folder path to be deleted is provided' do
+        let(:params) do
+          { course_id: assignment.course.id, assignment_id: assignment.id,
+            unzip: false, delete_folders: ['../../../../../doc'], path: '/' }
+        end
+
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+
+        it 'does not delete the folder' do
+          expect(Dir).to exist(File.expand_path(File.join(assignment.autotest_files_dir, '../../../../../doc')))
+        end
+      end
+      context 'when an invalid filename to be deleted is provided' do
+        let(:params) do
+          { course_id: assignment.course.id, assignment_id: assignment.id,
+            unzip: false, delete_files: ['../../../../../LICENSE'], path: '/' }
+        end
+
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+
+        it 'does not delete the file' do
+          expect(File).to exist(File.expand_path(File.join(assignment.autotest_files_dir, '../../../../../LICENSE')))
+        end
+      end
     end
     context 'GET download_specs' do
       context 'when the assignment has test settings' do
diff --git a/spec/controllers/exam_templates_controller_spec.rb b/spec/controllers/exam_templates_controller_spec.rb
index b69ff7888a..54c336370d 100644
--- a/spec/controllers/exam_templates_controller_spec.rb
+++ b/spec/controllers/exam_templates_controller_spec.rb
@@ -264,6 +264,34 @@
       before { get_as user, :view_logs, params: { assignment_id: exam_template.assignment.id, course_id: course.id } }
       it('should respond with 200') { expect(response.status).to eq 200 }
     end
+
+    describe '#download_generate' do
+      context 'when the filename is invalid' do
+        before do
+          get_as user, :download_generate,
+                 params: { assignment_id: exam_template.assignment.id, course_id: course.id, id: exam_template.id,
+                           file_name: '../../a.pdf' }
+        end
+
+        it 'responds with an error status code' do
+          expect(response).to have_http_status(:unprocessable_entity)
+        end
+      end
+    end
+
+    describe '#download_error_file' do
+      context 'when the filename is invalid' do
+        before do
+          get_as user, :download_error_file,
+                 params: { assignment_id: exam_template.assignment.id, course_id: course.id, id: exam_template.id,
+                           file_name: '../../a.pdf' }
+        end
+
+        it 'responds with an error status code' do
+          expect(response).to have_http_status(:unprocessable_entity)
+        end
+      end
+    end
   end
 
   describe 'When the user is instructor' do
diff --git a/spec/controllers/starter_file_groups_controller_spec.rb b/spec/controllers/starter_file_groups_controller_spec.rb
index 22e2b7c272..a8a62ec95c 100644
--- a/spec/controllers/starter_file_groups_controller_spec.rb
+++ b/spec/controllers/starter_file_groups_controller_spec.rb
@@ -67,6 +67,13 @@
         expect(response.body).to eq I18n.t('student.submission.missing_file', file_name: filename)
       end
     end
+    context 'when the filename is invalid' do
+      let(:filename) { '../../q2.txt' }
+
+      it 'should download a file with a warning message' do
+        expect(response.body).to eq(I18n.t('student.submission.missing_file', file_name: 'q2.txt'))
+      end
+    end
   end
   describe '#update' do
     subject { put_as role, :update, params: { name: 'b', course_id: course.id, id: starter_file_group.id } }
@@ -158,6 +165,18 @@
         expect(starter_file_group.files_and_dirs).to include('q1/new_nested_folder')
         expect(Dir.exist?(starter_file_group.path + 'q1/new_nested_folder')).to be true
       end
+
+      context 'when the folder path is invalid' do
+        let(:new_folders) { %w[../../hello] }
+
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+
+        it 'does not create the folder' do
+          expect(Dir).to_not exist(File.expand_path(File.join(assignment.autotest_files_dir, '../../hello')))
+        end
+      end
     end
     context 'deleting a file' do
       let(:delete_files) { %w[q2.txt q1/q1.txt] }
@@ -176,6 +195,16 @@
       it 'should not delete a starter file entry for the nested file' do
         expect(starter_file_group.starter_file_entries.pluck(:path)).to include('q1')
       end
+
+      context 'when the file path is invalid' do
+        let(:delete_files) { %w[../../../../../LICENSE] }
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+        it 'does not delete the file' do
+          expect(File).to exist(File.expand_path(File.join(assignment.autotest_files_dir, '../../../../../LICENSE')))
+        end
+      end
     end
     context 'deleting a folder' do
       let(:delete_folders) { %w[q1 q2/q2] }
@@ -196,6 +225,48 @@
       it 'should not delete a starter file entry for the nested file' do
         expect(starter_file_group.starter_file_entries.pluck(:path)).to include('q2')
       end
+
+      context 'when the folder path is invalid' do
+        let(:delete_folders) { %w[../../../../../doc] }
+        it 'flashes an error message' do
+          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+        end
+        it 'does not delete the folder' do
+          expect(Dir).to exist(File.expand_path(File.join(assignment.autotest_files_dir, '../../../../../doc')))
+        end
+      end
+    end
+    context 'when the path is invalid' do
+      subject do
+        put_as role, :update_files, params: { course_id: course.id,
+                                              id: starter_file_group.id,
+                                              unzip: unzip,
+                                              new_folders: new_folders,
+                                              delete_folders: delete_folders,
+                                              delete_files: delete_files,
+                                              new_files: new_files,
+                                              path: '../../' }
+      end
+      it 'should flash an error message' do
+        subject
+        expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+      end
+    end
+    context 'when the path is invalid' do
+      subject do
+        put_as role, :update_files, params: { course_id: course.id,
+                                              id: starter_file_group.id,
+                                              unzip: unzip,
+                                              new_folders: new_folders,
+                                              delete_folders: delete_folders,
+                                              delete_files: delete_files,
+                                              new_files: new_files,
+                                              path: '../../' }
+      end
+      it 'should flash an error message' do
+        subject
+        expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+      end
     end
   end
 end
diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb
index 39f927c1c8..dd065e9f3f 100644
--- a/spec/controllers/submissions_controller_spec.rb
+++ b/spec/controllers/submissions_controller_spec.rb
@@ -116,6 +116,49 @@
       end
     end
 
+    it 'cannot add files outside the repository when an invalid path is given' do
+      file = fixture_file_upload('Shapes.java', 'text/java')
+      bad_path = '../../'
+      post_as @student, :update_files,
+              params: { course_id: course.id, assignment_id: @assignment.id, new_files: [file], path: bad_path }
+
+      expect(response).to have_http_status :bad_request
+      expect(File).to_not exist(File.join(@grouping.group.repo_path, @grouping.assignment.repository_folder,
+                                          bad_path, 'Shapes.java'))
+    end
+
+    it 'cannot add folders outside the repository assignment folder' do
+      dir = '../hello'
+      post_as @student, :update_files,
+              params: { course_id: course.id, assignment_id: @assignment.id, new_folders: [dir], path: '/' }
+
+      expect(response).to have_http_status :unprocessable_entity
+      expect(Dir).to_not exist(File.join(@grouping.group.repo_path, @grouping.assignment.repository_folder,
+                                         dir))
+    end
+
+    it 'cannot delete files outside the repository assignment folder' do
+      post_as @student, :update_files,
+              params: { course_id: course.id, assignment_id: @assignment.id,
+                        delete_files: ['../../../../../LICENSE'], path: '/' }
+
+      expect(response).to have_http_status :unprocessable_entity
+      expect(File).to exist(
+        File.expand_path(File.join(@grouping.group.repo_path, '../../../../../LICENSE'))
+      )
+    end
+
+    it 'cannot delete folders outside the repository assignment folder' do
+      post_as @student, :update_files,
+              params: { course_id: course.id, assignment_id: @assignment.id,
+                        delete_folders: ['../../../../../doc'], path: '/' }
+
+      expect(response).to have_http_status :unprocessable_entity
+      expect(Dir).to exist(
+        File.expand_path(File.join(@grouping.group.repo_path, '../../../../../doc'))
+      )
+    end
+
     context 'submitting a url' do
       describe 'should add url files' do
         before :each do
@@ -1537,6 +1580,23 @@
       end
       it_behaves_like 'notebook types'
     end
+
+    context 'called with an invalid path' do
+      let(:filename) { 'example.ipynb' }
+      subject do
+        get_as instructor, :notebook_content, params: { course_id: course.id,
+                                                        assignment_id: assignment.id,
+                                                        file_name: filename,
+                                                        grouping_id: grouping.id,
+                                                        revision_identifier: submission.revision_identifier,
+                                                        path: '../..' }
+      end
+
+      it 'flashes an error message' do
+        subject
+        expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+      end
+    end
   end
 
   describe '#get_file' do

From 43a0e96e17f8de5dade1a433f12e01e6033641f2 Mon Sep 17 00:00:00 2001
From: Donny Wong <donnywong@cs.toronto.edu>
Date: Fri, 5 Apr 2024 14:27:00 -0400
Subject: [PATCH 2/2] v2.4.8

---
 Changelog.md       | 4 +++-
 app/MARKUS_VERSION | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Changelog.md b/Changelog.md
index 60d43116cd..16b529dae5 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -1,10 +1,12 @@
 # Changelog
 
+## [v2.4.8]
+- Validate user-provided paths (#7025)
+
 ## [v2.4.7]
 - Support Jupyter notebooks for results printing (#6993)
 - Enable bulk download of print PDFs for an assignments (#6998)
 - Fixed long annotations being cut off in the annotation table (#7001)
-- Validate user-provided paths (#7025)
 
 ## [v2.4.6]
 - Disallow students from uploading .git file and .git folder in their repository (#6963)
diff --git a/app/MARKUS_VERSION b/app/MARKUS_VERSION
index 01100baa6d..8744190e25 100644
--- a/app/MARKUS_VERSION
+++ b/app/MARKUS_VERSION
@@ -1 +1 @@
-VERSION=v2.4.7,PATCH_LEVEL=DEV
+VERSION=v2.4.8,PATCH_LEVEL=DEV