Merge branch 'security-update-bulk-imports-project-pipeline-14-10' into '14-10-stable-ee'

Update ProjectAttributesTransformer to use fixed number of attributes

See merge request gitlab-org/security/gitlab!2549

lib/bulk_imports/projects/graphql/get_project_query.rb

@@ -10,20 +10,8 @@ module BulkImports
           <<-'GRAPHQL'
           query($full_path: ID!) {
             project(fullPath: $full_path) {
-              description
               visibility
-              archived
               created_at: createdAt
-              shared_runners_enabled: sharedRunnersEnabled
-              container_registry_enabled: containerRegistryEnabled
-              only_allow_merge_if_pipeline_succeeds: onlyAllowMergeIfPipelineSucceeds
-              only_allow_merge_if_all_discussions_are_resolved: onlyAllowMergeIfAllDiscussionsAreResolved
-              request_access_enabled: requestAccessEnabled
-              printing_merge_request_link_enabled: printingMergeRequestLinkEnabled
-              remove_source_branch_after_merge: removeSourceBranchAfterMerge
-              autoclose_referenced_issues: autocloseReferencedIssues
-              suggestion_commit_message: suggestionCommitMessage
-              wiki_enabled: wikiEnabled
             }
           }
           GRAPHQL

lib/bulk_imports/projects/transformers/project_attributes_transformer.rb

@@ -7,16 +7,18 @@ module BulkImports
         PROJECT_IMPORT_TYPE = 'gitlab_project_migration'
 
         def transform(context, data)
+          project = {}
           entity = context.entity
           visibility = data.delete('visibility')
 
-          data['name'] = entity.destination_name
-          data['path'] = entity.destination_name.parameterize
-          data['import_type'] = PROJECT_IMPORT_TYPE
-          data['visibility_level'] = Gitlab::VisibilityLevel.string_options[visibility] if visibility.present?
-          data['namespace_id'] = Namespace.find_by_full_path(entity.destination_namespace)&.id if entity.destination_namespace.present?
+          project[:name] = entity.destination_name
+          project[:path] = entity.destination_name.parameterize
+          project[:created_at] = data['created_at']
+          project[:import_type] = PROJECT_IMPORT_TYPE
+          project[:visibility_level] = Gitlab::VisibilityLevel.string_options[visibility] if visibility.present?
+          project[:namespace_id] = Namespace.find_by_full_path(entity.destination_namespace)&.id if entity.destination_namespace.present?
 
-          data.transform_keys!(&:to_sym)
+          project
         end
       end
     end

lib/gitlab/import_export/decompressed_archive_size_validator.rb

@@ -8,6 +8,8 @@ module Gitlab
       DEFAULT_MAX_BYTES = 10.gigabytes.freeze
       TIMEOUT_LIMIT = 210.seconds
 
+      ServiceError = Class.new(StandardError)
+
       def initialize(archive_path:, max_bytes: self.class.max_bytes)
         @archive_path = archive_path
         @max_bytes = max_bytes
@@ -29,6 +31,8 @@ module Gitlab
         pgrp = nil
         valid_archive = true
 
+        validate_archive_path
+
         Timeout.timeout(TIMEOUT_LIMIT) do
           stdin, stdout, stderr, wait_thr = Open3.popen3(command, pgroup: true)
           stdin.close
@@ -78,15 +82,29 @@ module Gitlab
         false
       end
 
+      def validate_archive_path
+        Gitlab::Utils.check_path_traversal!(@archive_path)
+
+        raise(ServiceError, 'Archive path is not a string') unless @archive_path.is_a?(String)
+        raise(ServiceError, 'Archive path is a symlink') if File.lstat(@archive_path).symlink?
+        raise(ServiceError, 'Archive path is not a file') unless File.file?(@archive_path)
+      end
+
       def command
         "gzip -dc #{@archive_path} | wc -c"
       end
 
       def log_error(error)
+        archive_size = begin
+          File.size(@archive_path)
+        rescue StandardError
+          nil
+        end
+
         Gitlab::Import::Logger.info(
           message: error,
           import_upload_archive_path: @archive_path,
-          import_upload_archive_size: File.size(@archive_path)
+          import_upload_archive_size: archive_size
         )
       end
     end

spec/lib/bulk_imports/projects/pipelines/project_pipeline_spec.rb

@@ -25,18 +25,7 @@ RSpec.describe BulkImports::Projects::Pipelines::ProjectPipeline do
     let(:project_data) do
       {
         'visibility' => 'private',
-        'created_at' => 10.days.ago,
-        'archived' => false,
-        'shared_runners_enabled' => true,
-        'container_registry_enabled' => true,
-        'only_allow_merge_if_pipeline_succeeds' => true,
-        'only_allow_merge_if_all_discussions_are_resolved' => true,
-        'request_access_enabled' => true,
-        'printing_merge_request_link_enabled' => true,
-        'remove_source_branch_after_merge' => true,
-        'autoclose_referenced_issues' => true,
-        'suggestion_commit_message' => 'message',
-        'wiki_enabled' => true
+        'created_at' => '2016-08-12T09:41:03'
       }
     end
 
@@ -58,17 +47,8 @@ RSpec.describe BulkImports::Projects::Pipelines::ProjectPipeline do
 
       expect(imported_project).not_to be_nil
       expect(imported_project.group).to eq(group)
