From e4a241abefaa68492938c3fbb7e5e5401d12138e Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 8 Jan 2018 05:00:23 +0100 Subject: [PATCH] Fix bad URL schemes being accepted (#6219) * Fix actors accepting invalid URI schemes or different host between URI and URL * Fix statuses accepting invalid URI scheme or different host to actor * Adjust tests to new requirements * Improve readability of mismatching_origin?/invalid_origin? methods --- app/helpers/jsonld_helper.rb | 4 +++ app/lib/activitypub/activity/create.rb | 20 +++++++++-- .../activitypub/process_account_service.rb | 18 ++++++++-- spec/lib/activitypub/activity/create_spec.rb | 34 ++++++++++--------- .../fetch_remote_status_service_spec.rb | 10 +++--- 5 files changed, 62 insertions(+), 24 deletions(-) diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index 6c7c38070..9530ad9f3 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -39,6 +39,10 @@ module JsonLdHelper !json.nil? && equals_or_includes?(json['@context'], ActivityPub::TagManager::CONTEXT) end + def unsupported_uri_scheme?(uri) + !uri.start_with?('http://', 'https://') + end + def canonicalize(json) graph = RDF::Graph.new << JSON::LD::API.toRdf(json) graph.dump(:normalize) diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index d96446b56..64c429420 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -5,7 +5,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity CONVERTED_TYPES = %w(Image Video Article).freeze def perform - return if delete_arrived_first?(object_uri) || unsupported_object_type? + return if delete_arrived_first?(object_uri) || unsupported_object_type? || invalid_origin?(@object['id']) RedisLock.acquire(lock_options) do |lock| if lock.acquired? @@ -213,7 +213,14 @@ class ActivityPub::Activity::Create < ActivityPub::Activity def object_url return if @object['url'].blank? - url_to_href(@object['url'], 'text/html') + + url_candidate = url_to_href(@object['url'], 'text/html') + + if invalid_origin?(url_candidate) + nil + else + url_candidate + end end def content_language_map? @@ -245,6 +252,15 @@ class ActivityPub::Activity::Create < ActivityPub::Activity @skip_download ||= DomainBlock.find_by(domain: @account.domain)&.reject_media? end + def invalid_origin?(url) + return true if unsupported_uri_scheme?(url) + + needle = Addressable::URI.parse(url).host + haystack = Addressable::URI.parse(@account.uri).host + + !haystack.casecmp(needle).zero? + end + def reply_to_local? !replied_to_status.nil? && replied_to_status.account.local? end diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 0fbf18c00..f43edafe7 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -6,7 +6,7 @@ class ActivityPub::ProcessAccountService < BaseService # Should be called with confirmed valid JSON # and WebFinger-resolved username and domain def call(username, domain, json) - return if json['inbox'].blank? + return if json['inbox'].blank? || unsupported_uri_scheme?(json['id']) @json = json @uri = @json['id'] @@ -107,7 +107,21 @@ class ActivityPub::ProcessAccountService < BaseService def url return if @json['url'].blank? - url_to_href(@json['url'], 'text/html') + + url_candidate = url_to_href(@json['url'], 'text/html') + + if unsupported_uri_scheme?(url_candidate) || mismatching_origin?(url_candidate) + nil + else + url_candidate + end + end + + def mismatching_origin?(url) + needle = Addressable::URI.parse(url).host + haystack = Addressable::URI.parse(@uri).host + + !haystack.casecmp(needle).zero? end def outbox_total_items diff --git a/spec/lib/activitypub/activity/create_spec.rb b/spec/lib/activitypub/activity/create_spec.rb index 3c3991c13..51f54a398 100644 --- a/spec/lib/activitypub/activity/create_spec.rb +++ b/spec/lib/activitypub/activity/create_spec.rb @@ -6,7 +6,7 @@ RSpec.describe ActivityPub::Activity::Create do let(:json) do { '@context': 'https://www.w3.org/ns/activitystreams', - id: 'foo', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#foo'].join, type: 'Create', actor: ActivityPub::TagManager.instance.uri_for(sender), object: object_json, @@ -16,6 +16,8 @@ RSpec.describe ActivityPub::Activity::Create do subject { described_class.new(json, sender) } before do + sender.update(uri: ActivityPub::TagManager.instance.uri_for(sender)) + stub_request(:get, 'http://example.com/attachment.png').to_return(request_fixture('avatar.txt')) stub_request(:get, 'http://example.com/emoji.png').to_return(body: attachment_fixture('emojo.png')) end @@ -28,7 +30,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'standalone' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', } @@ -52,7 +54,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'public' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', to: 'https://www.w3.org/ns/activitystreams#Public', @@ -70,7 +72,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'unlisted' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', cc: 'https://www.w3.org/ns/activitystreams#Public', @@ -88,7 +90,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'private' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', to: 'http://example.com/followers', @@ -108,7 +110,7 @@ RSpec.describe ActivityPub::Activity::Create do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', to: ActivityPub::TagManager.instance.uri_for(recipient), @@ -128,7 +130,7 @@ RSpec.describe ActivityPub::Activity::Create do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', inReplyTo: ActivityPub::TagManager.instance.uri_for(original_status), @@ -151,7 +153,7 @@ RSpec.describe ActivityPub::Activity::Create do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', tag: [ @@ -174,7 +176,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with mentions missing href' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', tag: [ @@ -194,7 +196,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with media attachments' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', attachment: [ @@ -218,7 +220,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with media attachments missing url' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', attachment: [ @@ -239,7 +241,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with hashtags' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', tag: [ @@ -263,7 +265,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with hashtags missing name' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum', tag: [ @@ -284,7 +286,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with emojis' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum :tinking:', tag: [ @@ -310,7 +312,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with emojis missing name' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum :tinking:', tag: [ @@ -333,7 +335,7 @@ RSpec.describe ActivityPub::Activity::Create do context 'with emojis missing icon' do let(:object_json) do { - id: 'bar', + id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join, type: 'Note', content: 'Lorem ipsum :tinking:', tag: [ diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index ad26abc5b..a533e8413 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -21,6 +21,8 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do describe '#call' do before do + sender.update(uri: ActivityPub::TagManager.instance.uri_for(sender)) + stub_request(:head, 'https://example.com/watch?v=12345').to_return(status: 404, body: '') subject.call(object[:id], prefetched_body: Oj.dump(object)) end @@ -48,13 +50,13 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do { type: 'Link', mimeType: 'application/x-bittorrent', - href: 'https://example.com/12345.torrent', + href: "https://#{valid_domain}/12345.torrent", }, { type: 'Link', mimeType: 'text/html', - href: 'https://example.com/watch?v=12345', + href: "https://#{valid_domain}/watch?v=12345", }, ], } @@ -64,8 +66,8 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do status = sender.statuses.first expect(status).to_not be_nil - expect(status.url).to eq 'https://example.com/watch?v=12345' - expect(strip_tags(status.text)).to eq 'Nyan Cat 10 hours remix https://example.com/watch?v=12345' + expect(status.url).to eq "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