From 7d985f2aac639dc5fae528db2dbc4422ca10f276 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 3a92b4627da..0fc631e2aca 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 ae304e61e5c..21f5b1b4cc4 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 ab7ca469811..482f4e19eab 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 00000000000..b2374c49418 --- /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 792e9e8d4da..3d659142a05 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 681b5b2cd0b..4fae98ed726 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 a7961f8e8aa..19ee9faedd0 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 30b84f7d529..911c067133c 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 83fbf6d07d3..e5bd0c47c8e 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 12f0f1b0852..0a710f84361 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 ba77552c6c0..3f7bb7cc52c 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 0a6bdc322fb..b4e867c5f88 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 3ef8f14d9f6..7312dde582e 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 b43ae19d87a..643ba9cd328 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 d9c17470f5a..7d775a86d7b 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