Improve shared status verification (#2525)

* Instead of parsing shared status contents verbatim, make roundtrip
to purported original URL. Confirm that the "original" URL is from the
same domain as the author it claims to be from.

* Fix obvious typo, add comment

* Use URI look-up first

* Add test, update Goldfinger dependency to make less useless HTTP requests per Webfinger lookup

diff --git a/Gemfile.lock b/Gemfile.lock
index fc8d28104f7e92e27d56395098798bbea0377acf..a41187a92c8c1ce7162034b9540299af33488fb5 100644 (file)
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -165,7 +165,7 @@ GEM
       ruby-progressbar (~> 1.4)
     globalid (0.3.7)
       activesupport (>= 4.1.0)
-    goldfinger (1.1.2)
+    goldfinger (1.2.0)
       addressable (~> 2.4)
       http (~> 2.0)
       nokogiri (~> 1.6)
@@ -459,7 +459,7 @@ GEM
       execjs (>= 0.3.0, < 3)
     unf (0.1.4)
       unf_ext
-    unf_ext (0.0.7.3)
+    unf_ext (0.0.7.4)
     unicode-display_width (1.1.3)
     uniform_notifier (1.10.0)
     warden (1.2.7)

diff --git a/app/services/fetch_remote_status_service.rb b/app/services/fetch_remote_status_service.rb
index c666961ad2808d1ef09c86e0958f17bf4073b2a1..5a454808e59e095082f34966f491229847915506 100644 (file)
--- a/app/services/fetch_remote_status_service.rb
+++ b/app/services/fetch_remote_status_service.rb
@@ -39,9 +39,19 @@ class FetchRemoteStatusService < BaseService
 
     Rails.logger.debug "Going to webfinger #{username}@#{domain}"
 
-    return FollowRemoteAccountService.new.call("#{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?
+  end
 end

diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb
index d002b9130e49c58f82dbd1f7243fff72527698ff..799a9f6e3966f644b0f7163280f1637feecde963 100644 (file)
--- a/app/services/process_feed_service.rb
+++ b/app/services/process_feed_service.rb
@@ -47,8 +47,8 @@ class ProcessFeedService < BaseService
       return status unless just_created
 
       if verb == :share
-        original_status, = status_from_xml(@xml.at_xpath('.//activity:object', activity: TagManager::AS_XMLNS))
-        status.reblog    = original_status
+        original_status = shared_status_from_xml(@xml.at_xpath('.//activity:object', activity: TagManager::AS_XMLNS))
+        status.reblog   = original_status
 
         if original_status.nil?
           status.destroy
@@ -90,6 +90,14 @@ class ProcessFeedService < BaseService
       !([:post, :share, :delete].include?(verb) && [:activity, :note, :comment].include?(type))
     end
 
+    def shared_status_from_xml(entry)
+      status = find_status(id(entry))
+
+      return status unless status.nil?
+
+      FetchRemoteStatusService.new.call(url(entry))
+    end
+
     def status_from_xml(entry)
       # Return early if status already exists in db
       status = find_status(id(entry))

diff --git a/spec/services/follow_remote_account_service_spec.rb b/spec/services/follow_remote_account_service_spec.rb
index a8d4a7c6b24193adb123e6b2bbd462a9bbbb1c0d..66e9eb6c9de01850e11df025cea0786e4f22cd60 100644 (file)
--- a/spec/services/follow_remote_account_service_spec.rb
+++ b/spec/services/follow_remote_account_service_spec.rb
@@ -5,6 +5,7 @@ RSpec.describe FollowRemoteAccountService do
 
   before do
     stub_request(:get, "https://quitter.no/.well-known/host-meta").to_return(request_fixture('.host-meta.txt'))
+    stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:catsrgr8@example.com").to_return(status: 404)
     stub_request(:get, "https://example.com/.well-known/host-meta").to_return(status: 404)
     stub_request(:get, "https://quitter.no/.well-known/webfinger?resource=acct:gargron@quitter.no").to_return(request_fixture('webfinger.txt'))
     stub_request(:get, "https://quitter.no/.well-known/webfinger?resource=acct:catsrgr8@quitter.no").to_return(status: 404)

diff --git a/spec/services/process_feed_service_spec.rb b/spec/services/process_feed_service_spec.rb
index b15284fee458062e3b4043bd921855304fd805b9..7f2b4ce1d7096204b6f63570f59751c5be6e0615 100644 (file)
--- a/spec/services/process_feed_service_spec.rb
+++ b/spec/services/process_feed_service_spec.rb
@@ -1,52 +1,118 @@
 require 'rails_helper'
 
 RSpec.describe ProcessFeedService do
-  let(:body) { File.read(File.join(Rails.root, 'spec', 'fixtures', 'xml', 'mastodon.atom')) }
-  let(:account) { Fabricate(:account, username: 'localhost', domain: 'kickass.zone') }
-
   subject { ProcessFeedService.new }
 
-  before do
-    stub_request(:post, "https://pubsubhubbub.superfeedr.com/").to_return(:status => 200, :body => "", :headers => {})
-    stub_request(:get, "http://kickass.zone/system/accounts/avatars/000/000/001/large/eris.png").to_return(request_fixture('avatar.txt'))
-    stub_request(:get, "http://kickass.zone/system/media_attachments/files/000/000/002/original/morpheus_linux.jpg?1476059910").to_return(request_fixture('attachment1.txt'))
-    stub_request(:get, "http://kickass.zone/system/media_attachments/files/000/000/003/original/gizmo.jpg?1476060065").to_return(request_fixture('attachment2.txt'))
+  describe 'processing a feed' do
+    let(:body) { File.read(File.join(Rails.root, 'spec', 'fixtures', 'xml', 'mastodon.atom')) }
+    let(:account) { Fabricate(:account, username: 'localhost', domain: 'kickass.zone') }
 
-    subject.call(body, account)
-  end
+    before do
+      stub_request(:post, "https://pubsubhubbub.superfeedr.com/").to_return(:status => 200, :body => "", :headers => {})
+      stub_request(:get, "http://kickass.zone/system/accounts/avatars/000/000/001/large/eris.png").to_return(request_fixture('avatar.txt'))
+      stub_request(:get, "http://kickass.zone/system/media_attachments/files/000/000/002/original/morpheus_linux.jpg?1476059910").to_return(request_fixture('attachment1.txt'))
+      stub_request(:get, "http://kickass.zone/system/media_attachments/files/000/000/003/original/gizmo.jpg?1476060065").to_return(request_fixture('attachment2.txt'))
 
-  it 'updates remote user\'s account information' do
-    account.reload
-    expect(account.display_name).to eq '::1'
-    expect(account).to have_attached_file(:avatar)
-  end
+      subject.call(body, account)
+    end
 
-  it 'creates posts' do
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Status')).to_not be_nil
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Status')).to_not be_nil
-  end
+    it 'updates remote user\'s account information' do
+      account.reload
+      expect(account.display_name).to eq '::1'
+      expect(account).to have_attached_file(:avatar)
+    end
 
-  it 'ignores delete statuses unless they existed before' do
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Status')).to be_nil
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=12:objectType=Status')).to be_nil
-  end
+    it 'creates posts' do
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Status')).to_not be_nil
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Status')).to_not be_nil
+    end
 
-  it 'does not create statuses for follows' do
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Follow')).to be_nil
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Follow')).to be_nil
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=4:objectType=Follow')).to be_nil
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=7:objectType=Follow')).to be_nil
-  end
+    it 'ignores delete statuses unless they existed before' do
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Status')).to be_nil
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=12:objectType=Status')).to be_nil
+    end
+
+    it 'does not create statuses for follows' do
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Follow')).to be_nil
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Follow')).to be_nil
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=4:objectType=Follow')).to be_nil
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=7:objectType=Follow')).to be_nil
+    end
 
-  it 'does not create statuses for favourites' do
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Favourite')).to be_nil
-    expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Favourite')).to be_nil
+    it 'does not create statuses for favourites' do
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Favourite')).to be_nil
+      expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Favourite')).to be_nil
+    end
+
+    it 'creates posts with media' do
+      status = Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=14:objectType=Status')
+
+      expect(status).to_not be_nil
+      expect(status.media_attachments.first).to have_attached_file(:file)
+    end
   end
 
-  it 'creates posts with media' do
-    status = Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=14:objectType=Status')
+  it 'does not accept tampered reblogs' do
+    good_actor = Fabricate(:account, username: 'tracer', domain: 'overwatch.com')
+
+    real_body = <<XML
+<?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:overwatch.com,2017-04-27:objectId=4467137: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://overwatch.com/users/tracer</id>
+    <activity:object-type>http://activitystrea.ms/schema/1.0/person</activity:object-type>
+    <uri>https://overwatch.com/users/tracer</uri>
+    <name>tracer</name>
+  </author>
+  <content type="html">Overwatch rocks</content>
+</entry>
+XML
+
+    stub_request(:head, 'https://overwatch.com/users/tracer/updates/1').to_return(status: 200, headers: { 'Content-Type' => 'application/atom+xml' })
+    stub_request(:get, 'https://overwatch.com/users/tracer/updates/1').to_return(status: 200, body: real_body)
+
+    bad_actor = Fabricate(:account, username: 'sombra', domain: 'talon.xyz')
+
+    body = <<XML
+<?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:talon.xyz,2017-04-27:objectId=4467137:objectType=Status</id>
+  <published>2017-04-27T13:49:25Z</published>
+  <updated>2017-04-27T13:49:25Z</updated>
+  <author>
+    <id>https://talon.xyz/users/sombra</id>
+    <activity:object-type>http://activitystrea.ms/schema/1.0/person</activity:object-type>
+    <uri>https://talon.xyz/users/sombra</uri>
+    <name>sombra</name>
+  </author>
+  <activity:object-type>http://activitystrea.ms/schema/1.0/activity</activity:object-type>
+  <activity:verb>http://activitystrea.ms/schema/1.0/share</activity:verb>
+  <content type="html">Overwatch SUCKS AHAHA</content>
+  <activity:object>
+    <id>tag:overwatch.com,2017-04-27:objectId=4467137:objectType=Status</id>
+    <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://overwatch.com/users/tracer</id>
+      <activity:object-type>http://activitystrea.ms/schema/1.0/person</activity:object-type>
+      <uri>https://overwatch.com/users/tracer</uri>
+      <name>tracer</name>
+    </author>
+    <content type="html">Overwatch SUCKS AHAHA</content>
+    <link rel="alternate" type="text/html" href="https://overwatch.com/users/tracer/updates/1" />
+  </activity:object>
+</entry>
+XML
+    created_statuses = subject.call(body, bad_actor)
 
-    expect(status).to_not be_nil
-    expect(status.media_attachments.first).to have_attached_file(:file)
+    expect(created_statuses.first.reblog?).to be true
+    expect(created_statuses.first.account_id).to eq bad_actor.id
+    expect(created_statuses.first.reblog.account_id).to eq good_actor.id
+    expect(created_statuses.first.reblog.text).to eq 'Overwatch rocks'
   end
 end