Fix spurious edits and require incoming edits to be explicitly marked as such (#17918)

* Change post text edit to not be considered significant if it's identical after reformatting

* We don't need to clear previous change information anymore

* Require status edits to be explicit, except for poll tallies

* Fix tests

* Add some tests

* Add poll-related tests

* Add HTML-formatting related tests
This commit is contained in:
Claire 2022-04-06 21:01:02 +02:00 committed by GitHub
parent 454ef42aab
commit 8f91e304a5
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 220 additions and 12 deletions

View file

@ -17,10 +17,19 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
# Only native types can be updated at the moment # Only native types can be updated at the moment
return @status if !expected_type? || already_updated_more_recently? return @status if !expected_type? || already_updated_more_recently?
last_edit_date = status.edited_at.presence || status.created_at if @status_parser.edited_at.present? && (@status.edited_at.nil? || @status_parser.edited_at > @status.edited_at)
handle_explicit_update!
else
handle_implicit_update!
end
# Since we rely on tracking of previous changes, ensure clean slate @status
status.clear_changes_information end
private
def handle_explicit_update!
last_edit_date = @status.edited_at.presence || @status.created_at
# Only allow processing one create/update per status at a time # Only allow processing one create/update per status at a time
RedisLock.acquire(lock_options) do |lock| RedisLock.acquire(lock_options) do |lock|
@ -45,12 +54,20 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
end end
end end
forward_activity! if significant_changes? && @status_parser.edited_at.present? && @status_parser.edited_at > last_edit_date forward_activity! if significant_changes? && @status_parser.edited_at > last_edit_date
@status
end end
private def handle_implicit_update!
RedisLock.acquire(lock_options) do |lock|
if lock.acquired?
update_poll!(allow_significant_changes: false)
else
raise Mastodon::RaceConditionError
end
end
queue_poll_notifications!
end
def update_media_attachments! def update_media_attachments!
previous_media_attachments = @status.media_attachments.to_a previous_media_attachments = @status.media_attachments.to_a
@ -98,7 +115,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
@media_attachments_changed = true if @status.ordered_media_attachment_ids != previous_media_attachments_ids @media_attachments_changed = true if @status.ordered_media_attachment_ids != previous_media_attachments_ids
end end
def update_poll! def update_poll!(allow_significant_changes: true)
previous_poll = @status.preloadable_poll previous_poll = @status.preloadable_poll
@previous_expires_at = previous_poll&.expires_at @previous_expires_at = previous_poll&.expires_at
poll_parser = ActivityPub::Parser::PollParser.new(@json) poll_parser = ActivityPub::Parser::PollParser.new(@json)
@ -109,6 +126,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
# If for some reasons the options were changed, it invalidates all previous # If for some reasons the options were changed, it invalidates all previous
# votes, so we need to remove them # votes, so we need to remove them
@poll_changed = true if poll_parser.significantly_changes?(poll) @poll_changed = true if poll_parser.significantly_changes?(poll)
return if @poll_changed && !allow_significant_changes
poll.last_fetched_at = Time.now.utc poll.last_fetched_at = Time.now.utc
poll.options = poll_parser.options poll.options = poll_parser.options
@ -121,6 +139,8 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
@status.poll_id = poll.id @status.poll_id = poll.id
elsif previous_poll.present? elsif previous_poll.present?
return unless allow_significant_changes
previous_poll.destroy! previous_poll.destroy!
@poll_changed = true @poll_changed = true
@status.poll_id = nil @status.poll_id = nil
@ -132,7 +152,10 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
@status.spoiler_text = @status_parser.spoiler_text || '' @status.spoiler_text = @status_parser.spoiler_text || ''
@status.sensitive = @account.sensitized? || @status_parser.sensitive || false @status.sensitive = @account.sensitized? || @status_parser.sensitive || false
@status.language = @status_parser.language @status.language = @status_parser.language
@status.edited_at = @status_parser.edited_at || Time.now.utc if significant_changes?
@significant_changes = text_significantly_changed? || @status.spoiler_text_changed? || @media_attachments_changed || @poll_changed
@status.edited_at = @status_parser.edited_at if significant_changes?
@status.save! @status.save!
end end
@ -243,7 +266,14 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
end end
def significant_changes? def significant_changes?
@status.text_changed? || @status.text_previously_changed? || @status.spoiler_text_changed? || @status.spoiler_text_previously_changed? || @media_attachments_changed || @poll_changed @significant_changes
end
def text_significantly_changed?
return false unless @status.text_changed?
old, new = @status.text_change
HtmlAwareFormatter.new(old, false).to_s != HtmlAwareFormatter.new(new, false).to_s
end end
def already_updated_more_recently? def already_updated_more_recently?

View file

@ -195,7 +195,7 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do
let(:existing_status) { Fabricate(:status, account: sender, text: 'Foo', uri: note[:id]) } let(:existing_status) { Fabricate(:status, account: sender, text: 'Foo', uri: note[:id]) }
context 'with a Note object' do context 'with a Note object' do
let(:object) { note } let(:object) { note.merge(updated: '2021-09-08T22:39:25Z') }
it 'updates status' do it 'updates status' do
existing_status.reload existing_status.reload
@ -211,7 +211,7 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do
id: "https://#{valid_domain}/@foo/1234/create", id: "https://#{valid_domain}/@foo/1234/create",
type: 'Create', type: 'Create',
actor: ActivityPub::TagManager.instance.uri_for(sender), actor: ActivityPub::TagManager.instance.uri_for(sender),
object: note, object: note.merge(updated: '2021-09-08T22:39:25Z'),
} }
end end

