From 7fc884ba006d17e062f4c0f851d0cbd06e1f9289 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 2 Sep 2025 09:29:03 +0200 Subject: [PATCH] Fix banned text being able to be circumvented via unicode (#35978) --- app/lib/antispam.rb | 43 ++++++++++++++++------------- app/services/post_status_service.rb | 10 ++++--- spec/lib/antispam_spec.rb | 26 ++++++++++++++++- 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/app/lib/antispam.rb b/app/lib/antispam.rb index 7d8ed84d0e0..77bc8d003fd 100644 --- a/app/lib/antispam.rb +++ b/app/lib/antispam.rb @@ -32,26 +32,30 @@ class Antispam end end - def local_preflight_check!(status) - return unless considered_spam?(status) + def initialize(status) + @status = status + end - report_if_needed!(status.account) + def local_preflight_check! + return unless considered_spam? - raise SilentlyDrop, status + report_if_needed! + + raise SilentlyDrop, @status end private - def considered_spam?(status) - (all_time_suspicious?(status) || recent_suspicious?(status)) && suspicious_reply_or_mention?(status) + def considered_spam? + (all_time_suspicious? || recent_suspicious?) && suspicious_reply_or_mention? end - def all_time_suspicious?(status) - all_time_spammy_texts.any? { |spammy_text| status.text.include?(spammy_text) } + def all_time_suspicious? + all_time_spammy_texts.any? { |spammy_text| status_text.include?(spammy_text) } end - def recent_suspicious?(status) - status.account.created_at >= ACCOUNT_AGE_EXEMPTION.ago && spammy_texts.any? { |spammy_text| status.text.include?(spammy_text) } + def recent_suspicious? + @status.account.created_at >= ACCOUNT_AGE_EXEMPTION.ago && spammy_texts.any? { |spammy_text| status_text.include?(spammy_text) } end def spammy_texts @@ -62,25 +66,26 @@ class Antispam redis.smembers('antispam:all_time_spammy_texts') end - def suspicious_reply_or_mention?(status) - parent = status.thread - return true if parent.present? && !Follow.exists?(account_id: parent.account_id, target_account: status.account_id) - - account_ids = status.mentions.map(&:account_id).uniq - !Follow.exists?(account_id: account_ids, target_account_id: status.account.id) + def suspicious_reply_or_mention? + account_ids = ([@status.in_reply_to_account_id] + @status.mentions.map(&:account_id)).uniq + !Follow.exists?(account_id: account_ids, target_account_id: @status.account.id) end - def report_if_needed!(account) - return if system_reports.unresolved.exists?(target_account: account) + def report_if_needed! + return if system_reports.unresolved.exists?(target_account: @status.account) system_reports.create!( category: :spam, comment: 'Account automatically reported for posting a banned URL', - target_account: account + target_account: @status.account ) end def system_reports Account.representative.reports end + + def status_text + @status_text ||= @status.text.unicode_normalize(:nfkc).downcase + end end diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 103186b21e7..acf6126ccfe 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -39,8 +39,6 @@ class PostStatusService < BaseService @in_reply_to = @options[:thread] @quoted_status = @options[:quoted_status] - @antispam = Antispam.new - return idempotency_duplicate if idempotency_given? && idempotency_duplicate? validate_media! @@ -82,7 +80,9 @@ class PostStatusService < BaseService process_mentions_service.call(@status, save_records: false) safeguard_mentions!(@status) attach_quote!(@status) - @antispam.local_preflight_check!(@status) + + antispam = Antispam.new(@status) + antispam.local_preflight_check! # The following transaction block is needed to wrap the UPDATEs to # the media attachments when the status is created @@ -113,7 +113,9 @@ class PostStatusService < BaseService def schedule_status! status_for_validation = @account.statuses.build(status_attributes) - @antispam.local_preflight_check!(status_for_validation) + + antispam = Antispam.new(status_for_validation) + antispam.local_preflight_check! if status_for_validation.valid? # Marking the status as destroyed is necessary to prevent the status from being diff --git a/spec/lib/antispam_spec.rb b/spec/lib/antispam_spec.rb index 869fd4c211d..18a12e447f7 100644 --- a/spec/lib/antispam_spec.rb +++ b/spec/lib/antispam_spec.rb @@ -4,7 +4,7 @@ require 'rails_helper' RSpec.describe Antispam do describe '#local_preflight_check!' do - subject { described_class.new.local_preflight_check!(status) } + subject { described_class.new(status).local_preflight_check! } let(:status) { Fabricate :status } @@ -39,6 +39,30 @@ RSpec.describe Antispam do end end + context 'when status matches unicode variants' do + let(:status) { Fabricate :status, text: 'I use https://𝐛𝐚𝐧𝐧𝐞𝐝.𝐞𝐱𝐚𝐦𝐩𝐥𝐞 urls in my text' } + + it 'raises error and reports' do + expect { subject } + .to raise_error(described_class::SilentlyDrop) + .and change(spam_reports, :count).by(1) + end + + context 'when report already exists' do + before { Fabricate :report, account: Account.representative, target_account: status.account } + + it 'raises error and does not report' do + expect { subject } + .to raise_error(described_class::SilentlyDrop) + .and not_change(spam_reports, :count) + end + end + + def spam_reports + Account.representative.reports.where(target_account: status.account).spam + end + end + context 'when status does not match' do it { is_expected.to be_nil } end