Add API parameter to safeguard unexpect mentions in new posts (#18350)

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") }