Merge branch 'issue_447' into 'master'

Don't expose URL credentials in errors generated by Mirror sync.

REF: #447 

See merge request !382

app/models/project.rb

@@ -462,14 +462,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
@@ -571,7 +571,7 @@ class Project < ActiveRecord::Base
 
   def mark_import_as_failed(error_message)
     import_fail
-    update_column(:import_error, error_message)
+    update_column(:import_error, Gitlab::UrlSanitizer.sanitize(error_message))
   end
 
   def has_remote_mirror?

app/models/remote_mirror.rb

@@ -95,11 +95,11 @@ class RemoteMirror < ActiveRecord::Base
 
   def mark_as_failed(error_message)
     update_fail
-    update_column(:last_error, error_message)
+    update_column(:last_error, Gitlab::UrlSanitizer.sanitize(error_message))
   end
 
   def url=(value)
-    mirror_url = Gitlab::ImportUrl.new(value)
+    mirror_url = Gitlab::UrlSanitizer.new(value)
     self.credentials = mirror_url.credentials
 
     super(mirror_url.sanitized_url)
@@ -107,7 +107,7 @@ class RemoteMirror < ActiveRecord::Base
 
   def url
     if super
-      Gitlab::ImportUrl.new(super, credentials: credentials).full_url
+      Gitlab::UrlSanitizer.new(super, credentials: credentials).full_url
     end
   end
 

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)

lib/gitlab/import_url.rb → lib/gitlab/url_sanitizer.rb

 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).sanitized_url }
+    end
+
     def initialize(url, credentials: nil)
       @url = URI.parse(URI.encode(url))
       @credentials = credentials

spec/lib/gitlab/import_url_spec.rb

-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

spec/lib/gitlab/url_sanitizer_spec.rb

+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") }
+  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
+      })
+    end
+
+    it 'remove credentials from HTTP URLs' do
+      expect(filtered_content).to include("http://test.com/root/repoC.git/")
+    end
+
+    it 'remove credentials from HTTPS URLs' do
+      expect(filtered_content).to include("https://test.com/root/repoA.git/")
+    end
+
+    it 'remove 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
+  end
+end