From 84aeb62ee730d83438a070bdbcee7656ae2fd5d4 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Sun, 27 Sep 2026 19:58:33 +1000 Subject: [PATCH 1/2] feat(submissions): submission processing, DOCX feedback and upload rules Brings the T2 2026 submissions work from ontrack-features-t2-2026 11.0.x (reviewed and merged work) onto thoth-tech 11.0.x. Co-authored-by: maplefoxgit Co-authored-by: Thirus224849242 Co-authored-by: Tan Tai Co-authored-by: jmirchh75 Co-authored-by: anaghwadhwa123 --- app/api/entities/task_definition_entity.rb | 2 + app/api/entities/task_entity.rb | 6 + app/api/submission/portfolio_api.rb | 14 +- app/api/submission/portfolio_evidence_api.rb | 140 ++- app/helpers/comment_attachment_policy.rb | 76 ++ app/helpers/file_helper.rb | 192 +++- app/helpers/spreadsheet_upload_policy.rb | 59 + app/models/project.rb | 116 +- app/models/submission_history.rb | 4 +- app/models/test_attempt.rb | 19 +- app/sidekiq/accept_submission_job.rb | 46 +- app/views/shared/_file.pdf.erb | 6 + app/views/task/task_pdf.pdf.erb | 7 + ...dd_attachment_metadata_to_task_comments.rb | 13 + ...dd_submission_processing_state_to_tasks.rb | 9 + ..._submission_processing_options_to_tasks.rb | 10 + ...1000_add_resubmission_extension_setting.rb | 8 + docs/security/FILE-S01-security-findings.md | 29 + docs/security/FILE-S01-threat-model.md | 51 + docs/submission-history-access.md | 168 +++ .../effective-resubmission-deadline.md | 229 ++++ .../student-history-and-deadline-controls.md | 93 ++ docs/uploads/safe-upload-contract.md | 104 ++ lib/tasks/generate_pdfs.rake | 2 +- .../comments/batch03_docx_attachment_test.rb | 374 ++++++ .../comments/safe_attachment_policy_test.rb | 142 +++ test/api/resubmission_setting_test.rb | 72 ++ test/api/submission/portfolio_api_test.rb | 52 + .../submission_processing_api_test.rb | 130 +++ test/api/submission_access_test.rb | 184 +++ test/api/submission_history_access_test.rb | 388 +++++++ test/api/test_attempts_test.rb | 232 ++++ test/api/upload_security_test.rb | 1013 +++++++++++++++++ test/config/sidekiq_config_test.rb | 14 + test/lib/batch03_docx_file_helper_test.rb | 180 +++ test/lib/spreadsheet_upload_policy_test.rb | 91 ++ test/models/file_helper_test.rb | 94 ++ test/models/submission_history_test.rb | 58 + test/models/submission_lifecycle_test.rb | 145 +++ .../submission_processing_state_test.rb | 525 +++++++++ test/sidekiq/accept_submission_job_test.rb | 97 ++ 41 files changed, 5126 insertions(+), 68 deletions(-) create mode 100644 app/helpers/comment_attachment_policy.rb create mode 100644 app/helpers/spreadsheet_upload_policy.rb create mode 100644 db/migrate/20260831000001_add_attachment_metadata_to_task_comments.rb create mode 100644 db/migrate/20260831000002_add_submission_processing_state_to_tasks.rb create mode 100644 db/migrate/20260831000003_add_submission_processing_options_to_tasks.rb create mode 100644 db/migrate/20260920071000_add_resubmission_extension_setting.rb create mode 100644 docs/security/FILE-S01-security-findings.md create mode 100644 docs/security/FILE-S01-threat-model.md create mode 100644 docs/submission-history-access.md create mode 100644 docs/submission-lifecycle/effective-resubmission-deadline.md create mode 100644 docs/submission-lifecycle/student-history-and-deadline-controls.md create mode 100644 docs/uploads/safe-upload-contract.md create mode 100644 test/api/comments/batch03_docx_attachment_test.rb create mode 100644 test/api/comments/safe_attachment_policy_test.rb create mode 100644 test/api/resubmission_setting_test.rb create mode 100644 test/api/submission/portfolio_api_test.rb create mode 100644 test/api/submission/submission_processing_api_test.rb create mode 100644 test/api/submission_access_test.rb create mode 100644 test/api/submission_history_access_test.rb create mode 100644 test/api/upload_security_test.rb create mode 100644 test/config/sidekiq_config_test.rb create mode 100644 test/lib/batch03_docx_file_helper_test.rb create mode 100644 test/lib/spreadsheet_upload_policy_test.rb create mode 100644 test/models/submission_lifecycle_test.rb create mode 100644 test/models/submission_processing_state_test.rb create mode 100644 test/sidekiq/accept_submission_job_test.rb diff --git a/app/api/entities/task_definition_entity.rb b/app/api/entities/task_definition_entity.rb index 6027226c3d..3a8b6c6aa9 100644 --- a/app/api/entities/task_definition_entity.rb +++ b/app/api/entities/task_definition_entity.rb @@ -12,6 +12,7 @@ def staff?(my_role) expose :abbreviation expose :name expose :description + expose :resubmission_extensions_enabled expose :weighting expose :target_grade @@ -39,6 +40,7 @@ def staff?(my_role) expose :restrict_status_updates, if: ->(unit, options) { staff?(options[:my_role]) } expose :group_set_id, expose_nil: false expose :has_task_sheet?, as: :has_task_sheet + expose :task_sheet_filename expose :has_task_resources?, as: :has_task_resources expose :has_task_assessment_resources?, as: :has_task_assessment_resources, if: ->(unit, options) { staff?(options[:my_role]) } expose :has_task_assessment_script?, as: :has_task_assessment_script, if: ->(unit, options) { staff?(options[:my_role]) } diff --git a/app/api/entities/task_entity.rb b/app/api/entities/task_entity.rb index 1de8fb6b39..1748be3dda 100644 --- a/app/api/entities/task_entity.rb +++ b/app/api/entities/task_entity.rb @@ -18,6 +18,11 @@ class TaskEntity < Grape::Entity expose :target_start_date, expose_nil: false end + expose :effective_deadline + expose :effective_deadline_reason + expose :effective_deadline_source_id + expose :effective_deadline_date, format_with: :date_only, expose_nil: false + expose :extensions expose :scorm_extensions @@ -32,6 +37,7 @@ class TaskEntity < Grape::Entity expose :similarity_flag, unless: :update_only expose :num_new_comments, unless: :update_only + expose :has_feedback, unless: :update_only # Attributes only included in "update only" diff --git a/app/api/submission/portfolio_api.rb b/app/api/submission/portfolio_api.rb index aec64c65e1..0329bdc734 100644 --- a/app/api/submission/portfolio_api.rb +++ b/app/api/submission/portfolio_api.rb @@ -13,8 +13,8 @@ class PortfolioApi < Grape::API desc "Upload documents for inclusion in a project's portfolio" params do - requires :name, type: String, desc: 'Name of the part being uploaded' - requires :kind, type: String, desc: 'The kind of file being uploaded: document, code, or image' + requires :name, type: String, desc: 'Name of the part being uploaded' + requires :kind, type: String, values: %w[document code image], desc: 'The kind of file being uploaded: document, code, or image' requires :file0, type: File, desc: 'file 0.' end post '/submission/project/:id/portfolio' do @@ -34,6 +34,14 @@ class PortfolioApi < Grape::API error!({ error: "'#{file[:filename]}': #{file_result[:msg]}" }, 403) end + max_file_size = Doubtfire::Application.config.max_file_size.to_i + max_file_size = 10_000_000 if max_file_size <= 0 + size_in_mb = max_file_size / 1_000_000 + + if File.size(file[:tempfile].path) > max_file_size + error!({ error: "'#{file[:filename]}' exceeds the #{size_in_mb}MB file limit." }, 413) + end + # Move file into place result = project.move_to_portfolio(file, name, kind) # returns details of file @@ -43,7 +51,7 @@ class PortfolioApi < Grape::API desc 'Remove a file from the portfolio files for a unit' params do optional :idx, type: Integer, desc: 'The index of the file' - optional :kind, type: String, desc: 'The kind of file being removed: document, code, or image' + optional :kind, type: String, values: %w[document code image], desc: 'The kind of file being removed: document, code, or image' optional :name, type: String, desc: 'Name of file to remove' end delete '/submission/project/:id/portfolio' do diff --git a/app/api/submission/portfolio_evidence_api.rb b/app/api/submission/portfolio_evidence_api.rb index 8a6d36fe84..10272541cb 100644 --- a/app/api/submission/portfolio_evidence_api.rb +++ b/app/api/submission/portfolio_evidence_api.rb @@ -53,6 +53,16 @@ def self.logger error!({ error: "This task requires a group submission. Ensure you are in a group for the unit's #{task_definition.group_set.name}" }, 403) end + # A finished task stops accepting new student uploads. Without this the + # upload lands, submission_date and file_uploaded_at are rewritten and the + # assessed pdf is deleted and regenerated, while only the status transition + # is skipped. Staff are still allowed through on purpose, because a tutor + # sometimes has to upload on a student's behalf when a file is corrupt or + # went to the wrong task. + if task.task_submission_closed? && !authorise?(current_user, project, :assess) + error!({ error: 'This task is closed for new submissions.' }, 403) + end + # Check that prerequisite tasks are in the required minimum submitted state prerequisites = task_definition.task_prerequisites prerequisites.each do |prerequisite| @@ -156,16 +166,50 @@ def self.logger end task = project.task_for_task_definition(task_definition) + error!({ error: 'A submission for this task was not found.' }, 404) unless task - if task && PortfolioEvidence.recreate_task_pdf(task) + begin + task.regenerate_submission!(current_user) result = 'done' - else - result = 'false' + rescue ArgumentError => e + error!({ error: e.message }, 409) + rescue StandardError + error!({ error: 'The submitted files are not available to regenerate.' }, 422) end present :result, result, with: Grape::Presenters::Presenter end # put + desc 'Retry a failed or timed-out submission conversion' + post '/projects/:id/task_def_id/:task_definition_id/submission/retry' do + project = Project.find(params[:id]) + task_definition = project.unit.task_definitions.find(params[:task_definition_id]) + + unless authorise? current_user, project, :reprocess_submission + error!({ error: "Not authorised to retry task '#{task_definition.name}'" }, 401) + end + + task = project.task_for_task_definition(task_definition) + error!({ error: 'A submission for this task was not found.' }, 404) unless task + + begin + task.retry_submission_processing!(current_user) + rescue ArgumentError => e + error!({ error: e.message }, 409) + rescue StandardError + error!({ error: 'The submitted files are not available to retry.' }, 422) + end + + # For a group task the new state was written through other instances. + task.reload + present task.submission_processing_snapshot.merge( + submission_date: task.submission_date, + processing_error_code: task.submission_processing_error_code, + processing_attempts: task.submission_processing_attempts, + task_status: task.task_status.status_key + ), with: Grape::Presenters::Presenter + end + desc 'Get the timestamps of the last 10 submissions of a task' get '/projects/:id/task_def_id/:task_definition_id/submissions/timestamps' do project = Project.find(params[:id]) @@ -187,41 +231,89 @@ def self.logger desc 'Get all retained submission histories for a task' get '/projects/:id/task_def_id/:task_definition_id/submission_histories' do - project = Project.find(params[:id]) - task_definition = project.unit.task_definitions.find(params[:task_definition_id]) + project = Project.find_by(id: params[:id]) + error!({ error: 'Submission history is not available' }, 404) unless project - unless authorise? current_user, project, :get_submission - error!({ error: "Not authorised to get submission history for task '#{task_definition.name}'" }, 401) + unless authorise?(current_user, project, :get_submission) + error!({ error: 'Submission history is not available' }, 404) end - task = project.task_for_task_definition(task_definition) - unless task - error!({ error: 'A submission for this task definition has never been created' }, 404) + task_definition = project.unit.task_definitions.find_by(id: params[:task_definition_id]) + error!({ error: 'Submission history is not available' }, 404) unless task_definition + + task = project.tasks.find_by(task_definition: task_definition) + error!({ error: 'Submission history is not available' }, 404) unless task + + student_request = project.student == current_user + + if student_request && !authorise?(current_user, task, :get_submission) + error!({ error: 'Submission history is not available' }, 404) end - present task.submission_histories.order(submission_timestamp: :desc), - with: Entities::SubmissionHistoryEntity + histories = task.submission_histories.sort_by { |history| [-history.submission_timestamp.to_i, -history.id] } + + if student_request + archive_pending = SubmissionHistory.pending?(task) + latest_submission_at = task.submission_processing_started_at || task.submission_date + student_histories = histories.each_with_index.map do |history, index| + { + id: history.id, + version_order: index + 1, + current: index.zero? && !archive_pending && latest_submission_at.present? && + history.submission_timestamp.to_i >= latest_submission_at.to_i, + submission_timestamp: history.submission_timestamp, + status: history.has_submission_files? ? 'available' : 'unavailable' + } + end + + status 202 if archive_pending + present student_histories + else + present histories, with: Entities::SubmissionHistoryEntity + end end desc 'Download a retained submission history archive' get '/projects/:id/task_def_id/:task_definition_id/submission_histories/:history_id/files' do - project = Project.find(params[:id]) - task_definition = project.unit.task_definitions.find(params[:task_definition_id]) + project = Project.find_by(id: params[:id]) + error!({ error: 'Submission history is not available' }, 404) unless project - unless authorise? current_user, project.unit, :provide_feedback - error!({ error: "Not authorised to get submission history for task '#{task_definition.name}'" }, 401) + staff_access = authorise?(current_user, project.unit, :provide_feedback) + student_access = + project.student == current_user && authorise?(current_user, project, :get_submission) + + unless staff_access || student_access + error!({ error: 'Submission history is not available' }, 404) end - task = project.task_for_task_definition(task_definition) - history = task&.submission_histories&.find_by(id: params[:history_id]) - error!({ error: 'Submission history was not found' }, 404) unless history - error!({ error: 'Submission history files are not available' }, 404) unless history.has_submission_files? + task_definition = project.unit.task_definitions.find_by(id: params[:task_definition_id]) + error!({ error: 'Submission history is not available' }, 404) unless task_definition + + task = project.tasks.find_by(task_definition: task_definition) + error!({ error: 'Submission history is not available' }, 404) unless task + + student_access &&= authorise?(current_user, task, :get_submission) + + unless staff_access || student_access + error!({ error: 'Submission history is not available' }, 404) + end + + history = task.submission_histories.find_by(id: params[:history_id]) + error!({ error: 'Submission history is not available' }, 404) unless history + + unless history.has_submission_files? + error!({ error: 'Submission history files are not available' }, 404) + end filename = "#{project.student.username}-#{task_definition.abbreviation}-#{history.submission_timestamp}.zip" content_type 'application/octet-stream' header['Content-Disposition'] = "attachment; filename=#{filename}" - submission_zip_data = history.submission_zip_data + begin + submission_zip_data = history.submission_zip_data + rescue Zip::Error, Errno::ENOENT, Errno::EACCES + error!({ error: 'Submission history files are not available' }, 404) + end header['Content-Length'] = submission_zip_data.bytesize.to_s env['api.format'] = :binary body submission_zip_data @@ -339,7 +431,11 @@ def self.logger content_type 'application/octet-stream' header['Content-Disposition'] = "attachment; filename=#{filename}" - submission_zip_data = history.submission_zip_data + begin + submission_zip_data = history.submission_zip_data + rescue Zip::Error, Errno::ENOENT, Errno::EACCES + error!({ error: 'Submission history files are not available' }, 404) + end header['Content-Length'] = submission_zip_data.bytesize.to_s env['api.format'] = :binary body submission_zip_data diff --git a/app/helpers/comment_attachment_policy.rb b/app/helpers/comment_attachment_policy.rb new file mode 100644 index 0000000000..692d74f3f0 --- /dev/null +++ b/app/helpers/comment_attachment_policy.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +# Shared by the authenticated policy response, validation and storage dispatch. +# A category never authorises an arbitrary MIME type or filename extension. +module CommentAttachmentPolicy + MAX_BYTES = 30_000_000 # Exclusive, matching the existing comment API. + MAX_SELECTION_COUNT = 5 + CATEGORIES = [ + { id: 'pdf', name: 'PDF', extensions: %w[pdf], mime_types: %w[application/pdf], preview: 'pdf' }, + { id: 'document', name: 'Document', extensions: %w[docx], mime_types: [FileHelper::DOCX_MIME_TYPE], preview: 'download' }, + { id: 'spreadsheet', name: 'Spreadsheet', extensions: %w[csv xlsx], mime_types: %w[text/csv application/csv text/plain application/vnd.openxmlformats-officedocument.spreadsheetml.sheet], preview: 'download' }, + { id: 'image', name: 'Image', extensions: %w[png bmp tiff tif jpeg jpg gif], mime_types: %w[image/png image/bmp image/x-ms-bmp image/tiff image/jpeg image/gif], preview: 'image' }, + { id: 'audio', name: 'Audio', extensions: %w[wav ogg mp3 mp4 webm aac pcm aiff flac wma alac], mime_types: %w[audio/ video/webm application/ogg], preview: 'audio' } + ].freeze + + def self.category(filename) + extension = File.extname(filename.to_s).downcase.delete_prefix('.') + CATEGORIES.find { |item| item[:extensions].include?(extension) } + end + + def self.public_policy + { + version: 1, + max_bytes_exclusive: MAX_BYTES, + max_selection_count: MAX_SELECTION_COUNT, + categories: CATEGORIES.map { |item| item.except(:mime_types) } + } + end + + def self.validate(file) + path = file['tempfile'].path + filename = file['filename'] || file[:filename] + category = category(filename) + # MediaRecorder sends a Blob with the browser's default extensionless name. + category ||= CATEGORIES.find { |item| item[:id] == 'audio' } if filename == 'blob' + return rejected('UPLOAD_EMPTY', 'Attachment is empty.', file) unless File.size?(path) + return rejected('UPLOAD_TOO_LARGE', 'Attachment must be smaller than 30 MB.', file) if File.size(path) >= MAX_BYTES + return rejected('UPLOAD_EXTENSION_NOT_ALLOWED', 'Unsupported attachment format. Choose a format listed beside Attach a file.', file) unless category + + extension = File.extname(filename).downcase.delete_prefix('.') + detected = MimeCheckHelpers.mime_type(path).split(';').first + permitted_mimes = case extension + when 'pcm' then %w[audio/L16 audio/x-pcm application/octet-stream] + when 'csv' then %w[text/csv application/csv text/plain] + when 'xlsx' then %w[application/vnd.openxmlformats-officedocument.spreadsheetml.sheet application/zip] + else category[:mime_types] + end + return rejected('UPLOAD_MIME_INVALID', 'File contents do not match the selected format.', file) unless permitted_mimes.any? { |mime| detected == mime || (mime.end_with?('/') && detected.start_with?(mime)) } + + result = case extension + when 'docx' then FileHelper.validate_docx(path, max_file_size: MAX_BYTES - 1) + when 'xlsx' then FileHelper.validate_docx(path, format: 'xlsx', max_file_size: MAX_BYTES - 1) + when 'pdf' then FileHelper.validate_pdf(path) + when 'csv' then validate_csv(path) + else { valid: true } + end + return rejected(result[:encrypted] ? 'UPLOAD_ENCRYPTED' : 'UPLOAD_CORRUPT', 'The attachment is malformed, encrypted or contains unsupported active content.', file) if !result[:valid] || result[:encrypted] + + Rails.logger.debug('Uploaded file is accepted') + { accepted: true, msg: 'success', category: category[:id] } + end + + def self.validate_csv(path) + # Stream records so a near-limit CSV does not build a second in-memory table. + CSV.foreach(path, encoding: 'bom|utf-8') { |row| return { valid: false } if row.any? { |cell| cell&.include?("\0") } } + { valid: true } + rescue CSV::MalformedCSVError, EncodingError, ArgumentError + { valid: false } + end + + def self.rejected(code, message, file) + reason = { 'UPLOAD_EXTENSION_NOT_ALLOWED' => 'File extension check failed', 'UPLOAD_MIME_INVALID' => 'File MIME check failed' }.fetch(code, 'Upload rejected') + FileHelper.log_file_rejection(reason, 'comment_attachment', file, code: code) + { accepted: false, code: code, msg: message } + end +end diff --git a/app/helpers/file_helper.rb b/app/helpers/file_helper.rb index 8526c4b333..c840f6c7a5 100644 --- a/app/helpers/file_helper.rb +++ b/app/helpers/file_helper.rb @@ -14,6 +14,13 @@ module FileHelper extend TimeoutHelper extend MimeCheckHelpers + DOCX_MIME_TYPE = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' + DOCX_MAIN_DOCUMENT_CONTENT_TYPE = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document.main+xml' + OOXML_CONTENT_TYPES_NAMESPACE = 'http://schemas.openxmlformats.org/package/2006/content-types' + ACCEPTED_FILE_KINDS = %w[image code document word_document zip archive audio comment_attachment video csv].freeze + + CODE_UPLOAD_EXTENSIONS = %w[pas cpp c cs csv h hpp java py js html coffee rb css scss yaml yml xml json ts r rmd rnw rhtml rpres tex vb sql txt md jack hack asm hdl tst out cmp vm sh bat dat ipynb pml vue].freeze + ZIP_NESTED_ARCHIVE_EXTENSIONS = %w[ .7z .bz2 .ear .gz .jar .rar .tar .tar.bz2 .tar.gz .tar.xz .tbz .tbz2 .tgz .txz .war .xz .zip ].freeze @@ -29,7 +36,10 @@ def known_extension?(extn) # Test if a file should be accepted based on an expected kind # - file is passed the file uploaded to Doubtfire (a hash with all relevant data about the file) # - def accept_file(file, name, kind) + def accept_file(file, _name, kind) + return CommentAttachmentPolicy.validate(file) if kind == 'comment_attachment' + return SpreadsheetUploadPolicy.validate(file) if kind == 'csv' + case kind when 'image' mime_allow_list = ['image/png', 'image/gif', 'image/bmp', 'image/tiff', 'image/jpeg', 'image/x-ms-bmp'] @@ -40,6 +50,8 @@ def accept_file(file, name, kind) 'application/tst', 'text/x-cmp', 'text/x-vm', 'application/x-sh', 'application/x-bat', 'application/dat', 'application/x-wine-extension-ini'] when 'document' mime_allow_list = [ 'application/pdf' ] + when 'word_document' + mime_allow_list = [DOCX_MIME_TYPE] when 'zip', 'archive' mime_allow_list = [ 'application/zip', @@ -53,17 +65,34 @@ def accept_file(file, name, kind) when 'audio' mime_allow_list = ['audio/', 'video/webm', 'application/ogg', 'application/octet-stream'] when 'comment_attachment' - mime_allow_list = ['audio/', 'video/webm', 'application/ogg', 'image/', 'application/pdf', 'application/octet-stream'] + mime_allow_list = [ + 'audio/', + 'video/webm', + 'application/ogg', + 'image/', + 'application/pdf', + DOCX_MIME_TYPE, + 'application/octet-stream' + ] when 'video' mime_allow_list = ['video/mp4'] else - logger.error "Unknown type '#{kind}' provided for '#{name}'" + log_file_rejection('Unknown file type', 'unknown', file) + return { + accepted: false, + msg: 'unsupported file type.' + } end - extension_check = FileHelper.known_extension?(File.extname(file['tempfile']).downcase[1..]) + uploaded_filename = file['filename'] || file[:filename] || file['tempfile'].path + uploaded_extension = File.extname(uploaded_filename.to_s).downcase.delete_prefix('.') + extension_check = FileHelper.known_extension?(uploaded_extension) + extension_check ||= uploaded_extension == 'docx' && %w[comment_attachment word_document].include?(kind) + extension_check &&= uploaded_extension == 'docx' if kind == 'word_document' + extension_check &&= CODE_UPLOAD_EXTENSIONS.include?(uploaded_extension) if kind == 'code' unless extension_check msg = 'invalid file extension.' - logger.debug 'File extension check failed' + log_file_rejection('File extension check failed', kind, file) return { accepted: false, msg: msg @@ -73,20 +102,33 @@ def accept_file(file, name, kind) mime_check = mime_in_list?(file['tempfile'].path, mime_allow_list) unless mime_check msg = 'invalid file MIME type, file is likely corrupted.' - logger.debug 'File MIME check failed' + log_file_rejection('File MIME check failed', kind, file, + detected_mime: mime_type(file['tempfile'].path), allowed_mime: mime_allow_list) return { accepted: false, msg: msg } end + if uploaded_extension == 'docx' + docx_validation_result = validate_docx(file['tempfile'].path) + + unless docx_validation_result[:valid] + log_file_rejection('Word document is invalid', kind, file) + return { + accepted: false, + msg: docx_validation_result[:msg] + } + end + end + # Extra checks for PDF documents if kind == 'document' pdf_validation_result = validate_pdf(file['tempfile'].path) if pdf_validation_result[:encrypted] msg = 'PDF file is encrypted, encrypted files are not supported.' - logger.debug 'PDF file is encrypted' + log_file_rejection('PDF file is encrypted', kind, file) return { accepted: false, msg: msg @@ -95,7 +137,7 @@ def accept_file(file, name, kind) unless pdf_validation_result[:valid] msg = 'PDF file is corrupted.' - logger.debug 'PDF file is corrupted' + log_file_rejection('PDF file is corrupted', kind, file) return { accepted: false, msg: msg @@ -107,7 +149,7 @@ def accept_file(file, name, kind) zip_validation_result = validate_zip_upload(file['tempfile'].path, File.basename(file[:filename].to_s)) unless zip_validation_result[:valid] - logger.debug "Zip file is invalid: #{zip_validation_result[:msg]}" + log_file_rejection('Zip file is invalid', kind, file) return { accepted: false, msg: zip_validation_result[:msg] @@ -124,6 +166,21 @@ def accept_file(file, name, kind) } end + # Upload names and requirement labels can contain student information. Log + # only bounded extensions and validation metadata, never content or paths. + # JSON escaping also prevents client-controlled extensions forging log lines. + def log_file_rejection(reason, kind, file, **details) + extension = lambda do |value| + suffix = File.extname(value.to_s).downcase + suffix.match?(/\A\.[a-z0-9]{1,12}\z/) ? suffix : '[none or invalid]' + end + safe_kind = ACCEPTED_FILE_KINDS.include?(kind) ? kind : 'unknown' + details = details.merge(kind: safe_kind, + uploaded_extension: extension.call(file['filename'] || file[:filename]), + temporary_extension: extension.call(file['tempfile'].path)) + logger.info("#{reason} #{details.to_json}") + end + # # Sanitize the passed in paths, and ensure each part is valid # Will kill things like ../ etc or spaces in paths @@ -154,6 +211,23 @@ def sanitized_filename(filename) end end + # Preserve a human-readable upload name for display and Content-Disposition, + # while removing path and header-injection material. Storage never uses this + # value: comment attachments remain in an id-based internal path. + def safe_upload_filename(file_upload, fallback: 'attachment') + raw_name = file_upload['filename'] || file_upload[:filename] || fallback + utf8_name = raw_name.to_s.encode('UTF-8', invalid: :replace, undef: :replace, replace: '_') + basename = utf8_name.tr('\\', '/').split('/').last.to_s + basename = basename.gsub(/[[:cntrl:]]/, '').strip + basename = fallback if basename.blank? || %w[. ..].include?(basename) + + return basename if basename.length <= 255 + + extension = File.extname(basename) + stem_length = [255 - extension.length, 1].max + "#{File.basename(basename, extension)[0, stem_length]}#{extension}" + end + def task_file_dir_for_unit(unit, create = true) dst = unit_work_root(unit) << 'TaskFiles/' @@ -564,6 +638,102 @@ def validate_zip_upload(path, filename) end end + # A DOCX file is an OOXML zip package. libmagic can identify an arbitrary zip + # as a Word document from its filename or a small subset of entries, so check + # the package itself before accepting it as a comment attachment. + def validate_docx(path, format: 'docx', max_file_size: nil) + main_part = format == 'xlsx' ? 'xl/workbook.xml' : 'word/document.xml' + required_entries = [ + '[Content_Types].xml', + '_rels/.rels', + main_part + ].freeze + max_file_size ||= Doubtfire::Application.config.max_file_size.to_i + max_file_size = 10_000_000 if max_file_size <= 0 + max_uncompressed_size = max_file_size * zip_uncompressed_size_multiplier + + return { valid: false, msg: "Word document exceeds the #{max_file_size / 1_000_000}MB file limit." } if File.size(path) > max_file_size + + stats = { entries: 0, total_uncompressed_size: 0 } + entry_names = [] + content_types = nil + main_xml = nil + + Zip::File.open(path) do |zip_file| + zip_file.each do |entry| + raise 'Encrypted Word document entries are not supported.' if entry.respond_to?(:encrypted?) && entry.encrypted? + raise 'Word document contains an unsupported link entry.' if entry.respond_to?(:ftype) && entry.ftype == :symlink + safe_entry_name = entry.directory? ? entry.name.sub(%r{/+\z}, '') : entry.name + raise 'Word document contains a file with an unsafe path.' unless zip_path_safe?(safe_entry_name) + next if entry.directory? + + validate_zip_upload_entry!(entry.name, entry.size, stats, max_file_size, max_uncompressed_size) + raise 'Office document contains unsupported active or embedded content.' if entry.name.match?(%r{(?:vba|activex|embeddings|macrosheets|dialogsheets|externallinks|connections|querytables)|\.bin$}i) + raise 'Office document contains duplicate entries.' if entry_names.include?(entry.name) + entry_names << entry.name + content_types = entry.get_input_stream.read(1_000_000) if entry.name == '[Content_Types].xml' + main_xml = entry.get_input_stream.read(5_000_000) if entry.name == main_part + next unless entry.name.end_with?('.rels') + + relationships = Nokogiri::XML(entry.get_input_stream.read(1_000_000)) { |config| config.strict.nonet } + raise 'Office document contains unsafe XML declarations.' if relationships.internal_subset + relationships.xpath('//*[local-name()="Relationship"]').each do |relationship| + next unless relationship['TargetMode'].to_s.casecmp?('External') + # Ordinary hyperlinks do not fetch content automatically; external + # templates, workbooks, images and objects are not accepted. + raise 'Office document contains external content.' unless relationship['Type'].to_s.end_with?('/hyperlink') + end + end + end + + missing_entries = required_entries - entry_names + unless missing_entries.empty? + return { valid: false, msg: "Word document package is missing #{missing_entries.join(', ')}." } + end + + unless docx_main_document_declared?(content_types, format: format) + return { valid: false, msg: 'Word document package has an invalid main document content type.' } + end + + main_document = Nokogiri::XML(main_xml) { |config| config.strict.nonet } + expected_root = format == 'xlsx' ? 'workbook' : 'document' + expected_namespace = format == 'xlsx' ? 'http://schemas.openxmlformats.org/spreadsheetml/2006/main' : 'http://schemas.openxmlformats.org/wordprocessingml/2006/main' + unless main_document.root&.name == expected_root && main_document.root&.namespace&.href == expected_namespace && main_document.internal_subset.nil? + return { valid: false, msg: 'Office document contains an invalid main XML part.' } + end + + compressed_size = [File.size(path), 1].max + if stats[:total_uncompressed_size] / compressed_size > zip_compression_ratio_limit + return { valid: false, msg: "Word document compression ratio is too high. Limit is #{zip_compression_ratio_limit}:1." } + end + + { valid: true, msg: 'success' } + rescue Zip::Error, EOFError + { valid: false, msg: 'Word document is corrupted or is not a valid OOXML package.' } + rescue StandardError => e + { valid: false, msg: e.message } + end + + # The main part must be declared by the Override for /word/document.xml. + # Matching the type as a substring would also accept it inside a comment or + # on some other part. + def docx_main_document_declared?(content_types, format: 'docx') + return false if content_types.blank? + + document = Nokogiri::XML(content_types) { |config| config.strict.nonet } + return false if document.internal_subset || content_types.match?(/macroEnabled|vbaProject|activeX/i) + + part = format == 'xlsx' ? '/xl/workbook.xml' : '/word/document.xml' + expected_type = format == 'xlsx' ? 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet.main+xml' : DOCX_MAIN_DOCUMENT_CONTENT_TYPE + override = document.at_xpath( + "/ct:Types/ct:Override[@PartName='#{part}']", + 'ct' => OOXML_CONTENT_TYPES_NAMESPACE + ) + override.present? && override['ContentType'] == expected_type + rescue Nokogiri::XML::SyntaxError + false + end + def zip_tree_add_path(tree, path) clean_path = path.to_s.tr('\\', '/').sub(%r{\A\./+}, '').sub(%r{/+\z}, '') return if clean_path.blank? @@ -1025,8 +1195,10 @@ def line_wrap(path, width: 160) end # Export functions as module functions module_function :accept_file + module_function :log_file_rejection module_function :sanitized_path module_function :sanitized_filename + module_function :safe_upload_filename module_function :task_file_dir_for_unit module_function :tmp_file_dir module_function :tmp_file @@ -1063,6 +1235,8 @@ def line_wrap(path, width: 160) module_function :validate_zip_file module_function :validate_tar_file module_function :validate_zip_upload + module_function :validate_docx + module_function :docx_main_document_declared? module_function :zip_tree_add_path module_function :zip_tree_walk module_function :zip_file_tree diff --git a/app/helpers/spreadsheet_upload_policy.rb b/app/helpers/spreadsheet_upload_policy.rb new file mode 100644 index 0000000000..ec0cfdb127 --- /dev/null +++ b/app/helpers/spreadsheet_upload_policy.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'ole/storage' + +# Spreadsheet task files stay in their original format and are download-only. +# Chat deliberately excludes legacy XLS; task definitions retain the csv key. +module SpreadsheetUploadPolicy + def self.validate(file) + extension = File.extname(file['filename'] || file[:filename]).downcase + path = file['tempfile'].path + max_size = Doubtfire::Application.config.max_file_size.to_i + max_size = 10_000_000 if max_size <= 0 + unless File.size?(path) && File.size(path) <= max_size + FileHelper.log_file_rejection('Spreadsheet size check failed', 'csv', file) + return { accepted: false, msg: "Spreadsheet must not be empty or exceed #{max_size / 1_000_000}MB." } + end + detected = MimeCheckHelpers.mime_type(path).split(';').first + valid = case extension + when '.csv' + %w[text/csv application/csv text/plain].include?(detected) && CommentAttachmentPolicy.validate_csv(path)[:valid] + when '.xlsx' + %w[application/vnd.openxmlformats-officedocument.spreadsheetml.sheet application/zip].include?(detected) && FileHelper.validate_docx(path, format: 'xlsx')[:valid] + when '.xls' + %w[application/vnd.ms-excel application/x-ole-storage application/CDFV2].include?(detected) && valid_legacy_workbook?(path) + else false + end + FileHelper.log_file_rejection('Spreadsheet format check failed', 'csv', file) unless valid + { accepted: valid == true, msg: valid ? 'success' : 'Choose a valid, unencrypted CSV, XLS or XLSX spreadsheet without macros or embedded objects.' } + rescue StandardError + FileHelper.log_file_rejection('Spreadsheet validation failed', 'csv', file) + { accepted: false, msg: 'The spreadsheet could not be read. Save it as an unencrypted CSV or XLSX and try again.' } + end + + def self.valid_legacy_workbook?(path) + Ole::Storage.open(path) do |storage| + names = storage.dir.entries('/') - %w[. ..] + return false unless (names - ["Workbook", "Book", "\u0005SummaryInformation", "\u0005DocumentSummaryInformation"]).empty? + name = (names & %w[Workbook Book]).first + return false unless name + + storage.file.open(name) do |stream| + while (header = stream.read(4)) && !header.empty? + return false unless header.bytesize == 4 + record, size = header.unpack('vv') + data = stream.read(size) + return false unless data && data.bytesize == size + return false if record == 0x01ae && (data.bytesize < 4 || data.unpack('vv')[1] != 0x0401) # external workbook / DDE + return false if [0x002f, 0x00d3].include?(record) # encryption / VBA project + return false if record == 0x0085 && ![0, 2].include?(data.getbyte(5)) # macro / VB sheets + return false if record == 0x0809 && data.bytesize >= 4 && data.unpack('vv')[1] == 0x0040 + end + end + end + workbook = Roo::Spreadsheet.open(path, extension: :xls) + workbook.sheets.any? + ensure + workbook&.close if workbook.respond_to?(:close) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index 64dc33ed4e..58a1ec868a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -35,6 +35,10 @@ class Project < ApplicationRecord has_many :staff_notes, dependent: :destroy has_many :engagements, dependent: :destroy, inverse_of: :project + before_create :record_target_grade_change + before_update :record_target_grade_change, + if: :will_save_change_to_target_grade? + # Callbacks - methods called are private before_destroy :can_destroy? @@ -174,10 +178,30 @@ def enrol_in(tutorial) else # there is an existing enrolment... tutorial_enrolment.tutorial = tutorial tutorial_enrolment.update!(tutorial_id: tutorial.id) + notify_tutorial_changed(tutorial) end tutorial_enrolment end + def notify_tutorial_changed(tutorial) + student = self.student + return if student.blank? + + NotificationService.notify( + user: student, + type: 'general', + event: 'tutorial_changed', + message: "You have been moved to tutorial #{tutorial.abbreviation} in #{unit.code}. It meets on #{tutorial.meeting_day} at #{tutorial.meeting_time}.", + link: "/projects/#{id}/dashboard" + ) + rescue StandardError => e + logger.error( + "Failed to raise tutorial_changed notification for project #{id}: #{e.message}" + ) + end + + private :notify_tutorial_changed + def enrolled_in?(tutorial) tutorial_enrolments.select { |e| e.tutorial_id == tutorial.id }.count > 0 || tutorial_enrolments.where(tutorial_id: tutorial.id).count > 0 end @@ -278,7 +302,7 @@ def reference_date end def task_details_for_shallow_serializer(user) - tasks + task_rows = tasks .joins(:task_status) .joins("LEFT JOIN task_comments ON task_comments.task_id = tasks.id AND (task_comments.type IS NULL OR task_comments.type <> 'TaskStatusComment')") .joins("LEFT JOIN comments_read_receipts crr ON crr.task_comment_id = task_comments.id AND crr.user_id = #{user.id}") @@ -294,27 +318,69 @@ def task_details_for_shallow_serializer(user) 'completion_date', 'times_assessed', 'submission_date', 'grade', 'quality_pts', 'include_in_portfolio', 'grade' ) - .map do |r| - t = Task.find(r.id) - { - id: r.id, - status: TaskStatus.id_to_key(r.status_id), - task_definition_id: r.task_definition_id, - include_in_portfolio: r.include_in_portfolio, - times_assessed: r.times_assessed, - grade: r.grade, - quality_pts: r.quality_pts, - num_new_comments: r.number_unread, - similarity_flag: AuthorisationHelpers.authorise?(user, t, :view_plagiarism) ? r.similar_to_count > 0 : false, - extensions: t.extensions, - scorm_extensions: t.scorm_extensions, - due_date: t.due_date, - submission_date: t.submission_date, - completion_date: t.completion_date, - target_start_date: t.target_start_date, - target_due_date: t.target_due_date - } - end + .to_a + + # The aggregate rows intentionally select only the fields used directly in + # the response. Reload their complete Task records in one batch so due-date + # and authorisation helpers can use preloaded associations instead of doing + # a Task.find (plus project/unit/task-definition lookups) for every task. + tasks_by_id = Task + .where(id: task_rows.map(&:id)) + .preload(:granted_extension_comments, task_definition: :grade_due_dates, project: %i[unit user campus]) + .index_by(&:id) + + task_ids = task_rows.map(&:id) + + feedback_task_ids = TaskComment + .where(task_id: task_ids) + .where(content_type: %w[text audio image pdf discussion]) + .where(user_id: unit.staff.select(:user_id)) + .where.not("COALESCE(comment, '') LIKE ?", '**Automated Message:%') + .where( + <<~SQL.squish, + task_comments.created_at >= COALESCE( + ( + SELECT MIN(ready_comments.created_at) + FROM task_comments ready_comments + WHERE ready_comments.task_id = task_comments.task_id + AND ready_comments.content_type = 'status' + AND ready_comments.task_status_id = ? + ), + task_comments.created_at + ) + SQL + TaskStatus.ready_for_feedback.id + ) + .distinct + .pluck(:task_id) + .to_set + + task_rows.map do |r| + t = tasks_by_id.fetch(r.id) + { + id: r.id, + status: TaskStatus.id_to_key(r.status_id), + task_definition_id: r.task_definition_id, + include_in_portfolio: r.include_in_portfolio, + times_assessed: r.times_assessed, + grade: r.grade, + quality_pts: r.quality_pts, + num_new_comments: r.number_unread, + has_feedback: feedback_task_ids.include?(r.id), + similarity_flag: AuthorisationHelpers.authorise?(user, t, :view_plagiarism) ? r.similar_to_count > 0 : false, + extensions: t.extensions, + scorm_extensions: t.scorm_extensions, + due_date: t.due_date, + effective_deadline: t.effective_deadline, + effective_deadline_date: t.effective_deadline_date, + effective_deadline_reason: t.effective_deadline_reason, + effective_deadline_source_id: t.effective_deadline_source_id, + submission_date: t.submission_date, + completion_date: t.completion_date, + target_start_date: t.target_start_date, + target_due_date: t.target_due_date + } + end end def assigned_tasks @@ -636,7 +702,7 @@ def status_for_task_definition(td) # task if the task does not exist for this project. # def task_for_task_definition(td) - logger.debug "Finding task #{td.abbreviation} for project #{log_details}" + logger.debug "Finding task #{td.abbreviation} for project_id=#{id}" result = tasks.where(task_definition: td).first if result.nil? begin @@ -718,6 +784,10 @@ def escalation_attempts_remaining private + def record_target_grade_change + self.target_grade_changed_at = Time.current + end + def can_destroy? return true if tutorial_enrolments.count == 0 diff --git a/app/models/submission_history.rb b/app/models/submission_history.rb index 745925ac99..ab3adec4ba 100644 --- a/app/models/submission_history.rb +++ b/app/models/submission_history.rb @@ -41,7 +41,7 @@ def self.create_archive!(task, submission_timestamp) next if entry.name_is_directory? file_name = entry.name.split('/').last - next unless file_name&.match?(/^\d{3}-(?:document|code|image|zip|archive)/) + next unless file_name&.match?(/^\d{3}-(?:document|code|image|zip|archive|csv)/) next unless enabled_indexes.include?(file_name.to_i) destination.get_output_stream(File.join(history.entry_prefix, entry.name)) do |output| @@ -106,7 +106,7 @@ def has_submission_files? # rubocop:disable Naming/PredicateName return false unless File.exist?(archive_file_name) Zip::File.open(archive_file_name) { |archive| submission_entries(archive).any? } - rescue Zip::Error + rescue Zip::Error, Errno::ENOENT, Errno::EACCES false end diff --git a/app/models/test_attempt.rb b/app/models/test_attempt.rb index 9918414254..648269bc72 100644 --- a/app/models/test_attempt.rb +++ b/app/models/test_attempt.rb @@ -59,6 +59,8 @@ def specific_permission_hash(role, perm_hash, _other) # fields that must be synced from cmi data whenever it's updated # t.boolean :completion_status, default: false + + # staff owned, and no longer synced from cmi data. See cmi_datamodel= below. # t.boolean :success_status, default: false # t.float :score_scaled, default: 0 @@ -96,10 +98,19 @@ def cmi_datamodel=(data) end # IMPORTANT: always sync any model attributes with cmi values here to ensure consistency! - # attributes derived from cmi keys: completion_status, success_status, score_scaled + # attributes derived from cmi keys: completion_status self.completion_status = new_data['cmi.completion_status'] == 'completed' - self.success_status = new_data['cmi.success_status'] == 'passed' - self.score_scaled = new_data['cmi.score.scaled'] + + # success_status and score_scaled are deliberately no longer derived here. + # The datamodel is posted by the scorm package running in the student's own + # browser, and this setter is only reachable through the :update_attempt arm + # of PATCH test_attempts/:id, which only students hold. Deriving the pass and + # the score from that blob let a student decide their own result, which is + # what the route already refuses when it is asked for directly. + # override_success_status is now the only writer of success_status and the + # route gates it on :override_success_status. Nothing writes score_scaled, so + # it keeps its 0.0 column default. The datamodel is still stored exactly as + # it was posted, so the package keeps its runtime state and can resume. write_attribute(:cmi_datamodel, new_data.to_json) end @@ -151,7 +162,7 @@ def update_scorm_comment def success_status_description if self.success_status && self.score_scaled == 1 "Passed without mistakes" - elsif self.success_status && self.score_scaled < 1 + elsif self.success_status && self.score_scaled.to_f < 1 "Passed" else "Unsuccessful" diff --git a/app/sidekiq/accept_submission_job.rb b/app/sidekiq/accept_submission_job.rb index eaaf9b380e..b70456ec20 100644 --- a/app/sidekiq/accept_submission_job.rb +++ b/app/sidekiq/accept_submission_job.rb @@ -5,9 +5,14 @@ class AcceptSubmissionJob sidekiq_options lock: :until_executed, lock_args_method: ->(args) { [args.first] }, on_conflict: :reject, + queue: :submissions, retry: false - def perform(task_id, user_id, accepted_tii_eula, test_submission) + def perform(task_id, user_id, accepted_tii_eula, test_submission, *processing_options) + processing_mode = processing_options.first.to_s + queued_attempt = processing_options[1] + restore_archive = %w[retry_archive regenerate_only].include?(processing_mode) || processing_options.first == true + regeneration_only = processing_mode == 'regenerate_only' begin # Ensure cwd is valid... FileUtils.cd(Rails.root) @@ -16,7 +21,7 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) end begin - task = Task.find(task_id) + task = Task.find(task_id).submission_processing_task user = User.find(user_id) rescue StandardError => e logger.error e @@ -25,9 +30,35 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) begin logger.info "Accepting submission for task #{task.id} by user #{user.id}" + # Retries and regenerations carry the attempt they were queued for. Once a + # newer upload has been accepted (possible after the attempt timed out) + # this job is stale and must not restore the old archive over it. The + # check, the state change and the restore all happen under the lock the + # enqueuing request held: a fast worker waits for that request to commit, + # and no upload can be accepted between the check and the restore. + stale_attempt = nil + task.submission_processing_lock_target.with_lock do + task.reload + if queued_attempt.present? && task.submission_processing_attempts != queued_attempt.to_i + stale_attempt = task.submission_processing_attempts + else + task.mark_submission_processing!('processing') + task.prepare_submission_regeneration! if restore_archive + end + end + + unless stale_attempt.nil? + logger.info "Skipping stale submission processing for task #{task.id}: " \ + "queued for attempt #{queued_attempt}, now at #{stale_attempt}" + return + end + # Convert submission to PDF - task.convert_submission_to_pdf(log_to_stdout: true) + converted = task.convert_submission_to_pdf(log_to_stdout: true) + raise 'Submission files could not be prepared for conversion.' unless converted + task.mark_submission_processing!('ready') rescue StandardError => e + task.mark_submission_processing!('failed', error_code: 'conversion_failed') logger.error e # Send email to student if task pdf failed @@ -60,6 +91,10 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) return end + # Rebuilding a previously ready PDF must not create a duplicate Turnitin + # submission, moderation decision, or submission-history entry. + return if regeneration_only + # Mark this task for moderation tutor_user = task.project.tutor_for(task.task_definition) if tutor_user && !test_submission @@ -93,6 +128,9 @@ def perform(task_id, user_id, accepted_tii_eula, test_submission) } ) end - task.clear_in_process + task&.clear_in_process + if task && !task.submission_pdf_ready? + task.mark_submission_processing!('failed', error_code: 'processing_failed') + end end end diff --git a/app/views/shared/_file.pdf.erb b/app/views/shared/_file.pdf.erb index d97d945e7a..93b080d576 100644 --- a/app/views/shared/_file.pdf.erb +++ b/app/views/shared/_file.pdf.erb @@ -32,3 +32,9 @@ This zip file was uploaded with the portfolio. Zip contents are not expanded into this PDF. \end{tcolorbox} <% end %> + +<% if file_type == 'csv' %> +\begin{tcolorbox} +This spreadsheet was uploaded with the portfolio. Download the original submitted files to inspect it. +\end{tcolorbox} +<% end %> diff --git a/app/views/task/task_pdf.pdf.erb b/app/views/task/task_pdf.pdf.erb index e279bff132..f07e5b83a6 100644 --- a/app/views/task/task_pdf.pdf.erb +++ b/app/views/task/task_pdf.pdf.erb @@ -136,6 +136,13 @@ No Tutor % Supervisor's Name \includepdf[pages={<%= page_idx %>-<%= page_idx %>},fitpaper]{<%= file[:path] %>} <% end # end for %> <% end # end if %> + <% if file[:type] == 'csv' %> +\begin{tcolorbox} +\textbf{Spreadsheet submission}\\ +The original spreadsheet is retained without conversion. Download it to inspect all worksheets and values.\\ +\href{<%= @submitted_files_url %>}{Download submitted files (authentication required)} +\end{tcolorbox} + <% end %> <% if %w[zip archive].include?(file[:type]) %> <% archive_tree = FileHelper.zip_file_tree(file[:path], File.basename(file[:path])) %> \begin{tcolorbox}[enhanced,breakable,colback=orange!8!white,colframe=orange!80!black,boxrule=0.9pt,left=4mm,right=4mm,top=3mm,bottom=3mm] diff --git a/db/migrate/20260831000001_add_attachment_metadata_to_task_comments.rb b/db/migrate/20260831000001_add_attachment_metadata_to_task_comments.rb new file mode 100644 index 0000000000..4f7c8d0445 --- /dev/null +++ b/db/migrate/20260831000001_add_attachment_metadata_to_task_comments.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddAttachmentMetadataToTaskComments < ActiveRecord::Migration[8.0] + def change + add_column :task_comments, :attachment_original_filename, :string + add_column :task_comments, :attachment_content_type, :string + add_column :task_comments, :attachment_byte_size, :bigint + add_column :task_comments, :client_request_id, :string + add_index :task_comments, [:user_id, :task_id, :client_request_id], + unique: true, + name: 'idx_task_comments_user_task_client_request' + end +end diff --git a/db/migrate/20260831000002_add_submission_processing_state_to_tasks.rb b/db/migrate/20260831000002_add_submission_processing_state_to_tasks.rb new file mode 100644 index 0000000000..68a73aa7a3 --- /dev/null +++ b/db/migrate/20260831000002_add_submission_processing_state_to_tasks.rb @@ -0,0 +1,9 @@ +class AddSubmissionProcessingStateToTasks < ActiveRecord::Migration[8.0] + def change + add_column :tasks, :submission_processing_state, :string + add_column :tasks, :submission_processing_started_at, :datetime + add_column :tasks, :submission_processing_finished_at, :datetime + add_column :tasks, :submission_processing_error_code, :string + add_column :tasks, :submission_processing_attempts, :integer, default: 0, null: false + end +end diff --git a/db/migrate/20260831000003_add_submission_processing_options_to_tasks.rb b/db/migrate/20260831000003_add_submission_processing_options_to_tasks.rb new file mode 100644 index 0000000000..555cae2fa1 --- /dev/null +++ b/db/migrate/20260831000003_add_submission_processing_options_to_tasks.rb @@ -0,0 +1,10 @@ +class AddSubmissionProcessingOptionsToTasks < ActiveRecord::Migration[8.0] + # Kept separate from 20260831000002 because an environment built from #123 + # (for example through deploy#34's API pin) may already have run that one. + def change + add_column :tasks, :submission_processing_mode, :string + add_column :tasks, :submission_processing_user_id, :bigint + add_column :tasks, :submission_processing_test_submission, :boolean, default: false, null: false + add_column :tasks, :submission_processing_accepted_tii_eula, :boolean, default: false, null: false + end +end diff --git a/db/migrate/20260920071000_add_resubmission_extension_setting.rb b/db/migrate/20260920071000_add_resubmission_extension_setting.rb new file mode 100644 index 0000000000..64daf0421f --- /dev/null +++ b/db/migrate/20260920071000_add_resubmission_extension_setting.rb @@ -0,0 +1,8 @@ +class AddResubmissionExtensionSetting < ActiveRecord::Migration[8.0] + def change + add_column :task_definitions, :resubmission_extensions_enabled, :boolean, default: true, null: false + add_column :task_definitions, :resubmission_extensions_changed_at, :datetime + add_reference :task_definitions, :resubmission_extensions_changed_by, + foreign_key: { to_table: :users, on_delete: :nullify } + end +end diff --git a/docs/security/FILE-S01-security-findings.md b/docs/security/FILE-S01-security-findings.md new file mode 100644 index 0000000000..670c239688 --- /dev/null +++ b/docs/security/FILE-S01-security-findings.md @@ -0,0 +1,29 @@ +# FILE-S01 security findings and integration recommendation + +Date: 27 August 2026 + +## Disposition + +| Finding | Status | Evidence or follow-up | +| --- | --- | --- | +| Cross-project submission could create work before authorization | Verified | Exact 401 contract plus zero task, `TaskSubmission`, Sidekiq-job and storage deltas | +| Client extension or declared MIME could bypass content validation | Verified | API and `FileHelper` rejection tests assert exact MIME/extension outcomes | +| Unsafe ZIP paths or resource-amplifying archives | Verified | Traversal, entry-count, compression-ratio and total-uncompressed-size tests | +| A later duplicate could enqueue or store more work while processing | Verified for sequential duplicate | The test asserts one first-job/one first-payload and no second-request side effects | +| True simultaneous duplicate race | Open | Add two independent sessions/connections synchronized immediately inside the submission lock; assert one 201, one 403, one job and one payload | +| Rejected input could leave task-owned staging data | Verified before staging | Exact task-owned temporary, `new` and `in_process` paths remain absent after MIME rejection | +| Failure after staging or abandoned worker could leave data | Open | Inject a controlled failure after the first copy/move using isolated roots, define the cleanup contract, and assert owned paths are removed or recovered | +| Reviewed submission and comment-attachment logs expose content or student identifiers | Verified | Tests require exact 403/201 outcomes, safe markers, and absence of content, email, username and client filename; authentication and comment audit logging now use `user_id` | +| Repeated completed uploads could exhaust aggregate storage | Open | Define and test a per-user/unit quota or rate-limit policy; current evidence covers per-archive limits only | +| Comment attachment survives deletion | Verified | Direct model deletion, API deletion and subsequent 404 are covered | + +## Integration recommendation + +Merge the test and logging-sanitization changes after the exact security test +file and normal required CI checks pass. The evidence supports the verified +rows above. It does **not** support closing FILE-S01 as though true concurrent +races, post-staging cleanup and aggregate storage exhaustion were tested. + +Track the three open findings explicitly in the security objective. If the +ticket's acceptance criteria require every one of them before closure, keep the +ticket in progress even after this pull request merges. diff --git a/docs/security/FILE-S01-threat-model.md b/docs/security/FILE-S01-threat-model.md new file mode 100644 index 0000000000..ed04a4c851 --- /dev/null +++ b/docs/security/FILE-S01-threat-model.md @@ -0,0 +1,51 @@ +# FILE-S01 upload security threat model + +Date: 27 August 2026 + +## Scope + +This review covers the task-submission and task-comment attachment paths that +accept student-controlled files. The protected assets are another student's +work, task state and submission history, worker capacity, storage capacity, +server-side file paths, and identifiers or submitted content that could enter +logs. + +The relevant trust boundaries are: + +1. an unauthenticated client entering the authenticated API; +2. an authenticated student crossing into another project; +3. multipart metadata crossing into server-side MIME, extension and archive + validation; +4. a validated temporary upload crossing into task-owned staging storage and a + background job; and +5. request and validation data crossing into application logs. + +## Threats and verified controls + +| Threat | Expected control | Automated evidence | +| --- | --- | --- | +| Raw API submission without a session | Authentication rejects before task or storage work | `unauthenticated direct API upload is rejected with 419` | +| Cross-project POST, download or history access | Exact endpoint contract rejects the request; POST creates no task, submission, job or file | Three `student cannot ... another student` tests | +| Misleading extension, MIME or signature | Server-side allow-list and libmagic/PDF/archive validation reject the payload | MIME, signature, malformed-file and unsupported-extension tests | +| Archive traversal or resource amplification | Normalized archive paths plus entry-count, compression-ratio and uncompressed-size limits | ZIP traversal and resource-limit tests | +| Unsafe filename | Server-side path and filename sanitization | traversal, control-character and Unicode tests | +| Duplicate submission while work is queued | Task lock and queued-directory state reject the later request without additional state, payload or job | `sequential duplicate upload is blocked while first submission is queued` | +| Rejected input creates owned staging artifacts | Validation runs before state transition and staging | `early MIME rejection creates no task-owned staging artifacts` | +| Submission or attachment data leaks through reviewed log paths | Safe markers use internal ids and omit content, email, username and client filenames | three log-privacy tests | +| Deleted comment attachment remains retrievable | Model/API deletion removes the owned file and subsequent lookup returns 404 | comment attachment retention tests | + +## Deliberate limits + +The suite does not claim to prove all FILE-S01 risks are closed. In particular: + +- The duplicate test is sequential. It does not synchronize two independent + database connections at the row lock and therefore is not a true race test. +- Cleanup is proven only for rejection before staging. A controlled copy/move + failure after staging and an abandoned worker are not injected by this suite. +- Archive controls bound individual uploads, but the suite does not prove a + per-user storage quota or request-rate limit across many completed uploads. +- Log assertions cover submission validation and task-comment attachments, not + every historic file-related endpoint in the application. + +These limits are recorded as follow-ups in `FILE-S01-security-findings.md` and +must not be represented as passing evidence. diff --git a/docs/submission-history-access.md b/docs/submission-history-access.md new file mode 100644 index 0000000000..f148251590 --- /dev/null +++ b/docs/submission-history-access.md @@ -0,0 +1,168 @@ +# Student Submission History Access + +## Purpose + +SLR-H01 defines how students may access retained versions of their own previous submissions without exposing another student's submission history or internal storage details. + +## Authorisation model + +Student access is fail-closed. + +A student may retrieve submission-history metadata or download a retained archive only when the request resolves through the signed-in student's exact: + +1. project, +2. task definition, +3. task, and +4. submission history belonging to that task. + +A submission-history identifier does not grant access by itself. + +For student requests, both the project and task must authorise `:get_submission`. + +History reads authorise the project or staff unit permission before querying the existing task. They never create a task: an authorised request for a task that has not been created returns the same safe `404` response. + +The download route additionally preserves the existing staff path using the unit-level `:provide_feedback` permission. + +Invalid and unauthorised project, task-definition, task and history identifiers return the same generic response: + +`Submission history is not available` + +This prevents callers from using identifier substitution to determine whether another student's history exists. + +## Student metadata + +Students receive only the metadata required to display retained versions and request an authorised download: + +- `id` - only because the authorised download route requires the history identifier +- `version_order` - position in the returned history list, newest first +- `submission_timestamp` +- `status` + +The student response does not expose: + +- task IDs +- Overseer assessment IDs +- creation metadata not required by the student view +- filesystem paths +- archive paths +- storage implementation details + +The existing richer staff metadata contract is unchanged. + +## History states + +A retained history may have the following states: + +### available + +The history record exists and its retained archive contains submission files. + +### unavailable + +The authorised history record exists but its retained archive is missing, removed or otherwise has no available submission files. + +The download endpoint returns a safe `404` response for this case. + +### processing + +`SubmissionHistory.pending?(task)` indicates that a new retained history archive is currently being created. + +The metadata endpoint returns HTTP `202` while processing is active. Existing retained histories remain in the response and no synthetic history identifier is created for the pending archive. + +## Group submissions + +Current group membership is not evidence that a student was entitled to an older group submission. + +Historical access is therefore not granted by checking the student's current group. + +A student can access history only through their own authorised project and task. If existing immutable data cannot prove historical entitlement, the history remains unavailable to that student. + +Supporting historical access for former group members whose history is stored only against another member's task would require a separate immutable entitlement snapshot and is outside SLR-H01. + +Existing staff access is unchanged. + +## Retention + +Submission histories follow the configured unit and archive lifecycle. + +SLR-H01 does not guarantee that every submission version remains downloadable for an exact fixed period. Unit archival, deletion and other configured lifecycle operations may make a retained archive unavailable. + +For this reason the API reports file availability rather than promising a fixed retention duration. + +## Sanitised API examples + +### Student metadata + +Request: + +```http +GET /api/projects/123/task_def_id/45/submission_histories +``` + +Response: + +```json +[ + { + "id": 901, + "version_order": 1, + "submission_timestamp": "1788551000", + "status": "available" + } +] +``` + +### Authorised download + +Request: + +```http +GET /api/projects/123/task_def_id/45/submission_histories/901/files +``` + +If the signed-in user is authorised and the retained archive exists, the API returns the submission archive as a binary download with HTTP `200`. + +### Missing archive + +Response: + +```json +{ + "error": "Submission history files are not available" +} +``` + +HTTP `404`. + +### Invalid or unauthorised identifier + +Response: + +```json +{ + "error": "Submission history is not available" +} +``` + +HTTP `404`. + +## Security properties tested + +The API tests cover: + +- student's own metadata access +- student's own retained archive download +- existing staff metadata access +- existing staff archive download +- cross-student metadata access +- cross-student archive access +- substituted or invalid project identifiers +- task definitions from another unit +- substituted history identifiers +- histories belonging to another task +- changed current group membership +- missing retained archives +- processing archives +- minimal student metadata + +The student metadata and download routes both enforce authorisation server-side. diff --git a/docs/submission-lifecycle/effective-resubmission-deadline.md b/docs/submission-lifecycle/effective-resubmission-deadline.md new file mode 100644 index 0000000000..6e23185106 --- /dev/null +++ b/docs/submission-lifecycle/effective-resubmission-deadline.md @@ -0,0 +1,229 @@ +# The effective resubmission deadline + +**Status: the rule written here is the rule OnTrack already ran, written down. It is +not approved policy. SLR-E01 (Confirm the Intended Post-Feedback Deadline Rule) has +to confirm or correct it.** + +When staff send a task back to a student for more work, the student needs time to do +that work. If the deadline is close, OnTrack quietly moves it. Nobody had written down +what "close" means or how much time gets added, so SLR-E02 wrote it down and fixed the +parts that were wrong no matter which policy SLR-E01 lands on. + +## The rule as it stands + +A task earns one resubmission extension when all of these are true. + +| Condition | Where it lives | +|---|---| +| The task was set to Fix and Resubmit, Discuss, Rediscuss or Demonstrate | `Task#resubmission_extension_statuses` | +| The deadline is less than 7 days away, measured from the assessment | `Task#resubmission_extension_window` | +| The unit grants more than 0 weeks on resubmit | `Task#resubmission_extension_weeks` | +| The task can still be extended without passing the unit deadline | `Task#can_apply_for_extension?` | +| This round of feedback has not already had one | `Task#resubmission_extension_comment` | + +Two supporting values decide *when* those conditions are read. + +| Value | Where it lives | +|---|---| +| The moment the deadline passes, end of its day anywhere on earth | `Task#effective_deadline` | +| The far edge of the window, 7 days after the assessment | `Task#resubmission_extension_window_end` | + +The extension is the unit's `extension_weeks_on_resubmit_request`, capped so it never +runs past the unit deadline. Units that let students manage their own dates +(`allow_flexible_dates`) never get one. + +A round of feedback starts when the student submits. So a student who resubmits and is +sent back again earns another extension, and staff who assess the same submission twice +do not move the deadline twice. + +## What SLR-E01 has to decide + +1. Is 7 days the right window, and should it be measured from the assessment or from + the student reading the feedback. +2. Are those four statuses the right list. Demonstrate and Discuss ask the student to + turn up, not to resubmit, so they may not belong. +3. Is one extension per submission right, or should it be one per task for the whole + trimester. +4. Whether anything should be applied retroactively. Nothing here is. Tasks that were + over-extended by the old behaviour keep the weeks they were given. +5. Whose day a deadline belongs to. This branch says the student's, read off their campus, + because a deadline day that is not the student's day is not a deadline anyone can act + on. On an install that has left `campuses.timezone` empty nothing changes at all. On one + that has filled it in, a task near midnight can now fall on a different day than it did, + which means a small number of students qualify who did not, and the other way round. + +Changing 1, 2, 3 or 5 is a change to one of the methods named in the tables above. + +**Card requirement 1 is still open.** It asks the rule to conform to the approved policy, +and there is no approved policy: SLR-E01 has not started. Writing one here would be +inventing it. What this branch does instead is write down the rule OnTrack already ran and +put each part of it in one named place, so that confirming or correcting it later is a +small edit rather than an archaeology exercise. + +## Worked examples, all covered by tests in `test/models/task_test.rb` + +| Case | Result | +|---|---| +| Task due in 2 days, set to Fix and Resubmit | 1 week added, deadline moves once | +| Same task assessed again, same submission | Nothing changes | +| Same task set to Discuss straight after | Nothing changes | +| Student resubmits a week later, sent back again | A second week added | +| Tutor grants 2 more weeks, then reassesses | Stays at 3 weeks, nothing added or removed | +| Task due in 4 weeks, set to Fix and Resubmit | No extension | +| Unit grants 0 weeks on resubmit | No extension | +| Assessment processed with a date from 3 weeks ago | No extension, the window was shut then | +| A prerequisite fix cascades to a dependent task, twice | The dependent task gets 1 week, not 2 | +| Melbourne task due 10:30, either side of a clock change | Due at the end of the day it was set for, both times | +| Seven days from 09:00, the week the clocks move | Ends at 09:00, 167 real hours one way and 169 the other | +| Due Mon 5 Oct 2026, sent back Thu 1 Oct, clocks forward on the Sunday | Due Mon 12 Oct, not Sun the 11th | +| A student asks for a week, then is sent back near the deadline | Two separate extensions, only the second is a resubmission one | + +## Why the deadline used to move more than once + +`Task#grant_extension` adds weeks, it does not set them. The old code ran the whole check +on every call to `Task#assess`, so a second Fix and Resubmit on the same submission added +another week, and so did the recursive fix that cascades to dependent tasks. An already +overdue task was the worst case, because it stays inside the 7 day window after being +extended, so it could be extended again and again. + +## What the fix does + +- One extension per round of feedback. The check is an `ExtensionComment` recorded against + the task, so it survives restarts, retries and duplicate events, and needed no migration. +- That comment is also the audit trail. It records the weeks, the status that triggered it, + who assessed it, when, and a sentence the student can read. `task_status_id` is set on the + ones OnTrack worked out and nil on ones a student asked for, which is what tells them + apart. `ExtensionComment#serialize` exposes `resubmission_extension` and `source_status` + for the interface and for notifications. +- **Not `automatic`.** That word was already taken. `ExtensionComment#assess_extension` uses + it for a request a student made that the unit approved without a person weighing it up, + which is a different thing entirely - it is about who signed the extension off, not about + where it came from. One word carrying two meanings inside one class is how the wrong + branch gets taken, so the predicate is `resubmission_extension?` and the parameter on + `assess_extension` is `auto_approved`. Nothing in the class says "automatic" any more. +- The window is measured from the assessment's own timestamp rather than the wall clock, + so replaying an event gives the answer it gave at the time, and the seven days are added + as a duration rather than 168 fixed hours. +- The whole calculation is now done in the student's own time zone, which is the fix for + the date drift described in the next section. + +## Which day a deadline falls on + +This is the part that was wrong, and it was wrong in two ways at once. + +A deadline in OnTrack is a day, not an instant. A task due on Monday is not late until +Monday is over, and OnTrack is generous about that: it treats the deadline as the end of +that day *anywhere on earth*, which is 23:59:59 at UTC-12. So the one thing the code has +to get right is which day it is talking about. + +It got that day by reading the year, month and day straight off the deadline as the +database handed it back. That reads them in whatever `Time.zone` is, and **nothing in +`config/` sets `config.time_zone`, so `Time.zone` is UTC**. Meanwhile every campus carries +its own `timezone` column, added in `20251016033638_add_timezone_to_campuses`, and nothing +in this calculation read it. + +That is not just an offset. A campus in Melbourne is +11:00 through summer and +10:00 +through winter, so the same wall clock deadline sits on one UTC day for half the year and +the next one for the other half. A task due at 10:30 on Thursday 2 April 2026 was treated +as due on Wednesday the 1st. The identical task a week later, on Thursday 9 April, was +treated as due on Thursday the 9th, because the clocks had gone back on the Sunday in +between. Two deadlines set a week apart came out eight days apart. The seven day window +had the matching problem in the other direction, landing an hour late in the week the +clocks go forward and an hour early in the week they go back. + +The fix is that the calculation now names its own zone instead of inheriting one. + +| Method | What it does now | +|---|---| +| `Task#deadline_time_zone` | The campus's `timezone`, falling back to the application zone | +| `Task#deadline_date` | Reads a deadline's calendar day in that zone | +| `Task#to_same_day_anywhere_on_earth` | Builds the end of that day at a fixed `-12:00` | +| `Task#resubmission_extension_window_end` | Adds seven days in that zone, so it keeps its wall clock | + +`Campus#timezone` already falls back to the application zone when the column is empty, and +a project with no campus falls back to the same place. **So on an install that has not +filled in campus time zones, every one of these produces exactly the value it produced +before.** On an install that has filled them in, the deadline is now the student's day. + +`Task#raw_extension_date` and `Task#max_date_with_spec_con_days` were fixed at the same +time and for the same reason. They are what turn extension weeks into a date, so leaving +them reading the day in UTC would have put the corrected deadline back onto the wrong day +as soon as a task was extended. + +Three tests in `test/models/task_test.rb` cover this, all of them on a real Australian +campus and none of them touching the application zone. Reverted against the old +calculation they fail by a day on the deadline and by an hour on the window. + +**`config.time_zone` is deliberately still unset.** Setting it is a one line change with a +blast radius across every date in the product, and this branch targets `11.0.x`, which is a +release branch. It is named as a follow-up below rather than done here. + +## Known gaps, not fixed here + +Group submissions copy the submitter's extension count onto each member task and then run +the check on each of them, so a group can end up further ahead than its submitter. That is +a separate defect in `GroupSubmission#propagate_transition` and it needs its own ticket. + +These came out of an independent review of the change. None of them is a regression, every +one of them is either older than this branch or a consequence of deliberately not making a +retroactive change, and each needs a decision from SLR-E01 rather than a quiet fix. + +### SLR-E02-F1: set `config.time_zone`, or decide not to + +Nothing in `config/` sets `config.time_zone`, so the application zone is UTC everywhere. +The deadline calculation no longer cares, because it names the campus zone itself. It is +the only thing in the product that does. + +That is the follow-up. Every other date OnTrack renders, sorts, groups or writes to a +webcal is still read in UTC, including on a Melbourne campus that is ten or eleven hours +ahead of it, and a fair number of those will be a day out on the screen for exactly the +reason the deadline was. +Setting `config.time_zone` is one line, and one line with a blast radius across the whole +product, so it does not belong on `11.0.x` next to a deadline fix. **It is not done here on +purpose.** It needs its own ticket, its own read of what breaks, and a call on whether a +single application zone is even the right answer for a product with campuses on different +ones. + +### SLR-E02-F2: tasks extended by the old code carry no marker + +The guard asks whether this round of feedback already has an `ExtensionComment` recording a +resubmission extension. The old code created none, so a task that was already extended by +the old behaviour looks untouched. The first time the same submission is assessed after this +lands, it can be extended one more time. From then on it is idempotent like everything else. + +So the exposure is **one extra week, once, per affected task** - and only where the task was +already extended by the old code, is reassessed before the student submits again, and is +still inside the seven day window. The unbounded case, where an overdue task could be +extended on every single pass, is closed by this branch regardless. + +Three ways to close the rest were considered and none of them is safe to do here. + +| Option | Why not | +|---|---| +| Backfill comments for the old extensions | Nobody recorded which extensions were automatic or what triggered them, so this writes an audit trail that was never true, into every affected student's comment thread | +| Treat "extension weeks no comment accounts for" as already spent | `GroupSubmission#propagate_transition` copies the submitter's extension count onto every member task without a comment, so this would silently deny group members their first legitimate extension | +| Stamp a one-off marker in a migration | Needs a `task_status_id` the migration cannot know, and a student-visible comment on every affected task | + +**So this is a data migration decision, not a code one, and it needs the retroactivity call +from SLR-E01 first.** Requirement 6 of the card rules out retroactive changes, and every +option above is one. Named here so it is picked up deliberately rather than discovered. + +**Nothing serialises the check.** The read of the guard, the `extensions` update and the +comment insert are three statements with no lock and no transaction around them. Two +assessments landing together can both see no comment and both grant. A row lock on the task +would close it and is the obvious follow-up. + +**The extension is written before the comment.** `grant_extension` persists first and +`record_resubmission_extension` saves after. If the comment raises, the deadline has already +moved and no key exists to stop the next assessment moving it again. Wrapping the pair in a +transaction is the fix and it belongs with the lock above. + +**Group threads show one comment per member.** The comment is recorded against each member +task, and `Task#all_comments` returns every comment across the group submission, so a +three-person group assessed once shows three resubmission extension comments to everyone. The +extensions themselves are per task and correct. Only the thread is noisy. + +**Second-precision timestamps.** The guard compares `date_extension_assessed` against +`submission_date`. On a database still using the older second-precision `datetime` columns, +an assessment and a genuine resubmission inside the same second compare equal and the new +round is suppressed. Unlikely by hand, reachable by a script. diff --git a/docs/submission-lifecycle/student-history-and-deadline-controls.md b/docs/submission-lifecycle/student-history-and-deadline-controls.md new file mode 100644 index 0000000000..7c0b189dc6 --- /dev/null +++ b/docs/submission-lifecycle/student-history-and-deadline-controls.md @@ -0,0 +1,93 @@ +# Student history and deadline controls + +Implementation for SLR-H02/H03 and SLR-E03/E04/E05, building on API PRs #94 and #135. + +## Rule and policy boundary + +This change preserves the rule already merged in #94: eligible Fix and Resubmit, +Discuss, Rediscuss or Demonstrate feedback near a fixed target date adds the +unit-configured weeks, once per submission round, capped by the task maximum and +special consideration. Flexible dates remain student managed. It does not +recalculate historical feedback. + +The SLR-E01 PDF in web PR #249 explicitly says **Proposed – awaiting stakeholder +approval** and proposes seven calendar days from feedback with different repeated +feedback and flexible-date rules. That proposal is not an approved replacement +for #94. Reviewers should resolve that product decision separately; this PR does +not silently change the deployed calculation. The existing E02 DST and extension +regressions remain the executable specification for this change. + +## Task setting + +`resubmission_extensions_enabled` defaults to true for both existing and new +rows. The existing task-definition create/update authorization (`add_task_def`) +limits changes to convenors and administrators. Tutors, students and staff of +other units cannot update it. Disabling the setting only suppresses future +automatic extensions; it does not remove approved time or prevent a normal +extension request. The unit must still allow a positive number of resubmission +weeks and fixed dates. + +The API stamps the authenticated actor and time of the latest explicit change in +`resubmission_extensions_changed_by_id` and `resubmission_extensions_changed_at`. +Those fields are not client writable and are not added to student responses. +Defaults applied by migration have no invented historical actor. This is latest +change attribution, not a general audit-log subsystem. + +## Canonical deadline and delivery + +Task responses expose `effective_deadline_date`, `effective_deadline` (the instant +at end of day anywhere on earth), `effective_deadline_reason` and +`effective_deadline_source_id`. Reasons are `standard_due_date`, +`approved_extension`, `post_feedback_extension` or `flexible_date`. The flexible +case is a planned submission date, not a newly imposed hard deadline. + +The existing Webcal feed uses `effective_deadline_date` for its existing +`E-` all-day event. There is no new feed or second event. +Dates are read in the task campus time zone before converting to a civil date. + +An applied extension reserves one `resubmission_deadline_changed` notification +through NotificationService with a database-enforced dedupe key based on the +extension record. Message text includes the new date and general reason, never +feedback, task titles or student names. The existing extension category and +email/push preferences remain authoritative. Delivery occurs after all enclosing +transactions commit. Row locking serializes simultaneous extension attempts; +the deadline, replay marker and notification reservation commit together. +Group members keep their existing individual Task records and each affected +student receives only their own deadline notification. + +## Retained submission history + +The student list contains safe metadata only: record id, numeric newest-first +version order, submission/archive timestamp, availability and a current flag. +The latest archive is current only when it is at least as recent as the latest +processing start and no newer archive is pending. Legacy data without that +processing timestamp falls back to the recorded submission date. The UI also +identifies the latest retained archive without assuming that it is current. + +The original authorised download endpoint remains authoritative on every click. +Missing, removed and corrupt archives remain unavailable; their metadata is +retained where the row exists. HTTP 202 means archive creation is pending and +carries the existing retained versions. No unretained file is reconstructed and +no retention period changes. Group access remains tied to the student's own +historical Task; joining a group does not expose another member's old Task, and +leaving does not revoke archives retained on the student's own Task. Staff keep +the richer history response and existing comparison workflow. + +## Verification + +Run the focused API and existing policy/calendar packs: + +```sh +bundle exec rails test test/models/submission_lifecycle_test.rb test/api/resubmission_setting_test.rb test/api/submission_history_access_test.rb +bundle exec rails test test/models/task_test.rb -n '/resubmission|daylight|extension|deadline/' +bundle exec rails test test/models/webcal_test.rb test/api/webcal_api_test.rb test/models/submission_history_test.rb +``` + +The paired web PR supplies the student view and unit-chair setting. External +stakeholder approval and real Google/Apple/Outlook refresh behavior are outside +this repository change; CAL-Q02 remains responsible for calendar-client checks. + +The E05 trigger regression also found that `assess` skipped the declared +Discuss/Rediscuss/Demonstrate rule because those statuses took its completion +branch. The common eligibility check now runs after completion-date handling, +so every outcome declared by the existing rule reaches it. diff --git a/docs/uploads/safe-upload-contract.md b/docs/uploads/safe-upload-contract.md new file mode 100644 index 0000000000..8cd5cbdf9e --- /dev/null +++ b/docs/uploads/safe-upload-contract.md @@ -0,0 +1,104 @@ +# Safe upload contract and regression guide + +Implements FILE-B01, FILE-F02, FILE-T02 and the repository handover portion of +FILE-MVP01. This guide describes the changes in this PR; release still requires +review and merge. It extends the existing DOCX work (`batch03_docx_*` tests), +FILE-A01 policy by Sandil Sithmaka Bandara Weganthale, FILE-F01 requirement display, +and FILE-S01 security tests rather than replacing them. + +## Contract + +Authenticated `GET /api/task_comments/upload_policy` returns version 1, categories +with display names, extensions and preview modes, `max_bytes_exclusive: 30000000` +and `max_selection_count: 5`. The browser consumes this response; it must fail +closed for attachments if the response is unavailable. Text comments still work. +There is one file per POST. Five is the browser selection limit, not a server +quota: each selection is confirmed and each request independently validated. + +| Context | Accepted formats | Limit / rendering | +| --- | --- | --- | +| Task `csv` / Spreadsheet | CSV, XLS, XLSX | configured per-file maximum (10,000,000 bytes fallback), inclusive; original files retained, download-only notice in PDF | +| Task `code` / Code | existing curated frontend extensions | same task limit; existing code rendering | +| Other task categories | PDF, image, archive | existing configured requirements; `archive` aliases `zip` | +| Chat Document | DOCX | smaller than 30,000,000 bytes; download only | +| Chat Spreadsheet | CSV, XLSX | smaller than 30,000,000 bytes; download only | +| Chat PDF, Image, Audio | exact extensions from policy response | smaller than 30,000,000 bytes; existing conversion and rendering | + +FILE-A01's audit described `csv` as an existing task category. Inspection of the +current API instead found only the browser extension list: task-definition +validation, upload validation, and archived-file filtering omitted it. This PR +completes those paths while keeping the stored key `csv`. No data migration is +needed. Spreadsheet originals are not converted to CSV or reduced to one sheet. + +Legacy XLS remains available for task requirements with an OLE stream whitelist, +BIFF encryption/macro checks and a spreadsheet parser. New chat XLS is excluded: +save as CSV or macro-free XLSX. Macro-enabled files, embedded objects, executable +files, arbitrary archives in chat, encrypted packages and remote Office content +are excluded. Office hyperlinks may remain; external templates, images and +workbooks are rejected. Validation does not claim malware scanning. Raw PCM is +accepted only in the existing audio conversion path; generic binary is not +accepted for other audio extensions. Browser audio recordings named `blob` +remain supported only when their detected media type is audio/WebM. + +`document` remains the task PDF identifier. DOCX is supported in chat, not exposed +as a task requirement by this PR. Code keeps its reviewed list; arbitrary known +extensions are no longer accepted as Code simply because bytes look like text. + +## Validation and storage + +`CommentAttachmentPolicy` owns the chat policy. `SpreadsheetUploadPolicy` owns +task spreadsheet validation. `FileHelper` retains the shared validators. Files +are checked before permanent storage for size, extension and detected MIME. +Browser-reported MIME is never accepted as evidence of file contents. CSV is +parsed as UTF-8 records; XLSX/DOCX require safe ZIP entries, bounded expansion, +valid content types, valid main XML and safe relationship targets. Task files +are bounded before spreadsheet parsing. Archive protections remain in effect. + +The existing project authorization, task-scoped lookup, group access and retry +identifier contract remain intact. Generic attachments use existing ID-based +storage, sanitized metadata and the existing transaction cleanup path. Rejected +uploads stay in Rack-managed temporary storage; no task attachment is created. +A failure after storage removes the stored file before transaction rollback. + +Generic downloads always use `Content-Disposition: attachment`, `nosniff` and +the existing no-cache revalidation behavior. They preserve bytes and use an authenticated endpoint. +Old attachments remain accessible under the existing authorization rules. + +Errors preserve the existing `error` string and add stable `code` values: +`UPLOAD_EMPTY`, `UPLOAD_TOO_LARGE`, `UPLOAD_EXTENSION_NOT_ALLOWED`, +`UPLOAD_MIME_INVALID`, `UPLOAD_CORRUPT`, `UPLOAD_ENCRYPTED`. Authorization retains +existing 403/404 behavior. Rejections are logged at INFO without client names, +content, emails or server paths. Success is logged only after a comment exists. + +## Repeatable regression matrix + +Run with the repository's populated test database and normal test environment: + +```sh +bundle exec rails test test/lib/batch03_docx_file_helper_test.rb test/lib/spreadsheet_upload_policy_test.rb test/api/comments/batch03_docx_attachment_test.rb test/api/comments/safe_attachment_policy_test.rb +bundle exec rails test test/models/file_helper_test.rb test/api/upload_security_test.rb test/api/comments/comment_test.rb +``` + +| Acceptance / failure path | Automated coverage | +| --- | --- | +| Task spreadsheet key, original archive, curated Code list | `spreadsheet_upload_policy_test.rb` | +| Valid CSV/XLSX, legacy task XLS, chat exclusions | spreadsheet + safe attachment policy tests | +| Renamed, empty, exact-size, malformed, active, external and traversal payloads | spreadsheet + safe attachment policy tests; existing DOCX helper tests | +| Safe names, byte-preserving DOCX, converted images, old attachment types | `batch03_docx_attachment_test.rb`, `comment_test.rb` | +| Cross-project denial, group authorization, deletion | safe attachment tests, existing `comment_test.rb` / `upload_security_test.rb` | +| Retry idempotency, failure before/after storage, cleanup | existing `batch03_docx_attachment_test.rb` | +| Safe INFO rejection and truthful success logging | safe attachment tests and `file_helper_test.rb` | +| Task submissions: authorization, MIME, archive bounds, duplicates | `upload_security_test.rb` | +| Requirements, picker/drop/paste, confirmation, draft preservation, proxy 413 | companion doubtfire-web PR tests | +| Direct/proxied 29,999,999 and 30,000,000 byte uploads | doubtfire-deploy `production/tests/probe_upload_limits.py` | + +The proxy stays finite and larger than the application limit plus multipart +headers. The existing deploy default is 1g; the deploy PR validates a minimum +32m and provides JSON 413 responses. It does not increase the application limit. +See [deploy PR 37](https://github.com/ontrack-features-t2-2026/doubtfire-deploy/pull/37) +and [safe logging PR 167](https://github.com/ontrack-features-t2-2026/doubtfire-api/pull/167). + +Known security follow-ups from FILE-S01 (aggregate storage quotas, simultaneous +submission race coverage, abandoned worker cleanup) are not claimed resolved by +attachment-format support. The existing per-file and archive limits remain. +Review and merge are intentionally left to another reviewer. diff --git a/lib/tasks/generate_pdfs.rake b/lib/tasks/generate_pdfs.rake index 5db26d07e7..64108484a6 100644 --- a/lib/tasks/generate_pdfs.rake +++ b/lib/tasks/generate_pdfs.rake @@ -9,7 +9,7 @@ namespace :submission do end def is_process_running?(pid) - rreturn false if pid == 0 + return false if pid == 0 Process.getpgid(pid) true diff --git a/test/api/comments/batch03_docx_attachment_test.rb b/test/api/comments/batch03_docx_attachment_test.rb new file mode 100644 index 0000000000..56fec934b9 --- /dev/null +++ b/test/api/comments/batch03_docx_attachment_test.rb @@ -0,0 +1,374 @@ +# frozen_string_literal: true + +require 'test_helper' + +class Batch03DocxAttachmentTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + DOCX_MIME_TYPE = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' + + def app + Rails.application + end + + def setup + super + @project = FactoryBot.create(:project) + @student = @project.student + @task_definition = @project.unit.task_definitions.first + @task = @project.task_for_task_definition(@task_definition) + @comments_endpoint = "/api/projects/#{@project.id}/task_def_id/#{@task_definition.id}/comments" + @docx_path = Rails.root.join('test_files/TestWordDoc.docx') + add_auth_header_for(user: @student) + end + + def docx_upload(filename: 'Phone evidence.docx') + Rack::Test::UploadedFile.new( + @docx_path, + DOCX_MIME_TYPE, + true, + original_filename: filename + ) + end + + test 'uploads and downloads DOCX with exact bytes and attachment metadata' do + original_bytes = File.binread(@docx_path) + original_filename = 'Phone evidence original.docx' + + post @comments_endpoint, + comment: 'Evidence captured on phone', + attachment: docx_upload(filename: original_filename), + client_request_id: SecureRandom.uuid + + assert_equal 201, last_response.status, last_response.body + response = last_response_body + comment = TaskComment.find(response.fetch('id')) + + assert_equal 'document', comment.content_type + assert_equal '.docx', comment.attachment_extension + assert_equal original_filename, comment.attachment_file_name + assert_equal DOCX_MIME_TYPE, comment.attachment_mime_type + assert_equal original_bytes.bytesize, comment.attachment_size + assert_equal original_bytes, File.binread(comment.attachment_path) + + assert_equal true, response['has_attachment'] + assert_equal 'document', response['type'] + assert_equal original_filename, response['attachment_file_name'] + assert_equal DOCX_MIME_TYPE, response['attachment_mime_type'] + assert_equal original_bytes.bytesize, response['attachment_byte_size'] + + get "#{@comments_endpoint}/#{comment.id}" + + assert_equal 200, last_response.status, last_response.body + assert_equal original_bytes, last_response.body + assert_match(/\A#{Regexp.escape(DOCX_MIME_TYPE)}(?:;|\z)/, last_response.headers['Content-Type'].to_s) + assert_match(/attachment/i, last_response.headers['Content-Disposition'].to_s) + assert_includes last_response.headers['Content-Disposition'].to_s, original_filename + ensure + comment&.destroy + end + + test 'repeating a client request id returns the same comment and creates one row' do + client_request_id = SecureRandom.uuid + initial_count = @task.comments.where(user_id: @student.id, client_request_id: client_request_id).count + + post @comments_endpoint, + attachment: docx_upload, + client_request_id: client_request_id + + assert_equal 201, last_response.status, last_response.body + first_response = last_response_body + + post @comments_endpoint, + attachment: docx_upload(filename: 'Retry should not replace original.docx'), + client_request_id: client_request_id + + assert_equal 201, last_response.status, last_response.body + second_response = last_response_body + + assert_equal first_response['id'], second_response['id'] + assert_equal initial_count + 1, + @task.comments.where(user_id: @student.id, client_request_id: client_request_id).count + assert_equal 'Phone evidence.docx', TaskComment.find(first_response['id']).attachment_file_name + ensure + TaskComment.where(task: @task, user: @student, client_request_id: client_request_id).destroy_all if client_request_id + end + + test 'repeating a text client request id returns the same comment and creates one row' do + client_request_id = SecureRandom.uuid + initial_count = @task.comments.where(user_id: @student.id, client_request_id: client_request_id).count + + post_json @comments_endpoint, + comment: 'Typed once while uploading several attachments', + client_request_id: client_request_id + + assert_equal 201, last_response.status, last_response.body + first_response = last_response_body + + post_json @comments_endpoint, + comment: 'Typed once while uploading several attachments', + client_request_id: client_request_id + + assert_equal 201, last_response.status, last_response.body + second_response = last_response_body + + assert_equal first_response['id'], second_response['id'] + assert_equal initial_count + 1, + @task.comments.where(user_id: @student.id, client_request_id: client_request_id).count + ensure + TaskComment.where(task: @task, user: @student, client_request_id: client_request_id).destroy_all if client_request_id + end + + test 'serves a stored Unicode HTML-like filename with RFC 5987 download disposition' do + supplied_filename = 'evidence .DOCX' + stored_filename = '📱 evidence .DOCX' + + post @comments_endpoint, + attachment: docx_upload(filename: supplied_filename), + client_request_id: SecureRandom.uuid + + assert_equal 201, last_response.status, last_response.body + comment = TaskComment.find(last_response_body.fetch('id')) + assert_equal supplied_filename, comment.attachment_file_name + comment.update!(attachment_original_filename: stored_filename) + comment.reload + + assert_equal stored_filename, comment.attachment_file_name + assert_operator comment.attachment_file_name.length, :<=, 255 + assert_equal comment.attachment_file_name.gsub(/[[:cntrl:]]/, ''), comment.attachment_file_name + assert_not_includes comment.attachment_file_name, '/' + + get "#{@comments_endpoint}/#{comment.id}" + + assert_equal 200, last_response.status, last_response.body + disposition = last_response.headers['Content-Disposition'].to_s + assert_match(/\Aattachment;/i, disposition) + assert_match(/filename\*=UTF-8''/i, disposition) + assert_match(/%F0%9F%93%B1/i, disposition) + assert_no_match(/[\r\n]/, disposition) + ensure + comment&.destroy + end + + test 'rejects an attachment exactly at the 30MB boundary without creating a comment' do + initial_count = @task.comments.count + + Tempfile.create(['batch03-size-boundary', '.docx']) do |tempfile| + tempfile.truncate(30_000_000) + tempfile.flush + + post @comments_endpoint, + attachment: Rack::Test::UploadedFile.new( + tempfile.path, + DOCX_MIME_TYPE, + true, + original_filename: 'at-limit.docx' + ), + client_request_id: SecureRandom.uuid + end + + assert_includes 400..499, last_response.status, last_response.body + assert_match(/maximum attachment size of 30MB/i, last_response_body.fetch('error')) + assert_equal initial_count, @task.comments.count + end + + test 'a different student cannot download the DOCX attachment' do + unit = FactoryBot.create(:unit, student_count: 2) + owner_project, other_project = unit.active_projects.first(2) + task_definition = unit.task_definitions.first + endpoint = "/api/projects/#{owner_project.id}/task_def_id/#{task_definition.id}/comments" + + add_auth_header_for(user: owner_project.student) + post endpoint, + attachment: docx_upload, + client_request_id: SecureRandom.uuid + + assert_equal 201, last_response.status, last_response.body + comment = TaskComment.find(last_response_body.fetch('id')) + + add_auth_header_for(user: other_project.student) + get "#{endpoint}/#{comment.id}" + + assert_equal 403, last_response.status, last_response.body + assert_match(/cannot read the comments/i, last_response_body.fetch('error')) + ensure + comment&.destroy + unit&.destroy + end + + test 'a DOCX storage failure leaves no task comment row' do + initial_count = @task.comments.count + move_failure = lambda do |_source, _destination| + raise IOError, 'simulated Batch03 storage failure' + end + + FileUtils.stub(:mv, move_failure) do + post @comments_endpoint, + attachment: docx_upload, + client_request_id: SecureRandom.uuid + end + + assert_includes 500..599, last_response.status, last_response.body + assert_equal initial_count, @task.comments.count + end + + test 'a failure after the DOCX is stored removes the file before the rollback' do + initial_count = @task.comments.count + comment_dir = FileHelper.student_work_dir(:comment, @task) + files_before = Dir.children(comment_dir).sort + outer_transactions = TaskComment.connection.open_transactions + removals = [] + original_rm_f = FileUtils.method(:rm_f) + recording_rm_f = lambda do |path, **options| + removals << [path.to_s, TaskComment.connection.open_transactions] if path.to_s.start_with?(comment_dir) + original_rm_f.call(path, **options) + end + # safe_upload_filename runs after the file has been moved into place. + failure_after_storage = lambda do |*_args, **_options| + raise IOError, 'simulated failure after storage' + end + + FileUtils.stub(:rm_f, recording_rm_f) do + FileHelper.stub(:safe_upload_filename, failure_after_storage) do + post @comments_endpoint, + attachment: docx_upload, + client_request_id: SecureRandom.uuid + end + end + + assert_includes 500..599, last_response.status, last_response.body + assert_equal initial_count, @task.comments.count + assert_equal files_before, Dir.children(comment_dir).sort + + # Outside tests the rollback is a real one and resets the new row's id, + # which the storage path is built from, so the file must be removed while + # the transaction is still open. A test savepoint keeps the id, so the + # order is asserted directly. + removal = removals.find { |path, _open_transactions| path.end_with?('.docx') } + assert removal, 'the stored DOCX should be removed' + assert_operator removal.last, :>, outer_transactions + end + + test 'an overlapping attachment retry that loses the race returns the stored comment' do + client_request_id = SecureRandom.uuid + original_accept_file = FileHelper.method(:accept_file) + winner = nil + # The endpoint's format check runs after its client_request_id lookup, so + # storing the original request here reproduces a retry that missed it. + racing_accept_file = lambda do |*args| + winner ||= @task.add_text_comment(@student, 'Original request', nil, client_request_id) + original_accept_file.call(*args) + end + + FileHelper.stub(:accept_file, racing_accept_file) do + post @comments_endpoint, + attachment: docx_upload, + client_request_id: client_request_id + end + + assert_equal 201, last_response.status, last_response.body + assert_equal winner.id, last_response_body['id'] + assert_equal 1, @task.comments.where(user_id: @student.id, client_request_id: client_request_id).count + ensure + TaskComment.where(task: @task, user: @student, client_request_id: client_request_id).destroy_all if client_request_id + end + + test 'an overlapping text retry that loses the race returns the stored comment' do + client_request_id = SecureRandom.uuid + text = 'Typed once, sent twice' + parent = @task.add_text_comment(@student, 'Earlier message') + # The original request's comment, under a placeholder id so the retry's + # first lookup misses it. + winner = @task.add_text_comment(@student, text, nil, SecureRandom.uuid) + original_find = TaskComment.method(:find) + raced = false + # Replying makes the endpoint load the parent after its first lookup, and + # the original request "commits" here. The raw update stands in for that + # request's own connection: like a commit elsewhere, it does not clear this + # request's query cache, which still holds the first lookup's miss. + racing_find = lambda do |*args| + unless raced + raced = true + TaskComment.connection.raw_connection.query( + "UPDATE task_comments SET client_request_id = '#{client_request_id}' WHERE id = #{winner.id}" + ) + end + original_find.call(*args) + end + + ActiveRecord::Base.cache do + TaskComment.stub(:find, racing_find) do + post_json @comments_endpoint, + comment: text, + reply_to_id: parent.id, + client_request_id: client_request_id + end + end + + assert_equal 201, last_response.status, last_response.body + assert_equal winner.id, last_response_body['id'] + assert_equal 1, @task.comments.where(user_id: @student.id, client_request_id: client_request_id).count + ensure + TaskComment.where(task: @task, user: @student, client_request_id: client_request_id).destroy_all if client_request_id + parent&.destroy + end + + test 'a converted image downloads with the extension of the stored format' do + post @comments_endpoint, + attachment: Rack::Test::UploadedFile.new( + Rails.root.join('test_files/submissions/Deakin_Logo.jpeg'), + 'image/png', + true, + original_filename: 'Screenshot 1.png' + ), + client_request_id: SecureRandom.uuid + + assert_equal 201, last_response.status, last_response.body + comment = TaskComment.find(last_response_body.fetch('id')) + # Images other than GIF are stored as JPEG, so the download name follows. + assert_equal '.jpg', comment.attachment_extension + assert_equal 'Screenshot 1.jpg', comment.attachment_file_name + assert_equal 'Screenshot 1.jpg', last_response_body['attachment_file_name'] + ensure + comment&.destroy + end + + test 'a comment that is only a NUL character is not stored' do + initial_count = @task.comments.count + + post_json @comments_endpoint, comment: "\u0000" + + assert_equal 403, last_response.status, last_response.body + assert_equal initial_count, @task.comments.count + end + + test 'a text comment that only differs by surrounding whitespace is still a duplicate' do + post_json @comments_endpoint, comment: 'Please check the second figure' + assert_equal 201, last_response.status, last_response.body + + post_json @comments_endpoint, comment: " Please check the second figure \n" + + assert_equal 403, last_response.status, last_response.body + assert_equal 1, @task.comments.where(user_id: @student.id, comment: 'Please check the second figure').count + end + + test 'unsupported attachment returns a controlled 4xx without creating a comment' do + initial_count = @task.comments.count + invalid_upload = Rack::Test::UploadedFile.new( + Rails.root.join('test_files/submissions/test.txt'), + 'text/plain', + true, + original_filename: 'unsupported.txt' + ) + + post @comments_endpoint, + attachment: invalid_upload, + client_request_id: SecureRandom.uuid + + assert_includes 400..499, last_response.status, last_response.body + assert_match(/not an acceptable format/i, last_response_body.fetch('error')) + assert_equal initial_count, @task.comments.count + end +end diff --git a/test/api/comments/safe_attachment_policy_test.rb b/test/api/comments/safe_attachment_policy_test.rb new file mode 100644 index 0000000000..62a89d95d0 --- /dev/null +++ b/test/api/comments/safe_attachment_policy_test.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +require 'test_helper' + +class SafeAttachmentPolicyTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + setup do + @project = FactoryBot.create(:project) + @task_definition = @project.unit.task_definitions.first + @task = @project.task_for_task_definition(@task_definition) + @endpoint = "/api/projects/#{@project.id}/task_def_id/#{@task_definition.id}/comments" + add_auth_header_for(user: @project.student) + end + + def with_csv(content = "name,score\nExample,7\n", filename: 'results.csv') + Tempfile.create(['safe-attachment', '.csv']) do |file| + file.write(content) + file.flush + yield Rack::Test::UploadedFile.new(file.path, 'text/csv', true, original_filename: filename) + end + end + + test 'authenticated policy is explicit and keeps legacy XLS out of chat' do + get '/api/task_comments/upload_policy' + assert_equal 200, last_response.status + assert_equal 30_000_000, last_response_body['max_bytes_exclusive'] + spreadsheet = last_response_body['categories'].find { |item| item['id'] == 'spreadsheet' } + assert_equal %w[csv xlsx], spreadsheet['extensions'] + assert_equal 'download', spreadsheet['preview'] + assert_not_includes last_response.body, 'mime_types' + end + + test 'CSV is stored unchanged and downloaded with safe metadata and headers' do + with_csv do |file| + post @endpoint, attachment: file + end + assert_equal 201, last_response.status, last_response.body + comment = TaskComment.find(last_response_body['id']) + assert_equal 'spreadsheet', last_response_body['type'] + assert_equal 'results.csv', last_response_body['attachment_file_name'] + assert_equal "name,score\nExample,7\n", File.read(comment.attachment_path) + get "#{@endpoint}/#{comment.id}?as_attachment=false" + assert_equal 200, last_response.status + assert_match(/attachment/, last_response.headers['Content-Disposition']) + assert_equal 'nosniff', last_response.headers['X-Content-Type-Options'] + assert_equal 'no-cache', last_response.headers['Cache-Control'] + ensure + comment&.destroy + end + + test 'malformed and spoofed CSV or legacy XLS are rejected without stored rows' do + initial = @task.comments.count + [["\"unterminated", 'bad.csv'], ["MZ\x00binary", 'bad.csv'], ['a,b', 'macro.xls'], ['a,b', 'active.xlsm'], ['a,b', 'fake.xlsx']].each do |content, filename| + with_csv(content, filename: filename) { |file| post @endpoint, attachment: file } + assert_equal 403, last_response.status, last_response.body + assert_match(/\AUPLOAD_/, last_response_body['code']) + assert_equal initial, @task.comments.count + end + end + + test 'empty and exact boundary have stable failure codes' do + with_csv('') { |file| post @endpoint, attachment: file } + assert_equal 400, last_response.status + assert_equal 'UPLOAD_EMPTY', last_response_body['code'] + with_csv('x' * 30_000_000) { |file| post @endpoint, attachment: file } + assert_equal 413, last_response.status + assert_equal 'UPLOAD_TOO_LARGE', last_response_body['code'] + end + + test 'other project student cannot retrieve spreadsheet through either project route' do + with_csv { |file| post @endpoint, attachment: file } + assert_equal 201, last_response.status + comment = TaskComment.find(last_response_body['id']) + other = FactoryBot.create(:project) + add_auth_header_for(user: other.student) + get "#{@endpoint}/#{comment.id}" + assert_equal 403, last_response.status + get "/api/projects/#{other.id}/task_def_id/#{other.unit.task_definitions.first.id}/comments/#{comment.id}" + assert_equal 404, last_response.status + ensure + comment&.destroy + end + test 'a rejected upload logs a safe reason without claiming a comment was added' do + output = StringIO.new + log = ActiveSupport::Logger.new(output) + log.level = Logger::INFO + original = Rails.logger + Rails.logger = log + with_csv('private file content', filename: 'private-student-name.exe') do |file| + post @endpoint, attachment: file + end + assert_equal 403, last_response.status + assert_includes output.string, 'File extension check failed' + assert_not_includes output.string, 'added comment' + assert_not_includes output.string, 'private-student-name' + assert_not_includes output.string, 'private file content' + ensure + Rails.logger = original + end + + test 'task Spreadsheet requirement accepts CSV through the submission API and retains the original' do + @task_definition.update!( + start_date: Time.zone.now - 1.week, + target_date: Time.zone.now + 1.week, + upload_requirements: [{ 'key' => 'file0', 'name' => 'Results', 'type' => 'csv' }] + ) + with_csv do |file| + post "/api/projects/#{@project.id}/task_def_id/#{@task_definition.id}/submission", + trigger: 'ready_for_feedback', file0: file + end + assert_equal 201, last_response.status, last_response.body + @task.reload + stored = File.join(@task.student_work_dir(:new, false), '000-csv.csv') + assert File.exist?(stored) + assert_equal "name,score\nExample,7\n", File.read(stored) + assert(AcceptSubmissionJob.jobs.any? { |job| job['args'].first == @task.id }) + ensure + FileUtils.rm_rf(@task.student_work_dir(:new, false)) if @task + end + + test 'staff document and spreadsheet comments count as manual feedback but student attachments do not' do + tutor = @project.unit.staff.first.user + %w[document spreadsheet].each do |type| + comment = TaskComment.create!(task: @task, user: tutor, recipient: @project.student, + content_type: type, comment: 'Attached feedback') + assert @task.has_manual_feedback_since_first_ready_for_feedback? + assert @task.has_recent_manual_feedback_from_tutor?(tutor) + comment.update!(user: @project.student) + assert_not @task.has_manual_feedback_since_first_ready_for_feedback? + assert_not @task.has_recent_manual_feedback_from_tutor?(tutor) + comment.destroy! + end + end + +end diff --git a/test/api/resubmission_setting_test.rb b/test/api/resubmission_setting_test.rb new file mode 100644 index 0000000000..5d023eecc7 --- /dev/null +++ b/test/api/resubmission_setting_test.rb @@ -0,0 +1,72 @@ +require 'test_helper' + +class ResubmissionSettingTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + setup do + @unit = FactoryBot.create(:unit, student_count: 1, task_count: 1, staff_count: 2) + @definition = @unit.task_definitions.first + @endpoint = "/api/units/#{@unit.id}/task_definitions/#{@definition.id}" + end + + test 'convenor can disable and reenable one task with server controlled change attribution' do + add_auth_header_for(user: @unit.main_convenor_user) + put_json @endpoint, { task_def: { resubmission_extensions_enabled: false, + resubmission_extensions_changed_by_id: @unit.active_projects.first.student.id } } + assert_equal 200, last_response.status, last_response.body + assert_not @definition.reload.resubmission_extensions_enabled + assert_equal false, last_response_body['resubmission_extensions_enabled'] + assert_equal @unit.main_convenor_user.id, @definition.resubmission_extensions_changed_by_id + assert_not_nil @definition.resubmission_extensions_changed_at + original_time = @definition.resubmission_extensions_changed_at + put_json @endpoint, { task_def: { resubmission_extensions_enabled: false } } + assert_equal original_time, @definition.reload.resubmission_extensions_changed_at + put_json @endpoint, { task_def: { resubmission_extensions_enabled: true } } + assert_equal 200, last_response.status + assert @definition.reload.resubmission_extensions_enabled + end + + test 'student tutor and unrelated convenor cannot change the setting' do + tutor = FactoryBot.create(:user, :tutor) + FactoryBot.create(:unit_role, unit: @unit, user: tutor, role: Role.tutor) + outsider = FactoryBot.create(:unit, student_count: 0, task_count: 0).main_convenor_user + [@unit.active_projects.first.student, tutor, outsider].each do |user| + add_auth_header_for(user: user) + put_json @endpoint, { task_def: { resubmission_extensions_enabled: false } } + assert_equal 403, last_response.status, "Unexpected result for #{user.id}: #{last_response.body}" + assert @definition.reload.resubmission_extensions_enabled + assert_nil @definition.resubmission_extensions_changed_by_id + end + end + + test 'project load and task refresh expose the same canonical deadline metadata' do + @unit.update!(allow_flexible_dates: false, extension_weeks_on_resubmit_request: 1) + @definition.update!(start_date: Time.current - 2.weeks, target_date: Time.current - 2.days, + due_date: Time.current + 4.weeks, target_grade: 0) + project = @unit.active_projects.first + task = project.task_for_task_definition(@definition) + task.assess(TaskStatus.fix_and_resubmit, @unit.main_convenor_user) + task.reload + add_auth_header_for(user: project.student) + + get "/api/projects/#{project.id}" + assert_equal 200, last_response.status, last_response.body + row = last_response_body.fetch('tasks').find { |item| item['id'] == task.id } + assert_equal task.effective_deadline_date.iso8601, row['effective_deadline_date'] + assert_equal 'post_feedback_extension', row['effective_deadline_reason'] + assert_equal task.resubmission_extension_comment.id, row['effective_deadline_source_id'] + + get "/api/projects/#{project.id}/refresh_tasks/#{@definition.id}" + assert_equal 200, last_response.status, last_response.body + assert_equal row['effective_deadline_date'], last_response_body['effective_deadline_date'] + assert_equal row['effective_deadline_reason'], last_response_body['effective_deadline_reason'] + assert_equal row['effective_deadline_source_id'], last_response_body['effective_deadline_source_id'] + end + +end diff --git a/test/api/submission/portfolio_api_test.rb b/test/api/submission/portfolio_api_test.rb new file mode 100644 index 0000000000..9b7478f480 --- /dev/null +++ b/test/api/submission/portfolio_api_test.rb @@ -0,0 +1,52 @@ +# frozen_string_literal: true + +require 'test_helper' + +# PR-FILE-05 – Portfolio upload size limit +# +# The portfolio upload endpoint (POST /api/submission/project/:id/portfolio) +# previously enforced no file size limit at all. This test confirms the fix: +# a part exceeding Doubtfire::Application.config.max_file_size is rejected +# with 413, and confirms the rejected file is never copied into the +# project's portfolio directory (the status code alone does not prove that). +class PortfolioApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + + def with_tempfile(extension, content = 'dummy content') + Tempfile.create(['portfolio_size_test', extension]) do |f| + f.write(content) + f.flush + yield f + end + end + + test 'rejects portfolio part exceeding the configured max_file_size and stores nothing' do + original_max = Doubtfire::Application.config.max_file_size + Doubtfire::Application.config.max_file_size = 1_024 # 1 KB + + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + + add_auth_header_for(user: project.student) + + files_before = project.portfolio_files + + with_tempfile('.py', 'x' * 2_048) do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'text/plain', true) + post "/api/submission/project/#{project.id}/portfolio", + name: 'OversizedPart', + kind: 'code', + file0: uploaded + end + + assert_equal 413, last_response.status, + "Expected 413 for a portfolio part exceeding max_file_size, got: #{last_response.body}" + assert_match(/exceeds the \d+MB file limit/i, last_response.body) + assert_equal files_before, project.portfolio_files, + 'Rejected oversized portfolio upload must not add any file to the portfolio directory' + ensure + unit.destroy + Doubtfire::Application.config.max_file_size = original_max + end +end \ No newline at end of file diff --git a/test/api/submission/submission_processing_api_test.rb b/test/api/submission/submission_processing_api_test.rb new file mode 100644 index 0000000000..cf7a6b01bb --- /dev/null +++ b/test/api/submission/submission_processing_api_test.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +require 'test_helper' + +# POST /api/projects/:id/task_def_id/:task_definition_id/submission/retry +class SubmissionProcessingApiTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def retry_endpoint(project, task_definition) + "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/submission/retry" + end + + # The retry restores the done archive, so the task needs one on disk. + def write_done_archive(task, archive_task = task) + zip_path = task.zip_file_path_for_done_task + FileUtils.mkdir_p(File.dirname(zip_path)) + Zip::File.open(zip_path, Zip::File::CREATE) do |zip| + zip.get_output_stream("#{archive_task.id}/000-document.pdf") { |stream| stream.write('archived') } + end + zip_path + end + + def record_enqueued_jobs(jobs, &) + enqueue = lambda do |*args| + jobs << args + 'job-id' + end + AcceptSubmissionJob.stub(:perform_async, enqueue, &) + end + + test 'retrying a failed conversion queues the preserved archive and reports the new state' do + project = FactoryBot.create(:project) + task_definition = project.unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + task.update!( + submission_date: 1.hour.ago, + submission_processing_state: 'failed', + submission_processing_error_code: 'conversion_failed', + submission_processing_attempts: 1 + ) + zip_path = write_done_archive(task) + jobs = [] + + add_auth_header_for(user: project.student) + record_enqueued_jobs(jobs) do + post retry_endpoint(project, task_definition) + end + + assert_equal 201, last_response.status, last_response.body + assert_equal 'queued', last_response_body['processing_state'] + assert_equal 2, last_response_body['processing_attempts'] + assert_equal true, last_response_body['processing_pdf'] + # No uploader was recorded for this attempt, so the caller is used. + assert_equal [[task.id, project.student.id, false, false, 'retry_archive', 2]], jobs + ensure + FileUtils.rm_f(zip_path) if zip_path + end + + test 'a group retry reports the state written through the submitter task' do + unit = FactoryBot.create( + :unit, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 0 + ) + task_definition = FactoryBot.create(:task_definition, unit: unit, group_set: unit.group_sets.first) + group = unit.groups.first + submitter_project, member_project = group.projects.first(2) + submitter = submitter_project.task_for_task_definition(task_definition) + member = member_project.task_for_task_definition(task_definition) + group_submission = GroupSubmission.create!( + group: group, + task_definition: task_definition, + submitted_by_project: submitter_project + ) + [submitter, member].each do |task| + task.update!( + group_submission: group_submission, + submission_date: 1.hour.ago, + submission_processing_state: 'failed', + submission_processing_attempts: 1 + ) + end + zip_path = write_done_archive(submitter.reload) + jobs = [] + + # The member asks, the submitter's task is the one that is processed. + add_auth_header_for(user: member_project.student) + record_enqueued_jobs(jobs) do + post retry_endpoint(member_project, task_definition) + end + + assert_equal 201, last_response.status, last_response.body + assert_equal 'queued', last_response_body['processing_state'] + assert_equal 2, last_response_body['processing_attempts'] + assert_equal submitter.id, jobs.first.first + # The job must carry the attempt that was just recorded, or it stands down. + assert_equal 2, jobs.first.last + assert_equal 'queued', member.reload.submission_processing_state + ensure + FileUtils.rm_f(zip_path) if zip_path + end + + test 'a submission that has not failed cannot be retried' do + project = FactoryBot.create(:project) + task_definition = project.unit.task_definitions.first + task = project.task_for_task_definition(task_definition) + task.update!(submission_processing_state: 'queued', submission_processing_started_at: Time.current) + jobs = [] + + add_auth_header_for(user: project.student) + record_enqueued_jobs(jobs) do + post retry_endpoint(project, task_definition) + end + + assert_equal 409, last_response.status, last_response.body + assert_empty jobs + assert_equal 'queued', task.reload.submission_processing_state + end +end diff --git a/test/api/submission_access_test.rb b/test/api/submission_access_test.rb new file mode 100644 index 0000000000..b464a86931 --- /dev/null +++ b/test/api/submission_access_test.rb @@ -0,0 +1,184 @@ +# frozen_string_literal: true + +require 'test_helper' + +# Access-control regression coverage for the submission_details and +# submission_files endpoints in tasks_api.rb. Neither endpoint currently +# has any dedicated test coverage on main, despite both serving another +# student's submission data/files behind a single `authorise?` check. +# +# These tests do not change any application behaviour - they only assert +# that the existing `authorise? current_user, project, :get_submission` +# guard actually blocks the object-reference paths it's meant to. +class SubmissionAccessTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def setup + @unit = FactoryBot.create(:unit, perform_submissions: true, student_count: 3, staff_count: 1) + @task_definition = @unit.task_definitions.first + @owning_project = @unit.projects.first + @other_project = @unit.projects.second + + @convenor = @unit.main_convenor_user + @tutor = FactoryBot.create(:user, :tutor) + @unit.employ_staff(@tutor, Role.tutor) + + @other_unit = FactoryBot.create(:unit, student_count: 1, staff_count: 1) + @other_unit_task_definition = @other_unit.task_definitions.first + end + + def details_endpoint(project: @owning_project, task_definition: @task_definition) + "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/submission_details" + end + + def files_endpoint(project: @owning_project, task_definition: @task_definition) + "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/submission_files" + end + + # --------------------------------------------------------------------- + # submission_details + # --------------------------------------------------------------------- + + def test_submission_details_allows_owning_student + add_auth_header_for(user: @owning_project.student) + + get details_endpoint + + assert_equal 200, last_response.status + assert last_response_body.key?('has_pdf') + assert last_response_body.key?('processing_pdf') + end + + # A student is not a unit_role, so the claimed_by_unit_role_id key + # (staff-only data about who has claimed the overflow task) should not + # appear in their response at all. + def test_submission_details_does_not_expose_claim_info_to_student + add_auth_header_for(user: @owning_project.student) + + get details_endpoint + + assert_equal 200, last_response.status + refute last_response_body.key?('claimed_by_unit_role_id') + end + + def test_submission_details_allows_unit_convenor + add_auth_header_for(user: @convenor) + + get details_endpoint + + assert_equal 200, last_response.status + end + + # Staff (anyone with a unit_role) should see the claim-tracking field, + # even if it is null (no overflow claim exists yet). + def test_submission_details_exposes_claim_info_to_convenor + add_auth_header_for(user: @convenor) + + get details_endpoint + + assert_equal 200, last_response.status + assert last_response_body.key?('claimed_by_unit_role_id') + end + + def test_submission_details_allows_unit_tutor + add_auth_header_for(user: @tutor) + + get details_endpoint + + assert_equal 200, last_response.status + end + + def test_submission_details_blocks_other_student_in_same_unit + add_auth_header_for(user: @other_project.student) + + get details_endpoint(project: @owning_project) + + assert_equal 403, last_response.status + assert_equal 'You do not have permission to read submissions for this project.', + last_response_body['error'] + end + + def test_submission_details_blocks_staff_from_a_different_unit + other_unit_staff = @other_unit.main_convenor_user + add_auth_header_for(user: other_unit_staff) + + get details_endpoint(project: @owning_project) + + assert_equal 403, last_response.status + end + + def test_submission_details_blocks_unauthenticated_request + header 'auth_token', nil + header 'username', nil + + get details_endpoint + + assert_equal 419, last_response.status + end + + def test_submission_details_rejects_task_definition_from_another_unit + add_auth_header_for(user: @owning_project.student) + + get details_endpoint(task_definition: @other_unit_task_definition) + + assert_equal 404, last_response.status + end + + # --------------------------------------------------------------------- + # submission_files + # --------------------------------------------------------------------- + + def test_submission_files_allows_owning_student + add_auth_header_for(user: @owning_project.student) + + get files_endpoint + + assert_equal 200, last_response.status + end + + def test_submission_files_blocks_other_student_in_same_unit + add_auth_header_for(user: @other_project.student) + + get files_endpoint(project: @owning_project) + + assert_equal 403, last_response.status + end + + def test_submission_files_blocks_staff_from_a_different_unit + other_unit_staff = @other_unit.main_convenor_user + add_auth_header_for(user: other_unit_staff) + + get files_endpoint(project: @owning_project) + + assert_equal 403, last_response.status + end + + # Regression guard: the Content-Disposition filename is built from + # project.student.username. Confirm that only ever happens for a caller + # who has already passed the authorise? check - i.e. a cross-student + # request never reaches the point where the filename (and therefore the + # other student's username) is constructed or exposed in the response. + def test_submission_files_does_not_leak_owning_students_username_to_blocked_caller + add_auth_header_for(user: @other_project.student) + + get files_endpoint(project: @owning_project) + + assert_equal 403, last_response.status + refute_match(/#{@owning_project.student.username}/, last_response.headers['Content-Disposition'].to_s) + end + + def test_submission_files_blocks_unauthenticated_request + header 'auth_token', nil + header 'username', nil + + get files_endpoint + + assert_equal 419, last_response.status + end +end \ No newline at end of file diff --git a/test/api/submission_history_access_test.rb b/test/api/submission_history_access_test.rb new file mode 100644 index 0000000000..b00e1f5316 --- /dev/null +++ b/test/api/submission_history_access_test.rb @@ -0,0 +1,388 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'stringio' +require 'zip' + +class SubmissionHistoryAccessTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::AuthHelper + include TestHelpers::JsonHelper + + def app + Rails.application + end + + def setup + @unit = FactoryBot.create( + :unit, + task_count: 1, + student_count: 3, + staff_count: 1 + ) + + @task_definition = @unit.task_definitions.first + @owning_project = @unit.active_projects.first + @other_project = @unit.active_projects.second + @convenor = @unit.main_convenor_user + + @owning_task = @owning_project.task_for_task_definition(@task_definition) + @other_task = @other_project.task_for_task_definition(@task_definition) + + @older_history = FactoryBot.create( + :submission_history, + task: @owning_task, + submission_timestamp: (Time.current.to_i - 120).to_s + ) + + @history = FactoryBot.create( + :submission_history, + task: @owning_task, + submission_timestamp: (Time.current.to_i - 60).to_s + ) + + create_archive_for(@history) + + @other_unit = FactoryBot.create( + :unit, + task_count: 1, + student_count: 1, + staff_count: 1 + ) + + @other_unit_task_definition = @other_unit.task_definitions.first + end + + def teardown + SubmissionHistory.clear_pending(@owning_task) if @owning_task + FileUtils.rm_f(@history.archive_file_name) if @history + + @other_unit&.destroy + @unit&.destroy + end + + test 'student receives only safe metadata for own history' do + add_auth_header_for(user: @owning_project.student) + + get metadata_endpoint + + assert_equal 200, last_response.status + + histories = last_response_body + assert_equal 2, histories.length + + newest = histories.first + + assert_equal @history.id, newest['id'] + assert_equal 1, newest['version_order'] + assert_equal @history.submission_timestamp, newest['submission_timestamp'] + assert_equal 'available', newest['status'] + + assert_equal( + %w[current id status submission_timestamp version_order], + newest.keys.sort + ) + + older = histories.second + assert_equal @older_history.id, older['id'] + assert_equal 2, older['version_order'] + assert_equal 'unavailable', older['status'] + end + + test 'unauthorised history requests do not create tasks on another project' do + @owning_task.destroy! + add_auth_header_for(user: @other_project.student) + + assert_no_difference('Task.count') do + get metadata_endpoint + assert_safe_not_found + get files_endpoint + assert_safe_not_found + end + end + + test 'own history requests for a missing task are read only' do + @owning_task.destroy! + add_auth_header_for(user: @owning_project.student) + + assert_no_difference('Task.count') do + get metadata_endpoint + assert_safe_not_found + get files_endpoint + assert_safe_not_found + end + end + + test 'staff history requests for a missing task are read only' do + @owning_task.destroy! + add_auth_header_for(user: @convenor) + + assert_no_difference('Task.count') do + get metadata_endpoint + assert_safe_not_found + get files_endpoint + assert_safe_not_found + end + end + + test 'student can download own retained submission history' do + add_auth_header_for(user: @owning_project.student) + + get files_endpoint + + assert_equal 200, last_response.status + assert_match(%r{application/octet-stream}, last_response.headers['Content-Type']) + assert_match(@owning_project.student.username, last_response.headers['Content-Disposition']) + assert_match(@task_definition.abbreviation, last_response.headers['Content-Disposition']) + + Zip::File.open_buffer(StringIO.new(last_response.body)) do |archive| + assert archive.find_entry("#{@owning_task.id}/000-code.rb") + end + end + + test 'staff metadata keeps the existing richer contract' do + add_auth_header_for(user: @convenor) + + get metadata_endpoint + + assert_equal 200, last_response.status + + history = last_response_body.first + + assert history.key?('id') + assert history.key?('task_id') + assert history.key?('submission_timestamp') + assert history.key?('created_at') + assert history.key?('has_submission_files') + assert history.key?('overseer_assessment_id') + assert_not history.key?('version_order') + end + + test 'staff can still download retained submission history' do + add_auth_header_for(user: @convenor) + + get files_endpoint + + assert_equal 200, last_response.status + assert_match(%r{application/octet-stream}, last_response.headers['Content-Type']) + end + + test 'student cannot read another students history metadata' do + add_auth_header_for(user: @other_project.student) + + get metadata_endpoint(project: @owning_project) + + assert_safe_not_found + end + + test 'student cannot download another students history archive' do + add_auth_header_for(user: @other_project.student) + + get files_endpoint(project: @owning_project) + + assert_safe_not_found + end + + test 'invalid project id returns the same safe response' do + add_auth_header_for(user: @owning_project.student) + + invalid_project_id = Project.maximum(:id).to_i + 100_000 + + get metadata_endpoint(project_id: invalid_project_id) + + assert_safe_not_found + end + + test 'task definition from another unit returns the same safe response' do + add_auth_header_for(user: @owning_project.student) + + get metadata_endpoint(task_definition: @other_unit_task_definition) + + assert_safe_not_found + end + + test 'substituted history id cannot escape the authorised task' do + foreign_history = FactoryBot.create( + :submission_history, + task: @other_task, + submission_timestamp: (Time.current.to_i - 300).to_s + ) + + add_auth_header_for(user: @owning_project.student) + + get files_endpoint(history: foreign_history) + + assert_safe_not_found + end + + test 'invalid history id returns the same safe response' do + add_auth_header_for(user: @owning_project.student) + + invalid_history_id = SubmissionHistory.maximum(:id).to_i + 100_000 + + get files_endpoint(history_id: invalid_history_id) + + assert_safe_not_found + end + + test 'missing retained archive is reported as unavailable' do + add_auth_header_for(user: @owning_project.student) + + get files_endpoint(history: @older_history) + + assert_equal 404, last_response.status + assert_equal( + 'Submission history files are not available', + last_response_body['error'] + ) + end + + test 'pending archive creation is exposed as processing without a fake history id' do + SubmissionHistory.mark_pending(@owning_task) + + add_auth_header_for(user: @owning_project.student) + + get metadata_endpoint + + assert_equal 202, last_response.status + assert_kind_of Array, last_response_body + + ids = last_response_body.map { |history| history['id'] } + assert_includes ids, @history.id + assert_not_includes ids, nil + ensure + SubmissionHistory.clear_pending(@owning_task) + end + + test 'joining the current group later does not grant access to older history' do + group_set = FactoryBot.create(:group_set, unit: @unit) + group = FactoryBot.create( + :group, + group_set: group_set, + tutorial: @unit.tutorials.first + ) + + group.add_member(@owning_project, notify: false) + + group_submission = GroupSubmission.create!( + group: group, + task_definition: @task_definition, + submitted_by_project: @owning_project + ) + + @owning_task.update!(group_submission: group_submission) + + historical_group_history = FactoryBot.create( + :submission_history, + task: @owning_task, + submission_timestamp: (Time.current.to_i - 600).to_s + ) + + group.add_member(@other_project, notify: false) + + assert_includes group.reload.projects, @other_project + + add_auth_header_for(user: @other_project.student) + + get files_endpoint( + project: @owning_project, + history: historical_group_history + ) + + assert_safe_not_found + end + + test 'current version is distinguished from older retained and processing versions' do + @owning_task.update_columns(submission_processing_started_at: Time.at(@history.submission_timestamp.to_i - 1)) + add_auth_header_for(user: @owning_project.student) + get metadata_endpoint + assert_equal true, last_response_body.first['current'] + assert_equal false, last_response_body.second['current'] + SubmissionHistory.mark_pending(@owning_task) + get metadata_endpoint + assert_equal 202, last_response.status + assert last_response_body.none? { |version| version['current'] } + end + + test 'corrupt retained archive is unavailable and cannot be downloaded' do + File.binwrite(@history.archive_file_name, 'not a zip archive') + add_auth_header_for(user: @owning_project.student) + get metadata_endpoint + assert_equal 'unavailable', last_response_body.first['status'] + get files_endpoint + assert_equal 404, last_response.status + end + + test 'removed records and no history return an empty list without a fake version' do + @owning_task.submission_histories.destroy_all + add_auth_header_for(user: @owning_project.student) + get metadata_endpoint + assert_equal 200, last_response.status + assert_equal [], last_response_body + end + + test 'leaving a group preserves access only to archives retained on the students own task' do + group_set = FactoryBot.create(:group_set, unit: @unit) + group = FactoryBot.create(:group, group_set: group_set, tutorial: @unit.tutorials.first) + group.add_member(@owning_project, notify: false) + group.add_member(@other_project, notify: false) + group_submission = GroupSubmission.create!(group: group, task_definition: @task_definition, + submitted_by_project: @owning_project) + @owning_task.update!(group_submission: group_submission) + group.remove_member(@owning_project, notify: false) + add_auth_header_for(user: @owning_project.student) + get files_endpoint + assert_equal 200, last_response.status + get metadata_endpoint(project: @other_project) + assert_safe_not_found + end + + test 'numeric timestamps order legacy and modern versions deterministically' do + legacy = FactoryBot.create(:submission_history, task: @owning_task, submission_timestamp: '999999999') + add_auth_header_for(user: @owning_project.student) + get metadata_endpoint + assert_equal [@history.id, @older_history.id, legacy.id], last_response_body.map { |version| version['id'] } + end + + private + + def metadata_endpoint( + project: @owning_project, + project_id: nil, + task_definition: @task_definition + ) + id = project_id || project.id + + "/api/projects/#{id}/task_def_id/#{task_definition.id}/submission_histories" + end + + def files_endpoint( + project: @owning_project, + task_definition: @task_definition, + history: @history, + history_id: nil + ) + id = history_id || history.id + + "/api/projects/#{project.id}/task_def_id/#{task_definition.id}/submission_histories/#{id}/files" + end + + def assert_safe_not_found + assert_equal 404, last_response.status + assert_equal 'Submission history is not available', last_response_body['error'] + end + + def create_archive_for(history) + FileUtils.mkdir_p(history.output_path) + FileUtils.rm_f(history.archive_file_name) + + Zip::File.open(history.archive_file_name, create: true) do |archive| + entry_name = + "#{history.submission_timestamp}/#{history.task.id}/000-code.rb" + + archive.get_output_stream(entry_name) do |file| + file.write('puts "retained submission"') + end + end + end +end diff --git a/test/api/test_attempts_test.rb b/test/api/test_attempts_test.rb index be0c02ae5e..c4c3a5f126 100644 --- a/test/api/test_attempts_test.rb +++ b/test/api/test_attempts_test.rb @@ -5,6 +5,21 @@ class TestAttemptsTest < ActiveSupport::TestCase include TestHelpers::AuthHelper include TestHelpers::JsonHelper + def test_legacy_attempt_with_null_score_can_create_and_update_a_scorm_comment + project = FactoryBot.create(:project) + task = project.task_for_task_definition(project.unit.task_definitions.first) + attempt = TestAttempt.create!(task_id: task.id, success_status: true, score_scaled: nil) + attempt.add_scorm_comment + comment = ScormComment.find_by!(commentable_id: attempt.id) + assert_equal 'Passed', comment.comment + attempt.update_scorm_comment + assert_equal 'Passed', comment.reload.comment + attempt.update!(score_scaled: 0) + assert_equal 'Passed', attempt.success_status_description + attempt.update!(success_status: false) + assert_equal 'Unsuccessful', attempt.success_status_description + end + def app Rails.application end @@ -495,4 +510,221 @@ def test_delete_attempt td.destroy! unit.destroy! end + + # A student may write their own scorm runtime state, but the pass or fail + # decision belongs to staff. Sending it inside the datamodel must not move it. + def test_student_cannot_pass_own_attempt_via_datamodel + unit = FactoryBot.create(:unit) + project = unit.projects.first + user = project.student + td = scorm_task_definition(unit, 'ScormPassInjection') + + task = project.task_for_task_definition(td) + attempt = TestAttempt.create({ task_id: task.id }) + + dm = JSON.parse(attempt.cmi_datamodel) + dm["cmi.completion_status"] = "completed" + dm["cmi.success_status"] = "passed" + dm["cmi.score.scaled"] = "1" + + add_auth_header_for(user: user) + + patch "api/test_attempts/#{attempt.id}", { cmi_datamodel: dm.to_json } + assert_equal 200, last_response.status + + attempt = TestAttempt.find(attempt.id) + + # Completion is the student's own progress, so it still lands. + assert_equal true, attempt.completion_status + # The pass and the score are not, so both columns keep their defaults. The + # score is pinned to 0.0 rather than "not 1.0" so a partial score leaking + # through would fail here too. + assert_equal false, attempt.success_status + assert_equal 0.0, attempt.score_scaled + + td.destroy! + unit.destroy! + end + + # The ordinary case. Completion, resume and the interactions counter are the + # student's to write and none of them are affected by the change above. + def test_student_can_record_ordinary_progress_and_resume + unit = FactoryBot.create(:unit) + project = unit.projects.first + user = project.student + td = scorm_task_definition(unit, 'ScormProgress') + + task = project.task_for_task_definition(td) + attempt = TestAttempt.create({ task_id: task.id }) + + dm = JSON.parse(attempt.cmi_datamodel) + dm["cmi.completion_status"] = "incomplete" + dm["cmi.interactions._count"] = "3" + + add_auth_header_for(user: user) + + patch "api/test_attempts/#{attempt.id}", { cmi_datamodel: dm.to_json } + assert_equal 200, last_response.status + + attempt = TestAttempt.find(attempt.id) + saved = JSON.parse(attempt.cmi_datamodel) + + assert_equal "resume", saved["cmi.entry"] + assert_equal "3", saved["cmi.interactions._count"] + assert_equal false, attempt.completion_status + assert_equal false, attempt.terminated + + saved["cmi.completion_status"] = "completed" + + add_auth_header_for(user: user) + + patch "api/test_attempts/#{attempt.id}", { cmi_datamodel: saved.to_json, terminated: true } + assert_equal 200, last_response.status + + attempt = TestAttempt.find(attempt.id) + + assert_equal true, attempt.completion_status + assert_equal true, attempt.terminated + + td.destroy! + unit.destroy! + end + + # The staff path is untouched. It writes success_status directly and never + # goes through the datamodel setter. + def test_tutor_can_still_override_success_status + unit = FactoryBot.create(:unit) + project = unit.projects.first + user = project.student + td = scorm_task_definition(unit, 'ScormTutorOverride') + tutor = project.tutor_for(td) + + task = project.task_for_task_definition(td) + attempt = TestAttempt.create({ task_id: task.id }) + + dm = JSON.parse(attempt.cmi_datamodel) + dm["cmi.completion_status"] = "completed" + + add_auth_header_for(user: user) + + patch "api/test_attempts/#{attempt.id}", { cmi_datamodel: dm.to_json, terminated: true } + assert_equal 200, last_response.status + + add_auth_header_for(user: tutor) + + patch "api/test_attempts/#{attempt.id}", { success_status: true } + assert_equal 200, last_response.status + + attempt = TestAttempt.find(attempt.id) + + assert_equal true, attempt.success_status + assert_equal "passed", JSON.parse(attempt.cmi_datamodel)["cmi.success_status"] + + td.destroy! + unit.destroy! + end + + # The check that was already there on the route still stands. + def test_student_cannot_send_success_status_directly + unit = FactoryBot.create(:unit) + project = unit.projects.first + user = project.student + td = scorm_task_definition(unit, 'ScormDirectOverride') + + task = project.task_for_task_definition(td) + attempt = TestAttempt.create({ task_id: task.id }) + + add_auth_header_for(user: user) + + patch "api/test_attempts/#{attempt.id}", { success_status: true } + assert_equal 403, last_response.status + + attempt = TestAttempt.find(attempt.id) + + assert_equal false, attempt.success_status + + td.destroy! + unit.destroy! + end + + # The other half of the change, written down so it is not read later as a + # regression. A student whose package genuinely reports a pass no longer has + # that pass recorded. The attempt reads as unsuccessful, and because nothing + # reads it as a pass the student is not blocked from trying again. Staff + # recording it is the only path to a pass. + def test_legitimate_pass_is_only_recorded_by_staff + unit = FactoryBot.create(:unit) + project = unit.projects.first + user = project.student + td = scorm_task_definition(unit, 'ScormLegitimatePass') + tutor = project.tutor_for(td) + + task = project.task_for_task_definition(td) + attempt = TestAttempt.create({ task_id: task.id }) + + dm = JSON.parse(attempt.cmi_datamodel) + dm["cmi.completion_status"] = "completed" + dm["cmi.success_status"] = "passed" + dm["cmi.score.scaled"] = "1" + + add_auth_header_for(user: user) + + patch "api/test_attempts/#{attempt.id}", { cmi_datamodel: dm.to_json, terminated: true } + assert_equal 200, last_response.status + + attempt = TestAttempt.find(attempt.id) + + assert_equal true, attempt.completion_status + assert_equal false, attempt.success_status + assert_equal 0.0, attempt.score_scaled + + # The comment both the student and the tutor read on the task. + assert_equal "Unsuccessful", attempt.scorm_comment.comment + + # The attempt gate reads success_status, so it does not close on the student. + add_auth_header_for(user: user) + + post "api/projects/#{project.id}/task_def_id/#{td.id}/test_attempts" + assert_equal 201, last_response.status + + # And the staff override is still the way the pass gets recorded. + add_auth_header_for(user: tutor) + + patch "api/test_attempts/#{attempt.id}", { success_status: true } + assert_equal 200, last_response.status + + assert_equal true, TestAttempt.find(attempt.id).success_status + + td.destroy! + unit.destroy! + end + + # A scorm enabled task definition, with the settings the other tests in this + # file already use. Not marked private, because a private keyword here would + # silently stop minitest collecting any test method appended below it. + def scorm_task_definition(unit, abbreviation) + td = TaskDefinition.new( + { + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: "Test attempts #{abbreviation}", + description: 'Test attempts', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now - 1.week, + due_date: Time.zone.now + 1.week, + abbreviation: abbreviation, + restrict_status_updates: false, + upload_requirements: [], + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0, + scorm_enabled: true, + scorm_attempt_limit: 0 + } + ) + td.save! + td + end end diff --git a/test/api/upload_security_test.rb b/test/api/upload_security_test.rb new file mode 100644 index 0000000000..0f572f177c --- /dev/null +++ b/test/api/upload_security_test.rb @@ -0,0 +1,1013 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'zip' + +# FILE-S01 – Upload Authorisation and Abuse Tests +# +# Exercises the FILE-S01 controls that can be verified deterministically in the +# API test environment. The threat model and findings disposition in +# docs/security/ describe the covered controls and the deliberately open gaps. +# +# 1. Direct API upload without using the frontend +# 2. Access to another student's or project's attachment +# 3. Misleading extensions and mismatched MIME types +# 4. File-signature mismatch where the policy uses signature checks +# 5. Empty, oversized, malformed, and unsupported files +# 6. Path traversal, control characters, and unusual Unicode filenames +# 7. Download headers and active-content rendering behaviour +# 8. Macro-enabled documents, archives, encrypted files +# 9. Sequential duplicate upload and archive resource-exhaustion controls +# 10. Cleanup before staging for rejected uploads + +class UploadSecurityTest < ActiveSupport::TestCase + include Rack::Test::Methods + include TestHelpers::TestFileHelper + include TestHelpers::AuthHelper + + # ───────────────────────────────────────────────────────────────────────────── + # Helpers + # ───────────────────────────────────────────────────────────────────────────── + + # Build a minimal TaskDefinition with configurable upload requirements. + def create_task_definition(unit:, upload_requirements: [{ 'key' => 'file0', 'name' => 'Submission', 'type' => 'code' }]) + TaskDefinition.create!( + unit_id: unit.id, + tutorial_stream: unit.tutorial_streams.first, + name: 'Security Test Task', + description: 'Security Test Task', + weighting: 4, + target_grade: 0, + start_date: Time.zone.now - 2.weeks, + target_date: Time.zone.now + 1.week, + abbreviation: "SecTask#{SecureRandom.hex(4)}", + restrict_status_updates: false, + upload_requirements: upload_requirements, + plagiarism_warn_pct: 0.8, + is_graded: false, + max_quality_pts: 0 + ) + end + + # Create a Tempfile with given content and extension, yield it, then clean up. + def with_tempfile(extension, content = 'dummy content', binary: false) + Tempfile.create(['sec_test', extension]) do |f| + f.binmode if binary + f.write(content) + f.flush + yield f + end + end + + # Post a submission to the API with an arbitrary Rack::Test::UploadedFile. + # Uses the same hash structure that scoop_files expects (file is a Hash with + # :filename, :type, :name, :tempfile keys via Rack multipart parsing). + def post_submission(project, task_def, uploaded_file, trigger: 'ready_for_feedback') + data = { trigger: trigger, file0: uploaded_file } + post "/api/projects/#{project.id}/task_def_id/#{task_def.id}/submission", data + end + + def upload_storage_entries + roots = [ + File.join(Dir.tmpdir, 'doubtfire', 'new'), + FileHelper.student_work_dir(:new, nil, false), + FileHelper.student_work_dir(:in_process, nil, false) + ] + + roots.flat_map do |root| + next [] unless Dir.exist?(root) + + Dir.glob(File.join(root, '**', '*')) + end.sort + end + + def capture_rails_logs(level: Logger::DEBUG) + output = StringIO.new + test_logger = Logger.new(output) + test_logger.level = level + original_logger = Rails.logger + Rails.logger = test_logger + + yield + output.string + ensure + Rails.logger = original_logger if defined?(original_logger) && original_logger + end + + # ───────────────────────────────────────────────────────────────────────────── + # 1. Direct API upload without using the frontend + # The backend must enforce authentication and authorisation regardless of + # whether a frontend-originated cookie/CSRF token is present. + # ───────────────────────────────────────────────────────────────────────────── + + test 'unauthenticated direct API upload is rejected with 419' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + + # No auth header – simulate a raw API call with no session at all. + with_tempfile('.py', "print('hello')") do |f| + post_submission(project, td, Rack::Test::UploadedFile.new(f.path, 'text/plain')) + end + + assert_equal 419, last_response.status, + 'Expected 419 (authentication required) for unauthenticated direct API upload' + ensure + unit.destroy + end + + test 'authenticated direct API upload succeeds for own project' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + + add_auth_header_for(user: project.student) + + with_tempfile('.py', "print('hello')") do |f| + post_submission(project, td, Rack::Test::UploadedFile.new(f.path, 'text/plain')) + end + + assert_equal 201, last_response.status, + 'Expected 201 for a valid authenticated direct API upload' + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 2. Access to another student's or project's attachment + # A student must not be able to submit on behalf of another project, nor + # download another student's submission PDF. + # ───────────────────────────────────────────────────────────────────────────── + + test 'student cannot submit to another student\'s project' do + unit = FactoryBot.create(:unit, student_count: 2, task_count: 0) + projects = unit.active_projects + project_a = projects.first + project_b = projects.second + td = create_task_definition(unit: unit) + + # Authenticate as student A but post to project B's endpoint. + add_auth_header_for(user: project_a.student) + + side_effects_before = { + tasks: Task.count, + submissions: TaskSubmission.count, + jobs: AcceptSubmissionJob.jobs.size, + storage: upload_storage_entries + } + + with_tempfile('.py', "print('owned')") do |f| + post_submission(project_b, td, Rack::Test::UploadedFile.new(f.path, 'text/plain')) + end + + assert_equal 401, last_response.status, + 'Expected the current submission API contract to return 401 for a cross-project POST' + assert_match(/not authorised to submit task/i, last_response.body) + assert_equal side_effects_before[:tasks], Task.count, + 'Rejected cross-project POST must not create a task' + assert_equal side_effects_before[:submissions], TaskSubmission.count, + 'Rejected cross-project POST must not create a submission row' + assert_equal side_effects_before[:jobs], AcceptSubmissionJob.jobs.size, + 'Rejected cross-project POST must not enqueue submission processing' + assert_equal side_effects_before[:storage], upload_storage_entries, + 'Rejected cross-project POST must not write submission files' + ensure + unit.destroy + end + + test 'student cannot download another student\'s submission PDF' do + unit = FactoryBot.create(:unit, student_count: 2, task_count: 0) + projects = unit.active_projects + project_a = projects.first + project_b = projects.second + td = create_task_definition(unit: unit) + + # Authenticate as student B and attempt to fetch project A's submission. + add_auth_header_for(user: project_b.student) + + get "/api/projects/#{project_a.id}/task_def_id/#{td.id}/submission" + + assert_equal 401, last_response.status, + 'Expected the current submission API contract to return 401 for a cross-project GET' + assert_match(/not authorised to get task/i, last_response.body) + ensure + unit.destroy + end + + test 'student cannot access another student\'s submission history' do + unit = FactoryBot.create(:unit, student_count: 2, task_count: 0) + projects = unit.active_projects + project_a = projects.first + project_b = projects.second + td = create_task_definition(unit: unit) + + add_auth_header_for(user: project_b.student) + + get "/api/projects/#{project_a.id}/task_def_id/#{td.id}/submission_histories" + + assert_equal 404, last_response.status, + 'Cross-student history access should fail closed' + assert_match(/Submission history is not available/, last_response.body) + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 3. Misleading extensions and mismatched MIME types + # A file whose extension says .pdf but whose content (MIME) is something + # else must be rejected by the server-side MIME sniff. + # ───────────────────────────────────────────────────────────────────────────── + + test 'rejects file with PDF extension but plain-text content' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition( + unit: unit, + upload_requirements: [{ 'key' => 'file0', 'name' => 'Report', 'type' => 'document' }] + ) + + add_auth_header_for(user: project.student) + + # Actual content is plain text, but we claim .pdf and application/pdf. + with_tempfile('.pdf', 'This is not a PDF at all') do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'application/pdf', true) + post_submission(project, td, uploaded) + end + + assert_equal 403, last_response.status, + 'Expected MIME validation to reject PDF extension with non-PDF content' + assert_match(/invalid file MIME type/i, last_response.body) + ensure + unit.destroy + end + + test 'rejects executable disguised with .txt extension' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + + add_auth_header_for(user: project.student) + + # ELF magic bytes – a Linux executable masquerading as a text file. + elf_magic = "\x7fELF\x02\x01\x01\x00#{"\x00" * 8}" + with_tempfile('.txt', elf_magic, binary: true) do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'text/plain', true) + post_submission(project, td, uploaded) + end + + assert_equal 403, last_response.status, + 'Expected MIME validation to reject ELF binary with .txt extension' + assert_match(/invalid file MIME type/i, last_response.body) + ensure + unit.destroy + end + + test 'rejects PHP script with image extension' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition( + unit: unit, + upload_requirements: [{ 'key' => 'file0', 'name' => 'Image', 'type' => 'image' }] + ) + + add_auth_header_for(user: project.student) + + php_payload = '' + with_tempfile('.jpg', php_payload) do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'image/jpeg', true) + post_submission(project, td, uploaded) + end + + assert_equal 403, last_response.status, + 'Expected MIME validation to reject PHP payload with .jpg extension' + assert_match(/invalid file MIME type/i, last_response.body) + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 4. File-signature mismatch where the policy uses signature checks + # FileHelper uses FileMagic (libmagic) to detect the actual MIME type. + # Files whose magic bytes disagree with the declared type must be rejected. + # ───────────────────────────────────────────────────────────────────────────── + + test 'accept_file rejects file whose magic bytes mismatch the kind' do + # Use FileHelper directly to confirm the signature check, independent of + # the API layer. + result = with_tempfile('.pdf', "PK\x03\x04rest of zip", binary: true) do |f| + FileHelper.accept_file( + { filename: 'report.pdf', 'tempfile' => f }, + 'Report', + 'document' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject a file whose magic bytes are ZIP but kind is document' + assert_includes result[:msg].downcase, 'mime', + 'Expected rejection message to mention MIME type mismatch' + end + + test 'accept_file rejects HTML file presented as an image' do + html_content = '' + result = with_tempfile('.png', html_content) do |f| + FileHelper.accept_file( + { filename: 'photo.png', 'tempfile' => f }, + 'Photo', + 'image' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject HTML content submitted as an image' + end + + # ───────────────────────────────────────────────────────────────────────────── + # 5. Empty, oversized, malformed, and unsupported files + # ───────────────────────────────────────────────────────────────────────────── + + test 'empty file is rejected by MIME validation' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + + add_auth_header_for(user: project.student) + + with_tempfile('.py', '') do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'text/plain', true) + post_submission(project, td, uploaded) + end + + assert_equal 403, last_response.status, + "Expected MIME validation to reject an empty file, got: #{last_response.body}" + assert_match(/invalid file MIME type/i, last_response.body) + ensure + unit.destroy + end + + test 'rejects file exceeding the configured max_file_size' do + original_max = Doubtfire::Application.config.max_file_size + Doubtfire::Application.config.max_file_size = 1_024 # 1 KB + + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + + add_auth_header_for(user: project.student) + + with_tempfile('.py', 'x' * 2_048) do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'text/plain', true) + post_submission(project, td, uploaded) + end + + assert_equal 403, last_response.status, + 'Expected upload validation to reject a file exceeding max_file_size' + assert_match(/exceeds the \d+MB file limit/i, last_response.body) + ensure + unit.destroy + Doubtfire::Application.config.max_file_size = original_max + end + + test 'rejects malformed / corrupted PDF' do + result = File.open(Rails.root.join('test_files/submissions/corrupted.pdf')) do |f| + FileHelper.accept_file( + { filename: 'corrupted.pdf', 'tempfile' => f }, + 'Report', + 'document' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject a corrupted PDF' + assert_match(/corrupt/i, result[:msg]) + end + + test 'rejects unsupported file extension' do + result = with_tempfile('.exe', "MZ#{"\x90" * 10}", binary: true) do |f| + FileHelper.accept_file( + { filename: 'malware.exe', 'tempfile' => f }, + 'Code', + 'code' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject an .exe file' + assert_includes result[:msg].downcase, 'extension' + end + + test 'rejects malformed zip file' do + result = Tempfile.create(['bad', '.zip']) do |f| + f.write('this is not a zip file at all') + f.flush + FileHelper.accept_file( + { filename: 'submission.zip', 'tempfile' => f }, + 'Archive', + 'zip' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject a malformed zip file' + end + + # ───────────────────────────────────────────────────────────────────────────── + # 6. Path traversal, control characters, and unusual Unicode filenames + # ───────────────────────────────────────────────────────────────────────────── + + test 'rejects zip containing path traversal entry' do + Tempfile.create(['traversal', '.zip']) do |zip_file| + Zip::File.open(zip_file.path, Zip::File::CREATE) do |zip| + zip.get_output_stream('../../../etc/passwd') { |io| io.write('root:x:0:0') } + end + + result = FileHelper.accept_file( + { filename: 'submission.zip', 'tempfile' => zip_file }, + 'Archive', + 'zip' + ) + + assert_not result[:accepted], + 'Expected rejection for zip with path traversal entry' + assert_match(/unsafe path/i, result[:msg]) + end + end + + test 'sanitized_filename strips path separators and control characters' do + dangerous_names = [ + "../../../etc/passwd", + "..\\..\\windows\\system32\\cmd.exe", + "file\x00name.txt", # null byte + "file\x01name.txt", # SOH control char + "file\nname.txt", # newline + "file\rname.txt" # carriage return + ] + + dangerous_names.each do |name| + sanitized = FileHelper.sanitized_filename(name) + + assert_not_includes sanitized, '..', "sanitized_filename should remove '..' from '#{name}'" + assert_not_includes sanitized, '/', "sanitized_filename should remove '/' from '#{name}'" + assert_not_includes sanitized, '\\', "sanitized_filename should remove backslash from '#{name}'" + assert_not_includes sanitized, "\x00", "sanitized_filename should remove null byte from '#{name}'" + # Control characters (ASCII 0-31) should be stripped. + assert_equal sanitized, sanitized.gsub(/[[:cntrl:]]/, ''), + "sanitized_filename should remove control characters from '#{name}'" + end + end + + test 'sanitized_path does not allow traversal outside base directory' do + traversal_paths = [ + ['../secret', 'data'], + ['../../etc', 'passwd'], + ['valid', '../escape'] + ] + + traversal_paths.each do |parts| + result = FileHelper.sanitized_path(*parts) + assert_no_match(/\.\./, result, + "sanitized_path should not contain '..' for input #{parts.inspect}") + end + end + + test 'submission is accepted with a valid Unicode filename' do + # Unicode filenames that are unusual but legitimate should not crash the + # system, and accepted files should be stored safely. + unicode_name = "提出物_\u4E2D\u6587_file.py" + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + + add_auth_header_for(user: project.student) + + with_tempfile('.py', "print('hello')") do |f| + uploaded = Rack::Test::UploadedFile.new(f.path, 'text/plain', false, original_filename: unicode_name) + post_submission(project, td, uploaded) + end + + assert_equal 201, last_response.status, + "Expected a valid Unicode filename to be accepted, got: #{last_response.body}" + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 7. Download headers and active-content rendering behaviour + # Submission PDFs must be served with Content-Disposition: attachment and a + # safe Content-Type so browsers do not execute them inline. + # ───────────────────────────────────────────────────────────────────────────── + + test 'submission download is served as attachment not inline' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition( + unit: unit, + upload_requirements: [{ 'key' => 'file0', 'name' => 'Report', 'type' => 'document' }] + ) + + add_auth_header_for(user: project.student) + + data = with_file('test_files/submissions/valid.pdf', 'application/pdf', + { trigger: 'ready_for_feedback' }) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data + assert_equal 201, last_response.status, last_response.body + + get "/api/projects/#{project.id}/task_def_id/#{td.id}/submission?as_attachment=true" + + content_disp = last_response.headers['Content-Disposition'].to_s + assert_match(/attachment/i, content_disp, + 'Submission download should use Content-Disposition: attachment when requested') + ensure + unit.destroy + end + + test 'submission endpoint returns application/pdf content type' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition( + unit: unit, + upload_requirements: [{ 'key' => 'file0', 'name' => 'Report', 'type' => 'document' }] + ) + + add_auth_header_for(user: project.student) + + data = with_file('test_files/submissions/valid.pdf', 'application/pdf', + { trigger: 'ready_for_feedback' }) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data + assert_equal 201, last_response.status, last_response.body + + get "/api/projects/#{project.id}/task_def_id/#{td.id}/submission" + + content_type = last_response.headers['Content-Type'].to_s + assert_match(%r{application/pdf}, content_type, + 'Submission GET should return application/pdf, not text/html or similar') + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 8. Macro-enabled documents, archives, and encrypted files + # ───────────────────────────────────────────────────────────────────────────── + + test 'rejects encrypted PDF' do + result = File.open(Rails.root.join('test_files/submissions/encrypted.pdf')) do |f| + FileHelper.accept_file( + { filename: 'encrypted.pdf', 'tempfile' => f }, + 'Report', + 'document' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject an encrypted PDF' + assert_match(/encrypt/i, result[:msg]) + end + + test 'rejects unsupported Word document extension' do + # Production accepts PDF only for document uploads. DOCX is not a known + # extension and no conversion path runs from FileHelper.accept_file. + with_tempfile('.docx', "PK\x03\x04fake docx content", binary: true) do |f| + result = FileHelper.accept_file( + { filename: 'report.docx', 'tempfile' => f }, + 'Report', + 'document' + ) + + assert_not result[:accepted], 'Expected DOCX to be rejected for document uploads' + assert_equal 'invalid file extension.', result[:msg] + end + end + + test 'rejects zip containing nested archive' do + Tempfile.create(['nested', '.zip']) do |zip_file| + Zip::File.open(zip_file.path, Zip::File::CREATE) do |zip| + zip.get_output_stream('src/vendor.zip') { |io| io.write("PK#{"\x00" * 10}") } + end + + result = FileHelper.accept_file( + { filename: 'submission.zip', 'tempfile' => zip_file }, + 'Archive', + 'zip' + ) + + assert_not result[:accepted], + 'Expected rejection for zip containing a nested archive' + assert_match(/nested/i, result[:msg]) + end + end + + test 'rejects .xlsm (macro-enabled Excel) file submitted as a document' do + # .xlsm is not in the allowed extension list for 'document' kind. + result = with_tempfile('.xlsm', "PK\x03\x04fake xlsm", binary: true) do |f| + FileHelper.accept_file( + { filename: 'macro_sheet.xlsm', 'tempfile' => f }, + 'Spreadsheet', + 'document' + ) + end + + assert_not result[:accepted], + 'Expected accept_file to reject a macro-enabled spreadsheet as a document' + end + + # ───────────────────────────────────────────────────────────────────────────── + # 9. Sequential duplicate-upload / storage-exhaustion controls + # The zip abuse defences limit per-archive resource use. The API also + # rejects a later duplicate while the first accepted upload is queued. + # A true simultaneous race requires a separate multi-connection test. + # ───────────────────────────────────────────────────────────────────────────── + + test 'zip compression-ratio limit is enforced' do + # A zip that compresses highly repeated data is a potential zip bomb. + original_max = Doubtfire::Application.config.max_file_size + original_ratio = Doubtfire::Application.config.zip_compression_ratio_limit + Doubtfire::Application.config.max_file_size = 100_000_000 + Doubtfire::Application.config.zip_compression_ratio_limit = 5 + + Tempfile.create(['bomb', '.zip']) do |zip_file| + Zip::File.open(zip_file.path, Zip::File::CREATE) do |zip| + # Write 1 MB of all-zeroes – compresses to ~1 KB, ratio >> 5. + zip.get_output_stream('zeros.txt') { |io| io.write("\x00" * 1_000_000) } + end + + result = FileHelper.validate_zip_upload(zip_file.path, 'bomb.zip') + + assert_not result[:valid], + 'Expected zip with extreme compression ratio to be rejected' + assert_match(/ratio/i, result[:msg]) + end + ensure + Doubtfire::Application.config.max_file_size = original_max + Doubtfire::Application.config.zip_compression_ratio_limit = original_ratio + end + + test 'zip entry count limit is enforced' do + original_limit = Doubtfire::Application.config.zip_entry_limit + Doubtfire::Application.config.zip_entry_limit = 3 + + Tempfile.create(['manyfiles', '.zip']) do |zip_file| + Zip::File.open(zip_file.path, Zip::File::CREATE) do |zip| + 5.times { |i| zip.get_output_stream("file_#{i}.txt") { |io| io.write('x') } } + end + + result = FileHelper.validate_zip_upload(zip_file.path, 'manyfiles.zip') + + assert_not result[:valid], + 'Expected zip with too many entries to be rejected' + assert_match(/too many files/i, result[:msg]) + end + ensure + Doubtfire::Application.config.zip_entry_limit = original_limit + end + + test 'total uncompressed size limit is enforced across multiple files in zip' do + original_max = Doubtfire::Application.config.max_file_size + original_multiplier = Doubtfire::Application.config.zip_uncompressed_size_multiplier + Doubtfire::Application.config.max_file_size = 1_000 + Doubtfire::Application.config.zip_uncompressed_size_multiplier = 2 + + Tempfile.create(['bigzip', '.zip']) do |zip_file| + Zip::File.open(zip_file.path, Zip::File::CREATE) do |zip| + 3.times { |i| zip.get_output_stream("part_#{i}.txt") { |io| io.write('a' * 900) } } + end + + result = FileHelper.validate_zip_upload(zip_file.path, 'bigzip.zip') + + assert_not result[:valid], + 'Expected rejection when combined uncompressed zip size exceeds limit' + assert_match(/uncompressed size limit/i, result[:msg]) + end + ensure + Doubtfire::Application.config.max_file_size = original_max + Doubtfire::Application.config.zip_uncompressed_size_multiplier = original_multiplier + end + + test 'sequential duplicate upload is blocked while first submission is queued' do + # This deliberately exercises a later request, not a simultaneous race. The + # first request leaves its payload in :new; the second must be rejected + # without changing state, storing another payload, or enqueuing another job. + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + task = project.task_for_task_definition(td) + + add_auth_header_for(user: project.student) + + jobs_before = AcceptSubmissionJob.jobs.size + data = with_file('test_files/submissions/normal.py', 'text/plain', + { trigger: 'ready_for_feedback' }) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data + assert_equal 201, last_response.status, + "First upload should succeed (got: #{last_response.body})" + assert_equal jobs_before + 1, AcceptSubmissionJob.jobs.size, + 'First upload must enqueue exactly one processing job' + assert_equal :ready_for_feedback, task.reload.status, + 'First upload must perform the requested state transition' + + queued_dir = FileHelper.student_work_dir(:new, task, false) + payloads_after_first = Dir.glob(File.join(queued_dir, '*')).select { |path| File.file?(path) } + assert_equal 1, payloads_after_first.size, + 'First upload must leave exactly one payload queued for processing' + + first_submission_count = TaskSubmission.where(task: task).count + first_submission_date = task.submission_date + jobs_after_first = AcceptSubmissionJob.jobs.size + + data2 = with_file('test_files/submissions/normal.py', 'text/plain', + { trigger: 'need_help' }) + post "/api/projects/#{project.id}/task_def_id/#{td.id}/submission", data2 + + assert_equal 403, last_response.status, + 'Second upload while processing should be blocked with 403' + assert_match(/already being processed/i, last_response.body, + 'Response should explain the submission is already being processed') + assert_equal jobs_after_first, AcceptSubmissionJob.jobs.size, + 'Rejected duplicate must not enqueue another processing job' + assert_equal payloads_after_first, Dir.glob(File.join(queued_dir, '*')).select { |path| File.file?(path) }, + 'Rejected duplicate must not add or replace queued payloads' + assert_equal first_submission_count, TaskSubmission.where(task: task).count, + 'Rejected duplicate must not add a submission row' + assert_equal :ready_for_feedback, task.reload.status, + 'Rejected duplicate must not change the accepted submission state' + assert_equal first_submission_date, task.submission_date, + 'Rejected duplicate must not change the accepted submission timestamp' + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 10. Cleanup before staging for rejected uploads + # Early validation failures must not create task-owned staging paths. + # Post-staging failure and abandoned-worker cleanup remain open findings. + # ───────────────────────────────────────────────────────────────────────────── + + test 'early MIME rejection creates no task-owned staging artifacts' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + task = project.task_for_task_definition(td) + + add_auth_header_for(user: project.student) + + owned_staging_paths = [ + File.join(Dir.tmpdir, 'doubtfire', 'new', task.id.to_s), + FileHelper.student_work_dir(:new, task, false), + FileHelper.student_work_dir(:in_process, task, false) + ] + assert owned_staging_paths.none? { |path| File.exist?(path) }, + 'Fresh task must not already have submission staging paths' + + jobs_before = AcceptSubmissionJob.jobs.size + submissions_before = TaskSubmission.where(task: task).count + status_before = task.status + + elf_magic = "\x7fELF\x02\x01\x01\x00#{"\x00" * 8}" + with_tempfile('.txt', elf_magic, binary: true) do |f| + post_submission(project, td, Rack::Test::UploadedFile.new(f.path, 'text/plain', true)) + end + + # Assert the upload reached file validation and was rejected for its MIME, + # rather than passing on an unrelated authentication or processing error. + assert_equal 403, last_response.status, + "Expected MIME validation to reject the upload, got: #{last_response.body}" + assert_match(/invalid file MIME type/i, last_response.body) + assert owned_staging_paths.none? { |path| File.exist?(path) }, + 'Early rejection must not create task-owned staging files or directories' + assert_equal jobs_before, AcceptSubmissionJob.jobs.size, + 'Early rejection must not enqueue submission processing' + assert_equal submissions_before, TaskSubmission.where(task: task).count, + 'Early rejection must not create a submission row' + assert_equal status_before, task.reload.status, + 'Early rejection must not transition task state' + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 11. Logs must not contain file content, sensitive names, or unnecessary + # student information + # ───────────────────────────────────────────────────────────────────────────── + + test 'rejected submission logs a safe marker without content or student identifiers' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + student = project.student + td = create_task_definition(unit: unit) + project.task_for_task_definition(td) + + add_auth_header_for(user: student) + + sensitive_content = 'SENSITIVE_STUDENT_DATA_12345' + unsafe_filename = "rejected-#{student.username}-#{student.email}.txt" + elf_magic = "\x7fELF\x02\x01\x01\x00#{"\x00" * 8}" + + logged = capture_rails_logs do + with_tempfile('.txt', elf_magic + sensitive_content, binary: true) do |f| + uploaded = Rack::Test::UploadedFile.new( + f.path, + 'text/plain', + true, + original_filename: unsafe_filename + ) + post_submission(project, td, uploaded) + end + end + + assert_equal 403, last_response.status, + 'Rejected submission must reach and fail MIME validation' + assert_match(/invalid file MIME type/i, last_response.body) + assert_includes logged, 'File MIME check failed', + 'Expected safe validation marker proving the rejection path logged' + assert_not_includes logged, sensitive_content, + 'Rejected submission log must not include file content' + assert_not_includes logged, student.email, + 'Rejected submission log must not include student email' + assert_not_includes logged, student.username, + 'Rejected submission log must not include student username' + assert_not_includes logged, unsafe_filename, + 'Rejected submission log must not include the client filename' + ensure + unit.destroy + end + + test 'accepted submission logs safe markers without content or student identifiers' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + student = project.student + td = create_task_definition(unit: unit) + project.task_for_task_definition(td) + + add_auth_header_for(user: student) + + sensitive_content = "print('SENSITIVE_STUDENT_CODE_67890')" + unsafe_filename = "accepted-#{student.username}-#{student.email}.py" + + logged = capture_rails_logs do + with_tempfile('.py', sensitive_content) do |f| + uploaded = Rack::Test::UploadedFile.new( + f.path, + 'text/plain', + true, + original_filename: unsafe_filename + ) + post_submission(project, td, uploaded) + end + end + + assert_equal 201, last_response.status, + 'Accepted submission logging test must exercise the successful path' + assert_includes logged, 'Uploaded file is accepted', + 'Expected safe file-validation success marker' + assert_includes logged, 'Submission accepted! Status for task', + 'Expected safe submission success marker' + assert_not_includes logged, sensitive_content, + 'Accepted submission log must not include file content' + assert_not_includes logged, student.email, + 'Accepted submission log must not include student email' + assert_not_includes logged, student.username, + 'Accepted submission log must not include student username' + assert_not_includes logged, unsafe_filename, + 'Accepted submission log must not include the client filename' + ensure + unit.destroy + end + + test 'comment attachment logs a safe marker without content or student identifiers' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + student = project.student + td = create_task_definition(unit: unit) + task = project.task_for_task_definition(td) + + add_auth_header_for(user: student) + + sensitive_comment = 'SENSITIVE_COMMENT_BODY_24680' + unsafe_filename = "comment-#{student.username}-#{student.email}.pdf" + pdf_path = Rails.root.join('test_files/submissions/00_question.pdf') + + logged = capture_rails_logs do + post "/api/projects/#{project.id}/task_def_id/#{td.id}/comments", + comment: sensitive_comment, + attachment: Rack::Test::UploadedFile.new( + pdf_path, + 'application/pdf', + true, + original_filename: unsafe_filename + ) + end + + assert_equal 201, last_response.status, + 'Comment logging test must exercise a successful attachment upload' + assert_includes logged, "user_id=#{student.id} added comment for task #{task.id}", + 'Expected safe comment audit marker using an internal user id' + assert_includes logged, 'Uploaded file is accepted', + 'Expected safe attachment-validation success marker' + assert_not_includes logged, sensitive_comment, + 'Comment attachment log must not include comment content' + assert_not_includes logged, student.email, + 'Comment attachment log must not include student email' + assert_not_includes logged, student.username, + 'Comment attachment log must not include student username' + assert_not_includes logged, unsafe_filename, + 'Comment attachment log must not include the client filename' + ensure + unit.destroy + end + + # ───────────────────────────────────────────────────────────────────────────── + # 13. Attachment retention and deletion behaviour + # Deleting a comment must remove its attachment file from disk. + # Deleting a task must remove its submission files from disk. + # ───────────────────────────────────────────────────────────────────────────── + + test 'deleting a comment with an attachment removes the file from disk' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + task = project.task_for_task_definition(td) + student = project.student + + add_auth_header_for(user: student) + + # Post a comment with a PDF attachment via the API. + pdf_path = Rails.root.join('test_files/submissions/00_question.pdf') + post "/api/projects/#{project.id}/task_def_id/#{td.id}/comments", + comment: 'test attachment', + attachment: Rack::Test::UploadedFile.new(pdf_path, 'application/pdf', true) + + assert_equal 201, last_response.status, last_response.body + + comment = task.comments.last + attachment_path = comment.attachment_path + + assert File.exist?(attachment_path), + 'Attachment file should exist on disk after upload' + + comment.destroy + + assert_not File.exist?(attachment_path), + 'Attachment file must be removed from disk when the comment is deleted' + ensure + unit.destroy + end + + test 'deleting a task comment via API removes the attachment from disk' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + task = project.task_for_task_definition(td) + student = project.student + + add_auth_header_for(user: student) + + pdf_path = Rails.root.join('test_files/submissions/00_question.pdf') + post "/api/projects/#{project.id}/task_def_id/#{td.id}/comments", + comment: 'test attachment', + attachment: Rack::Test::UploadedFile.new(pdf_path, 'application/pdf', true) + + assert_equal 201, last_response.status, last_response.body + + comment = task.comments.last + attachment_path = comment.attachment_path + + assert File.exist?(attachment_path), 'Attachment must exist before deletion' + + delete "/api/projects/#{project.id}/task_def_id/#{td.id}/comments/#{comment.id}" + + assert_includes [200, 204], last_response.status, + 'Expected 200 or 204 on comment deletion' + assert_not File.exist?(attachment_path), + 'Attachment file must be removed from disk after API comment deletion' + ensure + unit.destroy + end + + test 'comment attachment returns 404 after comment is deleted' do + unit = FactoryBot.create(:unit, student_count: 1, task_count: 0) + project = unit.active_projects.first + td = create_task_definition(unit: unit) + task = project.task_for_task_definition(td) + student = project.student + + add_auth_header_for(user: student) + + pdf_path = Rails.root.join('test_files/submissions/00_question.pdf') + post "/api/projects/#{project.id}/task_def_id/#{td.id}/comments", + comment: 'test attachment', + attachment: Rack::Test::UploadedFile.new(pdf_path, 'application/pdf', true) + + assert_equal 201, last_response.status, last_response.body + + comment = task.comments.last + comment_id = comment.id + comment.destroy + + get "/api/projects/#{project.id}/task_def_id/#{td.id}/comments/#{comment_id}" + + assert_equal 404, last_response.status, + 'Fetching a deleted comment attachment must return 404' + ensure + unit.destroy + end + +end diff --git a/test/config/sidekiq_config_test.rb b/test/config/sidekiq_config_test.rb new file mode 100644 index 0000000000..2f1da57cc7 --- /dev/null +++ b/test/config/sidekiq_config_test.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'erb' +require 'yaml' + +class SidekiqConfigTest < ActiveSupport::TestCase + def test_production_worker_consumes_notification_and_submission_queues_before_default + rendered = ERB.new(Rails.root.join('config/sidekiq.yml').read).result + config = YAML.safe_load(rendered, permitted_classes: [Symbol], aliases: true) + + assert_equal %w[mailers notifications submissions default], config.fetch(:queues) + end +end diff --git a/test/lib/batch03_docx_file_helper_test.rb b/test/lib/batch03_docx_file_helper_test.rb new file mode 100644 index 0000000000..1a33914bb9 --- /dev/null +++ b/test/lib/batch03_docx_file_helper_test.rb @@ -0,0 +1,180 @@ +# frozen_string_literal: true + +# Keep this test outside test/helpers: test_helper requires every helper file +# into every test process, which would execute this test in multiple shards. + +require 'test_helper' +require 'zip' + +class Batch03DocxFileHelperTest < ActiveSupport::TestCase + DOCX_MAIN_CONTENT_TYPE = + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document.main+xml' + + def with_docx(entries) + Tempfile.create(['batch03-docx', '.docx']) do |tempfile| + tempfile.close + + Zip::File.open(tempfile.path, Zip::File::CREATE) do |zip| + entries.each do |name, content| + zip.get_output_stream(name) { |stream| stream.write(content) } + end + end + + yield tempfile.path + end + end + + def minimal_docx_entries(content_type: DOCX_MAIN_CONTENT_TYPE) + { + '[Content_Types].xml' => <<~XML, + + + + + XML + '_rels/.rels' => <<~XML, + + + + + XML + 'word/document.xml' => <<~XML + + + Batch 03 + + XML + } + end + + test 'strictly accepts a valid uppercase DOCX as a word document' do + path = Rails.root.join('test_files/TestWordDoc.docx') + + result = File.open(path, 'rb') do |file| + FileHelper.accept_file( + { filename: 'phone-evidence.DOCX', 'tempfile' => file }, + 'Batch 03 DOCX', + 'word_document' + ) + end + + assert result[:accepted], result[:msg] + assert_equal 'success', result[:msg] + end + + test 'rejects DOCX bytes when the extension does not identify a word document' do + path = Rails.root.join('test_files/TestWordDoc.docx') + + result = File.open(path, 'rb') do |file| + FileHelper.accept_file( + { filename: 'phone-evidence.pdf', 'tempfile' => file }, + 'Batch 03 mismatched DOCX', + 'word_document' + ) + end + + assert_not result[:accepted] + assert_equal 'invalid file extension.', result[:msg] + end + + test 'rejects non-DOCX bytes carrying a DOCX extension' do + path = Rails.root.join('test_files/submissions/00_question.pdf') + + result = File.open(path, 'rb') do |file| + FileHelper.accept_file( + { filename: 'disguised.DOCX', 'tempfile' => file }, + 'Batch 03 mismatched PDF', + 'word_document' + ) + end + + assert_not result[:accepted] + assert_match(/MIME type|corrupt|OOXML/i, result[:msg]) + end + + test 'rejects a corrupt file presented as DOCX' do + Tempfile.create(['batch03-corrupt', '.docx']) do |tempfile| + tempfile.binmode + tempfile.write("PK\x03\x04not-a-complete-zip") + tempfile.flush + + result = FileHelper.validate_docx(tempfile.path) + + assert_not result[:valid] + assert_match(/corrupt|valid OOXML/i, result[:msg]) + end + end + + test 'rejects a DOCX package missing a required OOXML part' do + entries = minimal_docx_entries + entries.delete('word/document.xml') + + with_docx(entries) do |path| + result = FileHelper.validate_docx(path) + + assert_not result[:valid] + assert_match(%r{missing word/document\.xml}i, result[:msg]) + end + end + + test 'rejects a DOCX package with an invalid main OOXML content type' do + with_docx(minimal_docx_entries(content_type: 'application/xml')) do |path| + result = FileHelper.validate_docx(path) + + assert_not result[:valid] + assert_match(/invalid main document content type/i, result[:msg]) + end + end + + test 'rejects a DOCX package containing a traversal entry' do + entries = minimal_docx_entries.merge('../../../escape.txt' => 'must not escape') + + with_docx(entries) do |path| + result = FileHelper.validate_docx(path) + + assert_not result[:valid] + assert_match(/unsafe path/i, result[:msg]) + end + end + + test 'rejects a DOCX package containing a nested archive' do + entries = minimal_docx_entries.merge('word/embeddings/payload.zip' => "PK\x03\x04nested") + + with_docx(entries) do |path| + result = FileHelper.validate_docx(path) + + assert_not result[:valid] + assert_match(/nested archives/i, result[:msg]) + end + end + + test 'safe upload filename bounds long Unicode and removes path and control material' do + raw_name = "../../private/#{'📱測' * 180}\r\nInjected: yes.DOCX" + + safe_name = FileHelper.safe_upload_filename({ 'filename' => raw_name }) + + assert_operator safe_name.length, :<=, 255 + assert_equal '.DOCX', File.extname(safe_name) + assert_not_includes safe_name, '/' + assert_not_includes safe_name, '\\' + assert_not_includes safe_name, '..' + assert_equal safe_name.gsub(/[[:cntrl:]]/, ''), safe_name + assert_includes safe_name, '' + assert_includes safe_name, '📱' + end + + test 'rejects a DOCX whose main part has the wrong type even when the right type appears elsewhere' do + entries = minimal_docx_entries(content_type: 'application/xml') + entries['[Content_Types].xml'] = entries['[Content_Types].xml'].sub( + '', + "" + ) + + with_docx(entries) do |path| + result = FileHelper.validate_docx(path) + + assert_not result[:valid] + assert_match(/invalid main document content type/i, result[:msg]) + end + end +end diff --git a/test/lib/spreadsheet_upload_policy_test.rb b/test/lib/spreadsheet_upload_policy_test.rb new file mode 100644 index 0000000000..ad6bf625b0 --- /dev/null +++ b/test/lib/spreadsheet_upload_policy_test.rb @@ -0,0 +1,91 @@ +# frozen_string_literal: true + +require 'test_helper' +require 'zip' +require 'spreadsheet' + +class SpreadsheetUploadPolicyTest < ActiveSupport::TestCase + def with_xlsx(extra = {}) + entries = { + '[Content_Types].xml' => '', + '_rels/.rels' => '', + 'xl/workbook.xml' => '' + }.merge(extra) + Tempfile.create(['sheet', '.xlsx']) do |file| + file.close + Zip::File.open(file.path, Zip::File::CREATE) do |zip| + entries.each { |name, bytes| zip.get_output_stream(name) { |stream| stream.write(bytes) } } + end + File.open(file.path) { |handle| yield({ filename: 'sheet.XLSX', 'tempfile' => handle }) } + end + end + + test 'real XLSX structure accepted for task and chat without altering bytes' do + with_xlsx do |file| + before = File.binread(file['tempfile'].path) + %w[csv comment_attachment].each do |kind| + result = FileHelper.accept_file(file, 'Spreadsheet', kind) + assert result[:accepted], result[:msg] + end + assert_equal before, File.binread(file['tempfile'].path) + end + end + + test 'Office macro embedded malformed external and traversal payloads are rejected' do + [ + { 'xl/vbaProject.bin' => 'macro' }, + { 'xl/embeddings/oleObject1.bin' => 'object' }, + { 'xl/connections.xml' => '' }, + { 'xl/externalLinks/externalLink1.xml' => '' }, + { 'xl/queryTables/queryTable1.xml' => '' }, + { '../escape' => 'bad' }, + { 'xl/workbook.xml' => '' }, + { 'xl/_rels/workbook.xml.rels' => '' } + ].each do |extra| + with_xlsx(extra) do |file| + result = FileHelper.accept_file(file, 'Spreadsheet', 'comment_attachment') + assert_not result[:accepted], extra.keys.inspect + assert_equal 'UPLOAD_CORRUPT', result[:code] + end + end + end + + test 'legacy XLS remains a task-only spreadsheet and must contain readable workbook records' do + Tempfile.create(['legacy', '.xls']) do |file| + workbook = Spreadsheet::Workbook.new + workbook.create_worksheet(name: 'Results').row(0).push('Name', 7) + workbook.write(file.path) + upload = { filename: 'legacy.xls', 'tempfile' => file } + result = FileHelper.accept_file(upload, 'Spreadsheet', 'csv') + assert result[:accepted], result[:msg] + assert_not FileHelper.accept_file(upload, 'Spreadsheet', 'comment_attachment')[:accepted] + end + end + + test 'csv requirement persists and spreadsheet originals are retained in the submission archive' do + project = FactoryBot.create(:project) + definition = project.unit.task_definitions.first + definition.update!(upload_requirements: [{ 'key' => 'file0', 'name' => 'Results', 'type' => 'csv' }]) + task = project.task_for_task_definition(definition) + assert_equal 'csv', definition.reload.upload_requirements.first['type'] + Dir.mktmpdir do |directory| + File.write(File.join(directory, '000-csv.csv'), "a,b\n1,2\n") + destination = File.join(directory, 'originals.zip') + assert task.compress_new_to_done(task_dir: "#{directory}/", zip_file_path: destination, rm_task_dir: false) + Zip::File.open(destination) do |zip| + assert_equal "a,b\n1,2\n", zip.read("#{task.id}/000-csv.csv") + end + end + definition.upload_requirements.first['type'] = 'arbitrary' + assert_not definition.valid? + end + test 'the existing Code extensions remain accepted but a renamed document is not Code' do + Tempfile.create(['source', '.py']) do |file| + file.write("print('hello')\n") + file.flush + assert FileHelper.accept_file({ filename: 'main.py', 'tempfile' => file }, 'Code', 'code')[:accepted] + assert_not FileHelper.accept_file({ filename: 'main.pdf', 'tempfile' => file }, 'Code', 'code')[:accepted] + end + end + +end diff --git a/test/models/file_helper_test.rb b/test/models/file_helper_test.rb index d77a026fa5..faf0030d12 100644 --- a/test/models/file_helper_test.rb +++ b/test/models/file_helper_test.rb @@ -3,6 +3,100 @@ require "zip" class FileHelperTest < ActiveSupport::TestCase + def capture_upload_logs + output = StringIO.new + test_logger = Logger.new(output) + test_logger.level = Logger::INFO + FileHelper.stub(:logger, test_logger) { yield } + output.string + end + + def test_extension_rejection_is_visible_at_info_without_filename_or_temp_path + Tempfile.create(['private-student-name', '.txt']) do |file| + file.write('private file content') + file.flush + logs = capture_upload_logs do + result = FileHelper.accept_file( + {'filename' => "private-student-email@example.invalid.exe", 'tempfile' => file}, + 'private requirement label', 'comment_attachment' + ) + refute result[:accepted] + end + assert_includes logs, 'File extension check failed' + assert_includes logs, '"kind":"comment_attachment"' + assert_includes logs, '"uploaded_extension":".exe"' + assert_includes logs, '"temporary_extension":".txt"' + refute_includes logs, 'private' + refute_includes logs, file.path + end + end + + def test_unknown_upload_kind_fails_closed_without_logging_caller_controlled_values + Tempfile.create(['private-student-name', '.txt']) do |file| + file.write('private file content') + file.flush + + logs = capture_upload_logs do + result = FileHelper.accept_file( + { 'filename' => 'private-student-name.txt', 'tempfile' => file }, + 'private requirement label', + "private-kind\nforged-log-entry" + ) + + assert_not result[:accepted] + assert_equal 'unsupported file type.', result[:msg] + end + + assert_includes logs, 'Unknown file type' + assert_includes logs, '"kind":"unknown"' + assert_not_includes logs, 'private' + assert_not_includes logs, 'forged-log-entry' + assert_not_includes logs, file.path + end + end + + def test_mime_rejection_is_visible_at_info_with_detected_type_and_policy + Tempfile.create(['private-report', '.pdf']) do |file| + file.write('private plain text pretending to be a PDF') + file.flush + logs = capture_upload_logs do + result = FileHelper.accept_file( + {filename: 'private-report.pdf', 'tempfile' => file}, 'PDF', 'document' + ) + refute result[:accepted] + end + assert_includes logs, 'File MIME check failed' + assert_includes logs, '"detected_mime":"text/plain' + assert_includes logs, '"allowed_mime":["application/pdf"]' + refute_includes logs, 'private' + refute_includes logs, file.path + end + end + + def test_structural_rejection_is_visible_but_success_stays_at_debug + Tempfile.create(['private-submission', '.zip']) do |file| + Zip::File.open(file.path, Zip::File::CREATE) do |zip| + zip.get_output_stream('../private-escape.txt') { |io| io.write('private content') } + end + logs = capture_upload_logs do + result = FileHelper.accept_file({filename: 'archive.zip', 'tempfile' => file}, 'Zip', 'zip') + refute result[:accepted] + end + assert_includes logs, 'Zip file is invalid' + refute_includes logs, 'private' + refute_includes logs, file.path + end + Tempfile.create(['valid', '.py']) do |file| + file.write("print('hello')\n") + file.flush + logs = capture_upload_logs do + result = FileHelper.accept_file({filename: 'valid.py', 'tempfile' => file}, 'Code', 'code') + assert result[:accepted], result[:msg] + end + assert_empty logs + end + end + def test_convert_use_with_gif in_file = "#{Rails.root}/test_files/submissions/unbelievable.gif" diff --git a/test/models/submission_history_test.rb b/test/models/submission_history_test.rb index 266d987309..32bbd68ba3 100644 --- a/test/models/submission_history_test.rb +++ b/test/models/submission_history_test.rb @@ -1,6 +1,7 @@ require 'test_helper' require 'tmpdir' require 'zip' +require 'spreadsheet' class SubmissionHistoryTest < ActiveSupport::TestCase def test_creates_archive_with_only_selected_upload_requirements @@ -39,6 +40,63 @@ def test_creates_archive_with_only_selected_upload_requirements end end + def test_retains_spreadsheet_originals_alongside_existing_selected_types + unit = FactoryBot.create(:unit, task_count: 1, student_count: 1) + task = unit.active_projects.first.task_for_task_definition(unit.task_definitions.first) + kinds = %w[csv csv csv code document image zip archive csv] + task.task_definition.update!( + assessment_enabled: false, + upload_requirements: kinds.each_with_index.map do |kind, index| + { 'key' => "file#{index}", 'name' => "Evidence #{index}", 'type' => kind, 'submission_history' => index != 8 } + end + ) + + legacy_workbook = Spreadsheet::Workbook.new + legacy_workbook.create_worksheet(name: 'Results').row(0).push('Name', 7) + legacy_bytes = StringIO.new(''.b) + legacy_workbook.write(legacy_bytes) + originals = { + '000-csv.csv' => "Name,Value\r\nCafé,7\r\n".b, + '001-csv.xlsx' => File.binread(Rails.root.join('test_files/csv_test_files/COS10001-Tasks.xlsx')), + '002-csv.xls' => legacy_bytes.string, + '003-code.rb' => 'puts "retained"', + '004-document.pdf' => '%PDF-original', + '005-image.png' => "\x89PNG\r\n".b, + '006-zip.zip' => 'original zip bytes', + '007-archive.zip' => 'original archive bytes' + } + + Dir.mktmpdir do |dir| + source_path = File.join(dir, 'done.zip') + Zip::File.open(source_path, create: true) do |archive| + originals.merge('008-csv.csv' => 'not selected', 'metadata.json' => '{}').each do |name, bytes| + archive.get_output_stream("#{task.id}/#{name}") { |output| output.write(bytes) } + end + end + with_file_helper_methods( + zip_file_path_for_done_task: source_path, + task_submission_identifier_path: File.join(dir, 'history') + ) do + history = SubmissionHistory.create_archive!(task, '54321') + assert history.has_submission_files? + Zip::File.open(history.archive_file_name) do |archive| + assert_equal originals.length, archive.entries.length + originals.each do |name, bytes| + assert_equal bytes, archive.read("54321/#{task.id}/#{name}").b + end + end + Zip::File.open_buffer(StringIO.new(history.submission_zip_data)) do |download| + assert_equal originals.length, download.entries.length + originals.each do |name, bytes| + assert_equal bytes, download.read("#{task.id}/#{name}").b + end + assert_nil download.find_entry("#{task.id}/008-csv.csv") + assert_nil download.find_entry("#{task.id}/metadata.json") + end + end + end + end + def test_does_not_create_record_when_archive_copy_fails unit = FactoryBot.create(:unit, task_count: 1) task = unit.active_projects.first.task_for_task_definition(unit.task_definitions.first) diff --git a/test/models/submission_lifecycle_test.rb b/test/models/submission_lifecycle_test.rb new file mode 100644 index 0000000000..8f8e940d55 --- /dev/null +++ b/test/models/submission_lifecycle_test.rb @@ -0,0 +1,145 @@ +require 'test_helper' +require 'minitest/mock' + +class SubmissionLifecycleTest < ActiveSupport::TestCase + setup do + @unit = FactoryBot.create(:unit, student_count: 2, task_count: 1, + start_date: Time.current - 6.weeks, end_date: Time.current + 10.weeks) + @unit.update!(extension_weeks_on_resubmit_request: 1, allow_flexible_dates: false) + @definition = @unit.task_definitions.first + @definition.update!(start_date: @unit.start_date, target_date: Time.current - 3.weeks, due_date: @unit.end_date, target_grade: 0) + @task = @unit.active_projects.first.task_for_task_definition(@definition) + @staff = @unit.main_convenor_user + end + + test 'task opt out preserves existing dates and can be enabled for future feedback' do + assert @definition.resubmission_extensions_enabled + @definition.update!(resubmission_extensions_enabled: false) + previous_date = @task.effective_deadline + @task.assess(TaskStatus.fix_and_resubmit, @staff) + assert_equal previous_date, @task.reload.effective_deadline + assert_empty deadline_notifications + @definition.update!(resubmission_extensions_enabled: true) + @task.assess(TaskStatus.discuss, @staff) + assert_equal 1, @task.reload.extensions + assert_equal 1, deadline_notifications.count + @definition.update!(resubmission_extensions_enabled: false) + assert_equal 1, @task.reload.extensions, 'Opting out must not revoke an existing extension' + end + + test 'replayed assessments use one existing notification event and safe message' do + @task.assess(TaskStatus.fix_and_resubmit, @staff) + @task.assess(TaskStatus.discuss, @staff) + assert_equal 1, @task.reload.extensions + assert_equal 1, deadline_notifications.count + notification = deadline_notifications.first + assert_equal @task.project.student, notification.user + assert_equal @task.resubmission_extension_comment, notification.notifiable + assert_includes notification.message, @task.effective_deadline_date.iso8601 + assert_equal "Your task deadline is now #{@task.effective_deadline_date.iso8601} (end of day anywhere on earth) after feedback requiring further action. Open OnTrack for details.", notification.message + assert_includes notification.link, ERB::Util.url_encode(@definition.abbreviation) + NotificationService.deliver(notification) + assert_equal 1, deadline_notifications.count + end + + test 'archive marker failure rolls back both the deadline and notification' do + @task.stub(:record_resubmission_extension, ->(*) { raise 'simulated persistence failure' }) do + assert_raises(RuntimeError) { @task.assess(TaskStatus.fix_and_resubmit, @staff) } + end + assert_equal 0, @task.reload.extensions + assert_nil @task.resubmission_extension_comment + assert_empty deadline_notifications + end + + test 'notification reservation failure rolls back extension and replay marker' do + NotificationService.stub(:reserve, ->(**) { raise 'simulated notification persistence failure' }) do + assert_raises(RuntimeError) { @task.assess(TaskStatus.fix_and_resubmit, @staff) } + end + assert_equal 0, @task.reload.extensions + assert_nil @task.resubmission_extension_comment + assert_empty deadline_notifications + end + + test 'a stale duplicate assessment without an original submission date cannot earn a second extension' do + stale_task = Task.find(@task.id) + @task.assess(TaskStatus.fix_and_resubmit, @staff, Time.current - 1.minute) + stale_task.assess(TaskStatus.fix_and_resubmit, @staff, Time.current) + assert_equal 1, @task.reload.extensions + assert_equal 1, deadline_notifications.count + end + + test 'task api and calendar use the same date and stable event after feedback' do + calendar = @task.project.student.create_webcal!(guid: SecureRandom.uuid) + before = calendar.to_ical.events.find { |event| event.uid == "E-#{@definition.id}" } + @task.assess(TaskStatus.rediscuss, @staff) + @task.reload + response = Entities::TaskEntity.represent(@task, update_only: true).as_json + assert_equal @task.effective_deadline_date.iso8601, response[:effective_deadline_date] + assert_equal 'post_feedback_extension', response[:effective_deadline_reason] + assert_equal @task.resubmission_extension_comment.id, response[:effective_deadline_source_id] + after = calendar.reload.to_ical.events.find { |event| event.uid.to_s == before.uid.to_s } + assert_equal @task.effective_deadline_date, after.dtstart.to_date + assert_equal before.uid.to_s, after.uid.to_s + assert_not_equal before.dtstart, after.dtstart + end + + test 'task api omits an effective deadline date when shallow task data has no date' do + task_data = { + id: @task.id, + task_definition_id: @definition.id, + due_date: @task.due_date, + effective_deadline_date: nil + } + + response = Entities::TaskEntity.represent(task_data).as_json + + assert_not response.key?(:effective_deadline_date) + end + + test 'unit disable and flexible dates do not raise automatic notifications' do + @unit.update!(extension_weeks_on_resubmit_request: 0) + @task.reload + @task.assess(TaskStatus.demonstrate, @staff) + assert_empty deadline_notifications + @unit.update!(extension_weeks_on_resubmit_request: 1, allow_flexible_dates: true) + @task.reload + @task.assess(TaskStatus.demonstrate, @staff) + assert_equal 0, @task.reload.extensions + assert_empty deadline_notifications + assert_equal 'flexible_date', @task.effective_deadline_reason + end + + test 'group feedback extends and notifies each affected student only once' do + group_set = FactoryBot.create(:group_set, unit: @unit) + group = FactoryBot.create(:group, group_set: group_set, tutorial: @unit.tutorials.first) + @definition.update!(group_set: group_set) + projects = @unit.active_projects.to_a + projects.each { |project| group.add_member(project, notify: false) } + submission = GroupSubmission.create!(group: group, task_definition: @definition, submitted_by_project: projects.first) + tasks = projects.map { |project| project.task_for_task_definition(@definition) } + tasks.each { |task| task.update!(group_submission: submission) } + tasks.first.trigger_transition(trigger: 'fix', by_user: @staff) + tasks.first.trigger_transition(trigger: 'fix', by_user: @staff) + tasks.each do |task| + assert_equal 1, task.reload.extensions + assert_equal 1, Notification.where(user: task.project.student, event: 'resubmission_deadline_changed').count + end + assert_equal 1, tasks.map(&:effective_deadline).uniq.length + end + + test 'each declared feedback outcome grants its first extension' do + [TaskStatus.fix_and_resubmit, TaskStatus.discuss, TaskStatus.rediscuss, TaskStatus.demonstrate].each do |status| + @task.comments.where(type: 'ExtensionComment').destroy_all + @task.update_columns(extensions: 0, submission_date: nil) + @task.reload.assess(status, @staff) + assert_equal 1, @task.reload.extensions, "Expected extension for #{status.name}" + assert_equal status, @task.resubmission_extension_comment.task_status + end + end + + private + + def deadline_notifications + Notification.where(user: @task.project.student, event: 'resubmission_deadline_changed') + end +end diff --git a/test/models/submission_processing_state_test.rb b/test/models/submission_processing_state_test.rb new file mode 100644 index 0000000000..00388bb8aa --- /dev/null +++ b/test/models/submission_processing_state_test.rb @@ -0,0 +1,525 @@ +require 'test_helper' + +class SubmissionProcessingStateTest < ActiveSupport::TestCase + include ActiveSupport::Testing::TimeHelpers + + def setup + @task = FactoryBot.create(:task) + end + + def test_active_queue_becomes_finite_timed_out_state + @task.update!( + submission_processing_state: 'queued', + submission_processing_started_at: 11.minutes.ago + ) + + @task.stub(:submission_pdf_ready?, false) do + @task.stub(:submission_files_ready?, true) do + snapshot = @task.submission_processing_snapshot + + assert_equal 'timed_out', snapshot[:processing_state] + assert_not snapshot[:processing_pdf] + assert snapshot[:retryable] + assert snapshot[:submission_files_ready] + end + end + end + + def test_ready_snapshot_reports_each_real_artifact_independently + @task.update!(submission_processing_state: 'ready') + + @task.stub(:submission_pdf_ready?, true) do + @task.stub(:submission_files_ready?, true) do + snapshot = @task.submission_processing_snapshot + + assert_equal 'ready', snapshot[:processing_state] + assert snapshot[:has_pdf] + assert snapshot[:pdf_ready] + assert snapshot[:submission_files_ready] + assert_not snapshot[:retryable] + end + end + end + + def test_previous_attempt_pdf_is_not_ready_for_a_new_attempt + Dir.mktmpdir do |directory| + pdf_path = File.join(directory, 'submission.pdf') + File.write(pdf_path, 'old submission') + old_time = 5.minutes.ago + File.utime(old_time.to_time, old_time.to_time, pdf_path) + @task.update!(submission_processing_started_at: Time.current) + + @task.stub(:final_pdf_path, pdf_path) do + assert_not @task.submission_pdf_ready? + end + end + end + + def test_current_attempt_pdf_completes_a_stale_rolling_deploy_state + @task.update!( + submission_processing_state: 'queued', + submission_processing_started_at: 1.minute.ago + ) + + @task.stub(:submission_pdf_ready?, true) do + @task.stub(:processing_pdf?, false) do + assert_equal 'ready', @task.effective_submission_processing_state + end + end + end + + def test_marking_a_new_attempt_clears_failure_and_increments_attempts + @task.update!( + submission_processing_state: 'failed', + submission_processing_error_code: 'conversion_failed', + submission_processing_attempts: 1 + ) + + @task.mark_submission_processing!('queued', now: Time.zone.parse('2026-08-31 10:00:00')) + @task.reload + + assert_equal 'queued', @task.submission_processing_state + assert_equal 2, @task.submission_processing_attempts + assert_nil @task.submission_processing_error_code + assert_nil @task.submission_processing_finished_at + assert_equal Time.zone.parse('2026-08-31 10:00:00'), @task.submission_processing_started_at + end + + def test_retry_requeues_only_the_preserved_submission_archive + user = @task.project.student + queued = false + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, false) do + @task.stub(:folder_exists_in_process?, false) do + @task.stub(:submission_files_ready?, true) do + AcceptSubmissionJob.stub(:perform_async, lambda { |task_id, user_id, _tii, _test, processing_mode, _attempt| + queued = task_id == @task.id && user_id == user.id && processing_mode == 'retry_archive' + 'job-id' + }) do + @task.retry_submission_processing!(user) + end + end + end + end + end + + assert queued + assert_equal 'queued', @task.reload.submission_processing_state + end + + def test_timed_out_unprocessed_upload_can_requeue_its_staged_files + user = @task.project.student + queued_without_regeneration = false + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, true) do + @task.stub(:folder_exists_in_process?, false) do + AcceptSubmissionJob.stub(:perform_async, lambda { |_task_id, _user_id, _tii, _test, processing_mode, _attempt| + queued_without_regeneration = processing_mode == 'process' + 'job-id' + }) do + @task.retry_submission_processing!(user) + end + end + end + end + + assert queued_without_regeneration + end + + def test_queue_conflict_rolls_back_retry_state + user = @task.project.student + @task.update!( + submission_processing_state: 'failed', + submission_processing_error_code: 'conversion_failed', + submission_processing_attempts: 2 + ) + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, true) do + @task.stub(:folder_exists_in_process?, false) do + AcceptSubmissionJob.stub(:perform_async, nil) do + assert_raises(ArgumentError) { @task.retry_submission_processing!(user) } + end + end + end + end + + @task.reload + assert_equal 'failed', @task.submission_processing_state + assert_equal 2, @task.submission_processing_attempts + assert_equal 'conversion_failed', @task.submission_processing_error_code + end + + def test_failed_archive_extraction_preserves_existing_work_directories + Dir.mktmpdir do |directory| + new_path = File.join(directory, 'new', @task.id.to_s) + in_process_path = File.join(directory, 'in_process', @task.id.to_s) + zip_path = File.join(directory, 'submission.zip') + FileUtils.mkdir_p(new_path) + FileUtils.mkdir_p(in_process_path) + File.write(File.join(new_path, 'new-marker'), 'new') + File.write(File.join(in_process_path, 'processing-marker'), 'processing') + File.write(zip_path, 'not a zip archive') + + work_dir = lambda do |type, _create = true| + type == :new ? "#{new_path}/" : "#{in_process_path}/" + end + + @task.stub(:zip_file_path_for_done_task, zip_path) do + @task.stub(:student_work_dir, work_dir) do + assert_raises(Zip::Error) { @task.prepare_submission_regeneration! } + end + end + + assert File.file?(File.join(new_path, 'new-marker')) + assert File.file?(File.join(in_process_path, 'processing-marker')) + end + end + + def test_group_member_uses_the_submitter_as_processing_identity + unit = FactoryBot.create( + :unit, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 0 + ) + task_definition = FactoryBot.create( + :task_definition, + unit: unit, + group_set: unit.group_sets.first + ) + group = unit.groups.first + submitter_project, member_project = group.projects.first(2) + submitter = submitter_project.task_for_task_definition(task_definition) + member = member_project.task_for_task_definition(task_definition) + group_submission = GroupSubmission.create!( + group: group, + task_definition: task_definition, + submitted_by_project: submitter_project + ) + submitter.update!(group_submission: group_submission) + member.update!(group_submission: group_submission) + + assert_equal submitter.id, member.submission_processing_task.id + assert_equal group.id, member.submission_processing_lock_target.id + + member.mark_submission_processing!('queued', now: Time.zone.parse('2026-08-31 12:00:00')) + + [submitter, member].each do |task| + task.reload + assert_equal 'queued', task.submission_processing_state + assert_equal 1, task.submission_processing_attempts + assert_equal Time.zone.parse('2026-08-31 12:00:00'), task.submission_processing_started_at + end + end + + def test_submission_job_uses_the_dedicated_queue + assert_equal :submissions, AcceptSubmissionJob.get_sidekiq_options['queue'] + end + + def test_group_regeneration_restores_the_archive_through_the_submitter + fixture = group_submission_fixture + submitter = fixture.fetch(:submitter) + zip_path = submitter.zip_file_path_for_done_task + new_path = submitter.student_work_dir(:new, false) + FileUtils.mkdir_p(File.dirname(zip_path)) + Zip::File.open(zip_path, Zip::File::CREATE) do |zip| + zip.get_output_stream("#{submitter.id}/000-document.pdf") { |stream| stream.write('restored') } + end + + # The member resolves the submitter through a freshly loaded instance. + assert fixture.fetch(:member).prepare_submission_regeneration! + assert_equal 'restored', File.read(File.join(new_path, '000-document.pdf')) + ensure + FileUtils.rm_f(zip_path) if zip_path + FileUtils.rm_rf(new_path) if new_path + end + + def test_a_new_upload_is_refused_while_a_regeneration_is_queued + user = @task.project.student + @task.stub(:submission_files_ready?, true) do + AcceptSubmissionJob.stub(:perform_async, 'job-id') do + @task.regenerate_submission!(user) + end + end + ui = Object.new + def ui.error!(body, status) + raise ArgumentError, "#{status} #{body['error']}" + end + + # The regeneration has staged no files yet, so only its recorded mode + # shows that an upload now would be replaced by the old archive. + assert_not @task.processing_pdf? + error = assert_raises(ArgumentError) do + @task.accept_submission(user, nil, ui, nil, 'ready_for_feedback', nil) + end + + assert_match(/already being processed/, error.message) + assert_equal 'regenerate_only', @task.reload.submission_processing_mode + end + + def test_retry_replays_the_options_the_upload_was_accepted_with + user = @task.project.student + @task.mark_submission_processing!('queued', user_id: user.id, test_submission: true, accepted_tii_eula: true) + @task.mark_submission_processing!('failed', error_code: 'conversion_failed') + replayed = nil + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, false) do + @task.stub(:folder_exists_in_process?, false) do + @task.stub(:submission_files_ready?, true) do + AcceptSubmissionJob.stub(:perform_async, lambda { |_task_id, _user_id, tii, test, processing_mode, _attempt| + replayed = [tii, test, processing_mode] + 'job-id' + }) do + @task.retry_submission_processing!(user) + end + end + end + end + end + + assert_equal [true, true, 'retry_archive'], replayed + assert @task.reload.submission_processing_test_submission, 'a retry keeps the recorded options' + end + + def test_a_retry_by_someone_else_replays_the_uploaders_attempt + student = @task.project.student + tutor = FactoryBot.create(:user) + @task.mark_submission_processing!('queued', user_id: student.id, test_submission: false, accepted_tii_eula: true) + @task.mark_submission_processing!('failed', error_code: 'conversion_failed') + replayed = nil + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, false) do + @task.stub(:folder_exists_in_process?, false) do + @task.stub(:submission_files_ready?, true) do + AcceptSubmissionJob.stub(:perform_async, lambda { |_task_id, user_id, tii, _test, _processing_mode, attempt| + replayed = [user_id, tii, attempt] + 'job-id' + }) do + @task.retry_submission_processing!(tutor) + end + end + end + end + end + + # The student's consent travels with the student, not with the tutor. + assert_equal [student.id, true, 2], replayed + end + + def test_consent_is_not_replayed_when_the_uploader_no_longer_exists + caller = FactoryBot.create(:user) + @task.mark_submission_processing!('queued', user_id: 0, accepted_tii_eula: true) + @task.mark_submission_processing!('failed', error_code: 'conversion_failed') + replayed = nil + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, false) do + @task.stub(:folder_exists_in_process?, false) do + @task.stub(:submission_files_ready?, true) do + AcceptSubmissionJob.stub(:perform_async, lambda { |_task_id, user_id, tii, _test, _processing_mode, _attempt| + replayed = [user_id, tii] + 'job-id' + }) do + @task.retry_submission_processing!(caller) + end + end + end + end + end + + assert_equal [caller.id, false], replayed + end + + def test_files_staged_after_ready_are_reported_as_a_newer_attempt + Dir.mktmpdir do |directory| + pdf_path = File.join(directory, 'submission.pdf') + File.write(pdf_path, 'previous attempt') + # A previous-release API staged a new upload after this attempt was ready. + @task.update!(submission_processing_state: 'ready', submission_processing_started_at: 1.hour.ago) + + @task.stub(:final_pdf_path, pdf_path) do + @task.stub(:processing_pdf?, true) do + @task.stub(:folder_exists_in_process?, false) do + @task.stub(:submission_processing_file_timestamp, 1.minute.ago) do + snapshot = @task.submission_processing_snapshot + + assert_equal 'queued', snapshot[:processing_state] + assert snapshot[:processing_pdf] + assert_not snapshot[:has_pdf] + assert_not snapshot[:pdf_ready] + end + end + end + end + end + end + + def test_a_failed_backup_move_leaves_the_staged_upload_in_place + Dir.mktmpdir do |directory| + new_path = File.join(directory, 'new', @task.id.to_s) + in_process_path = File.join(directory, 'in_process', @task.id.to_s) + FileUtils.mkdir_p(new_path) + File.write(File.join(new_path, 'staged-marker'), 'staged') + zip_path = File.join(directory, 'submission.zip') + Zip::File.open(zip_path, Zip::File::CREATE) do |zip| + zip.get_output_stream("#{@task.id}/000-document.pdf") { |stream| stream.write('archived') } + end + work_dir = lambda do |type, _create = true| + type == :new ? "#{new_path}/" : "#{in_process_path}/" + end + original_mv = FileUtils.method(:mv) + failing_backup_mv = lambda do |source, destination, **options| + raise Errno::EACCES, 'simulated backup failure' if source.to_s == new_path + + original_mv.call(source, destination, **options) + end + + @task.stub(:zip_file_path_for_done_task, zip_path) do + @task.stub(:student_work_dir, work_dir) do + FileUtils.stub(:mv, failing_backup_mv) do + assert_raises(Errno::EACCES) { @task.prepare_submission_regeneration! } + end + end + end + + assert_equal 'staged', File.read(File.join(new_path, 'staged-marker')) + end + end + + def test_a_failed_regeneration_is_retried_as_a_regeneration + user = @task.project.student + @task.mark_submission_processing!('queued', processing_mode: 'regenerate_only') + @task.mark_submission_processing!('failed', error_code: 'conversion_failed') + replayed_mode = nil + + @task.stub(:submission_processing_retryable?, true) do + @task.stub(:folder_exists_in_new?, false) do + @task.stub(:folder_exists_in_process?, false) do + @task.stub(:submission_files_ready?, true) do + AcceptSubmissionJob.stub(:perform_async, lambda { |_task_id, _user_id, _tii, _test, processing_mode, _attempt| + replayed_mode = processing_mode + 'job-id' + }) do + @task.retry_submission_processing!(user) + end + end + end + end + end + + # Not retry_archive, which would run Turnitin, moderation and history again. + assert_equal 'regenerate_only', replayed_mode + end + + def test_a_pdf_written_after_a_failure_makes_the_submission_ready_again + Dir.mktmpdir do |directory| + pdf_path = File.join(directory, 'submission.pdf') + File.write(pdf_path, 'replacement') + @task.update!( + submission_processing_state: 'failed', + submission_processing_started_at: 20.minutes.ago, + submission_processing_finished_at: 10.minutes.ago + ) + + @task.stub(:final_pdf_path, pdf_path) do + @task.stub(:processing_pdf?, false) do + snapshot = @task.submission_processing_snapshot + assert_equal 'ready', snapshot[:processing_state] + assert snapshot[:has_pdf] + + # A PDF from before the failure does not count. + File.utime(15.minutes.ago.to_time, 15.minutes.ago.to_time, pdf_path) + assert_equal 'failed', @task.effective_submission_processing_state + end + end + end + end + + def test_an_unsubmitted_group_task_reports_its_state + unit = FactoryBot.create( + :unit, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 0 + ) + task_definition = FactoryBot.create(:task_definition, unit: unit, group_set: unit.group_sets.first) + task = unit.groups.first.projects.first.task_for_task_definition(task_definition) + task.save! if task.new_record? + + # No group submission yet, so there is no done folder to resolve. + assert task.group_task? + assert_nil task.group_submission + snapshot = task.submission_processing_snapshot + + assert_equal 'not_submitted', snapshot[:processing_state] + assert_not snapshot[:submission_files_ready] + end + + def test_an_uncompressed_done_folder_can_still_be_regenerated + Dir.mktmpdir do |directory| + new_path = File.join(directory, 'new', @task.id.to_s) + in_process_path = File.join(directory, 'in_process', @task.id.to_s) + done_path = File.join(directory, 'done', @task.id.to_s) + FileUtils.mkdir_p(done_path) + File.write(File.join(done_path, '000-document.pdf'), 'kept') + work_dir = lambda do |type, _create = true| + { new: "#{new_path}/", in_process: "#{in_process_path}/", done: "#{done_path}/" }.fetch(type) + end + + @task.stub(:zip_file_path_for_done_task, File.join(directory, 'missing.zip')) do + @task.stub(:student_work_dir, work_dir) do + assert @task.submission_files_ready? + assert @task.prepare_submission_regeneration! + end + end + + assert_equal 'kept', File.read(File.join(new_path, '000-document.pdf')) + end + end + + private + + def group_submission_fixture + unit = FactoryBot.create( + :unit, + group_sets: 1, + groups: [{ gs: 0, students: 2 }], + student_count: 2, + unenrolled_student_count: 0, + part_enrolled_student_count: 0, + inactive_student_count: 0, + task_count: 0 + ) + task_definition = FactoryBot.create( + :task_definition, + unit: unit, + group_set: unit.group_sets.first + ) + group = unit.groups.first + submitter_project, member_project = group.projects.first(2) + submitter = submitter_project.task_for_task_definition(task_definition) + member = member_project.task_for_task_definition(task_definition) + group_submission = GroupSubmission.create!( + group: group, + task_definition: task_definition, + submitted_by_project: submitter_project + ) + submitter.update!(group_submission: group_submission) + member.update!(group_submission: group_submission) + + { submitter: submitter, member: member } + end +end diff --git a/test/sidekiq/accept_submission_job_test.rb b/test/sidekiq/accept_submission_job_test.rb new file mode 100644 index 0000000000..963f036b91 --- /dev/null +++ b/test/sidekiq/accept_submission_job_test.rb @@ -0,0 +1,97 @@ +require 'test_helper' + +class AcceptSubmissionJobTest < ActiveSupport::TestCase + def test_pdf_regeneration_stops_before_submission_side_effects + task = FactoryBot.create(:task) + user = task.project.student + states = [] + restored = false + + task.stub(:mark_submission_processing!, ->(state, **_options) { states << state }) do + task.stub(:prepare_submission_regeneration!, -> { restored = true }) do + task.stub(:convert_submission_to_pdf, true) do + task.stub(:project, -> { raise 'submission side effects must not run' }) do + Task.stub(:find, task) do + User.stub(:find, user) do + AcceptSubmissionJob.new.perform(task.id, user.id, false, false, 'regenerate_only') + end + end + end + end + end + end + + assert restored + assert_equal %w[processing ready], states + end + + def test_a_stale_restore_leaves_a_newer_upload_alone + task = FactoryBot.create(:task) + user = task.project.student + # Queued as attempt 1, but a newer upload has since been accepted as attempt 2. + task.update!(submission_processing_state: 'failed', submission_processing_attempts: 2) + touched = [] + + task.stub(:mark_submission_processing!, ->(state, **_options) { touched << state }) do + task.stub(:prepare_submission_regeneration!, -> { touched << :restore }) do + task.stub(:convert_submission_to_pdf, ->(**_options) { touched << :convert }) do + Task.stub(:find, task) do + User.stub(:find, user) do + AcceptSubmissionJob.new.perform(task.id, user.id, false, false, 'retry_archive', 1) + end + end + end + end + end + + assert_empty touched + end + + def test_the_archive_is_restored_while_the_submission_lock_is_held + task = FactoryBot.create(:task) + user = task.project.student + task.update!(submission_processing_state: 'queued', submission_processing_attempts: 1) + outer_transactions = Task.connection.open_transactions + restored_under_lock = nil + + # Uploads take the same lock, so none can land between the check and here. + task.stub(:prepare_submission_regeneration!, -> { restored_under_lock = Task.connection.open_transactions > outer_transactions }) do + task.stub(:convert_submission_to_pdf, ->(**_options) { true }) do + Task.stub(:find, task) do + User.stub(:find, user) do + AcceptSubmissionJob.new.perform(task.id, user.id, false, false, 'regenerate_only', 1) + end + end + end + end + + assert restored_under_lock + assert_equal 'ready', task.reload.submission_processing_state + end + + def test_the_attempt_check_reads_the_stored_row_not_the_loaded_object + task = FactoryBot.create(:task) + user = task.project.student + task.update!(submission_processing_state: 'failed', submission_processing_attempts: 1) + # The retry that queued this job recorded attempt 2 after the worker + # loaded its copy of the task. + Task.where(id: task.id).update_all(submission_processing_state: 'queued', submission_processing_attempts: 2) # rubocop:disable Rails/SkipsModelValidations + touched = [] + + task.stub(:mark_submission_processing!, ->(state, **_options) { touched << state }) do + task.stub(:prepare_submission_regeneration!, -> { touched << :restore }) do + task.stub(:convert_submission_to_pdf, ->(**_options) { true }) do + task.stub(:project, -> { raise 'stop after conversion' }) do + Task.stub(:find, task) do + User.stub(:find, user) do + AcceptSubmissionJob.new.perform(task.id, user.id, false, false, 'regenerate_only', 2) + end + end + end + end + end + end + + assert_equal ['processing', :restore, 'ready'], touched + end +end From 267dd8a83d19fe03231b42713f675385bed71259 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Mon, 28 Sep 2026 00:29:20 +1000 Subject: [PATCH 2/2] feat(submissions): bring in org api PR 178 for the submissions files Brings ontrack-features-t2-2026/doubtfire-api#178 to the files this PR already carries, so every file stays in exactly one PR. It carries the project change for how often the summary email arrives. --- app/models/project.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/project.rb b/app/models/project.rb index 58a1ec868a..53d7f7d5f2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -742,6 +742,7 @@ def send_weekly_status_email(summary_stats, middle_of_unit) # end return unless student.receive_feedback_notifications + return unless student.wants_digest_on?(summary_stats[:cadence]) return if portfolio_exists? && !middle_of_unit begin