Fix webfinger retries (#4275)

* Do not raise unretryable exceptions in ResolveRemoteAccountService

* Removed fatal exceptions from ResolveRemoteAccountService

Exceptions that cannot be retried should not be raised. New exception
class for those that can be retried (Mastodon::UnexpectedResponseError)

app/controllers/api/base_controller.rb

@@ -17,11 +17,7 @@ class Api::BaseController < ApplicationController
     render json: { error: 'Record not found' }, status: 404
   end
 
-  rescue_from Goldfinger::Error do
+  rescue_from HTTP::Error, Mastodon::UnexpectedResponseError do
-    render json: { error: 'Remote account could not be resolved' }, status: 422
-  end
-
-  rescue_from HTTP::Error do
     render json: { error: 'Remote data could not be fetched' }, status: 503
   end
 

app/lib/exceptions.rb

@@ -5,4 +5,14 @@ module Mastodon
   class NotPermittedError < Error; end
   class ValidationError < Error; end
   class RaceConditionError < Error; end
+
+  class UnexpectedResponseError < Error
+    def initialize(response = nil)
+      @response = response
+    end
+
+    def to_s
+      "#{@response.uri} returned code #{@response.code}"
+    end
+  end
 end

app/services/fetch_remote_account_service.rb

@@ -32,8 +32,5 @@ class FetchRemoteAccountService < BaseService
   rescue Nokogiri::XML::XPath::SyntaxError
     Rails.logger.debug 'Invalid XML or missing namespace'
     nil
-  rescue Goldfinger::NotFoundError, Goldfinger::Error
-    Rails.logger.debug 'Exceptions related to Goldfinger occurs'
-    nil
   end
 end

app/services/fetch_remote_status_service.rb

@@ -33,9 +33,6 @@ class FetchRemoteStatusService < BaseService
   rescue Nokogiri::XML::XPath::SyntaxError
     Rails.logger.debug 'Invalid XML or missing namespace'
     nil
-  rescue Goldfinger::NotFoundError, Goldfinger::Error
-    Rails.logger.debug 'Exceptions related to Goldfinger occurs'
-    nil
   end
 
   def confirmed_domain?(domain, account)

app/services/process_interaction_service.rb

@@ -47,7 +47,7 @@ class ProcessInteractionService < BaseService
         reflect_unblock!(account, target_account)
       end
     end
-  rescue Goldfinger::Error, HTTP::Error, OStatus2::BadSalmonError, Mastodon::NotPermittedError
+  rescue HTTP::Error, OStatus2::BadSalmonError, Mastodon::NotPermittedError
     nil
   end
 

app/services/resolve_remote_account_service.rb

@@ -23,18 +23,19 @@ class ResolveRemoteAccountService < BaseService
 
     @webfinger = Goldfinger.finger("acct:#{uri}")
 
-    raise Goldfinger::Error, 'Missing resource links' if links_missing?
-
     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 redirected.nil?
+      return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true)
     else
-      return call("#{confirmed_username}@#{confirmed_domain}", update_profile, true) if redirected.nil?
+      Rails.logger.debug 'Requested and returned acct URIs do not match'
-      raise Goldfinger::Error, 'Requested and returned acct URIs do not match'
+      return
     end
 
+    return if links_missing?
     return Account.find_local(@username) if TagManager.instance.local_domain?(@domain)
 
     RedisLock.acquire(lock_options) do |lock|
@@ -49,6 +50,9 @@ class ResolveRemoteAccountService < BaseService
     end
 
     @account
+  rescue Goldfinger::Error => e
+    Rails.logger.debug "Webfinger query for #{uri} unsuccessful: #{e}"
+    nil
   end
 
   private
@@ -57,7 +61,9 @@ class ResolveRemoteAccountService < BaseService
     @webfinger.link('http://schemas.google.com/g/2010#updates-from').nil? ||
       @webfinger.link('salmon').nil? ||
       @webfinger.link('http://webfinger.net/rel/profile-page').nil? ||
-      @webfinger.link('magic-public-key').nil?
+      @webfinger.link('magic-public-key').nil? ||
+      canonical_uri.nil? ||
+      hub_url.nil?
   end
 
   def webfinger_update_due?
@@ -123,19 +129,14 @@ class ResolveRemoteAccountService < BaseService
       author_uri = owner.at_xpath('./xmlns:uri') unless owner.nil?
     end
 
-    raise Goldfinger::Error, 'Author URI could not be found' if author_uri.nil?
+    @canonical_uri = author_uri.nil? ? nil : author_uri.content
-
-    @canonical_uri = author_uri.content
   end
 
   def hub_url
     return @hub_url if defined?(@hub_url)
 
     hubs     = atom.xpath('//xmlns:link[@rel="hub"]')
-
+    @hub_url = hubs.empty? || hubs.first['href'].nil? ? nil : hubs.first['href']
-    raise Goldfinger::Error, 'No PubSubHubbub hubs found' if hubs.empty? || hubs.first['href'].nil?
-
-    @hub_url = hubs.first['href']
   end
 
   def atom_body
@@ -143,7 +144,7 @@ class ResolveRemoteAccountService < BaseService
 
     response = Request.new(:get, atom_url).perform
 
