From 1b4054256f9d3302b44f71627a23bb0902578867 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 4 Jul 2022 11:08:30 +0200 Subject: [PATCH] Fix crash when a remote Flag activity mentions a private post (#18760) * Add tests * Fix crash when a remote Flag activity mentions a private post --- app/services/report_service.rb | 11 ++- spec/lib/activitypub/activity/flag_spec.rb | 88 ++++++++++++++++++++-- spec/services/report_service_spec.rb | 25 ++++++ 3 files changed, 115 insertions(+), 9 deletions(-) diff --git a/app/services/report_service.rb b/app/services/report_service.rb index 70212a6a7..bd67ff8d3 100644 --- a/app/services/report_service.rb +++ b/app/services/report_service.rb @@ -57,7 +57,16 @@ class ReportService < BaseService end def reported_status_ids - AccountStatusesFilter.new(@target_account, @source_account).results.with_discarded.find(Array(@status_ids)).pluck(:id) + return AccountStatusesFilter.new(@target_account, @source_account).results.with_discarded.find(Array(@status_ids)).pluck(:id) if @source_account.local? + + # If the account making reports is remote, it is likely anonymized so we have to relax the requirements for attaching statuses. + domain = @source_account.domain.to_s.downcase + has_followers = @target_account.followers.where(Account.arel_table[:domain].lower.eq(domain)).exists? + visibility = has_followers ? %i(public unlisted private) : %i(public unlisted) + scope = @target_account.statuses.with_discarded + scope.merge!(scope.where(visibility: visibility).or(scope.where('EXISTS (SELECT 1 FROM mentions m JOIN accounts a ON m.account_id = a.id WHERE lower(a.domain) = ?)', domain))) + # Allow missing posts to not drop reports that include e.g. a deleted post + scope.where(id: Array(@status_ids)).pluck(:id) end def payload diff --git a/spec/lib/activitypub/activity/flag_spec.rb b/spec/lib/activitypub/activity/flag_spec.rb index ec7359f2f..2f2d13876 100644 --- a/spec/lib/activitypub/activity/flag_spec.rb +++ b/spec/lib/activitypub/activity/flag_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Flag do - let(:sender) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/account') } + let(:sender) { Fabricate(:account, username: 'example.com', domain: 'example.com', uri: 'http://example.com/actor') } let(:flagged) { Fabricate(:account) } let(:status) { Fabricate(:status, account: flagged, uri: 'foobar') } let(:flag_id) { nil } @@ -23,16 +23,88 @@ RSpec.describe ActivityPub::Activity::Flag do describe '#perform' do subject { described_class.new(json, sender) } - before do - subject.perform + context 'when the reported status is public' do + before do + subject.perform + end + + it 'creates a report' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [status.id] + end end - it 'creates a report' do - report = Report.find_by(account: sender, target_account: flagged) + context 'when the reported status is private and should not be visible to the remote server' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } - expect(report).to_not be_nil - expect(report.comment).to eq 'Boo!!' - expect(report.status_ids).to eq [status.id] + before do + subject.perform + end + + it 'creates a report with no attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [] + end + end + + context 'when the reported status is private and the author has a follower on the remote instance' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } + let(:follower) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/users/account') } + + before do + follower.follow!(flagged) + subject.perform + end + + it 'creates a report with the attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [status.id] + end + end + + context 'when the reported status is private and the author mentions someone else on the remote instance' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } + let(:mentioned) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/users/account') } + + before do + status.mentions.create(account: mentioned) + subject.perform + end + + it 'creates a report with the attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [status.id] + end + end + + context 'when the reported status is private and the author mentions someone else on the local instance' do + let(:status) { Fabricate(:status, account: flagged, uri: 'foobar', visibility: :private) } + let(:mentioned) { Fabricate(:account) } + + before do + status.mentions.create(account: mentioned) + subject.perform + end + + it 'creates a report with no attached status' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment).to eq 'Boo!!' + expect(report.status_ids).to eq [] + end end end diff --git a/spec/services/report_service_spec.rb b/spec/services/report_service_spec.rb index 7e6a113e0..ea68b3344 100644 --- a/spec/services/report_service_spec.rb +++ b/spec/services/report_service_spec.rb @@ -28,6 +28,31 @@ RSpec.describe ReportService, type: :service do end end + context 'when the reported status is a DM' do + let(:target_account) { Fabricate(:account) } + let(:status) { Fabricate(:status, account: target_account, visibility: :direct) } + + subject do + -> { described_class.new.call(source_account, target_account, status_ids: [status.id]) } + end + + context 'when it is addressed to the reporter' do + before do + status.mentions.create(account: source_account) + end + + it 'creates a report' do + is_expected.to change { target_account.targeted_reports.count }.from(0).to(1) + end + end + + context 'when it is not addressed to the reporter' do + it 'errors out' do + is_expected.to raise_error + end + end + end + context 'when other reports already exist for the same target' do let!(:target_account) { Fabricate(:account) } let!(:other_report) { Fabricate(:report, target_account: target_account) }