* Fix URI not being sufficiently validated with prefetched JSON * Add additional id validation to OStatus documents, when possiblemaster
@@ -73,8 +73,10 @@ module JsonLdHelper | |||||
end | end | ||||
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 | rescue Oj::ParseError | ||||
nil | nil | ||||
end | end | ||||
@@ -7,7 +7,7 @@ class OStatus::Activity::Creation < OStatus::Activity::Base | |||||
return [nil, false] | return [nil, false] | ||||
end | end | ||||
return [nil, false] if @account.suspended? | |||||
return [nil, false] if @account.suspended? || invalid_origin? | |||||
RedisLock.acquire(lock_options) do |lock| | RedisLock.acquire(lock_options) do |lock| | ||||
if lock.acquired? | if lock.acquired? | ||||
@@ -204,6 +204,15 @@ class OStatus::Activity::Creation < OStatus::Activity::Base | |||||
end | end | ||||
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 | def lock_options | ||||
{ redis: Redis.current, key: "create:#{id}" } | { redis: Redis.current, key: "create:#{id}" } | ||||
end | end | ||||
@@ -11,7 +11,7 @@ class ActivityPub::FetchRemoteAccountService < BaseService | |||||
@json = if prefetched_body.nil? | @json = if prefetched_body.nil? | ||||
fetch_resource(uri, id) | fetch_resource(uri, id) | ||||
else | else | ||||
body_to_json(prefetched_body) | |||||
body_to_json(prefetched_body, compare_id: id ? uri : nil) | |||||
end | end | ||||
return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?) | return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?) | ||||
@@ -17,7 +17,7 @@ class ActivityPub::FetchRemoteKeyService < BaseService | |||||
@json = fetch_resource(uri, id) | @json = fetch_resource(uri, id) | ||||
end | end | ||||
else | else | ||||
@json = body_to_json(prefetched_body) | |||||
@json = body_to_json(prefetched_body, compare_id: id ? uri : nil) | |||||
end | end | ||||
return unless supported_context?(@json) && expected_type? | return unless supported_context?(@json) && expected_type? | ||||
@@ -8,7 +8,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService | |||||
@json = if prefetched_body.nil? | @json = if prefetched_body.nil? | ||||
fetch_resource(uri, id, on_behalf_of) | fetch_resource(uri, id, on_behalf_of) | ||||
else | else | ||||
body_to_json(prefetched_body) | |||||
body_to_json(prefetched_body, compare_id: id ? uri : nil) | |||||
end | end | ||||
return unless supported_context? && expected_type? | return unless supported_context? && expected_type? | ||||
@@ -27,7 +27,7 @@ class FetchRemoteAccountService < BaseService | |||||
account = author_from_xml(xml.at_xpath('/xmlns:feed', xmlns: OStatus::TagManager::XMLNS), false) | 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 | account | ||||
rescue TypeError | rescue TypeError | ||||
@@ -37,4 +37,9 @@ class FetchRemoteAccountService < BaseService | |||||
Rails.logger.debug 'Invalid XML or missing namespace' | Rails.logger.debug 'Invalid XML or missing namespace' | ||||
nil | nil | ||||
end | 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 | end |
@@ -59,7 +59,6 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do | |||||
it 'returns nil' do | it 'returns nil' do | ||||
expect(account).to be_nil | expect(account).to be_nil | ||||
end | end | ||||
end | end | ||||
context 'when URI and WebFinger share the same host' do | 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' | include_examples 'sets profile data' | ||||
end | 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 | ||||
end | end |
@@ -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" | expect(strip_tags(status.text)).to eq "Nyan Cat 10 hours remix https://#{valid_domain}/watch?v=12345" | ||||
end | end | ||||
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 | ||||
end | end |
@@ -1,7 +1,7 @@ | |||||
require 'rails_helper' | require 'rails_helper' | ||||
RSpec.describe FetchRemoteAccountService, type: :service do | RSpec.describe FetchRemoteAccountService, type: :service do | ||||
let(:url) { 'https://example.com' } | |||||
let(:url) { 'https://example.com/alice' } | |||||
let(:prefetched_body) { nil } | let(:prefetched_body) { nil } | ||||
let(:protocol) { :ostatus } | let(:protocol) { :ostatus } | ||||
subject { FetchRemoteAccountService.new.call(url, prefetched_body, protocol) } | subject { FetchRemoteAccountService.new.call(url, prefetched_body, protocol) } | ||||
@@ -46,6 +46,24 @@ RSpec.describe FetchRemoteAccountService, type: :service do | |||||
end | end | ||||
include_examples 'return Account' | 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 | end | ||||
context 'when prefetched_body is nil' do | context 'when prefetched_body is nil' do | ||||
@@ -32,4 +32,56 @@ RSpec.describe FetchRemoteStatusService, type: :service do | |||||
expect(status.text).to eq 'Lorem ipsum' | expect(status.text).to eq 'Lorem ipsum' | ||||
end | end | ||||
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 | end |