Improve error reporting and logging when processing remote accounts (#15605)

* Add a more descriptive PrivateNetworkAddressError exception class

* Remove unnecessary exception class to rescue clause

* Remove unnecessary include to JsonLdHelper

* Give more neutral error message when too many webfinger redirects

* Remove unnecessary guard condition

* Rework how “ActivityPub::FetchRemoteAccountService” handles errors

Add “suppress_errors” keyword argument to avoid raising errors in
ActivityPub::FetchRemoteAccountService#call (default/previous behavior).

* Rework how “ActivityPub::FetchRemoteKeyService” handles errors

Add “suppress_errors” keyword argument to avoid raising errors in
ActivityPub::FetchRemoteKeyService#call (default/previous behavior).

* Fix Webfinger::RedirectError not being a subclass of Webfinger::Error

* Add suppress_errors option to ResolveAccountService

Defaults to true (to preserve previous behavior). If set to false,
errors will be raised instead of caught, allowing the caller to be
informed of what went wrong.

* Return more precise error when failing to fetch account signing AP payloads

* Add tests

* Fixes

* Refactor error handling a bit

* Fix various issues

* Add specific error when provided Digest is not 256 bits of base64-encoded data

* Please CodeClimate

* Improve webfinger error reporting

1 files changed, 26 insertions, 12 deletions

diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb
index 9d01f5386..d7d739c59 100644
--- a/app/services/activitypub/fetch_remote_account_service.rb
+++ b/app/services/activitypub/fetch_remote_account_service.rb
@@ -5,10 +5,12 @@ class ActivityPub::FetchRemoteAccountService < BaseService
   include DomainControlHelper
   include WebfingerHelper
 
+  class Error < StandardError; end
+
   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)
+  def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true)
     return if domain_not_allowed?(uri)
     return ActivityPub::TagManager.instance.uri_to_resource(uri, Account) if ActivityPub::TagManager.instance.local_uri?(uri)
 
@@ -18,38 +20,50 @@ class ActivityPub::FetchRemoteAccountService < BaseService
       else
         body_to_json(prefetched_body, compare_id: id ? uri : nil)
       end
+    rescue Oj::ParseError
+      raise Error, "Error parsing JSON-LD document #{uri}"
     end
 
-    return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?)
+    raise Error, "Error fetching actor JSON at #{uri}" if @json.nil?
+    raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?
+    raise Error, "Unexpected object type for actor #{uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_type?
+    raise Error, "Actor #{uri} has moved to #{@json['movedTo']}" if break_on_redirect && @json['movedTo'].present?
 
     @uri      = @json['id']
     @username = @json['preferredUsername']
     @domain   = Addressable::URI.parse(@uri).normalized_host
 
-    return unless only_key || verified_webfinger?
+    check_webfinger! unless only_key
 
     ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key, verified_webfinger: !only_key)
-  rescue Oj::ParseError
-    nil
+  rescue Error => e
+    Rails.logger.debug "Fetching account #{uri} failed: #{e.message}"
+    raise unless suppress_errors
   end
 
   private
 
-  def verified_webfinger?
+  def check_webfinger!
     webfinger                            = webfinger!("acct:#{@username}@#{@domain}")
     confirmed_username, confirmed_domain = split_acct(webfinger.subject)
 
-    return webfinger.link('self', 'href') == @uri if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero?
+    if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero?
+      raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri
+      return
+    end
 
     webfinger                            = webfinger!("acct:#{confirmed_username}@#{confirmed_domain}")
     @username, @domain                   = split_acct(webfinger.subject)
 
-    return false unless @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero?
-    return false if webfinger.link('self', 'href') != @uri
+    unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
+      raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{uri} (stopped at #{@username}@#{@domain})"
+    end
 
-    true
-  rescue Webfinger::Error
-    false
+    raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri
+  rescue Webfinger::RedirectError => e
+    raise Error, e.message
+  rescue Webfinger::Error => e
+    raise Error, "Webfinger error when resolving #{@username}@#{@domain}: #{e.message}"
   end
 
   def split_acct(acct)