From 5efb1ff337e7c1c090eb9c43dc3ba5b284302460 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 11 Aug 2021 17:48:42 +0200 Subject: [PATCH] Fix followers synchronization mechanism not working when URI has empty path (#16510) * Fix followers synchronization mechanism not working when URI has empty path To my knowledge, there is no current implementation on the fediverse that can use bare domains (e.g., actor is at https://example.org instead of something like https://example.org/actor) that also plans to support the followers synchronization mechanism. However, Mastodon's current implementation would exclude such accounts from followers list. Also adds tests and rename them to reflect the proper method names. * Move url prefix regexp to its own constant --- app/models/account.rb | 7 ++- app/models/concerns/account_interactions.rb | 9 ++- app/workers/activitypub/delivery_worker.rb | 6 +- .../concerns/account_interactions_spec.rb | 63 +++++++++++-------- .../activitypub/delivery_worker_spec.rb | 2 +- 5 files changed, 49 insertions(+), 38 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index f7a7f4a7b4..2c2840ee48 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -58,8 +58,9 @@ class Account < ApplicationRecord hub_url ) - USERNAME_RE = /[a-z0-9_]+([a-z0-9_\.-]+[a-z0-9_]+)?/i - MENTION_RE = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[[:word:]\.\-]+[a-z0-9]+)?)/i + USERNAME_RE = /[a-z0-9_]+([a-z0-9_\.-]+[a-z0-9_]+)?/i + MENTION_RE = /(?<=^|[^\/[:word:]])@((#{USERNAME_RE})(?:@[[:word:]\.\-]+[a-z0-9]+)?)/i + URL_PREFIX_RE = /\Ahttp(s?):\/\/[^\/]+/ include AccountAssociations include AccountAvatar @@ -379,7 +380,7 @@ class Account < ApplicationRecord def synchronization_uri_prefix return 'local' if local? - @synchronization_uri_prefix ||= uri[/http(s?):\/\/[^\/]+\//] + @synchronization_uri_prefix ||= "#{uri[URL_PREFIX_RE]}/" end class Field < ActiveModelSerializers::Model diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index 4bf62539ce..8f19176a74 100644 --- a/app/models/concerns/account_interactions.rb +++ b/app/models/concerns/account_interactions.rb @@ -254,10 +254,13 @@ module AccountInteractions .where('users.current_sign_in_at > ?', User::ACTIVE_DURATION.ago) end - def remote_followers_hash(url_prefix) - Rails.cache.fetch("followers_hash:#{id}:#{url_prefix}") do + def remote_followers_hash(url) + url_prefix = url[Account::URL_PREFIX_RE] + return if url_prefix.blank? + + Rails.cache.fetch("followers_hash:#{id}:#{url_prefix}/") do digest = "\x00" * 32 - followers.where(Account.arel_table[:uri].matches(url_prefix + '%', false, true)).pluck_each(:uri) do |uri| + followers.where(Account.arel_table[:uri].matches("#{Account.sanitize_sql_like(url_prefix)}/%", false, true)).or(followers.where(uri: url_prefix)).pluck_each(:uri) do |uri| Xorcist.xor!(digest, Digest::SHA256.digest(uri)) end digest.unpack('H*')[0] diff --git a/app/workers/activitypub/delivery_worker.rb b/app/workers/activitypub/delivery_worker.rb index 6c5a576a70..788f2cf809 100644 --- a/app/workers/activitypub/delivery_worker.rb +++ b/app/workers/activitypub/delivery_worker.rb @@ -44,11 +44,7 @@ class ActivityPub::DeliveryWorker end def synchronization_header - "collectionId=\"#{account_followers_url(@source_account)}\", digest=\"#{@source_account.remote_followers_hash(inbox_url_prefix)}\", url=\"#{account_followers_synchronization_url(@source_account)}\"" - end - - def inbox_url_prefix - @inbox_url[/http(s?):\/\/[^\/]+\//] + "collectionId=\"#{account_followers_url(@source_account)}\", digest=\"#{@source_account.remote_followers_hash(@inbox_url)}\", url=\"#{account_followers_synchronization_url(@source_account)}\"" end def perform_request diff --git a/spec/models/concerns/account_interactions_spec.rb b/spec/models/concerns/account_interactions_spec.rb index 85fbf7e79c..ca243ebc5e 100644 --- a/spec/models/concerns/account_interactions_spec.rb +++ b/spec/models/concerns/account_interactions_spec.rb @@ -539,46 +539,57 @@ describe AccountInteractions do end end - describe '#followers_hash' do + describe '#remote_followers_hash' do let(:me) { Fabricate(:account, username: 'Me') } let(:remote_1) { Fabricate(:account, username: 'alice', domain: 'example.org', uri: 'https://example.org/users/alice') } let(:remote_2) { Fabricate(:account, username: 'bob', domain: 'example.org', uri: 'https://example.org/users/bob') } - let(:remote_3) { Fabricate(:account, username: 'eve', domain: 'foo.org', uri: 'https://foo.org/users/eve') } + let(:remote_3) { Fabricate(:account, username: 'instance-actor', domain: 'example.org', uri: 'https://example.org') } + let(:remote_4) { Fabricate(:account, username: 'eve', domain: 'foo.org', uri: 'https://foo.org/users/eve') } before do remote_1.follow!(me) remote_2.follow!(me) remote_3.follow!(me) + remote_4.follow!(me) me.follow!(remote_1) end - context 'on a local user' do - it 'returns correct hash for remote domains' do - expect(me.remote_followers_hash('https://example.org/')).to eq '707962e297b7bd94468a21bc8e506a1bcea607a9142cd64e27c9b106b2a5f6ec' - expect(me.remote_followers_hash('https://foo.org/')).to eq 'ccb9c18a67134cfff9d62c7f7e7eb88e6b803446c244b84265565f4eba29df0e' - end - - it 'invalidates cache as needed when removing or adding followers' do - expect(me.remote_followers_hash('https://example.org/')).to eq '707962e297b7bd94468a21bc8e506a1bcea607a9142cd64e27c9b106b2a5f6ec' - remote_1.unfollow!(me) - expect(me.remote_followers_hash('https://example.org/')).to eq '241b00794ce9b46aa864f3220afadef128318da2659782985bac5ed5bd436bff' - remote_1.follow!(me) - expect(me.remote_followers_hash('https://example.org/')).to eq '707962e297b7bd94468a21bc8e506a1bcea607a9142cd64e27c9b106b2a5f6ec' - end + it 'returns correct hash for remote domains' do + expect(me.remote_followers_hash('https://example.org/')).to eq '20aecbe774b3d61c25094370baf370012b9271c5b172ecedb05caff8d79ef0c7' + expect(me.remote_followers_hash('https://foo.org/')).to eq 'ccb9c18a67134cfff9d62c7f7e7eb88e6b803446c244b84265565f4eba29df0e' + expect(me.remote_followers_hash('https://foo.org.evil.com/')).to eq '0000000000000000000000000000000000000000000000000000000000000000' + expect(me.remote_followers_hash('https://foo')).to eq '0000000000000000000000000000000000000000000000000000000000000000' end - context 'on a remote user' do - it 'returns correct hash for remote domains' do - expect(remote_1.local_followers_hash).to eq Digest::SHA256.hexdigest(ActivityPub::TagManager.instance.uri_for(me)) - end + it 'invalidates cache as needed when removing or adding followers' do + expect(me.remote_followers_hash('https://example.org/')).to eq '20aecbe774b3d61c25094370baf370012b9271c5b172ecedb05caff8d79ef0c7' + remote_3.unfollow!(me) + expect(me.remote_followers_hash('https://example.org/')).to eq '707962e297b7bd94468a21bc8e506a1bcea607a9142cd64e27c9b106b2a5f6ec' + remote_1.unfollow!(me) + expect(me.remote_followers_hash('https://example.org/')).to eq '241b00794ce9b46aa864f3220afadef128318da2659782985bac5ed5bd436bff' + remote_1.follow!(me) + expect(me.remote_followers_hash('https://example.org/')).to eq '707962e297b7bd94468a21bc8e506a1bcea607a9142cd64e27c9b106b2a5f6ec' + end + end - it 'invalidates cache as needed when removing or adding followers' do - expect(remote_1.local_followers_hash).to eq Digest::SHA256.hexdigest(ActivityPub::TagManager.instance.uri_for(me)) - me.unfollow!(remote_1) - expect(remote_1.local_followers_hash).to eq '0000000000000000000000000000000000000000000000000000000000000000' - me.follow!(remote_1) - expect(remote_1.local_followers_hash).to eq Digest::SHA256.hexdigest(ActivityPub::TagManager.instance.uri_for(me)) - end + describe '#local_followers_hash' do + let(:me) { Fabricate(:account, username: 'Me') } + let(:remote_1) { Fabricate(:account, username: 'alice', domain: 'example.org', uri: 'https://example.org/users/alice') } + + before do + me.follow!(remote_1) + end + + it 'returns correct hash for local users' do + expect(remote_1.local_followers_hash).to eq Digest::SHA256.hexdigest(ActivityPub::TagManager.instance.uri_for(me)) + end + + it 'invalidates cache as needed when removing or adding followers' do + expect(remote_1.local_followers_hash).to eq Digest::SHA256.hexdigest(ActivityPub::TagManager.instance.uri_for(me)) + me.unfollow!(remote_1) + expect(remote_1.local_followers_hash).to eq '0000000000000000000000000000000000000000000000000000000000000000' + me.follow!(remote_1) + expect(remote_1.local_followers_hash).to eq Digest::SHA256.hexdigest(ActivityPub::TagManager.instance.uri_for(me)) end end diff --git a/spec/workers/activitypub/delivery_worker_spec.rb b/spec/workers/activitypub/delivery_worker_spec.rb index f4633731e5..d39393d507 100644 --- a/spec/workers/activitypub/delivery_worker_spec.rb +++ b/spec/workers/activitypub/delivery_worker_spec.rb @@ -11,7 +11,7 @@ describe ActivityPub::DeliveryWorker do let(:payload) { 'test' } before do - allow_any_instance_of(Account).to receive(:remote_followers_hash).with('https://example.com/').and_return('somehash') + allow_any_instance_of(Account).to receive(:remote_followers_hash).with('https://example.com/api').and_return('somehash') end describe 'perform' do