summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAdam Hegyi <ahegyi@gitlab.com>2019-07-11 06:52:09 +0200
committerAdam Hegyi <ahegyi@gitlab.com>2019-07-11 15:51:23 +0200
commit040143b5b3b3fcf843f0dcaaf1d56d024099e35f (patch)
tree84dc07f250adf031803be3966259cd2be27eb446
parent08a51a9db938bb05f9a4c999075d010079e16bad (diff)
downloadgitlab-ce-040143b5b3b3fcf843f0dcaaf1d56d024099e35f.tar.gz
Queries for Upload should be scoped by model
-rw-r--r--app/controllers/concerns/uploads_actions.rb2
-rw-r--r--app/uploaders/records_uploads.rb2
-rw-r--r--changelogs/unreleased/security-60551-fix-upload-scope-12-0.yml5
-rw-r--r--spec/controllers/groups/uploads_controller_spec.rb5
-rw-r--r--spec/controllers/projects/uploads_controller_spec.rb5
-rw-r--r--spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb10
-rw-r--r--spec/uploaders/records_uploads_spec.rb21
7 files changed, 48 insertions, 2 deletions
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index 59f6d3452a3..f5d35379e10 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -90,7 +90,7 @@ module UploadsActions
return unless uploader = build_uploader
upload_paths = uploader.upload_paths(params[:filename])
- upload = Upload.find_by(uploader: uploader_class.to_s, path: upload_paths)
+ upload = Upload.find_by(model: model, uploader: uploader_class.to_s, path: upload_paths)
upload&.build_uploader
end
# rubocop: enable CodeReuse/ActiveRecord
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
index 00b51f92b12..d7a859ebd5f 100644
--- a/app/uploaders/records_uploads.rb
+++ b/app/uploaders/records_uploads.rb
@@ -35,7 +35,7 @@ module RecordsUploads
end
def readd_upload
- uploads.where(path: upload_path).delete_all
+ uploads.where(model: model, path: upload_path).delete_all
upload.delete if upload
self.upload = build_upload.tap(&:save!)
diff --git a/changelogs/unreleased/security-60551-fix-upload-scope-12-0.yml b/changelogs/unreleased/security-60551-fix-upload-scope-12-0.yml
new file mode 100644
index 00000000000..7d7096833a7
--- /dev/null
+++ b/changelogs/unreleased/security-60551-fix-upload-scope-12-0.yml
@@ -0,0 +1,5 @@
+---
+title: Queries for Upload should be scoped by model
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb
index 0f99a957581..60342bf8e3d 100644
--- a/spec/controllers/groups/uploads_controller_spec.rb
+++ b/spec/controllers/groups/uploads_controller_spec.rb
@@ -10,6 +10,11 @@ describe Groups::UploadsController do
{ group_id: model }
end
+ let(:other_model) { create(:group, :public) }
+ let(:other_params) do
+ { group_id: other_model }
+ end
+
it_behaves_like 'handle uploads' do
let(:uploader_class) { NamespaceFileUploader }
end
diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb
index 776c1270977..661ed9840b1 100644
--- a/spec/controllers/projects/uploads_controller_spec.rb
+++ b/spec/controllers/projects/uploads_controller_spec.rb
@@ -10,6 +10,11 @@ describe Projects::UploadsController do
{ namespace_id: model.namespace.to_param, project_id: model }
end
+ let(:other_model) { create(:project, :public) }
+ let(:other_params) do
+ { namespace_id: other_model.namespace.to_param, project_id: other_model }
+ end
+
it_behaves_like 'handle uploads'
context 'when the URL the old style, without /-/system' do
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 59708173716..9036838e50a 100644
--- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
+++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
@@ -74,6 +74,16 @@ shared_examples 'handle uploads' do
UploadService.new(model, jpg, uploader_class).execute
end
+ context 'when accessing a specific upload via different model' do
+ it 'responds with status 404' do
+ params.merge!(other_params)
+
+ show_upload
+
+ expect(response).to have_gitlab_http_status(404)
+ end
+ end
+
context "when the model is public" do
before do
model.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb
index 42352f9b9f8..6134137d2b7 100644
--- a/spec/uploaders/records_uploads_spec.rb
+++ b/spec/uploaders/records_uploads_spec.rb
@@ -85,6 +85,27 @@ describe RecordsUploads do
expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect(Upload.count).to eq(1)
end
+
+ it 'does not affect other uploads with different model but the same path' do
+ project = create(:project)
+ other_project = create(:project)
+
+ uploader = RecordsUploadsExampleUploader.new(other_project)
+
+ upload_for_project = Upload.create!(
+ path: File.join('uploads', 'rails_sample.jpg'),
+ size: 512.kilobytes,
+ model: project,
+ uploader: uploader.class.to_s
+ )
+
+ uploader.store!(upload_fixture('rails_sample.jpg'))
+
+ upload_for_project_fresh = Upload.find(upload_for_project.id)
+
+ expect(upload_for_project).to eq(upload_for_project_fresh)
+ expect(Upload.count).to eq(2)
+ end
end
describe '#destroy_upload callback' do