-    raise Goldfinger::Error, "Feed attempt failed for #{atom_url}: HTTP #{response.code}" unless response.code == 200
+    raise Mastodon::UnexpectedResponseError, response unless response.code == 200
 
     @atom_body = response.to_s
   end

app/services/send_interaction_service.rb

@@ -14,7 +14,7 @@ class SendInteractionService < BaseService
 
     delivery = build_request.perform
 
-    raise "Delivery failed for #{target_account.salmon_url}: HTTP #{delivery.code}" unless delivery.code > 199 && delivery.code < 300
+    raise Mastodon::UnexpectedResponseError, delivery unless delivery.code > 199 && delivery.code < 300
   end
 
   private

app/services/subscribe_service.rb

@@ -18,7 +18,7 @@ class SubscribeService < BaseService
     else
       # The response was either a 429 rate limit, or a 5xx error.
       # We need to retry at a later time. Fail loudly!
-      raise "Subscription attempt failed for #{@account.acct} (#{@account.hub_url}): HTTP #{@response.code}"
+      raise Mastodon::UnexpectedResponseError, @response
     end
   end
 

app/workers/import_worker.rb

@@ -44,7 +44,7 @@ class ImportWorker
         target_account = ResolveRemoteAccountService.new.call(row.first)
         next if target_account.nil?
         MuteService.new.call(from_account, target_account)
-      rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError
+      rescue Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
         next
       end
     end
@@ -56,7 +56,7 @@ class ImportWorker
         target_account = ResolveRemoteAccountService.new.call(row.first)
         next if target_account.nil?
         BlockService.new.call(from_account, target_account)
-      rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError
+      rescue Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
         next
       end
     end
@@ -66,7 +66,7 @@ class ImportWorker
     import_rows.each do |row|
       begin
         FollowService.new.call(from_account, row.first)
-      rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError
+      rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
         next
       end
     end

app/workers/pubsubhubbub/delivery_worker.rb

@@ -23,7 +23,7 @@ class Pubsubhubbub::DeliveryWorker
   def process_delivery
     payload_delivery
 
-    raise "Delivery failed for #{subscription.callback_url}: HTTP #{payload_delivery.code}" unless response_successful?
+    raise Mastodon::UnexpectedResponseError, payload_delivery unless response_successful?
 
     subscription.touch(:last_successful_delivery_at)
   end

spec/controllers/api/base_controller_spec.rb

@@ -32,7 +32,7 @@ describe Api::BaseController do
       ActiveRecord::RecordInvalid => 422,
       Mastodon::ValidationError => 422,
       ActiveRecord::RecordNotFound => 404,
-      Goldfinger::Error => 422,
+      Mastodon::UnexpectedResponseError => 503,
       HTTP::Error => 503,
       OpenSSL::SSL::SSLError => 503,
       Mastodon::NotPermittedError => 403,

spec/services/resolve_remote_account_service_spec.rb

@@ -22,11 +22,11 @@ RSpec.describe ResolveRemoteAccountService do
   end
 
   it 'raises error if no such user can be resolved via webfinger' do
-    expect { subject.call('catsrgr8@quitter.no') }.to raise_error Goldfinger::Error
+    expect(subject.call('catsrgr8@quitter.no')).to be_nil
   end
 
   it 'raises error if the domain does not have webfinger' do
-    expect { subject.call('catsrgr8@example.com') }.to raise_error Goldfinger::Error
+    expect(subject.call('catsrgr8@example.com')).to be_nil
   end
 
   it 'returns an already existing remote account' do
@@ -58,7 +58,7 @@ RSpec.describe ResolveRemoteAccountService do
   end
 
   it 'prevents hijacking inexisting accounts' do
-    expect { subject.call('hacker2@redirected.com') }.to raise_error Goldfinger::Error
+    expect(subject.call('hacker2@redirected.com')).to be_nil
   end
 
   it 'returns a new remote account' do

spec/services/subscribe_service_spec.rb

@@ -33,11 +33,11 @@ RSpec.describe SubscribeService do
 
   it 'fails loudly if PuSH hub is unavailable' do
     stub_request(:post, 'http://hub.example.com/').to_return(status: 503)
-    expect { subject.call(account) }.to raise_error(/Subscription attempt failed/)
+    expect { subject.call(account) }.to raise_error Mastodon::UnexpectedResponseError
   end
 
   it 'fails loudly if rate limited' do
     stub_request(:post, 'http://hub.example.com/').to_return(status: 429)
-    expect { subject.call(account) }.to raise_error(/Subscription attempt failed/)
+    expect { subject.call(account) }.to raise_error Mastodon::UnexpectedResponseError
   end
 end

spec/workers/pubsubhubbub/delivery_worker_spec.rb

@@ -26,7 +26,7 @@ describe Pubsubhubbub::DeliveryWorker do
       subscription = Fabricate(:subscription)
 
       stub_request_to_respond_with(subscription, 500)
-      expect { subject.perform(subscription.id, payload) }.to raise_error(/Delivery failed/)
+      expect { subject.perform(subscription.id, payload) }.to raise_error Mastodon::UnexpectedResponseError
     end
 
     it 'updates subscriptions when delivery succeeds' do