From ae27d15be7768e1bc0ffdd751ad26732b0c92709 Mon Sep 17 00:00:00 2001 From: Derek Crosson Date: Mon, 3 Jan 2022 16:37:03 +0200 Subject: [PATCH] Prevent whitespace unicode characters from being used in article title and tag (#14909) * test: whitespace unicode characters cannot be used as titles or tags * feat: add localized error message * refactor: use localized error message * fix: whitespace unicode characters cannot be used as titles or tags * chore: fix locale after merge * refactor: fix indentation * Fix spelling of prohibited in method names Additionally, rubocop removed a redundant user_id validation since Article belongs to user. * Fix spelling Missed one method call on the same line (fixed the first of two instances needing changes) * add failing test cases The reorganization to remove let was due to a limit on nesting rspec contexts (it inside context inside describe inside describe when trying to use different titles in let blocks in contexts) The first test was clarified (the "U+202D" string that looks like the code for a unicode point is valid, but the character \u202d is expected to be invalid The remaining two tests are based on the feedback I'd given in the PR, failing because we don't assert title is present after removing unicode whitespace, and we don't actually set the title (gsub is non-destructive, returning a value). * Set title to sanitized title contains_prohibitied_unicode_characters? was using == (which is not a good match for strings to regexes), use =~ instead Change invalid example characters in titles from \u202d (bidi override) to \u200a (hair space) Assert that the empty title is blank and can't be blank (even though it contains no prohibitd characters after replacement) * change the definition of prohibited characters From the description, it looks like "unicode space characters" was the desired rejection set. It was unclear what the existing regex was matching on (I couldn't get the expected examples to match correctly). Given the intent, I select "unicode space property, except the ascii space character", which may _also_ be incorrect but passed the tests. * Remove now-invalid expectation for presence of user id Since this was redundant (belongs_to user), we no longer will validate presence of user id, and we should not test for it. * Only reject bidirectional text controls The original issue pointed to the BIDI controls as problematic. While they called out other "whitespace" characters, as we accept a wider range of input languages, the need to handle non-printing punctuation (for example a group separating space for some asian languages). It's possible a wider list of characters should be added - if that's the case I suggest this regex and the sanitization be moved to a standalone class, and each of the recommendations in https://www.w3.org/TR/unicode-xml/#Charlist be checked for applicability. * Check for blank title after sanitizing disallowed characters Since validate_title modifies the title, and doesn't set error messages on the model itself, check for non-empty title _after_ potentially removing any disallowed characters. * Remove invalid characters before validation Remove the validate_title and contains_prohibited_characters methods, and center all logic on the remove prohibitied unicode characters method. Remove unneeded and unused arguments (input string is always title, replacement is always removal/empty string). * handle case when validating null title If the title's nil, we don't want to call match? since it will fail. Title will sometimes be nil when validating. Co-authored-by: Ben Halpern Co-authored-by: Dan Uber --- app/models/article.rb | 9 ++++++++- app/models/tag.rb | 2 +- config/locales/en.yml | 1 + config/locales/fr.yml | 1 + spec/models/article_spec.rb | 29 ++++++++++++++++++++++++++++- spec/models/tag_spec.rb | 5 +++++ 6 files changed, 44 insertions(+), 3 deletions(-) diff --git a/app/models/article.rb b/app/models/article.rb index 1cea1c313..170790af6 100644 --- a/app/models/article.rb +++ b/app/models/article.rb @@ -32,6 +32,7 @@ class Article < ApplicationRecord MAX_USER_MENTION_LIVE_AT = Time.utc(2021, 4, 7).freeze UNIQUE_URL_ERROR = "has already been taken. " \ "Email #{ForemInstance.email} for further details.".freeze + PROHIBITED_UNICODE_CHARACTERS_REGEX = /[\u202a-\u202e]/ # BIDI embedding controls has_one :discussion_lock, dependent: :delete @@ -81,7 +82,6 @@ class Article < ApplicationRecord validates :slug, presence: { if: :published? }, format: /\A[0-9a-z\-_]*\z/ validates :slug, uniqueness: { scope: :user_id } validates :title, presence: true, length: { maximum: 128 } - validates :user_id, presence: true validates :user_subscriptions_count, presence: true validates :video, url: { allow_blank: true, schemes: %w[https http] } validates :video_closed_caption_track_url, url: { allow_blank: true, schemes: ["https"] } @@ -101,6 +101,7 @@ class Article < ApplicationRecord validate :validate_co_authors_exist, unless: -> { co_author_ids.blank? } before_validation :evaluate_markdown, :create_slug + before_validation :remove_prohibited_unicode_characters before_save :update_cached_user before_save :set_all_dates @@ -847,4 +848,10 @@ class Article < ApplicationRecord ::Articles::EnrichImageAttributesWorker.perform_async(id) end + + def remove_prohibited_unicode_characters + return unless title&.match?(PROHIBITED_UNICODE_CHARACTERS_REGEX) + + self.title = title.gsub(PROHIBITED_UNICODE_CHARACTERS_REGEX, "") + end end diff --git a/app/models/tag.rb b/app/models/tag.rb index 7fe324c6b..967cd7b7f 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -77,7 +77,7 @@ class Tag < ActsAsTaggableOn::Tag # [:alnum:] is not used here because it supports diacritical characters. # If we decide to allow diacritics in the future, we should replace the # following regex with [:alnum:]. - errors.add(:name, "contains non-alphanumeric characters") unless name.match?(/\A[[:alnum:]]+\z/i) + errors.add(:name, I18n.t("errors.messages.contains_prohibited_characters")) unless name.match?(/\A[[:alnum:]]+\z/i) end def errors_as_sentence diff --git a/config/locales/en.yml b/config/locales/en.yml index 2f2486504..3d5fbb08f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -140,6 +140,7 @@ en: bad_uri: is an invalid url blank: can't be blank confirmation: doesn't match %{attribute} + contains_prohibited_characters: contains non-alphanumeric or prohibited unicode characters empty: can't be empty equal_to: must be equal to %{count} even: must be even diff --git a/config/locales/fr.yml b/config/locales/fr.yml index 287ef18c2..372a61661 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -157,6 +157,7 @@ fr: blank: ne peut pas être vide confirmation: ne correspond pas à %{attribute} confirmation_period_expired: doit être confirmé dans %{period}, veuillez en demander un nouveau + contains_prohibited_characters: contient des caractères unicode non alphanumériques ou interdits empty: ne peut pas être vide equal_to: doit être égal à %{count} even: doit être pair diff --git a/spec/models/article_spec.rb b/spec/models/article_spec.rb index d9bea1e29..87c351a94 100644 --- a/spec/models/article_spec.rb +++ b/spec/models/article_spec.rb @@ -47,7 +47,6 @@ RSpec.describe Article, type: :model do it { is_expected.to validate_presence_of(:reactions_count) } it { is_expected.to validate_presence_of(:user_subscriptions_count) } it { is_expected.to validate_presence_of(:title) } - it { is_expected.to validate_presence_of(:user_id) } it { is_expected.to validate_uniqueness_of(:slug).scoped_to(:user_id) } @@ -233,6 +232,34 @@ RSpec.describe Article, type: :model do end end + describe "title validation" do + it "produces a proper title" do + test_article = build(:article, title: "An Article Title") + + test_article.validate + + expect(test_article.title).to eq("An Article Title") + end + + it "sanitizes the title" do + test_article = build(:article, title: "\u202dThis starts with BIDI override") + + test_article.validate + + expect(test_article.title).not_to match(/\u202d/) + expect(test_article.title).to eq("This starts with BIDI override") + end + + it "rejects empty titles after sanitizing" do + test_article = build(:article, title: "\u202a\u202b\u202c\u202d\u202e") + + test_article.validate + + expect(test_article).not_to be_valid + expect(test_article.errors_as_sentence).to match("Title can't be blank") + end + end + describe "tag validation" do let(:article) { build(:article, user: user) } diff --git a/spec/models/tag_spec.rb b/spec/models/tag_spec.rb index bcc14a811..81acf6ad0 100644 --- a/spec/models/tag_spec.rb +++ b/spec/models/tag_spec.rb @@ -86,6 +86,11 @@ RSpec.describe Tag, type: :model do end end + it "fails validation if name is a prohibited (whitespace) unicode character" do + tag.name = "U+202D" + expect(tag).not_to be_valid + end + describe "alias_for" do it "passes validation if the alias refers to an existing tag" do tag = create(:tag)