View file

@ -1,5 +1,9 @@
require 'rails_helper' require 'rails_helper'
def poll_option_json(name, votes)
{ type: 'Note', name: name, replies: { type: 'Collection', totalItems: votes } }
end
RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do
let!(:status) { Fabricate(:status, text: 'Hello world', account: Fabricate(:account, domain: 'example.com')) } let!(:status) { Fabricate(:status, text: 'Hello world', account: Fabricate(:account, domain: 'example.com')) }
@ -46,6 +50,180 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do
expect(status.reload.spoiler_text).to eq 'Show more' expect(status.reload.spoiler_text).to eq 'Show more'
end end
context 'when the changes are only in sanitized-out HTML' do
let!(:status) { Fabricate(:status, text: '<p>Hello world <a href="https://joinmastodon.org" rel="nofollow">joinmastodon.org</a></p>', account: Fabricate(:account, domain: 'example.com')) }
let(:payload) do
{
'@context': 'https://www.w3.org/ns/activitystreams',
id: 'foo',
type: 'Note',
updated: '2021-09-08T22:39:25Z',
content: '<p>Hello world <a href="https://joinmastodon.org" rel="noreferrer">joinmastodon.org</a></p>',
}
end
before do
subject.call(status, json)
end
it 'does not create any edits' do
expect(status.reload.edits).to be_empty
end
it 'does not mark status as edited' do
expect(status.edited?).to be false
end
end
context 'when the status has not been explicitly edited' do
let(:payload) do
{
'@context': 'https://www.w3.org/ns/activitystreams',
id: 'foo',
type: 'Note',
content: 'Updated text',
}
end
before do
subject.call(status, json)
end
it 'does not create any edits' do
expect(status.reload.edits).to be_empty
end
it 'does not mark status as edited' do
expect(status.reload.edited?).to be false
end
it 'does not update the text' do
expect(status.reload.text).to eq 'Hello world'
end
end
context 'when the status has not been explicitly edited and features a poll' do
let(:account) { Fabricate(:account, domain: 'example.com') }
let!(:expiration) { 10.days.from_now.utc }
let!(:status) do
Fabricate(:status,
text: 'Hello world',
account: account,
poll_attributes: {
options: %w(Foo Bar),
account: account,
multiple: false,
hide_totals: false,
expires_at: expiration
}
)
end
let(:payload) do
{
'@context': 'https://www.w3.org/ns/activitystreams',
id: 'https://example.com/foo',
type: 'Question',
content: 'Hello world',
endTime: expiration.iso8601,
oneOf: [
poll_option_json('Foo', 4),
poll_option_json('Bar', 3),
],
}
end
before do
subject.call(status, json)
end
it 'does not create any edits' do
expect(status.reload.edits).to be_empty
end
it 'does not mark status as edited' do
expect(status.reload.edited?).to be false
end
it 'does not update the text' do
expect(status.reload.text).to eq 'Hello world'
end
it 'updates tallies' do
expect(status.poll.reload.cached_tallies).to eq [4, 3]
end
end
context 'when the status changes a poll despite being not explicitly marked as updated' do
let(:account) { Fabricate(:account, domain: 'example.com') }
let!(:expiration) { 10.days.from_now.utc }
let!(:status) do
Fabricate(:status,
text: 'Hello world',
account: account,
poll_attributes: {
options: %w(Foo Bar),
account: account,
multiple: false,
hide_totals: false,
expires_at: expiration
}
)
end
let(:payload) do
{
'@context': 'https://www.w3.org/ns/activitystreams',
id: 'https://example.com/foo',
type: 'Question',
content: 'Hello world',
endTime: expiration.iso8601,
oneOf: [
poll_option_json('Foo', 4),
poll_option_json('Bar', 3),
poll_option_json('Baz', 3),
],
}
end
before do
subject.call(status, json)
end
it 'does not create any edits' do
expect(status.reload.edits).to be_empty
end
it 'does not mark status as edited' do
expect(status.reload.edited?).to be false
end
it 'does not update the text' do
expect(status.reload.text).to eq 'Hello world'
end
it 'does not update tallies' do
expect(status.poll.reload.cached_tallies).to eq [0, 0]
end
end
context 'when receiving an edit older than the latest processed' do
before do
status.snapshot!(at_time: status.created_at, rate_limit: false)
status.update!(text: 'Hello newer world', edited_at: Time.now.utc)
status.snapshot!(rate_limit: false)
end
it 'does not create any edits' do
expect { subject.call(status, json) }.not_to change { status.reload.edits.pluck(&:id) }
end
it 'does not update the text, spoiler_text or edited_at' do
expect { subject.call(status, json) }.not_to change { s = status.reload; [s.text, s.spoiler_text, s.edited_at] }
end
end
context 'with no changes at all' do context 'with no changes at all' do
let(:payload) do let(:payload) do
{ {