From 17dd8b126746da11e75c93d08c07aeda33d9db5a Mon Sep 17 00:00:00 2001 From: Sebastian Serth Date: Tue, 19 Dec 2023 09:36:15 +0100 Subject: [PATCH] Change syntax for routes with filename Previously, the filename was URL-encoded, thus each / was replaced with %2F. This caused issues with some Apache2 configuration, smartly mingling with the URL to either encode it a second time (resulting in %252F) or decoding it (generating a real /). However, for authenticated file downloads with the JWT, we hardly require a byte-by-byte matching. With these changes, the URL parameter is no longer URL-encoded, so that Apache2 won't break our implementation any longer. Further, we use this opportunity to get rid of the unnecessary .json extension for those filename routes, simplifying the routes generated and doing some further cleanup. --- app/assets/javascripts/editor/evaluation.js | 5 +- app/assets/javascripts/editor/execution.js | 67 ++++++++++--------- app/assets/javascripts/editor/submissions.js | 28 +++----- app/assets/javascripts/exercises.js.erb | 6 +- app/controllers/live_streams_controller.rb | 2 +- app/models/submission.rb | 1 - app/views/submissions/show.json.jbuilder | 7 -- config/routes.rb | 20 +++--- .../submissions_controller_spec.rb | 24 +------ 9 files changed, 64 insertions(+), 96 deletions(-) diff --git a/app/assets/javascripts/editor/evaluation.js b/app/assets/javascripts/editor/evaluation.js index 0be38dc0..2a992c08 100644 --- a/app/assets/javascripts/editor/evaluation.js +++ b/app/assets/javascripts/editor/evaluation.js @@ -14,11 +14,10 @@ CodeOceanEditorEvaluation = { this.stopCode(event); this.clearScoringOutput(); $('#submit').addClass("d-none"); - this.createSubmission(cause, null, function (response) { + this.createSubmission(cause, null, function (submission) { this.showSpinner($('#assess')); $('#score_div').removeClass('d-none'); - var url = response.score_url; - this.initializeSocketForScoring(url); + this.initializeSocketForScoring(submission.id); }.bind(this)); }, diff --git a/app/assets/javascripts/editor/execution.js b/app/assets/javascripts/editor/execution.js index c4248ae4..0d4287c8 100644 --- a/app/assets/javascripts/editor/execution.js +++ b/app/assets/javascripts/editor/execution.js @@ -1,56 +1,59 @@ CodeOceanEditorWebsocket = { websocket: null, + // Replace `http` with `ws` for the WebSocket connection. This also works with `https` and `wss`. + webSocketProtocol: window.location.protocol.replace(/^http/, 'ws').split(':')[0], - createSocketUrl: function(url, span) { - const sockURL = new URL(url, window.location); - // not needed any longer, we put it directly into the url: sockURL.pathname = url; + initializeSocket: function(urlHelper, params, closeCallback) { + // 1. Specify the protocol for all URLs to generate + params.protocol = this.webSocketProtocol; + params._options = true; - // replace `http` with `ws` for the WebSocket connection. This also works with `https` and `wss`. - sockURL.protocol = sockURL.protocol.replace("http", "ws"); + // 2. Create a new Sentry transaction. + // Since we want to group similar URLs, we use the URL without the ID and filename as the description. + const cleanedUrl = urlHelper({ + ...params, + ...(params.id && {id: '*'}), // Overwrite the ID with a wildcard only if it is present. + ...(params.filename && {filename: '*'}), // Overwrite the filename with a wildcard only if it is present. + }); + const sentryDescription = `WebSocket ${cleanedUrl}`; + const span = this.sentryTransaction?.startChild({op: 'websocket.client', description: sentryDescription, data: {...params}}) - // strip anchor if it is in the url - sockURL.hash = ''; - - if (span) { - const dynamicContext = this.sentryTransaction.getDynamicSamplingContext(); - const baggage = SentryUtils.dynamicSamplingContextToSentryBaggageHeader(dynamicContext); - if (baggage) { - sockURL.searchParams.set('HTTP_SENTRY_TRACE', span.toTraceparent()); - sockURL.searchParams.set('HTTP_BAGGAGE', baggage); - } + // 3. Create the actual WebSocket URL. + // This URL might contain Sentry Tracing headers to propagate the Sentry transaction. + if (span) { + const dynamicContext = this.sentryTransaction.getDynamicSamplingContext(); + const baggage = SentryUtils.dynamicSamplingContextToSentryBaggageHeader(dynamicContext); + if (baggage) { + params.HTTP_SENTRY_TRACE = span.toTraceparent(); + params.HTTP_BAGGAGE = baggage; } + } + const url = urlHelper({...params}); - return sockURL.toString(); - }, - - initializeSocket: function(url, closeCallback) { - const cleanedPath = url.replace(/\/\d+\//, '/*/').replace(/\/[^\/]+$/, '/*'); - const websocketHost = window.location.origin.replace(/^http/, 'ws'); - const sentryDescription = `WebSocket ${websocketHost}${cleanedPath}`; - const span = this.sentryTransaction?.startChild({op: 'websocket.client', description: sentryDescription}) - this.websocket = new CommandSocket(this.createSocketUrl(url, span), + // 4. Connect to the given URL. + this.websocket = new CommandSocket(url, function (evt) { this.resetOutputTab(); }.bind(this) ); CodeOceanEditorWebsocket.websocket = this.websocket; this.websocket.onError(this.showWebsocketError.bind(this)); - this.websocket.onClose( function(span, callback){ - span?.finish() + this.websocket.onClose(function(span, callback){ + span?.finish(); if(callback != null){ callback(); } }.bind(this, span, closeCallback)); }, - initializeSocketForTesting: function(url) { - this.initializeSocket(url); + initializeSocketForTesting: function(submissionID, filename) { + this.initializeSocket(Routes.test_submission_url, {id: submissionID, filename: filename}); this.websocket.on('default',this.handleTestResponse.bind(this)); this.websocket.on('exit', this.handleExitCommand.bind(this)); }, - initializeSocketForScoring: function(url) { - this.initializeSocket(url, function() { + initializeSocketForScoring: function(submissionID) { + this.initializeSocket(Routes.score_submission_url, {id: submissionID}, function() { $('#assess').one('click', this.scoreCode.bind(this)) }.bind(this)); this.websocket.on('default',this.handleScoringResponse.bind(this)); @@ -59,8 +62,8 @@ CodeOceanEditorWebsocket = { this.websocket.on('status', this.showStatus.bind(this)); }, - initializeSocketForRunning: function(url) { - this.initializeSocket(url); + initializeSocketForRunning: function(submissionID, filename) { + this.initializeSocket(Routes.run_submission_url, {id: submissionID, filename: filename}); this.websocket.on('input',this.showPrompt.bind(this)); this.websocket.on('write', this.printWebsocketOutput.bind(this)); this.websocket.on('clear', this.clearOutput.bind(this)); diff --git a/app/assets/javascripts/editor/submissions.js b/app/assets/javascripts/editor/submissions.js index e267553c..0e5d92b4 100644 --- a/app/assets/javascripts/editor/submissions.js +++ b/app/assets/javascripts/editor/submissions.js @@ -1,6 +1,4 @@ CodeOceanEditorSubmissions = { - FILENAME_URL_PLACEHOLDER: '{filename}', - AUTOSAVE_INTERVAL: 15 * 1000, autosaveTimer: null, autosaveLabel: "#statusbar #autosave", @@ -31,7 +29,7 @@ CodeOceanEditorSubmissions = { }, dataType: 'json', method: $(initiator).data('http-method') || 'POST', - url: url + '.json' + url: url, }); jqxhr.always(this.hideSpinner.bind(this)); jqxhr.done(this.createSubmissionCallback.bind(this)); @@ -99,12 +97,10 @@ CodeOceanEditorSubmissions = { downloadCode: function(event) { event.preventDefault(); - this.createSubmission('#download', null,function(response) { - var url = response.download_url; - + this.createSubmission('#download', null,function(submission) { // to download just a single file, use the following url - //var url = response.download_file_url.replace(FILENAME_URL_PLACEHOLDER, active_file.filename); - window.location = url; + // window.location = Routes.download_file_submission_url(submission.id, CodeOceanEditor.active_file.filename); + window.location = Routes.download_submission_url(submission.id); }); }, @@ -140,11 +136,11 @@ CodeOceanEditorSubmissions = { this.startSentryTransaction(cause); event.preventDefault(); if (cause.is(':visible')) { - this.createSubmission(cause, null, function (response) { - if (response.render_url === undefined) return; + this.createSubmission(cause, null, function (submission) { + if (submission.render_url === undefined) return; const active_file = CodeOceanEditor.active_file.filename; - const desired_file = response.render_url.filter(hash => hash.filepath === active_file); + const desired_file = submission.render_url.filter(hash => hash.filepath === active_file); const url = desired_file[0].url; // Allow to open the new tab even in Safari. // See: https://stackoverflow.com/a/70463940 @@ -156,7 +152,7 @@ CodeOceanEditorSubmissions = { this.printOutput({ stderr: message }, true, 0); - this.sendError(message, response.id); + this.sendError(message, submission.id); this.showOutputBar(); }; } @@ -184,8 +180,7 @@ CodeOceanEditorSubmissions = { this.showSpinner($('#run')); $('#score_div').addClass('d-none'); this.toggleButtonStates(); - const url = submission.run_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 - this.initializeSocketForRunning(url); + this.initializeSocketForRunning(submission.id, CodeOceanEditor.active_file.filename); }, testCode: function(event) { @@ -193,11 +188,10 @@ CodeOceanEditorSubmissions = { this.startSentryTransaction(cause); event.preventDefault(); if (cause.is(':visible')) { - this.createSubmission(cause, null, function(response) { + this.createSubmission(cause, null, function(submission) { this.showSpinner($('#test')); $('#score_div').addClass('d-none'); - var url = response.test_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 - this.initializeSocketForTesting(url); + this.initializeSocketForTesting(submission.id, CodeOceanEditor.active_file.filename); }.bind(this)); } }, diff --git a/app/assets/javascripts/exercises.js.erb b/app/assets/javascripts/exercises.js.erb index dee4a82e..ab0f14a0 100644 --- a/app/assets/javascripts/exercises.js.erb +++ b/app/assets/javascripts/exercises.js.erb @@ -104,9 +104,9 @@ $(document).on('turbolinks:load', function () { var jqxhr = $.ajax({ // normal file path (without json) would destroy the context object (the exercise) as well, due to redirection // to the context after the :destroy action. - contentType: 'Application/json', - url: fileUrl + '.json', - method: 'DELETE' + dataType: 'json', + method: 'DELETE', + url: fileUrl, }); jqxhr.done(function () { removeFileForm(fileUrl) diff --git a/app/controllers/live_streams_controller.rb b/app/controllers/live_streams_controller.rb index c51a6075..6a523f17 100644 --- a/app/controllers/live_streams_controller.rb +++ b/app/controllers/live_streams_controller.rb @@ -22,7 +22,7 @@ class LiveStreamsController < ApplicationController def download_arbitrary_file @execution_environment = authorize ExecutionEnvironment.find(params[:id]) - desired_file = params[:filename].to_s + desired_file = "/#{params[:filename]}" # The filename given is absolute; this is an admin-only action. runner = Runner.for(current_user, @execution_environment) fallback_location = shell_execution_environment_path(@execution_environment) privileged = params[:sudo] || @execution_environment.privileged_execution? diff --git a/app/models/submission.rb b/app/models/submission.rb index 75565104..00d06612 100644 --- a/app/models/submission.rb +++ b/app/models/submission.rb @@ -7,7 +7,6 @@ class Submission < ApplicationRecord CAUSES = %w[assess download file render run save submit test autosave requestComments remoteAssess remoteSubmit].freeze - FILENAME_URL_PLACEHOLDER = '{filename}' MAX_COMMENTS_ON_RECOMMENDED_RFC = 5 OLDEST_RFC_TO_SHOW = 1.month diff --git a/app/views/submissions/show.json.jbuilder b/app/views/submissions/show.json.jbuilder index d281d0f2..4ad8d9fb 100644 --- a/app/views/submissions/show.json.jbuilder +++ b/app/views/submissions/show.json.jbuilder @@ -4,10 +4,6 @@ json.id @submission.id json.files @submission.files do |file| json.extract! file, :id, :file_id end -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') unless @embed_options[:disable_download] json.render_url @submission.collect_files.select(&:visible) do |files| host = ApplicationController::RENDER_HOST || request.host @@ -17,6 +13,3 @@ unless @embed_options[:disable_download] 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') -json.finalize_url finalize_submission_path(@submission) diff --git a/config/routes.rb b/config/routes.rb index f2d4477a..c880ac96 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -FILENAME_REGEXP = /.+/ unless Kernel.const_defined?(:FILENAME_REGEXP) - Rails.application.routes.draw do resources :community_solutions, only: %i[index edit update] resources :error_template_attributes @@ -67,7 +65,7 @@ Rails.application.routes.draw do get :shell post 'shell', as: :execute_command, action: :execute_command get :list_files, as: :list_files_in - get 'download/:filename', as: :download_file_from, constraints: {filename: FILENAME_REGEXP}, action: :download_arbitrary_file, controller: 'live_streams' + get 'download/*filename', as: :download_file_from, action: :download_arbitrary_file, controller: 'live_streams', format: false # Admin file-system access to runners get :statistics post :sync_to_runner_management end @@ -131,8 +129,8 @@ Rails.application.routes.draw do namespace :code_ocean 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} + get '/uploads/files/:id/*filename', to: 'code_ocean/files#show_protected_upload', as: :protected_upload, format: false # View file, e.g., when implementing or viewing an exercise + get '/uploads/render_files/:id/*filename', to: 'code_ocean/files#render_protected_upload', as: :render_protected_upload, format: false # Render action with embedded files, i.e., images in user-created HTML resources :file_types @@ -154,14 +152,14 @@ Rails.application.routes.draw do resources :submissions, only: %i[create index show] do member do - get 'download', as: :download, action: :download - get 'download/:filename', as: :download_file, constraints: {filename: FILENAME_REGEXP}, action: :download_file - get 'download_stream/:filename', as: :download_stream_file, constraints: {filename: FILENAME_REGEXP}, action: :download_submission_file, controller: 'live_streams' - get 'render/:filename', as: :render, constraints: {filename: FILENAME_REGEXP}, action: :render_file - get 'run/:filename', as: :run, constraints: {filename: FILENAME_REGEXP}, action: :run + get 'download', as: :download, action: :download # Full submission download with RemoteEvaluationMapping + get 'download/*filename', as: :download_file, action: :download_file, format: false # Single file download, currently not used in the frontend (but working) + get 'download_stream/*filename', as: :download_stream_file, action: :download_submission_file, controller: 'live_streams', format: false # Access runner artifacts + get 'render/*filename', as: :render, action: :render_file, format: false + get 'run/*filename', as: :run, action: :run, format: false get :score get :statistics - get 'test/:filename', as: :test, constraints: {filename: FILENAME_REGEXP}, action: :test + get 'test/*filename', as: :test, action: :test, format: false get :finalize end end diff --git a/spec/controllers/submissions_controller_spec.rb b/spec/controllers/submissions_controller_spec.rb index 52ce2d6f..37f07c39 100644 --- a/spec/controllers/submissions_controller_spec.rb +++ b/spec/controllers/submissions_controller_spec.rb @@ -279,19 +279,9 @@ RSpec.describe SubmissionsController do expect_assigns(submission: :submission) expect_http_status(:ok) - %i[run test].each do |action| - describe "##{action}_url" do - let(:url) { response.parsed_body.with_indifferent_access.fetch("#{action}_url") } - - it "starts like the #{action} path" do - filename = File.basename(__FILE__) - expect(url).to start_with(Rails.application.routes.url_helpers.send(:"#{action}_submission_path", submission, filename).sub(filename, '')) - end - - it 'ends with a placeholder' do - expect(url).to end_with("#{Submission::FILENAME_URL_PLACEHOLDER}.json") - end - end + it 'includes the desired fields' do + expect(response.parsed_body.keys).to include('id', 'files') + expect(response.parsed_body['files'].first.keys).to include('id', 'file_id') end describe '#render_url' do @@ -307,14 +297,6 @@ RSpec.describe SubmissionsController do expect(url).to include '?token=' end end - - describe '#score_url' do - let(:url) { response.parsed_body.with_indifferent_access.fetch('score_url') } - - it 'corresponds to the score path' do - expect(url).to eq(Rails.application.routes.url_helpers.score_submission_path(submission, format: :json)) - end - end end describe 'GET #test' do