summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRobert Speicher <robert@gitlab.com>2016-05-19 03:00:01 +0000
committerRobert Speicher <robert@gitlab.com>2016-05-19 03:00:01 +0000
commit4607323e130fe5b04e830f7a6de8083b070808f1 (patch)
treea00611843950a53a5963e5a2a31e748e11102246
parent26eb3dd731c8eb8ade81a77f34c0444d693ac22a (diff)
parent0c47b68d0474b595bb03a49ce755c96f5d00fbf1 (diff)
downloadgitlab-ce-4607323e130fe5b04e830f7a6de8083b070808f1.tar.gz
Merge branch 'issue_17560' into 'master'
Mask credentials from URL when the import of project has failed. REF: #17560 See merge request !4185
-rw-r--r--app/models/project.rb4
-rw-r--r--app/workers/repository_import_worker.rb2
-rw-r--r--db/migrate/20160302152808_remove_wrong_import_url_from_projects.rb2
-rw-r--r--lib/gitlab/url_sanitizer.rb (renamed from lib/gitlab/import_url.rb)17
-rw-r--r--spec/lib/gitlab/import_url_spec.rb21
-rw-r--r--spec/lib/gitlab/url_sanitizer_spec.rb68
-rw-r--r--spec/workers/repository_import_worker_spec.rb26
7 files changed, 107 insertions, 33 deletions
diff --git a/app/models/project.rb b/app/models/project.rb
index 907d80c0681..9a34198e098 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -369,14 +369,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/url_sanitizer.rb
index d23b013c1f5..c59d53b941a 100644
--- a/lib/gitlab/import_url.rb
+++ b/lib/gitlab/url_sanitizer.rb
@@ -1,7 +1,13 @@
module Gitlab
- class ImportUrl
+ 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 = URI.parse(URI.encode(url))
+ @url = Addressable::URI.parse(URI.encode(url))
@credentials = credentials
end
@@ -9,6 +15,13 @@ module Gitlab
@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
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..de55334118f
--- /dev/null
+++ b/spec/lib/gitlab/url_sanitizer_spec.rb
@@ -0,0 +1,68 @@
+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 '.sanitize' do
+ def sanitize_url(url)
+ # We want to try with multi-line content because is how error messages are formatted
+ described_class.sanitize(%Q{
+ remote: Not Found
+ fatal: repository '#{url}' not found
+ })
+ end
+
+ it 'mask the credentials from HTTP URLs' do
+ filtered_content = sanitize_url('http://user:pass@test.com/root/repoC.git/')
+
+ expect(filtered_content).to include("http://*****:*****@test.com/root/repoC.git/")
+ end
+
+ it 'mask the credentials from HTTPS URLs' do
+ filtered_content = sanitize_url('https://user:pass@test.com/root/repoA.git/')
+
+ expect(filtered_content).to include("https://*****:*****@test.com/root/repoA.git/")
+ end
+
+ it 'mask credentials from SSH URLs' do
+ filtered_content = sanitize_url('ssh://user@host.test/path/to/repo.git')
+
+ 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
+ filtered_content = sanitize_url('git://host.test/path/to/repo.git')
+
+ expect(filtered_content).to include("git://host.test/path/to/repo.git")
+ end
+
+ it 'does not modify scp-like URLs' do
+ filtered_content = sanitize_url('user@server:project.git')
+
+ expect(filtered_content).to include("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 '#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
+
+end
diff --git a/spec/workers/repository_import_worker_spec.rb b/spec/workers/repository_import_worker_spec.rb
index 6739063543b..f1b1574abf4 100644
--- a/spec/workers/repository_import_worker_spec.rb
+++ b/spec/workers/repository_import_worker_spec.rb
@@ -6,14 +6,28 @@ 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).to include("https://*****:*****@test.com/root/repoC.git/")
+ end
end
end
end