From d6930b3847405dc9f8c1a54fb74d488a3c9a775e Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 13 Feb 2023 16:36:29 +0100 Subject: [PATCH] Add API parameter to safeguard unexpect mentions in new posts (#18350) --- app/controllers/api/v1/statuses_controller.rb | 8 ++++++ app/services/post_status_service.rb | 28 +++++++++++++++++-- app/services/process_mentions_service.rb | 19 +++++++------ .../api/v1/statuses_controller_spec.rb | 17 +++++++++++ spec/services/post_status_service_spec.rb | 21 +++++++++++++- .../services/process_mentions_service_spec.rb | 13 +++++++++ 6 files changed, 94 insertions(+), 12 deletions(-) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 9a8c0c161..fadd1b045 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -63,11 +63,18 @@ class Api::V1::StatusesController < Api::BaseController scheduled_at: status_params[:scheduled_at], application: doorkeeper_token.application, poll: status_params[:poll], + allowed_mentions: status_params[:allowed_mentions], idempotency: request.headers['Idempotency-Key'], with_rate_limit: true ) render json: @status, serializer: @status.is_a?(ScheduledStatus) ? REST::ScheduledStatusSerializer : REST::StatusSerializer + rescue PostStatusService::UnexpectedMentionsError => e + unexpected_accounts = ActiveModel::Serializer::CollectionSerializer.new( + e.accounts, + serializer: REST::AccountSerializer + ) + render json: { error: e.message, unexpected_accounts: unexpected_accounts }, status: 422 end def update @@ -128,6 +135,7 @@ class Api::V1::StatusesController < Api::BaseController :visibility, :language, :scheduled_at, + allowed_mentions: [], media_ids: [], media_attributes: [ :id, diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index bd3b69632..258af8827 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -6,6 +6,15 @@ class PostStatusService < BaseService MIN_SCHEDULE_OFFSET = 5.minutes.freeze + class UnexpectedMentionsError < StandardError + attr_reader :accounts + + def initialize(message, accounts) + super(message) + @accounts = accounts + end + end + # Post a text status update, fetch and notify remote users mentioned # @param [Account] account Account from which to post # @param [Hash] options @@ -21,6 +30,7 @@ class PostStatusService < BaseService # @option [Doorkeeper::Application] :application # @option [String] :idempotency Optional idempotency key # @option [Boolean] :with_rate_limit + # @option [Enumerable] :allowed_mentions Optional array of expected mentioned account IDs, raises `UnexpectedMentionsError` if unexpected accounts end up in mentions # @return [Status] def call(account, options = {}) @account = account @@ -63,14 +73,27 @@ class PostStatusService < BaseService end def process_status! + @status = @account.statuses.new(status_attributes) + process_mentions_service.call(@status, save_records: false) + safeguard_mentions!(@status) + # The following transaction block is needed to wrap the UPDATEs to # the media attachments when the status is created - ApplicationRecord.transaction do - @status = @account.statuses.create!(status_attributes) + @status.save! end end + def safeguard_mentions!(status) + return if @options[:allowed_mentions].nil? + expected_account_ids = @options[:allowed_mentions].map(&:to_i) + + unexpected_accounts = status.mentions.map(&:account).to_a.reject { |mentioned_account| expected_account_ids.include?(mentioned_account.id) } + return if unexpected_accounts.empty? + + raise UnexpectedMentionsError.new('Post would be sent to unexpected accounts', unexpected_accounts) + end + def schedule_status! status_for_validation = @account.statuses.build(status_attributes) @@ -93,7 +116,6 @@ class PostStatusService < BaseService def postprocess_status! process_hashtags_service.call(@status) - process_mentions_service.call(@status) Trends.tags.register(@status) LinkCrawlWorker.perform_async(@status.id) DistributionWorker.perform_async(@status.id) diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb index b117db8c2..93a96667e 100644 --- a/app/services/process_mentions_service.rb +++ b/app/services/process_mentions_service.rb @@ -3,12 +3,13 @@ class ProcessMentionsService < BaseService include Payloadable - # Scan status for mentions and fetch remote mentioned users, create - # local mention pointers, send Salmon notifications to mentioned - # remote users + # Scan status for mentions and fetch remote mentioned users, + # and create local mention pointers # @param [Status] status - def call(status) + # @param [Boolean] save_records Whether to save records in database + def call(status, save_records: true) @status = status + @save_records = save_records return unless @status.local? @@ -55,14 +56,15 @@ class ProcessMentionsService < BaseService next match if mention_undeliverable?(mentioned_account) || mentioned_account&.suspended? mention = @previous_mentions.find { |x| x.account_id == mentioned_account.id } - mention ||= mentioned_account.mentions.new(status: @status) + mention ||= @current_mentions.find { |x| x.account_id == mentioned_account.id } + mention ||= @status.mentions.new(account: mentioned_account) @current_mentions << mention "@#{mentioned_account.acct}" end - @status.save! + @status.save! if @save_records end def assign_mentions! @@ -73,11 +75,12 @@ class ProcessMentionsService < BaseService mentioned_account_ids = @current_mentions.map(&:account_id) blocked_account_ids = Set.new(@status.account.block_relationships.where(target_account_id: mentioned_account_ids).pluck(:target_account_id)) - @current_mentions.select! { |mention| !(blocked_account_ids.include?(mention.account_id) || blocked_domains.include?(mention.account.domain)) } + dropped_mentions, @current_mentions = @current_mentions.partition { |mention| blocked_account_ids.include?(mention.account_id) || blocked_domains.include?(mention.account.domain) } + dropped_mentions.each(&:destroy) end @current_mentions.each do |mention| - mention.save if mention.new_record? + mention.save if mention.new_record? && @save_records end # If previous mentions are no longer contained in the text, convert them diff --git a/spec/controllers/api/v1/statuses_controller_spec.rb b/spec/controllers/api/v1/statuses_controller_spec.rb index 24810a5d2..bd8b8013a 100644 --- a/spec/controllers/api/v1/statuses_controller_spec.rb +++ b/spec/controllers/api/v1/statuses_controller_spec.rb @@ -133,6 +133,23 @@ RSpec.describe Api::V1::StatusesController, type: :controller do end end + context 'with a safeguard' do + let!(:alice) { Fabricate(:account, username: 'alice') } + let!(:bob) { Fabricate(:account, username: 'bob') } + + before do + post :create, params: { status: '@alice hm, @bob is really annoying lately', allowed_mentions: [alice.id] } + end + + it 'returns http unprocessable entity' do + expect(response).to have_http_status(422) + end + + it 'returns serialized extra accounts in body' do + expect(body_as_json[:unexpected_accounts].map { |a| a.slice(:id, :acct) }).to eq [{ id: bob.id.to_s, acct: bob.acct }] + end + end + context 'with missing parameters' do before do post :create, params: {} diff --git a/spec/services/post_status_service_spec.rb b/spec/services/post_status_service_spec.rb index d21270c79..28f20e9c7 100644 --- a/spec/services/post_status_service_spec.rb +++ b/spec/services/post_status_service_spec.rb @@ -138,7 +138,26 @@ RSpec.describe PostStatusService, type: :service do status = subject.call(account, text: "test status update") expect(ProcessMentionsService).to have_received(:new) - expect(mention_service).to have_received(:call).with(status) + expect(mention_service).to have_received(:call).with(status, save_records: false) + end + + it 'safeguards mentions' do + account = Fabricate(:account) + mentioned_account = Fabricate(:account, username: 'alice') + unexpected_mentioned_account = Fabricate(:account, username: 'bob') + + expect do + subject.call(account, text: '@alice hm, @bob is really annoying lately', allowed_mentions: [mentioned_account.id]) + end.to raise_error(an_instance_of(PostStatusService::UnexpectedMentionsError).and having_attributes(accounts: [unexpected_mentioned_account])) + end + + it 'processes duplicate mentions correctly' do + account = Fabricate(:account) + mentioned_account = Fabricate(:account, username: 'alice') + + expect do + subject.call(account, text: '@alice @alice @alice hey @alice') + end.not_to raise_error end it 'processes hashtags' do diff --git a/spec/services/process_mentions_service_spec.rb b/spec/services/process_mentions_service_spec.rb index 5b9d17a4c..0dd62c807 100644 --- a/spec/services/process_mentions_service_spec.rb +++ b/spec/services/process_mentions_service_spec.rb @@ -47,6 +47,19 @@ RSpec.describe ProcessMentionsService, type: :service do end end + context 'mentioning a user several times when not saving records' do + let!(:remote_user) { Fabricate(:account, username: 'remote_user', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + let(:status) { Fabricate(:status, account: account, text: "Hello @#{remote_user.acct} @#{remote_user.acct} @#{remote_user.acct}", visibility: :public) } + + before do + subject.call(status, save_records: false) + end + + it 'creates exactly one mention' do + expect(status.mentions.size).to eq 1 + end + end + context 'with an IDN domain' do let!(:remote_user) { Fabricate(:account, username: 'sneak', protocol: :activitypub, domain: 'xn--hresiar-mxa.ch', inbox_url: 'http://example.com/inbox') } let!(:status) { Fabricate(:status, account: account, text: "Hello @sneak@hæresiar.ch") }