From 5f13ca5e54186b973b60efeecbd9518792fea003 Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Tue, 26 Jul 2022 18:15:42 +0300 Subject: [PATCH] FIX: Don't cook user fields to apply watched words (#17590) The previous method for reused the PrettyText logic which applied the watched word logic, but had the unwanted effect of cooking the text too. This meant that regular text values were converted to HTML. Follow up to commit 5a4c35f62714d2d72bc0ee57a10e08116bdc476a. --- app/models/user.rb | 2 +- app/models/user_profile.rb | 2 +- app/services/word_watcher.rb | 16 +++- spec/models/user_profile_spec.rb | 5 ++ spec/models/user_spec.rb | 10 +++ spec/services/word_watcher_spec.rb | 117 +++++++++++++++++------------ 6 files changed, 101 insertions(+), 51 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 602249376bf..98d979fbff1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1283,7 +1283,7 @@ class User < ActiveRecord::Base def apply_watched_words validatable_user_fields.each do |id, value| - set_user_field(id, PrettyText.cook(value).gsub(/^

(.*)<\/p>$/, "\\1")) + set_user_field(id, WordWatcher.apply_to_text(value)) end end diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb index bedb61a9aa7..f6b574d1c3b 100644 --- a/app/models/user_profile.rb +++ b/app/models/user_profile.rb @@ -164,7 +164,7 @@ class UserProfile < ActiveRecord::Base end def apply_watched_words - self.location = PrettyText.cook(location).gsub(/^

(.*)<\/p>$/, "\\1") + self.location = WordWatcher.apply_to_text(location) end def website_domain_validator diff --git a/app/services/word_watcher.rb b/app/services/word_watcher.rb index 443533a0c12..c61d0deff68 100644 --- a/app/services/word_watcher.rb +++ b/app/services/word_watcher.rb @@ -99,7 +99,7 @@ class WordWatcher end def self.censor(html) - regexp = WordWatcher.word_matcher_regexp(:censor) + regexp = word_matcher_regexp(:censor) return html if regexp.blank? doc = Nokogiri::HTML5::fragment(html) @@ -110,12 +110,24 @@ class WordWatcher end def self.censor_text(text) - regexp = WordWatcher.word_matcher_regexp(:censor) + regexp = word_matcher_regexp(:censor) return text if regexp.blank? censor_text_with_regexp(text, regexp) end + def self.apply_to_text(text) + if regexp = word_matcher_regexp(:censor) + text = censor_text_with_regexp(text, regexp) + end + + %i[replace link] + .flat_map { |type| word_matcher_regexps(type).to_a } + .reduce(text) do |t, (word_regexp, replacement)| + t.gsub(Regexp.new(word_regexp)) { |match| "#{match[0]}#{replacement}" } + end + end + def self.clear_cache! WatchedWord.actions.each do |a, i| Discourse.cache.delete word_matcher_regexp_key(a) diff --git a/spec/models/user_profile_spec.rb b/spec/models/user_profile_spec.rb index dcbafb5ab8b..c82ab5a7fb1 100644 --- a/spec/models/user_profile_spec.rb +++ b/spec/models/user_profile_spec.rb @@ -45,6 +45,11 @@ RSpec.describe UserProfile do context "when it does not contain watched words" do it { is_expected.to be_valid } end + + it "is not cooked" do + profile.location = "https://discourse.org" + expect { profile.save! }.not_to change { profile.location } + end end describe "#bio_raw" do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b641ade93d8..96e232450ae 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -224,6 +224,16 @@ RSpec.describe User do it { is_expected.to be_valid } end + context "when user fields contain URL" do + let(:value) { "https://discourse.org" } + let(:user_field_value) { user.reload.user_fields[user_field.id.to_s] } + + it "is not cooked" do + user.save! + expect(user_field_value).to eq "https://discourse.org" + end + end + context "with a multiselect user field" do fab!(:user_field) do Fabricate(:user_field, field_type: 'multiselect', show_on_profile: true) do diff --git a/spec/services/word_watcher_spec.rb b/spec/services/word_watcher_spec.rb index baa389af44b..d75871a2cb1 100644 --- a/spec/services/word_watcher_spec.rb +++ b/spec/services/word_watcher_spec.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true describe WordWatcher do - let(:raw) { "Do you like liquorice?\n\nI really like them. One could even say that I am *addicted* to liquorice. And if\nyou can mix it up with some anise, then I'm in heaven ;)" } after do @@ -15,67 +14,69 @@ describe WordWatcher do context 'format of the result regexp' do it "is correct when watched_words_regular_expressions = true" do SiteSetting.watched_words_regular_expressions = true - regexp = WordWatcher.word_matcher_regexp(:block) + regexp = described_class.word_matcher_regexp(:block) expect(regexp.inspect).to eq("/(#{word1})|(#{word2})/i") end it "is correct when watched_words_regular_expressions = false" do SiteSetting.watched_words_regular_expressions = false - regexp = WordWatcher.word_matcher_regexp(:block) + regexp = described_class.word_matcher_regexp(:block) expect(regexp.inspect).to eq("/(?:\\W|^)(#{word1}|#{word2})(?=\\W|$)/i") end end end - describe "word_matches_for_action?" do + describe "#word_matches_for_action?" do it "is falsey when there are no watched words" do - expect(WordWatcher.new(raw).word_matches_for_action?(:require_approval)).to be_falsey + expect(described_class.new(raw).word_matches_for_action?(:require_approval)).to be_falsey end context "with watched words" do fab!(:anise) { Fabricate(:watched_word, word: "anise", action: WatchedWord.actions[:require_approval]) } it "is falsey without a match" do - expect(WordWatcher.new("No liquorice for me, thanks...").word_matches_for_action?(:require_approval)).to be_falsey + expect(described_class.new("No liquorice for me, thanks...").word_matches_for_action?(:require_approval)).to be_falsey end it "is returns matched words if there's a match" do - m = WordWatcher.new(raw).word_matches_for_action?(:require_approval) - expect(m).to be_truthy - expect(m[1]).to eq(anise.word) + matches = described_class.new(raw).word_matches_for_action?(:require_approval) + expect(matches).to be_truthy + expect(matches[1]).to eq(anise.word) end it "finds at start of string" do - m = WordWatcher.new("#{anise.word} is garbage").word_matches_for_action?(:require_approval) - expect(m[1]).to eq(anise.word) + matches = described_class.new("#{anise.word} is garbage").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq(anise.word) end it "finds at end of string" do - m = WordWatcher.new("who likes #{anise.word}").word_matches_for_action?(:require_approval) - expect(m[1]).to eq(anise.word) + matches = described_class.new("who likes #{anise.word}").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq(anise.word) end it "finds non-letters in place of letters" do Fabricate(:watched_word, word: "co(onut", action: WatchedWord.actions[:require_approval]) - m = WordWatcher.new("This co(onut is delicious.").word_matches_for_action?(:require_approval) - expect(m[1]).to eq("co(onut") + + matches = described_class.new("This co(onut is delicious.").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq("co(onut") end it "handles * for wildcards" do Fabricate(:watched_word, word: "a**le*", action: WatchedWord.actions[:require_approval]) - m = WordWatcher.new("I acknowledge you.").word_matches_for_action?(:require_approval) - expect(m[1]).to eq("acknowledge") + + matches = described_class.new("I acknowledge you.").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq("acknowledge") end context "word boundary" do it "handles word boundary" do Fabricate(:watched_word, word: "love", action: WatchedWord.actions[:require_approval]) - expect(WordWatcher.new("I Love, bananas.").word_matches_for_action?(:require_approval)[1]).to eq("Love") - expect(WordWatcher.new("I LOVE; apples.").word_matches_for_action?(:require_approval)[1]).to eq("LOVE") - expect(WordWatcher.new("love: is a thing.").word_matches_for_action?(:require_approval)[1]).to eq("love") - expect(WordWatcher.new("I love. oranges").word_matches_for_action?(:require_approval)[1]).to eq("love") - expect(WordWatcher.new("I :love. pineapples").word_matches_for_action?(:require_approval)[1]).to eq("love") - expect(WordWatcher.new("peace ,love and understanding.").word_matches_for_action?(:require_approval)[1]).to eq("love") + expect(described_class.new("I Love, bananas.").word_matches_for_action?(:require_approval)[1]).to eq("Love") + expect(described_class.new("I LOVE; apples.").word_matches_for_action?(:require_approval)[1]).to eq("LOVE") + expect(described_class.new("love: is a thing.").word_matches_for_action?(:require_approval)[1]).to eq("love") + expect(described_class.new("I love. oranges").word_matches_for_action?(:require_approval)[1]).to eq("love") + expect(described_class.new("I :love. pineapples").word_matches_for_action?(:require_approval)[1]).to eq("love") + expect(described_class.new("peace ,love and understanding.").word_matches_for_action?(:require_approval)[1]).to eq("love") end end @@ -85,9 +86,11 @@ describe WordWatcher do %w{bananas hate hates}.each do |word| Fabricate(:watched_word, word: word, action: WatchedWord.actions[:block]) end - matches = WordWatcher.new("I hate bananas").word_matches_for_action?(:block, all_matches: true) + + matches = described_class.new("I hate bananas").word_matches_for_action?(:block, all_matches: true) expect(matches).to contain_exactly('hate', 'bananas') - matches = WordWatcher.new("She hates bananas too").word_matches_for_action?(:block, all_matches: true) + + matches = described_class.new("She hates bananas too").word_matches_for_action?(:block, all_matches: true) expect(matches).to contain_exactly('hates', 'bananas') end end @@ -101,10 +104,10 @@ describe WordWatcher do Fabricate(:watched_word, word: "(pine)?apples", action: WatchedWord.actions[:block]) Fabricate(:watched_word, word: "((move|store)(d)?)|((watch|listen)(ed|ing)?)", action: WatchedWord.actions[:block]) - matches = WordWatcher.new("pine pineapples apples").word_matches_for_action?(:block, all_matches: true) + matches = described_class.new("pine pineapples apples").word_matches_for_action?(:block, all_matches: true) expect(matches).to contain_exactly('pineapples', 'apples') - matches = WordWatcher.new("go watched watch ed ing move d moveed moved moving").word_matches_for_action?(:block, all_matches: true) + matches = described_class.new("go watched watch ed ing move d moveed moved moving").word_matches_for_action?(:block, all_matches: true) expect(matches).to contain_exactly(*%w{watched watch move moved}) end end @@ -113,20 +116,23 @@ describe WordWatcher do context "emojis" do it "handles emoji" do Fabricate(:watched_word, word: ":joy:", action: WatchedWord.actions[:require_approval]) - m = WordWatcher.new("Lots of emojis here :joy:").word_matches_for_action?(:require_approval) - expect(m[1]).to eq(":joy:") + + matches = described_class.new("Lots of emojis here :joy:").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq(":joy:") end it "handles unicode emoji" do Fabricate(:watched_word, word: "🎃", action: WatchedWord.actions[:require_approval]) - m = WordWatcher.new("Halloween party! 🎃").word_matches_for_action?(:require_approval) - expect(m[1]).to eq("🎃") + + matches = described_class.new("Halloween party! 🎃").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq("🎃") end it "handles emoji skin tone" do Fabricate(:watched_word, word: ":woman:t5:", action: WatchedWord.actions[:require_approval]) - m = WordWatcher.new("To Infinity and beyond! 🚀 :woman:t5:").word_matches_for_action?(:require_approval) - expect(m[1]).to eq(":woman:t5:") + + matches = described_class.new("To Infinity and beyond! 🚀 :woman:t5:").word_matches_for_action?(:require_approval) + expect(matches[1]).to eq(":woman:t5:") end end @@ -141,8 +147,9 @@ describe WordWatcher do word: /\btest\b/, action: WatchedWord.actions[:block] ) - m = WordWatcher.new("this is not a test.").word_matches_for_action?(:block) - expect(m[0]).to eq("test") + + matches = described_class.new("this is not a test.").word_matches_for_action?(:block) + expect(matches[0]).to eq("test") end it "supports regular expressions as a site setting" do @@ -151,12 +158,15 @@ describe WordWatcher do word: /tro[uo]+t/, action: WatchedWord.actions[:require_approval] ) - m = WordWatcher.new("Evil Trout is cool").word_matches_for_action?(:require_approval) - expect(m[0]).to eq("Trout") - m = WordWatcher.new("Evil Troot is cool").word_matches_for_action?(:require_approval) - expect(m[0]).to eq("Troot") - m = WordWatcher.new("trooooooooot").word_matches_for_action?(:require_approval) - expect(m[0]).to eq("trooooooooot") + + matches = described_class.new("Evil Trout is cool").word_matches_for_action?(:require_approval) + expect(matches[0]).to eq("Trout") + + matches = described_class.new("Evil Troot is cool").word_matches_for_action?(:require_approval) + expect(matches[0]).to eq("Troot") + + matches = described_class.new("trooooooooot").word_matches_for_action?(:require_approval) + expect(matches[0]).to eq("trooooooooot") end it "support uppercase" do @@ -166,16 +176,29 @@ describe WordWatcher do action: WatchedWord.actions[:require_approval] ) - m = WordWatcher.new('Amazing place').word_matches_for_action?(:require_approval) - expect(m).to be_nil - m = WordWatcher.new('Amazing applesauce').word_matches_for_action?(:require_approval) - expect(m[0]).to eq('applesauce') - m = WordWatcher.new('Amazing AppleSauce').word_matches_for_action?(:require_approval) - expect(m[0]).to eq('AppleSauce') + matches = described_class.new('Amazing place').word_matches_for_action?(:require_approval) + expect(matches).to be_nil + + matches = described_class.new('Amazing applesauce').word_matches_for_action?(:require_approval) + expect(matches[0]).to eq('applesauce') + + matches = described_class.new('Amazing AppleSauce').word_matches_for_action?(:require_approval) + expect(matches[0]).to eq('AppleSauce') end end end end + describe ".apply_to_text" do + fab!(:censored_word) { Fabricate(:watched_word, word: "censored", action: WatchedWord.actions[:censor]) } + fab!(:replaced_word) { Fabricate(:watched_word, word: "to replace", replacement: "replaced", action: WatchedWord.actions[:replace]) } + fab!(:link_word) { Fabricate(:watched_word, word: "https://notdiscourse.org", replacement: "https://discourse.org", action: WatchedWord.actions[:link]) } + + it "replaces all types of words" do + text = "hello censored world to replace https://notdiscourse.org" + expected = "hello #{described_class::REPLACEMENT_LETTER * 8} world replaced https://discourse.org" + expect(described_class.apply_to_text(text)).to eq(expected) + end + end end