From bafd22ecf487774c252a271d668716b0e1c84c6c Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 3 May 2017 17:02:18 +0200 Subject: [PATCH] Fix #2706 - Always respond with 200 to PuSH payloads (#2733) Fix #2196 - Respond with 201 when Salmon accepted, 400 when unverified Fix #2629 - Correctly handle confirm_domain? for local accounts Unify rules for extracting author acct from XML, prefer , fall back to + (see also #2017, #2172) --- app/controllers/api/salmon_controller.rb | 14 ++++++--- .../api/subscriptions_controller.rb | 5 ++- app/services/concerns/author_extractor.rb | 21 +++++++++++++ app/services/fetch_remote_account_service.rb | 17 +++------- app/services/fetch_remote_status_service.rb | 28 ++++------------- app/services/process_feed_service.rb | 16 ++-------- app/services/process_interaction_service.rb | 31 +++---------------- app/services/verify_salmon_service.rb | 26 ++++++++++++++++ .../api/subscriptions_controller_spec.rb | 4 +-- 9 files changed, 77 insertions(+), 85 deletions(-) create mode 100644 app/services/concerns/author_extractor.rb create mode 100644 app/services/verify_salmon_service.rb diff --git a/app/controllers/api/salmon_controller.rb b/app/controllers/api/salmon_controller.rb index a7872d5422..7fc5e548d6 100644 --- a/app/controllers/api/salmon_controller.rb +++ b/app/controllers/api/salmon_controller.rb @@ -5,13 +5,13 @@ class Api::SalmonController < ApiController respond_to :txt def update - body = request.body.read + payload = request.body.read - if body.nil? - head 200 - else - SalmonWorker.perform_async(@account.id, body.force_encoding('UTF-8')) + if !payload.nil? && verify?(payload) + SalmonWorker.perform_async(@account.id, payload.force_encoding('UTF-8')) head 201 + else + head 202 end end @@ -20,4 +20,8 @@ class Api::SalmonController < ApiController def set_account @account = Account.find(params[:id]) end + + def verify?(payload) + VerifySalmonService.new.call(payload) + end end diff --git a/app/controllers/api/subscriptions_controller.rb b/app/controllers/api/subscriptions_controller.rb index 51c4764362..135a5632e6 100644 --- a/app/controllers/api/subscriptions_controller.rb +++ b/app/controllers/api/subscriptions_controller.rb @@ -19,10 +19,9 @@ class Api::SubscriptionsController < ApiController if subscription.verify(body, request.headers['HTTP_X_HUB_SIGNATURE']) ProcessingWorker.perform_async(@account.id, body.force_encoding('UTF-8')) - head 201 - else - head 202 end + + head 200 end private diff --git a/app/services/concerns/author_extractor.rb b/app/services/concerns/author_extractor.rb new file mode 100644 index 0000000000..d99780e7d9 --- /dev/null +++ b/app/services/concerns/author_extractor.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module AuthorExtractor + def author_from_xml(xml) + # Try for acct + acct = xml.at_xpath('./xmlns:author/xmlns:email', xmlns: TagManager::XMLNS)&.content + + # Try + + if acct.blank? + username = xml.at_xpath('./xmlns:author/xmlns:name', xmlns: TagManager::XMLNS)&.content + uri = xml.at_xpath('./xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS)&.content + + return nil if username.blank? || uri.blank? + + domain = Addressable::URI.parse(uri).normalize.host + acct = "#{username}@#{domain}" + end + + FollowRemoteAccountService.new.call(acct) + end +end diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb index 7c2fb0ef12..98fdf3dfe9 100644 --- a/app/services/fetch_remote_account_service.rb +++ b/app/services/fetch_remote_account_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class FetchRemoteAccountService < BaseService + include AuthorExtractor + def call(url, prefetched_body = nil) if prefetched_body.nil? atom_url, body = FetchAtomService.new.call(url) @@ -19,21 +21,10 @@ class FetchRemoteAccountService < BaseService xml = Nokogiri::XML(body) xml.encoding = 'utf-8' - email = xml.at_xpath('//xmlns:author/xmlns:email').try(:content) - if email.nil? - url_parts = Addressable::URI.parse(url).normalize - username = xml.at_xpath('//xmlns:author/xmlns:name').try(:content) - domain = url_parts.host - else - username, domain = email.split('@') - end + account = author_from_xml(xml.at_xpath('/xmlns:feed', xmlns: TagManager::XMLNS)) - return nil if username.nil? || domain.nil? - - Rails.logger.debug "Going to webfinger #{username}@#{domain}" - - account = FollowRemoteAccountService.new.call("#{username}@#{domain}") UpdateRemoteProfileService.new.call(xml, account) unless account.nil? + account rescue TypeError Rails.logger.debug "Unparseable URL given: #{url}" diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb index 5a454808e5..f414813ada 100644 --- a/app/services/fetch_remote_status_service.rb +++ b/app/services/fetch_remote_status_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class FetchRemoteStatusService < BaseService + include AuthorExtractor + def call(url, prefetched_body = nil) if prefetched_body.nil? atom_url, body = FetchAtomService.new.call(url) @@ -21,37 +23,19 @@ class FetchRemoteStatusService < BaseService xml = Nokogiri::XML(body) xml.encoding = 'utf-8' - account = extract_author(url, xml) + account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS)) + domain = Addressable::URI.parse(url).normalize.host - return nil if account.nil? + return nil unless !account.nil? && confirmed_domain?(domain, account) statuses = ProcessFeedService.new.call(body, account) - statuses.first - end - - def extract_author(url, xml) - url_parts = Addressable::URI.parse(url).normalize - username = xml.at_xpath('//xmlns:author/xmlns:name').try(:content) - domain = url_parts.host - - return nil if username.nil? - - Rails.logger.debug "Going to webfinger #{username}@#{domain}" - - account = FollowRemoteAccountService.new.call("#{username}@#{domain}") - - # If the author's confirmed URLs do not match the domain of the URL - # we are reading this from, abort - return nil unless confirmed_domain?(domain, account) - - account rescue Nokogiri::XML::XPath::SyntaxError Rails.logger.debug 'Invalid XML or missing namespace' nil end def confirmed_domain?(domain, account) - domain.casecmp(account.domain).zero? || domain.casecmp(Addressable::URI.parse(account.remote_url).normalize.host).zero? + account.domain.nil? || domain.casecmp(account.domain).zero? || domain.casecmp(Addressable::URI.parse(account.remote_url).normalize.host).zero? end end diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb index 7a27b7b298..4d23a62621 100644 --- a/app/services/process_feed_service.rb +++ b/app/services/process_feed_service.rb @@ -20,6 +20,8 @@ class ProcessFeedService < BaseService end class ProcessEntry + include AuthorExtractor + def call(xml, account) @account = account @xml = xml @@ -108,7 +110,7 @@ class ProcessFeedService < BaseService # If that author cannot be found, don't record the status (do not misattribute) if account?(entry) begin - account = find_or_resolve_account(acct(entry)) + account = author_from_xml(entry) return [nil, false] if account.nil? rescue Goldfinger::Error return [nil, false] @@ -143,10 +145,6 @@ class ProcessFeedService < BaseService [status, true] end - def find_or_resolve_account(acct) - FollowRemoteAccountService.new.call(acct) - end - def find_or_resolve_status(parent, uri, url) status = find_status(uri) @@ -275,13 +273,5 @@ class ProcessFeedService < BaseService def account?(xml = @xml) !xml.at_xpath('./xmlns:author', xmlns: TagManager::XMLNS).nil? end - - def acct(xml = @xml) - username = xml.at_xpath('./xmlns:author/xmlns:name', xmlns: TagManager::XMLNS).content - url = xml.at_xpath('./xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS).content - domain = Addressable::URI.parse(url).normalize.host - - "#{username}@#{domain}" - end end end diff --git a/app/services/process_interaction_service.rb b/app/services/process_interaction_service.rb index 410e805d34..1f15a265d5 100644 --- a/app/services/process_interaction_service.rb +++ b/app/services/process_interaction_service.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class ProcessInteractionService < BaseService + include AuthorExtractor + # Record locally the remote interaction with our user # @param [String] envelope Salmon envelope # @param [Account] target_account Account the Salmon was addressed to @@ -10,18 +12,9 @@ class ProcessInteractionService < BaseService xml = Nokogiri::XML(body) xml.encoding = 'utf-8' - return unless contains_author?(xml) + account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS)) - username = xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:name', xmlns: TagManager::XMLNS).content - url = xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS).content - domain = Addressable::URI.parse(url).normalize.host - account = Account.find_by(username: username, domain: domain) - - if account.nil? - account = follow_remote_account_service.call("#{username}@#{domain}") - end - - return if account.suspended? + return if account.nil? || account.suspended? if salmon.verify(envelope, account.keypair) RemoteProfileUpdateWorker.perform_async(account.id, body.force_encoding('UTF-8'), true) @@ -59,10 +52,6 @@ class ProcessInteractionService < BaseService private - def contains_author?(xml) - !(xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:name', xmlns: TagManager::XMLNS).nil? || xml.at_xpath('/xmlns:entry/xmlns:author/xmlns:uri', xmlns: TagManager::XMLNS).nil?) - end - def mentions_account?(xml, account) xml.xpath('/xmlns:entry/xmlns:link[@rel="mentioned"]', xmlns: TagManager::XMLNS).each { |mention_link| return true if [TagManager.instance.uri_for(account), TagManager.instance.url_for(account)].include?(mention_link.attribute('href').value) } false @@ -144,16 +133,4 @@ class ProcessInteractionService < BaseService def salmon @salmon ||= OStatus2::Salmon.new end - - def follow_remote_account_service - @follow_remote_account_service ||= FollowRemoteAccountService.new - end - - def process_feed_service - @process_feed_service ||= ProcessFeedService.new - end - - def remove_status_service - @remove_status_service ||= RemoveStatusService.new - end end diff --git a/app/services/verify_salmon_service.rb b/app/services/verify_salmon_service.rb new file mode 100644 index 0000000000..cd674837dc --- /dev/null +++ b/app/services/verify_salmon_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +class VerifySalmonService < BaseService + include AuthorExtractor + + def call(payload) + body = salmon.unpack(payload) + + xml = Nokogiri::XML(body) + xml.encoding = 'utf-8' + + account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS)) + + if account.nil? + false + else + salmon.verify(payload, account.keypair) + end + end + + private + + def salmon + @salmon ||= OStatus2::Salmon.new + end +end diff --git a/spec/controllers/api/subscriptions_controller_spec.rb b/spec/controllers/api/subscriptions_controller_spec.rb index 44841176a0..97e36420d6 100644 --- a/spec/controllers/api/subscriptions_controller_spec.rb +++ b/spec/controllers/api/subscriptions_controller_spec.rb @@ -45,8 +45,8 @@ RSpec.describe Api::SubscriptionsController, type: :controller do post :update, params: { id: account.id } end - it 'returns http created' do - expect(response).to have_http_status(:created) + it 'returns http success' do + expect(response).to have_http_status(:success) end it 'creates statuses for feed' do