diff --git a/app/workers/object_storage/background_move_worker.rb b/app/workers/object_storage/background_move_worker.rb index bb51f0d7e1f4cdee082d83e807aa8a6b6257e40b..8b4c97e9a06389f574afca1800c7621f93d4bb01 100644 --- a/app/workers/object_storage/background_move_worker.rb +++ b/app/workers/object_storage/background_move_worker.rb @@ -14,13 +14,18 @@ class BackgroundMoveWorker # rubocop:disable Scalability/IdempotentWorker def perform(uploader_class_name, subject_class_name, file_field, subject_id) uploader_class = uploader_class_name.constantize subject_class = subject_class_name.constantize + mount_point = file_field&.to_sym return unless uploader_class < ObjectStorage::Concern return unless uploader_class.object_store_enabled? return unless uploader_class.background_upload_enabled? + unless valid_mount_point?(subject_class, uploader_class, mount_point) + raise(ArgumentError, "#{mount_point} not allowed for #{subject_class} in #{self.class.name}") + end + subject = subject_class.find(subject_id) - uploader = build_uploader(subject, file_field&.to_sym) + uploader = build_uploader(subject, mount_point) uploader.migrate!(ObjectStorage::Store::REMOTE) end @@ -28,8 +33,16 @@ def build_uploader(subject, mount_point) case subject when Upload then subject.retrieve_uploader(mount_point) else - subject.send(mount_point) # rubocop:disable GitlabSecurity/PublicSend + # This is safe because: + # 1. We don't pass any arguments to the method. + # 2. valid_mount_point? checks that this is in fact an uploader of the correct class. + # + subject.public_send(mount_point) # rubocop:disable GitlabSecurity/PublicSend end end + + def valid_mount_point?(subject_class, uploader_class, mount_point) + subject_class == Upload || subject_class.uploaders[mount_point] == uploader_class + end end end diff --git a/spec/uploaders/workers/object_storage/background_move_worker_spec.rb b/spec/uploaders/workers/object_storage/background_move_worker_spec.rb index a481939ed7a7c253472fea808c825b3d36e358f2..e549d42e28f3b4820f64e033483042fd48cedd7b 100644 --- a/spec/uploaders/workers/object_storage/background_move_worker_spec.rb +++ b/spec/uploaders/workers/object_storage/background_move_worker_spec.rb @@ -113,4 +113,40 @@ def perform end end end + + context 'with invalid input' do + before do + stub_lfs_object_storage(background_upload: true) + stub_artifacts_object_storage(background_upload: true) + stub_uploads_object_storage(AvatarUploader, background_upload: true) + end + + context 'with a file_field argument that is not an upload mount' do + it 'does nothing' do + expect(subject).not_to receive(:build_uploader) + expect(LfsObject).not_to receive(:find) + expect(Ci::JobArtifact).not_to receive(:find) + expect(AvatarUploader).not_to receive(:find) + + expect { subject.perform('LfsObjectUploader', 'LfsObject', 'avatar', 1) } + .to raise_error(ArgumentError, 'avatar not allowed for LfsObject in ObjectStorage::BackgroundMoveWorker') + + expect { subject.perform('JobArtifactUploader', 'Ci::JobArtifact', 'id', 2) } + .to raise_error(ArgumentError, 'id not allowed for Ci::JobArtifact in ObjectStorage::BackgroundMoveWorker') + + expect { subject.perform('AvatarUploader', 'User', 'file', 3) } + .to raise_error(ArgumentError, 'file not allowed for User in ObjectStorage::BackgroundMoveWorker') + end + end + + context 'with an uploader that does not match the given subject' do + it 'raises ArgumentError' do + expect(subject).not_to receive(:build_uploader) + expect(LfsObject).not_to receive(:find) + + expect { subject.perform('AvatarUploader', 'LfsObject', 'file', 1) } + .to raise_error(ArgumentError, 'file not allowed for LfsObject in ObjectStorage::BackgroundMoveWorker') + end + end + end end