From 0c7c188c459117770ac1f74f70a9e65ed2be606f Mon Sep 17 00:00:00 2001 From: Sorin Davidoi Date: Thu, 13 Jul 2017 22:15:32 +0200 Subject: Web Push Notifications (#3243) * feat: Register push subscription * feat: Notify when mentioned * feat: Boost, favourite, reply, follow, follow request * feat: Notification interaction * feat: Handle change of public key * feat: Unsubscribe if things go wrong * feat: Do not send normal notifications if push is enabled * feat: Focus client if open * refactor: Move push logic to WebPushSubscription * feat: Better title and body * feat: Localize messages * chore: Fix lint errors * feat: Settings * refactor: Lazy load * fix: Check if push settings exist * feat: Device-based preferences * refactor: Simplify logic * refactor: Pull request feedback * refactor: Pull request feedback * refactor: Create /api/web/push_subscriptions endpoint * feat: Spec PushSubscriptionController * refactor: WebPushSubscription => Web::PushSubscription * feat: Spec Web::PushSubscription * feat: Display first media attachment * feat: Support direction * fix: Stuff broken while rebasing * refactor: Integration with session activations * refactor: Cleanup * refactor: Simplify implementation * feat: Set VAPID keys via environment * chore: Comments * fix: Crash when no alerts * fix: Set VAPID keys in testing environment * fix: Follow link * feat: Notification actions * fix: Delete previous subscription * chore: Temporary logs * refactor: Move migration to a later date * fix: Fetch the correct session activation and misc bugs * refactor: Move migration to a later date * fix: Remove follow request (no notifications) * feat: Send administrator contact to push service * feat: Set time-to-live * fix: Do not show sensitive images * fix: Reducer crash in error handling * feat: Add badge * chore: Fix lint error * fix: Checkbox label overlap * fix: Check for payload support * fix: Rename action "type" (crash in latest Chrome) * feat: Action to expand notification * fix: Lint errors * fix: Unescape notification body * fix: Do not allow boosting if the status is hidden * feat: Add VAPID keys to the production sample environment * fix: Strip HTML tags from status * refactor: Better error messages * refactor: Handle browser not implementing the VAPID protocol (Samsung Internet) * fix: Error when target_status is nil * fix: Handle lack of image * fix: Delete reference to invalid subscriptions * feat: Better error handling * fix: Unescape HTML characters after tags are striped * refactor: Simpify code * fix: Modify to work with #4091 * Sort strings alphabetically * i18n: Updated Polish translation it annoys me that it's not fully localized :P * refactor: Use current_session in PushSubscriptionController * fix: Rebase mistake * fix: Set cacheName to mastodon * refactor: Pull request feedback * refactor: Remove logging statements * chore(yarn): Fix conflicts with master * chore(yarn): Copy latest from master * chore(yarn): Readd offline-plugin * refactor: Use save! and update! * refactor: Send notifications async * fix: Allow retry when push fails * fix: Save track for failed pushes * fix: Minify sw.js * fix: Remove account_id from fabricator --- app/workers/web_push_notification_worker.rb | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 app/workers/web_push_notification_worker.rb (limited to 'app/workers') diff --git a/app/workers/web_push_notification_worker.rb b/app/workers/web_push_notification_worker.rb new file mode 100644 index 000000000..0568a3e02 --- /dev/null +++ b/app/workers/web_push_notification_worker.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class WebPushNotificationWorker + include Sidekiq::Worker + + sidekiq_options backtrace: true + + def perform(recipient_id, notification_id) + recipient = Account.find(recipient_id) + notification = Notification.find(notification_id) + + sessions_with_subscriptions = recipient.user.session_activations.reject { |session| session.web_push_subscription.nil? } + + sessions_with_subscriptions.each do |session| + begin + session.web_push_subscription.push(notification) + rescue Webpush::InvalidSubscription, Webpush::ExpiredSubscription + # Subscription expiration is not currently implemented in any browser + session.web_push_subscription.destroy! + session.web_push_subscription = nil + session.save! + rescue Webpush::PayloadTooLarge => e + Rails.logger.error(e) + end + end + end +end -- cgit From 1618b68bfa740ed655ac45d7d5f4f46fed6c8c62 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 14 Jul 2017 20:41:49 +0200 Subject: HTTP signatures (#4146) * Add Request class with HTTP signature generator Spec: https://tools.ietf.org/html/draft-cavage-http-signatures-06 * Add HTTP signature verification concern * Add test for SignatureVerification concern * Add basic test for Request class * Make PuSH subscribe/unsubscribe requests use new Request class Accidentally fix lease_seconds not being set and sent properly, and change the new minimum subscription duration to 1 day * Make all PuSH workers use new Request class * Make Salmon sender use new Request class * Make FetchLinkService use new Request class * Make FetchAtomService use the new Request class * Make Remotable use the new Request class * Make ResolveRemoteAccountService use the new Request class * Add more tests * Allow +-30 seconds window for signed request to remain valid * Disable time window validation for signed requests, restore 7 days as PuSH subscription duration (which was previous default due to a bug) --- app/controllers/accounts_controller.rb | 1 + app/controllers/api/subscriptions_controller.rb | 2 +- app/controllers/concerns/signature_verification.rb | 87 ++++++++++++++++++++++ app/controllers/stream_entries_controller.rb | 1 + app/helpers/http_helper.rb | 17 ----- app/lib/provider_discovery.rb | 4 +- app/lib/request.rb | 70 +++++++++++++++++ app/models/account.rb | 2 +- app/models/concerns/remotable.rb | 3 +- app/models/subscription.rb | 4 +- app/services/fetch_atom_service.rb | 8 +- app/services/fetch_link_card_service.rb | 6 +- app/services/resolve_remote_account_service.rb | 3 +- app/services/send_interaction_service.rb | 14 +++- app/services/subscribe_service.rb | 48 ++++++++---- app/services/unsubscribe_service.rb | 31 +++++--- app/workers/pubsubhubbub/confirmation_worker.rb | 12 ++- app/workers/pubsubhubbub/delivery_worker.rb | 11 ++- .../concerns/signature_verification_spec.rb | 74 ++++++++++++++++++ spec/helpers/http_helper_spec.rb | 13 ---- spec/lib/request_spec.rb | 54 ++++++++++++++ .../pubsubhubbub/confirmation_worker_spec.rb | 2 +- spec/workers/pubsubhubbub/delivery_worker_spec.rb | 2 +- 23 files changed, 379 insertions(+), 90 deletions(-) create mode 100644 app/controllers/concerns/signature_verification.rb delete mode 100644 app/helpers/http_helper.rb create mode 100644 app/lib/request.rb create mode 100644 spec/controllers/concerns/signature_verification_spec.rb delete mode 100644 spec/helpers/http_helper_spec.rb create mode 100644 spec/lib/request_spec.rb (limited to 'app/workers') diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 11402ab79..69b520df1 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -2,6 +2,7 @@ class AccountsController < ApplicationController include AccountControllerConcern + include SignatureVerification def show respond_to do |format| diff --git a/app/controllers/api/subscriptions_controller.rb b/app/controllers/api/subscriptions_controller.rb index d3ea98676..89007f3d6 100644 --- a/app/controllers/api/subscriptions_controller.rb +++ b/app/controllers/api/subscriptions_controller.rb @@ -42,7 +42,7 @@ class Api::SubscriptionsController < Api::BaseController end def lease_seconds_or_default - (params['hub.lease_seconds'] || 86_400).to_i.seconds + (params['hub.lease_seconds'] || 1.day).to_i.seconds end def set_account diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb new file mode 100644 index 000000000..abe845d93 --- /dev/null +++ b/app/controllers/concerns/signature_verification.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +# Implemented according to HTTP signatures (Draft 6) +# +module SignatureVerification + extend ActiveSupport::Concern + + def signed_request? + request.headers['Signature'].present? + end + + def signed_request_account + return @signed_request_account if defined?(@signed_request_account) + + unless signed_request? + @signed_request_account = nil + return + end + + raw_signature = request.headers['Signature'] + signature_params = {} + + raw_signature.split(',').each do |part| + parsed_parts = part.match(/([a-z]+)="([^"]+)"/i) + next if parsed_parts.nil? || parsed_parts.size != 3 + signature_params[parsed_parts[1]] = parsed_parts[2] + end + + if incompatible_signature?(signature_params) + @signed_request_account = nil + return + end + + account = ResolveRemoteAccountService.new.call(signature_params['keyId'].gsub(/\Aacct:/, '')) + + if account.nil? + @signed_request_account = nil + return + end + + signature = Base64.decode64(signature_params['signature']) + compare_signed_string = build_signed_string(signature_params['headers']) + + if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string) + @signed_request_account = account + @signed_request_account + else + @signed_request_account = nil + end + end + + private + + def build_signed_string(signed_headers) + signed_headers = 'date' if signed_headers.blank? + + signed_headers.split(' ').map do |signed_header| + if signed_header == Request::REQUEST_TARGET + "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" + else + "#{signed_header}: #{request.headers[to_header_name(signed_header)]}" + end + end.join("\n") + end + + def matches_time_window? + begin + time_sent = DateTime.httpdate(request.headers['Date']) + rescue ArgumentError + return false + end + + (Time.now.utc - time_sent).abs <= 30 + end + + def to_header_name(name) + name.split(/-/).map(&:capitalize).join('-') + end + + def incompatible_signature?(signature_params) + signature_params['keyId'].blank? || + signature_params['signature'].blank? || + signature_params['algorithm'].blank? || + signature_params['algorithm'] != 'rsa-sha256' || + !signature_params['keyId'].start_with?('acct:') + end +end diff --git a/app/controllers/stream_entries_controller.rb b/app/controllers/stream_entries_controller.rb index 314d59619..54a435238 100644 --- a/app/controllers/stream_entries_controller.rb +++ b/app/controllers/stream_entries_controller.rb @@ -2,6 +2,7 @@ class StreamEntriesController < ApplicationController include Authorization + include SignatureVerification layout 'public' diff --git a/app/helpers/http_helper.rb b/app/helpers/http_helper.rb deleted file mode 100644 index e39a52da0..000000000 --- a/app/helpers/http_helper.rb +++ /dev/null @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module HttpHelper - def http_client(options = {}) - timeout = { write: 10, connect: 10, read: 10 }.merge(options) - - HTTP.headers(user_agent: user_agent) - .timeout(:per_operation, timeout) - .follow - end - - private - - def user_agent - @user_agent ||= "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +http://#{Rails.configuration.x.local_domain}/)" - end -end diff --git a/app/lib/provider_discovery.rb b/app/lib/provider_discovery.rb index 6d48cae2f..5e02e6806 100644 --- a/app/lib/provider_discovery.rb +++ b/app/lib/provider_discovery.rb @@ -1,11 +1,9 @@ # frozen_string_literal: true class ProviderDiscovery < OEmbed::ProviderDiscovery - extend HttpHelper - class << self def discover_provider(url, options = {}) - res = http_client.get(url) + res = Request.new(:get, url).perform format = options[:format] raise OEmbed::NotFound, url if res.code != 200 || res.mime_type != 'text/html' diff --git a/app/lib/request.rb b/app/lib/request.rb new file mode 100644 index 000000000..e73c5ac20 --- /dev/null +++ b/app/lib/request.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +class Request + REQUEST_TARGET = '(request-target)' + + include RoutingHelper + + def initialize(verb, url, options = {}) + @verb = verb + @url = Addressable::URI.parse(url).normalize + @options = options + @headers = {} + + set_common_headers! + end + + def on_behalf_of(account) + raise ArgumentError unless account.local? + @account = account + end + + def add_headers(new_headers) + @headers.merge!(new_headers) + end + + def perform + http_client.headers(headers).public_send(@verb, @url.to_s, @options) + end + + def headers + (@account ? @headers.merge('Signature' => signature) : @headers).without(REQUEST_TARGET) + end + + private + + def set_common_headers! + @headers[REQUEST_TARGET] = "#{@verb} #{@url.path}" + @headers['User-Agent'] = user_agent + @headers['Host'] = @url.host + @headers['Date'] = Time.now.utc.httpdate + end + + def signature + key_id = @account.to_webfinger_s + algorithm = 'rsa-sha256' + signature = Base64.strict_encode64(@account.keypair.sign(OpenSSL::Digest::SHA256.new, signed_string)) + + "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers}\",signature=\"#{signature}\"" + end + + def signed_string + @headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") + end + + def signed_headers + @headers.keys.join(' ').downcase + end + + def user_agent + @user_agent ||= "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +#{root_url})" + end + + def timeout + { write: 10, connect: 10, read: 10 } + end + + def http_client + HTTP.timeout(:per_operation, timeout).follow + end +end diff --git a/app/models/account.rb b/app/models/account.rb index 7243cb1a5..58b0a1086 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -130,7 +130,7 @@ class Account < ApplicationRecord end def subscription(webhook_url) - OStatus2::Subscription.new(remote_url, secret: secret, lease_seconds: 86_400 * 30, webhook: webhook_url, hub: hub_url) + OStatus2::Subscription.new(remote_url, secret: secret, lease_seconds: 30.days.seconds, webhook: webhook_url, hub: hub_url) end def save_with_optional_media! diff --git a/app/models/concerns/remotable.rb b/app/models/concerns/remotable.rb index b4f169649..1bd87a642 100644 --- a/app/models/concerns/remotable.rb +++ b/app/models/concerns/remotable.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true module Remotable - include HttpHelper extend ActiveSupport::Concern included do @@ -20,7 +19,7 @@ module Remotable return if !%w(http https).include?(parsed_url.scheme) || parsed_url.host.empty? || self[attribute_name] == url begin - response = http_client.get(url) + response = Request.new(:get, url).perform return if response.code != 200 diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 35a228df0..d9d5024a9 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -16,8 +16,8 @@ # class Subscription < ApplicationRecord - MIN_EXPIRATION = 7.days.seconds.to_i - MAX_EXPIRATION = 30.days.seconds.to_i + MIN_EXPIRATION = 1.day.to_i + MAX_EXPIRATION = 30.days.to_i belongs_to :account, required: true diff --git a/app/services/fetch_atom_service.rb b/app/services/fetch_atom_service.rb index d430b22e9..3ac441e3e 100644 --- a/app/services/fetch_atom_service.rb +++ b/app/services/fetch_atom_service.rb @@ -1,16 +1,14 @@ # frozen_string_literal: true class FetchAtomService < BaseService - include HttpHelper - def call(url) return if url.blank? - response = http_client.head(url) + response = Request.new(:head, url).perform Rails.logger.debug "Remote status HEAD request returned code #{response.code}" - response = http_client.get(url) if response.code == 405 + response = Request.new(:get, url).perform if response.code == 405 Rails.logger.debug "Remote status GET request returned code #{response.code}" @@ -49,6 +47,6 @@ class FetchAtomService < BaseService end def fetch(url) - http_client.get(url).to_s + Request.new(:get, url).perform.to_s end end diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb index 6ef3abb66..20c85e0ea 100644 --- a/app/services/fetch_link_card_service.rb +++ b/app/services/fetch_link_card_service.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class FetchLinkCardService < BaseService - include HttpHelper - URL_PATTERN = %r{https?://\S+} def call(status) @@ -13,7 +11,7 @@ class FetchLinkCardService < BaseService url = url.to_s card = PreviewCard.where(status: status).first_or_initialize(status: status, url: url) - res = http_client.head(url) + res = Request.new(:head, url).perform return if res.code != 200 || res.mime_type != 'text/html' @@ -80,7 +78,7 @@ class FetchLinkCardService < BaseService end def attempt_opengraph(card, url) - response = http_client.get(url) + response = Request.new(:get, url).perform return if response.code != 200 || response.mime_type != 'text/html' diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb index 362d0df98..d2dfda824 100644 --- a/app/services/resolve_remote_account_service.rb +++ b/app/services/resolve_remote_account_service.rb @@ -2,7 +2,6 @@ class ResolveRemoteAccountService < BaseService include OStatus2::MagicKey - include HttpHelper DFRN_NS = 'http://purl.org/macgirvin/dfrn/1.0' @@ -79,7 +78,7 @@ class ResolveRemoteAccountService < BaseService end def get_feed(url) - response = http_client(write: 20, connect: 20, read: 50).get(Addressable::URI.parse(url).normalize) + response = Request.new(:get, url).perform raise Goldfinger::Error, "Feed attempt failed for #{url}: HTTP #{response.code}" unless response.code == 200 [response.to_s, Nokogiri::XML(response)] end diff --git a/app/services/send_interaction_service.rb b/app/services/send_interaction_service.rb index 34c8f9e34..ef38a748b 100644 --- a/app/services/send_interaction_service.rb +++ b/app/services/send_interaction_service.rb @@ -12,13 +12,23 @@ class SendInteractionService < BaseService return if block_notification? - envelope = salmon.pack(@xml, @source_account.keypair) - delivery = salmon.post(@target_account.salmon_url, envelope) + delivery = build_request.perform + raise "Delivery failed for #{target_account.salmon_url}: HTTP #{delivery.code}" unless delivery.code > 199 && delivery.code < 300 end private + def build_request + request = Request.new(:post, @target_account.salmon_url, body: envelope) + request.add_headers('Content-Type' => 'application/magic-envelope+xml') + request + end + + def envelope + salmon.pack(@xml, @source_account.keypair) + end + def block_notification? DomainBlock.blocked?(@target_account.domain) end diff --git a/app/services/subscribe_service.rb b/app/services/subscribe_service.rb index 1e7984a7f..f58067038 100644 --- a/app/services/subscribe_service.rb +++ b/app/services/subscribe_service.rb @@ -2,34 +2,54 @@ class SubscribeService < BaseService def call(account) - account.secret = SecureRandom.hex + @account = account + @account.secret = SecureRandom.hex + @response = build_request.perform - subscription = account.subscription(api_subscription_url(account.id)) - response = subscription.subscribe - - if response_failed_permanently?(response) + if response_failed_permanently? # We're not allowed to subscribe. Fail and move on. - account.secret = '' - account.save! - elsif response_successful?(response) + @account.secret = '' + @account.save! + elsif response_successful? # The subscription will be confirmed asynchronously. - account.save! + @account.save! 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 "Subscription attempt failed for #{@account.acct} (#{@account.hub_url}): HTTP #{@response.code}" end end private + def build_request + request = Request.new(:post, @account.hub_url, form: subscription_params) + request.on_behalf_of(some_local_account) if some_local_account + request + end + + def subscription_params + { + 'hub.topic': @account.remote_url, + 'hub.mode': 'subscribe', + 'hub.callback': api_subscription_url(@account.id), + 'hub.verify': 'async', + 'hub.secret': @account.secret, + 'hub.lease_seconds': 7.days.seconds, + } + end + + def some_local_account + @some_local_account ||= Account.local.first + end + # Any response in the 3xx or 4xx range, except for 429 (rate limit) - def response_failed_permanently?(response) - (response.status.redirect? || response.status.client_error?) && !response.status.too_many_requests? + def response_failed_permanently? + (@response.status.redirect? || @response.status.client_error?) && !@response.status.too_many_requests? end # Any response in the 2xx range - def response_successful?(response) - response.status.success? + def response_successful? + @response.status.success? end end diff --git a/app/services/unsubscribe_service.rb b/app/services/unsubscribe_service.rb index 6db8dbdc4..c2f022d7d 100644 --- a/app/services/unsubscribe_service.rb +++ b/app/services/unsubscribe_service.rb @@ -2,17 +2,30 @@ class UnsubscribeService < BaseService def call(account) - subscription = account.subscription(api_subscription_url(account.id)) - response = subscription.unsubscribe + @account = account + @response = build_request.perform - unless response.status.success? - Rails.logger.debug "PuSH unsubscribe for #{account.acct} failed: #{response.status}" - end + Rails.logger.debug "PuSH unsubscribe for #{@account.acct} failed: #{@response.status}" unless @response.status.success? - account.secret = '' - account.subscription_expires_at = nil - account.save! + @account.secret = '' + @account.subscription_expires_at = nil + @account.save! rescue HTTP::Error, OpenSSL::SSL::SSLError - Rails.logger.debug "PuSH subscription request for #{account.acct} could not be made due to HTTP or SSL error" + Rails.logger.debug "PuSH subscription request for #{@account.acct} could not be made due to HTTP or SSL error" + end + + private + + def build_request + Request.new(:post, @account.hub_url, form: subscription_params) + end + + def subscription_params + { + 'hub.topic': @account.remote_url, + 'hub.mode': 'unsubscribe', + 'hub.callback': api_subscription_url(@account.id), + 'hub.verify': 'async', + } end end diff --git a/app/workers/pubsubhubbub/confirmation_worker.rb b/app/workers/pubsubhubbub/confirmation_worker.rb index 9186c5d7d..e1ccfb99c 100644 --- a/app/workers/pubsubhubbub/confirmation_worker.rb +++ b/app/workers/pubsubhubbub/confirmation_worker.rb @@ -60,9 +60,7 @@ class Pubsubhubbub::ConfirmationWorker end def callback_get_with_params - HTTP.headers(user_agent: 'Mastodon/PubSubHubbub') - .timeout(:per_operation, write: 20, connect: 20, read: 50) - .get(subscription.callback_url, params: callback_params) + Request.new(:get, subscription.callback_url, params: callback_params).perform end def callback_response_body @@ -71,10 +69,10 @@ class Pubsubhubbub::ConfirmationWorker def callback_params { - 'hub.topic' => account_url(subscription.account, format: :atom), - 'hub.mode' => mode, - 'hub.challenge' => challenge, - 'hub.lease_seconds' => subscription.lease_seconds, + 'hub.topic': account_url(subscription.account, format: :atom), + 'hub.mode': mode, + 'hub.challenge': challenge, + 'hub.lease_seconds': subscription.lease_seconds, } end diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb index 981838f33..05d160cf7 100644 --- a/app/workers/pubsubhubbub/delivery_worker.rb +++ b/app/workers/pubsubhubbub/delivery_worker.rb @@ -33,9 +33,9 @@ class Pubsubhubbub::DeliveryWorker end def callback_post_payload - HTTP.timeout(:per_operation, write: 50, connect: 20, read: 50) - .headers(headers) - .post(subscription.callback_url, body: payload) + request = Request.new(:post, subscription.callback_url, body: payload) + request.add_headers(headers) + request.perform end def blocked_domain? @@ -48,13 +48,12 @@ class Pubsubhubbub::DeliveryWorker def headers { - 'User-Agent' => 'Mastodon/PubSubHubbub', 'Content-Type' => 'application/atom+xml', - 'Link' => link_headers, + 'Link' => link_header, }.merge(signature_headers.to_h) end - def link_headers + def link_header LinkHeader.new([hub_link_header, self_link_header]).to_s end diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb new file mode 100644 index 000000000..b371795ab --- /dev/null +++ b/spec/controllers/concerns/signature_verification_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ApplicationController, type: :controller do + controller do + include SignatureVerification + + def success + head 200 + end + + def alternative_success + head 200 + end + end + + before do + routes.draw { get 'success' => 'anonymous#success' } + end + + context 'without signature header' do + before do + get :success + end + + describe '#signed_request?' do + it 'returns false' do + expect(controller.signed_request?).to be false + end + end + + describe '#signed_request_account' do + it 'returns nil' do + expect(controller.signed_request_account).to be_nil + end + end + end + + context 'with signature header' do + let!(:author) { Fabricate(:account) } + + before do + get :success + + fake_request = Request.new(:get, request.url) + fake_request.on_behalf_of(author) + + request.headers.merge!(fake_request.headers) + end + + describe '#signed_request?' do + it 'returns true' do + expect(controller.signed_request?).to be true + end + end + + describe '#signed_request_account' do + it 'returns an account' do + expect(controller.signed_request_account).to eq author + end + + it 'returns nil when path does not match' do + request.path = '/alternative-path' + expect(controller.signed_request_account).to be_nil + end + + it 'returns nil when method does not match' do + post :success + expect(controller.signed_request_account).to be_nil + end + end + end +end diff --git a/spec/helpers/http_helper_spec.rb b/spec/helpers/http_helper_spec.rb deleted file mode 100644 index b8e31b8e6..000000000 --- a/spec/helpers/http_helper_spec.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe HttpHelper do - describe 'http_client' do - it 'returns HTTP::Client with default options' do - options = helper.http_client.default_options - expect(options.headers['User-Agent']).to match /.+ \(Mastodon\/.+;\ \+http:\/\/cb6e6126\.ngrok\.io\/\)/ - expect(options.timeout_options).to eq read_timeout: 10, write_timeout: 10, connect_timeout: 10 - end - end -end diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb new file mode 100644 index 000000000..782f14b18 --- /dev/null +++ b/spec/lib/request_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Request do + subject { Request.new(:get, 'http://example.com') } + + describe '#headers' do + it 'returns user agent' do + expect(subject.headers['User-Agent']).to be_present + end + + it 'returns the date header' do + expect(subject.headers['Date']).to be_present + end + + it 'returns the host header' do + expect(subject.headers['Host']).to be_present + end + + it 'does not return virtual request-target header' do + expect(subject.headers['(request-target)']).to be_nil + end + end + + describe '#on_behalf_of' do + it 'when used, adds signature header' do + subject.on_behalf_of(Fabricate(:account)) + expect(subject.headers['Signature']).to be_present + end + end + + describe '#add_headers' do + it 'adds headers to the request' do + subject.add_headers('Test' => 'Foo') + expect(subject.headers['Test']).to eq 'Foo' + end + end + + describe '#perform' do + before do + stub_request(:get, 'http://example.com') + subject.perform + end + + it 'executes a HTTP request' do + expect(a_request(:get, 'http://example.com')).to have_been_made.once + end + + it 'sets headers' do + expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made + end + end +end diff --git a/spec/workers/pubsubhubbub/confirmation_worker_spec.rb b/spec/workers/pubsubhubbub/confirmation_worker_spec.rb index 1199d5801..8f66b4520 100644 --- a/spec/workers/pubsubhubbub/confirmation_worker_spec.rb +++ b/spec/workers/pubsubhubbub/confirmation_worker_spec.rb @@ -83,6 +83,6 @@ describe Pubsubhubbub::ConfirmationWorker do end def http_headers - { 'Connection' => 'close', 'Host' => 'example.com', 'User-Agent' => 'Mastodon/PubSubHubbub' } + { 'Connection' => 'close', 'Host' => 'example.com', 'User-Agent' => 'http.rb/2.2.2 (Mastodon/1.4.7; +https://cb6e6126.ngrok.io/)' } end end diff --git a/spec/workers/pubsubhubbub/delivery_worker_spec.rb b/spec/workers/pubsubhubbub/delivery_worker_spec.rb index 081dfa41c..a83245786 100644 --- a/spec/workers/pubsubhubbub/delivery_worker_spec.rb +++ b/spec/workers/pubsubhubbub/delivery_worker_spec.rb @@ -59,7 +59,7 @@ describe Pubsubhubbub::DeliveryWorker do 'Content-Type' => 'application/atom+xml', 'Host' => 'example.com', 'Link' => "; rel=\"hub\", ; rel=\"self\"", - 'User-Agent' => 'Mastodon/PubSubHubbub', + 'User-Agent' => 'http.rb/2.2.2 (Mastodon/1.4.7; +https://cb6e6126.ngrok.io/)', }.tap do |basic| known_digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), subscription.secret.to_s, payload) basic.merge('X-Hub-Signature' => "sha1=#{known_digest}") if subscription.secret? -- cgit From cd9b2ab2f70b6c1da5d0abeaa88eecdfc1b41f78 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 14 Jul 2017 23:01:20 +0200 Subject: Fix #2672 - Connect signed PuSH subscription requests to instance domain (#4205) * Fix #2672 - Connect signed PuSH subscription requests to instance domain Resolves #2739 * Fix return of locate_subscription * Fix tests --- app/controllers/api/push_controller.rb | 8 +++++++- app/models/subscription.rb | 2 +- app/services/pubsubhubbub/subscribe_service.rb | 16 +++++++++++++--- app/workers/pubsubhubbub/distribution_worker.rb | 8 ++++---- db/migrate/20170714184731_add_domain_to_subscriptions.rb | 5 +++++ db/schema.rb | 3 ++- spec/controllers/api/push_controller_spec.rb | 1 + 7 files changed, 33 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20170714184731_add_domain_to_subscriptions.rb (limited to 'app/workers') diff --git a/app/controllers/api/push_controller.rb b/app/controllers/api/push_controller.rb index 951867140..e04d19125 100644 --- a/app/controllers/api/push_controller.rb +++ b/app/controllers/api/push_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Api::PushController < Api::BaseController + include SignatureVerification + def update response, status = process_push_request render plain: response, status: status @@ -11,7 +13,7 @@ class Api::PushController < Api::BaseController def process_push_request case hub_mode when 'subscribe' - Pubsubhubbub::SubscribeService.new.call(account_from_topic, hub_callback, hub_secret, hub_lease_seconds) + Pubsubhubbub::SubscribeService.new.call(account_from_topic, hub_callback, hub_secret, hub_lease_seconds, verified_domain) when 'unsubscribe' Pubsubhubbub::UnsubscribeService.new.call(account_from_topic, hub_callback) else @@ -57,6 +59,10 @@ class Api::PushController < Api::BaseController TagManager.instance.web_domain?(hub_topic_domain) end + def verified_domain + return signed_request_account.domain if signed_request_account + end + def hub_topic_domain hub_topic_uri.host + (hub_topic_uri.port ? ":#{hub_topic_uri.port}" : '') end diff --git a/app/models/subscription.rb b/app/models/subscription.rb index d9d5024a9..bf643c1f9 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -1,5 +1,4 @@ # frozen_string_literal: true - # == Schema Information # # Table name: subscriptions @@ -13,6 +12,7 @@ # created_at :datetime not null # updated_at :datetime not null # last_successful_delivery_at :datetime +# domain :string # class Subscription < ApplicationRecord diff --git a/app/services/pubsubhubbub/subscribe_service.rb b/app/services/pubsubhubbub/subscribe_service.rb index eeb7ab258..2dba05b12 100644 --- a/app/services/pubsubhubbub/subscribe_service.rb +++ b/app/services/pubsubhubbub/subscribe_service.rb @@ -3,13 +3,15 @@ class Pubsubhubbub::SubscribeService < BaseService URL_PATTERN = /\A#{URI.regexp(%w(http https))}\z/ - attr_reader :account, :callback, :secret, :lease_seconds + attr_reader :account, :callback, :secret, + :lease_seconds, :domain - def call(account, callback, secret, lease_seconds) + def call(account, callback, secret, lease_seconds, verified_domain = nil) @account = account @callback = Addressable::URI.parse(callback).normalize.to_s @secret = secret @lease_seconds = lease_seconds + @domain = verified_domain process_subscribe end @@ -56,6 +58,14 @@ class Pubsubhubbub::SubscribeService < BaseService end def locate_subscription - Subscription.where(account: account, callback_url: callback).first_or_create!(account: account, callback_url: callback) + subscription = Subscription.find_by(account: account, callback_url: callback) + + if subscription.nil? + subscription = Subscription.new(account: account, callback_url: callback) + end + + subscription.domain = domain + subscription.save! + subscription end end diff --git a/app/workers/pubsubhubbub/distribution_worker.rb b/app/workers/pubsubhubbub/distribution_worker.rb index b41cec90d..7592354cc 100644 --- a/app/workers/pubsubhubbub/distribution_worker.rb +++ b/app/workers/pubsubhubbub/distribution_worker.rb @@ -35,16 +35,16 @@ class Pubsubhubbub::DistributionWorker @payload = AtomSerializer.render(AtomSerializer.new.feed(@account, stream_entries)) @domains = @account.followers.domains - Pubsubhubbub::DeliveryWorker.push_bulk(@subscriptions.reject { |s| !allowed_to_receive?(s.callback_url) }) do |subscription| + Pubsubhubbub::DeliveryWorker.push_bulk(@subscriptions.reject { |s| !allowed_to_receive?(s.callback_url, s.domain) }) do |subscription| [subscription.id, @payload] end end def active_subscriptions - Subscription.where(account: @account).active.select('id, callback_url') + Subscription.where(account: @account).active.select('id, callback_url, domain') end - def allowed_to_receive?(callback_url) - @domains.include?(Addressable::URI.parse(callback_url).host) + def allowed_to_receive?(callback_url, domain) + (!domain.nil? && @domains.include?(domain)) || @domains.include?(Addressable::URI.parse(callback_url).host) end end diff --git a/db/migrate/20170714184731_add_domain_to_subscriptions.rb b/db/migrate/20170714184731_add_domain_to_subscriptions.rb new file mode 100644 index 000000000..7c01a64f5 --- /dev/null +++ b/db/migrate/20170714184731_add_domain_to_subscriptions.rb @@ -0,0 +1,5 @@ +class AddDomainToSubscriptions < ActiveRecord::Migration[5.1] + def change + add_column :subscriptions, :domain, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index b2c59a0f6..5ec78a7c9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170713190709) do +ActiveRecord::Schema.define(version: 20170714184731) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -326,6 +326,7 @@ ActiveRecord::Schema.define(version: 20170713190709) do t.datetime "created_at", null: false t.datetime "updated_at", null: false t.datetime "last_successful_delivery_at" + t.string "domain" t.index ["account_id", "callback_url"], name: "index_subscriptions_on_account_id_and_callback_url", unique: true end diff --git a/spec/controllers/api/push_controller_spec.rb b/spec/controllers/api/push_controller_spec.rb index 18bfa70e5..647698bd1 100644 --- a/spec/controllers/api/push_controller_spec.rb +++ b/spec/controllers/api/push_controller_spec.rb @@ -21,6 +21,7 @@ RSpec.describe Api::PushController, type: :controller do 'https://callback.host/api', 'as1234df', '3600', + nil ) expect(response).to have_http_status(:success) end -- cgit From 05cd37097c134d559550adca6cd17cf7578be94b Mon Sep 17 00:00:00 2001 From: ThibG Date: Sat, 15 Jul 2017 17:24:35 +0200 Subject: Optimize uri normalization (#4212) * Add dependency on idn-ruby to speed up URI normalization * Use normalized_host instead of normalize.host when applicable When we are only interested in the normalized host, calling normalized_host avoids normalizing the other components of the URI as well as creating a new object --- Gemfile | 1 + Gemfile.lock | 2 ++ app/lib/tag_manager.rb | 2 +- app/services/concerns/author_extractor.rb | 2 +- app/services/fetch_remote_status_service.rb | 4 ++-- app/workers/pubsubhubbub/delivery_worker.rb | 2 +- 6 files changed, 8 insertions(+), 5 deletions(-) (limited to 'app/workers') diff --git a/Gemfile b/Gemfile index 5d5ddfae1..a6c2b2d65 100644 --- a/Gemfile +++ b/Gemfile @@ -36,6 +36,7 @@ gem 'htmlentities', '~> 4.3' gem 'http', '~> 2.2' gem 'http_accept_language', '~> 2.1' gem 'httplog', '~> 0.99' +gem 'idn-ruby', require: 'idn' gem 'kaminari', '~> 1.0' gem 'link_header', '~> 0.0' gem 'mime-types', '~> 3.1' diff --git a/Gemfile.lock b/Gemfile.lock index daef3e1ad..f637c9bbe 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -208,6 +208,7 @@ GEM parser (>= 2.2.3.0) rainbow (~> 2.2) terminal-table (>= 1.5.1) + idn-ruby (0.1.0) jmespath (1.3.1) json (2.1.0) jsonapi-renderer (0.1.2) @@ -528,6 +529,7 @@ DEPENDENCIES http_accept_language (~> 2.1) httplog (~> 0.99) i18n-tasks (~> 0.9) + idn-ruby kaminari (~> 1.0) letter_opener (~> 1.4) letter_opener_web (~> 1.3) diff --git a/app/lib/tag_manager.rb b/app/lib/tag_manager.rb index f1a2234dc..5f87a2a48 100644 --- a/app/lib/tag_manager.rb +++ b/app/lib/tag_manager.rb @@ -70,7 +70,7 @@ class TagManager uri = Addressable::URI.new uri.host = domain.gsub(/[\/]/, '') - uri.normalize.host + uri.normalized_host end def same_acct?(canonical, needle) diff --git a/app/services/concerns/author_extractor.rb b/app/services/concerns/author_extractor.rb index 00fe1c663..867d6dc25 100644 --- a/app/services/concerns/author_extractor.rb +++ b/app/services/concerns/author_extractor.rb @@ -14,7 +14,7 @@ module AuthorExtractor return nil if username.blank? || uri.blank? - domain = Addressable::URI.parse(uri).normalize.host + domain = Addressable::URI.parse(uri).normalized_host acct = "#{username}@#{domain}" end diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb index 4cfd33d90..6ac31e4d8 100644 --- a/app/services/fetch_remote_status_service.rb +++ b/app/services/fetch_remote_status_service.rb @@ -24,7 +24,7 @@ class FetchRemoteStatusService < BaseService xml.encoding = 'utf-8' account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: TagManager::XMLNS)) - domain = Addressable::URI.parse(url).normalize.host + domain = Addressable::URI.parse(url).normalized_host return nil unless !account.nil? && confirmed_domain?(domain, account) @@ -39,6 +39,6 @@ class FetchRemoteStatusService < BaseService end def confirmed_domain?(domain, account) - account.domain.nil? || 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).normalized_host).zero? end end diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb index 05d160cf7..2e1101b93 100644 --- a/app/workers/pubsubhubbub/delivery_worker.rb +++ b/app/workers/pubsubhubbub/delivery_worker.rb @@ -43,7 +43,7 @@ class Pubsubhubbub::DeliveryWorker end def host - Addressable::URI.parse(subscription.callback_url).normalize.host + Addressable::URI.parse(subscription.callback_url).normalized_host end def headers -- cgit