From: Tom Hughes <tom@compton.nu>
Date: Mon, 5 Nov 2018 21:22:48 +0000 (+0000)
Subject: Merge remote-tracking branch 'upstream/pull/2044'
X-Git-Tag: live~2819
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/a39c645602f9d27b7927500c93890f4d7d79620e?hp=e93e5fbe63d89e2ea546fbb294d8d96e06ef39f1

Merge remote-tracking branch 'upstream/pull/2044'
---

diff --git a/app/models/changeset_comment.rb b/app/models/changeset_comment.rb
index 756fda14c..a0ad6f2ea 100644
--- a/app/models/changeset_comment.rb
+++ b/app/models/changeset_comment.rb
@@ -28,7 +28,7 @@ class ChangesetComment < ActiveRecord::Base
   validates :changeset, :presence => true, :associated => true
   validates :author, :presence => true, :associated => true
   validates :visible, :inclusion => [true, false]
-  validates :body, :format => /\A[^\x00-\x08\x0b-\x0c\x0e-\x1f\x7f\ufffe\uffff]*\z/
+  validates :body, :characters => true
 
   # Return the comment text
   def body
diff --git a/app/models/changeset_tag.rb b/app/models/changeset_tag.rb
index 8d6cd45ac..942fafb2a 100644
--- a/app/models/changeset_tag.rb
+++ b/app/models/changeset_tag.rb
@@ -21,6 +21,6 @@ class ChangesetTag < ActiveRecord::Base
   belongs_to :changeset
 
   validates :changeset, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => :changeset_id }
 end
diff --git a/app/models/diary_comment.rb b/app/models/diary_comment.rb
index 63eae3f21..ade7a64ea 100644
--- a/app/models/diary_comment.rb
+++ b/app/models/diary_comment.rb
@@ -28,7 +28,7 @@ class DiaryComment < ActiveRecord::Base
 
   scope :visible, -> { where(:visible => true) }
 
-  validates :body, :presence => true
+  validates :body, :presence => true, :characters => true
   validates :diary_entry, :user, :associated => true
 
   after_save :spam_check
diff --git a/app/models/diary_entry.rb b/app/models/diary_entry.rb
index 2a24d8002..d61241993 100644
--- a/app/models/diary_entry.rb
+++ b/app/models/diary_entry.rb
@@ -37,8 +37,8 @@ class DiaryEntry < ActiveRecord::Base
 
   scope :visible, -> { where(:visible => true) }
 
-  validates :title, :body, :presence => true
-  validates :title, :length => 1..255
+  validates :title, :presence => true, :length => 1..255, :characters => true
+  validates :body, :presence => true, :characters => true
   validates :latitude, :allow_nil => true,
                        :numericality => { :greater_than_or_equal_to => -90,
                                           :less_than_or_equal_to => 90 }
diff --git a/app/models/issue_comment.rb b/app/models/issue_comment.rb
index 3a5894cb3..0841295e1 100644
--- a/app/models/issue_comment.rb
+++ b/app/models/issue_comment.rb
@@ -24,7 +24,7 @@ class IssueComment < ActiveRecord::Base
   belongs_to :issue
   belongs_to :user
 
-  validates :body, :presence => true
+  validates :body, :presence => true, :characters => true
   validates :user, :presence => true
   validates :issue, :presence => true
 end
diff --git a/app/models/message.rb b/app/models/message.rb
index aff628e03..4e89cb2e1 100644
--- a/app/models/message.rb
+++ b/app/models/message.rb
@@ -32,6 +32,7 @@ class Message < ActiveRecord::Base
 
   validates :title, :presence => true, :utf8 => true, :length => 1..255
   validates :body, :sent_on, :sender, :recipient, :presence => true
+  validates :title, :body, :characters => true
 
   def self.from_mail(mail, from, to)
     if mail.multipart?
diff --git a/app/models/node_tag.rb b/app/models/node_tag.rb
index 20065b993..43915bc12 100644
--- a/app/models/node_tag.rb
+++ b/app/models/node_tag.rb
@@ -18,6 +18,6 @@ class NodeTag < ActiveRecord::Base
   belongs_to :node
 
   validates :node, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => :node_id }
 end
