From 360b7ea190fa8e0f08a5c49213cd76a1e7aeeccd Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 10:19:31 +0200 Subject: [PATCH 01/11] Sanitize HTML preludes and drop stale Brakeman ignores --- app/helpers/html_prelude_helper.rb | 21 +++++ app/views/files/_files.html.erb | 4 +- config/brakeman.ignore | 90 ------------------- sorbet/rbi/dsl/api/v8/apidocs_controller.rbi | 1 + sorbet/rbi/dsl/application_controller.rbi | 1 + .../dsl/doorkeeper/application_controller.rbi | 1 + .../dsl/rails/conductor/base_controller.rbi | 1 + sorbet/rbi/shims/view_helpers.rbi | 11 +++ spec/views/files/files_spec.rb | 40 +++++++++ 9 files changed, 78 insertions(+), 92 deletions(-) create mode 100644 app/helpers/html_prelude_helper.rb create mode 100644 spec/views/files/files_spec.rb diff --git a/app/helpers/html_prelude_helper.rb b/app/helpers/html_prelude_helper.rb new file mode 100644 index 000000000..b5f29b212 --- /dev/null +++ b/app/helpers/html_prelude_helper.rb @@ -0,0 +1,21 @@ +# typed: strict +# frozen_string_literal: true + +module HtmlPreludeHelper + extend T::Sig + + ALLOWED_TAGS = T.let( + (Rails::HTML5::SafeListSanitizer.allowed_tags + %w[table thead tbody tfoot tr th td caption]).freeze, + T::Set[String] + ) + ALLOWED_ATTRIBUTES = T.let( + (Rails::HTML5::SafeListSanitizer.allowed_attributes + %w[style colspan rowspan]).freeze, + T::Set[String] + ) + + # Renders a teacher-written HTML prelude of an exercise file, keeping formatting but dropping scripts. + sig { params(html_prelude: String).returns(ActiveSupport::SafeBuffer) } + def sanitize_html_prelude(html_prelude) + T.cast(sanitize(html_prelude, tags: ALLOWED_TAGS, attributes: ALLOWED_ATTRIBUTES), ActiveSupport::SafeBuffer) + end +end diff --git a/app/views/files/_files.html.erb b/app/views/files/_files.html.erb index 1a4192c12..f0943ccc3 100644 --- a/app/views/files/_files.html.erb +++ b/app/views/files/_files.html.erb @@ -1,6 +1,6 @@ <% just_one_html_prelude = (files.count {|f| f.html_prelude != nil } == 1) %> <% if just_one_html_prelude %> - <%= raw(files.find {|f| f.html_prelude != nil }.html_prelude) %> + <%= sanitize_html_prelude(files.find {|f| f.html_prelude != nil }.html_prelude) %> <% end %> <% running_idx = 0 %> @@ -15,7 +15,7 @@
<% if !just_one_html_prelude && file.html_prelude %> - <%= raw(file.html_prelude) %> + <%= sanitize_html_prelude(file.html_prelude) %> <% end %> <% brush = case File.extname(file.path) diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 0200aeaac..9d9e52656 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -448,50 +448,6 @@ ], "note": "Course#material_url= prefixes anything without http(s):// with http://, so javascript: URLs can't get through." }, - { - "warning_type": "Cross-Site Scripting", - "warning_code": 2, - "fingerprint": "a8a1daeedec03d37d3282dde9662c963f55492b22ce9cb825eaff6f4a45d90a0", - "check_name": "CrossSiteScripting", - "message": "Unescaped model attribute", - "file": "app/views/files/_files.html.erb", - "line": 3, - "link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting", - "code": "T.let(Exercise.find(params[:exercise_id]).solution, T.nilable(Solution)).files.find do\n (f.html_prelude != nil)\n end.html_prelude", - "render_path": [ - { - "type": "controller", - "class": "SolutionsController", - "method": "show", - "line": 36, - "file": "app/controllers/solutions_controller.rb", - "rendered": { - "name": "solutions/show", - "file": "app/views/solutions/show.html.erb" - } - }, - { - "type": "template", - "name": "solutions/show", - "line": 3, - "file": "app/views/solutions/show.html.erb", - "rendered": { - "name": "files/_files", - "file": "app/views/files/_files.html.erb" - } - } - ], - "location": { - "type": "template", - "template": "files/_files" - }, - "user_input": "Exercise.find(params[:exercise_id]).solution", - "confidence": "Weak", - "cwe_id": [ - 79 - ], - "note": "Teachers write the .html preludes in their exercise repos; rendering them as HTML is the feature." - }, { "warning_type": "Command Injection", "warning_code": 14, @@ -561,52 +517,6 @@ ], "note": "Operator-run init script installer; the script name comes from the rake task argument." }, - { - "warning_type": "Command Injection", - "warning_code": 14, - "fingerprint": "5ca6d40038619fcfccd99ce710064c40e6c905f3d1faf5fa70ede67eba2b696c", - "check_name": "Execute", - "message": "Possible command injection", - "file": "lib/rust_langs_cli_executor.rb", - "line": 33, - "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", - "code": "Open3.capture2(\"#{self.langs_executable_path}/current\", \"prepare-submission\", \"--clone-path\", clone_path, \"--output-path\", output_path, \"--submission-path\", submission_path, \"--no-archive-prefix\", \"--stub-archive-path\", \"--output-format\", \"--tmc-param\")", - "render_path": null, - "location": { - "type": "method", - "class": "RustLangsCliExecutor", - "method": "self.prepare_submission" - }, - "user_input": "self.langs_executable_path", - "confidence": "Medium", - "cwe_id": [ - 77 - ], - "note": "Arguments are passed as an array to Open3, so no shell parses them." - }, - { - "warning_type": "Command Injection", - "warning_code": 14, - "fingerprint": "0c5c7d2990b8627b767deffda61fa918ed95d0fd4618db280a28d83055fe5dcf", - "check_name": "Execute", - "message": "Possible command injection", - "file": "lib/rust_langs_cli_executor.rb", - "line": 59, - "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", - "code": "Open3.popen2(\"#{self.langs_executable_path}/current\", \"refresh-course\", \"--cache-path\", T.must(course_template_refresh_task.course_template).cache_path, \"--cache-root\", Course.cache_root, \"--course-name\", T.must(course_template_refresh_task.course_template).name.to_s, \"--git-branch\", T.must(course_template_refresh_task.course_template).git_branch, \"--source-url\", T.must(course_template_refresh_task.course_template).source_url.to_s)", - "render_path": null, - "location": { - "type": "method", - "class": "RustLangsCliExecutor", - "method": "self.refresh" - }, - "user_input": "self.langs_executable_path", - "confidence": "Medium", - "cwe_id": [ - 77 - ], - "note": "Arguments are passed as an array to Open3, so no shell parses them." - }, { "warning_type": "Command Injection", "warning_code": 14, diff --git a/sorbet/rbi/dsl/api/v8/apidocs_controller.rbi b/sorbet/rbi/dsl/api/v8/apidocs_controller.rbi index 5d7065239..86cb6df96 100644 --- a/sorbet/rbi/dsl/api/v8/apidocs_controller.rbi +++ b/sorbet/rbi/dsl/api/v8/apidocs_controller.rbi @@ -27,6 +27,7 @@ class Api::V8::ApidocsController include ::ExtraFieldHelper include ::FeedbackAnswersHelper include ::FlashBlockHelper + include ::HtmlPreludeHelper include ::PasteHelper include ::PointsHelper include ::SessionsHelper diff --git a/sorbet/rbi/dsl/application_controller.rbi b/sorbet/rbi/dsl/application_controller.rbi index 479ba4adb..6a58409e0 100644 --- a/sorbet/rbi/dsl/application_controller.rbi +++ b/sorbet/rbi/dsl/application_controller.rbi @@ -27,6 +27,7 @@ class ApplicationController include ::ExtraFieldHelper include ::FeedbackAnswersHelper include ::FlashBlockHelper + include ::HtmlPreludeHelper include ::PasteHelper include ::PointsHelper include ::SessionsHelper diff --git a/sorbet/rbi/dsl/doorkeeper/application_controller.rbi b/sorbet/rbi/dsl/doorkeeper/application_controller.rbi index 17e219871..cfdfe8de0 100644 --- a/sorbet/rbi/dsl/doorkeeper/application_controller.rbi +++ b/sorbet/rbi/dsl/doorkeeper/application_controller.rbi @@ -27,6 +27,7 @@ class Doorkeeper::ApplicationController include ::ExtraFieldHelper include ::FeedbackAnswersHelper include ::FlashBlockHelper + include ::HtmlPreludeHelper include ::PasteHelper include ::PointsHelper include ::SessionsHelper diff --git a/sorbet/rbi/dsl/rails/conductor/base_controller.rbi b/sorbet/rbi/dsl/rails/conductor/base_controller.rbi index 962f53ad4..7ba722cd5 100644 --- a/sorbet/rbi/dsl/rails/conductor/base_controller.rbi +++ b/sorbet/rbi/dsl/rails/conductor/base_controller.rbi @@ -27,6 +27,7 @@ class Rails::Conductor::BaseController include ::ExtraFieldHelper include ::FeedbackAnswersHelper include ::FlashBlockHelper + include ::HtmlPreludeHelper include ::PasteHelper include ::PointsHelper include ::SessionsHelper diff --git a/sorbet/rbi/shims/view_helpers.rbi b/sorbet/rbi/shims/view_helpers.rbi index 25e34320c..48476db2b 100644 --- a/sorbet/rbi/shims/view_helpers.rbi +++ b/sorbet/rbi/shims/view_helpers.rbi @@ -124,6 +124,17 @@ module FlashBlockHelper include ApplicationHelper end +module HtmlPreludeHelper + include Kernel + include ERB::Util + include ActionView::Helpers + include ActionController::Base::HelperMethods + include GeneratedUrlHelpersModule + include GeneratedPathHelpersModule + include SessionsHelper + include ApplicationHelper +end + module PasteHelper include Kernel include ERB::Util diff --git a/spec/views/files/files_spec.rb b/spec/views/files/files_spec.rb new file mode 100644 index 000000000..be6efc57b --- /dev/null +++ b/spec/views/files/files_spec.rb @@ -0,0 +1,40 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'files/_files', type: :view do + def render_prelude(html) + file = SourceFileList::FileRecord.new('src/Main.java', 'class Main {}') + file.html_prelude = html + render partial: 'files/files', locals: { files: [file] } + rendered + end + + it 'strips scripts and event handlers from the HTML prelude' do + html = render_prelude(<<~HTML) + + + link + HTML + expect(html).not_to include('') + end + + it 'keeps formatting in the HTML prelude' do + html = render_prelude(<<~HTML) +

Task

+ +
int x = 1;
+
InOut
+

Note

+ HTML + expect(html).to include('

Task

') + expect(html).to include('
  • Docs
  • ') + expect(html).to include('
    int x = 1;
    ') + expect(html).to include('InOut') + expect(html).to include('

    Note

    ') + end +end From 894d2b9bc173ac0bb08b5d1bb6872324aa07ae24 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 10:39:06 +0200 Subject: [PATCH 02/11] Lock down sandbox results, archives and submission access --- app/controllers/application_controller.rb | 6 ++ app/controllers/participants_controller.rb | 1 + app/controllers/submissions_controller.rb | 11 ++- app/models/ability.rb | 2 +- app/models/submission.rb | 2 +- .../submissions/_submission_details.html.erb | 2 +- lib/safe_unzipper.rb | 67 ++++++++++----- lib/sandbox_results_saver.rb | 10 ++- .../participants_controller_spec.rb | 26 ++++++ .../controllers/submission_controller_spec.rb | 38 +++++++++ .../submission_download_authorization_spec.rb | 81 +++++++++++++++++++ spec/lib/safe_unzipper_spec.rb | 75 ++++++++++++----- .../rerun_exercise_submissions_spec.rb | 30 +++++++ spec/requests/sandbox_results_spec.rb | 77 ++++++++++++++++++ 14 files changed, 382 insertions(+), 46 deletions(-) create mode 100644 spec/controllers/submission_download_authorization_spec.rb create mode 100644 spec/requests/rerun_exercise_submissions_spec.rb create mode 100644 spec/requests/sandbox_results_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 071b8dcd5..f7abd5d4b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,6 +1,7 @@ # typed: strict # frozen_string_literal: true +require 'safe_unzipper' require 'version' require 'twitter_bootstrap_breadcrumbs' @@ -45,6 +46,11 @@ class ApplicationController < ActionController::Base respond_with_error('File not found', 404, exception) end + rescue_from SafeUnzipper::ArchiveTooLargeError do |exception| + T.bind(self, ApplicationController) + respond_with_error("The submitted files cannot be shown: #{exception.message}", 422) + end + rescue_from Rack::Timeout::RequestTimeoutException do |exception| T.bind(self, ApplicationController) respond_with_error('Request timed out', 504, exception) # status code 408 would be the correct status code, but when receiving 408 Firefox retries 10 times and then states the connection was reset instead of showing our error diff --git a/app/controllers/participants_controller.rb b/app/controllers/participants_controller.rb index 5008b0965..77dcd3773 100644 --- a/app/controllers/participants_controller.rb +++ b/app/controllers/participants_controller.rb @@ -151,6 +151,7 @@ def me def password_reset_link @user = User.find(params[:id]) authorize! :view_participant_information, @user + return respond_forbidden('This feature is only available to admins') unless current_user.administrator? return respond_forbidden('This feature is disabled for admin accounts') if @user.administrator? @password_reset_link = T.let(@user.generate_password_reset_link, T.nilable(String)) diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 4f1db0d8e..6064fb49a 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -50,14 +50,18 @@ def show respond_to do |format| format.html do - respond_forbidden if !current_user.administrator? && course.hide_submissions? + return respond_forbidden if !current_user.administrator? && course.hide_submissions? @files = T.let(SourceFileList.for_submission(submission), T.nilable(T.any(SourceFileList, T::Array[T::Hash[Symbol, String]]))) end format.zip do - respond_forbidden if !current_user.administrator? && course.hide_submissions? + return respond_forbidden if !current_user.administrator? && course.hide_submissions? send_data(submission.return_file, filename: "#{T.must(submission.user).login}-#{submission.exercise_name}-#{submission.id}.zip") end format.json do + # Only the files are hidden: clients poll this endpoint for the results of their own submissions. + include_files = !!params[:include_files] + return respond_forbidden if include_files && !current_user.administrator? && course.hide_submissions? + output = { api_version: ApiVersion::API_VERSION, all_tests_passed: submission.all_tests_passed?, @@ -106,7 +110,7 @@ def show end output = output.merge(T.must(status_fields)) output[:status] = :ok if output[:status] == :hidden - if !!params[:include_files] + if include_files output[:files] = SourceFileList.for_submission(submission).map { |f| { path: f.path, contents: f.contents } } end @@ -207,6 +211,7 @@ def update sig { void } def update_by_exercise exercise = T.must(@exercise) + authorize! :rerun, Submission.new(course: exercise.course, exercise: exercise) for submission in exercise.submissions schedule_for_rerun(submission, -2) end diff --git a/app/models/ability.rb b/app/models/ability.rb index 5eaa208c9..58a87ff5d 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -114,7 +114,7 @@ def initialize(user, auth_source: nil) end can :download, Submission do |sub| - !sub.course.hide_submission_results? && !sub.exercise&.hide_submission_results? && (can?(:read, sub) || sub.paste_visible_for?(user)) + !sub.course.hide_submission_results? && !sub.exercise&.hide_submission_results? && can?(:read, sub) end can :read_results, Submission do |sub| diff --git a/app/models/submission.rb b/app/models/submission.rb index 78c9f813b..0089a1765 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -373,7 +373,7 @@ def set_to_be_reprocessed!(priority = -200) # Changing it in the meantime will obsolete any runs currently being processed. sig { void } def randomize_secret_token - self.secret_token = rand(10**100).to_s + self.secret_token = SecureRandom.hex(32) end # How often we try to resend if no sandbox has received it yet diff --git a/app/views/submissions/_submission_details.html.erb b/app/views/submissions/_submission_details.html.erb index 2ea93dd24..3ef74c0e8 100644 --- a/app/views/submissions/_submission_details.html.erb +++ b/app/views/submissions/_submission_details.html.erb @@ -45,7 +45,7 @@
  • Needed <%= @submission.times_sent_to_sandbox %> processing attempts.
  • <% end %> - <% if can? :read, @submission or @is_paste %> + <% if can? :download, @submission %>
  • <%= link_to "Download as zip", submission_full_zip_index_path(@submission) %>
  • <% end %> diff --git a/lib/safe_unzipper.rb b/lib/safe_unzipper.rb index 278beeac2..868c0de6f 100644 --- a/lib/safe_unzipper.rb +++ b/lib/safe_unzipper.rb @@ -1,38 +1,65 @@ # typed: strict # frozen_string_literal: true +require 'find' require 'system_commands' +# Extracts untrusted zips, leaving only regular files and directories behind. class SafeUnzipper extend T::Sig + + # Far above any real exercise project; only archives built to exhaust the server reach these. + MAX_ENTRIES = 10_000 + MAX_UNCOMPRESSED_BYTES = T.let(100 * 1024 * 1024, Integer) + TIMEOUT_SECONDS = 60 + + # Raised before extraction for archives over the entry, size or time limits. + class ArchiveTooLargeError < StandardError; end + sig { params(zip_path: T.any(String, Pathname), dest_dir: T.any(String, Pathname)).void } def unzip(zip_path, dest_dir) + zip_path = zip_path.to_s dest_dir = File.absolute_path(dest_dir) + check_limits(zip_path) # Ideally we'd protect against `unzip` exploits too by running in a sandbox - SystemCommands.sh!('unzip', '-qqu', zip_path, '-d', dest_dir) - clean_up(dest_dir, dest_dir) + SystemCommands.sh!('unzip', '-qqu', zip_path, '-d', dest_dir, timeout: TIMEOUT_SECONDS) + clean_up(dest_dir) end private - sig { params(root_dir_abs: String, file: String).void } - def clean_up(root_dir_abs, file) - if File.symlink?(file) - dest = File.absolute_path(File.readlink(file), File.dirname(file)) - unless dest.start_with?(root_dir_abs) - ::Rails.logger.warn("Cleaning up external symlink to: #{dest}") - File.unlink(file) - end - elsif File.directory?(file) - Dir.entries(file).each do |entry| - if entry != '.' && entry != '..' - clean_up(root_dir_abs, file + '/' + entry) - end + sig { params(zip_path: String).void } + def check_limits(zip_path) + summary = SystemCommands.sh!('zipinfo', '-t', zip_path, timeout: TIMEOUT_SECONDS)[:output] + entry_count = summary[/(\d+) files?,/, 1] + raise "Unexpected zipinfo output: #{summary}" if entry_count.nil? + raise ArchiveTooLargeError, "the archive has more than #{MAX_ENTRIES} files." if entry_count.to_i > MAX_ENTRIES + raise ArchiveTooLargeError, "the archive extracts to more than #{MAX_UNCOMPRESSED_BYTES / 1024 / 1024} MB." if uncompressed_bytes_over_limit?(zip_path) + end + + # Decompresses to a pipe instead of trusting the sizes in the archive's headers, which can lie. + sig { params(zip_path: String).returns(T::Boolean) } + def uncompressed_bytes_over_limit?(zip_path) + total_bytes = 0 + IO.popen(['timeout', TIMEOUT_SECONDS.to_s, 'unzip', '-p', '-qq', zip_path], 'rb', in: File::NULL, err: File::NULL) do |io| + while (chunk = io.read(64 * 1024)) + total_bytes += chunk.bytesize + return true if total_bytes > MAX_UNCOMPRESSED_BYTES end - elsif !File.file?(file) - # Not a symlink, directory or regular file. Nuke. - # Currently unzip should never create these, according to the man page. - ::Rails.logger.warn("Cleaning up unusual file: #{file}") - File.unlink(file) + end + raise ArchiveTooLargeError, 'the archive took too long to extract.' if $?.exitstatus == 124 + false + end + + # Even links that look internal can escape through chained links, and + # nothing reading the extracted files needs them. + sig { params(dest_dir: String).void } + def clean_up(dest_dir) + Find.find(dest_dir) do |path| + stat = File.lstat(path) + next if stat.directory? || stat.file? + + ::Rails.logger.warn("Removing #{stat.ftype} from unzipped archive: #{path}") + File.unlink(path) end end end diff --git a/lib/sandbox_results_saver.rb b/lib/sandbox_results_saver.rb index 992f58396..6fa41c66a 100644 --- a/lib/sandbox_results_saver.rb +++ b/lib/sandbox_results_saver.rb @@ -15,7 +15,7 @@ class InvalidTokenError < RuntimeError; end sig { params(submission: Submission, results: T::Hash[String, T.untyped]).void } def self.save_results(submission, results) ActiveRecord::Base.transaction do - raise InvalidTokenError, 'Invalid or expired token' if results['token'] != submission.secret_token + raise InvalidTokenError, 'Invalid or expired token' unless valid_token?(submission, results['token']) submission.all_tests_passed = false submission.stdout = results['stdout'] @@ -68,6 +68,14 @@ def self.save_results(submission, results) end end + # The token is cleared once a submission is processed, so a blank one must never match. + sig { params(submission: Submission, token: T.untyped).returns(T::Boolean) } + private_class_method def self.valid_token?(submission, token) + expected = submission.secret_token + return false if submission.processed? || expected.blank? || !token.is_a?(String) || token.blank? + ActiveSupport::SecurityUtils.secure_compare(token, expected) + end + sig { params(submission: Submission, test_output: T.untyped).void } private_class_method def self.handle_tmc_langs_output(submission, test_output) if test_output.key? 'logs' diff --git a/spec/controllers/participants_controller_spec.rb b/spec/controllers/participants_controller_spec.rb index 392b607a2..a1c47080b 100644 --- a/spec/controllers/participants_controller_spec.rb +++ b/spec/controllers/participants_controller_spec.rb @@ -112,6 +112,32 @@ end end + describe 'GET /password_reset_link' do + let(:course) { FactoryBot.create(:course) } + + before do + FactoryBot.create(:awarded_point, course: course, user: @user) + end + + it 'is forbidden for a teacher of a course the user is in' do + teacher = FactoryBot.create(:user) + Teachership.create!(user: teacher, organization: course.organization) + controller.current_user = teacher + + expect(ActionToken).not_to receive(:generate_password_reset_key_for) + get :password_reset_link, params: { id: @user.id } + expect(response).to have_http_status(:forbidden) + end + + it 'shows the link to an admin' do + controller.current_user = FactoryBot.create(:admin) + + get :password_reset_link, params: { id: @user.id } + expect(response).to have_http_status(:ok) + expect(assigns(:password_reset_link)).to include('/reset_password/') + end + end + describe 'GET /show' do describe 'when logged in as an admin' do before :each do diff --git a/spec/controllers/submission_controller_spec.rb b/spec/controllers/submission_controller_spec.rb index bbab5e5dd..59bd2e8b9 100644 --- a/spec/controllers/submission_controller_spec.rb +++ b/spec/controllers/submission_controller_spec.rb @@ -189,6 +189,44 @@ expect(response).to have_http_status(:not_found) end end + + describe 'when the course hides submissions' do + before :each do + FactoryBot.create(:submission_data, submission: @submission) + @course.update!(hide_submissions: true) + end + + it 'forbids the zip' do + get :show, params: { id: @submission.id, format: :zip } + expect(response).to have_http_status(:forbidden) + end + + it 'forbids the files in JSON' do + get :show, params: { id: @submission.id, format: :json, api_version: ApiVersion::API_VERSION, include_files: 1 } + expect(response).to have_http_status(:forbidden) + end + + it 'still returns the results in JSON' do + get :show, params: { id: @submission.id, format: :json, api_version: ApiVersion::API_VERSION } + expect(response).to have_http_status(:ok) + check_common_keys(response.parsed_body) + end + end + + describe 'when the submission archive is too large to extract' do + render_views + + before :each do + FactoryBot.create(:submission_data, submission: @submission) + stub_const('SafeUnzipper::MAX_ENTRIES', 0) + end + + it 'explains why the files are not shown' do + get :show, params: { id: @submission.id } + expect(response).to have_http_status(:unprocessable_content) + expect(response.body).to include('The submitted files cannot be shown') + end + end end def check_common_keys(json) diff --git a/spec/controllers/submission_download_authorization_spec.rb b/spec/controllers/submission_download_authorization_spec.rb new file mode 100644 index 000000000..b6f49dcac --- /dev/null +++ b/spec/controllers/submission_download_authorization_spec.rb @@ -0,0 +1,81 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +shared_context 'a failing paste submission' do + let(:course) { FactoryBot.create(:course) } + let(:exercise) { FactoryBot.create(:exercise, course: course) } + let(:owner) { FactoryBot.create(:user) } + let(:other_student) { FactoryBot.create(:user) } + let(:teacher) do + FactoryBot.create(:user).tap { |t| Teachership.create!(user: t, organization: course.organization) } + end + let(:submission) do + FactoryBot.create(:submission, course: course, exercise: exercise, user: owner, + paste_available: true, all_tests_passed: false) + end + + before { FactoryBot.create(:submission_data, submission: submission) } +end + +describe FullZipController, type: :controller do + include_context 'a failing paste submission' + + before { allow_any_instance_of(SubmissionPackager).to receive(:get_full_zip).and_return('zip') } + + def download_as(user) + controller.current_user = user if user + get :index, params: { submission_id: submission.id } + end + + it 'refuses another student' do + download_as(other_student) + expect(response).to have_http_status(:forbidden) + end + + it 'refuses an anonymous user' do + download_as(nil) + expect(response).to redirect_to(%r{/login}) + end + + it 'lets the owner download' do + download_as(owner) + expect(response).to have_http_status(:ok) + end + + it 'lets a teacher download' do + download_as(teacher) + expect(response).to have_http_status(:ok) + end +end + +describe Api::V8::Core::SubmissionsController, type: :controller do + include_context 'a failing paste submission' + + def download_as(user) + token = user && double(resource_owner_id: user.id, acceptable?: true) + allow(controller).to receive(:doorkeeper_token) { token } + get :download, params: { id: submission.id } + end + + it 'refuses another student' do + download_as(other_student) + expect(response).to have_http_status(:forbidden) + end + + it 'refuses an anonymous user' do + download_as(nil) + expect(response).to have_http_status(:unauthorized).or have_http_status(:forbidden) + end + + it 'lets the owner download' do + download_as(owner) + expect(response).to have_http_status(:ok) + end + + it 'lets a teacher download' do + download_as(teacher) + expect(response).to have_http_status(:ok) + end +end diff --git a/spec/lib/safe_unzipper_spec.rb b/spec/lib/safe_unzipper_spec.rb index b533ed8f2..61b28f31c 100644 --- a/spec/lib/safe_unzipper_spec.rb +++ b/spec/lib/safe_unzipper_spec.rb @@ -7,34 +7,71 @@ require 'system_commands' describe SafeUnzipper do - it 'removes symlinks outside of the unzipped directory' do - Dir.mkdir('foo') - File.open('foo/good_file', 'wb') { |f| f.write('stuff') } - File.symlink('/etc/passwd', 'foo/bad_link') - File.symlink('../../' + File.basename(Dir.pwd), 'foo/bad_link2') + def zip_and_unzip SystemCommands.sh!('zip', '--symlinks', '-r', 'foo.zip', 'foo') - Dir.mkdir('out') SafeUnzipper.new.unzip('foo.zip', 'out') - - expect(File.exist?('out/foo/good_file')).to eq(true) - expect(File.exist?('out/foo/bad_link')).to eq(false) - expect(File.exist?('out/foo/bad_link2')).to eq(false) end - it 'preserves symlinks inside the unzipped directory' do + before do Dir.mkdir('foo') Dir.mkdir('foo/bar') - File.open('foo/good_file', 'wb') { |f| f.write('stuff') } - File.symlink('../good_file', 'foo/bar/good_link') - SystemCommands.sh!('zip', '--symlinks', '-r', 'foo.zip', 'foo') + File.write('foo/good_file', 'stuff') + File.write('foo/bar/nested_file', 'more stuff') + end + + it 'unzips regular files and directories' do + zip_and_unzip + + expect(File.read('out/foo/good_file')).to eq('stuff') + expect(File.read('out/foo/bar/nested_file')).to eq('more stuff') + end + + it 'removes symlinks outside of the unzipped directory' do + File.symlink('/etc/passwd', 'foo/bad_link') + File.symlink('../../' + File.basename(Dir.pwd), 'foo/bad_link2') + zip_and_unzip + + expect(File.exist?('out/foo/good_file')).to be(true) + expect(File.symlink?('out/foo/bad_link')).to be(false) + expect(File.symlink?('out/foo/bad_link2')).to be(false) + end + + it 'removes chained symlinks that only look internal' do + depth = Dir.pwd.count('/') + 5 + File.symlink('.', 'foo/s') + File.symlink(('s/' * depth) + ('../' * depth) + 'etc/hostname', 'foo/A.txt') + zip_and_unzip + leftover_links = Dir.glob('out/**/*', File::FNM_DOTMATCH).select { |path| File.symlink?(path) } + expect(leftover_links).to be_empty + expect(File.read('out/foo/good_file')).to eq('stuff') + end + + it 'refuses an archive that extracts to more than the size limit' do + stub_const('SafeUnzipper::MAX_UNCOMPRESSED_BYTES', 1000) + File.write('foo/big_file', '0' * 1001) + SystemCommands.sh!('zip', '-r', 'foo.zip', 'foo') Dir.mkdir('out') - SafeUnzipper.new.unzip('foo.zip', 'out') - expect(File.exist?('out/foo/good_file')).to eq(true) - expect(File.exist?('out/foo/bar/good_link')).to eq(true) - expect(File.symlink?('out/foo/bar/good_link')).to eq(true) - expect(File.readlink('out/foo/bar/good_link')).to eq('../good_file') + expect { SafeUnzipper.new.unzip('foo.zip', 'out') }.to raise_error(SafeUnzipper::ArchiveTooLargeError) + expect(Dir.empty?('out')).to be(true) + end + + it 'refuses an archive with more entries than the limit' do + stub_const('SafeUnzipper::MAX_ENTRIES', 3) + SystemCommands.sh!('zip', '-r', 'foo.zip', 'foo') + Dir.mkdir('out') + + expect { SafeUnzipper.new.unzip('foo.zip', 'out') }.to raise_error(SafeUnzipper::ArchiveTooLargeError) + expect(Dir.empty?('out')).to be(true) + end + + it 'removes symlinks inside the unzipped directory' do + File.symlink('../good_file', 'foo/bar/good_link') + zip_and_unzip + + expect(File.exist?('out/foo/good_file')).to be(true) + expect(File.symlink?('out/foo/bar/good_link')).to be(false) end end diff --git a/spec/requests/rerun_exercise_submissions_spec.rb b/spec/requests/rerun_exercise_submissions_spec.rb new file mode 100644 index 000000000..e93eddbf4 --- /dev/null +++ b/spec/requests/rerun_exercise_submissions_spec.rb @@ -0,0 +1,30 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Rerunning all submissions of an exercise', type: :request do + let(:course) { FactoryBot.create(:course) } + let(:exercise) { FactoryBot.create(:exercise, course: course) } + let(:student) { FactoryBot.create(:user) } + let!(:submission) { FactoryBot.create(:submission, course: course, exercise: exercise, user: student, processed: true) } + + def rerun_as(user) + put "/exercises/#{exercise.id}/submissions", + headers: { 'Authorization' => ActionController::HttpAuthentication::Basic.encode_credentials(user.login, user.password) } + end + + it 'is forbidden for a student' do + rerun_as(student) + expect(response).to have_http_status(:forbidden) + expect(submission.reload).to be_processed + end + + it 'is allowed for a teacher of the organization' do + teacher = FactoryBot.create(:user) + Teachership.create!(user: teacher, organization: course.organization) + rerun_as(teacher) + expect(response).to redirect_to("/exercises/#{exercise.id}") + expect(submission.reload).not_to be_processed + end +end diff --git a/spec/requests/sandbox_results_spec.rb b/spec/requests/sandbox_results_spec.rb new file mode 100644 index 000000000..859913802 --- /dev/null +++ b/spec/requests/sandbox_results_spec.rb @@ -0,0 +1,77 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Sandbox result callback', type: :request do + let(:submission) { FactoryBot.create(:submission, processed: false) } + let!(:point) { FactoryBot.create(:available_point, exercise: submission.exercise) } + let(:test_output) do + { + 'status' => 'PASSED', + 'testResults' => [{ 'name' => 'FooTest', 'successful' => true, 'points' => [point.name], 'message' => '' }], + 'logs' => {} + }.to_json + end + + def post_result(extra_params) + post "/submissions/#{submission.id}/result", params: { 'status' => 'finished', 'test_output' => test_output }.merge(extra_params) + end + + def expect_rejected + expect(response).to have_http_status(:unauthorized) + expect(AwardedPoint.count).to eq(0) + expect(submission.reload.test_case_runs).to be_empty + end + + it 'rejects a result without a token' do + post_result({}) + expect_rejected + expect(submission).not_to be_processed + end + + it 'rejects a result with a wrong token' do + post_result('token' => 'wrong') + expect_rejected + expect(submission).not_to be_processed + end + + it 'rejects a result for an already processed submission' do + token = submission.secret_token + submission.update!(processed: true) + post_result('token' => token) + expect_rejected + end + + it 'rejects a result without a token for a submission whose token was cleared' do + submission.update!(secret_token: nil, processed: true) + post_result({}) + expect_rejected + end + + it 'rejects a blank token while the submission has none' do + submission.update!(secret_token: nil) + post_result('token' => '') + expect_rejected + end + + it 'saves the result and awards points with the valid token' do + post_result('token' => submission.secret_token) + expect(response).to have_http_status(:ok) + submission.reload + expect(submission).to be_processed + expect(submission.secret_token).to be_nil + expect(submission.all_tests_passed).to be(true) + expect(AwardedPoint.where(submission: submission).map(&:name)).to eq([point.name]) + end + + it 'accepts the rotated token after a rerun' do + old_token = submission.secret_token + submission.set_to_be_reprocessed! + post_result('token' => old_token) + expect(response).to have_http_status(:unauthorized) + post_result('token' => submission.reload.secret_token) + expect(response).to have_http_status(:ok) + expect(submission.reload).to be_processed + end +end From ea07df636d217ca43f01f97141560def342591df Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 10:39:17 +0200 Subject: [PATCH 03/11] Scope org lookups, diff views and points sheets to the caller --- app/controllers/courses_controller.rb | 4 +-- app/controllers/points_controller.rb | 2 ++ .../setup/course_assistants_controller.rb | 2 +- app/controllers/setup/setup_controller.rb | 2 +- app/controllers/submissions_controller.rb | 1 + app/controllers/teachers_controller.rb | 2 +- spec/controllers/courses_controller_spec.rb | 29 +++++++++++++++---- spec/controllers/points_controller_spec.rb | 21 ++++++++++++++ .../setup/course_assistans_controller_spec.rb | 12 ++++++++ .../setup/course_details_controller_spec.rb | 10 +++++++ .../setup/course_finisher_controller_spec.rb | 10 +++++++ .../controllers/submission_controller_spec.rb | 26 +++++++++++++++++ spec/controllers/teachers_controller_spec.rb | 13 +++++++++ 13 files changed, 124 insertions(+), 10 deletions(-) diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index f057d2145..260a5be4b 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -161,7 +161,7 @@ def save_deadlines sig { void } def help - @course = Course.find(params[:course_id]) + @course = T.must(@organization).courses.find(params[:course_id]) authorize! :read, @course add_course_breadcrumb add_breadcrumb 'Help page' @@ -265,7 +265,7 @@ def set_organization sig { void } def set_course - @course = Course.find(params[:id]) + @course = T.must(@organization).courses.find(params[:id]) end sig { returns(T.any(ActionController::Parameters, T::Hash[T.untyped, T.untyped])) } diff --git a/app/controllers/points_controller.rb b/app/controllers/points_controller.rb index 5faea6829..22f5b693c 100644 --- a/app/controllers/points_controller.rb +++ b/app/controllers/points_controller.rb @@ -55,6 +55,7 @@ def show sheetname = T.let(params[:id], String) @sheetname = T.let(sheetname, T.nilable(String)) @course = Course.find(params[:course_id]) + authorize! :read, @course authorize! :see_points, @course return respond_unauthorized('Authentication required') if current_user.guest? @@ -69,6 +70,7 @@ def show exercises = Exercise.course_gdocs_sheet_exercises(@course, sheetname, current_user.administrator?).includes(:available_points).order!(:name) @exercises = T.let(exercises, T.nilable(ActiveRecord::Relation)) users_to_points = AwardedPoint.per_user_in_course_with_sheet(@course, sheetname, hidden: current_user.administrator?) + users_to_points = users_to_points.slice(current_user.login) unless can?(:teach, @course) @users_to_points = T.let(users_to_points, T.nilable(T::Hash[String, T::Array[String]])) users = @course.users.includes(:user_field_values) diff --git a/app/controllers/setup/course_assistants_controller.rb b/app/controllers/setup/course_assistants_controller.rb index 61b64cef6..284863a3e 100644 --- a/app/controllers/setup/course_assistants_controller.rb +++ b/app/controllers/setup/course_assistants_controller.rb @@ -51,7 +51,7 @@ def create sig { void } def destroy authorize! :modify_assistants, @course - @assistantship = Assistantship.find(params[:id]) + @assistantship = T.must(@course).assistantships.find(params[:id]) destroyed_email = T.must(@assistantship.user).email @assistantship.destroy! redirect_to setup_organization_course_course_assistants_path, notice: "Assistant #{destroyed_email} removed from course." diff --git a/app/controllers/setup/setup_controller.rb b/app/controllers/setup/setup_controller.rb index 726bd11e8..f4a67aa69 100644 --- a/app/controllers/setup/setup_controller.rb +++ b/app/controllers/setup/setup_controller.rb @@ -149,6 +149,6 @@ def set_organization sig { void } def set_course - @course = Course.find(params[:course_id]) unless params[:course_id].nil? + @course = T.must(@organization).courses.find(params[:course_id]) unless params[:course_id].nil? end end diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 6064fb49a..59c5dcd15 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -226,6 +226,7 @@ def difference_with_solution @exercise ||= submission.exercise exercise = @exercise return respond_not_found("This submission's exercise has been removed") if exercise.nil? + authorize! :read, exercise.solution @organization = T.must(@course).organization add_course_breadcrumb diff --git a/app/controllers/teachers_controller.rb b/app/controllers/teachers_controller.rb index 832445ed0..8f2bde91a 100644 --- a/app/controllers/teachers_controller.rb +++ b/app/controllers/teachers_controller.rb @@ -33,7 +33,7 @@ def create sig { void } def destroy authorize! :remove_teacher, @organization - @teachership = Teachership.find(params[:id]) + @teachership = T.must(@organization).teacherships.find(params[:id]) destroyed_username = T.must(@teachership.user).login @teachership.destroy! redirect_to organization_teachers_path, notice: "Teacher #{destroyed_username} removed from organization" diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 8346e938a..0f59d0c56 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -53,7 +53,7 @@ def get_index_json(options = {}) describe 'GET show' do before :each do - @course = FactoryBot.create(:course) + @course = FactoryBot.create(:course, organization: @organization) end describe 'for administrators' do @@ -107,7 +107,7 @@ def get_index_json(options = {}) describe 'in JSON format' do before :each do - @course = FactoryBot.create(:course, name: 'Course1') + @course = FactoryBot.create(:course, name: 'Course1', organization: @organization) @course.exercises << FactoryBot.create(:returnable_exercise, name: 'Exercise1', course: @course) @course.exercises << FactoryBot.create(:returnable_exercise, name: 'Exercise2', course: @course) @course.exercises << FactoryBot.create(:returnable_exercise, name: 'Exercise3', course: @course) @@ -220,7 +220,7 @@ def get_show_json(options = {}, parse_json = true) describe 'POST disable' do before :each do - @course = FactoryBot.create(:course) + @course = FactoryBot.create(:course, organization: @organization) end describe 'As a teacher' do @@ -242,7 +242,7 @@ def get_show_json(options = {}, parse_json = true) describe 'POST enable' do before :each do - @course = FactoryBot.create(:course) + @course = FactoryBot.create(:course, organization: @organization) end describe 'As a teacher' do @@ -347,7 +347,7 @@ def get_show_json(options = {}, parse_json = true) describe 'GET manage_unlocks' do it 'when non-teacher should respond with a 403' do - @course = FactoryBot.create :course + @course = FactoryBot.create :course, organization: @organization @course.organization = @organization controller.current_user = @user get :manage_unlocks, params: { organization_id: @organization.slug, id: @course.id } @@ -514,4 +514,23 @@ def get_show_json(options = {}, parse_json = true) end end end + + describe 'with a course of another organization in the URL' do + before :each do + @other_course = FactoryBot.create(:course, organization: FactoryBot.create(:accepted_organization)) + controller.current_user = @teacher + end + + it 'does not let a teacher disable it' do + post :disable, params: { organization_id: @organization.slug, id: @other_course.id } + expect(response.code.to_i).to eq(404) + expect(@other_course.reload.disabled?).to be(false) + end + + it 'does not let a teacher hide it' do + post :toggle_hidden, params: { organization_id: @organization.slug, id: @other_course.id } + expect(response.code.to_i).to eq(404) + expect(@other_course.reload.hidden).to be(false) + end + end end diff --git a/spec/controllers/points_controller_spec.rb b/spec/controllers/points_controller_spec.rb index 7fbb56b6d..56b611089 100644 --- a/spec/controllers/points_controller_spec.rb +++ b/spec/controllers/points_controller_spec.rb @@ -72,5 +72,26 @@ expect(response.code.to_i).to eq(403) end end + + describe 'in JSON format' do + before :each do + @other_user = FactoryBot.create(:user) + other_submission = FactoryBot.create(:submission, course: @course, user: @other_user, exercise: @exercise) + FactoryBot.create(:awarded_point, course: @course, name: @available_point.name, submission: other_submission, user: @other_user) + end + + it 'gives a student only their own points' do + get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: @sheetname, format: :json, api_version: ApiVersion::API_VERSION } + expect(response.parsed_body['users_to_points'].keys).to eq([@user.login]) + end + + it "gives a teacher everyone's points" do + teacher = FactoryBot.create(:user) + Teachership.create!(user: teacher, organization: @organization) + controller.current_user = teacher + get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: @sheetname, format: :json, api_version: ApiVersion::API_VERSION } + expect(response.parsed_body['users_to_points'].keys).to contain_exactly(@user.login, @other_user.login) + end + end end end diff --git a/spec/controllers/setup/course_assistans_controller_spec.rb b/spec/controllers/setup/course_assistans_controller_spec.rb index ba36f9d0c..f6163df30 100644 --- a/spec/controllers/setup/course_assistans_controller_spec.rb +++ b/spec/controllers/setup/course_assistans_controller_spec.rb @@ -112,4 +112,16 @@ expect(response.code.to_i).to eq(302) end end + + describe 'with an assistantship of another course' do + it 'does not let a teacher remove it' do + other_course = FactoryBot.create(:course, organization: FactoryBot.create(:accepted_organization)) + assistantship = Assistantship.create!(user: @user, course: other_course) + controller.current_user = @teacher + expect do + delete :destroy, params: { organization_id: @organization.slug, course_id: @course.id, id: assistantship.id } + end.not_to change(Assistantship, :count) + expect(response.code.to_i).to eq(404) + end + end end diff --git a/spec/controllers/setup/course_details_controller_spec.rb b/spec/controllers/setup/course_details_controller_spec.rb index 67b528119..9554e9549 100644 --- a/spec/controllers/setup/course_details_controller_spec.rb +++ b/spec/controllers/setup/course_details_controller_spec.rb @@ -274,4 +274,14 @@ def init_session started: Time.now } end + + describe 'with a course of another organization in the URL' do + it 'does not let a teacher rewrite its details' do + other_course = FactoryBot.create(:course, organization: FactoryBot.create(:accepted_organization), title: 'otherTitle') + controller.current_user = @teacher + put :update, params: { organization_id: @organization.slug, course_id: other_course.id, course: { title: 'hijacked' } } + expect(response.code.to_i).to eq(404) + expect(other_course.reload.title).to eq('otherTitle') + end + end end diff --git a/spec/controllers/setup/course_finisher_controller_spec.rb b/spec/controllers/setup/course_finisher_controller_spec.rb index 16d2c788b..785e4d862 100644 --- a/spec/controllers/setup/course_finisher_controller_spec.rb +++ b/spec/controllers/setup/course_finisher_controller_spec.rb @@ -83,4 +83,14 @@ def init_session started: Time.now } end + + describe 'with a course of another organization in the URL' do + it 'does not let a teacher publish it' do + other_course = FactoryBot.create(:course, organization: FactoryBot.create(:accepted_organization), disabled_status: :disabled) + controller.current_user = @teacher + post :create, params: { organization_id: @organization.slug, course_id: other_course.id, commit: 'Publish now' } + expect(response.code.to_i).to eq(404) + expect(other_course.reload.disabled?).to be(true) + end + end end diff --git a/spec/controllers/submission_controller_spec.rb b/spec/controllers/submission_controller_spec.rb index 59bd2e8b9..be58b84fe 100644 --- a/spec/controllers/submission_controller_spec.rb +++ b/spec/controllers/submission_controller_spec.rb @@ -227,6 +227,32 @@ expect(response.body).to include('The submitted files cannot be shown') end end + + describe 'GET difference_with_solution' do + before :each do + allow(SourceFileList).to receive_messages(for_submission: SourceFileList.new([]), for_solution: SourceFileList.new([])) + end + + it 'shows the difference to a teacher of a verified organization' do + teacher = FactoryBot.create(:verified_user) + Teachership.create!(user: teacher, organization: @organization) + controller.current_user = teacher + get :difference_with_solution, params: { submission_id: @submission.id } + expect(response).to have_http_status(:ok) + end + + it 'denies a teacher of an unverified organization' do + organization = FactoryBot.create(:organization) + course = FactoryBot.create(:course, organization: organization) + exercise = FactoryBot.create(:exercise, course: course) + submission = FactoryBot.create(:submission, course: course, user: @user, exercise: exercise) + teacher = FactoryBot.create(:verified_user) + Teachership.create!(user: teacher, organization: organization) + controller.current_user = teacher + get :difference_with_solution, params: { submission_id: submission.id } + expect(response).to have_http_status(:forbidden) + end + end end def check_common_keys(json) diff --git a/spec/controllers/teachers_controller_spec.rb b/spec/controllers/teachers_controller_spec.rb index b09fab74c..30c54b68d 100644 --- a/spec/controllers/teachers_controller_spec.rb +++ b/spec/controllers/teachers_controller_spec.rb @@ -83,4 +83,17 @@ end end end + + describe 'with a teachership of another organization' do + it 'does not let a teacher remove it' do + organization = FactoryBot.create(:accepted_organization) + Teachership.create!(user: @teacher, organization: organization) + other_teachership = Teachership.create!(user: @user, organization: FactoryBot.create(:accepted_organization)) + controller.current_user = @teacher + expect do + delete :destroy, params: { organization_id: organization.slug, id: other_teachership.id } + end.not_to change(Teachership, :count) + expect(response.code.to_i).to eq(404) + end + end end From 6cd7202b9c0bb677356fd629ea06dd3d8b26538d Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 11:02:51 +0200 Subject: [PATCH 04/11] Hide exam-mode results from API points, submission lists and status --- app/controllers/api/v8/base_controller.rb | 11 ++++ .../v8/courses/exercises/points_controller.rb | 2 +- .../exercises/users/points_controller.rb | 2 +- .../api/v8/courses/points_controller.rb | 2 +- .../api/v8/courses/submissions_controller.rb | 2 +- .../api/v8/courses/users/points_controller.rb | 2 +- .../courses/users/submissions_controller.rb | 2 +- .../exercises/users/submissions_controller.rb | 2 +- .../courses/exercises/points_controller.rb | 2 +- .../exercises/users/points_controller.rb | 2 +- .../courses/points_controller.rb | 2 +- .../courses/submissions_controller.rb | 2 +- .../courses/users/points_controller.rb | 2 +- .../courses/users/submissions_controller.rb | 2 +- app/controllers/exercise_status_controller.rb | 5 +- app/models/submission.rb | 12 ++-- lib/submission_list.rb | 5 +- .../api/v8/courses/points_controller_spec.rb | 56 +++++++++++++++++++ .../users/submissions_controller_spec.rb | 23 ++++++++ .../courses/users/points_controller_spec.rb | 12 ++++ .../exercise_status_controller_spec.rb | 12 ++++ 21 files changed, 142 insertions(+), 20 deletions(-) diff --git a/app/controllers/api/v8/base_controller.rb b/app/controllers/api/v8/base_controller.rb index 19c3ab19b..1b38d5576 100644 --- a/app/controllers/api/v8/base_controller.rb +++ b/app/controllers/api/v8/base_controller.rb @@ -70,6 +70,17 @@ def current_ability @current_ability ||= T.let(::Ability.new(current_user, auth_source: @auth_source), T.nilable(::Ability)) end + # Narrows `points` (awarded points of `course`) to what the caller may see. + # Raises CanCan::AccessDenied unless the caller may read the course and see its points. + sig { params(course: Course, points: ActiveRecord::Relation).returns(ActiveRecord::Relation) } + def visible_awarded_points(course, points) + authorize! :read, course + authorize! :see_points, course + return points if can?(:teach, course) + hidden_exercises = course.exercises.where(hidden: true).or(course.exercises.where(hide_submission_results: true)) + points.where.not(name: AvailablePoint.where(exercise_id: hidden_exercises.select(:id)).select(:name)) + end + sig { params(messages: T.any(String, T::Array[String])).returns(T::Hash[Symbol, T::Array[String]]) } def errors_json(messages) { errors: [*messages] } diff --git a/app/controllers/api/v8/courses/exercises/points_controller.rb b/app/controllers/api/v8/courses/exercises/points_controller.rb index e55115399..f4bd85d58 100644 --- a/app/controllers/api/v8/courses/exercises/points_controller.rb +++ b/app/controllers/api/v8/courses/exercises/points_controller.rb @@ -43,7 +43,7 @@ def index .where(submissions: { exercise_name: params[:exercise_name] }, course_id: course.id) - authorize! :read, points + points = visible_awarded_points(course, points) present(AwardedPoint.as_json_with_exercise_ids(points, course.exercises)) end end diff --git a/app/controllers/api/v8/courses/exercises/users/points_controller.rb b/app/controllers/api/v8/courses/exercises/users/points_controller.rb index 320cb16dc..bdf2f89f7 100644 --- a/app/controllers/api/v8/courses/exercises/users/points_controller.rb +++ b/app/controllers/api/v8/courses/exercises/users/points_controller.rb @@ -73,7 +73,7 @@ def index course_id: course.id, user_id: params[:user_id]) - authorize! :read, points + points = visible_awarded_points(course, points) present(AwardedPoint.as_json_with_exercise_ids(points, course.exercises)) end end diff --git a/app/controllers/api/v8/courses/points_controller.rb b/app/controllers/api/v8/courses/points_controller.rb index 15322d568..a406324c9 100644 --- a/app/controllers/api/v8/courses/points_controller.rb +++ b/app/controllers/api/v8/courses/points_controller.rb @@ -31,7 +31,7 @@ class PointsController < Api::V8::BaseController def index course = Course.find_by!(id: params[:course_id]) points = course.awarded_points.includes(:submission) - authorize_collection :read, points + points = visible_awarded_points(course, points) present AwardedPoint.as_json_with_exercise_ids(points, course.exercises) end end diff --git a/app/controllers/api/v8/courses/submissions_controller.rb b/app/controllers/api/v8/courses/submissions_controller.rb index edb9ff043..cf683b7ae 100644 --- a/app/controllers/api/v8/courses/submissions_controller.rb +++ b/app/controllers/api/v8/courses/submissions_controller.rb @@ -43,7 +43,7 @@ def index unauthorize_guest! course = Course.find_by!(id: params[:course_id]) submissions = Submission.where(course_id: course.id) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }) + readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) authorize! :read, readable present(readable) diff --git a/app/controllers/api/v8/courses/users/points_controller.rb b/app/controllers/api/v8/courses/users/points_controller.rb index e00f815fb..c7e59b3b1 100644 --- a/app/controllers/api/v8/courses/users/points_controller.rb +++ b/app/controllers/api/v8/courses/users/points_controller.rb @@ -54,7 +54,7 @@ def index course = Course.find_by!(id: params[:course_id]) params[:user_id] = current_user.id if params[:user_id] == 'current' points = course.awarded_points.includes(:submission).where(user_id: params[:user_id]) - authorize_collection :read, points + points = visible_awarded_points(course, points) present AwardedPoint.as_json_with_exercise_ids(points, course.exercises) end end diff --git a/app/controllers/api/v8/courses/users/submissions_controller.rb b/app/controllers/api/v8/courses/users/submissions_controller.rb index c4b6c6258..5d98ac9b6 100644 --- a/app/controllers/api/v8/courses/users/submissions_controller.rb +++ b/app/controllers/api/v8/courses/users/submissions_controller.rb @@ -75,7 +75,7 @@ def index course = Course.find_by!(id: params[:course_id]) params[:user_id] = current_user.id if params[:user_id] == 'current' submissions = Submission.where(course_id: course.id, user_id: params[:user_id]) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }) + readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) authorize! :read, readable present(readable) diff --git a/app/controllers/api/v8/exercises/users/submissions_controller.rb b/app/controllers/api/v8/exercises/users/submissions_controller.rb index 3672e9b76..4e489d336 100644 --- a/app/controllers/api/v8/exercises/users/submissions_controller.rb +++ b/app/controllers/api/v8/exercises/users/submissions_controller.rb @@ -65,7 +65,7 @@ def index exercise = Exercise.find_by!(id: params[:exercise_id]) params[:user_id] = current_user.id if params[:user_id] == 'current' submissions = Submission.where(course_id: exercise.course_id, exercise_name: exercise.name, user_id: params[:user_id]) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }) + readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) authorize! :read, readable present(readable) diff --git a/app/controllers/api/v8/organizations/courses/exercises/points_controller.rb b/app/controllers/api/v8/organizations/courses/exercises/points_controller.rb index d16b666f5..e0da86a49 100644 --- a/app/controllers/api/v8/organizations/courses/exercises/points_controller.rb +++ b/app/controllers/api/v8/organizations/courses/exercises/points_controller.rb @@ -45,7 +45,7 @@ def index .where(submissions: { exercise_name: params[:exercise_name] }, course_id: course.id) - authorize! :read, points + points = visible_awarded_points(course, points) present(AwardedPoint.as_json_with_exercise_ids(points, course.exercises)) end end diff --git a/app/controllers/api/v8/organizations/courses/exercises/users/points_controller.rb b/app/controllers/api/v8/organizations/courses/exercises/users/points_controller.rb index 5b256c3d8..1fc4eafac 100644 --- a/app/controllers/api/v8/organizations/courses/exercises/users/points_controller.rb +++ b/app/controllers/api/v8/organizations/courses/exercises/users/points_controller.rb @@ -76,7 +76,7 @@ def index course_id: course.id, user_id: params[:user_id]) - authorize! :read, points + points = visible_awarded_points(course, points) present(AwardedPoint.as_json_with_exercise_ids(points, course.exercises)) end end diff --git a/app/controllers/api/v8/organizations/courses/points_controller.rb b/app/controllers/api/v8/organizations/courses/points_controller.rb index c06be0b7e..daf782414 100644 --- a/app/controllers/api/v8/organizations/courses/points_controller.rb +++ b/app/controllers/api/v8/organizations/courses/points_controller.rb @@ -33,7 +33,7 @@ class PointsController < Api::V8::BaseController def index course = Course.find_by!(name: "#{params[:organization_slug]}-#{params[:course_name]}") points = course.awarded_points.includes(:submission) - authorize_collection :read, points + points = visible_awarded_points(course, points) present AwardedPoint.as_json_with_exercise_ids(points, course.exercises) end end diff --git a/app/controllers/api/v8/organizations/courses/submissions_controller.rb b/app/controllers/api/v8/organizations/courses/submissions_controller.rb index b2ff37a74..93c64d52c 100644 --- a/app/controllers/api/v8/organizations/courses/submissions_controller.rb +++ b/app/controllers/api/v8/organizations/courses/submissions_controller.rb @@ -45,7 +45,7 @@ def index unauthorize_guest! course = Course.find_by!(name: "#{params[:organization_slug]}-#{params[:course_name]}") submissions = Submission.where(course_id: course.id) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }) + readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) authorize_collection :read, readable present(readable) diff --git a/app/controllers/api/v8/organizations/courses/users/points_controller.rb b/app/controllers/api/v8/organizations/courses/users/points_controller.rb index 1253e1a6f..64897641c 100644 --- a/app/controllers/api/v8/organizations/courses/users/points_controller.rb +++ b/app/controllers/api/v8/organizations/courses/users/points_controller.rb @@ -57,7 +57,7 @@ def index course = Course.find_by!(name: "#{params[:organization_slug]}-#{params[:course_name]}") params[:user_id] = current_user.id if params[:user_id] == 'current' points = course.awarded_points.includes(:submission).where(user_id: params[:user_id]) - authorize_collection :read, points + points = visible_awarded_points(course, points) present AwardedPoint.as_json_with_exercise_ids(points, course.exercises) end end diff --git a/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb b/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb index 7aa792d96..139c69e64 100644 --- a/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb +++ b/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb @@ -78,7 +78,7 @@ def index course = Course.find_by!(name: "#{params[:organization_slug]}-#{params[:course_name]}") params[:user_id] = current_user.id if params[:user_id] == 'current' submissions = Submission.where(course_id: course.id, user_id: params[:user_id]) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }) + readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) authorize_collection :read, readable present(readable) diff --git a/app/controllers/exercise_status_controller.rb b/app/controllers/exercise_status_controller.rb index fa4fef7a8..fb73aa19a 100644 --- a/app/controllers/exercise_status_controller.rb +++ b/app/controllers/exercise_status_controller.rb @@ -18,10 +18,13 @@ def show user_subs = user.submissions.where(course_id: course.id).to_a.group_by(&:exercise_name) + can_teach = can?(:teach, course) results = {} course.exercises.each do |ex| + next unless can_teach || ex._fast_visible? ex.set_submissions_by!(user, user_subs.fetch(T.must(ex.name), [])) # used by completed_by? and attempted_by? - results[ex.name] = if ex.completed_by?(user) + results_hidden = !can_teach && (course.hide_submission_results? || ex.hide_submission_results?) + results[ex.name] = if !results_hidden && ex.completed_by?(user) 'completed' elsif ex.attempted_by?(user) 'attempted' diff --git a/app/models/submission.rb b/app/models/submission.rb index 0089a1765..9cac88739 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -421,9 +421,13 @@ def set_paste_key_if_paste_available self.paste_key = SecureRandom.urlsafe_base64 if paste_available? end - sig { params(submissions: T::Array[T.untyped]).returns(T::Array[T::Hash[Symbol, T.untyped]]) } - def self.filter_fields!(submissions) + # Maps `submissions` in place to their API list fields, as seen by `viewer`. + sig { params(submissions: T::Array[T.untyped], viewer: User).returns(T::Array[T::Hash[Symbol, T.untyped]]) } + def self.filter_fields!(submissions, viewer) + ActiveRecord::Associations::Preloader.new(records: submissions, associations: { course: :organization }).call + eager_load_exercises(submissions) submissions.map! do |sub| + results_visible = sub.can_see_results?(viewer) { id: sub.id, user_id: sub.user_id, @@ -432,8 +436,8 @@ def self.filter_fields!(submissions) exercise_name: sub.exercise_name, course_id: sub.course_id, processed: sub.processed, - all_tests_passed: sub.all_tests_passed, - points: sub.points, + all_tests_passed: results_visible ? sub.all_tests_passed : nil, + points: results_visible ? sub.points : nil, processing_tried_at: sub.processing_tried_at, processing_began_at: sub.processing_began_at, processing_completed_at: sub.processing_completed_at, diff --git a/lib/submission_list.rb b/lib/submission_list.rb index 9a396bbc8..afbc897d7 100644 --- a/lib/submission_list.rb +++ b/lib/submission_list.rb @@ -17,14 +17,15 @@ def submission_list_data(submissions) sig { params(submission: Submission).returns(T::Hash[Symbol, T.untyped]) } def submission_data(submission) + results_visible = submission.can_see_results?(@user) { exercise_name: submission.exercise_name, id: submission.id, user_id: submission.user_id, course_id: submission.course_id, created_at: submission.created_at, - all_tests_passed: submission.all_tests_passed, - points: submission.points, + all_tests_passed: results_visible ? submission.all_tests_passed : nil, + points: results_visible ? submission.points : nil, submitted_zip_url: @helpers.download_api_v8_core_submission_url(submission), paste_url: paste_url(submission), processing_time: submission.processing_time, diff --git a/spec/controllers/api/v8/courses/points_controller_spec.rb b/spec/controllers/api/v8/courses/points_controller_spec.rb index ebbe382b0..8f1c9b9cf 100644 --- a/spec/controllers/api/v8/courses/points_controller_spec.rb +++ b/spec/controllers/api/v8/courses/points_controller_spec.rb @@ -96,4 +96,60 @@ end end end + + describe 'GET points for a course in exam mode' do + before(:each) { course.update!(hide_submission_results: true) } + + describe 'as user' do + let(:current_user) { FactoryBot.create(:user) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it 'is forbidden' do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(403) + expect(response.body).not_to include course_point.name + end + end + + describe 'as guest' do + let(:current_user) { Guest.new } + let(:token) { nil } + + it 'requires authentication' do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(401) + expect(response.body).not_to include course_point.name + end + end + end + + describe 'GET points for a course with a hidden exercise' do + let!(:hidden_point) do + exercise = FactoryBot.create(:exercise, course: course, hidden: true) + FactoryBot.create(:available_point, exercise: exercise) + FactoryBot.create(:awarded_point, course: course, name: exercise.available_points.first.name) + end + + describe 'as user' do + let(:current_user) { FactoryBot.create(:user) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it "leaves out the hidden exercise's points" do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(200) + expect(response.body).to include course_point.name + expect(response.body).not_to include hidden_point.name + end + end + + describe 'as admin' do + let(:current_user) { FactoryBot.create(:admin) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it "includes the hidden exercise's points" do + get :index, params: { course_id: course.id } + expect(response.body).to include hidden_point.name + end + end + end end diff --git a/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb b/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb index e19d92571..413e98206 100644 --- a/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb +++ b/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb @@ -25,6 +25,29 @@ Assistantship.create(user: assistant, course: course) end + describe "GET a user's submissions in exam mode" do + before :each do + submission.update!(all_tests_passed: true, points: 'point1') + course.update!(hide_submission_results: true) + end + + it 'hides the results from the student' do + controller.current_user = user + get :index, params: { course_id: course.id, user_id: user.id } + listed = response.parsed_body.find { |s| s['id'] == submission.id } + expect(listed['all_tests_passed']).to be_nil + expect(listed['points']).to be_nil + end + + it 'shows the results to a teacher' do + controller.current_user = teacher + get :index, params: { course_id: course.id, user_id: user.id } + listed = response.parsed_body.find { |s| s['id'] == submission.id } + expect(listed['all_tests_passed']).to be(true) + expect(listed['points']).to eq('point1') + end + end + describe "GET single user's submissions" do describe 'by course id as json' do describe 'as an admin' do diff --git a/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb b/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb index e0ba785b6..b8da4ca0f 100644 --- a/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb +++ b/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb @@ -199,4 +199,16 @@ end end end + + describe "GET current user's points in exam mode" do + let(:current_user) { FactoryBot.create(:user) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it 'is forbidden' do + course.update!(hide_submission_results: true) + get :index, params: { organization_slug: organization.slug, course_name: course_name, user_id: 'current' } + expect(response).to have_http_status(403) + expect(response.body).not_to include current_user_course_point.name + end + end end diff --git a/spec/controllers/exercise_status_controller_spec.rb b/spec/controllers/exercise_status_controller_spec.rb index ecc31c2c1..8b8bb4b2c 100644 --- a/spec/controllers/exercise_status_controller_spec.rb +++ b/spec/controllers/exercise_status_controller_spec.rb @@ -57,6 +57,18 @@ def do_get expect(json[@exercise3.name]).to eq('not_attempted') end + it 'leaves out hidden exercises' do + @exercise3.update!(hidden: true) + do_get + expect(response.parsed_body).not_to have_key @exercise3.name + end + + it 'does not reveal completion while results are hidden' do + @course.update!(hide_submission_results: true) + do_get + expect(response.parsed_body[@exercise.name]).to eq('attempted') + end + it 'should work when using course and user name instrad of id:s' do get :show, params: { organization_id: @organization.slug, course_id: @course.name, id: @user.login, format: :json, api_version: ApiVersion::API_VERSION } expect(response).to be_successful From 3d8d2de56e61277578198b511122046f19415925 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 11:05:59 +0200 Subject: [PATCH 05/11] Check per-record read access in lists, solutions and stats --- .../course_template_refresh_channel.rb | 13 +++++++- .../v8/core/exercises/details_controller.rb | 5 +-- .../core/submissions/feedback_controller.rb | 4 +++ ...l_courses_with_this_template_controller.rb | 4 +-- app/controllers/courses_controller.rb | 4 +-- .../feedback_answers_controller.rb | 4 +++ app/controllers/stats_controller.rb | 4 ++- app/models/solution.rb | 4 +-- .../course_template_refresh_channel_spec.rb | 31 ++++++++++++++++--- .../core/exercises/details_controller_spec.rb | 19 ++++++++++++ .../submissions/feedback_controller_spec.rb | 8 +++++ ...rses_with_this_template_controller_spec.rb | 21 +++++++++++++ spec/controllers/courses_controller_spec.rb | 19 ++++++++++++ .../feedback_answers_controller_spec.rb | 11 +++++++ spec/controllers/stats_controller_spec.rb | 8 +++++ spec/models/solution_spec.rb | 19 ++++++++++++ 16 files changed, 164 insertions(+), 14 deletions(-) create mode 100644 spec/controllers/api/v8/core/exercises/details_controller_spec.rb create mode 100644 spec/controllers/api/v8/courses/all_courses_with_this_template_controller_spec.rb diff --git a/app/channels/course_template_refresh_channel.rb b/app/channels/course_template_refresh_channel.rb index 69225d53a..bbab6c61e 100644 --- a/app/channels/course_template_refresh_channel.rb +++ b/app/channels/course_template_refresh_channel.rb @@ -6,11 +6,22 @@ class CourseTemplateRefreshChannel < ApplicationCable::Channel sig { void } def subscribed - stream_from "CourseTemplateRefreshChannel-#{params[:courseTemplateId]}" + course_template = CourseTemplate.find_by(id: params[:courseTemplateId]) + return reject unless course_template && can_follow_refresh?(course_template) + stream_from "CourseTemplateRefreshChannel-#{course_template.id}" end sig { void } def unsubscribed # Any cleanup needed when channel is unsubscribed end + + private + # Matches who is shown refresh progress: template admins and teachers of a course using the template. + sig { params(course_template: CourseTemplate).returns(T::Boolean) } + def can_follow_refresh?(course_template) + # current_user is delegated from the connection at runtime, so Sorbet cannot see it. + ability = Ability.new(T.unsafe(self).current_user) + ability.can?(:create, CourseTemplate) || course_template.courses.any? { |course| ability.can?(:teach, course) } + end end diff --git a/app/controllers/api/v8/core/exercises/details_controller.rb b/app/controllers/api/v8/core/exercises/details_controller.rb index 1eface8c7..c03761641 100644 --- a/app/controllers/api/v8/core/exercises/details_controller.rb +++ b/app/controllers/api/v8/core/exercises/details_controller.rb @@ -49,8 +49,9 @@ def show exercise_ids = params[:ids] return respond_not_found('Query param ids is empty. Example: ?ids=1,2,3') if exercise_ids.blank? || exercise_ids.empty? - exercises = Exercise.where(id: exercise_ids.split(',')).includes(:course) - authorize! :read, exercises + exercises = Exercise.where(id: exercise_ids.split(',')).includes(:course).to_a + readable_courses = exercises.filter_map(&:course).uniq.select { |course| can?(:read, course) } + exercises = exercises.select { |exercise| readable_courses.include?(exercise.course) } data = exercises.map do |exercise| { id: exercise.id, diff --git a/app/controllers/api/v8/core/submissions/feedback_controller.rb b/app/controllers/api/v8/core/submissions/feedback_controller.rb index 384cabd08..352d6b01a 100644 --- a/app/controllers/api/v8/core/submissions/feedback_controller.rb +++ b/app/controllers/api/v8/core/submissions/feedback_controller.rb @@ -56,6 +56,10 @@ def create end answer_records.each { |record| authorize! :create, record } + course_question_ids = T.must(submission.course).feedback_question_ids + if answer_records.any? { |record| course_question_ids.exclude?(record.feedback_question_id) } + return respond_with_error('Feedback question not found in this course', 422) + end begin ActiveRecord::Base.connection.transaction(requires_new: true) do answer_records.each(&:save!) diff --git a/app/controllers/api/v8/courses/all_courses_with_this_template_controller.rb b/app/controllers/api/v8/courses/all_courses_with_this_template_controller.rb index 445622c03..f24feff68 100644 --- a/app/controllers/api/v8/courses/all_courses_with_this_template_controller.rb +++ b/app/controllers/api/v8/courses/all_courses_with_this_template_controller.rb @@ -40,8 +40,8 @@ class AllCoursesWithThisTemplateController < Api::V8::BaseController def index unauthorize_guest! course = Course.find_by!(id: params[:course_id]) - courses = Course.where(course_template_id: course[:course_template_id]) - authorize! :read, courses + authorize! :read, course + courses = Course.where(course_template_id: course[:course_template_id]).select { |c| can?(:read, c) } present(courses) end end diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index 260a5be4b..c5d23823b 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -69,13 +69,13 @@ def show # Method for teacher to give a single course for students to select. sig { void } def show_json - course = [Course.find(params[:id])] + course = T.must(@organization).courses.find(params[:id]) authorize! :read, course return respond_unauthorized('Authentication required') if current_user.guest? data = { api_version: ApiVersion::API_VERSION, - courses: CourseList.new(current_user, view_context).course_list_data(T.must(@organization), course) + courses: CourseList.new(current_user, view_context).course_list_data(T.must(@organization), [course]) } render json: data.to_json end diff --git a/app/controllers/feedback_answers_controller.rb b/app/controllers/feedback_answers_controller.rb index 7934abcde..4fd862bb1 100644 --- a/app/controllers/feedback_answers_controller.rb +++ b/app/controllers/feedback_answers_controller.rb @@ -104,6 +104,10 @@ def create answer: answer_hash[:answer]) end answer_records.each { |record| authorize! :create, record } + course_question_ids = T.must(submission.course).feedback_question_ids + if answer_records.any? { |record| course_question_ids.exclude?(record.feedback_question_id) } + return respond_with_error('Feedback question not found in this course', 422) + end begin ActiveRecord::Base.connection.transaction(requires_new: true) do diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 43aff9f52..bf32ea8a7 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -42,7 +42,9 @@ def show private sig { void } def get_vars - @course = Course.find(params[:course_id]) if params[:course_id] + return unless params[:course_id] + @course = Course.find(params[:course_id]) + authorize! :read, @course end sig { void } diff --git a/app/models/solution.rb b/app/models/solution.rb index 974371562..7d926f234 100644 --- a/app/models/solution.rb +++ b/app/models/solution.rb @@ -34,14 +34,14 @@ def visible_to?(user, ignore_email_verification = false) false elsif course.hide_submission_results? || @exercise.hide_submission_results? false - elsif course.model_solution_visible_before_completion? && course.enabled? - true elsif @exercise.completed_by?(user) true elsif !course.visible_to?(user) false elsif !@exercise.visible_to?(user) false + elsif course.model_solution_visible_before_completion? && course.enabled? + true else show_when_completed = SiteSetting.value('show_model_solutions_when_exercise_completed') show_when_expired = SiteSetting.value('show_model_solutions_when_exercise_expired') diff --git a/spec/channels/course_template_refresh_channel_spec.rb b/spec/channels/course_template_refresh_channel_spec.rb index 6bb54160c..8f664e9a0 100644 --- a/spec/channels/course_template_refresh_channel_spec.rb +++ b/spec/channels/course_template_refresh_channel_spec.rb @@ -1,8 +1,31 @@ # typed: false # frozen_string_literal: true -# require 'rails_helper' +require 'spec_helper' -# RSpec.describe CourseTemplateRefreshChannel, type: :channel do -# pending "add some examples to (or delete) #{__FILE__}" -# end +describe CourseTemplateRefreshChannel, type: :channel do + let(:course_template) { FactoryBot.create(:course_template) } + let(:organization) { FactoryBot.create(:accepted_organization) } + let!(:course) { FactoryBot.create(:course, organization: organization, course_template: course_template, source_url: course_template.source_url) } + + it 'streams refresh progress to a teacher of a course using the template' do + teacher = FactoryBot.create(:user) + Teachership.create!(user: teacher, organization: organization) + stub_connection current_user: teacher + subscribe courseTemplateId: course_template.id + expect(subscription).to be_confirmed + expect(subscription).to have_stream_from("CourseTemplateRefreshChannel-#{course_template.id}") + end + + it 'streams refresh progress to an admin' do + stub_connection current_user: FactoryBot.create(:admin) + subscribe courseTemplateId: course_template.id + expect(subscription).to be_confirmed + end + + it 'rejects a student' do + stub_connection current_user: FactoryBot.create(:user) + subscribe courseTemplateId: course_template.id + expect(subscription).to be_rejected + end +end diff --git a/spec/controllers/api/v8/core/exercises/details_controller_spec.rb b/spec/controllers/api/v8/core/exercises/details_controller_spec.rb new file mode 100644 index 000000000..2f89c31fe --- /dev/null +++ b/spec/controllers/api/v8/core/exercises/details_controller_spec.rb @@ -0,0 +1,19 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe Api::V8::Core::Exercises::DetailsController, type: :controller do + let(:visible_exercise) { FactoryBot.create(:exercise) } + let(:hidden_course_exercise) { FactoryBot.create(:exercise, course: FactoryBot.create(:course, hidden: true)) } + + before :each do + allow(controller).to receive(:doorkeeper_token).and_return(nil) + end + + it 'leaves out exercises of courses the caller cannot read' do + get :show, params: { ids: "#{visible_exercise.id},#{hidden_course_exercise.id}" } + expect(response).to have_http_status(200) + expect(response.parsed_body['exercises'].pluck('id')).to eq([visible_exercise.id]) + end +end diff --git a/spec/controllers/api/v8/core/submissions/feedback_controller_spec.rb b/spec/controllers/api/v8/core/submissions/feedback_controller_spec.rb index a41e80ddf..467c5da63 100644 --- a/spec/controllers/api/v8/core/submissions/feedback_controller_spec.rb +++ b/spec/controllers/api/v8/core/submissions/feedback_controller_spec.rb @@ -26,6 +26,14 @@ }.to change(FeedbackAnswer, :count).by(1) expect(response.code.to_i).to eq(302) end + + it "can't answer another course's question" do + other_question = FactoryBot.create(:feedback_question) + expect do + post :create, params: { submission_id: submission.id, answers: [{ answer: '2', question_id: other_question.id }] } + end.not_to change(FeedbackAnswer, :count) + expect(response.code.to_i).to eq(422) + end end describe 'As an unauthenticated user' do diff --git a/spec/controllers/api/v8/courses/all_courses_with_this_template_controller_spec.rb b/spec/controllers/api/v8/courses/all_courses_with_this_template_controller_spec.rb new file mode 100644 index 000000000..5ec022f5b --- /dev/null +++ b/spec/controllers/api/v8/courses/all_courses_with_this_template_controller_spec.rb @@ -0,0 +1,21 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe Api::V8::Courses::AllCoursesWithThisTemplateController, type: :controller do + let(:template) { FactoryBot.create(:course_template) } + let!(:course) { FactoryBot.create(:course, course_template: template, source_url: template.source_url) } + let!(:hidden_course) { FactoryBot.create(:course, course_template: template, source_url: template.source_url, hidden: true) } + let(:user) { FactoryBot.create(:user) } + + before :each do + allow(controller).to receive(:doorkeeper_token).and_return(double(resource_owner_id: user.id, acceptable?: true)) + end + + it 'lists only the courses the caller can read' do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(200) + expect(response.parsed_body.pluck('id')).to eq([course.id]) + end +end diff --git a/spec/controllers/courses_controller_spec.rb b/spec/controllers/courses_controller_spec.rb index 0f59d0c56..708471215 100644 --- a/spec/controllers/courses_controller_spec.rb +++ b/spec/controllers/courses_controller_spec.rb @@ -204,6 +204,25 @@ def get_show_json(options = {}, parse_json = true) end end + describe 'GET show_json' do + def get_show_json(course) + @request.env['HTTP_AUTHORIZATION'] = ActionController::HttpAuthentication::Basic.encode_credentials(@user.login, @user.password) + get :show_json, params: { organization_id: @organization.slug, id: course.id, format: 'json', api_version: ApiVersion::API_VERSION } + end + + it 'shows a readable course' do + course = FactoryBot.create(:course, organization: @organization) + get_show_json(course) + expect(response.parsed_body['courses'].pluck('id')).to eq([course.id]) + end + + it 'denies a hidden course' do + course = FactoryBot.create(:course, organization: @organization, hidden: true) + get_show_json(course) + expect(response.code.to_i).to eq(403) + end + end + describe 'POST refresh' do before :each do controller.current_user = FactoryBot.create :user diff --git a/spec/controllers/feedback_answers_controller_spec.rb b/spec/controllers/feedback_answers_controller_spec.rb index ce224462b..bed856618 100644 --- a/spec/controllers/feedback_answers_controller_spec.rb +++ b/spec/controllers/feedback_answers_controller_spec.rb @@ -52,6 +52,17 @@ expect(FeedbackAnswer.count).to eq(0) end + it "should not accept answers to another course's question" do + other_question = FactoryBot.create(:feedback_question, kind: 'text') + params = @valid_params.clone + params[:answers][0][:question_id] = other_question.id + + post :create, params: params + + expect(response).not_to be_successful + expect(FeedbackAnswer.count).to eq(0) + end + it 'should not allow answering on behalf of another user' do bypass_rescue diff --git a/spec/controllers/stats_controller_spec.rb b/spec/controllers/stats_controller_spec.rb index 623d0f7f2..a2cf3c2cd 100644 --- a/spec/controllers/stats_controller_spec.rb +++ b/spec/controllers/stats_controller_spec.rb @@ -32,4 +32,12 @@ def get_submissions(time_params) expect(response.parsed_body).to be_an(Array) end end + describe 'GET course stats of a hidden course' do + it 'is denied to a student' do + course = FactoryBot.create(:course, hidden: true) + controller.current_user = FactoryBot.create(:user) + get :index, params: { organization_id: course.organization.slug, course_id: course.id } + expect(response).to have_http_status(:forbidden) + end + end end diff --git a/spec/models/solution_spec.rb b/spec/models/solution_spec.rb index 6caced002..650995a66 100644 --- a/spec/models/solution_spec.rb +++ b/spec/models/solution_spec.rb @@ -46,6 +46,25 @@ expect(sol).not_to be_visible_to(user) end + describe 'when the course shows model solutions before completion' do + let(:user) { FactoryBot.create(:verified_user) } + let(:ex) { FactoryBot.create(:exercise) } + + before :each do + ex.course.organization.verified = true + ex.course.model_solution_visible_before_completion = true + end + + it 'should be visible' do + expect(ex.solution).to be_visible_to(user) + end + + it 'should not be visible for a hidden exercise' do + ex.hidden = true + expect(ex.solution).not_to be_visible_to(user) + end + end + it 'should not be visible if the exercise is not visible to the user' do show_when_completed(true) show_when_expired(true) From 36b1d6160f90e7f8957d5ba43c38deaf4b4c14ce Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 10:41:53 +0200 Subject: [PATCH 06/11] Harden OAuth tokens, password checks, throttles and CSRF --- app/controllers/api/beta/base_controller.rb | 4 +- app/controllers/api/v8/base_controller.rb | 13 +++- .../api/v8/users/password_reset_controller.rb | 3 +- app/controllers/application_controller.rb | 7 ++ app/controllers/auths_controller.rb | 12 ++-- app/helpers/sessions_helper.rb | 10 +-- app/mailers/password_reset_key_mailer.rb | 5 +- app/services/password_failure_throttle.rb | 47 +++++++++++++ .../reset_link_email.html.erb | 2 +- .../reset_link_email.text.erb | 2 +- config/initializers/rack_attack.rb | 14 ++-- sorbet/rbi/dsl/password_reset_key_mailer.rbi | 10 +-- .../api/v8/users_controller_spec.rb | 2 +- spec/controllers/auths_controller_spec.rb | 20 ++++++ spec/requests/api_access_token_spec.rb | 60 ++++++++++++++++ spec/requests/api_session_csrf_spec.rb | 53 ++++++++++++++ .../password_failure_throttle_spec.rb | 69 +++++++++++++++++++ ...signup_and_password_reset_throttle_spec.rb | 46 +++++++++++++ spec/support/rack_attack.rb | 23 +++++++ 19 files changed, 367 insertions(+), 35 deletions(-) create mode 100644 app/services/password_failure_throttle.rb create mode 100644 spec/requests/api_access_token_spec.rb create mode 100644 spec/requests/api_session_csrf_spec.rb create mode 100644 spec/requests/password_failure_throttle_spec.rb create mode 100644 spec/requests/signup_and_password_reset_throttle_spec.rb create mode 100644 spec/support/rack_attack.rb diff --git a/app/controllers/api/beta/base_controller.rb b/app/controllers/api/beta/base_controller.rb index fee736024..a229ffffd 100644 --- a/app/controllers/api/beta/base_controller.rb +++ b/app/controllers/api/beta/base_controller.rb @@ -35,8 +35,8 @@ def present(hash) private sig { void } def authenticate_user! - if doorkeeper_token - @current_user = T.let(User.find(doorkeeper_token.resource_owner_id), T.nilable(User)) + if (token = doorkeeper_token) + @current_user = T.let(User.find_by(id: token.resource_owner_id), T.nilable(User)) end return if @current_user diff --git a/app/controllers/api/v8/base_controller.rb b/app/controllers/api/v8/base_controller.rb index 1b38d5576..2e823be77 100644 --- a/app/controllers/api/v8/base_controller.rb +++ b/app/controllers/api/v8/base_controller.rb @@ -49,14 +49,21 @@ def present(hash) sig { void } def authenticate_user! return if @current_user - if doorkeeper_token - @current_user = T.let(User.find_by(id: doorkeeper_token.resource_owner_id), T.nilable(User)) + if (token = doorkeeper_token) + @current_user = T.let(User.find_by(id: token.resource_owner_id), T.nilable(User)) raise 'Invalid token' unless @current_user elsif Rails.configuration.x.accept_courses_mooc_fi_tokens @current_user = CoursesMoocFiAuthentication.user_for(request) @auth_source = T.let(:courses_mooc_fi_token, T.nilable(Symbol)) if @current_user end - @current_user ||= user_from_session || Guest.new + @current_user ||= user_from_session_with_verified_csrf || Guest.new + end + + # The session cookie authenticates API calls made from TMC's own pages, so those need the + # CSRF token that the skipped verify_authenticity_token would otherwise demand. + sig { returns(T.nilable(User)) } + def user_from_session_with_verified_csrf + user_from_session if verified_request? end # Before authenticate_user! has run (ApplicationController's own before_actions), this is a Guest. diff --git a/app/controllers/api/v8/users/password_reset_controller.rb b/app/controllers/api/v8/users/password_reset_controller.rb index 8acc23cea..539bd6623 100644 --- a/app/controllers/api/v8/users/password_reset_controller.rb +++ b/app/controllers/api/v8/users/password_reset_controller.rb @@ -26,8 +26,7 @@ def create end key = ActionToken.generate_password_reset_key_for(user) - # TODO: Whitelist origins - PasswordResetKeyMailer.reset_link_email(user, key, params['origin']).deliver_now + PasswordResetKeyMailer.reset_link_email(user, key).deliver_now render json: { success: true } diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f7abd5d4b..7726ecd8a 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -92,6 +92,13 @@ def current_ability @current_ability ||= T.let(::Ability.new(current_user), T.nilable(::Ability)) end + # The request's Doorkeeper access token, or nil when it is missing, revoked or expired. + sig { returns(T.nilable(Doorkeeper::AccessToken)) } + def doorkeeper_token + token = super + token if token&.accessible? + end + sig { void } def set_default_url_options Rails.application.routes.default_url_options[:host] = request.host_with_port diff --git a/app/controllers/auths_controller.rb b/app/controllers/auths_controller.rb index 859f3107c..7f05338b3 100644 --- a/app/controllers/auths_controller.rb +++ b/app/controllers/auths_controller.rb @@ -16,7 +16,7 @@ def show user = User.find_by(login: params[:username]) user ||= User.find_by('lower(email) = ?', params[:username].downcase) # Allows using oauth2 tokens of the new api for authenticating - res = if user && Doorkeeper::AccessToken.find_by(resource_owner_id: user.id, token: params[:session_id]) + res = if user && Doorkeeper::AccessToken.find_by(resource_owner_id: user.id, token: params[:session_id])&.accessible? OK_MESSAGE elsif user && find_session_by_id(params[:session_id])&.belongs_to?(user) OK_MESSAGE @@ -26,13 +26,13 @@ def show return render plain: res end - user = User.find_by(login: params[:username]) - user ||= User.find_by('lower(email) = ?', params[:username].downcase) - msg = if user && params[:password].present? && user.has_password?(params[:password]) - OK_MESSAGE + status = if params[:username].present? && params[:password].present? + User.authenticate_with_status(params[:username].to_s, params[:password].to_s).last else - FAIL_MESSAGE + :rejected end + PasswordFailureThrottle.record_failure(request) if status == :rejected + msg = status == :accepted ? OK_MESSAGE : FAIL_MESSAGE respond_to do |format| format.any(:html, :text) do # Work around bug in HTTP library used by tmc-comet and accept HTML mime type diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 5ee221505..329238ab8 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -44,9 +44,11 @@ def user_from_session sig { returns(T.nilable(User)) } def user_from_basic_auth - if request&.authorization - username, password = ActionController::HttpAuthentication::Basic.user_name_and_password(request) - User.authenticate(username, password) if username && password - end + return unless request && ActionController::HttpAuthentication::Basic.has_basic_credentials?(request) + username, password = ActionController::HttpAuthentication::Basic.user_name_and_password(request) + return unless username && password + user, status = User.authenticate_with_status(username, password) + PasswordFailureThrottle.record_failure(request) if status == :rejected + user if status == :accepted end end diff --git a/app/mailers/password_reset_key_mailer.rb b/app/mailers/password_reset_key_mailer.rb index 17f780116..f00dd3f04 100644 --- a/app/mailers/password_reset_key_mailer.rb +++ b/app/mailers/password_reset_key_mailer.rb @@ -3,10 +3,9 @@ class PasswordResetKeyMailer < ActionMailer::Base extend T::Sig - sig { params(user: User, key: ActionToken, origin: T.nilable(String)).void } - def reset_link_email(user, key, origin = nil) + sig { params(user: User, key: ActionToken).void } + def reset_link_email(user, key) @user = T.let(user, T.nilable(User)) - @origin = T.let(origin, T.nilable(String)) settings = SiteSetting.value('emails') subject = 'Reset your mooc.fi account password' diff --git a/app/services/password_failure_throttle.rb b/app/services/password_failure_throttle.rb new file mode 100644 index 000000000..a3fee125f --- /dev/null +++ b/app/services/password_failure_throttle.rb @@ -0,0 +1,47 @@ +# typed: strict +# frozen_string_literal: true + +# Bans an IP from password checks on /auth and HTTP Basic auth after too many wrong passwords. +# Only failures count: NetBeans sends Basic credentials on every request and tmc-comet calls /auth +# constantly, so a request-count throttle would lock out busy classrooms behind one NAT address. +# The limits match the request-count login throttles in config/initializers/rack_attack.rb. +module PasswordFailureThrottle + extend T::Sig + + LIMITS = T.let([ + { maxretry: 20, findtime: 1.minute, bantime: 1.minute }, + { maxretry: 100, findtime: 1.hour, bantime: 1.hour }, + { maxretry: 500, findtime: 1.day, bantime: 1.day } + ].freeze, T::Array[T::Hash[Symbol, T.untyped]]) + + # Rack::Attack passes a Rack::Request, controllers an ActionDispatch::Request. + AnyRequest = T.type_alias { T.any(Rack::Request, ActionDispatch::Request) } + + # Counts a rejected password from the request's IP. Call only when the password was judged wrong, + # never when the check could not be made (courses.mooc.fi unavailable). + sig { params(request: AnyRequest).void } + def self.record_failure(request) + LIMITS.each do |limit| + Rack::Attack::Fail2Ban.filter(discriminator(request, limit), limit) { true } + end + end + + # Whether the request checks a password and its IP is banned from doing so. + sig { params(request: AnyRequest).returns(T::Boolean) } + def self.blocked?(request) + checks_password?(request) && LIMITS.any? { |limit| Rack::Attack::Fail2Ban.banned?(discriminator(request, limit)) } + end + + sig { params(request: AnyRequest).returns(T::Boolean) } + def self.checks_password?(request) + request.get_header('HTTP_AUTHORIZATION').to_s.match?(/\ABasic /i) || + request.path.match?(%r{\A/auth(\.\w+)?\z}) + end + private_class_method :checks_password? + + sig { params(request: AnyRequest, limit: T::Hash[Symbol, T.untyped]).returns(String) } + def self.discriminator(request, limit) + "password-failures:#{limit[:findtime].to_i}:#{request.ip}" + end + private_class_method :discriminator +end diff --git a/app/views/password_reset_key_mailer/reset_link_email.html.erb b/app/views/password_reset_key_mailer/reset_link_email.html.erb index 315848bfa..205778d17 100644 --- a/app/views/password_reset_key_mailer/reset_link_email.html.erb +++ b/app/views/password_reset_key_mailer/reset_link_email.html.erb @@ -4,7 +4,7 @@ -

    Someone requested a password reset for your mooc.fi account <%= @user.email %>.<%= @origin ? " The request originated from '#{@origin}'." : '' %> Please proceed to the following URL to change your password

    +

    Someone requested a password reset for your mooc.fi account <%= @user.email %>. Please proceed to the following URL to change your password

    <%= @url %> diff --git a/app/views/password_reset_key_mailer/reset_link_email.text.erb b/app/views/password_reset_key_mailer/reset_link_email.text.erb index c9d991b56..07f3ceb00 100644 --- a/app/views/password_reset_key_mailer/reset_link_email.text.erb +++ b/app/views/password_reset_key_mailer/reset_link_email.text.erb @@ -1,4 +1,4 @@ -Someone requested a password reset for your mooc.fi account <%= @user.email %>.<% @origin.nil? ? '' : " The request originated from #{@origin}." %> Please proceed to the following URL to change your password +Someone requested a password reset for your mooc.fi account <%= @user.email %>. Please proceed to the following URL to change your password <%= @url %> diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 13560012d..aa9667db8 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -67,17 +67,21 @@ def self.user_identifier_discriminator(access_token) req.post?) end + blocklist('password checks after too many wrong passwords') do |req| + PasswordFailureThrottle.blocked?(req) + end + # Limit the number of account creations to 15 accounts per minute throttle('account creations per minute', limit: 15, period: 1.minute) do |req| req.ip if req.path == '/user' || - (req.path == 'api/v8/users' && + (req.path.match?(%r{\A/api/v8/users(\.\w+)?\z}) && req.post?) end # Limit the number of account creations to 1000 per day throttle('account creations per day', limit: 1000, period: 1.day) do |req| req.ip if req.path == '/user' || - (req.path == 'api/v8/users' && + (req.path.match?(%r{\A/api/v8/users(\.\w+)?\z}) && req.post?) end @@ -105,14 +109,16 @@ def self.user_identifier_discriminator(access_token) end end + PASSWORD_RESET_PATH = T.let(%r{\A/(password_reset_keys|api/v8/users/password_reset)(\.\w+)?\z}, Regexp) + # Limit the number of password resets per ip to 10 per hour throttle('password resets per ip per hour', limit: 10, period: 1.hour) do |req| - req.ip if req.path == '/password_reset_keys' && + req.ip if req.path.match?(PASSWORD_RESET_PATH) && req.post? end # Limit the number of password resets per email to 5 per day throttle('password resets per email per day', limit: 5, period: 1.day) do |req| - req.params['email'] if req.path == '/password_reset_keys' && req.post? + req.params['email'].to_s.strip.downcase.presence if req.path.match?(PASSWORD_RESET_PATH) && req.post? end end diff --git a/sorbet/rbi/dsl/password_reset_key_mailer.rbi b/sorbet/rbi/dsl/password_reset_key_mailer.rbi index 15cf8969a..2bb6b5dc5 100644 --- a/sorbet/rbi/dsl/password_reset_key_mailer.rbi +++ b/sorbet/rbi/dsl/password_reset_key_mailer.rbi @@ -10,13 +10,7 @@ class PasswordResetKeyMailer include GeneratedMountedHelpers class << self - sig do - params( - user: ::User, - key: ::ActionToken, - origin: T.nilable(::String) - ).returns(::ActionMailer::MessageDelivery) - end - def reset_link_email(user, key, origin = T.unsafe(nil)); end + sig { params(user: ::User, key: ::ActionToken).returns(::ActionMailer::MessageDelivery) } + def reset_link_email(user, key); end end end diff --git a/spec/controllers/api/v8/users_controller_spec.rb b/spec/controllers/api/v8/users_controller_spec.rb index 030bc3795..955555283 100644 --- a/spec/controllers/api/v8/users_controller_spec.rb +++ b/spec/controllers/api/v8/users_controller_spec.rb @@ -138,7 +138,7 @@ end describe 'PUT update password' do - let!(:token) { double resource_owner_id: user.id, acceptable?: true } + let!(:token) { Doorkeeper::AccessToken.create!(resource_owner_id: user.id, scopes: 'public') } before :each do user.password = 'oldpassword' diff --git a/spec/controllers/auths_controller_spec.rb b/spec/controllers/auths_controller_spec.rb index 7489b0214..4ba9b8996 100644 --- a/spec/controllers/auths_controller_spec.rb +++ b/spec/controllers/auths_controller_spec.rb @@ -6,6 +6,7 @@ describe AuthsController, type: :controller do before :each do @user = FactoryBot.create(:user, login: 'foo', password: 'bar') + allow_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).and_return(true) end it 'tells whether the given user/password is valid or not' do @@ -44,4 +45,23 @@ expect(response).to be_successful expect(response.body).to eq('OK') end + + it 'fails without a username' do + get :show, params: { password: 'bar', format: :text } + expect(response).to be_successful + expect(response.body).to eq('FAIL') + end + + it 'checks the password of an externally managed user with courses.mooc.fi' do + @user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + expect_any_instance_of(User).to receive(:courses_mooc_fi_authentication_status).with('bar').and_return(:rejected) + get :show, params: { username: 'foo', password: 'bar', format: :text } + expect(response.body).to eq('FAIL') + end + + it 'refuses the local password of a user flagged as externally managed without an id' do + @user.update_column(:password_managed_by_courses_mooc_fi, true) + get :show, params: { username: 'foo', password: 'bar', format: :text } + expect(response.body).to eq('FAIL') + end end diff --git a/spec/requests/api_access_token_spec.rb b/spec/requests/api_access_token_spec.rb new file mode 100644 index 000000000..5f74aa14b --- /dev/null +++ b/spec/requests/api_access_token_spec.rb @@ -0,0 +1,60 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'API access tokens', type: :request do + let(:user) { FactoryBot.create(:user) } + let(:application) { Doorkeeper::Application.create!(name: 'Client', redirect_uri: 'https://example.com/callback') } + let!(:token) { Doorkeeper::AccessToken.create!(application: application, resource_owner_id: user.id, scopes: 'public') } + + def bearer + { 'Authorization' => "Bearer #{token.token}" } + end + + describe 'API v8' do + it 'accepts a live token' do + get '/api/v8/users/current', headers: bearer + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(user.id) + end + + it 'rejects a revoked token' do + token.revoke + get '/api/v8/users/current', headers: bearer + expect(response).to have_http_status(:unauthorized) + end + + it 'rejects an expired token' do + token.update!(expires_in: 60, created_at: 2.minutes.ago) + get '/api/v8/users/current', headers: bearer + expect(response).to have_http_status(:unauthorized) + end + end + + describe 'API beta' do + it 'accepts a live token' do + get '/api/beta/participant', headers: bearer + expect(response).to have_http_status(:ok) + end + + it 'rejects a revoked token' do + token.revoke + get '/api/beta/participant', headers: bearer + expect(response).to have_http_status(:unauthorized) + end + end + + describe '/auth with a session_id' do + it 'accepts a live token' do + get '/auth.text', params: { username: user.login, session_id: token.token } + expect(response.body).to eq('OK') + end + + it 'rejects a revoked token' do + token.revoke + get '/auth.text', params: { username: user.login, session_id: token.token } + expect(response.body).to eq('FAIL') + end + end +end diff --git a/spec/requests/api_session_csrf_spec.rb b/spec/requests/api_session_csrf_spec.rb new file mode 100644 index 000000000..db2e0c233 --- /dev/null +++ b/spec/requests/api_session_csrf_spec.rb @@ -0,0 +1,53 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'API v8 CSRF protection', type: :request do + let!(:user) { FactoryBot.create(:user, login: 'student', password: 'secret') } + + around do |example| + ActionController::Base.allow_forgery_protection = true + example.run + ensure + ActionController::Base.allow_forgery_protection = false + end + + def update_current_user(headers = {}) + put '/api/v8/users/current', params: { user: { email: user.email } }, headers: headers + end + + context 'when authenticated with the session cookie' do + before do + allow_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).and_return(true) + get '/login' + post '/sessions', params: { session: { login: 'student', password: 'secret' }, authenticity_token: csrf_token } + end + + def csrf_token + response.body[/ csrf_token) + expect(response).to have_http_status(:ok) + end + + it 'still accepts reads without a CSRF token' do + get '/api/v8/users/current' + expect(response.parsed_body['id']).to eq(user.id) + end + end + + it 'accepts a write authenticated with a bearer token without a CSRF token' do + token = Doorkeeper::AccessToken.create!(resource_owner_id: user.id, scopes: 'public') + update_current_user('Authorization' => "Bearer #{token.token}") + expect(response).to have_http_status(:ok) + end +end diff --git a/spec/requests/password_failure_throttle_spec.rb b/spec/requests/password_failure_throttle_spec.rb new file mode 100644 index 000000000..7af0d504d --- /dev/null +++ b/spec/requests/password_failure_throttle_spec.rb @@ -0,0 +1,69 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Password failure throttle', type: :request do + include_context 'with Rack::Attack' + + let!(:user) { FactoryBot.create(:user, login: 'student', password: 'right') } + let(:ip) { '203.0.113.7' } + + before do + allow_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).and_return(true) + end + + def auth(password, from: ip) + post '/auth.text', params: { username: 'student', password: password }, env: { 'REMOTE_ADDR' => from } + end + + def basic(password, from: ip) + credentials = ActionController::HttpAuthentication::Basic.encode_credentials('student', password) + get '/', headers: { 'Authorization' => credentials }, env: { 'REMOTE_ADDR' => from } + end + + it 'blocks /auth from an IP after 20 wrong passwords' do + 20.times do + auth('wrong') + expect(response.body).to eq('FAIL') + end + + auth('right') + expect(response).to have_http_status(:forbidden) + + auth('right', from: '198.51.100.1') + expect(response.body).to eq('OK') + end + + it 'blocks HTTP Basic auth from an IP after 20 wrong passwords' do + 20.times do + basic('wrong') + expect(response).to have_http_status(:ok) + end + + basic('right') + expect(response).to have_http_status(:forbidden) + + get '/', env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:ok) + end + + it 'does not count successful password checks' do + 50.times do + auth('right') + expect(response.body).to eq('OK') + basic('right') + expect(response).to have_http_status(:ok) + end + end + + it 'does not count checks courses.mooc.fi could not make' do + user.update!(password_managed_by_courses_mooc_fi: true, courses_mooc_fi_user_id: SecureRandom.uuid) + allow_any_instance_of(User).to receive(:courses_mooc_fi_authentication_status).and_return(:unavailable) + 25.times { auth('right') } + + allow_any_instance_of(User).to receive(:courses_mooc_fi_authentication_status).and_return(:accepted) + auth('right') + expect(response.body).to eq('OK') + end +end diff --git a/spec/requests/signup_and_password_reset_throttle_spec.rb b/spec/requests/signup_and_password_reset_throttle_spec.rb new file mode 100644 index 000000000..63b0180ff --- /dev/null +++ b/spec/requests/signup_and_password_reset_throttle_spec.rb @@ -0,0 +1,46 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Signup and password reset throttles', type: :request do + include_context 'with Rack::Attack' + + let(:ip) { '203.0.113.9' } + + it 'limits API password resets per email' do + FactoryBot.create(:user, email: 'student@example.com') + 5.times do + post '/api/v8/users/password_reset', params: { email: 'student@example.com' }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:ok) + end + + post '/api/v8/users/password_reset.json', params: { email: 'Student@example.com' }, env: { 'REMOTE_ADDR' => '198.51.100.2' } + expect(response).to have_http_status(:service_unavailable) + end + + it 'limits password resets per IP across the web form and the API' do + 5.times { |i| post '/password_reset_keys', params: { email: "nobody#{i}@example.com" }, env: { 'REMOTE_ADDR' => ip } } + 5.times { |i| post '/api/v8/users/password_reset', params: { email: "noone#{i}@example.com" }, env: { 'REMOTE_ADDR' => ip } } + expect(response).to have_http_status(:not_found) + + post '/api/v8/users/password_reset', params: { email: 'another@example.com' }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:service_unavailable) + end + + it 'limits API signups per IP' do + 15.times { post '/api/v8/users', params: { user: { email: '' } }, env: { 'REMOTE_ADDR' => ip } } + expect(response).not_to have_http_status(:service_unavailable) + + post '/api/v8/users', params: { user: { email: '' } }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:service_unavailable) + end + + it 'leaves the client-supplied origin out of the reset email' do + FactoryBot.create(:user, email: 'student@example.com') + expect do + post '/api/v8/users/password_reset', params: { email: 'student@example.com', origin: 'Call +1 555 0100 to verify' } + end.to change { ActionMailer::Base.deliveries.size }.by(1) + expect(ActionMailer::Base.deliveries.last.body.encoded).not_to include('555 0100') + end +end diff --git a/spec/support/rack_attack.rb b/spec/support/rack_attack.rb new file mode 100644 index 000000000..388dd11df --- /dev/null +++ b/spec/support/rack_attack.rb @@ -0,0 +1,23 @@ +# typed: false +# frozen_string_literal: true + +# Runs request specs through Rack::Attack with a fresh counter store and a frozen clock. +RSpec.shared_context 'with Rack::Attack' do + include ActiveSupport::Testing::TimeHelpers + + around do |example| + store = Rack::Attack.cache.store + Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new + Rack::Attack.enabled = true + # Fixed so that the counters' time windows do not roll over mid-example. + travel_to(Time.zone.local(2026, 1, 1, 12, 0, 0)) { example.run } + ensure + Rack::Attack.enabled = false + Rack::Attack.cache.store = store + end + + # config.ru mounts Rack::Attack outside the Rails middleware stack. + def app + Rack::Attack.new(Rails.application) + end +end From e27d2e4a7147f4013c42b014157f13b1afb0b99d Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 11:11:02 +0200 Subject: [PATCH 07/11] Harden sessions, flash, logs, CSV exports and recorded IPs --- .../core/exercises/submissions_controller.rb | 2 +- app/controllers/certificates_controller.rb | 2 +- app/controllers/emails_controller.rb | 10 ++++++++- app/controllers/exercises_controller.rb | 12 +++++----- app/helpers/bootstrap_flash_helper.rb | 4 ++-- app/helpers/sessions_helper.rb | 3 +++ app/services/csv_cell.rb | 15 +++++++++++++ app/views/layouts/bare.html.erb | 2 +- app/views/points/index.csv.erb | 4 ++-- config/application.rb | 7 ++++-- .../initializers/filter_parameter_logging.rb | 7 ------ config/initializers/rack_attack.rb | 2 +- config/initializers/session_store.rb | 2 +- spec/controllers/emails_controller_spec.rb | 9 ++++++++ spec/helpers/bootstrap_flash_helper_spec.rb | 16 ++++++++++++++ spec/helpers/sessions_helper_spec.rb | 3 +++ spec/requests/flash_html_spec.rb | 19 ++++++++++++++++ spec/requests/session_cookie_spec.rb | 22 ++++++++++++++++++- 18 files changed, 114 insertions(+), 27 deletions(-) create mode 100644 app/services/csv_cell.rb delete mode 100644 config/initializers/filter_parameter_logging.rb create mode 100644 spec/helpers/bootstrap_flash_helper_spec.rb create mode 100644 spec/requests/flash_html_spec.rb diff --git a/app/controllers/api/v8/core/exercises/submissions_controller.rb b/app/controllers/api/v8/core/exercises/submissions_controller.rb index c71273f58..7f4bdb184 100644 --- a/app/controllers/api/v8/core/exercises/submissions_controller.rb +++ b/app/controllers/api/v8/core/exercises/submissions_controller.rb @@ -100,7 +100,7 @@ def create Time.at(params[:client_time].to_i) end, client_nanotime: params[:client_nanotime], - client_ip: request.env['HTTP_X_FORWARDED_FOR'] || request.remote_ip, + client_ip: request.remote_ip, processing_priority: low_priority ? -100 : 0 ) diff --git a/app/controllers/certificates_controller.rb b/app/controllers/certificates_controller.rb index 479293108..f6f4cf502 100644 --- a/app/controllers/certificates_controller.rb +++ b/app/controllers/certificates_controller.rb @@ -37,7 +37,7 @@ def create authorize! :create, @certificate if @certificate.save - redirect_to participant_certificates_path(@user), notice: "Certificate successfully created. View certificate" + redirect_to participant_certificates_path(@user), notice: helpers.safe_join(['Certificate successfully created. ', helpers.link_to('View certificate', certificate_path(@certificate))]) else render action: :new end diff --git a/app/controllers/emails_controller.rb b/app/controllers/emails_controller.rb index 1c1f78e9b..9158d02bc 100644 --- a/app/controllers/emails_controller.rb +++ b/app/controllers/emails_controller.rb @@ -1,6 +1,8 @@ # typed: strict # frozen_string_literal: true +require 'csv' + # Displays the raw list of participant e-mails, useful for mass-mailing scripts. class EmailsController < ApplicationController extend T::Sig @@ -47,7 +49,13 @@ def index_course respond_to do |format| format.html format.text { render plain: students.map { |s| s.email.to_s }.join("\n") } - format.csv { render plain: "Username,Email\n" + students.map { |s| "#{s.username},#{s.email}" }.join("\n") } + format.csv do + csv = CSV.generate do |rows| + rows << %w[Username Email] + students.each { |s| rows << [s.username, s.email].map { |cell| CsvCell.escape(cell) } } + end + render plain: csv + end end end end diff --git a/app/controllers/exercises_controller.rb b/app/controllers/exercises_controller.rb index c9daeb2e2..fdef7fd08 100644 --- a/app/controllers/exercises_controller.rb +++ b/app/controllers/exercises_controller.rb @@ -89,12 +89,10 @@ def toggle_submission_result_visibility @exercise = Exercise.find(params[:id]) authorize! :toggle_submission_result_visibility, @exercise.course @exercise.toggle_submission_result_visiblity - status = @exercise.hide_submission_results ? 'hidden' : 'visible' - redirect_to exercise_path(@exercise), notice: "

    Exercise status update:

    -
      -
    • Submission results are now #{status}
    • -
    • Points are now #{status}
    • -
    • Model solutions is now #{status}
    • -
    " + status = helpers.tag.b(@exercise.hide_submission_results ? 'hidden' : 'visible') + changes = ['Submission results are now', 'Points are now', 'Model solutions is now'].map do |what| + helpers.tag.li(helpers.safe_join([what, ' ', status])) + end + redirect_to exercise_path(@exercise), notice: helpers.safe_join([helpers.tag.h4('Exercise status update:'), helpers.tag.ul(helpers.safe_join(changes))]) end end diff --git a/app/helpers/bootstrap_flash_helper.rb b/app/helpers/bootstrap_flash_helper.rb index a732f5f0e..a411d8fe5 100644 --- a/app/helpers/bootstrap_flash_helper.rb +++ b/app/helpers/bootstrap_flash_helper.rb @@ -20,10 +20,10 @@ def bootstrap_flash Array(message).each do |msg| text = content_tag(:div, content_tag(:button, raw('×'), :class => 'close', 'data-dismiss' => 'alert') + - msg.html_safe, class: "alert alert-#{type}") + msg, class: "alert alert-#{type}") flash_messages << text if message end end - flash_messages.join("\n").html_safe + safe_join(flash_messages, "\n") end end diff --git a/app/helpers/sessions_helper.rb b/app/helpers/sessions_helper.rb index 329238ab8..37e7821b2 100644 --- a/app/helpers/sessions_helper.rb +++ b/app/helpers/sessions_helper.rb @@ -5,6 +5,9 @@ module SessionsHelper extend T::Sig sig { params(user: User).returns(User) } def sign_in(user) + return_to = session[:return_to] + reset_session + session[:return_to] = return_to if return_to session[:user_id] = user.id @current_user = T.let(user, T.nilable(User)) user diff --git a/app/services/csv_cell.rb b/app/services/csv_cell.rb new file mode 100644 index 000000000..ecec3717b --- /dev/null +++ b/app/services/csv_cell.rb @@ -0,0 +1,15 @@ +# typed: strict +# frozen_string_literal: true + +# Prepares values for CSV exports that teachers open in spreadsheet programs. +module CsvCell + extend T::Sig + + FORMULA_START = T.let(/\A[=+\-@\t\r]/, Regexp) + + # The value, prefixed with ' when it is a string a spreadsheet would evaluate as a formula. + sig { params(value: T.untyped).returns(T.untyped) } + def self.escape(value) + value.is_a?(String) && value.match?(FORMULA_START) ? "'#{value}" : value + end +end diff --git a/app/views/layouts/bare.html.erb b/app/views/layouts/bare.html.erb index b48ea1db2..47876e9b3 100644 --- a/app/views/layouts/bare.html.erb +++ b/app/views/layouts/bare.html.erb @@ -17,7 +17,7 @@
    <% for msg_type in [:notice, :warning, :error, :alert] %> <% if flash[msg_type] %> -
    <%= raw(flash[msg_type]) %>
    +
    <%= flash[msg_type] %>
    <% end %> <% end %> diff --git a/app/views/points/index.csv.erb b/app/views/points/index.csv.erb index 377bc1211..b6e790d1f 100644 --- a/app/views/points/index.csv.erb +++ b/app/views/points/index.csv.erb @@ -6,7 +6,7 @@ arr << :email if can?(:teach, @course) arr << :username if can?(:teach, @course) && params[:include_username] arr += @summary[:sheets].map {|sheet| sheet[:name] } + ["Total"] - csv << arr + csv << arr.map { |cell| CsvCell.escape(cell) } @summary[:users].each do |user| points = @summary[:sheets].map do |sheet| @@ -24,6 +24,6 @@ arr << user.email if can?(:teach, @course) arr << user.login if can?(:teach, @course) && params[:include_username] arr += points + [total] - csv << arr + csv << arr.map { |cell| CsvCell.escape(cell) } end end.html_safe %> \ No newline at end of file diff --git a/config/application.rb b/config/application.rb index 14331a97c..ac0809804 100644 --- a/config/application.rb +++ b/config/application.rb @@ -32,7 +32,10 @@ class Application < Rails::Application # config.i18n.default_locale = :de # Configure sensitive parameters which will be filtered from the log file. - config.filter_parameters += %i[password session_id api_password submission_file return_file test_output stdout stderr vm_log] + config.filter_parameters += %i[password session_id api_password submission_file return_file test_output stdout stderr vm_log + token access_token refresh_token client_secret secret _key crypt otp salt] + # Exact, so that other keys containing "code" stay readable. + config.filter_parameters << /\Acode\z/ # Do not swallow errors in after_commit/after_rollback callbacks. # config.eager_load_paths << Rails.root.join('lib') @@ -45,7 +48,7 @@ class Application < Rails::Application config.x.accept_courses_mooc_fi_tokens = ENV['ACCEPT_COURSES_MOOC_FI_TOKENS'] == 'true' - config.middleware.insert_before 0, Rack::Cors, debug: true, logger: (-> { Rails.logger }) do + config.middleware.insert_before 0, Rack::Cors, debug: false, logger: (-> { Rails.logger }) do T.bind(self, Rack::Cors) allow do origins '*' diff --git a/config/initializers/filter_parameter_logging.rb b/config/initializers/filter_parameter_logging.rb deleted file mode 100644 index c787b99f3..000000000 --- a/config/initializers/filter_parameter_logging.rb +++ /dev/null @@ -1,7 +0,0 @@ -# typed: strict -# frozen_string_literal: true - -# Be sure to restart your server when you modify this file. - -# Configure sensitive parameters which will be filtered from the log file. -Rails.application.config.filter_parameters += [:password] diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index aa9667db8..644723f95 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -6,7 +6,7 @@ LOGGER = T.let(Logger.new('log/rack-attack.log'), Logger) ActiveSupport::Notifications.subscribe('rack.attack') do |_name, _start, _finish, _request_id, req| req = req[:request] - msg = [req.env['rack.attack.match_type'], req.ip, req.request_method, req.fullpath, ('"' + req.user_agent.to_s + '"')].join(' ') + msg = [req.env['rack.attack.match_type'], req.ip, req.request_method, req.path, ('"' + req.user_agent.to_s + '"')].join(' ') if %i[throttle blocklist].include?(req.env['rack.attack.match_type']) LOGGER.error(msg) else diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb index 88bbe7f42..fff5eb550 100644 --- a/config/initializers/session_store.rb +++ b/config/initializers/session_store.rb @@ -3,4 +3,4 @@ # Be sure to restart your server when you modify this file. -Rails.application.config.session_store :active_record_store, key: SiteSetting.value(:session_cookie_key), httponly: false +Rails.application.config.session_store :active_record_store, key: SiteSetting.value(:session_cookie_key), secure: Rails.env.production? diff --git a/spec/controllers/emails_controller_spec.rb b/spec/controllers/emails_controller_spec.rb index d8ab7bc1d..400ba886d 100644 --- a/spec/controllers/emails_controller_spec.rb +++ b/spec/controllers/emails_controller_spec.rb @@ -66,6 +66,15 @@ expect(assigns(:students)).not_to include(@user3) end + it 'exports a CSV that spreadsheets do not evaluate as formulas' do + @user1.update_column(:login, '=HYPERLINK("http://example.com","x")') + controller.current_user = @teacher + get :index, params: { organization_id: @organization.slug, id: @course.id }, format: :csv + rows = CSV.parse(response.body) + expect(rows.first).to eq(%w[Username Email]) + expect(rows).to include(["'=HYPERLINK(\"http://example.com\",\"x\")", @user1.email]) + end + describe 'HTML page' do render_views diff --git a/spec/helpers/bootstrap_flash_helper_spec.rb b/spec/helpers/bootstrap_flash_helper_spec.rb new file mode 100644 index 000000000..58f824f51 --- /dev/null +++ b/spec/helpers/bootstrap_flash_helper_spec.rb @@ -0,0 +1,16 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe BootstrapFlashHelper, type: :helper do + it 'escapes plain-text messages' do + flash[:notice] = 'Teacher removed from organization' + expect(helper.bootstrap_flash).to include('Teacher <script>alert(1)</script> removed') + end + + it 'keeps HTML that was built as safe' do + flash[:notice] = helper.safe_join(['Created. ', helper.link_to('View', '/certificates/1')]) + expect(helper.bootstrap_flash).to include('Created. View') + end +end diff --git a/spec/helpers/sessions_helper_spec.rb b/spec/helpers/sessions_helper_spec.rb index 8784f0723..23b758ca5 100644 --- a/spec/helpers/sessions_helper_spec.rb +++ b/spec/helpers/sessions_helper_spec.rb @@ -6,6 +6,9 @@ describe SessionsHelper, type: :helper do let!(:user) { FactoryBot.create(:user) } + # Helper specs run without a controller, which is what defines reset_session. + before { allow(self).to receive(:reset_session) } + describe '#sign_in' do it 'should assign user to @current_user' do expect(sign_in(user)).to eq(@current_user) diff --git a/spec/requests/flash_html_spec.rb b/spec/requests/flash_html_spec.rb new file mode 100644 index 000000000..245ed5e3c --- /dev/null +++ b/spec/requests/flash_html_spec.rb @@ -0,0 +1,19 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe 'HTML flash messages', type: :request do + it 'survive the redirect through the session' do + FactoryBot.create(:admin, login: 'admin', password: 'secret') + exercise = FactoryBot.create(:exercise) + allow_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).and_return(true) + post '/sessions', params: { session: { login: 'admin', password: 'secret' } } + + post "/org/#{exercise.course.organization.slug}/exercises/#{exercise.id}/toggle_submission_result_visibility" + follow_redirect! + + expect(response.body).to include('

    Exercise status update:

    ') + expect(response.body).to include('Points are now hidden') + end +end diff --git a/spec/requests/session_cookie_spec.rb b/spec/requests/session_cookie_spec.rb index c96ab4291..2218aaf77 100644 --- a/spec/requests/session_cookie_spec.rb +++ b/spec/requests/session_cookie_spec.rb @@ -4,9 +4,29 @@ require 'spec_helper' describe 'Session cookie', type: :request do + def session_cookie + Array(response.headers['Set-Cookie']).join("\n").lines.find { |line| line.start_with?("#{SiteSetting.value(:session_cookie_key)}=") } + end + it 'is SameSite=Lax so cross-site links into OAuth authorization keep the login' do get '/login' - session_cookie = Array(response.headers['Set-Cookie']).join("\n").lines.find { |line| line.start_with?("#{SiteSetting.value(:session_cookie_key)}=") } expect(session_cookie).to match(/samesite=lax/i) end + + it 'is HttpOnly' do + get '/login' + expect(session_cookie).to match(/httponly/i) + end + + it 'gets a new session id on sign-in and keeps the return path' do + FactoryBot.create(:user, login: 'student', password: 'secret') + allow_any_instance_of(User).to receive(:post_new_user_to_courses_mooc_fi).and_return(true) + get '/login', params: { return_to: '/org' } + id_before = session.id.to_s + + post '/sessions', params: { session: { login: 'student', password: 'secret' } } + + expect(session.id.to_s).not_to eq(id_before) + expect(response).to redirect_to('/org') + end end From 29c60c7d772210849b5ec8a43caab392c596e9ca Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Mon, 9 Nov 2026 11:21:59 +0200 Subject: [PATCH 08/11] Download tmc-langs safely and fix security review findings --- .../rust_langs_downloader_task.rb | 82 +++++++++++--- app/controllers/api/v8/base_controller.rb | 18 ++- .../v8/core/exercises/details_controller.rb | 7 +- .../core/submissions/feedback_controller.rb | 3 +- .../api/v8/courses/submissions_controller.rb | 2 +- .../courses/users/submissions_controller.rb | 2 +- .../exercises/users/submissions_controller.rb | 2 +- .../courses/submissions_controller.rb | 2 +- .../courses/users/submissions_controller.rb | 2 +- app/controllers/emails_controller.rb | 4 +- app/controllers/exercise_status_controller.rb | 9 +- .../feedback_answers_controller.rb | 3 +- app/controllers/participants_controller.rb | 4 +- app/controllers/points_controller.rb | 13 ++- app/controllers/submissions_controller.rb | 2 +- app/helpers/html_prelude_helper.rb | 2 + app/models/awarded_point.rb | 14 ++- app/models/course.rb | 13 +++ app/models/exercise.rb | 11 +- app/models/feedback_answer.rb | 7 ++ app/models/submission.rb | 18 ++- app/services/csv_cell.rb | 8 ++ app/views/points/index.csv.erb | 6 +- config/brakeman.ignore | 103 ------------------ config/environments/production.rb | 3 + config/initializers/rack_attack.rb | 36 +++++- lib/safe_unzipper.rb | 16 ++- lib/submission_list.rb | 3 +- lib/system_commands.rb | 5 +- .../rust_langs_downloader_task_spec.rb | 68 ++++++++++++ .../core/exercises/details_controller_spec.rb | 36 ++++++ .../exercises/points_controller_spec.rb | 21 ++++ .../api/v8/courses/points_controller_spec.rb | 68 ++++++++++-- .../courses/users/points_controller_spec.rb | 12 +- .../courses/points_controller_spec.rb | 9 +- .../courses/users/points_controller_spec.rb | 18 +-- .../exercise_status_controller_spec.rb | 46 +++++++- .../participants_controller_spec.rb | 10 ++ spec/controllers/points_controller_spec.rb | 14 +++ .../controllers/submission_controller_spec.rb | 11 ++ spec/lib/safe_unzipper_spec.rb | 9 ++ spec/requests/basic_auth_challenge_spec.rb | 10 +- ...signup_and_password_reset_throttle_spec.rb | 58 ++++++++++ 43 files changed, 589 insertions(+), 201 deletions(-) create mode 100644 spec/background_tasks/rust_langs_downloader_task_spec.rb diff --git a/app/background_tasks/rust_langs_downloader_task.rb b/app/background_tasks/rust_langs_downloader_task.rb index 868a563c8..e713a694a 100644 --- a/app/background_tasks/rust_langs_downloader_task.rb +++ b/app/background_tasks/rust_langs_downloader_task.rb @@ -1,14 +1,32 @@ # typed: strict # frozen_string_literal: true +require 'digest' require 'rust_langs_cli_executor' class RustLangsDownloaderTask extend T::Sig - @@rust_langs_version = T.let('0.39.8', String) + + # Raised when wget fails to download the tmc-langs binary. + class DownloadFailed < StandardError; end + + # Raised when a downloaded tmc-langs binary does not match RUST_LANGS_SHA256. + class ChecksumMismatch < StandardError; end + + # Raised when a downloaded tmc-langs binary with the right checksum cannot run on this host. + class BinaryDoesNotRun < StandardError; end + + # Bump together: the hash is that of the x86_64 Linux binary of this version, as published + # next to it at download.mooc.fi in tmc-langs-cli-x86_64-unknown-linux-gnu-.sha256. + RUST_LANGS_VERSION = '0.39.8' + RUST_LANGS_SHA256 = '0c39368c5c70fb89ae30fe38619c1870522ee512ee67f6f59ab8e5a2dfd45f73' + + FIRST_RETRY_DELAY_SECONDS = 60 + MAX_RETRY_DELAY_SECONDS = 3600 sig { void } def initialize + @retry_delay_seconds = T.let(nil, T.nilable(Integer)) end sig { void } @@ -16,26 +34,31 @@ def run Rails.logger.info('Creating directory vendor/tmc-langs-rust') unless File.directory?('vendor/tmc-langs-rust') Dir.mkdir('vendor/tmc-langs-rust') unless File.directory?('vendor/tmc-langs-rust') - executable = "vendor/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version}" + executable = "vendor/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{RUST_LANGS_VERSION}" unless File.exist?(executable) previous = nil previous = File.readlink('vendor/tmc-langs-rust/current') if File.exist? 'vendor/tmc-langs-rust/current' previous = nil if previous == executable - Rails.logger.info("Downloading tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version}") - `wget https://download.mooc.fi/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version} -O #{executable}` - `chmod +x #{executable}` - - command_output = `#{executable} --help` - unless command_output.include?('tmc-langs') - Rails.logger.error("Downloaded file did not work, removing #{executable}") - File.delete(executable) - return + Rails.logger.info("Downloading tmc-langs-cli-x86_64-unknown-linux-gnu-#{RUST_LANGS_VERSION}") + download = "#{executable}.download" + begin + download!(download) + verify_checksum!(download) + File.chmod(0o755, download) + verify_runs!(download) + rescue DownloadFailed, ChecksumMismatch, BinaryDoesNotRun => e + FileUtils.rm_f(download) + Rails.logger.error(e.message) + @retry_delay_seconds = @retry_delay_seconds ? [@retry_delay_seconds * 2, MAX_RETRY_DELAY_SECONDS].min : FIRST_RETRY_DELAY_SECONDS + raise end + @retry_delay_seconds = nil + File.rename(download, executable) Dir.chdir('vendor/tmc-langs-rust') do - File.symlink("tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version}", 'new-current') + File.symlink("tmc-langs-cli-x86_64-unknown-linux-gnu-#{RUST_LANGS_VERSION}", 'new-current') File.rename('new-current', 'current') end Rails.logger.info('TMC langs rust downloaded') @@ -50,8 +73,41 @@ def run end end + # Seconds until the daemon runs the task again: 1 normally, doubling from FIRST_RETRY_DELAY_SECONDS + # up to MAX_RETRY_DELAY_SECONDS after consecutive failed downloads. sig { returns(Integer) } def wait_delay - 1 + @retry_delay_seconds || 1 end + + private + # Downloads the pinned version to `path`, raising DownloadFailed if wget fails. + sig { params(path: String).void } + def download!(path) + return if system('wget', "https://download.mooc.fi/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{RUST_LANGS_VERSION}", '-O', path) + + raise DownloadFailed, "wget failed to download tmc-langs #{RUST_LANGS_VERSION}: #{Process.last_status.inspect}" + end + + # Raises BinaryDoesNotRun unless `path` runs and identifies itself as tmc-langs. + sig { params(path: String).void } + def verify_runs!(path) + command_output = begin + IO.popen([path, '--help'], &:read) + rescue SystemCallError + '' + end + return if command_output.include?('tmc-langs') + + raise BinaryDoesNotRun, "Downloaded tmc-langs #{RUST_LANGS_VERSION} did not run; removed it" + end + + # Raises ChecksumMismatch unless `path` holds the pinned binary. + sig { params(path: String).void } + def verify_checksum!(path) + actual = File.file?(path) ? Digest::SHA256.file(path).hexdigest : nil + return if actual == RUST_LANGS_SHA256 + + raise ChecksumMismatch, "tmc-langs #{RUST_LANGS_VERSION} download has SHA-256 #{actual || 'none'}, expected #{RUST_LANGS_SHA256}; removed it" + end end diff --git a/app/controllers/api/v8/base_controller.rb b/app/controllers/api/v8/base_controller.rb index 2e823be77..6914fc65a 100644 --- a/app/controllers/api/v8/base_controller.rb +++ b/app/controllers/api/v8/base_controller.rb @@ -77,15 +77,25 @@ def current_ability @current_ability ||= T.let(::Ability.new(current_user, auth_source: @auth_source), T.nilable(::Ability)) end - # Narrows `points` (awarded points of `course`) to what the caller may see. - # Raises CanCan::AccessDenied unless the caller may read the course and see its points. + # Narrows `points` (awarded points of `course`) to what the caller may see. Teachers see all; + # anyone else only their own, none in exam mode or as a guest, and never those of exercises + # that are hidden or hiding their results. + # Raises CanCan::AccessDenied unless the caller may read the course. sig { params(course: Course, points: ActiveRecord::Relation).returns(ActiveRecord::Relation) } def visible_awarded_points(course, points) authorize! :read, course - authorize! :see_points, course return points if can?(:teach, course) + return points.none if current_user.guest? || !can?(:see_points, course) hidden_exercises = course.exercises.where(hidden: true).or(course.exercises.where(hide_submission_results: true)) - points.where.not(name: AvailablePoint.where(exercise_id: hidden_exercises.select(:id)).select(:name)) + points.where(user_id: current_user.id) + .where.not(name: AvailablePoint.where(exercise_id: hidden_exercises.select(:id)).select(:name)) + end + + # The API list fields of those `submissions` the current user may read. + sig { params(submissions: ActiveRecord::Relation).returns(T::Array[T::Hash[Symbol, T.untyped]]) } + def readable_submission_fields(submissions) + readable = submissions.preload(course: :organization).select { |sub| sub.readable_by?(current_user) } + Submission.filter_fields!(readable, current_user) end sig { params(messages: T.any(String, T::Array[String])).returns(T::Hash[Symbol, T::Array[String]]) } diff --git a/app/controllers/api/v8/core/exercises/details_controller.rb b/app/controllers/api/v8/core/exercises/details_controller.rb index c03761641..8fea861db 100644 --- a/app/controllers/api/v8/core/exercises/details_controller.rb +++ b/app/controllers/api/v8/core/exercises/details_controller.rb @@ -49,9 +49,10 @@ def show exercise_ids = params[:ids] return respond_not_found('Query param ids is empty. Example: ?ids=1,2,3') if exercise_ids.blank? || exercise_ids.empty? - exercises = Exercise.where(id: exercise_ids.split(',')).includes(:course).to_a - readable_courses = exercises.filter_map(&:course).uniq.select { |course| can?(:read, course) } - exercises = exercises.select { |exercise| readable_courses.include?(exercise.course) } + exercises = Exercise.where(id: exercise_ids.split(',')).includes(course: :organization).to_a + exercises = exercises.group_by(&:course).flat_map do |course, course_exercises| + course && can?(:read, course) ? course.exercises_visible_to(current_user, course_exercises) : [] + end data = exercises.map do |exercise| { id: exercise.id, diff --git a/app/controllers/api/v8/core/submissions/feedback_controller.rb b/app/controllers/api/v8/core/submissions/feedback_controller.rb index 352d6b01a..9c51af367 100644 --- a/app/controllers/api/v8/core/submissions/feedback_controller.rb +++ b/app/controllers/api/v8/core/submissions/feedback_controller.rb @@ -56,8 +56,7 @@ def create end answer_records.each { |record| authorize! :create, record } - course_question_ids = T.must(submission.course).feedback_question_ids - if answer_records.any? { |record| course_question_ids.exclude?(record.feedback_question_id) } + if FeedbackAnswer.any_outside_course?(answer_records, T.must(submission.course)) return respond_with_error('Feedback question not found in this course', 422) end begin diff --git a/app/controllers/api/v8/courses/submissions_controller.rb b/app/controllers/api/v8/courses/submissions_controller.rb index cf683b7ae..f2222e1b4 100644 --- a/app/controllers/api/v8/courses/submissions_controller.rb +++ b/app/controllers/api/v8/courses/submissions_controller.rb @@ -43,7 +43,7 @@ def index unauthorize_guest! course = Course.find_by!(id: params[:course_id]) submissions = Submission.where(course_id: course.id) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) + readable = readable_submission_fields(submissions) authorize! :read, readable present(readable) diff --git a/app/controllers/api/v8/courses/users/submissions_controller.rb b/app/controllers/api/v8/courses/users/submissions_controller.rb index 5d98ac9b6..721d0126c 100644 --- a/app/controllers/api/v8/courses/users/submissions_controller.rb +++ b/app/controllers/api/v8/courses/users/submissions_controller.rb @@ -75,7 +75,7 @@ def index course = Course.find_by!(id: params[:course_id]) params[:user_id] = current_user.id if params[:user_id] == 'current' submissions = Submission.where(course_id: course.id, user_id: params[:user_id]) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) + readable = readable_submission_fields(submissions) authorize! :read, readable present(readable) diff --git a/app/controllers/api/v8/exercises/users/submissions_controller.rb b/app/controllers/api/v8/exercises/users/submissions_controller.rb index 4e489d336..aabffd04d 100644 --- a/app/controllers/api/v8/exercises/users/submissions_controller.rb +++ b/app/controllers/api/v8/exercises/users/submissions_controller.rb @@ -65,7 +65,7 @@ def index exercise = Exercise.find_by!(id: params[:exercise_id]) params[:user_id] = current_user.id if params[:user_id] == 'current' submissions = Submission.where(course_id: exercise.course_id, exercise_name: exercise.name, user_id: params[:user_id]) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) + readable = readable_submission_fields(submissions) authorize! :read, readable present(readable) diff --git a/app/controllers/api/v8/organizations/courses/submissions_controller.rb b/app/controllers/api/v8/organizations/courses/submissions_controller.rb index 93c64d52c..2ad603440 100644 --- a/app/controllers/api/v8/organizations/courses/submissions_controller.rb +++ b/app/controllers/api/v8/organizations/courses/submissions_controller.rb @@ -45,7 +45,7 @@ def index unauthorize_guest! course = Course.find_by!(name: "#{params[:organization_slug]}-#{params[:course_name]}") submissions = Submission.where(course_id: course.id) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) + readable = readable_submission_fields(submissions) authorize_collection :read, readable present(readable) diff --git a/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb b/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb index 139c69e64..e587e6e3e 100644 --- a/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb +++ b/app/controllers/api/v8/organizations/courses/users/submissions_controller.rb @@ -78,7 +78,7 @@ def index course = Course.find_by!(name: "#{params[:organization_slug]}-#{params[:course_name]}") params[:user_id] = current_user.id if params[:user_id] == 'current' submissions = Submission.where(course_id: course.id, user_id: params[:user_id]) - readable = Submission.filter_fields!(submissions.select { |sub| sub.readable_by?(current_user) }, current_user) + readable = readable_submission_fields(submissions) authorize_collection :read, readable present(readable) diff --git a/app/controllers/emails_controller.rb b/app/controllers/emails_controller.rb index 9158d02bc..da2adc031 100644 --- a/app/controllers/emails_controller.rb +++ b/app/controllers/emails_controller.rb @@ -50,9 +50,9 @@ def index_course format.html format.text { render plain: students.map { |s| s.email.to_s }.join("\n") } format.csv do - csv = CSV.generate do |rows| + csv = CsvCell.generate do |rows| rows << %w[Username Email] - students.each { |s| rows << [s.username, s.email].map { |cell| CsvCell.escape(cell) } } + students.each { |s| rows << [s.username, s.email] } end render plain: csv end diff --git a/app/controllers/exercise_status_controller.rb b/app/controllers/exercise_status_controller.rb index fb73aa19a..a74c0ec59 100644 --- a/app/controllers/exercise_status_controller.rb +++ b/app/controllers/exercise_status_controller.rb @@ -8,17 +8,22 @@ class ExerciseStatusController < ApplicationController def show course_id = params[:course_id] user_id = params[:id] + return respond_unauthorized if current_user.guest? course = Course.where(id: course_id).first || Course.where(name: course_id).first return respond_not_found('Course not found') if course.nil? return respond_forbidden unless course.visible_to?(Guest.new) || current_user.administrator? - user = User.where(id: user_id).first || User.where(login: user_id).first + can_teach = can?(:teach, course) + # A numeric login would otherwise resolve to the user with that id. + user = current_user if user_id == current_user.login + user ||= User.where(id: user_id).first || User.where(login: user_id).first + # Checked before the 404 so that students cannot probe which logins exist. + return respond_forbidden unless can_teach || user == current_user return respond_not_found('User not found') if user.nil? user_subs = user.submissions.where(course_id: course.id).to_a.group_by(&:exercise_name) - can_teach = can?(:teach, course) results = {} course.exercises.each do |ex| next unless can_teach || ex._fast_visible? diff --git a/app/controllers/feedback_answers_controller.rb b/app/controllers/feedback_answers_controller.rb index 4fd862bb1..bdb4927d6 100644 --- a/app/controllers/feedback_answers_controller.rb +++ b/app/controllers/feedback_answers_controller.rb @@ -104,8 +104,7 @@ def create answer: answer_hash[:answer]) end answer_records.each { |record| authorize! :create, record } - course_question_ids = T.must(submission.course).feedback_question_ids - if answer_records.any? { |record| course_question_ids.exclude?(record.feedback_question_id) } + if FeedbackAnswer.any_outside_course?(answer_records, T.must(submission.course)) return respond_with_error('Feedback question not found in this course', 422) end diff --git a/app/controllers/participants_controller.rb b/app/controllers/participants_controller.rb index 77dcd3773..caa411739 100644 --- a/app/controllers/participants_controller.rb +++ b/app/controllers/participants_controller.rb @@ -70,7 +70,7 @@ def index render json: index_json_data end format.csv do - render_csv(text: index_csv, filename: 'participants.csv') + render_csv(body: index_csv, filename: 'participants.csv') end end end @@ -234,7 +234,7 @@ def index_csv extra_fields = T.must(@extra_fields) visible_columns = T.must(@visible_columns) group_completion = @group_completion - CSV.generate(force_quotes: true) do |csv| + CsvCell.generate(force_quotes: true) do |csv| title_row = (ordinary_fields + extra_fields.map(&:name)).filter_map { |f| f.humanize if visible_columns.include?(f) } completion_cols = T.let([], T::Array[String]) diff --git a/app/controllers/points_controller.rb b/app/controllers/points_controller.rb index 22f5b693c..6e3b29b06 100644 --- a/app/controllers/points_controller.rb +++ b/app/controllers/points_controller.rb @@ -11,6 +11,7 @@ class PointsController < ApplicationController sig { void } def index @course = Course.find(params[:course_id]) + authorize! :read, @course authorize! :see_points, @course return respond_unauthorized('Authentication required') if current_user.guest? add_course_breadcrumb @@ -59,7 +60,8 @@ def show authorize! :see_points, @course return respond_unauthorized('Authentication required') if current_user.guest? - if can?(:teach, @course) + can_teach = can?(:teach, @course) + if can_teach @user_fields = UserField.all.select(&:show_in_participant_list?) end @@ -69,19 +71,20 @@ def show exercises = Exercise.course_gdocs_sheet_exercises(@course, sheetname, current_user.administrator?).includes(:available_points).order!(:name) @exercises = T.let(exercises, T.nilable(ActiveRecord::Relation)) - users_to_points = AwardedPoint.per_user_in_course_with_sheet(@course, sheetname, hidden: current_user.administrator?) - users_to_points = users_to_points.slice(current_user.login) unless can?(:teach, @course) + users_to_points = AwardedPoint.per_user_in_course_with_sheet(@course, sheetname, hidden: current_user.administrator?, + only_for_user: can_teach ? nil : current_user) @users_to_points = T.let(users_to_points, T.nilable(T::Hash[String, T::Array[String]])) users = @course.users.includes(:user_field_values) - if params[:sort_by] == 'points' + if !can_teach + users = [current_user] + elsif params[:sort_by] == 'points' users = users.sort_by do |u| [-users_to_points.fetch(u.login, []).size, u.login.downcase] end else users.order! end - users = [current_user] unless can?(:teach, @course) @users = T.let(users, T.nilable(T.any(ActiveRecord::Relation, T::Array[User]))) respond_to do |format| format.html diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 59c5dcd15..caa814546 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -157,7 +157,7 @@ def create message_for_reviewer: params[:request_review] ? (params[:message_for_reviewer] || '') : '', client_time: params[:client_time] ? Time.at(params[:client_time].to_i) : nil, client_nanotime: params[:client_nanotime], - client_ip: request.env['HTTP_X_FORWARDED_FOR'] || request.remote_ip, + client_ip: request.remote_ip, processing_priority: low_priority ? -100 : 0 ) diff --git a/app/helpers/html_prelude_helper.rb b/app/helpers/html_prelude_helper.rb index b5f29b212..7ef548169 100644 --- a/app/helpers/html_prelude_helper.rb +++ b/app/helpers/html_prelude_helper.rb @@ -1,6 +1,7 @@ # typed: strict # frozen_string_literal: true +# Renders the HTML preludes that teachers write at the top of exercise files. module HtmlPreludeHelper extend T::Sig @@ -8,6 +9,7 @@ module HtmlPreludeHelper (Rails::HTML5::SafeListSanitizer.allowed_tags + %w[table thead tbody tfoot tr th td caption]).freeze, T::Set[String] ) + # Rails' safe list plus inline styles and table spans; the sanitizer still filters style properties. ALLOWED_ATTRIBUTES = T.let( (Rails::HTML5::SafeListSanitizer.allowed_attributes + %w[style colspan rowspan]).freeze, T::Set[String] diff --git a/app/models/awarded_point.rb b/app/models/awarded_point.rb index d7299a803..7475509f9 100644 --- a/app/models/awarded_point.rb +++ b/app/models/awarded_point.rb @@ -166,14 +166,18 @@ def self.users_in_course_with_sheet(course, sheetname) end # Gets a hash of user to array of point names awarded for exercises of the given sheet - sig { params(course: Course, sheetname: String, opts: T::Hash[Symbol, T::Boolean]).returns(T::Hash[String, T::Array[String]]) } - def self.per_user_in_course_with_sheet(course, sheetname, opts = {}) + sig do + params(course: Course, sheetname: String, hidden: T::Boolean, only_for_user: T.nilable(User)) + .returns(T::Hash[String, T::Array[String]]) + end + def self.per_user_in_course_with_sheet(course, sheetname, hidden: false, only_for_user: nil) users = User.arel_table awarded_points = AwardedPoint.arel_table - sql = per_user_in_course_with_sheet_query(course, sheetname, opts[:hidden]) - .project([users[:login].as('username'), awarded_points[:name].as('name')]) - .to_sql + query = per_user_in_course_with_sheet_query(course, sheetname, hidden) + .project([users[:login].as('username'), awarded_points[:name].as('name')]) + query.where(users[:id].eq(only_for_user.id)) if only_for_user + sql = query.to_sql result = T.let({}, T::Hash[String, T::Array[String]]) ActiveRecord::Base.connection.execute(sql).each do |record| diff --git a/app/models/course.rb b/app/models/course.rb index 4dee5213d..d5a062fc5 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -396,6 +396,19 @@ def unlocked_exercise_names_for(user, exercises_to_check) end.to_set(&:name) end + # The batched form of Exercise#visible_to? for exercises of this course, with a constant + # number of queries for exercises without unlock conditions. + sig { params(user: User, exercises_to_check: T::Array[Exercise]).returns(T::Array[Exercise]) } + def exercises_visible_to(user, exercises_to_check) + return exercises_to_check if user.administrator? || user.teacher?(organization) || user.assistant?(self) + + UncomputedUnlock.resolve(self, user) + unlocked = unlocked_exercise_names_for(user, exercises_to_check) + exercises_to_check.select do |ex| + ex.visible_to_non_staff?(user) { unlocked.include?(ex.name) } + end + end + sig { params(user: User).returns(T::Array[Exercise]) } def unlockable_exercises_for(user) UncomputedUnlock.resolve(self, user) diff --git a/app/models/exercise.rb b/app/models/exercise.rb index 7229cd2a7..ac212a905 100644 --- a/app/models/exercise.rb +++ b/app/models/exercise.rb @@ -203,11 +203,18 @@ def submittable_by?(user) (!expired_for?(user) && !hidden? && published? && !disabled? && !user.guest? && unlocked_for?(user))) end - # Whether a user may see all metadata about the exercise + # Whether a user may see all metadata about the exercise. Course#exercises_visible_to batches this. sig { params(user: User).returns(T::Boolean) } def visible_to?(user) user.administrator? || user.teacher?(T.must(course).organization) || user.assistant?(T.must(course)) || - (_fast_visible? && (unlocked_for?(user) || unlock_spec_obj.permits_unlock_for?(user))) + visible_to_non_staff?(user) { unlocked_for?(user) } + end + + # The part of visible_to? for users who are not course staff; the block tells whether the + # exercise is unlocked for the user, so batch callers can answer it from preloaded unlocks. + sig { params(user: User, unlocked: T.proc.returns(T::Boolean)).returns(T::Boolean) } + def visible_to_non_staff?(user, &unlocked) + _fast_visible? && (yield || unlock_spec_obj.permits_unlock_for?(user)) end sig { returns(T::Boolean) } diff --git a/app/models/feedback_answer.rb b/app/models/feedback_answer.rb index 4623b0a49..d6c814629 100644 --- a/app/models/feedback_answer.rb +++ b/app/models/feedback_answer.rb @@ -12,6 +12,13 @@ class FeedbackAnswer < ApplicationRecord validates_with Validators::FeedbackAnswerFormatValidator + # Whether any of `answers` names a feedback question that is not one of `course`'s. + sig { params(answers: T::Array[FeedbackAnswer], course: Course).returns(T::Boolean) } + def self.any_outside_course?(answers, course) + course_question_ids = course.feedback_question_ids + answers.any? { |answer| course_question_ids.exclude?(answer.feedback_question_id) } + end + sig { returns(T::Boolean) } def replied? reply_to_feedback_answers.count > 0 diff --git a/app/models/submission.rb b/app/models/submission.rb index 9cac88739..16531d290 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -421,13 +421,25 @@ def set_paste_key_if_paste_available self.paste_key = SecureRandom.urlsafe_base64 if paste_available? end - # Maps `submissions` in place to their API list fields, as seen by `viewer`. + # can_see_results? for many submissions seen by one viewer, evaluated once per course and exercise. + sig { params(viewer: User).returns(T.proc.params(submission: Submission).returns(T::Boolean)) } + def self.results_visibility_for(viewer) + visible_by_exercise = T.let({}, T::Hash[[T.nilable(Integer), T.nilable(String)], T::Boolean]) + lambda do |submission| + visible_by_exercise.fetch([submission.course_id, submission.exercise_name]) do |key| + visible_by_exercise[key] = submission.can_see_results?(viewer) + end + end + end + + # Maps `submissions` in place to their API list fields, as seen by `viewer`. Preload their courses' + # organizations first; Api::V8::BaseController#readable_submission_fields does. sig { params(submissions: T::Array[T.untyped], viewer: User).returns(T::Array[T::Hash[Symbol, T.untyped]]) } def self.filter_fields!(submissions, viewer) - ActiveRecord::Associations::Preloader.new(records: submissions, associations: { course: :organization }).call eager_load_exercises(submissions) + results_visible_to_viewer = results_visibility_for(viewer) submissions.map! do |sub| - results_visible = sub.can_see_results?(viewer) + results_visible = results_visible_to_viewer.call(sub) { id: sub.id, user_id: sub.user_id, diff --git a/app/services/csv_cell.rb b/app/services/csv_cell.rb index ecec3717b..870d093ca 100644 --- a/app/services/csv_cell.rb +++ b/app/services/csv_cell.rb @@ -1,6 +1,8 @@ # typed: strict # frozen_string_literal: true +require 'csv' + # Prepares values for CSV exports that teachers open in spreadsheet programs. module CsvCell extend T::Sig @@ -12,4 +14,10 @@ module CsvCell def self.escape(value) value.is_a?(String) && value.match?(FORMULA_START) ? "'#{value}" : value end + + # CSV.generate with every written cell passed through escape. + sig { params(options: T.untyped, block: T.proc.params(csv: CSV).void).returns(String) } + def self.generate(**options, &block) + CSV.generate(**options, write_converters: [->(cell) { escape(cell) }], &block) + end end diff --git a/app/views/points/index.csv.erb b/app/views/points/index.csv.erb index b6e790d1f..ffa1c4c82 100644 --- a/app/views/points/index.csv.erb +++ b/app/views/points/index.csv.erb @@ -1,12 +1,12 @@ <% require 'csv' %> -<%= CSV.generate(:force_quotes => true) do |csv| +<%= CsvCell.generate(force_quotes: true) do |csv| arr = [] arr << :id if can?(:teach, @course) arr += @user_fields.map(&:label) if @user_fields arr << :email if can?(:teach, @course) arr << :username if can?(:teach, @course) && params[:include_username] arr += @summary[:sheets].map {|sheet| sheet[:name] } + ["Total"] - csv << arr.map { |cell| CsvCell.escape(cell) } + csv << arr @summary[:users].each do |user| points = @summary[:sheets].map do |sheet| @@ -24,6 +24,6 @@ arr << user.email if can?(:teach, @course) arr << user.login if can?(:teach, @course) && params[:include_username] arr += points + [total] - csv << arr.map { |cell| CsvCell.escape(cell) } + csv << arr end end.html_safe %> \ No newline at end of file diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 9d9e52656..d23d5f4d2 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,74 +1,5 @@ { "ignored_warnings": [ - { - "warning_type": "Command Injection", - "warning_code": 14, - "fingerprint": "5dbd2de58c6ff2fb90e9b725058df7915ee3085e51b0c1b024b5fd3869e33512", - "check_name": "Execute", - "message": "Possible command injection", - "file": "app/background_tasks/rust_langs_downloader_task.rb", - "line": 27, - "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", - "code": "`wget https://download.mooc.fi/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version} -O #{\"vendor/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version}\"}`", - "render_path": null, - "location": { - "type": "method", - "class": "RustLangsDownloaderTask", - "method": "run" - }, - "user_input": "@@rust_langs_version", - "confidence": "Medium", - "cwe_id": [ - 77 - ], - "note": "The version is a constant in this file." - }, - { - "warning_type": "Command Injection", - "warning_code": 14, - "fingerprint": "73ea6fc75cbc4ef690218d09aed5b24541f0e36d45bf8ef19a3e6575d72ff54b", - "check_name": "Execute", - "message": "Possible command injection", - "file": "app/background_tasks/rust_langs_downloader_task.rb", - "line": 28, - "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", - "code": "`chmod +x #{\"vendor/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version}\"}`", - "render_path": null, - "location": { - "type": "method", - "class": "RustLangsDownloaderTask", - "method": "run" - }, - "user_input": "@@rust_langs_version", - "confidence": "Medium", - "cwe_id": [ - 77 - ], - "note": "The version is a constant in this file." - }, - { - "warning_type": "Command Injection", - "warning_code": 14, - "fingerprint": "59cdc01b2c3cedcc5ffe248e63f31eccf08f22365063c1310677546b607bd989", - "check_name": "Execute", - "message": "Possible command injection", - "file": "app/background_tasks/rust_langs_downloader_task.rb", - "line": 30, - "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", - "code": "`#{\"vendor/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{@@rust_langs_version}\"} --help`", - "render_path": null, - "location": { - "type": "method", - "class": "RustLangsDownloaderTask", - "method": "run" - }, - "user_input": "@@rust_langs_version", - "confidence": "Medium", - "cwe_id": [ - 77 - ], - "note": "The version is a constant in this file." - }, { "warning_type": "File Access", "warning_code": 16, @@ -414,40 +345,6 @@ ], "note": "Both values go through connection.quote." }, - { - "warning_type": "Cross-Site Scripting", - "warning_code": 4, - "fingerprint": "db72cd434e0e7d9b51fb2524142b90d72743f31acd5d4fa87ed3c260e8b45ae9", - "check_name": "LinkToHref", - "message": "Potentially unsafe model attribute in `link_to` href", - "file": "app/views/courses/help.html.erb", - "line": 33, - "link": "https://brakemanscanner.org/docs/warning_types/link_to_href", - "code": "link_to(Course.find(params[:course_id]).material_url, Course.find(params[:course_id]).material_url, :target => \"_blank\")", - "render_path": [ - { - "type": "controller", - "class": "CoursesController", - "method": "help", - "line": 168, - "file": "app/controllers/courses_controller.rb", - "rendered": { - "name": "courses/help", - "file": "app/views/courses/help.html.erb" - } - } - ], - "location": { - "type": "template", - "template": "courses/help" - }, - "user_input": "Course.find(params[:course_id]).material_url", - "confidence": "Weak", - "cwe_id": [ - 79 - ], - "note": "Course#material_url= prefixes anything without http(s):// with http://, so javascript: URLs can't get through." - }, { "warning_type": "Command Injection", "warning_code": 14, diff --git a/config/environments/production.rb b/config/environments/production.rb index 96a486270..0f1cc3a53 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -52,6 +52,9 @@ # Which storage.yml to use config.active_storage.service = :local + # nginx terminates TLS and may not pass the scheme on; the secure session cookie relies on this. + config.assume_ssl = true + # Force all access to the app over SSL, use Strict-Transport-Security, and use secure cookies. # config.force_ssl = true diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 644723f95..fdfc38b5d 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -71,17 +71,19 @@ def self.user_identifier_discriminator(access_token) PasswordFailureThrottle.blocked?(req) end + API_ACCOUNT_CREATION_PATH = T.let(%r{\A/api/v8/users(\.\w+)?\z}, Regexp) + # Limit the number of account creations to 15 accounts per minute throttle('account creations per minute', limit: 15, period: 1.minute) do |req| req.ip if req.path == '/user' || - (req.path.match?(%r{\A/api/v8/users(\.\w+)?\z}) && + (req.path.match?(API_ACCOUNT_CREATION_PATH) && req.post?) end # Limit the number of account creations to 1000 per day throttle('account creations per day', limit: 1000, period: 1.day) do |req| req.ip if req.path == '/user' || - (req.path.match?(%r{\A/api/v8/users(\.\w+)?\z}) && + (req.path.match?(API_ACCOUNT_CREATION_PATH) && req.post?) end @@ -117,8 +119,36 @@ def self.user_identifier_discriminator(access_token) req.post? end + # Reset requests carry only an email, so anything larger is not a real client. + PASSWORD_RESET_JSON_MAX_BYTES = 8192 + + # The email a password reset is requested for, picked the way Rails params do: the query string + # wins over the body, and every content type Rails parses as JSON is read as JSON. Oversized + # JSON bodies share one key so that padding cannot dodge the per-email limit. + sig { params(req: Rack::Attack::Request).returns(T.nilable(String)) } + def self.password_reset_email(req) + email = req.GET['email'] + email ||= Mime::Type.lookup(req.media_type.to_s).symbol == :json ? json_body_email(req) : req.POST['email'] + email.to_s.strip.downcase.presence + end + + sig { params(req: Rack::Attack::Request).returns(T.untyped) } + private_class_method def self.json_body_email(req) + body = req.body + json = body.read(PASSWORD_RESET_JSON_MAX_BYTES + 1).to_s + body.rewind + return 'oversized JSON body' if json.bytesize > PASSWORD_RESET_JSON_MAX_BYTES + + parsed = begin + JSON.parse(json) + rescue JSON::ParserError + nil + end + parsed['email'] if parsed.is_a?(Hash) + end + # Limit the number of password resets per email to 5 per day throttle('password resets per email per day', limit: 5, period: 1.day) do |req| - req.params['email'].to_s.strip.downcase.presence if req.path.match?(PASSWORD_RESET_PATH) && req.post? + password_reset_email(req) if req.path.match?(PASSWORD_RESET_PATH) && req.post? end end diff --git a/lib/safe_unzipper.rb b/lib/safe_unzipper.rb index 868c0de6f..7d92edc71 100644 --- a/lib/safe_unzipper.rb +++ b/lib/safe_unzipper.rb @@ -11,9 +11,10 @@ class SafeUnzipper # Far above any real exercise project; only archives built to exhaust the server reach these. MAX_ENTRIES = 10_000 MAX_UNCOMPRESSED_BYTES = T.let(100 * 1024 * 1024, Integer) - TIMEOUT_SECONDS = 60 + # Per command; the three commands of an unzip must together fit in Rack::Timeout's 120 s. + TIMEOUT_SECONDS = 30 - # Raised before extraction for archives over the entry, size or time limits. + # Raised for archives over the entry, size or time limits. class ArchiveTooLargeError < StandardError; end sig { params(zip_path: T.any(String, Pathname), dest_dir: T.any(String, Pathname)).void } @@ -22,20 +23,27 @@ def unzip(zip_path, dest_dir) dest_dir = File.absolute_path(dest_dir) check_limits(zip_path) # Ideally we'd protect against `unzip` exploits too by running in a sandbox - SystemCommands.sh!('unzip', '-qqu', zip_path, '-d', dest_dir, timeout: TIMEOUT_SECONDS) + run_with_timeout('unzip', '-qqu', zip_path, '-d', dest_dir) clean_up(dest_dir) end private sig { params(zip_path: String).void } def check_limits(zip_path) - summary = SystemCommands.sh!('zipinfo', '-t', zip_path, timeout: TIMEOUT_SECONDS)[:output] + summary = run_with_timeout('zipinfo', '-t', zip_path) entry_count = summary[/(\d+) files?,/, 1] raise "Unexpected zipinfo output: #{summary}" if entry_count.nil? raise ArchiveTooLargeError, "the archive has more than #{MAX_ENTRIES} files." if entry_count.to_i > MAX_ENTRIES raise ArchiveTooLargeError, "the archive extracts to more than #{MAX_UNCOMPRESSED_BYTES / 1024 / 1024} MB." if uncompressed_bytes_over_limit?(zip_path) end + sig { params(command: String).returns(String) } + def run_with_timeout(*command) + T.unsafe(SystemCommands).sh!(*command, timeout: TIMEOUT_SECONDS)[:output] + rescue SystemCommands::TimeoutError + raise ArchiveTooLargeError, 'the archive took too long to extract.' + end + # Decompresses to a pipe instead of trusting the sizes in the archive's headers, which can lie. sig { params(zip_path: String).returns(T::Boolean) } def uncompressed_bytes_over_limit?(zip_path) diff --git a/lib/submission_list.rb b/lib/submission_list.rb index afbc897d7..a9d8b4e9a 100644 --- a/lib/submission_list.rb +++ b/lib/submission_list.rb @@ -8,6 +8,7 @@ class SubmissionList def initialize(user, helpers) @user = user @helpers = helpers + @results_visible_to_user = T.let(Submission.results_visibility_for(user), T.proc.params(submission: Submission).returns(T::Boolean)) end sig { params(submissions: T::Enumerable[Submission]).returns(T::Array[T::Hash[Symbol, T.untyped]]) } @@ -17,7 +18,7 @@ def submission_list_data(submissions) sig { params(submission: Submission).returns(T::Hash[Symbol, T.untyped]) } def submission_data(submission) - results_visible = submission.can_see_results?(@user) + results_visible = @results_visible_to_user.call(submission) { exercise_name: submission.exercise_name, id: submission.id, diff --git a/lib/system_commands.rb b/lib/system_commands.rb index 35c22f771..d07fde124 100644 --- a/lib/system_commands.rb +++ b/lib/system_commands.rb @@ -12,6 +12,9 @@ module SystemCommands extend T::Helpers requires_ancestor { Kernel } + # Raised by sh! when its timeout kills the command. + class TimeoutError < RuntimeError; end + module_function # Prefer sh! instead @@ -47,7 +50,7 @@ def sh!(*args) output = `#{cmd} 2>&1` status = $? - raise "Command '#{cmd}' timed out, status #{status.inspect}. The output follows:\n#{output}" if status.exitstatus == 124 + raise TimeoutError, "Command '#{cmd}' timed out, status #{status.inspect}. The output follows:\n#{output}" if status.exitstatus == 124 raise "Command `#{cmd}` failed with status #{status.inspect}. The output follows:\n#{output}" unless status.success? raise "Expected no output from `#{cmd}` but got: #{output}" if options[:assert_silent] && !output.empty? diff --git a/spec/background_tasks/rust_langs_downloader_task_spec.rb b/spec/background_tasks/rust_langs_downloader_task_spec.rb new file mode 100644 index 000000000..a2e1cb73c --- /dev/null +++ b/spec/background_tasks/rust_langs_downloader_task_spec.rb @@ -0,0 +1,68 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe RustLangsDownloaderTask do + let(:task) { described_class.new } + let(:fake_binary) { "#!/bin/sh\necho tmc-langs\n" } + let(:executable) { "vendor/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{described_class::RUST_LANGS_VERSION}" } + + # The cwd is the emptied per-example tmp/tests, which stands in for Rails.root here. + before :each do + Dir.mkdir('vendor') + allow(task).to receive(:sleep) + allow(task).to receive(:system) do |*args| + File.write(args.fetch(3), fake_binary) + true + end + end + + it 'installs a download that matches the pinned SHA-256' do + stub_const("#{described_class}::RUST_LANGS_SHA256", Digest::SHA256.hexdigest(fake_binary)) + task.run + expect(File.executable?(executable)).to be(true) + expect(File.readlink('vendor/tmc-langs-rust/current')).to eq(File.basename(executable)) + end + + it 'deletes and refuses a download that does not match the pinned SHA-256' do + expect(Rails.logger).to receive(:error).with(/SHA-256/) + expect { task.run }.to raise_error(described_class::ChecksumMismatch) + expect(Dir.children('vendor/tmc-langs-rust')).to be_empty + end + + it 'raises with the exit status when wget fails' do + allow(task).to receive(:system) do |*args| + File.write(args.fetch(3), 'partial') + Kernel.system('sh', '-c', 'exit 8') + end + expect { task.run }.to raise_error(described_class::DownloadFailed, /exit 8/) + expect(Dir.children('vendor/tmc-langs-rust')).to be_empty + end + + it 'deletes a download that does not run and backs off' do + stub_const("#{described_class}::RUST_LANGS_SHA256", Digest::SHA256.hexdigest("#!/bin/sh\nexit 1\n")) + allow(task).to receive(:system) do |*args| + File.write(args.fetch(3), "#!/bin/sh\nexit 1\n") + true + end + expect(Rails.logger).to receive(:error).with(/did not run/) + expect { task.run }.to raise_error(described_class::BinaryDoesNotRun) + expect(Dir.children('vendor/tmc-langs-rust')).to be_empty + expect(task.wait_delay).to eq(60) + end + + it 'backs off after consecutive failures, up to an hour, and resets after a success' do + allow(Rails.logger).to receive(:error) + expect(task.wait_delay).to eq(1) + delays = Array.new(8) do + expect { task.run }.to raise_error(described_class::ChecksumMismatch) + task.wait_delay + end + expect(delays).to eq([60, 120, 240, 480, 960, 1920, 3600, 3600]) + + stub_const("#{described_class}::RUST_LANGS_SHA256", Digest::SHA256.hexdigest(fake_binary)) + task.run + expect(task.wait_delay).to eq(1) + end +end diff --git a/spec/controllers/api/v8/core/exercises/details_controller_spec.rb b/spec/controllers/api/v8/core/exercises/details_controller_spec.rb index 2f89c31fe..fb08d2a1c 100644 --- a/spec/controllers/api/v8/core/exercises/details_controller_spec.rb +++ b/spec/controllers/api/v8/core/exercises/details_controller_spec.rb @@ -11,9 +11,45 @@ allow(controller).to receive(:doorkeeper_token).and_return(nil) end + def query_count(&) + count = 0 + counter = ->(*, payload) { count += 1 unless %w[SCHEMA TRANSACTION].include?(payload[:name]) } + ActiveSupport::Notifications.subscribed(counter, 'sql.active_record', &) + count + end + it 'leaves out exercises of courses the caller cannot read' do get :show, params: { ids: "#{visible_exercise.id},#{hidden_course_exercise.id}" } expect(response).to have_http_status(200) expect(response.parsed_body['exercises'].pluck('id')).to eq([visible_exercise.id]) end + + it 'leaves out hidden exercises of a readable course' do + hidden_exercise = FactoryBot.create(:exercise, course: visible_exercise.course, hidden: true) + get :show, params: { ids: "#{visible_exercise.id},#{hidden_exercise.id}" } + expect(response.parsed_body['exercises'].pluck('id')).to eq([visible_exercise.id]) + end + + it 'gives a teacher the hidden exercises of their course' do + hidden_exercise = FactoryBot.create(:exercise, course: visible_exercise.course, hidden: true) + teacher = FactoryBot.create(:user) + Teachership.create!(user: teacher, organization: visible_exercise.course.organization) + allow(controller).to receive(:doorkeeper_token).and_return(double(resource_owner_id: teacher.id, acceptable?: true)) + get :show, params: { ids: "#{visible_exercise.id},#{hidden_exercise.id}" } + expect(response.parsed_body['exercises'].pluck('id')).to contain_exactly(visible_exercise.id, hidden_exercise.id) + end + + it 'runs the same number of queries for 2 and 6 exercises' do + user = FactoryBot.create(:user) + allow(controller).to receive(:doorkeeper_token).and_return(double(resource_owner_id: user.id, acceptable?: true)) + course = visible_exercise.course + exercises = [visible_exercise] + FactoryBot.create_list(:exercise, 5, course: course) + exercises.last.update!(hidden: true) + + few = query_count { get :show, params: { ids: exercises.first(2).map(&:id).join(',') } } + expect(response.parsed_body['exercises'].size).to eq(2) + many = query_count { get :show, params: { ids: exercises.map(&:id).join(',') } } + expect(response.parsed_body['exercises'].size).to eq(5) + expect(many).to eq(few) + end end diff --git a/spec/controllers/api/v8/courses/exercises/points_controller_spec.rb b/spec/controllers/api/v8/courses/exercises/points_controller_spec.rb index 839bdf465..f4374abe0 100644 --- a/spec/controllers/api/v8/courses/exercises/points_controller_spec.rb +++ b/spec/controllers/api/v8/courses/exercises/points_controller_spec.rb @@ -35,6 +35,27 @@ end end + describe 'As a student' do + let(:token) { double resource_owner_id: user.id, acceptable?: true } + + it "returns only the student's own points" do + get :index, params: { course_id: course.id, exercise_name: exercise.name } + expect(response.parsed_body.map { |p| p['awarded_point']['id'] }).to eq([awarded_point2.id]) + end + end + + describe 'As a teacher' do + let(:teacher) { FactoryBot.create(:user) } + let(:token) { double resource_owner_id: teacher.id, acceptable?: true } + + before { Teachership.create!(user: teacher, organization: organization) } + + it "returns every user's points" do + get :index, params: { course_id: course.id, exercise_name: exercise.name } + expect(response.parsed_body.map { |p| p['awarded_point']['id'] }).to contain_exactly(awarded_point1.id, awarded_point2.id) + end + end + describe 'As a guest' do let(:token) { nil } describe 'when searching for awarded points' do diff --git a/spec/controllers/api/v8/courses/points_controller_spec.rb b/spec/controllers/api/v8/courses/points_controller_spec.rb index 8f1c9b9cf..da2ec55ea 100644 --- a/spec/controllers/api/v8/courses/points_controller_spec.rb +++ b/spec/controllers/api/v8/courses/points_controller_spec.rb @@ -51,13 +51,13 @@ let(:token) { double resource_owner_id: current_user.id, acceptable?: true } describe 'when course ID given' do - it "shows only user's point information" do + it "shows only the caller's own points" do get :index, params: { course_id: course.id } expect(response).to have_http_status(200) expect(response.body).not_to include point.name expect(response.body).to include current_user_course_point.name expect(response.body).not_to include current_user_point.name - expect(response.body).to include course_point.name + expect(response.body).not_to include course_point.name end end describe 'when invalid course ID given' do @@ -78,11 +78,10 @@ let(:token) { nil } describe 'when course ID given' do - it "shows only user's point information" do + it 'lists no points' do get :index, params: { course_id: course.id } expect(response).to have_http_status(200) - expect(response.body).not_to include point.name - expect(response.body).to include course_point.name + expect(response.parsed_body).to eq([]) end end describe 'when invalid course ID given' do @@ -100,6 +99,44 @@ describe 'GET points for a course in exam mode' do before(:each) { course.update!(hide_submission_results: true) } + describe 'as user' do + let(:current_user) { FactoryBot.create(:user) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it 'lists no points' do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(200) + expect(response.parsed_body).to eq([]) + end + end + + describe 'as guest' do + let(:current_user) { Guest.new } + let(:token) { nil } + + it 'lists no points' do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(200) + expect(response.parsed_body).to eq([]) + end + end + + describe 'as admin' do + let(:current_user) { FactoryBot.create(:admin) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it 'lists the points' do + get :index, params: { course_id: course.id } + expect(response.body).to include course_point.name + end + end + end + + describe 'GET points for a hidden course the caller is not on' do + let!(:current_user_course_point) { nil } + + before(:each) { course.update!(hidden: true) } + describe 'as user' do let(:current_user) { FactoryBot.create(:user) } let(:token) { double resource_owner_id: current_user.id, acceptable?: true } @@ -123,11 +160,28 @@ end end + describe 'GET points for a course with an exercise in exam mode' do + let!(:exam_point) do + exercise = FactoryBot.create(:exercise, course: course, hide_submission_results: true) + FactoryBot.create(:available_point, exercise: exercise) + FactoryBot.create(:awarded_point, course: course, user: current_user, name: exercise.available_points.first.name) + end + let(:current_user) { FactoryBot.create(:user) } + let(:token) { double resource_owner_id: current_user.id, acceptable?: true } + + it "leaves out the exercise's points" do + get :index, params: { course_id: course.id } + expect(response).to have_http_status(200) + expect(response.body).to include current_user_course_point.name + expect(response.body).not_to include exam_point.name + end + end + describe 'GET points for a course with a hidden exercise' do let!(:hidden_point) do exercise = FactoryBot.create(:exercise, course: course, hidden: true) FactoryBot.create(:available_point, exercise: exercise) - FactoryBot.create(:awarded_point, course: course, name: exercise.available_points.first.name) + FactoryBot.create(:awarded_point, course: course, user: current_user, name: exercise.available_points.first.name) end describe 'as user' do @@ -137,7 +191,7 @@ it "leaves out the hidden exercise's points" do get :index, params: { course_id: course.id } expect(response).to have_http_status(200) - expect(response.body).to include course_point.name + expect(response.body).to include current_user_course_point.name expect(response.body).not_to include hidden_point.name end end diff --git a/spec/controllers/api/v8/courses/users/points_controller_spec.rb b/spec/controllers/api/v8/courses/users/points_controller_spec.rb index 6717dd4ac..fb399c3d6 100644 --- a/spec/controllers/api/v8/courses/users/points_controller_spec.rb +++ b/spec/controllers/api/v8/courses/users/points_controller_spec.rb @@ -130,10 +130,10 @@ let(:token) { double resource_owner_id: current_user.id, acceptable?: true } describe 'when course ID given' do - it "shows only user's point information" do + it "lists none of another user's points" do get :index, params: { course_id: point.course_id, user_id: point.user_id } expect(response).to have_http_status(200) - expect(response.body).to include point.name + expect(response.parsed_body).to eq([]) expect(response.body).not_to include current_user_course_point.name expect(response.body).not_to include current_user_point.name expect(response.body).not_to include course_point.name @@ -157,10 +157,10 @@ let(:token) { double resource_owner_id: current_user.id, acceptable?: true } describe 'when course ID given' do - it "shows only user's point information" do + it "lists none of another user's points" do get :index, params: { course_id: point.course_id, user_id: point.user_id } expect(response).to have_http_status(200) - expect(response.body).to include point.name + expect(response.parsed_body).to eq([]) expect(response.body).not_to include course_point.name end end @@ -180,10 +180,10 @@ let(:token) { nil } describe 'when course ID given' do - it "shows only user's point information" do + it 'lists no points' do get :index, params: { course_id: point.course_id, user_id: point.user_id } expect(response).to have_http_status(200) - expect(response.body).to include point.name + expect(response.parsed_body).to eq([]) expect(response.body).not_to include course_point.name end end diff --git a/spec/controllers/api/v8/organizations/courses/points_controller_spec.rb b/spec/controllers/api/v8/organizations/courses/points_controller_spec.rb index c45600f86..a0c8acf20 100644 --- a/spec/controllers/api/v8/organizations/courses/points_controller_spec.rb +++ b/spec/controllers/api/v8/organizations/courses/points_controller_spec.rb @@ -52,13 +52,13 @@ let(:token) { double resource_owner_id: current_user.id, acceptable?: true } describe 'when course name given' do - it "shows only user's point information" do + it "shows only the caller's own points" do get :index, params: { organization_slug: organization.slug, course_name: course_name } expect(response).to have_http_status(200) expect(response.body).not_to include point.name expect(response.body).to include current_user_course_point.name expect(response.body).not_to include current_user_point.name - expect(response.body).to include course_point.name + expect(response.body).not_to include course_point.name end end describe 'when invalid course name given' do @@ -79,11 +79,10 @@ let(:token) { nil } describe 'when course name given' do - it "shows only user's point information" do + it 'lists no points' do get :index, params: { organization_slug: organization.slug, course_name: course_name } expect(response).to have_http_status(200) - expect(response.body).not_to include point.name - expect(response.body).to include course_point.name + expect(response.parsed_body).to eq([]) end end describe 'when invalid course name given' do diff --git a/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb b/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb index b8da4ca0f..75ebc36f8 100644 --- a/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb +++ b/spec/controllers/api/v8/organizations/courses/users/points_controller_spec.rb @@ -131,10 +131,10 @@ let(:token) { double resource_owner_id: current_user.id, acceptable?: true } describe 'when course name given' do - it "shows only user's point information" do + it "lists none of another user's points" do get :index, params: { organization_slug: organization.slug, course_name: course_name, user_id: course_point.user_id } expect(response).to have_http_status(200) - expect(response.body).to include course_point.name + expect(response.parsed_body).to eq([]) expect(response.body).not_to include current_user_course_point.name expect(response.body).not_to include current_user_point.name expect(response.body).not_to include point.name @@ -158,10 +158,10 @@ let(:token) { double resource_owner_id: current_user.id, acceptable?: true } describe 'when course name given' do - it "shows only user's point information" do + it "lists none of another user's points" do get :index, params: { organization_slug: organization.slug, course_name: course_name, user_id: course_point.user_id } expect(response).to have_http_status(200) - expect(response.body).to include course_point.name + expect(response.parsed_body).to eq([]) expect(response.body).not_to include point.name end end @@ -181,10 +181,10 @@ let(:token) { nil } describe 'when course name given' do - it "shows only user's point information" do + it 'lists no points' do get :index, params: { organization_slug: organization.slug, course_name: course_name, user_id: course_point.user_id } expect(response).to have_http_status(200) - expect(response.body).to include course_point.name + expect(response.parsed_body).to eq([]) expect(response.body).not_to include point.name end end @@ -204,11 +204,11 @@ let(:current_user) { FactoryBot.create(:user) } let(:token) { double resource_owner_id: current_user.id, acceptable?: true } - it 'is forbidden' do + it 'lists no points' do course.update!(hide_submission_results: true) get :index, params: { organization_slug: organization.slug, course_name: course_name, user_id: 'current' } - expect(response).to have_http_status(403) - expect(response.body).not_to include current_user_course_point.name + expect(response).to have_http_status(200) + expect(response.parsed_body).to eq([]) end end end diff --git a/spec/controllers/exercise_status_controller_spec.rb b/spec/controllers/exercise_status_controller_spec.rb index 8b8bb4b2c..35ae6129e 100644 --- a/spec/controllers/exercise_status_controller_spec.rb +++ b/spec/controllers/exercise_status_controller_spec.rb @@ -6,8 +6,7 @@ describe ExerciseStatusController, type: :controller do before :each do @organization = FactoryBot.create(:accepted_organization) - @course = FactoryBot.create(:course) - @course.organization = @organization + @course = FactoryBot.create(:course, organization: @organization) @exercise = FactoryBot.create(:exercise, course: @course) @exercise2 = FactoryBot.create(:exercise, course: @course) @exercise3 = FactoryBot.create(:exercise, course: @course) @@ -42,6 +41,10 @@ exercise: @exercise2) end + before :each do + controller.current_user = @user + end + def do_get get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: @user.id, format: :json, api_version: ApiVersion::API_VERSION } end @@ -79,18 +82,55 @@ def do_get expect(json[@exercise2.name]).to eq('attempted') expect(json[@exercise3.name]).to eq('not_attempted') end + + it "shows students their own status by a numeric login that is also another user's id" do + other = FactoryBot.create(:user) + @user.update!(login: other.id.to_s) + get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: @user.login, format: :json, api_version: ApiVersion::API_VERSION } + expect(response).to have_http_status(:ok) + expect(response.parsed_body[@exercise.name]).to eq('completed') + end + + it 'requires authentication' do + controller.current_user = Guest.new + do_get + expect(response).to have_http_status(:unauthorized) + end + + it "refuses another student's status" do + controller.current_user = FactoryBot.create(:user) + do_get + expect(response).to have_http_status(:forbidden) + end + + it "shows a teacher a student's status" do + teacher = FactoryBot.create(:user) + Teachership.create!(user: teacher, organization: @organization) + controller.current_user = teacher + do_get + expect(response).to have_http_status(:ok) + expect(response.parsed_body[@exercise.name]).to eq('completed') + end end it 'responds 404 for an unknown course' do + controller.current_user = FactoryBot.create(:user) get :show, params: { organization_id: @organization.slug, course_id: 'no-such-course', id: 'nobody', format: :json, api_version: ApiVersion::API_VERSION } expect(response).to have_http_status(:not_found) expect(response.parsed_body['error']).to eq('Course not found') end - it 'responds 404 for an unknown user' do + it 'responds 404 to an admin for an unknown user' do + controller.current_user = FactoryBot.create(:admin) get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: 'nobody', format: :json, api_version: ApiVersion::API_VERSION } expect(response).to have_http_status(:not_found) expect(response.parsed_body['error']).to eq('User not found') end + + it 'responds 403 to a student for an unknown user' do + controller.current_user = FactoryBot.create(:user) + get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: 'nobody', format: :json, api_version: ApiVersion::API_VERSION } + expect(response).to have_http_status(:forbidden) + end end end diff --git a/spec/controllers/participants_controller_spec.rb b/spec/controllers/participants_controller_spec.rb index a1c47080b..f09583230 100644 --- a/spec/controllers/participants_controller_spec.rb +++ b/spec/controllers/participants_controller_spec.rb @@ -29,6 +29,16 @@ end end + describe 'GET /index as CSV' do + it 'neutralizes cells a spreadsheet would run as formulas' do + FactoryBot.create(:user, login: '=HYPERLINK("http://x")') + controller.current_user = FactoryBot.create(:admin) + get :index, format: :csv + expect(response.body).to include %("'=HYPERLINK(""http://x"")") + expect(response.body).not_to include %("=HYPERLINK) + end + end + describe 'POST /force_migrate_to_courses_mooc_fi' do describe 'when logged in as a non-admin' do before :each do diff --git a/spec/controllers/points_controller_spec.rb b/spec/controllers/points_controller_spec.rb index 56b611089..c30031504 100644 --- a/spec/controllers/points_controller_spec.rb +++ b/spec/controllers/points_controller_spec.rb @@ -40,6 +40,13 @@ expect(response.code.to_i).to eq(403) end end + + it 'denies a course the user cannot read' do + @course.update!(hidden: true) + controller.current_user = FactoryBot.create(:user) + get :index, params: { organization_id: @organization.slug, course_id: @course.id } + expect(response.code.to_i).to eq(403) + end end describe 'GET show' do @@ -73,6 +80,13 @@ end end + it 'shows the sheet to a student with no points in it' do + controller.current_user = FactoryBot.create(:user) + get :show, params: { organization_id: @organization.slug, course_id: @course.id, id: @sheetname } + expect(response).to have_http_status(:ok) + expect(response.body).not_to have_content('✔') + end + describe 'in JSON format' do before :each do @other_user = FactoryBot.create(:user) diff --git a/spec/controllers/submission_controller_spec.rb b/spec/controllers/submission_controller_spec.rb index be58b84fe..8432fb18a 100644 --- a/spec/controllers/submission_controller_spec.rb +++ b/spec/controllers/submission_controller_spec.rb @@ -137,6 +137,17 @@ end end + describe 'POST create' do + it 'records the client IP from remote_ip, not a client-sent X-Forwarded-For' do + exercise = FactoryBot.create(:returnable_exercise, course: @course) + zip = Rack::Test::UploadedFile.new(Rails.root.join('spec/fixtures/submission_uploads/empty.zip'), 'application/octet-stream') + request.env['REMOTE_ADDR'] = '203.0.113.7' + request.env['HTTP_X_FORWARDED_FOR'] = '198.51.100.9' + expect { post :create, params: { exercise_id: exercise.id, submission: { file: zip } } }.to change(Submission, :count).by(1) + expect(Submission.last.client_ip).to eq('203.0.113.7') + end + end + describe 'GET index' do render_views diff --git a/spec/lib/safe_unzipper_spec.rb b/spec/lib/safe_unzipper_spec.rb index 61b28f31c..2a964190d 100644 --- a/spec/lib/safe_unzipper_spec.rb +++ b/spec/lib/safe_unzipper_spec.rb @@ -67,6 +67,15 @@ def zip_and_unzip expect(Dir.empty?('out')).to be(true) end + it 'refuses an archive whose extraction times out' do + SystemCommands.sh!('zip', '-r', 'foo.zip', 'foo') + Dir.mkdir('out') + allow(SystemCommands).to receive(:sh!).and_call_original + allow(SystemCommands).to receive(:sh!).with('unzip', any_args).and_raise(SystemCommands::TimeoutError) + + expect { SafeUnzipper.new.unzip('foo.zip', 'out') }.to raise_error(SafeUnzipper::ArchiveTooLargeError, /too long/) + end + it 'removes symlinks inside the unzipped directory' do File.symlink('../good_file', 'foo/bar/good_link') zip_and_unzip diff --git a/spec/requests/basic_auth_challenge_spec.rb b/spec/requests/basic_auth_challenge_spec.rb index 4d9b42df2..f791d10d2 100644 --- a/spec/requests/basic_auth_challenge_spec.rb +++ b/spec/requests/basic_auth_challenge_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' -describe 'Basic auth challenge on JSON 403 responses', type: :request do +describe 'Basic auth challenge on JSON 401 responses', type: :request do let(:course) { FactoryBot.create(:course, hidden: true) } let(:path) { "/org/#{course.organization.slug}/courses/#{course.id}/exercise_status/someone.json" } @@ -13,19 +13,19 @@ def get_status(client_params) it 'is not sent to NetBeans plugins that send no version' do get_status(client: 'netbeans_plugin') - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:unauthorized) expect(response.headers['WWW-Authenticate']).to be_nil end it 'is not sent to NetBeans plugins older than 0.8.0' do get_status(client: 'netbeans_plugin', client_version: '0.7.9') - expect(response).to have_http_status(:forbidden) + expect(response).to have_http_status(:unauthorized) expect(response.headers['WWW-Authenticate']).to be_nil end it 'is sent to NetBeans plugins from 0.8.0 on' do get_status(client: 'netbeans_plugin', client_version: '0.8.0') - expect(response).to have_http_status(:forbidden) - expect(response.headers['WWW-Authenticate']).to eq('Basic realm="Forbidden"') + expect(response).to have_http_status(:unauthorized) + expect(response.headers['WWW-Authenticate']).to eq('Basic realm="Authentication required"') end end diff --git a/spec/requests/signup_and_password_reset_throttle_spec.rb b/spec/requests/signup_and_password_reset_throttle_spec.rb index 63b0180ff..660cd8c83 100644 --- a/spec/requests/signup_and_password_reset_throttle_spec.rb +++ b/spec/requests/signup_and_password_reset_throttle_spec.rb @@ -19,6 +19,64 @@ expect(response).to have_http_status(:service_unavailable) end + it 'limits API password resets per email sent as JSON' do + FactoryBot.create(:user, email: 'student@example.com') + 5.times do |i| + post '/api/v8/users/password_reset', params: { email: 'student@example.com' }.to_json, + headers: { 'CONTENT_TYPE' => 'application/json' }, env: { 'REMOTE_ADDR' => "198.51.100.#{i}" } + expect(response).to have_http_status(:ok) + end + + post '/api/v8/users/password_reset', params: { email: 'Student@example.com' }.to_json, + headers: { 'CONTENT_TYPE' => 'application/json' }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:service_unavailable) + end + + it 'counts the email from the query string, which Rails prefers over the body' do + FactoryBot.create(:user, email: 'student@example.com') + 5.times do |i| + post '/password_reset_keys?email=student@example.com', params: { email: "decoy#{i}@example.com" }, + env: { 'REMOTE_ADDR' => "198.51.100.#{i}" } + end + expect(response).not_to have_http_status(:service_unavailable) + + post '/password_reset_keys?email=student@example.com', params: { email: 'decoy@example.com' }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:service_unavailable) + end + + it 'limits password resets per email sent with a JSON synonym content type' do + FactoryBot.create(:user, email: 'student@example.com') + 5.times do |i| + post '/api/v8/users/password_reset', params: { email: 'student@example.com' }.to_json, + headers: { 'CONTENT_TYPE' => 'text/x-json' }, env: { 'REMOTE_ADDR' => "198.51.100.#{i}" } + expect(response).to have_http_status(:ok) + end + + post '/api/v8/users/password_reset', params: { email: 'student@example.com' }.to_json, + headers: { 'CONTENT_TYPE' => 'text/x-json' }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:service_unavailable) + end + + it 'limits padded JSON password resets together' do + 5.times do |i| + post '/api/v8/users/password_reset', params: { email: "nobody#{i}@example.com", padding: 'x' * 10_000 }.to_json, + headers: { 'CONTENT_TYPE' => 'application/json' }, env: { 'REMOTE_ADDR' => "198.51.100.#{i}" } + end + expect(response).not_to have_http_status(:service_unavailable) + + post '/api/v8/users/password_reset', params: { email: 'someone@example.com', padding: 'x' * 10_000 }.to_json, + headers: { 'CONTENT_TYPE' => 'application/json' }, env: { 'REMOTE_ADDR' => ip } + expect(response).to have_http_status(:service_unavailable) + end + + it 'leaves a JSON body readable for the app' do + FactoryBot.create(:user, email: 'student@example.com') + expect do + post '/api/v8/users/password_reset', params: { email: 'student@example.com' }.to_json, + headers: { 'CONTENT_TYPE' => 'application/json' }, env: { 'REMOTE_ADDR' => ip } + end.to change { ActionMailer::Base.deliveries.size }.by(1) + end + it 'limits password resets per IP across the web form and the API' do 5.times { |i| post '/password_reset_keys', params: { email: "nobody#{i}@example.com" }, env: { 'REMOTE_ADDR' => ip } } 5.times { |i| post '/api/v8/users/password_reset', params: { email: "noone#{i}@example.com" }, env: { 'REMOTE_ADDR' => ip } } From 988caf790586d2f5bb9577b54f53dbe2313bb1ed Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 10 Nov 2026 09:49:57 +0200 Subject: [PATCH 09/11] Send nullable fields in forms tmc-langs can parse --- app/controllers/api/v8/organizations_controller.rb | 8 ++++---- app/models/organization.rb | 2 +- app/models/submission.rb | 4 ++-- lib/submission_list.rb | 2 +- .../api/v8/courses/users/submissions_controller_spec.rb | 2 +- spec/controllers/api/v8/organizations_controller_spec.rb | 8 ++++++++ 6 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v8/organizations_controller.rb b/app/controllers/api/v8/organizations_controller.rb index 657c51ef6..ad69b2280 100644 --- a/app/controllers/api/v8/organizations_controller.rb +++ b/app/controllers/api/v8/organizations_controller.rb @@ -53,10 +53,10 @@ def index .select { |org| org.visibility_allowed?(request, current_user) } orgs = Organization.all if current_user.administrator? - orgs = orgs.map { |o| o.attributes.merge(logo_path: o.logo_path) } if current_user.administrator? - - unless current_user.administrator? - orgs = orgs.map { |o| { name: o.name, information: o.information, slug: o.slug, logo_path: o.logo_path, pinned: o.pinned } } + orgs = if current_user.administrator? + orgs.map { |o| o.attributes.merge(o.org_as_json.stringify_keys) } + else + orgs.map(&:org_as_json) end authorize! :read, orgs present(orgs) diff --git a/app/models/organization.rb b/app/models/organization.rb index 1ca6d6c2d..abfedd0ec 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -67,7 +67,7 @@ def self.init(params, initial_user) sig { returns(T::Hash[Symbol, T.untyped]) } def org_as_json - { name: name, information: information, slug: slug, logo_path: logo_path, pinned: pinned } + { name: name, information: information.to_s, slug: slug, logo_path: logo_path, pinned: pinned } end sig { params(user: T.nilable(User)).returns(T::Boolean) } diff --git a/app/models/submission.rb b/app/models/submission.rb index 16531d290..c7adcd1d2 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -448,7 +448,7 @@ def self.filter_fields!(submissions, viewer) exercise_name: sub.exercise_name, course_id: sub.course_id, processed: sub.processed, - all_tests_passed: results_visible ? sub.all_tests_passed : nil, + all_tests_passed: results_visible && sub.all_tests_passed, points: results_visible ? sub.points : nil, processing_tried_at: sub.processing_tried_at, processing_began_at: sub.processing_began_at, @@ -463,7 +463,7 @@ def self.filter_fields!(submissions, viewer) newer_submission_reviewed: sub.newer_submission_reviewed, review_dismissed: sub.review_dismissed, paste_available: sub.paste_available, - message_for_paste: sub.message_for_paste, + message_for_paste: sub.message_for_paste.to_s, paste_key: sub.paste_key } end diff --git a/lib/submission_list.rb b/lib/submission_list.rb index a9d8b4e9a..c280658be 100644 --- a/lib/submission_list.rb +++ b/lib/submission_list.rb @@ -25,7 +25,7 @@ def submission_data(submission) user_id: submission.user_id, course_id: submission.course_id, created_at: submission.created_at, - all_tests_passed: results_visible ? submission.all_tests_passed : nil, + all_tests_passed: results_visible && submission.all_tests_passed, points: results_visible ? submission.points : nil, submitted_zip_url: @helpers.download_api_v8_core_submission_url(submission), paste_url: paste_url(submission), diff --git a/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb b/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb index 413e98206..a53cb5982 100644 --- a/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb +++ b/spec/controllers/api/v8/courses/users/submissions_controller_spec.rb @@ -35,7 +35,7 @@ controller.current_user = user get :index, params: { course_id: course.id, user_id: user.id } listed = response.parsed_body.find { |s| s['id'] == submission.id } - expect(listed['all_tests_passed']).to be_nil + expect(listed['all_tests_passed']).to be(false) expect(listed['points']).to be_nil end diff --git a/spec/controllers/api/v8/organizations_controller_spec.rb b/spec/controllers/api/v8/organizations_controller_spec.rb index b94e77c61..656c08a4d 100644 --- a/spec/controllers/api/v8/organizations_controller_spec.rb +++ b/spec/controllers/api/v8/organizations_controller_spec.rb @@ -20,6 +20,14 @@ expect(r[0]['pinned']).to eq(organization.pinned) end + it 'lists missing information as an empty string, which tmc-langs requires' do + organization.update!(information: nil) + get :index + expect(response.parsed_body[0]['information']).to eq('') + get :show, params: { slug: organization.slug } + expect(response.parsed_body['information']).to eq('') + end + it 'should not show hidden organizations' do get :index From 86eec396a84eee5df18739fd31424d5a66905b50 Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 10 Nov 2026 09:49:57 +0200 Subject: [PATCH 10/11] Accept tmc_cli_rust and test the client-facing APIs --- config/site.defaults.yml | 2 + config/site.dev.yml | 2 + .../api/v8/application/password_login_spec.rb | 47 ++++++++++++++ .../model_solutions_controller_spec.rb | 62 +++++++++++++++++++ .../users/submissions_controller_spec.rb | 54 ++++++++++++++++ 5 files changed, 167 insertions(+) create mode 100644 spec/controllers/api/v8/application/password_login_spec.rb create mode 100644 spec/controllers/api/v8/exercises/model_solutions_controller_spec.rb create mode 100644 spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb diff --git a/config/site.defaults.yml b/config/site.defaults.yml index 333dbdadb..69e50b6f4 100644 --- a/config/site.defaults.yml +++ b/config/site.defaults.yml @@ -118,6 +118,8 @@ valid_clients: min_version: 0.0.1 - name: tmc_cli min_version: 0.0.1 + - name: tmc_cli_rust + min_version: 0.0.0 - name: netbeans_plugin min_version: 0.2.22 - name: tmc.py diff --git a/config/site.dev.yml b/config/site.dev.yml index 6a68902ca..242a0601a 100644 --- a/config/site.dev.yml +++ b/config/site.dev.yml @@ -11,6 +11,8 @@ valid_clients: min_version: 0.0.1 - name: tmc_cli min_version: 0.0.1 + - name: tmc_cli_rust + min_version: 0.0.0 - name: netbeans_plugin min_version: 0.2.22 - name: tmc.py diff --git a/spec/controllers/api/v8/application/password_login_spec.rb b/spec/controllers/api/v8/application/password_login_spec.rb new file mode 100644 index 000000000..6c027d21e --- /dev/null +++ b/spec/controllers/api/v8/application/password_login_spec.rb @@ -0,0 +1,47 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +# The login tmc-langs does: fetch the client's OAuth credentials, then exchange a password for a token. +describe 'Password login of the TMC clients', type: :request do + let!(:user) { FactoryBot.create(:verified_user, password: 'correct horse') } + + def credentials(client_name) + get "/api/v8/application/#{client_name}/credentials" + response.parsed_body + end + + def exchange_password(app, password) + post '/oauth/token', params: { grant_type: 'password', username: user.email, password: password }, + headers: { 'Authorization' => ActionController::HttpAuthentication::Basic.encode_credentials(app['application_id'], app['secret']) } + end + + it 'gives every supported client the same credentials on each request' do + %w[vscode_plugin tmc_cli_rust].each do |client_name| + app = credentials(client_name) + expect(response).to have_http_status(:ok) + expect(app.keys).to contain_exactly('application_id', 'secret') + expect(credentials(client_name)).to eq(app) + end + end + + it 'refuses an unsupported client' do + get '/api/v8/application/unknown_client/credentials' + expect(response).to have_http_status(:forbidden) + expect(Doorkeeper::Application.where(name: 'unknown_client')).to be_empty + end + + it 'exchanges a correct password for a bearer token' do + exchange_password(credentials('vscode_plugin'), 'correct horse') + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to include('access_token' => be_present, 'token_type' => 'Bearer') + expect(Doorkeeper::AccessToken.find_by(token: response.parsed_body['access_token']).resource_owner_id).to eq(user.id) + end + + it 'refuses a wrong password' do + exchange_password(credentials('vscode_plugin'), 'wrong horse') + expect(response).to have_http_status(:bad_request) + expect(response.parsed_body['error']).to eq('invalid_grant') + end +end diff --git a/spec/controllers/api/v8/exercises/model_solutions_controller_spec.rb b/spec/controllers/api/v8/exercises/model_solutions_controller_spec.rb new file mode 100644 index 000000000..dd79b20a0 --- /dev/null +++ b/spec/controllers/api/v8/exercises/model_solutions_controller_spec.rb @@ -0,0 +1,62 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe Api::V8::Exercises::ModelSolutionsController, type: :controller do + let(:organization) { FactoryBot.create(:accepted_organization) } + let(:course) { FactoryBot.create(:course, organization: organization) } + let(:exercise) { FactoryBot.create(:exercise, course: course) } + let(:student) { FactoryBot.create(:verified_user) } + + before do + allow_any_instance_of(Solution).to receive(:files).and_return([]) + end + + def sign_in(as) + allow(controller).to receive(:doorkeeper_token).and_return(double(resource_owner_id: as.id, acceptable?: true)) + end + + def complete_exercises(count) + FactoryBot.create_list(:exercise, count, course: course).each do |ex| + FactoryBot.create(:submission, user: student, course: course, exercise: ex, all_tests_passed: true) + end + end + + it 'gives a teacher the solution and logs the access' do + teacher = FactoryBot.create(:verified_user) + Teachership.create!(user: teacher, organization: organization) + sign_in(teacher) + get :index, params: { exercise_id: exercise.id } + expect(response).to have_http_status(:ok) + expect(response.parsed_body).to eq('exercise' => { 'id' => exercise.id }, 'solution' => { 'files' => [] }) + expect(ModelSolutionAccessLog.where(user: teacher, exercise_name: exercise.name).count).to eq(1) + end + + it 'refuses a student who has not earned the solution' do + sign_in(student) + get :index, params: { exercise_id: exercise.id } + expect(response).to have_http_status(:forbidden) + expect(ModelSolutionAccessLog.count).to eq(0) + end + + it 'spends a token the student earned, and spends it only once' do + course.update!(grant_model_solution_token_every_nth_completed_exercise: 2) + complete_exercises(2) + sign_in(student) + 2.times do + get :index, params: { exercise_id: exercise.id } + expect(response).to have_http_status(:ok) + end + expect(ModelSolutionTokenUsed.where(user: student, exercise_name: exercise.name).count).to eq(1) + end + + it 'refuses a student whose tokens are spent' do + course.update!(grant_model_solution_token_every_nth_completed_exercise: 2) + complete_exercises(2) + ModelSolutionTokenUsed.create!(user: student, course: course, exercise_name: 'another', cost: 1) + sign_in(student) + get :index, params: { exercise_id: exercise.id } + expect(response).to have_http_status(:forbidden) + end +end diff --git a/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb b/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb new file mode 100644 index 000000000..ba609f000 --- /dev/null +++ b/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb @@ -0,0 +1,54 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe Api::V8::Exercises::Users::SubmissionsController, type: :controller do + let(:user) { FactoryBot.create(:verified_user) } + let(:exercise) { FactoryBot.create(:exercise) } + + def sign_in(as) + allow(controller).to receive(:doorkeeper_token).and_return(as && double(resource_owner_id: as.id, acceptable?: true)) + end + + def submission(owner, **attributes) + FactoryBot.create(:submission, user: owner, course: exercise.course, exercise: exercise, **attributes) + end + + it "lists the user's own submissions of the exercise" do + own = submission(user) + submission(FactoryBot.create(:user)) + sign_in(user) + get :index, params: { exercise_id: exercise.id, user_id: 'current' } + expect(response).to have_http_status(:ok) + expect(response.parsed_body.pluck('id')).to eq([own.id]) + end + + it "does not list another user's submissions to a student" do + submission(FactoryBot.create(:user)) + sign_in(user) + get :index, params: { exercise_id: exercise.id, user_id: Submission.last.user_id } + expect(response.parsed_body).to eq([]) + end + + it 'lists a null paste message as an empty string, which tmc-langs requires' do + submission(user, message_for_paste: nil) + sign_in(user) + get :index, params: { exercise_id: exercise.id, user_id: 'current' } + expect(response.parsed_body.first['message_for_paste']).to eq('') + end + + it 'hides the results of an exam-mode exercise without nulling the pass flag' do + exercise.update!(hide_submission_results: true) + submission(user, all_tests_passed: true) + sign_in(user) + get :index, params: { exercise_id: exercise.id, user_id: 'current' } + expect(response.parsed_body.first).to include('all_tests_passed' => false, 'points' => nil) + end + + it 'requires a signed-in user' do + sign_in(nil) + get :index, params: { exercise_id: exercise.id, user_id: 'current' } + expect(response).to have_http_status(:unauthorized) + end +end From 684d3b01f5016964e80913902591ddcd3848304f Mon Sep 17 00:00:00 2001 From: Henrik Nygren Date: Tue, 10 Nov 2026 09:49:57 +0200 Subject: [PATCH 11/11] Run the vscode extension's tmc-langs build against the server --- .github/workflows/ci.yml | 40 ++++ .../rust_langs_downloader_task.rb | 8 +- .../api/v8/organizations_controller.rb | 9 +- .../api/v8/users/teacherships_controller.rb | 11 +- app/models/submission.rb | 10 +- bin/spec_clients | 4 + lib/submission_list.rb | 3 +- spec/client_compat/clients.json | 8 + spec/client_compat/tmc_langs_clients_spec.rb | 189 ++++++++++++++++++ .../users/submissions_controller_spec.rb | 5 +- .../api/v8/organization_logos_spec.rb | 28 +++ .../v8/users/teacherships_controller_spec.rb | 19 ++ spec/support/tmc_langs_client.rb | 140 +++++++++++++ 13 files changed, 450 insertions(+), 24 deletions(-) create mode 100755 bin/spec_clients create mode 100644 spec/client_compat/clients.json create mode 100644 spec/client_compat/tmc_langs_clients_spec.rb create mode 100644 spec/controllers/api/v8/organization_logos_spec.rb create mode 100644 spec/controllers/api/v8/users/teacherships_controller_spec.rb create mode 100644 spec/support/tmc_langs_client.rb diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5eb29e31d..01f31093f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -147,3 +147,43 @@ jobs: run: docker compose run web bin/rake db:create db:schema:load RAILS_ENV=test - name: Run spec_integration tests run: docker compose run web bin/spec_integration + + client_matrix: + runs-on: ubuntu-latest + outputs: + clients: ${{ steps.manifest.outputs.clients }} + steps: + - name: Checkout repository + uses: actions/checkout@v4 + - name: Read the client manifest + id: manifest + run: echo "clients=$(jq -c . spec/client_compat/clients.json)" >> "$GITHUB_OUTPUT" + + spec_clients: + needs: client_matrix + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + client: ${{ fromJSON(needs.client_matrix.outputs.clients) }} + name: spec_clients (${{ matrix.client.client_name }} ${{ matrix.client.client_version }}) + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + submodules: false + - name: Setup git submodules + run: | + sed -i 's/git@github.com:/https:\/\/github.com\//' .gitmodules + git submodule update --init --recursive + - name: Cache tmc-langs-cli + uses: actions/cache@v4 + with: + path: vendor/tmc-langs-rust/clients + key: tmc-langs-cli-${{ matrix.client.langs_version }}-${{ matrix.client.sha256 }} + - name: Docker compose build + run: docker compose build + - name: Setup database & test env + run: docker compose run web bin/rake db:create db:schema:load RAILS_ENV=test + - name: Run spec_clients tests + run: docker compose run -e TMC_LANGS_CLIENT=${{ matrix.client.client_name }}@${{ matrix.client.client_version }} web bin/spec_clients diff --git a/app/background_tasks/rust_langs_downloader_task.rb b/app/background_tasks/rust_langs_downloader_task.rb index e713a694a..901b33b07 100644 --- a/app/background_tasks/rust_langs_downloader_task.rb +++ b/app/background_tasks/rust_langs_downloader_task.rb @@ -24,6 +24,12 @@ class BinaryDoesNotRun < StandardError; end FIRST_RETRY_DELAY_SECONDS = 60 MAX_RETRY_DELAY_SECONDS = 3600 + # The published x86_64 Linux build of a tmc-langs-cli version. + sig { params(version: String).returns(String) } + def self.download_url(version) + "https://download.mooc.fi/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{version}" + end + sig { void } def initialize @retry_delay_seconds = T.let(nil, T.nilable(Integer)) @@ -84,7 +90,7 @@ def wait_delay # Downloads the pinned version to `path`, raising DownloadFailed if wget fails. sig { params(path: String).void } def download!(path) - return if system('wget', "https://download.mooc.fi/tmc-langs-rust/tmc-langs-cli-x86_64-unknown-linux-gnu-#{RUST_LANGS_VERSION}", '-O', path) + return if system('wget', RustLangsDownloaderTask.download_url(RUST_LANGS_VERSION), '-O', path) raise DownloadFailed, "wget failed to download tmc-langs #{RUST_LANGS_VERSION}: #{Process.last_status.inspect}" end diff --git a/app/controllers/api/v8/organizations_controller.rb b/app/controllers/api/v8/organizations_controller.rb index ad69b2280..c10610f31 100644 --- a/app/controllers/api/v8/organizations_controller.rb +++ b/app/controllers/api/v8/organizations_controller.rb @@ -48,15 +48,10 @@ class OrganizationsController < Api::V8::BaseController sig { void } def index - orgs = Organization - .visible_organizations - .select { |org| org.visibility_allowed?(request, current_user) } - - orgs = Organization.all if current_user.administrator? orgs = if current_user.administrator? - orgs.map { |o| o.attributes.merge(o.org_as_json.stringify_keys) } + Organization.all.map { |o| o.attributes.merge(o.org_as_json.stringify_keys) } else - orgs.map(&:org_as_json) + Organization.visible_organizations.filter_map { |org| org.org_as_json if org.visibility_allowed?(request, current_user) } end authorize! :read, orgs present(orgs) diff --git a/app/controllers/api/v8/users/teacherships_controller.rb b/app/controllers/api/v8/users/teacherships_controller.rb index ad83061d4..076f42486 100644 --- a/app/controllers/api/v8/users/teacherships_controller.rb +++ b/app/controllers/api/v8/users/teacherships_controller.rb @@ -76,16 +76,7 @@ def index authorize! :read, user taught_organizations = T.let(user.teacherships.pluck(:organization_id), T::Array[Integer]) - readable = Organization.find(taught_organizations).map do |o| - { - 'organization_id': o.id, - 'name': o.name, - 'information': o.information, - 'slug': o.slug, - 'logo_path': o.logo_path, - 'pinned': o.pinned, - } - end + readable = Organization.find(taught_organizations).map { |o| o.org_as_json.merge(organization_id: o.id) } authorize! :read, readable present(readable) diff --git a/app/models/submission.rb b/app/models/submission.rb index c7adcd1d2..60dcec534 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -422,6 +422,13 @@ def set_paste_key_if_paste_available end # can_see_results? for many submissions seen by one viewer, evaluated once per course and exercise. + # all_tests_passed and points as submission listings show them. Hidden results stay a boolean + # false, not null, because tmc-langs requires one. + sig { params(visible: T::Boolean).returns(T::Hash[Symbol, T.untyped]) } + def listed_results(visible) + { all_tests_passed: visible && all_tests_passed, points: visible ? points : nil } + end + sig { params(viewer: User).returns(T.proc.params(submission: Submission).returns(T::Boolean)) } def self.results_visibility_for(viewer) visible_by_exercise = T.let({}, T::Hash[[T.nilable(Integer), T.nilable(String)], T::Boolean]) @@ -448,8 +455,7 @@ def self.filter_fields!(submissions, viewer) exercise_name: sub.exercise_name, course_id: sub.course_id, processed: sub.processed, - all_tests_passed: results_visible && sub.all_tests_passed, - points: results_visible ? sub.points : nil, + **sub.listed_results(results_visible), processing_tried_at: sub.processing_tried_at, processing_began_at: sub.processing_began_at, processing_completed_at: sub.processing_completed_at, diff --git a/bin/spec_clients b/bin/spec_clients new file mode 100755 index 000000000..5eacdf0a1 --- /dev/null +++ b/bin/spec_clients @@ -0,0 +1,4 @@ +#!/bin/sh + +RSPEC_PATTERN="spec/client_compat/**/*.rb" +bundle exec rake spec SPEC_OPTS="--pattern $RSPEC_PATTERN --format documentation" diff --git a/lib/submission_list.rb b/lib/submission_list.rb index c280658be..7c27983d4 100644 --- a/lib/submission_list.rb +++ b/lib/submission_list.rb @@ -25,8 +25,7 @@ def submission_data(submission) user_id: submission.user_id, course_id: submission.course_id, created_at: submission.created_at, - all_tests_passed: results_visible && submission.all_tests_passed, - points: results_visible ? submission.points : nil, + **submission.listed_results(results_visible), submitted_zip_url: @helpers.download_api_v8_core_submission_url(submission), paste_url: paste_url(submission), processing_time: submission.processing_time, diff --git a/spec/client_compat/clients.json b/spec/client_compat/clients.json new file mode 100644 index 000000000..0e267b0f7 --- /dev/null +++ b/spec/client_compat/clients.json @@ -0,0 +1,8 @@ +[ + { + "client_name": "vscode_plugin", + "client_version": "3.5.5", + "langs_version": "0.39.9", + "sha256": "7b69d627795ebef8fb7362000736dc936ad3e7c52bea81a88a1407a9180374a1" + } +] diff --git a/spec/client_compat/tmc_langs_clients_spec.rb b/spec/client_compat/tmc_langs_clients_spec.rb new file mode 100644 index 000000000..853c468c0 --- /dev/null +++ b/spec/client_compat/tmc_langs_clients_spec.rb @@ -0,0 +1,189 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +# Runs each client's own tmc-langs-cli build against the server, so a response the client +# can't parse fails here instead of in students' editors. +describe 'tmc-langs clients', integration: true do + include GitTestActions + include IntegrationTestActions + + let(:root_url) { "#{Capybara.current_session.server.base_url}/" } + # Organization information and message_for_paste are nullable, but tmc-langs requires strings. + let(:organization) { FactoryBot.create(:accepted_organization, slug: 'compat', information: nil) } + let(:teacher) { FactoryBot.create(:verified_user) } + let(:password) { 'correct horse battery' } + let(:student) { FactoryBot.create(:verified_user, password: password) } + + before do + Teachership.create!(user: teacher, organization: organization) + repo_path = "#{Dir.pwd}/remote_repo" + create_bare_repo(repo_path) + @course = FactoryBot.create(:course, name: 'compat-course', organization: organization, source_url: repo_path) + repo = clone_course_repo(@course) + repo.copy_simple_exercise('Basic') + repo.copy_simple_exercise('Locked') + repo.add_commit_push + @course.refresh(teacher.id) + RefreshCourseTask.new.run + @course.reload + + FactoryBot.create(:course, name: 'compat-hidden', organization: organization, hidden: true) + @exercise = @course.exercises.find_by!(name: 'Basic') + @exercise.deadline_spec = ['2099-12-31 23:59'].to_json + @exercise.soft_deadline_spec = ['2099-06-30 23:59'].to_json + @exercise.save! + locked = @course.exercises.find_by!(name: 'Locked') + locked.unlock_spec = ['exercise Basic'].to_json + locked.save! + @question = FactoryBot.create(:feedback_question, course: @course, question: 'How hard was it?') + + zip = FixtureExercise.get('SimpleExercise', "#{Dir.pwd}/web/Basic").tap(&:make_zip).zip_path + @web_submission = Submission.create!(user: student, course: @course, exercise: @exercise, processed: true, + return_file: File.binread(zip), message_for_paste: nil) + @reviewed_submission = Submission.create!(user: student, course: @course, exercise: @exercise, processed: true, reviewed: true, + return_file: File.binread(zip)) + Review.create!(submission: @reviewed_submission, reviewer: teacher, review_body: 'Looks good', marked_as_read: false) + end + + TmcLangsClient.manifest.each do |client| + context "#{client[:client_name]} #{client[:client_version]} (tmc-langs #{client[:langs_version]})" do + around do |example| + Dir.mktmpdir do |dir| + @langs = TmcLangsClient.new(client, root_url, dir) + example.run + end + end + + def log_in + @langs.tmc('login', '--base64', '--email', student.email, '--stdin', stdin: Base64.strict_encode64(password), kind: nil) + end + + def download_exercise(*flags) + downloaded = @langs.tmc('download-or-update-course-exercises', *flags, '--exercise-id', @exercise.id, kind: 'tmc-exercise-download')['downloaded'] + expect(downloaded.pluck('id')).to eq([@exercise.id]) + downloaded.first['path'] + end + + # Feeds the sandbox result for the submission `run` posted, through the endpoint the sandbox calls. + def finish_submission(run, known_submission_ids) + submission = nil + wait_until(timeout: 30) { submission = Submission.where.not(id: known_submission_ids).first } + sleep 2.5 # lets the client poll the processing submission a couple of times + expect(run).to be_running + + submission.update!(secret_token: 'sandbox-token') + test_output = { + status: 'PASSED', + testResults: [{ name: 'SimpleTest testEverything', successful: true, points: @exercise.available_points.map(&:name), message: '', exception: nil }], + logs: {} + } + response = Net::HTTP.post_form(URI("#{root_url}submissions/#{submission.id}/result"), + token: 'sandbox-token', status: 'finished', exit_code: '0', test_output: test_output.to_json) + expect(response.code).to eq('200') + submission + end + + it 'logs in with a password and is refused once the token is revoked' do + log_in + expect(@langs.tmc('logged-in', kind: 'token')).to include('access_token', 'token_type') + expect(File).to exist(@langs.credentials_path) + + Doorkeeper::AccessToken.where(resource_owner_id: student.id).find_each(&:revoke) + error = @langs.start('get-course-details', '--course-id', @course.id).error + # tmc-langs 0.39.9 keeps credentials.json after this 401, so the user has to log out before logging in again. + expect(error['trace'].join).to include('401 Unauthorized') + @langs.tmc('logout', kind: nil) + log_in + expect(@langs.tmc('get-course-details', '--course-id', @course.id, kind: 'course-details')['id']).to eq(@course.id) + + @langs.tmc('logout', kind: nil) + expect(@langs.tmc('logged-in', kind: nil)).to be_nil + end + + it 'browses organizations, courses and exercises' do + log_in + expect(@langs.tmc('get-organizations', kind: 'organizations').pluck('slug')).to eq(['compat']) + expect(@langs.tmc('get-organization', '--organization', 'compat', kind: 'organization')) + .to include('slug' => 'compat', 'logo_path' => 'missing.png') + expect(@langs.tmc('get-courses', '--organization', 'compat', kind: 'courses').pluck('name')).to eq(['compat-course']) + + details = @langs.tmc('get-course-details', '--course-id', @course.id, kind: 'course-details') + expect(details['exercises'].pluck('name')).to eq(['Basic']) + expect(details['exercises'].first).to include('locked' => false, 'checksum' => @exercise.checksum, + 'deadline' => start_with('2099-12-31'), 'soft_deadline' => start_with('2099-06-30')) + + exercises = @langs.tmc('get-course-exercises', '--course-id', @course.id, kind: 'course-exercises') + expect(exercises.find { |ex| ex['name'] == 'Basic' }['available_points'].pluck('name')).to match_array(@exercise.available_points.map(&:name)) + expect(@langs.tmc('get-course-settings', '--course-id', @course.id, kind: 'course-data')).to include('name' => 'compat-course', 'hidden' => false) + expect(@langs.tmc('get-course-data', '--course-id', @course.id, kind: 'combined-course-data').keys).to contain_exactly('details', 'exercises', 'settings') + + expect(@langs.tmc('get-exercise-details', '--exercise-id', @exercise.id, kind: 'exercise-details')['exercise_name']).to eq('Basic') + submissions = @langs.tmc('get-exercise-submissions', '--exercise-id', @exercise.id, kind: 'submissions') + expect(submissions.pluck('id', 'reviewed')).to contain_exactly([@web_submission.id, false], [@reviewed_submission.id, true]) + end + + it 'downloads, updates and resets exercises' do + log_in + exercise_path = download_exercise('--download-template') + source = File.join(exercise_path, 'src/SimpleStuff.java') + expect(File).to exist(source) + redownload = @langs.tmc('download-or-update-course-exercises', '--exercise-id', @exercise.id, kind: 'tmc-exercise-download') + expect(redownload['skipped'].pluck('id')).to eq([@exercise.id]) + + expect(@langs.tmc('check-exercise-updates', kind: 'updated-exercises')).to eq([]) + @exercise.update!(checksum: 'changed-on-the-server') + expect(@langs.tmc('check-exercise-updates', kind: 'updated-exercises')).to eq([{ 'id' => @exercise.id }]) + + saved_states = Submission.where(exercise: @exercise, user: student) + expect do + File.write(source, 'edited') + @langs.tmc('download-old-submission', '--submission-id', @web_submission.id, '--save-old-state', + '--exercise-id', @exercise.id, '--output-path', exercise_path, kind: nil) + expect(File.read(source)).to eq(File.read(File.join(FixtureExercise.fixture_exercises_root, 'SimpleExercise/src/SimpleStuff.java'))) + + File.write(source, 'edited again') + @langs.tmc('reset-exercise', '--save-old-state', '--exercise-id', @exercise.id, '--exercise-path', exercise_path, kind: nil) + expect(File.read(source)).not_to eq('edited again') + end.to change(saved_states, :count).by(2) + end + + it 'submits, waits for the results, sends feedback and pastes' do + log_in + exercise_path = download_exercise + known = Submission.pluck(:id) + run = @langs.start('submit', '--submission-path', exercise_path, '--exercise-id', @exercise.id) + submission = finish_submission(run, known) + + result = run.result.dig('data', 'output-data') + expect(result).to include('status' => 'ok', 'all_tests_passed' => true, 'exercise_name' => 'Basic') + expect(result['points']).to match_array(@exercise.available_points.map(&:name)) + expect(result['feedback_questions'].pluck('id')).to eq([@question.id]) + + @langs.tmc('send-feedback', '--feedback-url', result['feedback_answer_url'], '--feedback', @question.id, 'Not very', + kind: 'submission-feedback-response') + expect(FeedbackAnswer.where(submission: submission).pluck(:answer)).to eq(['Not very']) + + paste = @langs.tmc('paste', '--exercise-id', @exercise.id, '--submission-path', exercise_path, kind: 'new-submission') + expect(paste['paste_url']).to be_present + end + + it 'works on an exercise whose results are hidden until the exam ends' do + @exercise.update!(hide_submission_results: true) + @web_submission.update!(all_tests_passed: true) + log_in + + listed = @langs.tmc('get-exercise-submissions', '--exercise-id', @exercise.id, kind: 'submissions').find { |sub| sub['id'] == @web_submission.id } + expect(listed).to include('all_tests_passed' => false, 'points' => nil) + details = @langs.tmc('get-exercise-details', '--exercise-id', @exercise.id, kind: 'exercise-details') + expect(details['submissions'].find { |sub| sub['id'] == @web_submission.id }).to include('all_tests_passed' => false) + + exercise_path = download_exercise + # The client stops polling as soon as the server says the results are hidden. + result = @langs.tmc('submit', '--submission-path', exercise_path, '--exercise-id', @exercise.id, kind: 'submission-finished') + expect(result).to include('status' => 'hidden', 'points' => []) + end + end + end +end diff --git a/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb b/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb index ba609f000..e0c40aac7 100644 --- a/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb +++ b/spec/controllers/api/v8/exercises/users/submissions_controller_spec.rb @@ -25,9 +25,10 @@ def submission(owner, **attributes) end it "does not list another user's submissions to a student" do - submission(FactoryBot.create(:user)) + other = FactoryBot.create(:user) + submission(other) sign_in(user) - get :index, params: { exercise_id: exercise.id, user_id: Submission.last.user_id } + get :index, params: { exercise_id: exercise.id, user_id: other.id } expect(response.parsed_body).to eq([]) end diff --git a/spec/controllers/api/v8/organization_logos_spec.rb b/spec/controllers/api/v8/organization_logos_spec.rb new file mode 100644 index 000000000..895da47fb --- /dev/null +++ b/spec/controllers/api/v8/organization_logos_spec.rb @@ -0,0 +1,28 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +# The vscode extension's webview loads `` itself, and +# `/logos/small_logo/missing.png` when logo_path ends in missing.png. +describe 'Organization logos the clients load', type: :request do + let(:organization) { FactoryBot.create(:accepted_organization) } + + it 'serves the placeholder logo' do + get '/logos/small_logo/missing.png' + expect(response).to have_http_status(:ok) + expect(response.media_type).to eq('image/png') + end + + it 'serves an uploaded logo at the logo_path the API lists' do + organization.logo.attach(io: StringIO.new(Vips::Image.black(300, 120).pngsave_buffer), filename: 'logo.png', content_type: 'image/png') + get "/api/v8/org/#{organization.slug}.json" + logo_path = response.parsed_body['logo_path'] + expect(logo_path).to start_with('/') + + get logo_path + get response.location while response.redirect? + expect(response).to have_http_status(:ok) + expect(Vips::Image.new_from_buffer(response.body, '').width).to eq(100) + end +end diff --git a/spec/controllers/api/v8/users/teacherships_controller_spec.rb b/spec/controllers/api/v8/users/teacherships_controller_spec.rb new file mode 100644 index 000000000..9915168da --- /dev/null +++ b/spec/controllers/api/v8/users/teacherships_controller_spec.rb @@ -0,0 +1,19 @@ +# typed: false +# frozen_string_literal: true + +require 'spec_helper' + +describe Api::V8::Users::TeachershipsController, type: :controller do + let(:teacher) { FactoryBot.create(:verified_user) } + let(:organization) { FactoryBot.create(:accepted_organization, information: nil) } + + before do + Teachership.create!(user: teacher, organization: organization) + allow(controller).to receive(:doorkeeper_token).and_return(double(resource_owner_id: teacher.id, acceptable?: true)) + end + + it "lists the user's organizations, with missing information as an empty string" do + get :index, params: { user_id: 'current' } + expect(response.parsed_body).to match([include('organization_id' => organization.id, 'slug' => organization.slug, 'information' => '')]) + end +end diff --git a/spec/support/tmc_langs_client.rb b/spec/support/tmc_langs_client.rb new file mode 100644 index 000000000..4f8f0de34 --- /dev/null +++ b/spec/support/tmc_langs_client.rb @@ -0,0 +1,140 @@ +# typed: false +# frozen_string_literal: true + +require 'digest' +require 'fileutils' +require 'net/http' +require 'open3' +require 'tmpdir' + +# Drives the published tmc-langs-cli build that a client ships, against the test server, +# the way that client does. Entries come from spec/client_compat/clients.json. +class TmcLangsClient + # A tmc-langs command failed, timed out or printed output of an unexpected kind. + class CommandFailed < StandardError; end + + # A command running in the background, like the blocking `submit` a client waits on. + class Run + def initialize(command, env, stdin) + @command = command + stdin_pipe, stdout, stderr, @wait_thread = Open3.popen3(env, *command) + @stdout = Thread.new { stdout.read.tap { stdout.close } } + @stderr = Thread.new { stderr.read.tap { stderr.close } } + begin + stdin_pipe.write(stdin) if stdin + rescue Errno::EPIPE + nil # it exited without reading stdin; its output says why + end + stdin_pipe.close + end + + def running? + @wait_thread.alive? + end + + # The last output line, parsed; raises unless the command finished successfully. + def result(timeout: TIMEOUT_SECONDS) + last = last_line(timeout) + raise CommandFailed, "#{description} failed\n#{output}" if last['result'] == 'error' + last + end + + # The error output data of a command that is expected to fail. + def error(timeout: TIMEOUT_SECONDS) + last = last_line(timeout) + raise CommandFailed, "#{description} did not fail\n#{output}" unless last['result'] == 'error' + last.dig('data', 'output-data') + end + + private + def last_line(timeout) + unless @wait_thread.join(timeout) + begin + Process.kill('KILL', @wait_thread.pid) + rescue Errno::ESRCH + nil + end + raise CommandFailed, "#{description} timed out after #{timeout} s\n#{output}" + end + last = @stdout.value.lines.reverse_each.lazy.filter_map { |line| JSON.parse(line) rescue nil }.first + raise CommandFailed, "#{description} printed no result\n#{output}" unless last && last['status'] == 'finished' + last + end + + def description + @command.drop(1).join(' ') + end + + def output + "stdout:\n#{@stdout.value}\nstderr:\n#{@stderr.value}" + end + end + + TIMEOUT_SECONDS = 60 + MANIFEST_PATH = Rails.root.join('spec/client_compat/clients.json') + + # TMC_LANGS_CLIENT=@ limits the suite to one client, as each CI job does. + def self.manifest + clients = JSON.parse(File.read(MANIFEST_PATH), symbolize_names: true) + only = ENV['TMC_LANGS_CLIENT'].presence + return clients unless only + + clients.select { |client| "#{client[:client_name]}@#{client[:client_version]}" == only }.presence || + raise("TMC_LANGS_CLIENT=#{only} matches no client in #{MANIFEST_PATH}") + end + + # client: one manifest entry. root_url: the server the client talks to. + def initialize(client, root_url, dir) + @client_name = client[:client_name] + @client_version = client[:client_version] + @credentials_path = File.join(dir, 'config', "tmc-#{@client_name}", 'credentials.json') + @executable = self.class.fetch_executable(client[:langs_version], client[:sha256]) + @env = { + 'TMC_LANGS_TMC_ROOT_URL' => root_url, + # Nothing listens here; the commands under test never need courses.mooc.fi. + 'TMC_LANGS_MOOC_ROOT_URL' => 'http://127.0.0.1:9/', + 'TMC_LANGS_CONFIG_DIR' => File.join(dir, 'config'), + 'TMC_LANGS_DEFAULT_PROJECTS_DIR' => File.join(dir, 'projects'), + 'RUST_LOG' => 'info' + } + end + + # Runs `tmc ` and returns the output data, checking it is of the expected kind (nil for none). + def tmc(*args, kind:, stdin: nil) + data = start(*args, stdin: stdin).result['data'] || {} + output_kind = data['output-data-kind'] + raise CommandFailed, "#{args.first} returned #{output_kind.inspect}, not #{kind.inspect}" unless output_kind == kind + data['output-data'] + end + + # Starts `tmc ` and returns the Run without waiting, for a command expected to fail or to + # check on while it runs; #tmc is the blocking form. + def start(*args, stdin: nil) + Run.new([@executable, 'tmc', '--client-name', @client_name, '--client-version', @client_version, *args.map(&:to_s)], @env, stdin) + end + + # Where the client keeps its OAuth token. + attr_reader :credentials_path + + # Downloads the build once into vendor/, next to the one the server packages submissions with. + def self.fetch_executable(version, sha256) + @executables ||= {} + @executables[[version, sha256]] ||= download_executable(version, sha256) + end + + private_class_method def self.download_executable(version, sha256) + path = Rails.root.join('vendor/tmc-langs-rust/clients', "tmc-langs-cli-#{version}").to_s + return path if File.exist?(path) && Digest::SHA256.file(path).hexdigest == sha256 + + response = Net::HTTP.get_response(URI(RustLangsDownloaderTask.download_url(version))) + raise "Downloading tmc-langs-cli #{version} failed: HTTP #{response.code}" unless response.is_a?(Net::HTTPSuccess) + actual = Digest::SHA256.hexdigest(response.body) + raise "tmc-langs-cli #{version} has checksum #{actual}, expected #{sha256}" unless actual == sha256 + + FileUtils.mkdir_p(File.dirname(path)) + File.binwrite("#{path}.part", response.body) + File.chmod(0o755, "#{path}.part") + File.rename("#{path}.part", path) + path + end +end