Merge branch 'fix-500-for-invalid-upload-path' into 'master'

Fix 500 error when loading an invalid upload URL

Closes gitlab-ee#4998

See merge request gitlab-org/gitlab-ce!17267

6 files changed, 40 insertions, 1 deletions

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)