From 0e222f02d8b8de24577b754eea4539b29621719f Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 17 Jun 2016 15:09:39 +0200
Subject: [PATCH 01/14] fixing URL validation for import_url on projects

---
 app/models/project.rb                       |  4 +-
 app/validators/addressable_url_validator.rb | 49 +++++++++++++++++++++
 spec/models/project_spec.rb                 |  5 +++
 3 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 app/validators/addressable_url_validator.rb

diff --git a/app/models/project.rb b/app/models/project.rb
index 0bb815e64e..a3f78349e9 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -153,9 +153,7 @@ class Project < ActiveRecord::Base
   validates :namespace, presence: true
   validates_uniqueness_of :name, scope: :namespace_id
   validates_uniqueness_of :path, scope: :namespace_id
-  validates :import_url,
-    url: { protocols: %w(ssh git http https) },
-    if: :external_import?
+  validates :import_url, addressable_url: true, if: :external_import?
   validates :star_count, numericality: { greater_than_or_equal_to: 0 }
   validate :check_limit, on: :create
   validate :avatar_type,
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
new file mode 100644
index 0000000000..4e1a01a1bf
--- /dev/null
+++ b/app/validators/addressable_url_validator.rb
@@ -0,0 +1,49 @@
+# UrlValidator
+#
+# Custom validator for URLs.
+#
+# By default, only URLs for the HTTP(S) protocols will be considered valid.
+# Provide a `:protocols` option to configure accepted protocols.
+#
+# Example:
+#
+#   class User < ActiveRecord::Base
+#     validates :personal_url, url: true
+#
+#     validates :ftp_url, url: { protocols: %w(ftp) }
+#
+#     validates :git_url, url: { protocols: %w(http https ssh git) }
+#   end
+#
+class AddressableUrlValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    unless valid_url?(value)
+      record.errors.add(attribute, "must be a valid URL")
+    end
+  end
+
+  private
+
+  def default_options
+    @default_options ||= { protocols: %w(http https ssh git) }
+  end
+
+  def valid_url?(value)
+    return false unless value
+
+    value.strip!
+
+    valid_uri?(value) && valid_protocol?(value)
+  rescue Addressable::URI::InvalidURIError
+    false
+  end
+
+  def valid_uri?(value)
+    Addressable::URI.parse(strip).is_a?(Addressable::URI)
+  end
+
+  def valid_protocol?(value)
+    options = default_options.merge(self.options)
+    value =~ /\A#{URI.regexp(options[:protocols])}\z/
+  end
+end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index fedab1f913..c99fd7c633 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -63,6 +63,11 @@ describe Project, models: true do
       expect(project2).not_to be_valid
       expect(project2.errors[:limit_reached].first).to match(/Personal project creation is not allowed/)
     end
+
+    it 'should not allow an invalid URI as import_url' do
+      project2 = build(:project)
+      expect(project2).to be_valid
+    end
   end
 
   describe 'default_scope' do

From a5abec905fc69a9999887ea11335f032b4dfa957 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 20 Jun 2016 11:34:34 +0200
Subject: [PATCH 02/14] fix addressable url validator

---
 app/validators/addressable_url_validator.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 4e1a01a1bf..7aab66548e 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -1,18 +1,18 @@
-# UrlValidator
+# AddressableUrlValidator
 #
-# Custom validator for URLs.
+# Custom validator for URLs. This is a 
 #
-# By default, only URLs for the HTTP(S) protocols will be considered valid.
+# By default, only URLs for http, https, ssh, and git protocols will be considered valid.
 # Provide a `:protocols` option to configure accepted protocols.
 #
 # Example:
 #
 #   class User < ActiveRecord::Base
-#     validates :personal_url, url: true
+#     validates :personal_url, addressable_url: true
 #
-#     validates :ftp_url, url: { protocols: %w(ftp) }
+#     validates :ftp_url, addressable_url: { protocols: %w(ftp) }
 #
-#     validates :git_url, url: { protocols: %w(http https ssh git) }
+#     validates :git_url, addressable_url: { protocols: %w(http https ssh git) }
 #   end
 #
 class AddressableUrlValidator < ActiveModel::EachValidator
@@ -39,7 +39,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   end
 
   def valid_uri?(value)
-    Addressable::URI.parse(strip).is_a?(Addressable::URI)
+    Addressable::URI.parse(value).is_a?(Addressable::URI)
   end
 
   def valid_protocol?(value)

