From 63f097979990bf5ba9db848b8a253056bad781af Mon Sep 17 00:00:00 2001
From: Akihiko Odaki <akihiko.odaki.4i@stu.hosei.ac.jp>
Date: Wed, 4 Oct 2017 08:13:48 +0900
Subject: [PATCH] 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.
---
 .../concerns/signature_verification.rb        |  2 +-
 app/helpers/jsonld_helper.rb                  | 13 +++++-
 app/lib/activitypub/activity/announce.rb      |  2 +-
 app/lib/activitypub/activity/create.rb        |  2 +-
 app/lib/activitypub/linked_data_signature.rb  |  2 +-
 app/lib/ostatus/activity/creation.rb          |  9 ----
 .../fetch_remote_account_service.rb           | 10 +++--
 .../activitypub/fetch_remote_key_service.rb   | 25 ++++++++---
 .../fetch_remote_status_service.rb            | 37 ++++++++---------
 .../activitypub/process_account_service.rb    |  6 +--
 app/services/fetch_atom_service.rb            | 13 ++----
 app/services/fetch_remote_account_service.rb  | 14 +++----
 app/services/fetch_remote_status_service.rb   | 16 ++++----
 .../resolve_remote_account_service.rb         |  2 +-
 spec/helpers/jsonld_helper_spec.rb            | 35 +++++++++++++++-
 .../fetch_remote_account_service_spec.rb      |  2 +-
 .../fetch_remote_status_service_spec.rb       | 41 +------------------
 17 files changed, 118 insertions(+), 113 deletions(-)

diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb
index dc2d9a67..2baafb5b 100644
--- 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
index d82a0733..c23a2e09 100644
--- 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
index 4516454e..1cf84428 100644
--- 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
index 55addd66..be656de4 100644
--- 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
index adb8b6cd..16142a6f 100644
--- 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
index 2687776f..511c462d 100644
--- 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
index cb6e4074..e6c6338b 100644
--- 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
index ebd64071..ce1048fe 100644
--- 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
index a95931af..c7414f16 100644
--- 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
index 81120953..f93baf4b 100644
--- 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
index 9c5777b5..bcf516bc 100644
--- 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
index bd98e70d..a0f031a4 100644
--- 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
index 1b90854c..cacf6ba5 100644
--- 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
index 93ba0770..3d0a36f6 100644
--- 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
index 7d3912e6..48bfdc30 100644
--- 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
index ed7e9bba..c50d3fb9 100644
--- 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
index 3b22257e..ebf42239 100644
--- 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