Merge pull request #2882 from evazion/fix-tag-name-validation

Enforce stricter rules for tag names

app/logical/tag_name_validator.rb

@@ -0,0 +1,30 @@
+class TagNameValidator < ActiveModel::EachValidator
+  def validate_each(record, attribute, value)
+    case Tag.normalize_name(value)
+    when /\A_*\z/
+      record.errors[attribute] << "'#{value}' cannot be blank"
+    when /\*/
+      record.errors[attribute] << "'#{value}' cannot contain asterisks ('*')"
+    when /,/
+      record.errors[attribute] << "'#{value}' cannot contain commas (',')"
+    when /\A~/
+      record.errors[attribute] << "'#{value}' cannot begin with a tilde ('~')"
+    when /\A-/
+      record.errors[attribute] << "'#{value}' cannot begin with a dash ('-')"
+    when /\A_/
+      record.errors[attribute] << "'#{value}' cannot begin with an underscore"
+    when /_\z/
+      record.errors[attribute] << "'#{value}' cannot end with an underscore"
+    when /__/
+      record.errors[attribute] << "'#{value}' cannot contain consecutive underscores"
+    when /[^[[:graph:]]]/
+      record.errors[attribute] << "'#{value}' cannot contain non-printable characters"
+    when /[^[[:ascii:]]]/
+      record.errors[attribute] << "'#{value}' must consist of only ASCII characters"
+    when /\A(#{Tag::METATAGS}|#{Tag::SUBQUERY_METATAGS}):(.+)\z/i
+      record.errors[attribute] << "'#{value}' cannot begin with '#{$1}:'"
+    when /\A(#{Tag.categories.regexp}):(.+)\z/i
+      record.errors[attribute] << "'#{value}' cannot begin with '#{$1}:'"
+    end
+  end
+end

app/models/post.rb

@@ -16,6 +16,7 @@ class Post < ActiveRecord::Base
   before_validation :remove_parent_loops
   validates_uniqueness_of :md5, :on => :create
   validates_inclusion_of :rating, in: %w(s q e), message: "rating must be s, q, or e"
+  validate :tag_names_are_valid
   validate :post_is_not_its_own_parent
   validate :updater_can_change_rating
   before_save :update_tag_post_counts
@@ -1307,13 +1308,6 @@ class Post < ActiveRecord::Base
       end
     end
 
-    def post_is_not_its_own_parent
-      if !new_record? && id == parent_id
-        errors[:base] << "Post cannot have itself as a parent"
-        false
-      end
-    end
-
     def parent_exists?
       Post.exists?(parent_id)
     end
@@ -1689,6 +1683,40 @@ class Post < ActiveRecord::Base
       end
     end
   end
+
+  module ValidationMethods
+    def post_is_not_its_own_parent
+      if !new_record? && id == parent_id
+        errors[:base] << "Post cannot have itself as a parent"
+        false
+      end
+    end
+
+    def updater_can_change_rating
+      if rating_changed? && is_rating_locked?
+        # Don't forbid changes if the rating lock was just now set in the same update.
+        if !is_rating_locked_changed?
+          errors.add(:rating, "is locked and cannot be changed. Unlock the post first.")
+        end
+      end
+    end
+
+    def tag_names_are_valid
+      # only validate new tags; allow invalid names for tags that already exist.
+      added_tags = tag_array - tag_array_was
+      new_tags = added_tags - Tag.where(name: added_tags).pluck(:name)
+
+      new_tags.each do |name|
+        tag = Tag.new
+        tag.name = name
+        tag.valid?
+
+        tag.errors.messages.each do |attribute, messages|
+          errors[:tag_string] << "tag #{attribute} #{messages.join(';')}"
+        end
+      end
+    end
+  end
   
   include FileMethods
   include ImageMethods
@@ -1709,6 +1737,7 @@ class Post < ActiveRecord::Base
   extend SearchMethods
   include PixivMethods
   include IqdbMethods
+  include ValidationMethods
   include Danbooru::HasBitFlags
 
   BOOLEAN_ATTRIBUTES = %w(
@@ -1765,15 +1794,6 @@ class Post < ActiveRecord::Base
     save
   end
 
-  def updater_can_change_rating
-    if rating_changed? && is_rating_locked?
-      # Don't forbid changes if the rating lock was just now set in the same update.
-      if !is_rating_locked_changed?
-        errors.add(:rating, "is locked and cannot be changed. Unlock the post first.")
-      end
-    end
-  end
-
   def update_column(name, value)
     ret = super(name, value)
     notify_pubsub

app/models/tag.rb

@@ -6,6 +6,8 @@ class Tag < ActiveRecord::Base
   attr_accessible :is_locked, :as => [:moderator, :admin]
   has_one :wiki_page, :foreign_key => "title", :primary_key => "name"
 
+  validates :name, uniqueness: true, tag_name: true, on: :create
+
   module ApiMethods
     def to_legacy_json
       return {
@@ -179,7 +181,7 @@ class Tag < ActiveRecord::Base
 
   module NameMethods
     def normalize_name(name)
-      name.mb_chars.downcase.tr(" ", "_").gsub(/\A[-~]+/, "").gsub(/\*/, "").to_s
+      name.to_s.mb_chars.downcase.strip.tr(" ", "_").to_s
     end
 
     def find_or_create_by_name(name, options = {})
@@ -227,13 +229,13 @@ class Tag < ActiveRecord::Base
     def scan_query(query)
       tagstr = normalize(query)
       list = tagstr.scan(/-?source:".*?"/) || []
-      list + tagstr.gsub(/-?source:".*?"/, "").scan(/\S+/).uniq
+      list + tagstr.gsub(/-?source:".*?"/, "").scan(/[^[:space:]]+/).uniq
     end
 
     def scan_tags(tags, options = {})
       tagstr = normalize(tags)
       list = tagstr.scan(/source:".*?"/) || []
-      list += tagstr.gsub(/source:".*?"/, "").gsub(/[%,]/, "").scan(/\S+/).uniq
+      list += tagstr.gsub(/source:".*?"/, "").scan(/[^[:space:]]+/).uniq
       if options[:strip_metatags]
         list = list.map {|x| x.sub(/^[-~]/, "")}
       end

test/unit/post_test.rb

@@ -558,6 +558,72 @@ class PostTest < ActiveSupport::TestCase
         end
       end
 
+      context "tagged with a valid tag" do
+        subject { @post }
+
+        should allow_value("touhou 100%").for(:tag_string)
+        should allow_value("touhou FOO").for(:tag_string)
+        should allow_value("touhou -foo").for(:tag_string)
+        should allow_value("touhou pool:foo").for(:tag_string)
+        should allow_value("touhou -pool:foo").for(:tag_string)
+        should allow_value("touhou newpool:foo").for(:tag_string)
+        should allow_value("touhou fav:self").for(:tag_string)
+        should allow_value("touhou -fav:self").for(:tag_string)
+        should allow_value("touhou upvote:self").for(:tag_string)
+        should allow_value("touhou downvote:self").for(:tag_string)
+        should allow_value("touhou parent:1").for(:tag_string)
+        should allow_value("touhou child:1").for(:tag_string)
+        should allow_value("touhou source:foo").for(:tag_string)
+        should allow_value("touhou rating:z").for(:tag_string)
+        should allow_value("touhou locked:rating").for(:tag_string)
+        should allow_value("touhou -locked:rating").for(:tag_string)
+
+        # \u3000 = ideographic space, \u00A0 = no-break space
+        should allow_value("touhou\u3000foo").for(:tag_string)
+        should allow_value("touhou\u00A0foo").for(:tag_string)
+      end
+
+      context "tagged with an invalid tag" do
+        subject { @post }
+
+        context "that doesn't already exist" do
+          should_not allow_value("touhou user:evazion").for(:tag_string)
+          should_not allow_value("touhou *~foo").for(:tag_string)
+          should_not allow_value("touhou *-foo").for(:tag_string)
+          should_not allow_value("touhou ,-foo").for(:tag_string)
+
+          should_not allow_value("touhou ___").for(:tag_string)
+          should_not allow_value("touhou ~foo").for(:tag_string)
+          should_not allow_value("touhou _foo").for(:tag_string)
+          should_not allow_value("touhou foo_").for(:tag_string)
+          should_not allow_value("touhou foo__bar").for(:tag_string)
+          should_not allow_value("touhou foo*bar").for(:tag_string)
+          should_not allow_value("touhou foo,bar").for(:tag_string)
+          should_not allow_value("touhou foo\abar").for(:tag_string)
+          should_not allow_value("touhou café").for(:tag_string)
+          should_not allow_value("touhou 東方").for(:tag_string)
+        end
+
+        context "that already exists" do
+          setup do
+            %W(___ ~foo _foo foo_ foo__bar foo*bar foo,bar foo\abar café 東方).each do |tag|
+              FactoryGirl.build(:tag, name: tag).save(validate: false)
+            end
+          end
+
+          should allow_value("touhou ___").for(:tag_string)
+          should allow_value("touhou ~foo").for(:tag_string)
+          should allow_value("touhou _foo").for(:tag_string)
+          should allow_value("touhou foo_").for(:tag_string)
+          should allow_value("touhou foo__bar").for(:tag_string)
+          should allow_value("touhou foo*bar").for(:tag_string)
+          should allow_value("touhou foo,bar").for(:tag_string)
+          should allow_value("touhou foo\abar").for(:tag_string)
+          should allow_value("touhou café").for(:tag_string)
+          should allow_value("touhou 東方").for(:tag_string)
+        end
+      end
+
       context "tagged with a metatag" do
         context "for a parent" do
           setup do
@@ -674,6 +740,13 @@ class PostTest < ActiveSupport::TestCase
                 assert_equal("pool:#{@pool.id} pool:series", @post.pool_string)
               end
             end
+
+            context "with special characters" do
+              should "not strip '%' from the name" do
+                @post.update(tag_string: "aaa newpool:ichigo_100%")
+                assert(Pool.exists?(name: "ichigo_100%"))
+              end
+            end
           end
         end
 

test/unit/tag_test.rb

@@ -137,9 +137,9 @@ class TagTest < ActiveSupport::TestCase
       assert_equal(%w(~AAa -BBB* -bbb*), Tag.scan_query("~AAa -BBB* -bbb*"))
     end
 
-    should "strip out invalid characters when scanning" do
+    should "not strip out valid characters when scanning" do
       assert_equal(%w(aaa bbb), Tag.scan_tags("aaa bbb"))
-      assert_equal(%w(-BB;B* -b_b_b_), Tag.scan_tags("-B,B;B* -b_b_b_"))
+      assert_equal(%w(favgroup:yondemasu_yo,_azazel-san. pool:ichigo_100%), Tag.scan_tags("favgroup:yondemasu_yo,_azazel-san. pool:ichigo_100%"))
     end
 
     should "cast values" do
@@ -207,6 +207,32 @@ class TagTest < ActiveSupport::TestCase
         assert_equal(Tag.categories.artist, tag.category)
       end
     end
+
+    context "during name validation" do
+      # tags with spaces or uppercase are allowed because they are normalized
+      # to lowercase with underscores.
+      should allow_value(" foo ").for(:name).on(:create)
+      should allow_value("foo bar").for(:name).on(:create)
+      should allow_value("FOO").for(:name).on(:create)
+
+      should_not allow_value("").for(:name).on(:create)
+      should_not allow_value("___").for(:name).on(:create)
+      should_not allow_value("~foo").for(:name).on(:create)
+      should_not allow_value("-foo").for(:name).on(:create)
+      should_not allow_value("_foo").for(:name).on(:create)
+      should_not allow_value("foo_").for(:name).on(:create)
+      should_not allow_value("foo__bar").for(:name).on(:create)
+      should_not allow_value("foo*bar").for(:name).on(:create)
+      should_not allow_value("foo,bar").for(:name).on(:create)
+      should_not allow_value("foo\abar").for(:name).on(:create)
+      should_not allow_value("café").for(:name).on(:create)
+      should_not allow_value("東方").for(:name).on(:create)
+
+      metatags = Tag::METATAGS.split("|") + Tag::SUBQUERY_METATAGS.split("|") + Danbooru.config.tag_category_mapping.keys
+      metatags.split("|").each do |metatag|
+        should_not allow_value("#{metatag}:foo").for(:name).on(:create)
+      end
+    end
   end
 
   context "A tag with a negative post count" do