Merge branch 'fix-url-validator' into 'master'

Allow UrlValidator to work with attr_encrypted

See merge request gitlab-org/gitlab-ce!21776

2 files changed, 26 insertions, 3 deletions

diff --git a/app/validators/url_validator.rb b/app/validators/url_validator.rb
index faaf1283078..216acf79cbd 100644
--- a/app/validators/url_validator.rb
+++ b/app/validators/url_validator.rb
@@ -41,12 +41,13 @@ class UrlValidator < ActiveModel::EachValidator
   def validate_each(record, attribute, value)
     @record = record
 
-    if value.present?
-      value.strip!
-    else
+    unless value.present?
       record.errors.add(attribute, 'must be a valid URL')
+      return
     end
 
+    value = strip_value!(record, attribute, value)
+
     Gitlab::UrlBlocker.validate!(value, blocker_args)
   rescue Gitlab::UrlBlocker::BlockedUrlError => e
     record.errors.add(attribute, "is blocked: #{e.message}")
@@ -54,6 +55,13 @@ class UrlValidator < ActiveModel::EachValidator
 
   private
 
+  def strip_value!(record, attribute, value)
+    new_value = value.strip
+    return value if new_value == value
+
+    record.public_send("#{attribute}=", new_value) # rubocop:disable GitlabSecurity/PublicSend
+  end
+
   def default_options
     # By default the validator doesn't block any url based on the ip address
     {
diff --git a/spec/validators/url_validator_spec.rb b/spec/validators/url_validator_spec.rb
index 93fe013d11c..ab6100509a6 100644
--- a/spec/validators/url_validator_spec.rb
+++ b/spec/validators/url_validator_spec.rb
@@ -24,6 +24,21 @@ describe UrlValidator do
 
       expect(badge.errors.empty?).to be true
     end
+
+    it 'strips urls' do
+      badge.link_url = "\n\r\n\nhttps://127.0.0.1\r\n\r\n\n\n\n"
+
+      # It's unusual for a validator to modify its arguments. Some extensions,
+      # such as attr_encrypted, freeze the string to signal that modifications
+      # will not be persisted, so freeze this string to ensure the scheme is
+      # compatible with them.
+      badge.link_url.freeze
+
+      subject
+
+      expect(badge.errors).to be_empty
+      expect(badge.link_url).to eq('https://127.0.0.1')
+    end
   end
 
   context 'when allow_localhost is set to false' do