Improve federated ID validation (#8372)

* Fix URI not being sufficiently validated with prefetched JSON

* Add additional id validation to OStatus documents, when possible

diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb
index 9d2b6cf00d28024db7c3cb4a5fd9a9b152fd66cf..5323972723ee324dd59f33a812235e6d7ce8c5c0 100644 (file)
--- a/app/helpers/jsonld_helper.rb
+++ b/app/helpers/jsonld_helper.rb
@@ -73,8 +73,10 @@ module JsonLdHelper
     end
   end
 
-  def body_to_json(body)
-    body.is_a?(String) ? Oj.load(body, mode: :strict) : body
+  def body_to_json(body, compare_id: nil)
+    json = body.is_a?(String) ? Oj.load(body, mode: :strict) : body
+    return if compare_id.present? && json['id'] != compare_id
+    json
   rescue Oj::ParseError
     nil
   end

diff --git a/app/lib/ostatus/activity/creation.rb b/app/lib/ostatus/activity/creation.rb
index d3a303a0c4896f23edeaf68db9a7ab77f622c4a0..8f8c700521525204b6a568df71ce4543ac06b0f8 100644 (file)
--- a/app/lib/ostatus/activity/creation.rb
+++ b/app/lib/ostatus/activity/creation.rb
@@ -7,7 +7,7 @@ class OStatus::Activity::Creation < OStatus::Activity::Base
       return [nil, false]
     end
 
-    return [nil, false] if @account.suspended?
+    return [nil, false] if @account.suspended? || invalid_origin?
 
     RedisLock.acquire(lock_options) do |lock|
       if lock.acquired?
@@ -204,6 +204,15 @@ class OStatus::Activity::Creation < OStatus::Activity::Base
     end
   end
 
+  def invalid_origin?
+    return false unless id.start_with?('http') # Legacy IDs cannot be checked
+
+    needle = Addressable::URI.parse(id).normalized_host
+
+    !(needle.casecmp(@account.domain).zero? ||
+      needle.casecmp(Addressable::URI.parse(@account.remote_url.presence || @account.uri).normalized_host).zero?)
+  end
+
   def lock_options
     { redis: Redis.current, key: "create:#{id}" }
   end

diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb
index 41fec9170a2bd8b71e4e76d713e4cb852f637346..1ec9ee5dd6358feb12da0122637126e14b875094 100644 (file)
--- a/app/services/activitypub/fetch_remote_account_service.rb
+++ b/app/services/activitypub/fetch_remote_account_service.rb
@@ -11,7 +11,7 @@ class ActivityPub::FetchRemoteAccountService < BaseService
     @json = if prefetched_body.nil?
               fetch_resource(uri, id)
             else
-              body_to_json(prefetched_body)
+              body_to_json(prefetched_body, compare_id: id ? uri : nil)
             end
 
     return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?)

diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb
index 505baccd46e2305e56a6b1d9c786040d0812d629..df17d907932740815745de361492678b736c0133 100644 (file)
--- a/app/services/activitypub/fetch_remote_key_service.rb
+++ b/app/services/activitypub/fetch_remote_key_service.rb
@@ -17,7 +17,7 @@ class ActivityPub::FetchRemoteKeyService < BaseService
         @json = fetch_resource(uri, id)
       end
     else
-      @json = body_to_json(prefetched_body)
+      @json = body_to_json(prefetched_body, compare_id: id ? uri : nil)
     end
 
     return unless supported_context?(@json) && expected_type?

diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb
index 2b447abb32eb5fa95771d9cb7424b138326f6125..469821032ab69746c12ea79b868f1e4b1626fa9c 100644 (file)
--- a/app/services/activitypub/fetch_remote_status_service.rb
+++ b/app/services/activitypub/fetch_remote_status_service.rb
@@ -8,7 +8,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService
     @json = if prefetched_body.nil?
               fetch_resource(uri, id, on_behalf_of)
             else
-              body_to_json(prefetched_body)
+              body_to_json(prefetched_body, compare_id: id ? uri : nil)
             end
 
     return unless supported_context? && expected_type?

diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb
index a0f031a4450651dd860200eb4ad8b0871c38dc72..cfc560022fe6d6f7aae8fb122c798aab0bb4dde3 100644 (file)
--- a/app/services/fetch_remote_account_service.rb
+++ b/app/services/fetch_remote_account_service.rb
@@ -27,7 +27,7 @@ class FetchRemoteAccountService < BaseService
 
     account = author_from_xml(xml.at_xpath('/xmlns:feed', xmlns: OStatus::TagManager::XMLNS), false)
 
-    UpdateRemoteProfileService.new.call(xml, account) unless account.nil?
+    UpdateRemoteProfileService.new.call(xml, account) if account.present? && trusted_domain?(url, account)
 
     account
   rescue TypeError
@@ -37,4 +37,9 @@ class FetchRemoteAccountService < BaseService
     Rails.logger.debug 'Invalid XML or missing namespace'
     nil
   end
+
+  def trusted_domain?(url, account)
+    domain = Addressable::URI.parse(url).normalized_host
+    domain.casecmp(account.domain).zero? || domain.casecmp(Addressable::URI.parse(account.remote_url.presence || account.uri).normalized_host).zero?
+  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 dba55c034619660c72ebeaa31b0f31f5fc9ad15a..aa13f0a9b7936774212ffa16b2f7148206a1d4dd 100644 (file)
--- a/spec/services/activitypub/fetch_remote_account_service_spec.rb
+++ b/spec/services/activitypub/fetch_remote_account_service_spec.rb
@@ -59,7 +59,6 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do
       it 'returns nil' do
         expect(account).to be_nil
       end
-
     end
 
     context 'when URI and WebFinger share the same host' do
@@ -119,5 +118,11 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do
 
       include_examples 'sets profile data'
     end
+
+    context 'with wrong id' do
+      it 'does not create account' do
+        expect(subject.call('https://fake.address/@foo', prefetched_body: Oj.dump(actor))).to be_nil
+      end
+    end
   end
 end

diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb
index 549eb80fa6454fbc790fefb879b151a6344bb4bf..9ae4099969405194c47006a7c3313a6afb5ab869 100644 (file)
--- a/spec/services/activitypub/fetch_remote_status_service_spec.rb
+++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb
@@ -70,5 +70,27 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do
         expect(strip_tags(status.text)).to eq "Nyan Cat 10 hours remix https://#{valid_domain}/watch?v=12345"
       end
     end
+
+    context 'with wrong id' do
+      let(:note) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: "https://real.address/@foo/1234",
+          type: 'Note',
+          content: 'Lorem ipsum',
+          attributedTo: ActivityPub::TagManager.instance.uri_for(sender),
+        }
+      end
+
+      let(:object) do
+        temp = note.dup
+        temp[:id] = 'https://fake.address/@foo/5678'
+        temp
+      end
+
+      it 'does not create status' do
+        expect(sender.statuses.first).to be_nil
+      end
+    end
   end
 end

diff --git a/spec/services/fetch_remote_account_service_spec.rb b/spec/services/fetch_remote_account_service_spec.rb
index 1c3abe8f3c7b0e2359609ad83a723ae1d2f6b0f3..20dd505d01e8f49a62743d4ff9930f1cc96a80a5 100644 (file)
--- a/spec/services/fetch_remote_account_service_spec.rb
+++ b/spec/services/fetch_remote_account_service_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 RSpec.describe FetchRemoteAccountService, type: :service do
-  let(:url) { 'https://example.com' }
+  let(:url) { 'https://example.com/alice' }
   let(:prefetched_body) { nil }
   let(:protocol) { :ostatus }
   subject { FetchRemoteAccountService.new.call(url, prefetched_body, protocol) }
@@ -46,6 +46,24 @@ RSpec.describe FetchRemoteAccountService, type: :service do
     end
 
     include_examples 'return Account'
+
+    it 'does not update account information if XML comes from an unverified domain' do
+      feed_xml = <<-XML.squish
+        <?xml version="1.0" encoding="UTF-8"?>
+        <feed xml:lang="en-US" xmlns="http://www.w3.org/2005/Atom" xmlns:thr="http://purl.org/syndication/thread/1.0" xmlns:georss="http://www.georss.org/georss" xmlns:activity="http://activitystrea.ms/spec/1.0/" xmlns:media="http://purl.org/syndication/atommedia" xmlns:poco="http://portablecontacts.net/spec/1.0" xmlns:ostatus="http://ostatus.org/schema/1.0" xmlns:statusnet="http://status.net/schema/api/1/">
+          <author>
+            <activity:object-type>http://activitystrea.ms/schema/1.0/person</activity:object-type>
+            <uri>http://kickass.zone/users/localhost</uri>
+            <name>localhost</name>
+            <poco:preferredUsername>localhost</poco:preferredUsername>
+            <poco:displayName>Villain!!!</poco:displayName>
+          </author>
+        </feed>
+      XML
+
+      returned_account = described_class.new.call('https://real-fake-domains.com/alice', feed_xml, :ostatus)
+      expect(returned_account.display_name).to_not eq 'Villain!!!'
+    end
   end
 
   context 'when prefetched_body is nil' do

