Commit 57f082d9 authored by James Lopez's avatar James Lopez Committed by Yorick Peterse

Fix path disclosure on Project Import

parent 8a948a20
# 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
...@@ -24,8 +24,16 @@ module Projects ...@@ -24,8 +24,16 @@ module Projects
import_data import_data
success 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}") 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 end
private private
...@@ -35,7 +43,7 @@ module Projects ...@@ -35,7 +43,7 @@ module Projects
begin begin
Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS) Gitlab::UrlBlocker.validate!(project.import_url, ports: Project::VALID_IMPORT_PORTS)
rescue Gitlab::UrlBlocker::BlockedUrlError => e rescue Gitlab::UrlBlocker::BlockedUrlError => e
raise Error, "Blocked import URL: #{e.message}" raise e, "Blocked import URL: #{e.message}"
end end
end end
......
---
title: Fix path disclosure on project import error
merge_request:
author:
type: security
...@@ -8,6 +8,7 @@ module Gitlab ...@@ -8,6 +8,7 @@ module Gitlab
def initialize(project) def initialize(project)
@project = project @project = project
@errors = [] @errors = []
@logger = Gitlab::Import::Logger.build
end end
def active_export_count def active_export_count
...@@ -23,19 +24,16 @@ module Gitlab ...@@ -23,19 +24,16 @@ module Gitlab
end end
def error(error) def error(error)
error_out(error.message, caller[0].dup) log_error(message: error.message, caller: caller[0].dup)
add_error_message(error.message) log_debug(backtrace: error.backtrace&.join("\n"))
Gitlab::Sentry.track_acceptable_exception(error, extra: log_base_data)
# Debug: add_error_message(error.message)
if error.backtrace
Rails.logger.error("Import/Export backtrace: #{error.backtrace.join("\n")}")
else
Rails.logger.error("No backtrace found")
end
end end
def add_error_message(error_message) def add_error_message(message)
@errors << error_message @errors << filtered_error_message(message)
end end
def after_export_in_progress? def after_export_in_progress?
...@@ -52,8 +50,25 @@ module Gitlab ...@@ -52,8 +50,25 @@ module Gitlab
@project.disk_path @project.disk_path
end end
def error_out(message, caller) def log_error(details)
Rails.logger.error("Import/Export error raised on #{caller}: #{message}") @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 end
def after_export_lock_file def after_export_lock_file
......
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
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
include ImportExport::CommonUtil include ImportExport::CommonUtil
describe Gitlab::ImportExport::VersionChecker do 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 describe 'bundle a project Git repo' do
let(:version) { Gitlab::ImportExport.version } let(:version) { Gitlab::ImportExport.version }
......
# 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
...@@ -136,12 +136,12 @@ describe Projects::ImportService do ...@@ -136,12 +136,12 @@ describe Projects::ImportService do
end end
it 'fails if repository import fails' do 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 result = subject.execute
expect(result[:status]).to eq :error 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 end
context 'when repository import scheduled' do context 'when repository import scheduled' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment