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

This commit is contained in:
Claire 2023-02-13 16:36:29 +01:00 committed by GitHub
parent c84f38abc4
commit d6930b3847
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 94 additions and 12 deletions

View file

@ -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,

View file

@ -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)

View file

@ -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

View file

@ -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: {}

View file

@ -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

View file

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