From f56c44d1c7273025d38b05b2e5ee5c54e8545ac9 Mon Sep 17 00:00:00 2001 From: Rafael dos Santos Silva Date: Mon, 27 Jun 2022 16:16:33 -0300 Subject: [PATCH] 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 | 9 +++++++++ config/locales/server.en.yml | 1 + spec/lib/post_creator_spec.rb | 10 +++++++--- spec/models/watched_word_spec.rb | 6 ++++++ spec/requests/admin/watched_words_controller_spec.rb | 6 ++++++ 5 files changed, 29 insertions(+), 3 deletions(-) diff --git a/app/models/watched_word.rb b/app/models/watched_word.rb index f08f523b1c8..fafbdd388a6 100644 --- a/app/models/watched_word.rb +++ b/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) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 31fd766d6e1..d1356341edd 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb index 1c929c26095..237c91b3eff 100644 --- a/spec/lib/post_creator_spec.rb +++ b/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']) diff --git a/spec/models/watched_word_spec.rb b/spec/models/watched_word_spec.rb index 60ed4e1c66b..f43ec6a774f 100644 --- a/spec/models/watched_word_spec.rb +++ b/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/") diff --git a/spec/requests/admin/watched_words_controller_spec.rb b/spec/requests/admin/watched_words_controller_spec.rb index 5d1c6b3e74a..09930880234 100644 --- a/spec/requests/admin/watched_words_controller_spec.rb +++ b/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