Merge branch 'security-60551-fix-upload-scope' into 'master'

Queries for Upload should be scoped by model

See merge request gitlab/gitlabhq!3229

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

app/uploaders/records_uploads.rb

@@ -27,7 +27,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!)

changelogs/unreleased/security-60551-fix-upload-scope.yml

+---
+title: Queries for Upload should be scoped by model
+merge_request:
+author:
+type: security

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

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

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)

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