Merge branch 'security-mk-exclude-orphaned-upload-files-from-export-11-0' into 'security-11-0'

[11.0] Resolve "Orphaned upload files are accessible via project exports"

See merge request gitlab/gitlabhq!2466

2 files changed, 58 insertions, 18 deletions

diff --git a/lib/gitlab/import_export/uploads_saver.rb b/lib/gitlab/import_export/uploads_saver.rb
index 2f08dda55fd..58e11d27ee7 100644
--- a/lib/gitlab/import_export/uploads_saver.rb
+++ b/lib/gitlab/import_export/uploads_saver.rb
@@ -3,20 +3,38 @@ module Gitlab
     class UploadsSaver
       include Gitlab::ImportExport::CommandLineUtil
 
+      UPLOADS_BATCH_SIZE = 100
+
       def initialize(project:, shared:)
         @project = project
         @shared = shared
       end
 
       def save
-        return true unless File.directory?(uploads_path)
-
-        copy_files(uploads_path, uploads_export_path)
+        copy_project_uploads
+        true
       rescue => e
         @shared.error(e)
         false
       end
 
+      def copy_project_uploads
+        each_uploader do |uploader|
+          next unless uploader.file
+          # export of object stored uploads is not yet implemented
+          next unless uploader.upload.local?
+          next unless uploader.upload.exist?
+
+          copy_files(uploader.absolute_path, File.join(uploads_export_path, uploader.upload.path))
+        end
+      end
+
+      def each_uploader
+        @project.uploads.find_each(batch_size: UPLOADS_BATCH_SIZE) do |upload|
+          yield(upload.build_uploader)
+        end
+      end
+
       def uploads_path
         FileUploader.absolute_base_dir(@project)
       end
diff --git a/spec/lib/gitlab/import_export/uploads_saver_spec.rb b/spec/lib/gitlab/import_export/uploads_saver_spec.rb
index 1304d8fabfc..bb7feb364cf 100644
--- a/spec/lib/gitlab/import_export/uploads_saver_spec.rb
+++ b/spec/lib/gitlab/import_export/uploads_saver_spec.rb
@@ -3,11 +3,15 @@ require 'spec_helper'
 describe Gitlab::ImportExport::UploadsSaver do
   describe 'bundle a project Git repo' do
     let(:export_path) { "#{Dir.tmpdir}/uploads_saver_spec" }
-    let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
     let(:shared) { project.import_export_shared }
+    let(:upload) { create(:upload, :issuable_upload, model: project) }
+    subject(:saver) { described_class.new(shared: shared, project: project) }
 
     before do
       allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path)
+
+      FileUtils.mkdir_p(File.dirname(upload.absolute_path))
+      FileUtils.touch(upload.absolute_path)
     end
 
     after do
@@ -17,12 +21,6 @@ describe Gitlab::ImportExport::UploadsSaver do
     describe 'legacy storage' do
       let(:project) { create(:project, :legacy_storage) }
 
-      subject(:saver) { described_class.new(shared: shared, project: project) }
-
-      before do
-        UploadService.new(project, file, FileUploader).execute
-      end
-
       it 'saves the uploads successfully' do
         expect(saver.save).to be true
       end
@@ -32,19 +30,43 @@ describe Gitlab::ImportExport::UploadsSaver do
 
         uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
 
-        expect(uploads).to include('banana_sample.gif')
+        expect(uploads).to include(File.basename(upload.path))
       end
-    end
 
-    describe 'hashed storage' do
-      let(:project) { create(:project) }
+      context 'with orphaned project upload files' do
+        before do
+          upload.delete
+        end
+
+        after do
+          File.delete(upload.absolute_path) if File.exist?(upload.absolute_path)
+        end
+
+        it 'excludes orphaned upload files' do
+          saver.save
 
-      subject(:saver) { described_class.new(shared: shared, project: project) }
+          uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
 
-      before do
-        UploadService.new(project, file, FileUploader).execute
+          expect(uploads).not_to include(File.basename(upload.path))
+        end
       end
 
+      context 'with an upload missing its file' do
+        before do
+          File.delete(upload.absolute_path) if File.exist?(upload.absolute_path)
+        end
+
+        it 'does not cause errors' do
+          saver.save
+
+          expect(shared.errors).to be_empty
+        end
+      end
+    end
+
+    describe 'hashed storage' do
+      let(:project) { create(:project) }
+
       it 'saves the uploads successfully' do
         expect(saver.save).to be true
       end
@@ -54,7 +76,7 @@ describe Gitlab::ImportExport::UploadsSaver do
 
         uploads = Dir.glob(File.join(saver.uploads_export_path, '**/*')).map { |file| File.basename(file) }
 
-        expect(uploads).to include('banana_sample.gif')
+        expect(uploads).to include(File.basename(upload.path))
       end
     end
   end