Fix #3272: Unicode tags are still being allowed.

* Don't allow adding tags with invalid names when they already exist in
  the tags table.
* If an invalid tag is added, show an warning and ignore the tag instead
  of failing with a hard error.
* Move the _(cosplay) tag validation into the tag name validator.

app/logical/tag_name_validator.rb

@@ -17,14 +17,21 @@ class TagNameValidator < ActiveModel::EachValidator
       record.errors[attribute] << "'#{value}' cannot end with an underscore"
     when /__/
       record.errors[attribute] << "'#{value}' cannot contain consecutive underscores"
-    when /[^[[:graph:]]]/
+    when /[^[:graph:]]/
       record.errors[attribute] << "'#{value}' cannot contain non-printable characters"
-    when /[^[[:ascii:]]]/
+    when /[^[:ascii:]]/
       record.errors[attribute] << "'#{value}' must consist of only ASCII characters"
     when /\A(#{Tag::METATAGS.join("|")}):(.+)\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}:'"
+    when /\A(.+)_\(cosplay\)\z/i
+      tag_name = TagAlias.to_aliased([$1]).first
+      tag = Tag.find_by_name(tag_name)
+
+      if tag.present? && tag.category != Tag.categories.character
+        record.errors[attribute] << "#{tag_name} must be a character tag"
+      end
     end
   end
 end

app/models/post.rb

@@ -21,7 +21,6 @@ class Post < ApplicationRecord
   before_validation :remove_parent_loops
   validates_uniqueness_of :md5, :on => :create, message: ->(obj, data) { "duplicate: #{Post.find_by_md5(obj.md5).id}"}
   validates_inclusion_of :rating, in: %w(s q e), message: "rating must be s, q, or e"
-  validate :tag_names_are_valid
   validate :added_tags_are_valid
   validate :removed_tags_are_valid
   validate :has_artist_tag
@@ -662,12 +661,16 @@ class Post < ApplicationRecord
       set_tag_string(normalized_tags.join(" "))
     end
 
-    def remove_invalid_tags(tags)
+    def remove_invalid_tags(tag_names)
-      invalid_tags = Tag.invalid_cosplay_tags(tags)
+      invalid_tags = tag_names.map { |name| Tag.new(name: name) }.select { |tag| tag.invalid?(:name) }
-      if invalid_tags.present?
+
-        self.warnings[:base] << "The root tag must be a character tag: #{invalid_tags.map {|tag| "[b]#{tag}[/b]" }.join(", ")}"
+      invalid_tags.each do |tag|
+        tag.errors.messages.each do |attribute, messages|
+          warnings[:base] << "Couldn't add tag: #{messages.join(';')}"
+        end
       end
-      tags - invalid_tags
+
+      tag_names - invalid_tags.map(&:name)
     end
 
     def remove_negated_tags(tags)
@@ -1755,22 +1758,6 @@ class Post < ApplicationRecord
       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
-
     def added_tags_are_valid
       new_tags = added_tags.select { |t| t.post_count <= 0 }
       new_general_tags = new_tags.select { |t| t.category == Tag.categories.general }

app/models/tag.rb

@@ -52,7 +52,8 @@ class Tag < ApplicationRecord
   has_many :antecedent_implications, -> {active}, :class_name => "TagImplication", :foreign_key => "antecedent_name", :primary_key => "name"
   has_many :consequent_implications, -> {active}, :class_name => "TagImplication", :foreign_key => "consequent_name", :primary_key => "name"
 
-  validates :name, uniqueness: true, tag_name: true, on: :create
+  validates :name, tag_name: true, uniqueness: true, on: :create
+  validates :name, tag_name: true, on: :name
   validates_inclusion_of :category, in: TagCategory.category_ids
 
   before_save :update_category_cache, if: :category_changed?
@@ -1019,14 +1020,6 @@ class Tag < ApplicationRecord
     cosplay_tags.grep(/\A(.+)_\(cosplay\)\Z/) { "#{TagAlias.to_aliased([$1]).first}_(cosplay)" } + other_tags
   end
 
-  def self.invalid_cosplay_tags(tags)
-    tags.grep(/\A(.+)_\(cosplay\)\Z/) {|match| [match,TagAlias.to_aliased([$1]).first] }.
-      select do |name|
-        tag = Tag.find_by_name(name[1])
-        !tag.nil? && tag.category != Tag.categories.character
-      end.map {|tag| tag[0]}
-  end
-
   def editable_by?(user)
     return true if user.is_admin?
     return true if !is_locked? && user.is_builder? && post_count < 1_000

test/unit/post_test.rb

@@ -5,6 +5,16 @@ class PostTest < ActiveSupport::TestCase
     assert_equal(posts.map(&:id), Post.tag_match(query).pluck(:id))
   end
 
+  def self.assert_invalid_tag(tag_name)
+    should "not allow '#{tag_name}' to be tagged" do
+      post = build(:post, tag_string: "touhou #{tag_name}")
+
+      assert(post.valid?)
+      assert_equal("touhou", post.tag_string)
+      assert_equal(1, post.warnings[:base].grep(/Couldn't add tag/).count)
+    end
+  end
+
   def setup
     super
 
@@ -664,43 +674,41 @@ class PostTest < ActiveSupport::TestCase
       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)
+          assert_invalid_tag("user:evazion")
-          should_not allow_value("touhou *~foo").for(:tag_string)
+          assert_invalid_tag("*~foo")
-          should_not allow_value("touhou *-foo").for(:tag_string)
+          assert_invalid_tag("*-foo")
-          should_not allow_value("touhou ,-foo").for(:tag_string)
+          assert_invalid_tag(",-foo")
 
-          should_not allow_value("touhou ___").for(:tag_string)
+          assert_invalid_tag("___")
-          should_not allow_value("touhou ~foo").for(:tag_string)
+          assert_invalid_tag("~foo")
-          should_not allow_value("touhou _foo").for(:tag_string)
+          assert_invalid_tag("_foo")
-          should_not allow_value("touhou foo_").for(:tag_string)
+          assert_invalid_tag("foo_")
-          should_not allow_value("touhou foo__bar").for(:tag_string)
+          assert_invalid_tag("foo__bar")
-          should_not allow_value("touhou foo*bar").for(:tag_string)
+          assert_invalid_tag("foo*bar")
-          should_not allow_value("touhou foo,bar").for(:tag_string)
+          assert_invalid_tag("foo,bar")
-          should_not allow_value("touhou foo\abar").for(:tag_string)
+          assert_invalid_tag("foo\abar")
-          should_not allow_value("touhou café").for(:tag_string)
+          assert_invalid_tag("café")
-          should_not allow_value("touhou 東方").for(:tag_string)
+          assert_invalid_tag("東方")
         end
 
         context "that already exists" do
           setup do
             %W(___ ~foo _foo foo_ foo__bar foo*bar foo,bar foo\abar café 東方).each do |tag|
-              FactoryBot.build(:tag, name: tag).save(validate: false)
+              build(:tag, name: tag).save(validate: false)
             end
           end
 
-          should allow_value("touhou ___").for(:tag_string)
+          assert_invalid_tag("___")
-          should allow_value("touhou ~foo").for(:tag_string)
+          assert_invalid_tag("~foo")
-          should allow_value("touhou _foo").for(:tag_string)
+          assert_invalid_tag("_foo")
-          should allow_value("touhou foo_").for(:tag_string)
+          assert_invalid_tag("foo_")
-          should allow_value("touhou foo__bar").for(:tag_string)
+          assert_invalid_tag("foo__bar")
-          should allow_value("touhou foo*bar").for(:tag_string)
+          assert_invalid_tag("foo*bar")
-          should allow_value("touhou foo,bar").for(:tag_string)
+          assert_invalid_tag("foo,bar")
-          should allow_value("touhou foo\abar").for(:tag_string)
+          assert_invalid_tag("foo\abar")
-          should allow_value("touhou café").for(:tag_string)
+          assert_invalid_tag("café")
-          should allow_value("touhou 東方").for(:tag_string)
+          assert_invalid_tag("東方")
         end
       end
 
@@ -1234,17 +1242,24 @@ class PostTest < ActiveSupport::TestCase
       end
 
       context "with *_(cosplay) tags" do
-        setup do
+        should "add the character tags and the cosplay tag" do
           @post.add_tag("hakurei_reimu_(cosplay)")
           @post.add_tag("hatsune_miku_(cosplay)")
           @post.save
-        end
 
-        should "add the character tags and the cosplay tag" do
           assert(@post.has_tag?("hakurei_reimu"))
           assert(@post.has_tag?("hatsune_miku"))
           assert(@post.has_tag?("cosplay"))
         end
+
+        should "not add the _(cosplay) tag if it conflicts with an existing tag" do
+          create(:tag, name: "little_red_riding_hood", category: Tag.categories.copyright)
+          @post = create(:post, tag_string: "little_red_riding_hood_(cosplay)")
+
+          refute(@post.has_tag?("little_red_riding_hood"))
+          refute(@post.has_tag?("cosplay"))
+          assert(@post.warnings[:base].grep(/Couldn't add tag/).present?)
+        end
       end
 
       context "that has been updated" do

test/unit/tag_test.rb

@@ -260,6 +260,17 @@ class TagTest < ActiveSupport::TestCase
       metatags.each do |metatag|
         should_not allow_value("#{metatag}:foo").for(:name).on(:create)
       end
+
+      context "a cosplay tag" do
+        setup do
+          create(:tag, name: "bkub", category: Tag.categories.artist)
+          create(:tag, name: "fumimi", category: Tag.categories.character)
+        end
+
+        should allow_value("fumimi_(cosplay)").for(:name)
+        should allow_value("new_tag_(cosplay)").for(:name)
+        should_not allow_value("bkub_(cosplay)").for(:name)
+      end
     end
   end