diff --git a/app/models/note_comment.rb b/app/models/note_comment.rb
index 1772b3f97..f94032e1e 100644
--- a/app/models/note_comment.rb
+++ b/app/models/note_comment.rb
@@ -33,8 +33,7 @@ class NoteComment < ActiveRecord::Base
   validates :visible, :inclusion => [true, false]
   validates :author, :associated => true
   validates :event, :inclusion => %w[opened closed reopened commented hidden]
-  validates :body, :length => { :maximum => 2000 },
-                   :format => /\A[^\x00-\x08\x0b-\x0c\x0e-\x1f\x7f\ufffe\uffff]*\z/
+  validates :body, :length => { :maximum => 2000 }, :characters => true
 
   # Return the comment text
   def body
diff --git a/app/models/old_node_tag.rb b/app/models/old_node_tag.rb
index 9e03d34f9..77b78751b 100644
--- a/app/models/old_node_tag.rb
+++ b/app/models/old_node_tag.rb
@@ -19,6 +19,6 @@ class OldNodeTag < ActiveRecord::Base
   belongs_to :old_node, :foreign_key => [:node_id, :version]
 
   validates :old_node, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => [:node_id, :version] }
 end
diff --git a/app/models/old_relation_tag.rb b/app/models/old_relation_tag.rb
index 052b60853..4a247949c 100644
--- a/app/models/old_relation_tag.rb
+++ b/app/models/old_relation_tag.rb
@@ -19,6 +19,6 @@ class OldRelationTag < ActiveRecord::Base
   belongs_to :old_relation, :foreign_key => [:relation_id, :version]
 
   validates :old_relation, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => [:relation_id, :version] }
 end
diff --git a/app/models/old_way_tag.rb b/app/models/old_way_tag.rb
index 8fffebc25..5832f6d4f 100644
--- a/app/models/old_way_tag.rb
+++ b/app/models/old_way_tag.rb
@@ -19,6 +19,6 @@ class OldWayTag < ActiveRecord::Base
   belongs_to :old_way, :foreign_key => [:way_id, :version]
 
   validates :old_way, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => [:way_id, :version] }
 end
diff --git a/app/models/redaction.rb b/app/models/redaction.rb
index b835864db..e6d748706 100644
--- a/app/models/redaction.rb
+++ b/app/models/redaction.rb
@@ -31,7 +31,8 @@ class Redaction < ActiveRecord::Base
   has_many :old_ways
   has_many :old_relations
 
-  validates :description, :presence => true
+  validates :title, :presence => true, :characters => true
+  validates :description, :presence => true, :characters => true
   validates :description_format, :inclusion => { :in => %w[text html markdown] }
 
   # this method overrides the AR default to provide the rich
diff --git a/app/models/relation_tag.rb b/app/models/relation_tag.rb
index 5dc609987..151615f72 100644
--- a/app/models/relation_tag.rb
+++ b/app/models/relation_tag.rb
@@ -18,6 +18,6 @@ class RelationTag < ActiveRecord::Base
   belongs_to :relation
 
   validates :relation, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => :relation_id }
 end
diff --git a/app/models/report.rb b/app/models/report.rb
index 63296a094..9bbf221df 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -27,7 +27,7 @@ class Report < ActiveRecord::Base
 
   validates :issue, :presence => true
   validates :user, :presence => true
-  validates :details, :presence => true
+  validates :details, :presence => true, :characters => true
   validates :category, :presence => true
 
   def self.categories_for(reportable)
diff --git a/app/models/trace.rb b/app/models/trace.rb
index 5096a81aa..1d0b11d43 100644
--- a/app/models/trace.rb
+++ b/app/models/trace.rb
@@ -38,8 +38,8 @@ class Trace < ActiveRecord::Base
   scope :tagged, ->(t) { joins(:tags).where(:gpx_file_tags => { :tag => t }) }
 
   validates :user, :presence => true, :associated => true
-  validates :name, :presence => true, :length => 1..255
-  validates :description, :presence => { :on => :create }, :length => 1..255
+  validates :name, :presence => true, :length => 1..255, :characters => true
+  validates :description, :presence => { :on => :create }, :length => 1..255, :characters => true
   validates :timestamp, :presence => true
   validates :visibility, :inclusion => %w[private public trackable identifiable]
 
