From 19f909855190cda30119b8f150c5db85c1579146 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 22 May 2023 13:15:21 +0200 Subject: [PATCH] Allow reports with long comments from remote instances, but truncate (#25028) --- app/lib/activitypub/activity/flag.rb | 6 ++++- app/lib/activitypub/tag_manager.rb | 4 +++ app/models/report.rb | 9 +++---- spec/lib/activitypub/activity/flag_spec.rb | 31 ++++++++++++++++++++++ spec/models/report_spec.rb | 11 ++++++-- spec/services/report_service_spec.rb | 12 ++++++--- 6 files changed, 62 insertions(+), 11 deletions(-) diff --git a/app/lib/activitypub/activity/flag.rb b/app/lib/activitypub/activity/flag.rb index dc808ad36..dc1932f59 100644 --- a/app/lib/activitypub/activity/flag.rb +++ b/app/lib/activitypub/activity/flag.rb @@ -16,7 +16,7 @@ class ActivityPub::Activity::Flag < ActivityPub::Activity @account, target_account, status_ids: target_statuses.nil? ? [] : target_statuses.map(&:id), - comment: @json['content'] || '', + comment: report_comment, uri: report_uri ) end @@ -35,4 +35,8 @@ class ActivityPub::Activity::Flag < ActivityPub::Activity def report_uri @json['id'] unless @json['id'].nil? || non_matching_uri_hosts?(@account.uri, @json['id']) end + + def report_comment + (@json['content'] || '')[0...5000] + end end diff --git a/app/lib/activitypub/tag_manager.rb b/app/lib/activitypub/tag_manager.rb index a65a9565a..864328631 100644 --- a/app/lib/activitypub/tag_manager.rb +++ b/app/lib/activitypub/tag_manager.rb @@ -28,6 +28,8 @@ class ActivityPub::TagManager return activity_account_status_url(target.account, target) if target.reblog? short_account_status_url(target.account, target) + when :flag + target.uri end end @@ -43,6 +45,8 @@ class ActivityPub::TagManager account_status_url(target.account, target) when :emoji emoji_url(target) + when :flag + target.uri end end diff --git a/app/models/report.rb b/app/models/report.rb index c3a0c4c8b..e738281ad 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -40,7 +40,10 @@ class Report < ApplicationRecord scope :resolved, -> { where.not(action_taken_at: nil) } scope :with_accounts, -> { includes([:account, :target_account, :action_taken_by_account, :assigned_account].index_with({ user: [:invite_request, :invite] })) } - validates :comment, length: { maximum: 1_000 } + # A report is considered local if the reporter is local + delegate :local?, to: :account + + validates :comment, length: { maximum: 1_000 }, if: :local? validates :rule_ids, absence: true, unless: :violation? validate :validate_rule_ids @@ -51,10 +54,6 @@ class Report < ApplicationRecord violation: 2_000, } - def local? - false # Force uri_for to use uri attribute - end - before_validation :set_uri, only: :create after_create_commit :trigger_webhooks diff --git a/spec/lib/activitypub/activity/flag_spec.rb b/spec/lib/activitypub/activity/flag_spec.rb index 005e185e6..601473069 100644 --- a/spec/lib/activitypub/activity/flag_spec.rb +++ b/spec/lib/activitypub/activity/flag_spec.rb @@ -39,6 +39,37 @@ RSpec.describe ActivityPub::Activity::Flag do end end + context 'when the report comment is excessively long' do + subject do + described_class.new({ + '@context': 'https://www.w3.org/ns/activitystreams', + id: flag_id, + type: 'Flag', + content: long_comment, + actor: ActivityPub::TagManager.instance.uri_for(sender), + object: [ + ActivityPub::TagManager.instance.uri_for(flagged), + ActivityPub::TagManager.instance.uri_for(status), + ], + }.with_indifferent_access, sender) + end + + let(:long_comment) { Faker::Lorem.characters(number: 6000) } + + before do + subject.perform + end + + it 'creates a report but with a truncated comment' do + report = Report.find_by(account: sender, target_account: flagged) + + expect(report).to_not be_nil + expect(report.comment.length).to eq 5000 + expect(report.comment).to eq long_comment[0...5000] + expect(report.status_ids).to eq [status.id] + end + end + 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) } diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index b006f60bb..0093dcd8d 100644 --- a/spec/models/report_spec.rb +++ b/spec/models/report_spec.rb @@ -121,10 +121,17 @@ describe Report do end describe 'validations' do - it 'is invalid if comment is longer than 1000 characters' do + let(:remote_account) { Fabricate(:account, domain: 'example.com', protocol: :activitypub, inbox_url: 'http://example.com/inbox') } + + it 'is invalid if comment is longer than 1000 characters only if reporter is local' do report = Fabricate.build(:report, comment: Faker::Lorem.characters(number: 1001)) - report.valid? + expect(report.valid?).to be false expect(report).to model_have_error_on_field(:comment) end + + it 'is valid if comment is longer than 1000 characters and reporter is not local' do + report = Fabricate.build(:report, account: remote_account, comment: Faker::Lorem.characters(number: 1001)) + expect(report.valid?).to be true + end end end diff --git a/spec/services/report_service_spec.rb b/spec/services/report_service_spec.rb index 29207462a..b8ceedb85 100644 --- a/spec/services/report_service_spec.rb +++ b/spec/services/report_service_spec.rb @@ -6,6 +6,14 @@ RSpec.describe ReportService, type: :service do subject { described_class.new } let(:source_account) { Fabricate(:account) } + let(:target_account) { Fabricate(:account) } + + context 'with a local account' do + it 'has a uri' do + report = subject.call(source_account, target_account) + expect(report.uri).to_not be_nil + end + end context 'with a remote account' do let(:remote_account) { Fabricate(:account, domain: 'example.com', protocol: :activitypub, inbox_url: 'http://example.com/inbox') } @@ -35,7 +43,6 @@ RSpec.describe ReportService, type: :service do -> { described_class.new.call(source_account, target_account, status_ids: [status.id]) } end - let(:target_account) { Fabricate(:account) } let(:status) { Fabricate(:status, account: target_account, visibility: :direct) } context 'when it is addressed to the reporter' do @@ -91,8 +98,7 @@ RSpec.describe ReportService, type: :service do -> { described_class.new.call(source_account, target_account) } end - let!(:target_account) { Fabricate(:account) } - let!(:other_report) { Fabricate(:report, target_account: target_account) } + let!(:other_report) { Fabricate(:report, target_account: target_account) } before do ActionMailer::Base.deliveries.clear