Merge branch 'security-import-path-logging-11-5' into 'security-11-5'

[11.5] Fix error disclosure on Project Import

See merge request gitlab/gitlabhq!2732

(cherry picked from commit 427577d2adfd1833f6f0722a16b5410cc8d6d96b)

2e6e5af0 Fix path disclosure on Project Import
101acd98 Remove Sentry method call

8 files changed, 101 insertions, 17 deletions

diff --git a/app/services/projects/import_error_filter.rb b/app/services/projects/import_error_filter.rb
new file mode 100644
index 00000000000..a0fc5149bb4
--- /dev/null
+++ b/app/services/projects/import_error_filter.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+module Projects
+  # Used by project imports, it removes any potential paths
+  # included in an error message that could be stored in the DB
+  class ImportErrorFilter
+    ERROR_MESSAGE_FILTER = /[^\s]*#{File::SEPARATOR}[^\s]*(?=(\s|\z))/
+    FILTER_MESSAGE = '[FILTERED]'
+
+    def self.filter_message(message)
+      message.gsub(ERROR_MESSAGE_FILTER, FILTER_MESSAGE)
+    end
+  end
+end
diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb
index 3353aec6aec..c737ced0885 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -24,8 +24,12 @@ module Projects
       import_data
 
       success
-    rescue => e
+    rescue Gitlab::UrlBlocker::BlockedUrlError => e
       error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{e.message}")
+    rescue => e
+      message = Projects::ImportErrorFilter.filter_message(e.message)
+
+      error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{message}")
     end
 
     private
@@ -35,7 +39,7 @@ module Projects
         begin
           Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS)
         rescue Gitlab::UrlBlocker::BlockedUrlError => e
-          raise Error, "Blocked import URL: #{e.message}"
+          raise e, "Blocked import URL: #{e.message}"
         end
       end
 
diff --git a/changelogs/unreleased/security-import-path-logging.yml b/changelogs/unreleased/security-import-path-logging.yml
new file mode 100644
index 00000000000..2ba2d88d82a
--- /dev/null
+++ b/changelogs/unreleased/security-import-path-logging.yml
@@ -0,0 +1,5 @@
+---
+title: Fix path disclosure on project import error
+merge_request:
+author:
+type: security
diff --git a/lib/gitlab/import_export/shared.rb b/lib/gitlab/import_export/shared.rb
index 6d7c36ce38b..5371a6fe902 100644
--- a/lib/gitlab/import_export/shared.rb
+++ b/lib/gitlab/import_export/shared.rb
@@ -6,6 +6,7 @@ module Gitlab
       def initialize(project)
         @project = project
         @errors = []
+        @logger = Gitlab::Import::Logger.build
       end
 
       def active_export_count
@@ -21,19 +22,14 @@ module Gitlab
       end
 
       def error(error)
-        error_out(error.message, caller[0].dup)
-        add_error_message(error.message)
+        log_error(message: error.message, caller: caller[0].dup)
+        log_debug(backtrace: error.backtrace&.join("\n"))
 
-        # Debug:
-        if error.backtrace
-          Rails.logger.error("Import/Export backtrace: #{error.backtrace.join("\n")}")
-        else
-          Rails.logger.error("No backtrace found")
-        end
+        add_error_message(error.message)
       end
 
-      def add_error_message(error_message)
-        @errors << error_message
+      def add_error_message(message)
+        @errors << filtered_error_message(message)
       end
 
       def after_export_in_progress?
@@ -50,8 +46,25 @@ module Gitlab
         @project.disk_path
       end
 
-      def error_out(message, caller)
-        Rails.logger.error("Import/Export error raised on #{caller}: #{message}")
+      def log_error(details)
+        @logger.error(log_base_data.merge(details))
+      end
+
+      def log_debug(details)
+        @logger.debug(log_base_data.merge(details))
+      end
+
+      def log_base_data
+        {
+          importer: 'Import/Export',
+          import_jid: @project&.import_state&.import_jid,
+          project_id: @project&.id,
+          project_path: @project&.full_path
+        }
+      end
+
+      def filtered_error_message(message)
+        Projects::ImportErrorFilter.filter_message(message)
       end
 
       def after_export_lock_file
diff --git a/spec/lib/gitlab/import_export/shared_spec.rb b/spec/lib/gitlab/import_export/shared_spec.rb
new file mode 100644
index 00000000000..f2d750c6595
--- /dev/null
+++ b/spec/lib/gitlab/import_export/shared_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+require 'fileutils'
+
+describe Gitlab::ImportExport::Shared do
+  let(:project) { build(:project) }
+  subject { project.import_export_shared }
+
+  describe '#error' do
+    let(:error) { StandardError.new('Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file') }
+
+    it 'filters any full paths' do
+      subject.error(error)
+
+      expect(subject.errors).to eq(['Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]'])
+    end
+
+    it 'calls the error logger with the full message' do
+      expect(subject).to receive(:log_error).with(hash_including(message: error.message))
+
+      subject.error(error)
+    end
+
+    it 'calls the debug logger with a backtrace' do
+      error.set_backtrace('backtrace')
+
+      expect(subject).to receive(:log_debug).with(hash_including(backtrace: 'backtrace'))
+
+      subject.error(error)
+    end
+  end
+end
diff --git a/spec/lib/gitlab/import_export/version_checker_spec.rb b/spec/lib/gitlab/import_export/version_checker_spec.rb
index 49d857d9483..76f8253ec9b 100644
--- a/spec/lib/gitlab/import_export/version_checker_spec.rb
+++ b/spec/lib/gitlab/import_export/version_checker_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 include ImportExport::CommonUtil
 
 describe Gitlab::ImportExport::VersionChecker do
-  let(:shared) { Gitlab::ImportExport::Shared.new(nil) }
+  let!(:shared) { Gitlab::ImportExport::Shared.new(nil) }
 
   describe 'bundle a project Git repo' do
     let(:version) { Gitlab::ImportExport.version }
diff --git a/spec/services/projects/import_error_filter_spec.rb b/spec/services/projects/import_error_filter_spec.rb
new file mode 100644
index 00000000000..312b658de89
--- /dev/null
+++ b/spec/services/projects/import_error_filter_spec.rb
@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Projects::ImportErrorFilter do
+  it 'filters any full paths' do
+    message = 'Error importing into /my/folder Permission denied @ unlink_internal - /var/opt/gitlab/gitlab-rails/shared/a/b/c/uploads/file'
+
+    expect(described_class.filter_message(message)).to eq('Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED]')
+  end
+
+  it 'filters any relative paths ignoring single slash ones' do
+    message = 'Error importing into my/project Permission denied @ unlink_internal - ../file/ and folder/../file'
+
+    expect(described_class.filter_message(message)).to eq('Error importing into [FILTERED] Permission denied @ unlink_internal - [FILTERED] and [FILTERED]')
+  end
+end
diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb
index c525de6cd25..7faf0fc2868 100644
--- a/spec/services/projects/import_service_spec.rb
+++ b/spec/services/projects/import_service_spec.rb
@@ -136,12 +136,12 @@ describe Projects::ImportService do
         end
 
         it 'fails if repository import fails' do
-          expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository'))
+          expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository /a/b/c'))
 
           result = subject.execute
 
           expect(result[:status]).to eq :error
-          expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository"
+          expect(result[:message]).to eq "Error importing repository #{project.safe_import_url} into #{project.full_path} - Failed to import the repository [FILTERED]"
         end
 
         context 'when repository import scheduled' do