From fe523a304520a09f6371f45bd63b9e8988776c03 Mon Sep 17 00:00:00 2001 From: Claire Date: Sun, 4 Dec 2022 21:23:19 +0100 Subject: Fix unbounded recursion in account discovery (#1994) * Fix trying to fetch posts from other users when fetching featured posts * Rate-limit discovery of new subdomains * Put a limit on recursively discovering new accounts --- .../fetch_featured_collection_service.rb | 4 ++-- .../activitypub/fetch_remote_account_service.rb | 2 +- .../activitypub/fetch_remote_actor_service.rb | 4 ++-- .../activitypub/fetch_remote_status_service.rb | 8 ++++--- .../activitypub/process_account_service.rb | 25 ++++++++++++++++++---- .../activitypub/process_status_update_service.rb | 5 +++-- 6 files changed, 34 insertions(+), 14 deletions(-) (limited to 'app/services') diff --git a/app/services/activitypub/fetch_featured_collection_service.rb b/app/services/activitypub/fetch_featured_collection_service.rb index 50a187ad9..a746ef4d6 100644 --- a/app/services/activitypub/fetch_featured_collection_service.rb +++ b/app/services/activitypub/fetch_featured_collection_service.rb @@ -46,9 +46,9 @@ class ActivityPub::FetchFeaturedCollectionService < BaseService next unless item.is_a?(String) || item['type'] == 'Note' uri = value_or_id(item) - next if ActivityPub::TagManager.instance.local_uri?(uri) + next if ActivityPub::TagManager.instance.local_uri?(uri) || invalid_origin?(uri) - status = ActivityPub::FetchRemoteStatusService.new.call(uri, on_behalf_of: local_follower) + status = ActivityPub::FetchRemoteStatusService.new.call(uri, on_behalf_of: local_follower, expected_actor_uri: @account.uri, request_id: @options[:request_id]) next unless status&.account_id == @account.id status.id diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index ca7a8c6ca..7aba8269e 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -2,7 +2,7 @@ class ActivityPub::FetchRemoteAccountService < ActivityPub::FetchRemoteActorService # Does a WebFinger roundtrip on each call, unless `only_key` is true - def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true) + def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil) actor = super return actor if actor.nil? || actor.is_a?(Account) diff --git a/app/services/activitypub/fetch_remote_actor_service.rb b/app/services/activitypub/fetch_remote_actor_service.rb index db09c38d8..a25fa54c4 100644 --- a/app/services/activitypub/fetch_remote_actor_service.rb +++ b/app/services/activitypub/fetch_remote_actor_service.rb @@ -10,7 +10,7 @@ class ActivityPub::FetchRemoteActorService < BaseService SUPPORTED_TYPES = %w(Application Group Organization Person Service).freeze # Does a WebFinger roundtrip on each call, unless `only_key` is true - def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true) + def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil) return if domain_not_allowed?(uri) return ActivityPub::TagManager.instance.uri_to_actor(uri) if ActivityPub::TagManager.instance.local_uri?(uri) @@ -35,7 +35,7 @@ class ActivityPub::FetchRemoteActorService < BaseService check_webfinger! unless only_key - ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key, verified_webfinger: !only_key) + ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key, verified_webfinger: !only_key, request_id: request_id) rescue Error => e Rails.logger.debug "Fetching actor #{uri} failed: #{e.message}" raise unless suppress_errors diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index 803098245..21b9242f8 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -4,7 +4,8 @@ class ActivityPub::FetchRemoteStatusService < BaseService include JsonLdHelper # Should be called when uri has already been checked for locality - def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil) + def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil) + @request_id = request_id @json = begin if prefetched_body.nil? fetch_resource(uri, id, on_behalf_of) @@ -30,6 +31,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService end return if activity_json.nil? || object_uri.nil? || !trustworthy_attribution?(@json['id'], actor_uri) + return if expected_actor_uri.present? && actor_uri != expected_actor_uri return ActivityPub::TagManager.instance.uri_to_resource(object_uri, Status) if ActivityPub::TagManager.instance.local_uri?(object_uri) actor = account_from_uri(actor_uri) @@ -40,7 +42,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService # activity as an update rather than create activity_json['type'] = 'Update' if equals_or_includes_any?(activity_json['type'], %w(Create)) && Status.where(uri: object_uri, account_id: actor.id).exists? - ActivityPub::Activity.factory(activity_json, actor).perform + ActivityPub::Activity.factory(activity_json, actor, request_id: request_id).perform end private @@ -52,7 +54,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService def account_from_uri(uri) actor = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) - actor = ActivityPub::FetchRemoteAccountService.new.call(uri, id: true) if actor.nil? || actor.possibly_stale? + actor = ActivityPub::FetchRemoteAccountService.new.call(uri, id: true, request_id: @request_id) if actor.nil? || actor.possibly_stale? actor end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 99bcb3835..2da9096c7 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -6,6 +6,9 @@ class ActivityPub::ProcessAccountService < BaseService include Redisable include Lockable + SUBDOMAINS_RATELIMIT = 10 + DISCOVERIES_PER_REQUEST = 400 + # Should be called with confirmed valid JSON # and WebFinger-resolved username and domain def call(username, domain, json, options = {}) @@ -15,9 +18,12 @@ class ActivityPub::ProcessAccountService < BaseService @json = json @uri = @json['id'] @username = username - @domain = domain + @domain = TagManager.instance.normalize_domain(domain) @collections = {} + # The key does not need to be unguessable, it just needs to be somewhat unique + @options[:request_id] ||= "#{Time.now.utc.to_i}-#{username}@#{domain}" + with_lock("process_account:#{@uri}") do @account = Account.remote.find_by(uri: @uri) if @options[:only_key] @account ||= Account.find_remote(@username, @domain) @@ -25,7 +31,18 @@ class ActivityPub::ProcessAccountService < BaseService @old_protocol = @account&.protocol @suspension_changed = false - create_account if @account.nil? + if @account.nil? + with_redis do |redis| + return nil if redis.pfcount("unique_subdomains_for:#{PublicSuffix.domain(@domain, ignore_private: true)}") >= SUBDOMAINS_RATELIMIT + + discoveries = redis.incr("discovery_per_request:#{@options[:request_id]}") + redis.expire("discovery_per_request:#{@options[:request_id]}", 5.minutes.seconds) + return nil if discoveries > DISCOVERIES_PER_REQUEST + end + + create_account + end + update_account process_tags @@ -150,7 +167,7 @@ class ActivityPub::ProcessAccountService < BaseService end def check_featured_collection! - ActivityPub::SynchronizeFeaturedCollectionWorker.perform_async(@account.id, { 'hashtag' => @json['featuredTags'].blank? }) + ActivityPub::SynchronizeFeaturedCollectionWorker.perform_async(@account.id, { 'hashtag' => @json['featuredTags'].blank?, 'request_id' => @options[:request_id] }) end def check_featured_tags_collection! @@ -254,7 +271,7 @@ class ActivityPub::ProcessAccountService < BaseService def moved_account account = ActivityPub::TagManager.instance.uri_to_resource(@json['movedTo'], Account) - account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], id: true, break_on_redirect: true) + account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], id: true, break_on_redirect: true, request_id: @options[:request_id]) account end diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb index a0605b1a3..fad19f87f 100644 --- a/app/services/activitypub/process_status_update_service.rb +++ b/app/services/activitypub/process_status_update_service.rb @@ -5,7 +5,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService include Redisable include Lockable - def call(status, json) + def call(status, json, request_id: nil) raise ArgumentError, 'Status has unsaved changes' if status.changed? @json = json @@ -15,6 +15,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService @account = status.account @media_attachments_changed = false @poll_changed = false + @request_id = request_id # Only native types can be updated at the moment return @status if !expected_type? || already_updated_more_recently? @@ -191,7 +192,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService next if href.blank? account = ActivityPub::TagManager.instance.uri_to_resource(href, Account) - account ||= ActivityPub::FetchRemoteAccountService.new.call(href) + account ||= ActivityPub::FetchRemoteAccountService.new.call(href, request_id: @request_id) next if account.nil? -- cgit