summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/services/projects/import_error_filter.rb14
-rw-r--r--app/services/projects/import_service.rb12
-rw-r--r--changelogs/unreleased/security-import-path-logging.yml5
-rw-r--r--lib/gitlab/import_export/shared.rb39
-rw-r--r--spec/lib/gitlab/import_export/shared_spec.rb31
-rw-r--r--spec/lib/gitlab/import_export/version_checker_spec.rb2
-rw-r--r--spec/services/projects/import_error_filter_spec.rb17
-rw-r--r--spec/services/projects/import_service_spec.rb4
8 files changed, 107 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..5861b803996 100644
--- a/app/services/projects/import_service.rb
+++ b/app/services/projects/import_service.rb
@@ -24,8 +24,16 @@ module Projects
import_data
success
- rescue => e
+ rescue Gitlab::UrlBlocker::BlockedUrlError => e
+ Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type })
+
error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{e.message}")
+ rescue => e
+ message = Projects::ImportErrorFilter.filter_message(e.message)
+
+ Gitlab::Sentry.track_acceptable_exception(e, extra: { project_path: project.full_path, importer: project.import_type })
+
+ error("Error importing repository #{project.safe_import_url} into #{project.full_path} - #{message}")
end
private
@@ -35,7 +43,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 c13e6c1d83b..947caaaefee 100644
--- a/lib/gitlab/import_export/shared.rb
+++ b/lib/gitlab/import_export/shared.rb
@@ -8,6 +8,7 @@ module Gitlab
def initialize(project)
@project = project
@errors = []
+ @logger = Gitlab::Import::Logger.build
end
def active_export_count
@@ -23,19 +24,16 @@ 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"))
+
+ Gitlab::Sentry.track_acceptable_exception(error, extra: log_base_data)
- # 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?
@@ -52,8 +50,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..5848dbfcaac 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.import_url} into #{project.full_path} - Failed to import the repository [FILTERED]"
end
context 'when repository import scheduled' do
View Lorry Controller Status