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] 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 9f76b8fa2d9..118811cdda5 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 2b1b25ab9d2..89ce61b95ec 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 634a15aea01..c97acf7da95 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 9cb84faaec1..e111691ea3c 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 7d02fe3c971..2eb6085a3ca 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