From d9dc84325f3651205cdf7b129de061d03ab9a2a0 Mon Sep 17 00:00:00 2001 From: evazion Date: Mon, 5 Dec 2022 01:22:20 -0600 Subject: [PATCH] Fix #5365: Don't allow whitespace-only text submission. Fix bug where it was possible to submit blank text in various text fields. Caused by `String#blank?` not considering certain Unicode characters as blank. `blank?` is defined as `match?(/\A[[:space:]]*\z/)`, where `[[:space:]]` matches ASCII spaces (space, tab, newline, etc) and Unicode characters in the Space category ([1]). However, there are other space-like characters not in the Space category. This includes U+200B (Zero-Width Space), and many more. It turns out the "Default ignorable code points" [2][3] are what we're after. These are the set of 400 or so formatting and control characters that are invisible when displayed. Note that there are other control characters that aren't invisible when rendered, instead they're shown with a placeholder glyph. These include the ASCII C0 and C1 control codes [4], certain Unicode control characters [5], and unassigned, reserved, and private use codepoints. There is one outlier: the Braille pattern blank (U+2800) [6]. This character is visually blank, but is not considered to be a space or an ignorable code point. [1]: https://codepoints.net/search?gc[]=Z [2]: https://codepoints.net/search?DI=1 [3]: https://www.unicode.org/review/pr-5.html [4]: https://codepoints.net/search?gc[]=Cc [5]: https://codepoints.net/search?gc[]=Cf [6]: https://codepoints.net/U+2800 [7]: https://en.wikipedia.org/wiki/Whitespace_character [8]: https://character.construction/blanks [9]: https://invisible-characters.com --- app/logical/visible_string_validator.rb | 20 ++++++++++++++++++++ app/models/ban.rb | 2 +- app/models/bulk_update_request.rb | 6 +++--- app/models/comment.rb | 2 +- app/models/dmail.rb | 4 ++-- app/models/favorite_group.rb | 2 +- app/models/forum_post.rb | 2 +- app/models/forum_topic.rb | 2 +- app/models/ip_ban.rb | 2 +- app/models/moderation_report.rb | 2 +- app/models/note.rb | 2 +- app/models/pool.rb | 2 +- app/models/post_appeal.rb | 2 +- app/models/post_flag.rb | 2 +- app/models/saved_search.rb | 2 +- app/models/user_feedback.rb | 2 +- app/models/wiki_page.rb | 2 +- config/initializers/core_extensions.rb | 16 ++++++++++++++++ test/unit/comment_test.rb | 5 ++++- test/unit/dmail_test.rb | 4 ++++ test/unit/favorite_group_test.rb | 3 +++ test/unit/forum_post_test.rb | 8 ++++++++ test/unit/forum_topic_test.rb | 8 ++++++++ test/unit/note_test.rb | 6 ++++++ test/unit/pool_test.rb | 14 +++++++++++--- test/unit/post_appeal_test.rb | 9 +++++++++ test/unit/post_flag_test.rb | 8 ++++++++ test/unit/string_test.rb | 22 ++++++++++++++++++++++ test/unit/user_feedback_test.rb | 6 ++++++ test/unit/wiki_page_test.rb | 8 ++++++++ 30 files changed, 152 insertions(+), 23 deletions(-) create mode 100644 app/logical/visible_string_validator.rb diff --git a/app/logical/visible_string_validator.rb b/app/logical/visible_string_validator.rb new file mode 100644 index 000000000..0716f588b --- /dev/null +++ b/app/logical/visible_string_validator.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# A custom validator for ensuring a string isn't blank. Like the `presence` validator, but checks for certain invisible +# Unicode characters such as zero-width spaces too. +# +# @example +# validates :body, visible_string: true +# +# @see https://invisible-characters.com/ +# @see https://guides.rubyonrails.org/active_record_validations.html#presence +# @see https://guides.rubyonrails.org/active_record_validations.html#custom-validators +class VisibleStringValidator < ActiveModel::EachValidator + def validate_each(record, attr, string) + return if options[:allow_empty] && string.empty? + + if string.nil? || string.invisible? + record.errors.add(attr, "can't be blank") + end + end +end diff --git a/app/models/ban.rb b/app/models/ban.rb index bc5747476..16c3fd61c 100644 --- a/app/models/ban.rb +++ b/app/models/ban.rb @@ -14,7 +14,7 @@ class Ban < ApplicationRecord validates :duration, presence: true validates :duration, inclusion: { in: [1.day, 3.days, 1.week, 1.month, 3.months, 6.months, 1.year, 100.years], message: "%{value} is not a valid ban duration" }, if: :duration_changed? - validates :reason, presence: true + validates :reason, visible_string: true validate :user, :validate_user_is_bannable, on: :create scope :unexpired, -> { where("bans.created_at + bans.duration > ?", Time.zone.now) } diff --git a/app/models/bulk_update_request.rb b/app/models/bulk_update_request.rb index 61d6cca9d..91f740531 100644 --- a/app/models/bulk_update_request.rb +++ b/app/models/bulk_update_request.rb @@ -10,9 +10,9 @@ class BulkUpdateRequest < ApplicationRecord belongs_to :forum_post, optional: true belongs_to :approver, optional: true, class_name: "User" - validates :reason, presence: true, on: :create - validates :script, presence: true - validates :title, presence: true, if: ->(rec) { rec.forum_topic_id.blank? } + validates :reason, visible_string: true, on: :create + validates :script, visible_string: true + validates :title, visible_string: true, if: ->(rec) { rec.forum_topic_id.blank? } validates :forum_topic, presence: true, if: ->(rec) { rec.forum_topic_id.present? } validates :status, inclusion: { in: STATUSES } validate :validate_script, if: :script_changed? diff --git a/app/models/comment.rb b/app/models/comment.rb index 4eeb6d16e..19fdbaa58 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -13,7 +13,7 @@ class Comment < ApplicationRecord has_many :active_votes, -> { active }, class_name: "CommentVote" has_many :mod_actions, as: :subject, dependent: :destroy - validates :body, presence: true, length: { maximum: 15_000 }, if: :body_changed? + validates :body, visible_string: true, length: { maximum: 15_000 }, if: :body_changed? before_create :autoreport_spam before_save :handle_reports_on_deletion diff --git a/app/models/dmail.rb b/app/models/dmail.rb index 1b6cbf788..2bfde03a7 100644 --- a/app/models/dmail.rb +++ b/app/models/dmail.rb @@ -4,8 +4,8 @@ class Dmail < ApplicationRecord attr_accessor :creator_ip_addr, :disable_email_notifications validate :validate_sender_is_not_limited, on: :create - validates :title, presence: true, length: { maximum: 200 }, if: :title_changed? - validates :body, presence: true, length: { maximum: 50_000 }, if: :body_changed? + validates :title, visible_string: true, length: { maximum: 200 }, if: :title_changed? + validates :body, visible_string: true, length: { maximum: 50_000 }, if: :body_changed? belongs_to :owner, :class_name => "User" belongs_to :to, :class_name => "User" diff --git a/app/models/favorite_group.rb b/app/models/favorite_group.rb index 9d744b102..4e80c5fbe 100644 --- a/app/models/favorite_group.rb +++ b/app/models/favorite_group.rb @@ -5,7 +5,7 @@ class FavoriteGroup < ApplicationRecord before_validation :normalize_name - validates :name, presence: true + validates :name, visible_string: true validates :name, uniqueness: { case_sensitive: false, scope: :creator_id } validate :validate_name, if: :name_changed? validate :creator_can_create_favorite_groups, :on => :create diff --git a/app/models/forum_post.rb b/app/models/forum_post.rb index cb5e6ec72..fbc4652f2 100644 --- a/app/models/forum_post.rb +++ b/app/models/forum_post.rb @@ -16,7 +16,7 @@ class ForumPost < ApplicationRecord has_one :tag_implication has_one :bulk_update_request - validates :body, presence: true, length: { maximum: 200_000 }, if: :body_changed? + validates :body, visible_string: true, length: { maximum: 200_000 }, if: :body_changed? validate :validate_deletion_of_original_post validate :validate_undeletion_of_post diff --git a/app/models/forum_topic.rb b/app/models/forum_topic.rb index fd8af88b2..a53d4e054 100644 --- a/app/models/forum_topic.rb +++ b/app/models/forum_topic.rb @@ -27,7 +27,7 @@ class ForumTopic < ApplicationRecord has_many :tag_implications has_many :mod_actions, as: :subject, dependent: :destroy - validates :title, presence: true, length: { maximum: 200 }, if: :title_changed? + validates :title, visible_string: true, length: { maximum: 200 }, if: :title_changed? validates :category_id, inclusion: { in: CATEGORIES.keys } validates :min_level, inclusion: { in: MIN_LEVELS.values } diff --git a/app/models/ip_ban.rb b/app/models/ip_ban.rb index 8440ea345..60fc911c1 100644 --- a/app/models/ip_ban.rb +++ b/app/models/ip_ban.rb @@ -7,7 +7,7 @@ class IpBan < ApplicationRecord has_many :mod_actions, as: :subject, dependent: :destroy validate :validate_ip_addr - validates :reason, presence: true + validates :reason, visible_string: true after_save :create_mod_action diff --git a/app/models/moderation_report.rb b/app/models/moderation_report.rb index 60e4d4f92..d6e192202 100644 --- a/app/models/moderation_report.rb +++ b/app/models/moderation_report.rb @@ -10,7 +10,7 @@ class ModerationReport < ApplicationRecord has_many :mod_actions, as: :subject, dependent: :destroy before_validation(on: :create) { model.lock! } - validates :reason, presence: true + validates :reason, visible_string: true validates :model_type, inclusion: { in: MODEL_TYPES } validates :creator, uniqueness: { scope: [:model_type, :model_id], message: "have already reported this message." }, on: :create diff --git a/app/models/note.rb b/app/models/note.rb index 3bde8579f..bd0625f85 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -11,7 +11,7 @@ class Note < ApplicationRecord validates :y, presence: true validates :width, presence: true validates :height, presence: true - validates :body, presence: true + validates :body, visible_string: true validate :note_within_image after_save :update_post after_save :create_version diff --git a/app/models/pool.rb b/app/models/pool.rb index 0a66afd2d..6250c08e7 100644 --- a/app/models/pool.rb +++ b/app/models/pool.rb @@ -6,7 +6,7 @@ class Pool < ApplicationRecord array_attribute :post_ids, parse: /\d+/, cast: :to_i - validates :name, uniqueness: { case_sensitive: false }, if: :name_changed? + validates :name, visible_string: true, uniqueness: { case_sensitive: false }, if: :name_changed? validate :validate_name, if: :name_changed? validates :category, inclusion: { in: %w[series collection] } validate :updater_can_edit_deleted diff --git a/app/models/post_appeal.rb b/app/models/post_appeal.rb index de0c5612d..437781f88 100644 --- a/app/models/post_appeal.rb +++ b/app/models/post_appeal.rb @@ -4,7 +4,7 @@ class PostAppeal < ApplicationRecord belongs_to :creator, :class_name => "User" belongs_to :post - validates :reason, length: { maximum: 140 } + validates :reason, visible_string: { allow_empty: true }, length: { maximum: 140 } validate :validate_post_is_appealable, on: :create validate :validate_creator_is_not_limited, on: :create validates :creator, uniqueness: { scope: :post, message: "have already appealed this post" }, on: :create diff --git a/app/models/post_flag.rb b/app/models/post_flag.rb index 10ad3bc2c..011a6687f 100644 --- a/app/models/post_flag.rb +++ b/app/models/post_flag.rb @@ -10,7 +10,7 @@ class PostFlag < ApplicationRecord belongs_to :post before_validation { post.lock! } - validates :reason, presence: true, length: { in: 1..140 } + validates :reason, visible_string: true, length: { in: 1..140 } validate :validate_creator_is_not_limited, on: :create validate :validate_post, on: :create validates :creator_id, uniqueness: { scope: :post_id, on: :create, unless: :is_deletion, message: "have already flagged this post" } diff --git a/app/models/saved_search.rb b/app/models/saved_search.rb index c4d547f64..da50b3ce9 100644 --- a/app/models/saved_search.rb +++ b/app/models/saved_search.rb @@ -11,7 +11,7 @@ class SavedSearch < ApplicationRecord normalize :query, :normalize_query normalize :labels, :normalize_labels - validates :query, presence: true + validates :query, visible_string: true validate :validate_count, on: :create scope :labeled, ->(label) { where_array_includes_any_lower(:labels, [normalize_label(label)]) } diff --git a/app/models/user_feedback.rb b/app/models/user_feedback.rb index 17c3611e6..3930d08ce 100644 --- a/app/models/user_feedback.rb +++ b/app/models/user_feedback.rb @@ -7,7 +7,7 @@ class UserFeedback < ApplicationRecord belongs_to :user belongs_to :creator, class_name: "User" - validates :body, presence: true + validates :body, visible_string: true validates :category, presence: true, inclusion: { in: %w[positive negative neutral] } after_create :create_dmail, unless: :disable_dmail_notification after_update :create_mod_action diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 240824ccc..d6a92a510 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -14,7 +14,7 @@ class WikiPage < ApplicationRecord array_attribute :other_names # XXX must come after `normalize :other_names` validates :title, tag_name: true, presence: true, uniqueness: true, if: :title_changed? - validates :body, presence: true, unless: -> { is_deleted? || other_names.present? } + validates :body, visible_string: true, unless: -> { is_deleted? || other_names.present? } validate :validate_rename validate :validate_other_names diff --git a/config/initializers/core_extensions.rb b/config/initializers/core_extensions.rb index 1dbf8c990..19272c542 100644 --- a/config/initializers/core_extensions.rb +++ b/config/initializers/core_extensions.rb @@ -5,6 +5,22 @@ require "danbooru" module Danbooru module Extensions module String + # https://invisible-characters.com + # https://character.construction/blanks + # https://www.unicode.org/review/pr-5.html (5.22 Default Ignorable Code Points) + # https://en.wikipedia.org/wiki/Whitespace_character + # + # [[:space:]] = https://codepoints.net/search?gc[]=Z (Space_Separator | Line_Separator | Paragraph_Separator | U+0009 | U+000A | U+000B | U+000C | U+000D | U+0085) + # \p{di} = https://codepoints.net/search?DI=1 (Default_Ignorable_Code_Point) + # \u2800 = https://codepoints.net/U+2800 (BRAILLE PATTERN BLANK) + INVISIBLE_REGEX = /\A[[:space:]\p{di}\u2800]*\z/ + + # Returns true if the string consists entirely of invisible characters. Like `#blank?`, but includes control + # characters and certain other invisible Unicode characters that aren't classified as spaces. + def invisible? + match?(INVISIBLE_REGEX) + end + def to_escaped_for_sql_like string = self.gsub(/%|_|\*|\\\*|\\\\|\\/) do |str| case str diff --git a/test/unit/comment_test.rb b/test/unit/comment_test.rb index 5eb39ad2b..8c2b63c4d 100644 --- a/test/unit/comment_test.rb +++ b/test/unit/comment_test.rb @@ -202,8 +202,11 @@ class CommentTest < ActiveSupport::TestCase end context "during validation" do - subject { FactoryBot.build(:comment) } + subject { build(:comment) } + + should_not allow_value("").for(:body) should_not allow_value(" ").for(:body) + should_not allow_value("\u200B").for(:body) end end end diff --git a/test/unit/dmail_test.rb b/test/unit/dmail_test.rb index 5f25bff0f..6269021df 100644 --- a/test/unit/dmail_test.rb +++ b/test/unit/dmail_test.rb @@ -149,8 +149,12 @@ class DmailTest < ActiveSupport::TestCase context "during validation" do subject { FactoryBot.build(:dmail) } + should_not allow_value("").for(:title) should_not allow_value(" ").for(:title) + should_not allow_value("\u200B").for(:title) + should_not allow_value("").for(:body) should_not allow_value(" ").for(:body) + should_not allow_value("\u200B").for(:body) should_not allow_value(nil).for(:to) should_not allow_value(nil).for(:from) should_not allow_value(nil).for(:owner) diff --git a/test/unit/favorite_group_test.rb b/test/unit/favorite_group_test.rb index a5f9e8d89..26699157f 100644 --- a/test/unit/favorite_group_test.rb +++ b/test/unit/favorite_group_test.rb @@ -68,5 +68,8 @@ class FavoriteGroupTest < ActiveSupport::TestCase should_not allow_value("_").for(:name) should_not allow_value("any").for(:name) should_not allow_value("none").for(:name) + should_not allow_value("").for(:name) + should_not allow_value(" ").for(:name) + should_not allow_value("\u200B").for(:name) end end diff --git a/test/unit/forum_post_test.rb b/test/unit/forum_post_test.rb index b29b646a4..e082d0441 100644 --- a/test/unit/forum_post_test.rb +++ b/test/unit/forum_post_test.rb @@ -165,5 +165,13 @@ class ForumPostTest < ActiveSupport::TestCase assert_equal(@second_user.id, @post.updater_id) end end + + context "during validation" do + subject { build(:forum_post) } + + should_not allow_value("").for(:body) + should_not allow_value(" ").for(:body) + should_not allow_value("\u200B").for(:body) + end end end diff --git a/test/unit/forum_topic_test.rb b/test/unit/forum_topic_test.rb index 207fa4f75..3d3bd0597 100644 --- a/test/unit/forum_topic_test.rb +++ b/test/unit/forum_topic_test.rb @@ -82,5 +82,13 @@ class ForumTopicTest < ActiveSupport::TestCase end end end + + context "during validation" do + subject { build(:forum_topic) } + + should_not allow_value("").for(:title) + should_not allow_value(" ").for(:title) + should_not allow_value("\u200B").for(:title) + end end end diff --git a/test/unit/note_test.rb b/test/unit/note_test.rb index 6129156f1..194a5a79c 100644 --- a/test/unit/note_test.rb +++ b/test/unit/note_test.rb @@ -145,5 +145,11 @@ class NoteTest < ActiveSupport::TestCase end end end + + context "when validating notes" do + should_not allow_value("").for(:body) + should_not allow_value(" ").for(:body) + should_not allow_value("\u200B").for(:body) + end end end diff --git a/test/unit/pool_test.rb b/test/unit/pool_test.rb index 8e2ba5f32..bd5d29cc8 100644 --- a/test/unit/pool_test.rb +++ b/test/unit/pool_test.rb @@ -244,9 +244,17 @@ class PoolTest < ActiveSupport::TestCase end context "when validating names" do - ["foo,bar", "foo*bar", "123", "___", " ", "any", "none", "series", "collection"].each do |bad_name| - should_not allow_value(bad_name).for(:name) - end + should_not allow_value("foo,bar").for(:name) + should_not allow_value("foo*bar").for(:name) + should_not allow_value("123").for(:name) + should_not allow_value("any").for(:name) + should_not allow_value("none").for(:name) + should_not allow_value("series").for(:name) + should_not allow_value("collection").for(:name) + should_not allow_value("___").for(:name) + should_not allow_value(" ").for(:name) + should_not allow_value("\u200B").for(:name) + should_not allow_value("").for(:name) end end diff --git a/test/unit/post_appeal_test.rb b/test/unit/post_appeal_test.rb index e69f97ab8..1b52d3d36 100644 --- a/test/unit/post_appeal_test.rb +++ b/test/unit/post_appeal_test.rb @@ -52,6 +52,15 @@ class PostAppealTest < ActiveSupport::TestCase end end end + + context "validation" do + subject { build(:post_appeal) } + + should allow_value("").for(:reason) + + should_not allow_value(" ").for(:reason) + should_not allow_value("\u200B").for(:reason) + end end end end diff --git a/test/unit/post_flag_test.rb b/test/unit/post_flag_test.rb index dde365a72..98db81a6d 100644 --- a/test/unit/post_flag_test.rb +++ b/test/unit/post_flag_test.rb @@ -114,5 +114,13 @@ class PostFlagTest < ActiveSupport::TestCase end end end + + context "during validation" do + subject { build(:post_flag) } + + should_not allow_value("").for(:reason) + should_not allow_value(" ").for(:reason) + should_not allow_value("\u200B").for(:reason) + end end end diff --git a/test/unit/string_test.rb b/test/unit/string_test.rb index 1691d4ec2..c178c8633 100644 --- a/test/unit/string_test.rb +++ b/test/unit/string_test.rb @@ -41,4 +41,26 @@ class StringTest < ActiveSupport::TestCase assert_equal("foo\r\nbar", "foo\u2029bar".normalize_whitespace) end end + + context "String#invisible?" do + should "work" do + assert_equal(true, "".invisible?) + assert_equal(true, " ".invisible?) + assert_equal(true, "\v\t\f\r\n".invisible?) + assert_equal(true, "\u00A0\u00AD\u034F\u061C\u115F\u1160\u17B4\u17B5\u180B\u180C\u180D\u180E".invisible?) + assert_equal(true, "\u200B\u200C\u200D\u200E\u200F\u2028\u2029\u2060\u206F\u2800\u3000\u3164".invisible?) + assert_equal(true, "\uFE00\uFE0F\uFEFF\uFFA0".invisible?) + assert_equal(true, "\u{E0001}\u{E007F}".invisible?) + + assert_equal(false, "foo".invisible?) + assert_equal(false, "\u0000".invisible?) # https://codepoints.net/U+0000 (NULL) + assert_equal(false, "\u0001".invisible?) # https://codepoints.net/U+0001 (START OF HEADING) + assert_equal(false, "\uE000".invisible?) # https://codepoints.net/U+E000 (PRIVATE USE CHARACTER) + assert_equal(false, "\uFDD0".invisible?) # https://codepoints.net/U+FDD0 (NONCHARACTER) + assert_equal(false, "\uFFF9".invisible?) # https://codepoints.net/U+FFF9 (INTERLINEAR ANNOTATION ANCHOR) + assert_equal(false, "\uFFFF".invisible?) # https://codepoints.net/U+FFFF (NONCHARACTER) + assert_equal(false, "\u{1D159}".invisible?) # https://codepoints.net/U+1D159 (MUSICAL SYMBOL NULL NOTEHEAD) + assert_equal(false, "\u{1D455}".invisible?) # https://codepoints.net/U+1D455 () + end + end end diff --git a/test/unit/user_feedback_test.rb b/test/unit/user_feedback_test.rb index 5f20faa66..034af6e7c 100644 --- a/test/unit/user_feedback_test.rb +++ b/test/unit/user_feedback_test.rb @@ -17,5 +17,11 @@ class UserFeedbackTest < ActiveSupport::TestCase assert_equal(dmail, user.dmails.last.body) end end + + context "on validation" do + should_not allow_value("").for(:body) + should_not allow_value(" ").for(:body) + should_not allow_value("\u200B").for(:body) + end end end diff --git a/test/unit/wiki_page_test.rb b/test/unit/wiki_page_test.rb index 4a754b886..ce2c3a8d4 100644 --- a/test/unit/wiki_page_test.rb +++ b/test/unit/wiki_page_test.rb @@ -114,6 +114,8 @@ class WikiPageTest < ActiveSupport::TestCase should normalize_attribute(:title).from(" Foo___ Bar ").to("foo_bar") should_not allow_value("").for(:title).on(:create) + should_not allow_value(" ").for(:title).on(:create) + should_not allow_value("\u200B").for(:title).on(:create) should_not allow_value("___").for(:title).on(:create) should_not allow_value("-foo").for(:title).on(:create) should_not allow_value("/foo").for(:title).on(:create) @@ -126,6 +128,12 @@ class WikiPageTest < ActiveSupport::TestCase should_not allow_value("X"*171).for(:title).on(:create) end + context "during body validation" do + should_not allow_value("").for(:body) + should_not allow_value(" ").for(:body) + should_not allow_value("\u200B").for(:body) + end + context "with other names" do should "not allow artist wikis to have other names" do tag = create(:artist_tag)