From 4e921832272425352d28cad550bfc4dffd6d0e78 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 9 Jul 2019 03:27:35 +0200 Subject: [PATCH] Refactor domain block checks (#11268) --- .../concerns/signature_verification.rb | 4 + app/helpers/domain_control_helper.rb | 17 +++ app/lib/tag_manager.rb | 3 + .../fetch_featured_collection_service.rb | 3 +- .../fetch_remote_account_service.rb | 14 ++- .../activitypub/fetch_remote_poll_service.rb | 2 + .../activitypub/process_account_service.rb | 5 +- .../activitypub/process_collection_service.rb | 4 +- .../activitypub/process_poll_service.rb | 1 + app/services/resolve_account_service.rb | 101 +++++++++++------- spec/services/resolve_account_service_spec.rb | 5 + 11 files changed, 108 insertions(+), 51 deletions(-) create mode 100644 app/helpers/domain_control_helper.rb diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 90a57197c..0ccdf5ec9 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -5,6 +5,8 @@ module SignatureVerification extend ActiveSupport::Concern + include DomainControlHelper + def signed_request? request.headers['Signature'].present? end @@ -126,6 +128,8 @@ module SignatureVerification if key_id.start_with?('acct:') stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, '')) } elsif !ActivityPub::TagManager.instance.local_uri?(key_id) + return if domain_not_allowed?(key_id) + account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account) account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false) } account diff --git a/app/helpers/domain_control_helper.rb b/app/helpers/domain_control_helper.rb new file mode 100644 index 000000000..efd328f81 --- /dev/null +++ b/app/helpers/domain_control_helper.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module DomainControlHelper + def domain_not_allowed?(uri_or_domain) + return if uri_or_domain.blank? + + domain = begin + if uri_or_domain.include?('://') + Addressable::URI.parse(uri_or_domain).domain + else + uri_or_domain + end + end + + DomainBlock.blocked?(domain) + end +end diff --git a/app/lib/tag_manager.rb b/app/lib/tag_manager.rb index daf4f556b..c88cf4994 100644 --- a/app/lib/tag_manager.rb +++ b/app/lib/tag_manager.rb @@ -24,13 +24,16 @@ class TagManager def same_acct?(canonical, needle) return true if canonical.casecmp(needle).zero? + username, domain = needle.split('@') + local_domain?(domain) && canonical.casecmp(username).zero? end def local_url?(url) uri = Addressable::URI.parse(url).normalize domain = uri.host + (uri.port ? ":#{uri.port}" : '') + TagManager.instance.web_domain?(domain) end end diff --git a/app/services/activitypub/fetch_featured_collection_service.rb b/app/services/activitypub/fetch_featured_collection_service.rb index 6a137b520..2c2770466 100644 --- a/app/services/activitypub/fetch_featured_collection_service.rb +++ b/app/services/activitypub/fetch_featured_collection_service.rb @@ -4,13 +4,12 @@ class ActivityPub::FetchFeaturedCollectionService < BaseService include JsonLdHelper def call(account) - return if account.featured_collection_url.blank? + return if account.featured_collection_url.blank? || account.suspended? || account.local? @account = account @json = fetch_resource(@account.featured_collection_url, true) return unless supported_context? - return if @account.suspended? || @account.local? case @json['type'] when 'Collection', 'CollectionPage' diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index 3c2044941..d65c8f951 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -2,18 +2,22 @@ class ActivityPub::FetchRemoteAccountService < BaseService include JsonLdHelper + include DomainControlHelper 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) + return if domain_not_allowed?(uri) return ActivityPub::TagManager.instance.uri_to_resource(uri, Account) if ActivityPub::TagManager.instance.local_uri?(uri) - @json = if prefetched_body.nil? - fetch_resource(uri, id) - else - body_to_json(prefetched_body, compare_id: id ? uri : nil) - end + @json = begin + if prefetched_body.nil? + fetch_resource(uri, id) + else + body_to_json(prefetched_body, compare_id: id ? uri : nil) + end + end return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?) diff --git a/app/services/activitypub/fetch_remote_poll_service.rb b/app/services/activitypub/fetch_remote_poll_service.rb index 854a32d05..1c79ecf11 100644 --- a/app/services/activitypub/fetch_remote_poll_service.rb +++ b/app/services/activitypub/fetch_remote_poll_service.rb @@ -5,7 +5,9 @@ class ActivityPub::FetchRemotePollService < BaseService def call(poll, on_behalf_of = nil) json = fetch_resource(poll.status.uri, true, on_behalf_of) + return unless supported_context?(json) + ActivityPub::ProcessPollService.new.call(poll, json) end end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 3857e7c16..603e27ed9 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -2,11 +2,12 @@ class ActivityPub::ProcessAccountService < BaseService include JsonLdHelper + include DomainControlHelper # Should be called with confirmed valid JSON # and WebFinger-resolved username and domain def call(username, domain, json, options = {}) - return if json['inbox'].blank? || unsupported_uri_scheme?(json['id']) + return if json['inbox'].blank? || unsupported_uri_scheme?(json['id']) || domain_not_allowed?(domain) @options = options @json = json @@ -15,8 +16,6 @@ class ActivityPub::ProcessAccountService < BaseService @domain = domain @collections = {} - return if auto_suspend? - RedisLock.acquire(lock_options) do |lock| if lock.acquired? @account = Account.find_remote(@username, @domain) diff --git a/app/services/activitypub/process_collection_service.rb b/app/services/activitypub/process_collection_service.rb index 881df478b..a2a2e7071 100644 --- a/app/services/activitypub/process_collection_service.rb +++ b/app/services/activitypub/process_collection_service.rb @@ -8,9 +8,7 @@ class ActivityPub::ProcessCollectionService < BaseService @json = Oj.load(body, mode: :strict) @options = options - return unless supported_context? - return if different_actor? && verify_account!.nil? - return if @account.suspended? || @account.local? + return if !supported_context? || (different_actor? && verify_account!.nil?) || @account.suspended? || @account.local? case @json['type'] when 'Collection', 'CollectionPage' diff --git a/app/services/activitypub/process_poll_service.rb b/app/services/activitypub/process_poll_service.rb index 61357abd3..2fbce65b9 100644 --- a/app/services/activitypub/process_poll_service.rb +++ b/app/services/activitypub/process_poll_service.rb @@ -5,6 +5,7 @@ class ActivityPub::ProcessPollService < BaseService def call(poll, json) @json = json + return unless expected_type? previous_expires_at = poll.expires_at diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index 0ea31a0d8..41a2eb158 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -1,75 +1,108 @@ # frozen_string_literal: true -require_relative '../models/account' - class ResolveAccountService < BaseService include JsonLdHelper + include DomainControlHelper - # Find or create a local account for a remote user. - # When creating, look up the user's webfinger and fetch all - # important information from their feed - # @param [String, Account] uri User URI in the form of username@domain + class WebfingerRedirectError < StandardError; end + + # Find or create an account record for a remote user. When creating, + # look up the user's webfinger and fetch ActivityPub data + # @param [String, Account] uri URI in the username@domain format or account record # @param [Hash] options + # @option options [Boolean] :redirected Do not follow further Webfinger redirects + # @option options [Boolean] :skip_webfinger Do not attempt to refresh account data # @return [Account] def call(uri, options = {}) + return if uri.blank? + + process_options!(uri, options) + + # First of all we want to check if we've got the account + # record with the URI already, and if so, we can exit early + + return if domain_not_allowed?(@domain) + + @account ||= Account.find_remote(@username, @domain) + + return @account if @account&.local? || !webfinger_update_due? + + # At this point we are in need of a Webfinger query, which may + # yield us a different username/domain through a redirect + + process_webfinger! + + # Because the username/domain pair may be different than what + # we already checked, we need to check if we've already got + # the record with that URI, again + + return if domain_not_allowed?(@domain) + + @account ||= Account.find_remote(@username, @domain) + + return @account if @account&.local? || !webfinger_update_due? + + # Now it is certain, it is definitely a remote account, and it + # either needs to be created, or updated from fresh data + + process_account! + rescue Goldfinger::Error, WebfingerRedirectError, Oj::ParseError => e + Rails.logger.debug "Webfinger query for #{@uri} failed: #{e}" + nil + end + + private + + def process_options!(uri, options) @options = options if uri.is_a?(Account) @account = uri @username = @account.username @domain = @account.domain - uri = "#{@username}@#{@domain}" - - return @account if @account.local? || !webfinger_update_due? + @uri = [@username, @domain].compact.join('@') else + @uri = uri @username, @domain = uri.split('@') - - return Account.find_local(@username) if TagManager.instance.local_domain?(@domain) - - @account = Account.find_remote(@username, @domain) - - return @account unless webfinger_update_due? end - Rails.logger.debug "Looking up webfinger for #{uri}" - - @webfinger = Goldfinger.finger("acct:#{uri}") + @domain = nil if TagManager.instance.local_domain?(@domain) + end + def process_webfinger! + @webfinger = Goldfinger.finger("acct:#{@uri}") confirmed_username, confirmed_domain = @webfinger.subject.gsub(/\Aacct:/, '').split('@') if confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? @username = confirmed_username @domain = confirmed_domain - elsif options[:redirected].nil? - return call("#{confirmed_username}@#{confirmed_domain}", options.merge(redirected: true)) + elsif @options[:redirected].nil? + @account = ResolveAccountService.new.call("#{confirmed_username}@#{confirmed_domain}", @options.merge(redirected: true)) else - Rails.logger.debug 'Requested and returned acct URIs do not match' - return + raise WebfingerRedirectError, "The URI #{uri} tries to hijack #{@username}@#{@domain}" end - return Account.find_local(@username) if TagManager.instance.local_domain?(@domain) + @domain = nil if TagManager.instance.local_domain?(@domain) + end + + def process_account! return unless activitypub_ready? RedisLock.acquire(lock_options) do |lock| if lock.acquired? @account = Account.find_remote(@username, @domain) - next unless @account.nil? || @account.activitypub? + next if (@account.present? && !@account.activitypub?) || actor_json.nil? - handle_activitypub + @account = ActivityPub::ProcessAccountService.new.call(@username, @domain, actor_json) else raise Mastodon::RaceConditionError end end @account - rescue Goldfinger::Error => e - Rails.logger.debug "Webfinger query for #{uri} unsuccessful: #{e}" - nil end - private - def webfinger_update_due? @account.nil? || ((!@options[:skip_webfinger] || @account.ostatus?) && @account.possibly_stale?) end @@ -78,14 +111,6 @@ class ResolveAccountService < BaseService !@webfinger.link('self').nil? && ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@webfinger.link('self').type) end - def handle_activitypub - return if actor_json.nil? - - @account = ActivityPub::ProcessAccountService.new.call(@username, @domain, actor_json) - rescue Oj::ParseError - nil - end - def actor_url @actor_url ||= @webfinger.link('self').href end diff --git a/spec/services/resolve_account_service_spec.rb b/spec/services/resolve_account_service_spec.rb index 7a64f4161..cea942e39 100644 --- a/spec/services/resolve_account_service_spec.rb +++ b/spec/services/resolve_account_service_spec.rb @@ -53,6 +53,11 @@ RSpec.describe ResolveAccountService, type: :service do fail_occurred = false return_values = Concurrent::Array.new + # Preload classes that throw circular dependency errors in threads + Account + TagManager + DomainBlock + threads = Array.new(5) do Thread.new do true while wait_for_start