-      expect(imported_project.suggestion_commit_message).to eq('message')
-      expect(imported_project.archived?).to eq(project_data['archived'])
-      expect(imported_project.shared_runners_enabled?).to eq(project_data['shared_runners_enabled'])
-      expect(imported_project.container_registry_enabled?).to eq(project_data['container_registry_enabled'])
-      expect(imported_project.only_allow_merge_if_pipeline_succeeds?).to eq(project_data['only_allow_merge_if_pipeline_succeeds'])
-      expect(imported_project.only_allow_merge_if_all_discussions_are_resolved?).to eq(project_data['only_allow_merge_if_all_discussions_are_resolved'])
-      expect(imported_project.request_access_enabled?).to eq(project_data['request_access_enabled'])
-      expect(imported_project.printing_merge_request_link_enabled?).to eq(project_data['printing_merge_request_link_enabled'])
-      expect(imported_project.remove_source_branch_after_merge?).to eq(project_data['remove_source_branch_after_merge'])
-      expect(imported_project.autoclose_referenced_issues?).to eq(project_data['autoclose_referenced_issues'])
-      expect(imported_project.wiki_enabled?).to eq(project_data['wiki_enabled'])
+      expect(imported_project.visibility).to eq(project_data['visibility'])
+      expect(imported_project.created_at).to eq(project_data['created_at'])
     end
   end
 

spec/lib/bulk_imports/projects/transformers/project_attributes_transformer_spec.rb

@@ -25,8 +25,8 @@ RSpec.describe BulkImports::Projects::Transformers::ProjectAttributesTransformer
 
     let(:data) do
       {
-        'name' => 'source_name',
-        'visibility' => 'private'
+        'visibility' => 'private',
+        'created_at' => '2016-11-18T09:29:42.634Z'
       }
     end
 
@@ -76,8 +76,21 @@ RSpec.describe BulkImports::Projects::Transformers::ProjectAttributesTransformer
       end
     end
 
-    it 'converts all keys to symbols' do
-      expect(transformed_data.keys).to contain_exactly(:name, :path, :import_type, :visibility_level, :namespace_id)
+    context 'when data has extra keys' do
+      it 'returns a fixed number of keys' do
+        data = {
+          'visibility' => 'private',
+          'created_at' => '2016-11-18T09:29:42.634Z',
+          'my_key' => 'my_key',
+          'another_key' => 'another_key',
+          'last_key' => 'last_key'
+        }
+
+        transformed_data = described_class.new.transform(context, data)
+
+        expect(transformed_data.keys)
+          .to contain_exactly(:created_at, :import_type, :name, :namespace_id, :path, :visibility_level)
+      end
     end
   end
 end

spec/lib/gitlab/import_export/decompressed_archive_size_validator_spec.rb

@@ -86,6 +86,65 @@ RSpec.describe Gitlab::ImportExport::DecompressedArchiveSizeValidator do
         include_examples 'logs raised exception and terminates validator process group'
       end
     end
+
+    context 'archive path validation' do
+      let(:filesize) { nil }
+
+      before do
+        expect(Gitlab::Import::Logger)
+          .to receive(:info)
+          .with(
+            import_upload_archive_path: filepath,
+            import_upload_archive_size: filesize,
+            message: error_message
+          )
+      end
+
+      context 'when archive path is traversed' do
+        let(:filepath) { '/foo/../bar' }
+        let(:error_message) { 'Invalid path' }
+
+        it 'returns false' do
+          expect(subject.valid?).to eq(false)
+        end
+      end
+
+      context 'when archive path is not a string' do
+        let(:filepath) { 123 }
+        let(:error_message) { 'Archive path is not a string' }
+
+        it 'returns false' do
+          expect(subject.valid?).to eq(false)
+        end
+      end
+
+      context 'which archive path is a symlink' do
+        let(:filepath) { File.join(Dir.tmpdir, 'symlink') }
+        let(:error_message) { 'Archive path is a symlink' }
+
+        before do
+          FileUtils.ln_s(filepath, filepath, force: true)
+        end
+
+        it 'returns false' do
+          expect(subject.valid?).to eq(false)
+        end
+      end
+
+      context 'when archive path is not a file' do
+        let(:filepath) { Dir.mktmpdir }
+        let(:filesize) { File.size(filepath) }
+        let(:error_message) { 'Archive path is not a file' }
+
+        after do
+          FileUtils.rm_rf(filepath)
+        end
+
+        it 'returns false' do
+          expect(subject.valid?).to eq(false)
+        end
+      end
+    end
   end
 
   def create_compressed_file

spec/services/bulk_imports/file_decompression_service_spec.rb

@@ -80,7 +80,8 @@ RSpec.describe BulkImports::FileDecompressionService do
       subject { described_class.new(tmpdir: tmpdir, filename: 'symlink.gz') }
 
       it 'raises an error and removes the file' do
-        expect { subject.execute }.to raise_error(described_class::ServiceError, 'Invalid file')
+        expect { subject.execute }
+          .to raise_error(BulkImports::FileDecompressionService::ServiceError, 'File decompression error')
 
         expect(File.exist?(symlink)).to eq(false)
       end