commit: 63f097979990bf5ba9db848b8a253056bad781af parent: ec13cfa4f940e9f9441ceff1f7389bb0e1bd61fb Author: Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp> Date: Wed, 4 Oct 2017 08:13:48 +0900 Validate id of ActivityPub representations (#5114) Additionally, ActivityPub::FetchRemoteStatusService no longer parses activities. OStatus::Activity::Creation no longer delegates to ActivityPub because the provided ActivityPub representations are not signed while OStatus representations are.Diffstat:
17 files changed, 118 insertions(+), 113 deletions(-)
diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb @@ -117,7 +117,7 @@ module SignatureVerification ResolveRemoteAccountService.new.call(key_id.gsub(/\Aacct:/, '')) elsif !ActivityPub::TagManager.instance.local_uri?(key_id) account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account) - account ||= ActivityPub::FetchRemoteKeyService.new.call(key_id) + account ||= ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false) account end end diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb @@ -22,7 +22,18 @@ module JsonLdHelper graph.dump(:normalize) end - def fetch_resource(uri) + def fetch_resource(uri, id) + unless id + json = fetch_resource_without_id_validation(uri) + return unless json + uri = json['id'] + end + + json = fetch_resource_without_id_validation(uri) + json.present? && json['id'] == uri ? json : nil + end + + def fetch_resource_without_id_validation(uri) response = build_request(uri).perform return if response.code != 200 body_to_json(response.to_s) diff --git a/app/lib/activitypub/activity/announce.rb b/app/lib/activitypub/activity/announce.rb @@ -27,7 +27,7 @@ class ActivityPub::Activity::Announce < ActivityPub::Activity if object_uri.start_with?('http') return if ActivityPub::TagManager.instance.local_uri?(object_uri) - ActivityPub::FetchRemoteStatusService.new.call(object_uri) + ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true) elsif @object['url'].present? ::FetchRemoteStatusService.new.call(@object['url']) end diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb @@ -80,7 +80,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity return if tag['href'].blank? account = account_from_uri(tag['href']) - account = FetchRemoteAccountService.new.call(tag['href']) if account.nil? + account = FetchRemoteAccountService.new.call(tag['href'], id: false) if account.nil? return if account.nil? account.mentions.create(status: status) end diff --git a/app/lib/activitypub/linked_data_signature.rb b/app/lib/activitypub/linked_data_signature.rb @@ -19,7 +19,7 @@ class ActivityPub::LinkedDataSignature return unless type == 'RsaSignature2017' creator = ActivityPub::TagManager.instance.uri_to_resource(creator_uri, Account) - creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri) + creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false) return if creator.nil? diff --git a/app/lib/ostatus/activity/creation.rb b/app/lib/ostatus/activity/creation.rb @@ -9,11 +9,6 @@ class OStatus::Activity::Creation < OStatus::Activity::Base return [nil, false] if @account.suspended? - if activitypub_uri? && [:public, :unlisted].include?(visibility_scope) - result = perform_via_activitypub - return result if result.first.present? - end - RedisLock.acquire(lock_options) do |lock| if lock.acquired? # Return early if status already exists in db @@ -66,10 +61,6 @@ class OStatus::Activity::Creation < OStatus::Activity::Base status end - def perform_via_activitypub - [find_status(activitypub_uri) || ActivityPub::FetchRemoteStatusService.new.call(activitypub_uri), false] - end - def content @xml.at_xpath('./xmlns:content', xmlns: OStatus::TagManager::XMLNS).content end diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb @@ -5,14 +5,18 @@ class ActivityPub::FetchRemoteAccountService < BaseService # Should be called when uri has already been checked for locality # Does a WebFinger roundtrip on each call - def call(uri, prefetched_json = nil) - @json = body_to_json(prefetched_json) || fetch_resource(uri) + def call(uri, id: true, prefetched_body: nil) + @json = if prefetched_body.nil? + fetch_resource(uri, id) + else + body_to_json(prefetched_body) + end return unless supported_context? && expected_type? @uri = @json['id'] @username = @json['preferredUsername'] - @domain = Addressable::URI.parse(uri).normalized_host + @domain = Addressable::URI.parse(@uri).normalized_host return unless verified_webfinger? diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb @@ -4,13 +4,26 @@ class ActivityPub::FetchRemoteKeyService < BaseService include JsonLdHelper # Returns account that owns the key - def call(uri, prefetched_json = nil) - @json = body_to_json(prefetched_json) || fetch_resource(uri) + def call(uri, id: true, prefetched_body: nil) + if prefetched_body.nil? + if id + @json = fetch_resource_without_id_validation(uri) + if person? + @json = fetch_resource(@json['id'], true) + elsif uri != @json['id'] + return + end + else + @json = fetch_resource(uri, id) + end + else + @json = body_to_json(prefetched_body) + end return unless supported_context?(@json) && expected_type? - return find_account(uri, @json) if person? + return find_account(@json['id'], @json) if person? - @owner = fetch_resource(owner_uri) + @owner = fetch_resource(owner_uri, true) return unless supported_context?(@owner) && confirmed_owner? @@ -19,9 +32,9 @@ class ActivityPub::FetchRemoteKeyService < BaseService private - def find_account(uri, prefetched_json) + def find_account(uri, prefetched_body) account = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) - account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_json) + account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_body: prefetched_body) account end diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb @@ -4,36 +4,33 @@ class ActivityPub::FetchRemoteStatusService < BaseService include JsonLdHelper # Should be called when uri has already been checked for locality - def call(uri, prefetched_json = nil) - @json = body_to_json(prefetched_json) || fetch_resource(uri) + def call(uri, id: true, prefetched_body: nil) + @json = if prefetched_body.nil? + fetch_resource(uri, id) + else + body_to_json(prefetched_body) + end - return unless supported_context? + return unless expected_type? && supported_context? - activity = activity_json - actor_id = value_or_id(activity['actor']) - - return unless expected_type?(activity) && trustworthy_attribution?(uri, actor_id) + return if actor_id.nil? || !trustworthy_attribution?(@json['id'], actor_id) actor = ActivityPub::TagManager.instance.uri_to_resource(actor_id, Account) - actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id) if actor.nil? + actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id, id: true) if actor.nil? return if actor.suspended? - ActivityPub::Activity.factory(activity, actor).perform + ActivityPub::Activity.factory(activity_json, actor).perform end private def activity_json - if %w(Note Article).include? @json['type'] - { - 'type' => 'Create', - 'actor' => first_of_value(@json['attributedTo']), - 'object' => @json, - } - else - @json - end + { 'type' => 'Create', 'actor' => actor_id, 'object' => @json } + end + + def actor_id + first_of_value(@json['attributedTo']) end def trustworthy_attribution?(uri, attributed_to) @@ -44,7 +41,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService super(@json) end - def expected_type?(json) - %w(Create Announce).include? json['type'] + def expected_type? + %w(Note Article).include? @json['type'] end end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb @@ -90,7 +90,7 @@ class ActivityPub::ProcessAccountService < BaseService return if value.nil? return value['url'] if value.is_a?(Hash) - image = fetch_resource(value) + image = fetch_resource_without_id_validation(value) image['url'] if image end @@ -100,7 +100,7 @@ class ActivityPub::ProcessAccountService < BaseService return if value.nil? return value['publicKeyPem'] if value.is_a?(Hash) - key = fetch_resource(value) + key = fetch_resource_without_id_validation(value) key['publicKeyPem'] if key end @@ -130,7 +130,7 @@ class ActivityPub::ProcessAccountService < BaseService return if @json[type].blank? return @collections[type] if @collections.key?(type) - collection = fetch_resource(@json[type]) + collection = fetch_resource_without_id_validation(@json[type]) @collections[type] = collection.is_a?(Hash) && collection['totalItems'].present? && collection['totalItems'].is_a?(Numeric) ? collection['totalItems'] : nil rescue HTTP::Error, OpenSSL::SSL::SSLError diff --git a/app/services/fetch_atom_service.rb b/app/services/fetch_atom_service.rb @@ -41,10 +41,11 @@ class FetchAtomService < BaseService return nil if @response.code != 200 if @response.mime_type == 'application/atom+xml' - [@url, @response.to_s, :ostatus] + [@url, { prefetched_body: @response.to_s }, :ostatus] elsif ['application/activity+json', 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"'].include?(@response.mime_type) - if supported_activity?(@response.to_s) - [@url, @response.to_s, :activitypub] + json = body_to_json(body) + if supported_context?(json) && json['type'] == 'Person' && json['inbox'].present? + [json['id'], { id: true }, :activitypub] else @unsupported_activity = true nil @@ -79,10 +80,4 @@ class FetchAtomService < BaseService result end - - def supported_activity?(body) - json = body_to_json(body) - return false unless supported_context?(json) - json['type'] == 'Person' ? json['inbox'].present? : true - end end diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb @@ -5,24 +5,24 @@ class FetchRemoteAccountService < BaseService def call(url, prefetched_body = nil, protocol = :ostatus) if prefetched_body.nil? - resource_url, body, protocol = FetchAtomService.new.call(url) + resource_url, resource_options, protocol = FetchAtomService.new.call(url) else - resource_url = url - body = prefetched_body + resource_url = url + resource_options = { prefetched_body: prefetched_body } end case protocol when :ostatus - process_atom(resource_url, body) + process_atom(resource_url, **resource_options) when :activitypub - ActivityPub::FetchRemoteAccountService.new.call(resource_url, body) + ActivityPub::FetchRemoteAccountService.new.call(resource_url, **resource_options) end end private - def process_atom(url, body) - xml = Nokogiri::XML(body) + def process_atom(url, prefetched_body:) + xml = Nokogiri::XML(prefetched_body) xml.encoding = 'utf-8' account = author_from_xml(xml.at_xpath('/xmlns:feed', xmlns: OStatus::TagManager::XMLNS), false) diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb @@ -5,26 +5,26 @@ class FetchRemoteStatusService < BaseService def call(url, prefetched_body = nil, protocol = :ostatus) if prefetched_body.nil? - resource_url, body, protocol = FetchAtomService.new.call(url) + resource_url, resource_options, protocol = FetchAtomService.new.call(url) else - resource_url = url - body = prefetched_body + resource_url = url + resource_options = { prefetched_body: prefetched_body } end case protocol when :ostatus - process_atom(resource_url, body) + process_atom(resource_url, **resource_options) when :activitypub - ActivityPub::FetchRemoteStatusService.new.call(resource_url, body) + ActivityPub::FetchRemoteStatusService.new.call(resource_url, **resource_options) end end private - def process_atom(url, body) + def process_atom(url, prefetched_body:) Rails.logger.debug "Processing Atom for remote status at #{url}" - xml = Nokogiri::XML(body) + xml = Nokogiri::XML(prefetched_body) xml.encoding = 'utf-8' account = author_from_xml(xml.at_xpath('/xmlns:entry', xmlns: OStatus::TagManager::XMLNS)) @@ -32,7 +32,7 @@ class FetchRemoteStatusService < BaseService return nil unless !account.nil? && confirmed_domain?(domain, account) - statuses = ProcessFeedService.new.call(body, account) + statuses = ProcessFeedService.new.call(prefetched_body, account) statuses.first rescue Nokogiri::XML::XPath::SyntaxError Rails.logger.debug 'Invalid XML or missing namespace' diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb @@ -189,7 +189,7 @@ class ResolveRemoteAccountService < BaseService def actor_json return @actor_json if defined?(@actor_json) - json = fetch_resource(actor_url) + json = fetch_resource(actor_url, false) @actor_json = supported_context?(json) && json['type'] == 'Person' ? json : nil end diff --git a/spec/helpers/jsonld_helper_spec.rb b/spec/helpers/jsonld_helper_spec.rb @@ -30,6 +30,39 @@ describe JsonLdHelper do end describe '#fetch_resource' do - pending + context 'when the second argument is false' do + it 'returns resource even if the retrieved ID and the given URI does not match' do + stub_request(:get, 'https://bob/').to_return body: '{"id": "https://alice/"}' + stub_request(:get, 'https://alice/').to_return body: '{"id": "https://alice/"}' + + expect(fetch_resource('https://bob/', false)).to eq({ 'id' => 'https://alice/' }) + end + + it 'returns nil if the object identified by the given URI and the object identified by the retrieved ID does not match' do + stub_request(:get, 'https://mallory/').to_return body: '{"id": "https://marvin/"}' + stub_request(:get, 'https://marvin/').to_return body: '{"id": "https://alice/"}' + + expect(fetch_resource('https://mallory/', false)).to eq nil + end + end + + context 'when the second argument is true' do + it 'returns nil if the retrieved ID and the given URI does not match' do + stub_request(:get, 'https://mallory/').to_return body: '{"id": "https://alice/"}' + expect(fetch_resource('https://mallory/', true)).to eq nil + end + end + end + + describe '#fetch_resource_without_id_validation' do + it 'returns nil if the status code is not 200' do + stub_request(:get, 'https://host/').to_return status: 400, body: '{}' + expect(fetch_resource_without_id_validation('https://host/')).to eq nil + end + + it 'returns hash' do + stub_request(:get, 'https://host/').to_return status: 200, body: '{}' + expect(fetch_resource_without_id_validation('https://host/')).to eq({}) + end end end diff --git a/spec/services/activitypub/fetch_remote_account_service_spec.rb b/spec/services/activitypub/fetch_remote_account_service_spec.rb @@ -16,7 +16,7 @@ RSpec.describe ActivityPub::FetchRemoteAccountService do end describe '#call' do - let(:account) { subject.call('https://example.com/alice') } + let(:account) { subject.call('https://example.com/alice', id: true) } shared_examples 'sets profile data' do it 'returns an account' do diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -15,21 +15,11 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do } end - let(:create) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: "https://#{valid_domain}/@foo/1234/activity", - type: 'Create', - actor: ActivityPub::TagManager.instance.uri_for(sender), - object: note, - } - end - subject { described_class.new } describe '#call' do before do - subject.call(object[:id], Oj.dump(object)) + subject.call(object[:id], prefetched_body: Oj.dump(object)) end context 'with Note object' do @@ -42,34 +32,5 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do expect(status.text).to eq 'Lorem ipsum' end end - - context 'with Create activity' do - let(:object) { create } - - it 'creates status' do - status = sender.statuses.first - - expect(status).to_not be_nil - expect(status.text).to eq 'Lorem ipsum' - end - end - - context 'with Announce activity' do - let(:status) { Fabricate(:status, account: recipient) } - - let(:object) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: "https://#{valid_domain}/@foo/1234/activity", - type: 'Announce', - actor: ActivityPub::TagManager.instance.uri_for(sender), - object: ActivityPub::TagManager.instance.uri_for(status), - } - end - - it 'creates a reblog by sender of status' do - expect(sender.reblogged?(status)).to be true - end - end end end