diff --git a/app/models/tracetag.rb b/app/models/tracetag.rb
index 1a4fbd398..84b6c6dfa 100644
--- a/app/models/tracetag.rb
+++ b/app/models/tracetag.rb
@@ -22,5 +22,5 @@ class Tracetag < ActiveRecord::Base
   belongs_to :trace, :foreign_key => "gpx_id"
 
   validates :trace, :associated => true
-  validates :tag, :length => 1..255, :format => %r{\A[^/;.,?]*\z}
+  validates :tag, :length => 1..255, :characters => { :url_safe => true }
 end
diff --git a/app/models/user.rb b/app/models/user.rb
index de5529e02..27ed7b648 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -88,17 +88,14 @@ class User < ActiveRecord::Base
                     :default_url => "/assets/:class/:attachment/:style.png",
                     :styles => { :large => "100x100>", :small => "50x50>" }
 
-  validates :display_name, :presence => true, :allow_nil => true, :length => 3..255,
+  validates :display_name, :presence => true, :length => 3..255,
                            :exclusion => %w[new terms save confirm confirm-email go_public reset-password forgot-password suspended]
   validates :display_name, :if => proc { |u| u.display_name_changed? },
                            :uniqueness => { :case_sensitive => false }
   validates :display_name, :if => proc { |u| u.display_name_changed? },
