From 42635966281faa2ca79376c02c01976dc5c8047f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 18 May 2016 13:17:32 -0500 Subject: Mask credentials from URL when import of project has failed. --- app/models/project.rb | 4 +- app/workers/repository_import_worker.rb | 2 +- ...152808_remove_wrong_import_url_from_projects.rb | 2 +- lib/gitlab/import_url.rb | 41 -------------- lib/gitlab/url_sanitizer.rb | 54 +++++++++++++++++++ spec/lib/gitlab/import_url_spec.rb | 21 -------- spec/lib/gitlab/url_sanitizer_spec.rb | 63 ++++++++++++++++++++++ spec/workers/repository_import_worker_spec.rb | 27 +++++++--- 8 files changed, 142 insertions(+), 72 deletions(-) delete mode 100644 lib/gitlab/import_url.rb create mode 100644 lib/gitlab/url_sanitizer.rb delete mode 100644 spec/lib/gitlab/import_url_spec.rb create mode 100644 spec/lib/gitlab/url_sanitizer_spec.rb diff --git a/app/models/project.rb b/app/models/project.rb index a3c4f1d8e9b..b464335597d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -376,14 +376,14 @@ class Project < ActiveRecord::Base end def import_url=(value) - import_url = Gitlab::ImportUrl.new(value) + import_url = Gitlab::UrlSanitizer.new(value) create_or_update_import_data(credentials: import_url.credentials) super(import_url.sanitized_url) end def import_url if import_data && super - import_url = Gitlab::ImportUrl.new(super, credentials: import_data.credentials) + import_url = Gitlab::UrlSanitizer.new(super, credentials: import_data.credentials) import_url.full_url else super diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index 2937493c614..fbc7ed63c6a 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -13,7 +13,7 @@ class RepositoryImportWorker result = Projects::ImportService.new(project, current_user).execute if result[:status] == :error - project.update(import_error: result[:message]) + project.update(import_error: Gitlab::UrlSanitizer.sanitize(result[:message])) project.import_fail return end diff --git a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb index 8a351cf27a3..561c18a5776 100644 --- a/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb +++ b/db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb @@ -24,7 +24,7 @@ class RemoveWrongImportUrlFromProjects < ActiveRecord::Migration def process_projects_with_wrong_url projects_with_wrong_import_url.each do |project| begin - import_url = Gitlab::ImportUrl.new(project["import_url"]) + import_url = Gitlab::UrlSanitizer.new(project["import_url"]) update_import_url(import_url, project) update_import_data(import_url, project) diff --git a/lib/gitlab/import_url.rb b/lib/gitlab/import_url.rb deleted file mode 100644 index d23b013c1f5..00000000000 --- a/lib/gitlab/import_url.rb +++ /dev/null @@ -1,41 +0,0 @@ -module Gitlab - class ImportUrl - def initialize(url, credentials: nil) - @url = URI.parse(URI.encode(url)) - @credentials = credentials - end - - def sanitized_url - @sanitized_url ||= safe_url.to_s - end - - def credentials - @credentials ||= { user: @url.user, password: @url.password } - end - - def full_url - @full_url ||= generate_full_url.to_s - end - - private - - def generate_full_url - return @url unless valid_credentials? - @full_url = @url.dup - @full_url.user = credentials[:user] - @full_url.password = credentials[:password] - @full_url - end - - def safe_url - safe_url = @url.dup - safe_url.password = nil - safe_url.user = nil - safe_url - end - - def valid_credentials? - credentials && credentials.is_a?(Hash) && credentials.any? - end - end -end diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb new file mode 100644 index 00000000000..c59d53b941a --- /dev/null +++ b/lib/gitlab/url_sanitizer.rb @@ -0,0 +1,54 @@ +module Gitlab + class UrlSanitizer + def self.sanitize(content) + regexp = URI::Parser.new.make_regexp(['http', 'https', 'ssh', 'git']) + + content.gsub(regexp) { |url| new(url).masked_url } + end + + def initialize(url, credentials: nil) + @url = Addressable::URI.parse(URI.encode(url)) + @credentials = credentials + end + + def sanitized_url + @sanitized_url ||= safe_url.to_s + end + + def masked_url + url = @url.dup + url.password = "*****" unless url.password.nil? + url.user = "*****" unless url.user.nil? + url.to_s + end + + def credentials + @credentials ||= { user: @url.user, password: @url.password } + end + + def full_url + @full_url ||= generate_full_url.to_s + end + + private + + def generate_full_url + return @url unless valid_credentials? + @full_url = @url.dup + @full_url.user = credentials[:user] + @full_url.password = credentials[:password] + @full_url + end + + def safe_url + safe_url = @url.dup + safe_url.password = nil + safe_url.user = nil + safe_url + end + + def valid_credentials? + credentials && credentials.is_a?(Hash) && credentials.any? + end + end +end diff --git a/spec/lib/gitlab/import_url_spec.rb b/spec/lib/gitlab/import_url_spec.rb deleted file mode 100644 index f758cb8693c..00000000000 --- a/spec/lib/gitlab/import_url_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'spec_helper' - -describe Gitlab::ImportUrl do - - let(:credentials) { { user: 'blah', password: 'password' } } - let(:import_url) do - Gitlab::ImportUrl.new("https://github.com/me/project.git", credentials: credentials) - end - - describe :full_url do - it { expect(import_url.full_url).to eq("https://blah:password@github.com/me/project.git") } - end - - describe :sanitized_url do - it { expect(import_url.sanitized_url).to eq("https://github.com/me/project.git") } - end - - describe :credentials do - it { expect(import_url.credentials).to eq(credentials) } - end -end diff --git a/spec/lib/gitlab/url_sanitizer_spec.rb b/spec/lib/gitlab/url_sanitizer_spec.rb new file mode 100644 index 00000000000..118445fdf17 --- /dev/null +++ b/spec/lib/gitlab/url_sanitizer_spec.rb @@ -0,0 +1,63 @@ +require 'spec_helper' + +describe Gitlab::UrlSanitizer, lib: true do + let(:credentials) { { user: 'blah', password: 'password' } } + let(:url_sanitizer) do + described_class.new("https://github.com/me/project.git", credentials: credentials) + end + + describe '#full_url' do + it { expect(url_sanitizer.full_url).to eq("https://blah:password@github.com/me/project.git") } + + it 'supports scp-like URLs' do + sanitizer = described_class.new('user@server:project.git') + + expect(sanitizer.full_url).to eq('user@server:project.git') + end + end + + describe '#sanitized_url' do + it { expect(url_sanitizer.sanitized_url).to eq("https://github.com/me/project.git") } + end + + describe '#credentials' do + it { expect(url_sanitizer.credentials).to eq(credentials) } + end + + describe '.sanitize' do + let(:filtered_content) do + described_class.sanitize(%Q{remote: Not Found + fatal: repository 'http://user:pass@test.com/root/repoC.git/' not found + remote: Not Found + fatal: repository 'https://user:pass@test.com/root/repoA.git/' not found + remote: Not Found + ssh://user@host.test/path/to/repo.git + remote: Not Found + git://host.test/path/to/repo.git + remote: Not Found + user@server:project.git + }) + end + + it 'mask the credentials from HTTP URLs' do + expect(filtered_content).to include("http://*****:*****@test.com/root/repoC.git/") + end + + it 'mask the credentials from HTTPS URLs' do + expect(filtered_content).to include("https://*****:*****@test.com/root/repoA.git/") + end + + it 'mask credentials from SSH URLs' do + expect(filtered_content).to include("ssh://*****@host.test/path/to/repo.git") + end + + it 'does not modify Git URLs' do + # git protocol does not support authentication + expect(filtered_content).to include("git://host.test/path/to/repo.git") + end + + it 'does not modify scp-like URLs' do + expect(filtered_content).to include("user@server:project.git") + end + end +end diff --git a/spec/workers/repository_import_worker_spec.rb b/spec/workers/repository_import_worker_spec.rb index 6739063543b..2137e415c55 100644 --- a/spec/workers/repository_import_worker_spec.rb +++ b/spec/workers/repository_import_worker_spec.rb @@ -6,14 +6,29 @@ describe RepositoryImportWorker do subject { described_class.new } describe '#perform' do - it 'imports a project' do - expect_any_instance_of(Projects::ImportService).to receive(:execute). - and_return({ status: :ok }) + context 'when the import was successful' do + it 'imports a project' do + expect_any_instance_of(Projects::ImportService).to receive(:execute). + and_return({ status: :ok }) - expect_any_instance_of(Repository).to receive(:expire_emptiness_caches) - expect_any_instance_of(Project).to receive(:import_finish) + expect_any_instance_of(Repository).to receive(:expire_emptiness_caches) + expect_any_instance_of(Project).to receive(:import_finish) - subject.perform(project.id) + subject.perform(project.id) + end + end + + context 'when the import has failed' do + it 'hide the credentials that were used in the import URL' do + error = %Q{remote: Not Found fatal: repository 'https://user:pass@test.com/root/repoC.git/' not found } + expect_any_instance_of(Projects::ImportService).to receive(:execute). + and_return({ status: :error, message: error }) + + subject.perform(project.id) + + expect(project.reload.import_error).not_to include("https://user:pass@test.com/root/repoC.git/") + expect(project.reload.import_error).to include("https://*****:*****@test.com/root/repoC.git/") + end end end end -- cgit v1.2.1