Close access control and input handling holes, test against tmc-langs

.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

app/background_tasks/rust_langs_downloader_task.rb

@@ -1,41 +1,70 @@
 # 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-<version>.sha256.
+  RUST_LANGS_VERSION = '0.39.8'
+  RUST_LANGS_SHA256 = '0c39368c5c70fb89ae30fe38619c1870522ee512ee67f6f59ab8e5a2dfd45f73'
+
+  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))
   end
 
   sig { void }
   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 +79,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', RustLangsDownloaderTask.download_url(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

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

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

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.
@@ -70,6 +77,27 @@ 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. 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
+          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(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]]) }
         def errors_json(messages)
           { errors: [*messages] }

app/controllers/api/v8/core/exercises/details_controller.rb

@@ -49,8 +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)
-            authorize! :read, exercises
+            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,

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
               )
 

app/controllers/api/v8/core/submissions/feedback_controller.rb

@@ -56,6 +56,9 @@ def create
             end
 
             answer_records.each { |record| authorize! :create, record }
+            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
               ActiveRecord::Base.connection.transaction(requires_new: true) do
                 answer_records.each(&:save!)

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

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

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

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

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 = readable_submission_fields(submissions)
 
           authorize! :read, readable
           present(readable)

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

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 = readable_submission_fields(submissions)
 
             authorize! :read, readable
             present(readable)

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 = readable_submission_fields(submissions)
 
             authorize! :read, readable
             present(readable)

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

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

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

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 = readable_submission_fields(submissions)
 
             authorize_collection :read, readable
             present(readable)