diff --git a/spec/services/fetch_remote_status_service_spec.rb b/spec/services/fetch_remote_status_service_spec.rb
index 0df9c329ab28e96ae1f692eec6481504780b6ba9..f9db024b93ab17dcb17988144c9f212bf391ad92 100644 (file)
--- a/spec/services/fetch_remote_status_service_spec.rb
+++ b/spec/services/fetch_remote_status_service_spec.rb
@@ -32,4 +32,56 @@ RSpec.describe FetchRemoteStatusService, type: :service do
       expect(status.text).to eq 'Lorem ipsum'
     end
   end
+
+  context 'protocol is :ostatus' do
+    subject { described_class.new }
+
+    before do
+      Fabricate(:account, username: 'tracer', domain: 'real.domain', remote_url: 'https://real.domain/users/tracer')
+    end
+
+    it 'does not create status with author at different domain' do
+      status_body = <<-XML.squish
+        <?xml version="1.0"?>
+        <entry xmlns="http://www.w3.org/2005/Atom" xmlns:thr="http://purl.org/syndication/thread/1.0" xmlns:activity="http://activitystrea.ms/spec/1.0/" xmlns:poco="http://portablecontacts.net/spec/1.0" xmlns:media="http://purl.org/syndication/atommedia" xmlns:ostatus="http://ostatus.org/schema/1.0" xmlns:mastodon="http://mastodon.social/schema/1.0">
+          <id>tag:real.domain,2017-04-27:objectId=4487555:objectType=Status</id>
+          <published>2017-04-27T13:49:25Z</published>
+          <updated>2017-04-27T13:49:25Z</updated>
+          <activity:object-type>http://activitystrea.ms/schema/1.0/note</activity:object-type>
+          <activity:verb>http://activitystrea.ms/schema/1.0/post</activity:verb>
+          <author>
+            <id>https://real.domain/users/tracer</id>
+            <activity:object-type>http://activitystrea.ms/schema/1.0/person</activity:object-type>
+            <uri>https://real.domain/users/tracer</uri>
+            <name>tracer</name>
+          </author>
+          <content type="html">Overwatch rocks</content>
+        </entry>
+      XML
+
+      expect(subject.call('https://fake.domain/foo', status_body, :ostatus)).to be_nil
+    end
+
+    it 'does not create status with wrong id when id uses http format' do
+      status_body = <<-XML.squish
+        <?xml version="1.0"?>
+        <entry xmlns="http://www.w3.org/2005/Atom" xmlns:thr="http://purl.org/syndication/thread/1.0" xmlns:activity="http://activitystrea.ms/spec/1.0/" xmlns:poco="http://portablecontacts.net/spec/1.0" xmlns:media="http://purl.org/syndication/atommedia" xmlns:ostatus="http://ostatus.org/schema/1.0" xmlns:mastodon="http://mastodon.social/schema/1.0">
+          <id>https://other-real.domain/statuses/123</id>
+          <published>2017-04-27T13:49:25Z</published>
+          <updated>2017-04-27T13:49:25Z</updated>
+          <activity:object-type>http://activitystrea.ms/schema/1.0/note</activity:object-type>
+          <activity:verb>http://activitystrea.ms/schema/1.0/post</activity:verb>
+          <author>
+            <id>https://real.domain/users/tracer</id>
+            <activity:object-type>http://activitystrea.ms/schema/1.0/person</activity:object-type>
+            <uri>https://real.domain/users/tracer</uri>
+            <name>tracer</name>
+          </author>
+          <content type="html">Overwatch rocks</content>
+        </entry>
+      XML
+
+      expect(subject.call('https://real.domain/statuses/456', status_body, :ostatus)).to be_nil
+    end
+  end
 end