From 896e09d055979cdfe6e20a8b5939c9a263f7e48a Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 20 Jun 2016 15:31:03 +0200
Subject: [PATCH 03/14] started working on a migration for projects that have
 current import_url issues

---
 app/validators/addressable_url_validator.rb   | 14 ++---
 ...620110927_fix_no_validatable_import_url.rb | 52 +++++++++++++++++++
 2 files changed, 59 insertions(+), 7 deletions(-)
 create mode 100644 db/migrate/20160620110927_fix_no_validatable_import_url.rb

diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 7aab66548e..585dc182e2 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -1,6 +1,6 @@
 # AddressableUrlValidator
 #
-# Custom validator for URLs. This is a 
+# Custom validator for URLs. This is a
 #
 # By default, only URLs for http, https, ssh, and git protocols will be considered valid.
 # Provide a `:protocols` option to configure accepted protocols.
@@ -22,12 +22,6 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     end
   end
 
-  private
-
-  def default_options
-    @default_options ||= { protocols: %w(http https ssh git) }
-  end
-
   def valid_url?(value)
     return false unless value
 
@@ -38,6 +32,12 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     false
   end
 
+  private
+
+  def default_options
+    @default_options ||= { protocols: %w(http https ssh git) }
+  end
+
   def valid_uri?(value)
     Addressable::URI.parse(value).is_a?(Addressable::URI)
   end
diff --git a/db/migrate/20160620110927_fix_no_validatable_import_url.rb b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
new file mode 100644
index 0000000000..e56a8a0c85
--- /dev/null
+++ b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
@@ -0,0 +1,52 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class FixNoValidatableImportUrl < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  class SqlBatches
+
+    attr_reader :results, :query
+
+    def initialize(batch_size: 100, query:)
+      @offset = 0
+      @batch_size = batch_size
+      @query = query
+      @results = []
+    end
+
+    def next
+      @results = ActiveRecord::Base.connection.execute(batched_sql)
+      @offset += @batch_size
+      @results.any?
+    end
+
+    private
+
+    def batched_sql
+      "#{@query} OFFSET #{@offset} LIMIT #{@batch_size}"
+    end
+  end
+
+  def up
+    invalid_import_url_project_ids.each { |project_id| cleanup_import_url(project_id) }
+  end
+
+  def invalid_import_url_project_ids
+    ids = []
+    batches = SqlBatches.new(query: "SELECT id, import_url FROM projects WHERE import_url IS NOT NULL")
+
+    while batches.nexts
+      ids += batches.results.map { |result| invalid_url?(result[:import_url]) ? result[:id] : nil }
+    end
+
+    ids.compact
+  end
+
+  def invalid_url?(url)
+    AddressableUrlValidator.new({ attributes: 1 }).valid_url?(url)
+  end
+
+  def cleanup_import_url(project_id)
+    execute("UPDATE projects SET mirror = false, import_url = NULL WHERE id = #{project_id}")
+  end
+end

From 6d763831d00027600e4da9807e6be3afb47abd4b Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 20 Jun 2016 17:20:53 +0200
Subject: [PATCH 04/14] fixed a few MySQL issues and added changelog

---
 CHANGELOG                                     |  1 +
 app/validators/addressable_url_validator.rb   |  6 +--
 ...620110927_fix_no_validatable_import_url.rb | 53 +++++++++++++++----
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 44e6a19474..5b84d136d2 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.9.0 (unreleased)
+  - Set import_url validation to be more strict
   - Fix error when CI job variables key specified but not defined
   - Fix pipeline status when there are no builds in pipeline
   - Fix Error 500 when using closes_issues API with an external issue tracker
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 585dc182e2..ee6a5a1185 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -22,6 +22,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     end
   end
 
+  private
+
   def valid_url?(value)
     return false unless value
 
@@ -32,8 +34,6 @@ class AddressableUrlValidator < ActiveModel::EachValidator
     false
   end
 
-  private
-
   def default_options
     @default_options ||= { protocols: %w(http https ssh git) }
   end
@@ -44,6 +44,6 @@ class AddressableUrlValidator < ActiveModel::EachValidator
 
   def valid_protocol?(value)
     options = default_options.merge(self.options)
