diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb index 7ad79a1e56c..3dbfabcae8a 100644 --- a/app/controllers/concerns/uploads_actions.rb +++ b/app/controllers/concerns/uploads_actions.rb @@ -24,7 +24,7 @@ module UploadsActions # - or redirect to its URL # def show - return render_404 unless uploader.exists? + return render_404 unless uploader&.exists? if uploader.file_storage? disposition = uploader.image_or_video? ? 'inline' : 'attachment' @@ -71,6 +71,9 @@ module UploadsActions def build_uploader_from_params uploader = uploader_class.new(model, secret: params[:secret]) + + return nil unless uploader.model_valid? + uploader.retrieve_from_store!(params[:filename]) uploader end diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb index a9e5c028b03..010100f2da1 100644 --- a/app/uploaders/gitlab_uploader.rb +++ b/app/uploaders/gitlab_uploader.rb @@ -67,6 +67,10 @@ class GitlabUploader < CarrierWave::Uploader::Base super || file&.filename end + def model_valid? + !!model + end + private # Designed to be overridden by child uploaders that have a dynamic path diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index e7d9ecd3222..f2ad0badd53 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -14,6 +14,12 @@ class PersonalFileUploader < FileUploader File.join(model.class.to_s.underscore, model.id.to_s) end + # model_path_segment does not require a model to be passed, so we can always + # generate a path, even when there's no model. + def model_valid? + true + end + # Revert-Override def store_dir File.join(base_dir, dynamic_segment) diff --git a/changelogs/unreleased/fix-500-for-invalid-upload-path.yml b/changelogs/unreleased/fix-500-for-invalid-upload-path.yml new file mode 100644 index 00000000000..a4ce00c64c4 --- /dev/null +++ b/changelogs/unreleased/fix-500-for-invalid-upload-path.yml @@ -0,0 +1,5 @@ +--- +title: Fix 500 error when loading an invalid upload URL +merge_request: +author: +type: fixed diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index d572085661d..eca9baed9c9 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/spec/controllers/projects/uploads_controller_spec.rb @@ -7,4 +7,12 @@ describe Projects::UploadsController do end it_behaves_like 'handle uploads' + + context 'when the URL the old style, without /-/system' do + it 'responds with a redirect to the login page' do + get :show, namespace_id: 'project', project_id: 'avatar', filename: 'foo.png', secret: 'bar' + + expect(response).to redirect_to(new_user_session_path) + end + end end diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index 7ce80c82439..ea7dbade171 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -89,6 +89,19 @@ shared_examples 'handle uploads' do end end + context "when neither the uploader nor the model exists" do + before do + allow_any_instance_of(Upload).to receive(:build_uploader).and_return(nil) + allow(controller).to receive(:find_model).and_return(nil) + end + + it "responds with status 404" do + show_upload + + expect(response).to have_gitlab_http_status(404) + end + end + context "when the file doesn't exist" do before do allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)