From ba148e082df2dfecafa6ee32bf4a64dcf80affc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Thu, 28 Apr 2022 11:00:02 +0200 Subject: [PATCH] FIX: Apply watched words to user fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we don’t apply watched words to custom user fields nor user profile fields. This led to users being able to use blocked words in their bio, location or some custom user fields. This patch addresses this issue by adding some validations so it’s not possible anymore to save the User model or the UserProfile model if they contain blocked words. --- app/models/user.rb | 5 +++ app/models/user_profile.rb | 4 +- spec/jobs/activation_reminder_emails_spec.rb | 7 +--- spec/jobs/bulk_invite_spec.rb | 2 +- spec/models/user_profile_spec.rb | 43 ++++++++++++++++++-- spec/models/user_spec.rb | 27 +++++++++++- spec/services/user_anonymizer_spec.rb | 4 +- 7 files changed, 78 insertions(+), 14 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 2ff2618e3e..9a171a67ff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -114,6 +114,7 @@ class User < ActiveRecord::Base validates :name, user_full_name: true, if: :will_save_change_to_name?, length: { maximum: 255 } validates :ip_address, allowed_ip_address: { on: :create, message: :signup_not_allowed } validates :primary_email, presence: true + validates :custom_fields_values, watched_words: true validates_associated :primary_email, message: -> (_, user_email) { user_email[:value]&.errors[:email]&.first } after_initialize :add_trust_level @@ -1791,6 +1792,10 @@ class User < ActiveRecord::Base ) SQL end + + def custom_fields_values + custom_fields.values.join(" ") + end end # == Schema Information diff --git a/app/models/user_profile.rb b/app/models/user_profile.rb index 4b2f4f62b6..66186468d7 100644 --- a/app/models/user_profile.rb +++ b/app/models/user_profile.rb @@ -7,9 +7,11 @@ class UserProfile < ActiveRecord::Base belongs_to :granted_title_badge, class_name: "Badge" belongs_to :featured_topic, class_name: 'Topic' - validates :bio_raw, length: { maximum: 3000 } + validates :bio_raw, length: { maximum: 3000 }, watched_words: true validates :website, url: true, allow_blank: true, if: Proc.new { |c| c.new_record? || c.website_changed? } validates :user, presence: true + validates :location, watched_words: true + before_save :cook after_save :trigger_badges after_save :pull_hotlinked_image diff --git a/spec/jobs/activation_reminder_emails_spec.rb b/spec/jobs/activation_reminder_emails_spec.rb index 3179c8bc11..4b451c2821 100644 --- a/spec/jobs/activation_reminder_emails_spec.rb +++ b/spec/jobs/activation_reminder_emails_spec.rb @@ -12,12 +12,9 @@ describe Jobs::ActivationReminderEmails do expect { described_class.new.execute({}) } .to change { ActionMailer::Base.deliveries.size }.by(1) .and change { user.email_tokens.count }.by(1) - - expect(user.custom_fields['activation_reminder']).to eq("t") + .and change { user.reload.custom_fields[:activation_reminder] }.to "t" expect { described_class.new.execute({}) }.to change { ActionMailer::Base.deliveries.size }.by(0) - - user.activate - expect(user.reload.custom_fields['activation_reminder']).to eq(nil) + expect { user.activate }.to change { user.reload.custom_fields[:activation_reminder] }.to nil end it 'should not email active users' do diff --git a/spec/jobs/bulk_invite_spec.rb b/spec/jobs/bulk_invite_spec.rb index 66c1cf0377..dd983a9e24 100644 --- a/spec/jobs/bulk_invite_spec.rb +++ b/spec/jobs/bulk_invite_spec.rb @@ -100,7 +100,7 @@ describe Jobs::BulkInvite do expect(User.where(staged: true).find_by_email('test@discourse.org')).to eq(nil) expect(user.user_fields[user_field.id.to_s]).to eq('value 1') expect(user.user_fields[user_field_color.id.to_s]).to eq('Blue') - expect(staged_user.user_fields[user_field.id.to_s]).to eq('value 2') + expect(staged_user.reload.user_fields[user_field.id.to_s]).to eq('value 2') expect(staged_user.user_fields[user_field_color.id.to_s]).to eq(nil) new_staged_user = User.where(staged: true).find_by_email('test2@discourse.org') expect(new_staged_user.user_fields[user_field.id.to_s]).to eq('value 3') diff --git a/spec/models/user_profile_spec.rb b/spec/models/user_profile_spec.rb index f45fa76651..b88aefacd9 100644 --- a/spec/models/user_profile_spec.rb +++ b/spec/models/user_profile_spec.rb @@ -1,6 +1,45 @@ # frozen_string_literal: true -describe UserProfile do +RSpec.describe UserProfile do + describe "Validations" do + subject(:profile) { user.user_profile } + + fab!(:user) { Fabricate(:user) } + fab!(:watched_word) { Fabricate(:watched_word, word: "bad") } + + describe "#location" do + context "when it contains watched words" do + before { profile.location = "bad location" } + + it "is not valid" do + profile.valid? + expect(profile.errors[:base].size).to eq(1) + expect(profile.errors.messages[:base]).to include(/you can't post the word/) + end + end + + context "when it does not contain watched words" do + it { is_expected.to be_valid } + end + end + + describe "#bio_raw" do + context "when it contains watched words" do + before { profile.bio_raw = "bad bio" } + + it "is not valid" do + profile.valid? + expect(profile.errors[:base].size).to eq(1) + expect(profile.errors.messages[:base]).to include(/you can't post the word/) + end + end + + context "when it does not contain watched words" do + it { is_expected.to be_valid } + end + end + end + it 'is created automatically when a user is created' do user = Fabricate(:evil_trout) expect(user.user_profile).to be_present @@ -224,7 +263,5 @@ describe UserProfile do expect(user.card_background_upload).to eq(nil) end end - end - end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 73d1b5fc5c..e8824059bc 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1,9 +1,9 @@ # frozen_string_literal: true -describe User do +RSpec.describe User do fab!(:group) { Fabricate(:group) } - let(:user) { Fabricate(:user, last_seen_at: 1.day.ago) } + subject(:user) { Fabricate(:user, last_seen_at: 1.day.ago) } def user_error_message(*keys) I18n.t(:"activerecord.errors.models.user.attributes.#{keys.join('.')}") @@ -136,6 +136,29 @@ describe User do end end end + + describe "#user_fields" do + fab!(:user_field) { Fabricate(:user_field) } + fab!(:watched_word) { Fabricate(:watched_word, word: "bad") } + + before { user.set_user_field(user_field.id, value) } + + context "when user fields contain watched words" do + let(:value) { "bad user field value" } + + it "is not valid" do + user.valid? + expect(user.errors[:base].size).to eq(1) + expect(user.errors.messages[:base]).to include(/you can't post the word/) + end + end + + context "when user fields do not contain watched words" do + let(:value) { "good user field value" } + + it { is_expected.to be_valid } + end + end end describe '#count_by_signup_date' do diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 0f7ccc2b74..9156b59fa2 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -289,9 +289,9 @@ describe UserAnonymizer do "user_field_#{field2.id}": "bar", "another_field": "456" } + user.save - expect { make_anonymous }.to change { user.custom_fields } - expect(user.reload.custom_fields).to eq("some_field" => "123", "another_field" => "456") + expect { make_anonymous }.to change { user.reload.custom_fields }.to "some_field" => "123", "another_field" => "456" end end end