-    value =~ /\A#{URI.regexp(options[:protocols])}\z/
+    !!(value =~ /\A#{URI.regexp(options[:protocols])}\z/)
   end
 end
diff --git a/db/migrate/20160620110927_fix_no_validatable_import_url.rb b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
index e56a8a0c85..9cb84faaec 100644
--- a/db/migrate/20160620110927_fix_no_validatable_import_url.rb
+++ b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
@@ -1,5 +1,9 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
+# Updates project records containing invalid URLs using the AddressableUrlValidator.
+# This is optimized assuming the number of invalid records is low, but
+# we still need to loop through all the projects with an +import_url+
+# so we use batching for the latter.
+#
+# This migration is non-reversible as we would have to keep the old data.
 
 class FixNoValidatableImportUrl < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
@@ -14,8 +18,8 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
       @results = []
     end
 
-    def next
-      @results = ActiveRecord::Base.connection.execute(batched_sql)
+    def next?
+      @results = ActiveRecord::Base.connection.exec_query(batched_sql)
       @offset += @batch_size
       @results.any?
     end
@@ -23,11 +27,36 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
     private
 
     def batched_sql
-      "#{@query} OFFSET #{@offset} LIMIT #{@batch_size}"
+      "#{@query} LIMIT #{@batch_size} OFFSET #{@offset}"
+    end
+  end
+
+  # AddressableValidator - Snapshot of AddressableUrlValidator
+  module AddressableUrlValidatorSnap
+    extend self
+
+    def valid_url?(value)
+      return false unless value
+
+      value.strip!
+
+      valid_uri?(value) && valid_protocol?(value)
+    rescue Addressable::URI::InvalidURIError
+      false
+    end
+
+    def valid_uri?(value)
+      Addressable::URI.parse(value).is_a?(Addressable::URI)
+    end
+
+    def valid_protocol?(value)
+      !!(value =~ /\A#{URI.regexp(%w(http https ssh git))}\z/)
     end
   end
 
   def up
+    say('Cleaning up invalid import URLs... This may take a few minutes if we have a large number of imported projects.')
+
     invalid_import_url_project_ids.each { |project_id| cleanup_import_url(project_id) }
   end
 
@@ -35,18 +64,20 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
     ids = []
     batches = SqlBatches.new(query: "SELECT id, import_url FROM projects WHERE import_url IS NOT NULL")
 
-    while batches.nexts
-      ids += batches.results.map { |result| invalid_url?(result[:import_url]) ? result[:id] : nil }
+    while batches.next?
+      batches.results.each do |result|
+        ids << result['id'] unless valid_url?(result['import_url'])
+      end
     end
 
-    ids.compact
+    ids
   end
 
-  def invalid_url?(url)
-    AddressableUrlValidator.new({ attributes: 1 }).valid_url?(url)
+  def valid_url?(url)
+    AddressableUrlValidatorSnap.valid_url?(url)
   end
 
   def cleanup_import_url(project_id)
-    execute("UPDATE projects SET mirror = false, import_url = NULL WHERE id = #{project_id}")
+    execute("UPDATE projects SET import_url = NULL WHERE id = #{project_id}")
   end
 end

From 4273e07e009b63dfd69b824c244826e7e62ac057 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Mon, 20 Jun 2016 17:25:51 +0200
Subject: [PATCH 05/14] fix comment

---
 app/validators/addressable_url_validator.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index ee6a5a1185..64e8581e0d 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -1,6 +1,6 @@
 # AddressableUrlValidator
 #
-# Custom validator for URLs. This is a
+# Custom validator for URLs. This is a stricter version of UrlValidator.
 #
 # By default, only URLs for http, https, ssh, and git protocols will be considered valid.
 # Provide a `:protocols` option to configure accepted protocols.

From 48d76ecec8ec0387db4fbbda3f065c424c3ea51a Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 22 Jun 2016 16:39:16 +0200
Subject: [PATCH 06/14] another fix and fixed spec

---
 app/models/project.rb       | 2 ++
 spec/models/project_spec.rb | 9 ++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index ceebfcd733..64c1722ccf 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -447,6 +447,8 @@ class Project < ActiveRecord::Base
     import_url = Gitlab::UrlSanitizer.new(value)
     create_or_update_import_data(credentials: import_url.credentials)
     super(import_url.sanitized_url)
+  rescue Addressable::URI::InvalidURIError
+    errors.add(:import_url, 'must be a valid URL.')
   end
 
   def import_url
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 897b6898f5..5859691a3c 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -65,7 +65,14 @@ describe Project, models: true do
     end
 
     it 'should not allow an invalid URI as import_url' do
-      project2 = build(:project)
+      project2 = build(:project, import_url: 'invalid://')
+
+      expect(project2).not_to be_valid
+    end
+
+    it 'should allow a valid URI as import_url' do
+      project2 = build(:project, import_url: 'ssh://test@gitlab.com/project.git')
+
       expect(project2).to be_valid
     end
   end

From 1e1bf322896fc515157b943a35e41632e26cda07 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Wed, 22 Jun 2016 16:48:57 +0200
Subject: [PATCH 07/14] fix chnagelog

---
 CHANGELOG | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index bd6c922d30..4cbb1a9107 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,11 +1,12 @@
 Please view this file on the master branch, on stable branches it's out of date.
 
-v 8.9.0 (unreleased)
-  - Set import_url validation to be more strict
 v 8.10.0 (unreleased)
   - Wrap code blocks on Activies and Todos page. !4783 (winniehell)
   - Fix MR-auto-close text added to description. !4836
 
+v 8.9.1 (unreleased)
+   - Set import_url validation to be more strict
+
 v 8.9.0
   - Fix builds API response not including commit data
   - Fix error when CI job variables key specified but not defined

From 58c49966fa73469e324c51e26d8bc9a482627818 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 23 Jun 2016 17:18:02 +0200
Subject: [PATCH 08/14] updated validator based on feedback

---
 app/validators/addressable_url_validator.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 64e8581e0d..cbb80b5c68 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -29,9 +29,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
 
     value.strip!
 
-    valid_uri?(value) && valid_protocol?(value)
-  rescue Addressable::URI::InvalidURIError
-    false
+    valid_protocol?(value) && valid_uri?(value)
   end
 
   def default_options
@@ -40,6 +38,8 @@ class AddressableUrlValidator < ActiveModel::EachValidator
 
   def valid_uri?(value)
     Addressable::URI.parse(value).is_a?(Addressable::URI)
+  rescue Addressable::URI::InvalidURIError
+    false
   end
 
   def valid_protocol?(value)

From 8076d38a1487dd5b64153cd20eb696358b2f7acf Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 24 Jun 2016 11:35:32 +0200
Subject: [PATCH 09/14] added more info on how addressable URI differs from
 what we use in UrlValidator

---
 app/validators/addressable_url_validator.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index cbb80b5c68..634a15aea0 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -1,6 +1,8 @@
 # AddressableUrlValidator
 #
-# Custom validator for URLs. This is a stricter version of UrlValidator.
+# Custom validator for URLs. This is a stricter version of UrlValidator - it also checks
+# for using the right protocol, but it actually parses the URL checking for any syntax errors.
+# The regex is also different from `URI` as we use `Addressable::URI` here.
 #
 # By default, only URLs for http, https, ssh, and git protocols will be considered valid.
 # Provide a `:protocols` option to configure accepted protocols.

From 5b893d603dd68f263129523f13e8eb68b67fe790 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 30 Jun 2016 13:17:37 +0200
Subject: [PATCH 10/14] few changes based on feedback

---
 CHANGELOG                                           |  4 +---
 app/models/project.rb                               |  4 ++--
 app/validators/addressable_url_validator.rb         | 13 +++++--------
 .../20160620110927_fix_no_validatable_import_url.rb |  6 +++---
 lib/gitlab/url_sanitizer.rb                         | 10 +++++++++-
 5 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 9f76b8fa2d..118811cdda 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -14,6 +14,7 @@ v 8.10.0 (unreleased)
   - Check for conflicts with existing Project's wiki path when creating a new project.
   - Add API endpoint for a group issues !4520 (mahcsig)
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
+  - Set import_url validation to be more strict
 
 v 8.9.3 (unreleased)
   - Fix encrypted data backwards compatibility after upgrading attr_encrypted gem
@@ -66,9 +67,6 @@ v 8.9.1
   - Add SMTP as default delivery method to match gitlab-org/omnibus-gitlab!826. !4915
   - Remove duplicate 'New Page' button on edit wiki page
 
-v 8.9.1 (unreleased)
-   - Set import_url validation to be more strict
-
 v 8.9.0
   - Fix builds API response not including commit data
   - Fix error when CI job variables key specified but not defined
diff --git a/app/models/project.rb b/app/models/project.rb
index 2b1b25ab9d..89ce61b95e 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -445,11 +445,11 @@ class Project < ActiveRecord::Base
   end
 
   def import_url=(value)
+    return super(value) unless Gitlab::UrlSanitizer.valid?(value)
+
     import_url = Gitlab::UrlSanitizer.new(value)
     create_or_update_import_data(credentials: import_url.credentials)
     super(import_url.sanitized_url)
-  rescue Addressable::URI::InvalidURIError
-    errors.add(:import_url, 'must be a valid URL.')
   end
 
   def import_url
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 634a15aea0..c97acf7da9 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -18,6 +18,9 @@
 #   end
 #
 class AddressableUrlValidator < ActiveModel::EachValidator
+
+  DEFAULT_OPTIONS = { protocols: %w(http https ssh git) }
+
   def validate_each(record, attribute, value)
     unless valid_url?(value)
       record.errors.add(attribute, "must be a valid URL")
@@ -29,15 +32,9 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   def valid_url?(value)
     return false unless value
 
-    value.strip!
-
     valid_protocol?(value) && valid_uri?(value)
   end
 
-  def default_options
-    @default_options ||= { protocols: %w(http https ssh git) }
-  end
-
   def valid_uri?(value)
     Addressable::URI.parse(value).is_a?(Addressable::URI)
   rescue Addressable::URI::InvalidURIError
@@ -45,7 +42,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   end
 
   def valid_protocol?(value)
-    options = default_options.merge(self.options)
-    !!(value =~ /\A#{URI.regexp(options[:protocols])}\z/)
+    options = DEFAULT_OPTIONS.merge(self.options)
+    value =~ /\A#{URI.regexp(options[:protocols])}\z/
   end
 end
diff --git a/db/migrate/20160620110927_fix_no_validatable_import_url.rb b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
index 9cb84faaec..e111691ea3 100644
--- a/db/migrate/20160620110927_fix_no_validatable_import_url.rb
+++ b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
@@ -38,8 +38,6 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
     def valid_url?(value)
       return false unless value
 
-      value.strip!
-
       valid_uri?(value) && valid_protocol?(value)
     rescue Addressable::URI::InvalidURIError
       false
@@ -50,11 +48,13 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
     end
 
     def valid_protocol?(value)
-      !!(value =~ /\A#{URI.regexp(%w(http https ssh git))}\z/)
+      value =~ /\A#{URI.regexp(%w(http https ssh git))}\z/
     end
   end
 
   def up
+    return unless defined?(Addressable::URI::InvalidURIError)
+
     say('Cleaning up invalid import URLs... This may take a few minutes if we have a large number of imported projects.')
 
     invalid_import_url_project_ids.each { |project_id| cleanup_import_url(project_id) }
diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb
index 7d02fe3c97..2eb6085a3c 100644
--- a/lib/gitlab/url_sanitizer.rb
+++ b/lib/gitlab/url_sanitizer.rb
@@ -1,5 +1,9 @@
 module Gitlab
   class UrlSanitizer
+
+    attr_reader :valid
+    alias_method :valid?, :valid
+
     def self.sanitize(content)
       regexp = URI::Parser.new.make_regexp(['http', 'https', 'ssh', 'git'])
 
@@ -7,8 +11,12 @@ module Gitlab
     end
 
     def initialize(url, credentials: nil)
-      @url = Addressable::URI.parse(url)
+      @valid = true
+      @url = Addressable::URI.parse(url.strip)
       @credentials = credentials
+    rescue Addressable::URI::InvalidURIError
+      @valid = false
+      raise
     end
 
     def sanitized_url

From 545b92af067832d560846b7ec7cb26caf3302275 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 30 Jun 2016 14:30:07 +0200
Subject: [PATCH 11/14] use class method

---
 lib/gitlab/url_sanitizer.rb | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb
index 2eb6085a3c..50febfc18f 100644
--- a/lib/gitlab/url_sanitizer.rb
+++ b/lib/gitlab/url_sanitizer.rb
@@ -1,22 +1,23 @@
 module Gitlab
   class UrlSanitizer
 
-    attr_reader :valid
-    alias_method :valid?, :valid
-
     def self.sanitize(content)
       regexp = URI::Parser.new.make_regexp(['http', 'https', 'ssh', 'git'])
 
       content.gsub(regexp) { |url| new(url).masked_url }
     end
 
+    def self.valid?(url)
+      Addressable::URI.parse(url.strip)
+
+      true
+    rescue Addressable::URI::InvalidURIError
+      false
+    end
+
     def initialize(url, credentials: nil)
-      @valid = true
       @url = Addressable::URI.parse(url.strip)
       @credentials = credentials
-    rescue Addressable::URI::InvalidURIError
-      @valid = false
-      raise
     end
 
     def sanitized_url

From ef5713546bacc653f598eb692b728e35abdb8ab7 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 30 Jun 2016 17:22:56 +0200
Subject: [PATCH 12/14] few more changes from suggestions

---
 CHANGELOG                                                  | 2 --
 app/validators/addressable_url_validator.rb                | 1 -
 db/migrate/20160620110927_fix_no_validatable_import_url.rb | 5 ++++-
 lib/gitlab/url_sanitizer.rb                                | 1 -
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 5b12ba589b..3145178d54 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -25,8 +25,6 @@ v 8.10.0 (unreleased)
   - Add Bugzilla integration !4930 (iamtjg)
   - Allow [ci skip] to be in any case and allow [skip ci]. !4785 (simon_w)
   - Set import_url validation to be more strict
-
-v 8.9.3 (unreleased)
   - Add basic system information like memory and disk usage to the admin panel
 
 v 8.9.4 (unreleased)
diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index c97acf7da9..63761c8172 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -18,7 +18,6 @@
 #   end
 #
 class AddressableUrlValidator < ActiveModel::EachValidator
-
   DEFAULT_OPTIONS = { protocols: %w(http https ssh git) }
 
   def validate_each(record, attribute, value)
diff --git a/db/migrate/20160620110927_fix_no_validatable_import_url.rb b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
index e111691ea3..3e3837ab7e 100644
--- a/db/migrate/20160620110927_fix_no_validatable_import_url.rb
+++ b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
@@ -53,7 +53,10 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
   end
 
   def up
-    return unless defined?(Addressable::URI::InvalidURIError)
+    unless defined?(Addressable::URI::InvalidURIError)
+      say('Skipping cleaning up invalid import URLs as class from Addressable iss missing')
+      return
+    end
 
     say('Cleaning up invalid import URLs... This may take a few minutes if we have a large number of imported projects.')
 
diff --git a/lib/gitlab/url_sanitizer.rb b/lib/gitlab/url_sanitizer.rb
index 50febfc18f..86ed18fb50 100644
--- a/lib/gitlab/url_sanitizer.rb
+++ b/lib/gitlab/url_sanitizer.rb
@@ -1,6 +1,5 @@
 module Gitlab
   class UrlSanitizer
-
     def self.sanitize(content)
       regexp = URI::Parser.new.make_regexp(['http', 'https', 'ssh', 'git'])
 

From 26ce833a2143485bb0485c8b01d78561adf7c86d Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Thu, 30 Jun 2016 18:19:34 +0200
Subject: [PATCH 13/14] typo

---
 db/migrate/20160620110927_fix_no_validatable_import_url.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/db/migrate/20160620110927_fix_no_validatable_import_url.rb b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
index 3e3837ab7e..82a616c62d 100644
--- a/db/migrate/20160620110927_fix_no_validatable_import_url.rb
+++ b/db/migrate/20160620110927_fix_no_validatable_import_url.rb
@@ -54,7 +54,7 @@ class FixNoValidatableImportUrl < ActiveRecord::Migration
 
   def up
     unless defined?(Addressable::URI::InvalidURIError)
-      say('Skipping cleaning up invalid import URLs as class from Addressable iss missing')
+      say('Skipping cleaning up invalid import URLs as class from Addressable is missing')
       return
     end
 

From 54a50bf81d7bb304adaedffd8eb3e0bc0fc348a9 Mon Sep 17 00:00:00 2001
From: James Lopez <james@jameslopez.es>
Date: Fri, 1 Jul 2016 09:02:45 +0200
Subject: [PATCH 14/14] refactor url validator to use sanitizer for check

---
 app/validators/addressable_url_validator.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/app/validators/addressable_url_validator.rb b/app/validators/addressable_url_validator.rb
index 63761c8172..09bfa613cb 100644
--- a/app/validators/addressable_url_validator.rb
+++ b/app/validators/addressable_url_validator.rb
@@ -35,9 +35,7 @@ class AddressableUrlValidator < ActiveModel::EachValidator
   end
 
   def valid_uri?(value)
-    Addressable::URI.parse(value).is_a?(Addressable::URI)
-  rescue Addressable::URI::InvalidURIError
-    false
+    Gitlab::UrlSanitizer.valid?(value)
   end
 
   def valid_protocol?(value)