-                           :format => { :with => %r{\A[^\x00-\x1f\x7f\ufffe\uffff/;.,?%#]*\z} }
-  validates :display_name, :if => proc { |u| u.display_name_changed? },
-                           :format => { :with => /\A\S/, :message => "has leading whitespace" }
-  validates :display_name, :if => proc { |u| u.display_name_changed? },
-                           :format => { :with => /\S\z/, :message => "has trailing whitespace" }
-  validates :email, :presence => true, :confirmation => true
+                           :characters => { :url_safe => true },
+                           :whitespace => { :leading => false, :trailing => false }
+  validates :email, :presence => true, :confirmation => true, :characters => true
   validates :email, :if => proc { |u| u.email_changed? },
                     :uniqueness => { :case_sensitive => false }
   validates :pass_crypt, :confirmation => true, :length => 8..255
diff --git a/app/models/user_block.rb b/app/models/user_block.rb
index 9f32862af..02af385a7 100644
--- a/app/models/user_block.rb
+++ b/app/models/user_block.rb
@@ -26,6 +26,7 @@
 
 class UserBlock < ActiveRecord::Base
   validate :moderator_permissions
+  validates :reason, :characters => true
 
   belongs_to :user, :class_name => "User", :foreign_key => :user_id
   belongs_to :creator, :class_name => "User", :foreign_key => :creator_id
diff --git a/app/models/user_preference.rb b/app/models/user_preference.rb
index 69b0e9dde..3963bd02a 100644
--- a/app/models/user_preference.rb
+++ b/app/models/user_preference.rb
@@ -17,7 +17,7 @@ class UserPreference < ActiveRecord::Base
   belongs_to :user
 
   validates :user, :presence => true, :associated => true
-  validates :k, :v, :length => 1..255
+  validates :k, :v, :length => 1..255, :characters => true
 
   # Turn this Node in to an XML Node without the <osm> wrapper.
   def to_xml_node
diff --git a/app/models/way_tag.rb b/app/models/way_tag.rb
index 8ef75f38e..c4df0abb5 100644
--- a/app/models/way_tag.rb
+++ b/app/models/way_tag.rb
@@ -18,6 +18,6 @@ class WayTag < ActiveRecord::Base
   belongs_to :way
 
   validates :way, :presence => true, :associated => true
-  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }
+  validates :k, :v, :allow_blank => true, :length => { :maximum => 255 }, :characters => true
   validates :k, :uniqueness => { :scope => :way_id }
 end
diff --git a/app/validators/characters_validator.rb b/app/validators/characters_validator.rb
new file mode 100644
index 000000000..ee5b1206a
--- /dev/null
+++ b/app/validators/characters_validator.rb
@@ -0,0 +1,12 @@
+class CharactersValidator < ActiveModel::EachValidator
+  INVALID_CHARS = "\x00-\x08\x0b-\x0c\x0e-\x1f\x7f\ufffe\uffff".freeze
+  INVALID_URL_CHARS = "/;.,?%#".freeze
+
+  def validate_each(record, attribute, value)
+    record.errors[attribute] << (options[:message] || I18n.t("validations.invalid_characters")) if value =~ /[#{INVALID_CHARS}]/
+
+    if options[:url_safe]
+      record.errors[attribute] << (options[:message] || I18n.t("validations.url_characters", :characters => INVALID_URL_CHARS)) if value =~ /[#{INVALID_URL_CHARS}]/
+    end
+  end
+end
diff --git a/app/validators/whitespace_validator.rb b/app/validators/whitespace_validator.rb
new file mode 100644
index 000000000..d177df92a
--- /dev/null
+++ b/app/validators/whitespace_validator.rb
@@ -0,0 +1,11 @@
+class WhitespaceValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    unless options.fetch(:leading, true)
+      record.errors[attribute] << (options[:message] || I18n.t("validations.leading_whitespace")) if value =~ /\A\s/
+    end
+
+    unless options.fetch(:trailing, true)
+      record.errors[attribute] << (options[:message] || I18n.t("validations.trailing_whitespace")) if value =~ /\s\z/
+    end
+  end
+end
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 7b13a894b..a9370018a 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -2562,3 +2562,8 @@ en:
       not_empty: "Redaction is not empty. Please un-redact all versions belonging to this redaction before destroying it."
       flash: "Redaction destroyed."
       error: "There was an error destroying this redaction."
+  validations:
+    leading_whitespace: "has leading whitespace"
+    trailing_whitespace: "has trailing whitespace"
+    invalid_characters: "contains invalid characters"
+    url_characters: "contains special URL characters (%{characters})"
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
index 0e618cd22..a17c30deb 100644
--- a/test/models/user_test.rb
+++ b/test/models/user_test.rb
@@ -65,16 +65,13 @@ class UserTest < ActiveSupport::TestCase
   def test_display_name_length
     user = build(:user)
     user.display_name = "123"
-    assert user.valid?, " should allow nil display name"
+    assert user.valid?, "should allow 3 char name name"
     user.display_name = "12"
     assert_not user.valid?, "should not allow 2 char name"
     user.display_name = ""
-    assert_not user.valid?
+    assert_not user.valid?, "should not allow blank/0 char name"
     user.display_name = nil
-    # Don't understand why it isn't allowing a nil value,
-    # when the validates statements specifically allow it
-    # It appears the database does not allow null values
-    assert_not user.valid?
+    assert_not user.valid?, "should not allow nil value"
   end
 
   def test_display_name_valid
@@ -82,14 +79,15 @@ class UserTest < ActiveSupport::TestCase
     # expact are allowed
     # However, would they affect the xml planet dumps?
     ok = ["Name", "'me", "he\"", "<hr>", "*ho", "\"help\"@",
-          "vergrößern", "ルシステムにも対応します", "輕觸搖晃的遊戲"]
+          "vergrößern", "ルシステムにも対応します", "輕觸搖晃的遊戲", "space space"]
     # These need to be 3 chars in length, otherwise the length test above
     # should be used.
     bad = ["<hr/>", "test@example.com", "s/f", "aa/", "aa;", "aa.",
            "aa,", "aa?", "/;.,?", "も対応します/", "#ping",
            "foo\x1fbar", "foo\x7fbar", "foo\ufffebar", "foo\uffffbar",
            "new", "terms", "save", "confirm", "confirm-email",
-           "go_public", "reset-password", "forgot-password", "suspended"]
+           "go_public", "reset-password", "forgot-password", "suspended",
+           "trailing whitespace ", " leading whitespace"]
     ok.each do |display_name|
       user = build(:user)
       user.display_name = display_name
diff --git a/test/validators/characters_validator_test.rb b/test/validators/characters_validator_test.rb
new file mode 100644
index 000000000..e1409a5dc
--- /dev/null
+++ b/test/validators/characters_validator_test.rb
@@ -0,0 +1,66 @@
+require "test_helper"
+
+class InvalidCharsValidatable
+  include ActiveModel::Validations
+  validates :chars, :characters => true
+  attr_accessor :chars
+end
+
+class InvalidUrlCharsValidatable
+  include ActiveModel::Validations
+  validates :chars, :characters => { :url_safe => true }
+  attr_accessor :chars
+end
+
+class CharactersValidatorTest < ActiveSupport::TestCase
+  include Rails::Dom::Testing::Assertions::SelectorAssertions
+
+  def test_with_valid_chars
+    c = InvalidCharsValidatable.new
+
+    valid = ["Name.", "'me", "he\"", "<hr>", "*ho", "\"help\"@",
+             "vergrößern", "ルシステムにも対応します", "輕觸搖晃的遊戲", "/;.,?%#"]
+
+    valid.each do |v|
+      c.chars = v
+      assert c.valid?, "'#{v}' should be valid"
+    end
+  end
+
+  def test_with_invalid_chars
+    c = InvalidCharsValidatable.new
+
+    invalid = ["\x7f<hr/>", "test@example.com\x0e-", "s/\x1ff", "aa/\ufffe",
+               "aa\x0b-,", "aa?\x08", "/;\uffff.,?", "\x00-も対応します/", "\x0c#ping",
+               "foo\x1fbar", "foo\x7fbar", "foo\ufffebar", "foo\uffffbar"]
+
+    invalid.each do |v|
+      c.chars = v
+      assert_not c.valid?, "'#{v}' should not be valid"
+    end
+  end
+
+  def test_with_valid_url_chars
+    c = InvalidUrlCharsValidatable.new
+
+    valid = ["Name", "'me", "he\"", "<hr>", "*ho", "\"help\"@",
+             "vergrößern", "ルシステムにも対応します", "輕觸搖晃的遊戲"]
+
+    valid.each do |v|
+      c.chars = v
+      assert c.valid?, "'#{v}' should be valid"
+    end
+  end
+
+  def test_with_invalid_url_chars
+    c = InvalidUrlCharsValidatable.new
+
+    invalid = ["Name.", "you;me", "he\"#", "<hr/>", "50%", "good?",
+               "vergrößern,deutsche", "ルシステムに;.も対応します", "輕觸搖/晃的遊戲", "/;.,?%#"]
+
+    invalid.each do |v|
+      c.chars = v
+      assert_not c.valid?, "'#{v}' should not be valid"
+    end
+  end
+end
diff --git a/test/validators/whitespace_validator_test.rb b/test/validators/whitespace_validator_test.rb
new file mode 100644
index 000000000..c908538d1
--- /dev/null
+++ b/test/validators/whitespace_validator_test.rb
@@ -0,0 +1,61 @@
+require "test_helper"
+
+class LeadingWhitespaceValidatable
+  include ActiveModel::Validations
+  validates :string, :whitespace => { :leading => false }
+  attr_accessor :string
+end
+
+class TrailingWhitespaceValidatable
+  include ActiveModel::Validations
+  validates :string, :whitespace => { :trailing => false }
+  attr_accessor :string
+end
+
+class WhitespaceValidatorTest < ActiveSupport::TestCase
+  include Rails::Dom::Testing::Assertions::SelectorAssertions
+
+  def test_with_leading_whitespace
+    validator = LeadingWhitespaceValidatable.new
+
+    strings = [" ", " test", "  ", "\ttest"]
+
+    strings.each do |v|
+      validator.string = v
+      assert_not validator.valid?, "'#{v}' should not be valid"
+    end
+  end
+
+  def test_without_leading_whitespace
+    validator = LeadingWhitespaceValidatable.new
+
+    strings = ["test", "test ", "t est", "test\t", ".test", "_test"]
+
+    strings.each do |v|
+      validator.string = v
+      assert validator.valid?, "'#{v}' should be valid"
+    end
+  end
+
+  def test_with_trailing_whitespace
+    validator = TrailingWhitespaceValidatable.new
+
+    strings = [" ", "test ", "  ", "test\t", "_test_ "]
+
+    strings.each do |v|
+      validator.string = v
+      assert_not validator.valid?, "'#{v}' should not be valid"
+    end
+  end
+
+  def test_without_trailing_whitespace
+    validator = TrailingWhitespaceValidatable.new
+
+    strings = ["test", " test", "tes t", "\ttest", "test.", "test_"]
+
+    strings.each do |v|
+      validator.string = v
+      assert validator.valid?, "'#{v}' should be valid"
+    end
+  end
+end