From adaf3f7b7a54667e0d3ad840492ea4c72427acf5 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 8 Oct 2020 00:34:57 +0200 Subject: [PATCH] Remove dependency on goldfinger gem (#14919) There are edge cases where requests to certain hosts timeout when using the vanilla HTTP.rb gem, which the goldfinger gem uses. Now that we no longer need to support OStatus servers, webfinger logic is so simple that there is no point encapsulating it in a gem, so we can just use our own Request class. With that, we benefit from more robust timeout code and IPv4/IPv6 resolution. Fix #14091 --- Gemfile | 1 - Gemfile.lock | 6 -- app/helpers/webfinger_helper.rb | 33 +------ app/lib/webfinger.rb | 93 +++++++++++++++++++ app/models/account_alias.rb | 2 +- app/models/account_migration.rb | 2 +- app/models/form/redirect.rb | 2 +- app/models/remote_follow.rb | 10 +- .../fetch_remote_account_service.rb | 7 +- app/services/process_mentions_service.rb | 2 +- app/services/resolve_account_service.rb | 9 +- app/views/well_known/host_meta/show.xml.ruby | 1 - .../remote_follow_controller_spec.rb | 10 +- .../well_known/host_meta_controller_spec.rb | 2 +- spec/lib/feed_manager_spec.rb | 1 + 15 files changed, 114 insertions(+), 67 deletions(-) create mode 100644 app/lib/webfinger.rb diff --git a/Gemfile b/Gemfile index 3a92b4627d..0fc631e2ac 100644 --- a/Gemfile +++ b/Gemfile @@ -54,7 +54,6 @@ gem 'doorkeeper', '~> 5.4' gem 'ed25519', '~> 1.2' gem 'fast_blank', '~> 1.0' gem 'fastimage' -gem 'goldfinger', '~> 2.1' gem 'hiredis', '~> 0.6' gem 'redis-namespace', '~> 1.8' gem 'health_check', git: 'https://github.com/ianheggie/health_check', ref: '0b799ead604f900ed50685e9b2d469cd2befba5b' diff --git a/Gemfile.lock b/Gemfile.lock index ae304e61e5..21f5b1b4cc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -240,11 +240,6 @@ GEM ruby-progressbar (~> 1.4) globalid (0.4.2) activesupport (>= 4.2.0) - goldfinger (2.1.1) - addressable (~> 2.5) - http (~> 4.0) - nokogiri (~> 1.8) - oj (~> 3.0) hamlit (2.13.0) temple (>= 0.8.2) thor @@ -714,7 +709,6 @@ DEPENDENCIES fog-core (<= 2.1.0) fog-openstack (~> 0.3) fuubar (~> 2.5) - goldfinger (~> 2.1) hamlit-rails (~> 0.2) health_check! hiredis (~> 0.6) diff --git a/app/helpers/webfinger_helper.rb b/app/helpers/webfinger_helper.rb index ab7ca46981..482f4e19ea 100644 --- a/app/helpers/webfinger_helper.rb +++ b/app/helpers/webfinger_helper.rb @@ -1,38 +1,7 @@ # frozen_string_literal: true -# Monkey-patch on monkey-patch. -# Because it conflicts with the request.rb patch. -class HTTP::Timeout::PerOperationOriginal < HTTP::Timeout::PerOperation - def connect(socket_class, host, port, nodelay = false) - ::Timeout.timeout(@connect_timeout, HTTP::TimeoutError) do - @socket = socket_class.open(host, port) - @socket.setsockopt(Socket::IPPROTO_TCP, Socket::TCP_NODELAY, 1) if nodelay - end - end -end - module WebfingerHelper def webfinger!(uri) - hidden_service_uri = /\.(onion|i2p)(:\d+)?$/.match(uri) - - raise Mastodon::HostValidationError, 'Instance does not support hidden service connections' if !Rails.configuration.x.access_to_hidden_service && hidden_service_uri - - opts = { - ssl: !hidden_service_uri, - - headers: { - 'User-Agent': Mastodon::Version.user_agent, - }, - - timeout_class: HTTP::Timeout::PerOperationOriginal, - - timeout_options: { - write_timeout: 10, - connect_timeout: 5, - read_timeout: 10, - }, - } - - Goldfinger::Client.new(uri, opts.merge(Rails.configuration.x.http_client_proxy)).finger + Webfinger.new(uri).perform end end diff --git a/app/lib/webfinger.rb b/app/lib/webfinger.rb new file mode 100644 index 0000000000..b2374c4941 --- /dev/null +++ b/app/lib/webfinger.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +class Webfinger + class Error < StandardError; end + + class Response + def initialize(body) + @json = Oj.load(body, mode: :strict) + end + + def subject + @json['subject'] + end + + def link(rel, attribute) + links.dig(rel, attribute) + end + + private + + def links + @links ||= @json['links'].map { |link| [link['rel'], link] }.to_h + end + end + + def initialize(uri) + _, @domain = uri.split('@') + + raise ArgumentError, 'Webfinger requested for local account' if @domain.nil? + + @uri = uri + end + + def perform + Response.new(body_from_webfinger) + rescue Oj::ParseError + raise Webfinger::Error, "Invalid JSON in response for #{@uri}" + rescue Addressable::URI::InvalidURIError + raise Webfinger::Error, "Invalid URI for #{@uri}" + end + + private + + def body_from_webfinger(url = standard_url, use_fallback = true) + webfinger_request(url).perform do |res| + if res.code == 200 + res.body_with_limit + elsif res.code == 404 && use_fallback + body_from_host_meta + else + raise Webfinger::Error, "Request for #{@uri} returned HTTP #{res.code}" + end + end + end + + def body_from_host_meta + host_meta_request.perform do |res| + if res.code == 200 + body_from_webfinger(url_from_template(res.body_with_limit), false) + else + raise Webfinger::Error, "Request for #{@uri} returned HTTP #{res.code}" + end + end + end + + def url_from_template(str) + link = Nokogiri::XML(str).at_xpath('//xmlns:Link[@rel="lrdd"]') + + if link.present? + link['template'].gsub('{uri}', @uri) + else + raise Webfinger::Error, "Request for #{@uri} returned host-meta without link to Webfinger" + end + rescue Nokogiri::XML::XPath::SyntaxError + raise Webfinger::Error, "Invalid XML encountered in host-meta for #{@uri}" + end + + def host_meta_request + Request.new(:get, host_meta_url).add_headers('Accept' => 'application/xrd+xml, application/xml, text/xml') + end + + def webfinger_request(url) + Request.new(:get, url).add_headers('Accept' => 'application/jrd+json, application/json') + end + + def standard_url + "https://#{@domain}/.well-known/webfinger?resource=#{@uri}" + end + + def host_meta_url + "https://#{@domain}/.well-known/host-meta" + end +end diff --git a/app/models/account_alias.rb b/app/models/account_alias.rb index 792e9e8d4d..3d659142a0 100644 --- a/app/models/account_alias.rb +++ b/app/models/account_alias.rb @@ -33,7 +33,7 @@ class AccountAlias < ApplicationRecord def set_uri target_account = ResolveAccountService.new.call(acct) self.uri = ActivityPub::TagManager.instance.uri_for(target_account) unless target_account.nil? - rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::Error + rescue Webfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::Error # Validation will take care of it end diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb index 681b5b2cd0..4fae98ed72 100644 --- a/app/models/account_migration.rb +++ b/app/models/account_migration.rb @@ -54,7 +54,7 @@ class AccountMigration < ApplicationRecord def set_target_account self.target_account = ResolveAccountService.new.call(acct) - rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::Error + rescue Webfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::Error # Validation will take care of it end diff --git a/app/models/form/redirect.rb b/app/models/form/redirect.rb index a7961f8e8a..19ee9faedd 100644 --- a/app/models/form/redirect.rb +++ b/app/models/form/redirect.rb @@ -32,7 +32,7 @@ class Form::Redirect def set_target_account @target_account = ResolveAccountService.new.call(acct) - rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::Error + rescue Webfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::Error # Validation will take care of it end diff --git a/app/models/remote_follow.rb b/app/models/remote_follow.rb index 30b84f7d52..911c067133 100644 --- a/app/models/remote_follow.rb +++ b/app/models/remote_follow.rb @@ -56,7 +56,7 @@ class RemoteFollow if domain.nil? @addressable_template = Addressable::Template.new("#{authorize_interaction_url}?uri={uri}") - elsif redirect_url_link.nil? || redirect_url_link.template.nil? + elsif redirect_uri_template.nil? missing_resource_error else @addressable_template = Addressable::Template.new(redirect_uri_template) @@ -64,16 +64,12 @@ class RemoteFollow end def redirect_uri_template - redirect_url_link.template - end - - def redirect_url_link - acct_resource&.link('http://ostatus.org/schema/1.0/subscribe') + acct_resource&.link('http://ostatus.org/schema/1.0/subscribe', 'template') end def acct_resource @acct_resource ||= webfinger!("acct:#{acct}") - rescue Goldfinger::Error, HTTP::ConnectionError + rescue Webfinger::Error, HTTP::ConnectionError nil end diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index 83fbf6d07d..e5bd0c47c8 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -39,17 +39,16 @@ class ActivityPub::FetchRemoteAccountService < BaseService 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? + return webfinger.link('self', 'href') == @uri if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? webfinger = webfinger!("acct:#{confirmed_username}@#{confirmed_domain}") @username, @domain = split_acct(webfinger.subject) - self_reference = webfinger.link('self') return false unless @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? - return false if self_reference&.href != @uri + return false if webfinger.link('self', 'href') != @uri true - rescue Goldfinger::Error + rescue Webfinger::Error false end diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb index 12f0f1b085..0a710f8436 100644 --- a/app/services/process_mentions_service.rb +++ b/app/services/process_mentions_service.rb @@ -29,7 +29,7 @@ class ProcessMentionsService < BaseService if mention_undeliverable?(mentioned_account) begin mentioned_account = resolve_account_service.call(Regexp.last_match(1)) - rescue Goldfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::UnexpectedResponseError + rescue Webfinger::Error, HTTP::Error, OpenSSL::SSL::SSLError, Mastodon::UnexpectedResponseError mentioned_account = nil end end diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index ba77552c6c..3f7bb7cc52 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -26,11 +26,10 @@ class ResolveAccountService < BaseService @account ||= Account.find_remote(@username, @domain) - return @account if @account&.local? || !webfinger_update_due? + return @account if @account&.local? || @domain.nil? || !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!(@uri) # Because the username/domain pair may be different than what @@ -47,7 +46,7 @@ class ResolveAccountService < BaseService # either needs to be created, or updated from fresh data process_account! - rescue Goldfinger::Error, WebfingerRedirectError, Oj::ParseError => e + rescue Webfinger::Error, WebfingerRedirectError, Oj::ParseError => e Rails.logger.debug "Webfinger query for #{@uri} failed: #{e}" nil end @@ -118,11 +117,11 @@ class ResolveAccountService < BaseService end def activitypub_ready? - !@webfinger.link('self').nil? && ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@webfinger.link('self').type) + ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@webfinger.link('self', 'type')) end def actor_url - @actor_url ||= @webfinger.link('self').href + @actor_url ||= @webfinger.link('self', 'href') end def actor_json diff --git a/app/views/well_known/host_meta/show.xml.ruby b/app/views/well_known/host_meta/show.xml.ruby index 0a6bdc322f..b4e867c5f8 100644 --- a/app/views/well_known/host_meta/show.xml.ruby +++ b/app/views/well_known/host_meta/show.xml.ruby @@ -5,7 +5,6 @@ doc << Ox::Element.new('XRD').tap do |xrd| xrd << Ox::Element.new('Link').tap do |link| link['rel'] = 'lrdd' - link['type'] = 'application/xrd+xml' link['template'] = @webfinger_template end end diff --git a/spec/controllers/remote_follow_controller_spec.rb b/spec/controllers/remote_follow_controller_spec.rb index 3ef8f14d9f..7312dde582 100644 --- a/spec/controllers/remote_follow_controller_spec.rb +++ b/spec/controllers/remote_follow_controller_spec.rb @@ -43,8 +43,7 @@ describe RemoteFollowController do end it 'renders new when template is nil' do - link_with_nil_template = double(template: nil) - resource_with_link = double(link: link_with_nil_template) + resource_with_link = double(link: nil) allow_any_instance_of(WebfingerHelper).to receive(:webfinger!).with('acct:user@example.com').and_return(resource_with_link) post :create, params: { account_username: @account.to_param, remote_follow: { acct: 'user@example.com' } } @@ -55,8 +54,7 @@ describe RemoteFollowController do context 'when webfinger values are good' do before do - link_with_template = double(template: 'http://example.com/follow_me?acct={uri}') - resource_with_link = double(link: link_with_template) + resource_with_link = double(link: 'http://example.com/follow_me?acct={uri}') allow_any_instance_of(WebfingerHelper).to receive(:webfinger!).with('acct:user@example.com').and_return(resource_with_link) post :create, params: { account_username: @account.to_param, remote_follow: { acct: 'user@example.com' } } end @@ -78,8 +76,8 @@ describe RemoteFollowController do expect(response).to render_template(:new) end - it 'renders new with error when goldfinger fails' do - allow_any_instance_of(WebfingerHelper).to receive(:webfinger!).with('acct:user@example.com').and_raise(Goldfinger::Error) + it 'renders new with error when webfinger fails' do + allow_any_instance_of(WebfingerHelper).to receive(:webfinger!).with('acct:user@example.com').and_raise(Webfinger::Error) post :create, params: { account_username: @account.to_param, remote_follow: { acct: 'user@example.com' } } expect(response).to render_template(:new) diff --git a/spec/controllers/well_known/host_meta_controller_spec.rb b/spec/controllers/well_known/host_meta_controller_spec.rb index b43ae19d87..643ba9cd32 100644 --- a/spec/controllers/well_known/host_meta_controller_spec.rb +++ b/spec/controllers/well_known/host_meta_controller_spec.rb @@ -12,7 +12,7 @@ describe WellKnown::HostMetaController, type: :controller do expect(response.body).to eq < - + XML end diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb index d9c17470f5..7d775a86d7 100644 --- a/spec/lib/feed_manager_spec.rb +++ b/spec/lib/feed_manager_spec.rb @@ -108,6 +108,7 @@ RSpec.describe FeedManager do it 'returns false for status by followee mentioning another account' do bob.follow!(alice) + jeff.follow!(alice) status = PostStatusService.new.call(alice, text: 'Hey @jeff') expect(FeedManager.instance.filter?(:home, status, bob)).to be false end