FEATURE: Validate tags in WatchedWords (#17254)

* FEATURE: Validate tags in WatchedWords

We didn't validate watched words automatic tagging, so it was possible
for an admin to created watched words with an empty tag list which would
result in an exception when users tried to create a new topic that
matched the misconfigured watched word.

Bug report: https://meta.discourse.org/t/lib-topic-creator-fails-when-the-word-math-appears-in-the-topic-title-or-text/231018?u=falco

app/models/watched_word.rb

@@ -28,6 +28,7 @@ class WatchedWord < ActiveRecord::Base
   validates :action, presence: true
 
   validate :replacement_is_url, if: -> { action == WatchedWord.actions[:link] }
+  validate :replacement_is_tag_list, if: -> { action == WatchedWord.actions[:tag] }
 
   validates_each :word do |record, attr, val|
     if WatchedWord.where(action: record.action).count >= MAX_WORDS_PER_ACTION
@@ -50,6 +51,14 @@ class WatchedWord < ActiveRecord::Base
     end
   end
 
+  def replacement_is_tag_list
+    tag_list = replacement&.split(',')
+    tags = Tag.where(name: tag_list)
+    if (tag_list.blank? || tags.empty? || tag_list.size != tags.size)
+      errors.add(:base, :invalid_tag_list)
+    end
+  end
+
   def self.create_or_update_word(params)
     new_word = normalize_word(params[:word])
     w = WatchedWord.where("word ILIKE ?", new_word).first || WatchedWord.new(word: new_word)

config/locales/server.en.yml

@@ -639,6 +639,7 @@ en:
               too_many: "Too many words for that action"
             base:
               invalid_url: "Replacement URL is invalid"
+              invalid_tag_list: "Replacement tag list is invalid"
 
   uncategorized_category_name: "Uncategorized"
 

spec/lib/post_creator_spec.rb

@@ -529,11 +529,15 @@ describe PostCreator do
             before do
               SiteSetting.min_trust_to_create_tag = 0
               SiteSetting.min_trust_level_to_tag_topics = 0
+              Fabricate(:tag, name: 'greetings')
+              Fabricate(:tag, name: 'hey')
+              Fabricate(:tag, name: 'about-art')
+              Fabricate(:tag, name: 'about-artists')
             end
 
             context "without regular expressions" do
               it "works with many tags" do
-                Fabricate(:watched_word, action: WatchedWord.actions[:tag], word: "HELLO", replacement: "greetings , hey")
+                Fabricate(:watched_word, action: WatchedWord.actions[:tag], word: "HELLO", replacement: "greetings,hey")
 
                 @post = creator.create
                 expect(@post.topic.tags.map(&:name)).to match_array(['greetings', 'hey'])
@@ -548,7 +552,7 @@ describe PostCreator do
               end
 
               it "does not treat as regular expressions" do
-                Fabricate(:watched_word, action: WatchedWord.actions[:tag], word: "he(llo|y)", replacement: "greetings , hey")
+                Fabricate(:watched_word, action: WatchedWord.actions[:tag], word: "he(llo|y)", replacement: "greetings,hey")
 
                 @post = creator_with_tags.create
                 expect(@post.topic.tags.map(&:name)).to match_array(tag_names)
@@ -558,7 +562,7 @@ describe PostCreator do
             context "with regular expressions" do
               it "works" do
                 SiteSetting.watched_words_regular_expressions = true
-                Fabricate(:watched_word, action: WatchedWord.actions[:tag], word: "he(llo|y)", replacement: "greetings , hey")
+                Fabricate(:watched_word, action: WatchedWord.actions[:tag], word: "he(llo|y)", replacement: "greetings,hey")
 
                 @post = creator_with_tags.create
                 expect(@post.topic.tags.map(&:name)).to match_array(tag_names + ['greetings', 'hey'])

spec/models/watched_word_spec.rb

@@ -89,6 +89,12 @@ describe WatchedWord do
       }.to_not change { described_class.count }
     end
 
+    it "error when an tag action is created without valid tags" do
+      expect {
+        described_class.create!(word: "ramones", action: described_class.actions[:tag])
+      }.to raise_error(ActiveRecord::RecordInvalid)
+    end
+
     it "replaces link with absolute URL" do
       word = Fabricate(:watched_word, action: described_class.actions[:link], word: "meta1")
       expect(word.replacement).to eq("http://test.localhost/")

spec/requests/admin/watched_words_controller_spec.rb

@@ -30,6 +30,9 @@ RSpec.describe Admin::WatchedWordsController do
     context 'logged in as admin' do
       before do
         sign_in(admin)
+        Fabricate(:tag, name: 'tag1')
+        Fabricate(:tag, name: 'tag2')
+        Fabricate(:tag, name: 'tag3')
       end
 
       it 'creates the words from the file' do
@@ -80,6 +83,9 @@ RSpec.describe Admin::WatchedWordsController do
     context 'logged in as admin' do
       before do
         sign_in(admin)
+        Fabricate(:tag, name: 'tag1')
+        Fabricate(:tag, name: 'tag2')
+        Fabricate(:tag, name: 'tag3')
       end
 
       it "words of different actions are downloaded separately" do