From b9d35eaf2c5402ff75bfc235a2267898ee0a94c7 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 15 Aug 2019 16:41:27 -0500 Subject: [PATCH] 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 | 11 ++++- app/models/post.rb | 31 ++++--------- app/models/tag.rb | 11 +---- test/unit/post_test.rb | 75 ++++++++++++++++++------------- test/unit/tag_test.rb | 11 +++++ 5 files changed, 76 insertions(+), 63 deletions(-) diff --git a/app/logical/tag_name_validator.rb b/app/logical/tag_name_validator.rb index ec7fe3c07..78fc09e66 100644 --- a/app/logical/tag_name_validator.rb +++ b/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 diff --git a/app/models/post.rb b/app/models/post.rb index cdaec6f5c..ed05adab0 100644 --- a/app/models/post.rb +++ b/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) - invalid_tags = Tag.invalid_cosplay_tags(tags) - if invalid_tags.present? - self.warnings[:base] << "The root tag must be a character tag: #{invalid_tags.map {|tag| "[b]#{tag}[/b]" }.join(", ")}" + def remove_invalid_tags(tag_names) + invalid_tags = tag_names.map { |name| Tag.new(name: name) }.select { |tag| tag.invalid?(:name) } + + 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 } diff --git a/app/models/tag.rb b/app/models/tag.rb index d7e2960b7..7c73a79fb 100644 --- a/app/models/tag.rb +++ b/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 diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index 29e2677ff..d80eeb346 100644 --- a/test/unit/post_test.rb +++ b/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) - 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) + assert_invalid_tag("user:evazion") + assert_invalid_tag("*~foo") + assert_invalid_tag("*-foo") + assert_invalid_tag(",-foo") - 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) + assert_invalid_tag("___") + assert_invalid_tag("~foo") + assert_invalid_tag("_foo") + assert_invalid_tag("foo_") + assert_invalid_tag("foo__bar") + assert_invalid_tag("foo*bar") + assert_invalid_tag("foo,bar") + assert_invalid_tag("foo\abar") + assert_invalid_tag("café") + 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) - 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) + assert_invalid_tag("___") + assert_invalid_tag("~foo") + assert_invalid_tag("_foo") + assert_invalid_tag("foo_") + assert_invalid_tag("foo__bar") + assert_invalid_tag("foo*bar") + assert_invalid_tag("foo,bar") + assert_invalid_tag("foo\abar") + assert_invalid_tag("café") + 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 diff --git a/test/unit/tag_test.rb b/test/unit/tag_test.rb index a01458075..28f8cab21 100644 --- a/test/unit/tag_test.rb +++ b/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