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
This commit is contained in:
Sean McGivern 2018-02-22 16:09:11 +00:00
commit 05f4788eab
6 changed files with 40 additions and 1 deletions

View file

@ -24,7 +24,7 @@ module UploadsActions
# - or redirect to its URL # - or redirect to its URL
# #
def show def show
return render_404 unless uploader.exists? return render_404 unless uploader&.exists?
if uploader.file_storage? if uploader.file_storage?
disposition = uploader.image_or_video? ? 'inline' : 'attachment' disposition = uploader.image_or_video? ? 'inline' : 'attachment'
@ -71,6 +71,9 @@ module UploadsActions
def build_uploader_from_params def build_uploader_from_params
uploader = uploader_class.new(model, secret: params[:secret]) uploader = uploader_class.new(model, secret: params[:secret])
return nil unless uploader.model_valid?
uploader.retrieve_from_store!(params[:filename]) uploader.retrieve_from_store!(params[:filename])
uploader uploader
end end

View file

@ -67,6 +67,10 @@ class GitlabUploader < CarrierWave::Uploader::Base
super || file&.filename super || file&.filename
end end
def model_valid?
!!model
end
private private
# Designed to be overridden by child uploaders that have a dynamic path # Designed to be overridden by child uploaders that have a dynamic path

View file

@ -14,6 +14,12 @@ class PersonalFileUploader < FileUploader
File.join(model.class.to_s.underscore, model.id.to_s) File.join(model.class.to_s.underscore, model.id.to_s)
end 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 # Revert-Override
def store_dir def store_dir
File.join(base_dir, dynamic_segment) File.join(base_dir, dynamic_segment)

View file

@ -0,0 +1,5 @@
---
title: Fix 500 error when loading an invalid upload URL
merge_request:
author:
type: fixed

View file

@ -7,4 +7,12 @@ describe Projects::UploadsController do
end end
it_behaves_like 'handle uploads' 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 end

View file

@ -89,6 +89,19 @@ shared_examples 'handle uploads' do
end end
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 context "when the file doesn't exist" do
before do before do
allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false) allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)