Merge pull request #7030 from donny-wong/v2.4.8

V2.4.8

Changelog.md

@@ -1,5 +1,8 @@
 # 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)

app/MARKUS_VERSION

@@ -1 +1 @@
-VERSION=v2.4.7,PATCH_LEVEL=DEV
+VERSION=v2.4.8,PATCH_LEVEL=DEV

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

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

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

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

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

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