From 16c00ec13671af0ce2bddf3ef9c29109c8c7a28d Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Fri, 23 Sep 2022 11:26:56 +0200 Subject: [PATCH] Add support for signed URLs used by the render_file function --- Gemfile | 1 + Gemfile.lock | 1 + app/assets/javascripts/editor/submissions.js | 6 +- app/controllers/application_controller.rb | 12 +- .../code_ocean/files_controller.rb | 18 +++ app/controllers/submissions_controller.rb | 45 ++++--- app/helpers/authenticated_url_helper.rb | 111 ++++++++++++++++++ app/policies/code_ocean/file_policy.rb | 8 ++ app/policies/submission_policy.rb | 6 +- app/views/submissions/show.json.jbuilder | 10 +- config/action_mailer.yml.ci | 1 - config/action_mailer.yml.example | 1 - config/code_ocean.yml.example | 4 + .../initializers/content_security_policy.rb | 1 + config/routes.rb | 1 + .../submissions_controller_spec.rb | 34 ++++-- 16 files changed, 229 insertions(+), 31 deletions(-) create mode 100644 app/helpers/authenticated_url_helper.rb diff --git a/Gemfile b/Gemfile index ca7b3088..cc2e5d00 100644 --- a/Gemfile +++ b/Gemfile @@ -19,6 +19,7 @@ gem 'ims-lti', '< 2.0.0' gem 'jbuilder' gem 'json_schemer' gem 'js-routes' +gem 'jwt' gem 'kramdown' gem 'mimemagic' gem 'net-http-persistent' diff --git a/Gemfile.lock b/Gemfile.lock index 8e8468d3..734558c1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -571,6 +571,7 @@ DEPENDENCIES jbuilder js-routes json_schemer + jwt kramdown letter_opener listen diff --git a/app/assets/javascripts/editor/submissions.js b/app/assets/javascripts/editor/submissions.js index e8cae56e..b2dcafee 100644 --- a/app/assets/javascripts/editor/submissions.js +++ b/app/assets/javascripts/editor/submissions.js @@ -134,7 +134,11 @@ CodeOceanEditorSubmissions = { event.preventDefault(); if ($('#render').is(':visible')) { this.createSubmission('#render', null, function (response) { - var url = response.render_url.replace(this.FILENAME_URL_PLACEHOLDER, CodeOceanEditor.active_file.filename.replace(/#$/,'')); // remove # if it is the last character, this is not part of the filename and just an anchor + if (response.render_url === undefined) return; + + const active_file = CodeOceanEditor.active_file.filename.replace(/#$/,''); // remove # if it is the last character, this is not part of the filename and just an anchor + const desired_file = response.render_url.filter(hash => hash.filepath === active_file); + const url = desired_file[0].url; var pop_up_window = window.open(url); if (pop_up_window) { pop_up_window.onerror = function (message) { diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 9836e10b..f4e0d041 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -7,6 +7,7 @@ class ApplicationController < ActionController::Base MEMBER_ACTIONS = %i[destroy edit show update].freeze RENDER_HOST = CodeOcean::Config.new(:code_ocean).read[:render_host] + before_action :deny_access_from_render_host after_action :verify_authorized, except: %i[welcome] around_action :mnemosyne_trace around_action :switch_locale @@ -68,7 +69,7 @@ class ApplicationController < ActionController::Base id: current_user.id, type: current_user.class.name, username: current_user.displayname, - consumer: current_user.consumer.name + consumer: current_user.consumer&.name ) end private :set_sentry_context @@ -95,10 +96,13 @@ class ApplicationController < ActionController::Base def render_error(message, status) set_sentry_context respond_to do |format| - format.html do + format.any do # Prevent redirect loop if request.url == request.referer redirect_to :root, alert: message + # Redirect to main domain if the request originated from our render_host + elsif request.path == '/' && request.host == RENDER_HOST + redirect_to Rails.application.config.action_mailer.default_url_options else redirect_back fallback_location: :root, allow_other_host: false, alert: message end @@ -116,6 +120,10 @@ class ApplicationController < ActionController::Base end private :switch_locale + def deny_access_from_render_host + raise Pundit::NotAuthorizedError if RENDER_HOST.present? && request.host == RENDER_HOST + end + def welcome # Show root page end diff --git a/app/controllers/code_ocean/files_controller.rb b/app/controllers/code_ocean/files_controller.rb index 4d87684e..5c1c46c1 100644 --- a/app/controllers/code_ocean/files_controller.rb +++ b/app/controllers/code_ocean/files_controller.rb @@ -5,6 +5,12 @@ module CodeOcean include CommonBehavior include FileParameters + # Overwrite the CSP header and some default actions for the :render_protected_upload action + content_security_policy false, only: :render_protected_upload + skip_before_action :deny_access_from_render_host, only: :render_protected_upload + skip_before_action :verify_authenticity_token, only: :render_protected_upload + before_action :require_user!, except: :render_protected_upload + def authorize! authorize(@file) end @@ -19,6 +25,18 @@ module CodeOcean send_file(real_location, type: @file.native_file.content_type, filename: @file.name_with_extension, disposition: 'attachment') end + def render_protected_upload + # Set @current_user with a new *learner* for Pundit checks + @current_user = ExternalUser.new + + @file = authorize AuthenticatedUrlHelper.retrieve!(CodeOcean::File, request) + + raise Pundit::NotAuthorizedError unless @file.name_with_extension == params[:filename] + + real_location = Pathname(@file.native_file.current_path).realpath + send_file(real_location, type: @file.native_file.content_type, filename: @file.name_with_extension) + end + def create @file = CodeOcean::File.new(file_params) if @file.file_template_id diff --git a/app/controllers/submissions_controller.rb b/app/controllers/submissions_controller.rb index 71fe97f5..8e8e0993 100644 --- a/app/controllers/submissions_controller.rb +++ b/app/controllers/submissions_controller.rb @@ -7,22 +7,17 @@ class SubmissionsController < ApplicationController include SubmissionParameters include Tubesock::Hijack - before_action :require_user! - before_action :set_submission, only: %i[download download_file render_file run score show statistics test] + before_action :set_submission, only: %i[download download_file run score show statistics test] before_action :set_testrun, only: %i[run score test] before_action :set_files, only: %i[download show] - before_action :set_files_and_specific_file, only: %i[download_file render_file run test] + before_action :set_files_and_specific_file, only: %i[download_file run test] before_action :set_content_type_nosniff, only: %i[download download_file render_file] - # Overwrite the CSP header for the :render_file action - content_security_policy only: :render_file do |policy| - policy.img_src :none - policy.script_src :none - policy.font_src :none - policy.style_src :none - policy.connect_src :none - policy.form_action :none - end + # Overwrite the CSP header and some default actions for the :render_file action + content_security_policy false, only: :render_file + skip_before_action :deny_access_from_render_host, only: :render_file + skip_before_action :verify_authenticity_token, only: :render_file + before_action :require_user!, except: :render_file def create @submission = Submission.new(submission_params) @@ -80,10 +75,28 @@ class SubmissionsController < ApplicationController end def render_file - # If a file should not be downloaded, it should not be rendered either - raise Pundit::NotAuthorizedError if @embed_options[:disable_download] + # Set @current_user with a new *learner* for Pundit checks + @current_user = ExternalUser.new - send_data(@file.read, filename: @file.name_with_extension, disposition: 'inline') + @submission = authorize AuthenticatedUrlHelper.retrieve!(Submission, request, cookies) + + # Throws an exception if the file is not found + set_files_and_specific_file + + # Allows access to other files of the same submission, e.g., a linked JS or CSS file where we cannot expect a token in the URL + cookie_name = AuthenticatedUrlHelper.cookie_name_for(:render_file_token) + if params[AuthenticatedUrlHelper.query_parameter].present? + cookies[cookie_name] = AuthenticatedUrlHelper.prepare_short_living_cookie(request.url) + cookies.commit! + end + + # Finally grant access and send the file + if @file.native_file? + url = render_protected_upload_url(id: @file.id, filename: @file.name_with_extension) + redirect_to AuthenticatedUrlHelper.sign(url, @file) + else + send_data(@file.content, filename: @file.name_with_extension, disposition: 'inline') + end end # rubocop:disable Metrics/CyclomaticComplexity @@ -372,7 +385,7 @@ class SubmissionsController < ApplicationController # @file contains the specific file requested for run / test / render / ... set_files @file = @files.detect {|file| file.filepath == sanitize_filename } - head :not_found unless @file + raise ActiveRecord::RecordNotFound unless @file end def set_files diff --git a/app/helpers/authenticated_url_helper.rb b/app/helpers/authenticated_url_helper.rb new file mode 100644 index 00000000..f77b63fe --- /dev/null +++ b/app/helpers/authenticated_url_helper.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +module AuthenticatedUrlHelper + include Pundit::Authorization + + class << self + TOKEN_ALGORITHM = 'HS512' + TOKEN_EXPIRATION = 10.minutes + TOKEN_SECRET = Rails.application.secrets.secret_key_base + TOKEN_PARAM = :token + COOKIE_EXPIRATION = 30.seconds + + def sign(url, object) + payload = {object_id: object.id, object_type: object.class.name, url: url, exp: TOKEN_EXPIRATION.from_now.to_i} + token = JWT.encode payload, TOKEN_SECRET, TOKEN_ALGORITHM + + add_query_parameters(url, {TOKEN_PARAM => token}) + end + + def retrieve!(klass, request, cookies = {}) + # Don't use the default session mechanism and default cookie + request.session_options[:skip] = true + # Show errors as JSON format, if any + request.format = :json + + # Disallow access from normal domain and show an error instead + if ApplicationController::RENDER_HOST.present? && request.host != ApplicationController::RENDER_HOST + raise Pundit::NotAuthorizedError + end + + cookie_name = AuthenticatedUrlHelper.cookie_name_for(:render_file_token) + object = klass.find(request.parameters[:id]) + + signed_url = request.parameters[TOKEN_PARAM].present? ? request.url : cookies[cookie_name] + # Throws an exception if the token is not matching the object or has expired + AuthenticatedUrlHelper.verify!(signed_url, object, klass) + + object + end + + def verify!(url, object, klass) + original_url, removed_parameters = remove_query_parameters(url, [TOKEN_PARAM]) + expected_payload = {object_id: object.id, object_type: klass.name, url: original_url} + token = removed_parameters[TOKEN_PARAM] + + begin + payload, = JWT.decode token, TOKEN_SECRET, true, algorithm: TOKEN_ALGORITHM + rescue JWT::DecodeError + raise Pundit::NotAuthorizedError + end + + raise Pundit::NotAuthorizedError unless payload.symbolize_keys.except(:exp) == expected_payload + end + + def prepare_short_living_cookie(value) + { + value: value, + expires: COOKIE_EXPIRATION.from_now, + httponly: true, + same_site: :strict, + secure: Rails.env.production? || Rails.env.staging?, + path: Rails.application.config.relative_url_root, + } + end + + def cookie_name_for(name) + if (Rails.env.production? || Rails.env.staging?) \ + && Rails.application.config.relative_url_root == '/' + "__Host-#{name}" + elsif Rails.env.production? || Rails.env.staging? + "__Secure-#{name}" + else + name + end + end + + def query_parameter + TOKEN_PARAM + end + + private + + def add_query_parameters(url, parameters) + parsed_url = URI.parse url + + # Add the given parameters to the query string + query_params = CGI.parse(parsed_url.query || '') + query_params.merge!(parameters) + + # Add the query string back to the URL + parsed_url.query = URI.encode_www_form(query_params) + + # Return the full URL + parsed_url.to_s + end + + def remove_query_parameters(url, parameters) + parsed_url = URI.parse url + + # Remove the given parameters from the query string + query_params = Rack::Utils.parse_nested_query(parsed_url.query || '') + removed_params = query_params.slice!(parameters) + + # Add the query string back to the URL + parsed_url.query = URI.encode_www_form(query_params).presence + + # Return the full URL and removed parameters + [parsed_url.to_s, removed_params.symbolize_keys] + end + end +end diff --git a/app/policies/code_ocean/file_policy.rb b/app/policies/code_ocean/file_policy.rb index d5be20cb..450b5968 100644 --- a/app/policies/code_ocean/file_policy.rb +++ b/app/policies/code_ocean/file_policy.rb @@ -26,6 +26,14 @@ module CodeOcean end end + def render_protected_upload? + return no_one if @record.native_file? && !@record.native_file_location_valid? + return no_one if @record.context.is_a?(Exercise) && (@record.context.unpublished || @record.hidden) + + # The AuthenticatedUrlHelper will check for more details, but we cannot determine a specific user + everyone + end + def create? if @record.context.is_a?(Exercise) admin? || author? diff --git a/app/policies/submission_policy.rb b/app/policies/submission_policy.rb index 63a7b2b7..c174c074 100644 --- a/app/policies/submission_policy.rb +++ b/app/policies/submission_policy.rb @@ -6,11 +6,15 @@ class SubmissionPolicy < ApplicationPolicy end # insights? is used in the flowr_controller.rb as we use it to authorize the user for a submission - %i[download? download_file? render_file? run? score? show? statistics? stop? test? + %i[download? download_file? run? score? show? statistics? stop? test? insights?].each do |action| define_method(action) { admin? || author? } end + def render_file? + everyone + end + def index? admin? end diff --git a/app/views/submissions/show.json.jbuilder b/app/views/submissions/show.json.jbuilder index 9c280bee..6922deff 100644 --- a/app/views/submissions/show.json.jbuilder +++ b/app/views/submissions/show.json.jbuilder @@ -5,6 +5,14 @@ json.download_url download_submission_path(@submission, format: :json) json.score_url score_submission_path(@submission, format: :json) json.download_file_url download_file_submission_path(@submission, 'a.', format: :json).gsub(/a\.\.json$/, '{filename}.json') -json.render_url render_submission_path(@submission, 'a.', format: :json).gsub(/a\.\.json$/, '{filename}.json') +unless @embed_options[:disable_download] + json.render_url @submission.collect_files.select(&:visible) do |files| + host = ApplicationController::RENDER_HOST || request.host + url = render_submission_url(@submission, files.filepath, host: host) + + json.filepath files.filepath + json.url AuthenticatedUrlHelper.sign(url, @submission) + end +end json.run_url run_submission_path(@submission, 'a.', format: :json).gsub(/a\.\.json$/, '{filename}.json') json.test_url test_submission_path(@submission, 'a.', format: :json).gsub(/a\.\.json$/, '{filename}.json') diff --git a/config/action_mailer.yml.ci b/config/action_mailer.yml.ci index c9830072..7e407bdb 100644 --- a/config/action_mailer.yml.ci +++ b/config/action_mailer.yml.ci @@ -3,5 +3,4 @@ test: from: codeocean@hpi.de default_url_options: host: localhost - port: 3000 delivery_method: :test diff --git a/config/action_mailer.yml.example b/config/action_mailer.yml.example index 503b8447..43a279dc 100644 --- a/config/action_mailer.yml.example +++ b/config/action_mailer.yml.example @@ -19,5 +19,4 @@ test: <<: *default default_url_options: host: localhost - port: 3000 delivery_method: :test diff --git a/config/code_ocean.yml.example b/config/code_ocean.yml.example index 207038d5..03547875 100644 --- a/config/code_ocean.yml.example +++ b/config/code_ocean.yml.example @@ -1,4 +1,8 @@ default: &default + # A public-facing host to be used for the render_file function of the SubmissionsController. + # User content will be served from this host. If not set, the default host is used (less secure!). + # render_host: codeoceanusercontent.com + flowr: # When enabled, flowr can assist learners with related search results from # StackOverflow.com regarding exceptions that occurred during code execution. diff --git a/config/initializers/content_security_policy.rb b/config/initializers/content_security_policy.rb index 9bef6035..8faf0242 100644 --- a/config/initializers/content_security_policy.rb +++ b/config/initializers/content_security_policy.rb @@ -38,6 +38,7 @@ Rails.application.config.content_security_policy do |policy| # Code executions might return a base64 encoded image as a :data URI policy.img_src :self, :data policy.object_src :none + policy.media_src :self policy.script_src :self, :report_sample # Our ACE editor unfortunately requires :unsafe_inline for the code highlighting policy.style_src :self, :unsafe_inline, :report_sample diff --git a/config/routes.rb b/config/routes.rb index a1a5e0f7..b6882d99 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -129,6 +129,7 @@ Rails.application.routes.draw do resources :files, only: %i[create destroy] end get '/uploads/files/:id/:filename', to: 'code_ocean/files#show_protected_upload', as: :protected_upload, constraints: {filename: FILENAME_REGEXP} + get '/uploads/render_files/:id/:filename', to: 'code_ocean/files#render_protected_upload', as: :render_protected_upload, constraints: {filename: FILENAME_REGEXP} resources :file_types diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index a55e1cf9..916b561d 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -40,7 +40,7 @@ describe SubmissionsController do describe 'GET #download_file' do context 'with an invalid filename' do - before { get :download_file, params: {filename: SecureRandom.hex, id: submission.id} } + before { get :download_file, params: {filename: SecureRandom.hex, id: submission.id, format: :json} } expect_http_status(:not_found) end @@ -108,28 +108,32 @@ describe SubmissionsController do describe 'GET #render_file' do let(:file) { submission.files.first } + let(:signed_url) { AuthenticatedUrlHelper.sign(render_submission_url(submission, filename), submission) } + let(:token) { Rack::Utils.parse_nested_query(URI.parse(signed_url).query)['token'] } context 'with an invalid filename' do - before { get :render_file, params: {filename: SecureRandom.hex, id: submission.id} } + let(:filename) { SecureRandom.hex } + + before { get :render_file, params: {filename: filename, id: submission.id, token: token} } expect_http_status(:not_found) end context 'with a valid filename' do let(:submission) { create(:submission, exercise: create(:audio_video)) } + let(:filename) { file.name_with_extension } - before { get :render_file, params: {filename: file.name_with_extension, id: submission.id} } + before { get :render_file, params: {filename: filename, id: submission.id, token: token} } context 'with a binary file' do let(:file) { submission.collect_files.detect {|file| file.file_type.file_extension == '.mp4' } } + let(:signed_url_video) { AuthenticatedUrlHelper.sign(render_protected_upload_url(id: file, filename: file.name_with_extension), file) } expect_assigns(file: :file) expect_assigns(submission: :submission) - expect_content_type('video/mp4') - expect_http_status(:ok) - it 'renders the file content' do - expect(response.body).to eq(file.read) + it 'sets the correct redirect' do + expect(response.location).to eq signed_url_video end end @@ -184,7 +188,7 @@ describe SubmissionsController do expect_assigns(submission: :submission) expect_http_status(:ok) - %i[render run test].each do |action| + %i[run test].each do |action| describe "##{action}_url" do let(:url) { JSON.parse(response.body).with_indifferent_access.fetch("#{action}_url") } @@ -199,6 +203,20 @@ describe SubmissionsController do end end + describe '#render_url' do + let(:supported_urls) { JSON.parse(response.body).with_indifferent_access.fetch('render_url') } + let(:file) { submission.collect_files.detect(&:main_file?) } + let(:url) { supported_urls.find {|hash| hash[:filepath] == file.filepath }['url'] } + + it 'starts like the render path' do + expect(url).to start_with(Rails.application.routes.url_helpers.render_submission_url(submission, file.filepath, host: request.host)) + end + + it 'includes a token' do + expect(url).to include '?token=' + end + end + describe '#score_url' do let(:url) { JSON.parse(response.body).with_indifferent_access.fetch('score_url') }