Skip to content

Commit

Permalink
Improve shared status verification (mastodon#2525)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@Gargron
Gargron authored Apr 27, 2017
1 parent b8e7eee commit 2af4f3c
Show file tree
Hide file tree
Showing 5 changed files with 126 additions and 41 deletions.
4 changes: 2 additions & 2 deletions Gemfile.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
12 changes: 11 additions & 1 deletion app/services/fetch_remote_status_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,19 @@ def extract_author(url, xml)

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
12 changes: 10 additions & 2 deletions app/services/process_feed_service.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ def create_status
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
Expand Down Expand Up @@ -90,6 +90,14 @@ def skip_unsupported_type?
!([: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))
Expand Down
1 change: 1 addition & 0 deletions spec/services/follow_remote_account_service_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

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:[email protected]").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:[email protected]").to_return(request_fixture('webfinger.txt'))
stub_request(:get, "https://quitter.no/.well-known/webfinger?resource=acct:[email protected]").to_return(status: 404)
Expand Down
138 changes: 102 additions & 36 deletions spec/services/process_feed_service_spec.rb
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 2af4f3c

Please sign in to comment.