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 <bendhalpern@gmail.com>
Co-authored-by: Dan Uber <dan@forem.com>

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

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

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

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

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) }
 

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)