Mask credentials from URL when import of project has failed.issue_17560

7 files changed, 103 insertions, 33 deletions

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/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..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