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
2017-04-27 17:06:47 +02:00 · 2017-04-27 17:06:47 +02:00 · 2af4f3c4e2
commit 2af4f3c4e2
parent b8e7eee837
5 changed files with 127 additions and 42 deletions
--- 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)
--- 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
--- 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))
--- 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)
--- 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)
+    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'))
+
+      subject.call(body, account)
+    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 '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 '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
+    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 '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 'does not accept tampered reblogs' do
+    good_actor = Fabricate(:account, username: 'tracer', domain: 'overwatch.com')

-  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
+    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

-  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
+    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)

-  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
+    bad_actor = Fabricate(:account, username: 'sombra', domain: 'talon.xyz')

-  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
+    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)

-  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)
+    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