Skip to content

Commit

Permalink
Fix insufficient origin validation
Browse files Browse the repository at this point in the history
  • Loading branch information
ClearlyClaire committed Feb 1, 2024
1 parent ad2d2e8 commit 766619a
Show file tree
Hide file tree
Showing 11 changed files with 35 additions and 35 deletions.
2 changes: 1 addition & 1 deletion app/controllers/concerns/signature_verification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ def account_from_key_id(key_id)
stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, ''), suppress_errors: false) }
elsif !ActivityPub::TagManager.instance.local_uri?(key_id)
account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account)
account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false, suppress_errors: false) }
account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, suppress_errors: false) }
account
end
rescue Mastodon::PrivateNetworkAddressError => e
Expand Down
4 changes: 2 additions & 2 deletions app/helpers/jsonld_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,8 @@ def safe_for_forwarding?(original, compacted)
end
end

def fetch_resource(uri, id, on_behalf_of = nil)
unless id
def fetch_resource(uri, id_is_known, on_behalf_of = nil)
unless id_is_known
json = fetch_resource_without_id_validation(uri, on_behalf_of)

return if !json.is_a?(Hash) || unsupported_uri_scheme?(json['id'])
Expand Down
4 changes: 2 additions & 2 deletions app/lib/activitypub/activity.rb
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,9 @@ def fetch_remote_original_status
return if ActivityPub::TagManager.instance.local_uri?(object_uri)

if dereferenced?
ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true, prefetched_body: @object)
ActivityPub::FetchRemoteStatusService.new.call(object_uri, prefetched_body: @object)
else
ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true, on_behalf_of: @account.followers.local.first)
ActivityPub::FetchRemoteStatusService.new.call(object_uri, on_behalf_of: @account.followers.local.first)
end
elsif @object['url'].present?
::FetchRemoteStatusService.new.call(@object['url'])
Expand Down
2 changes: 1 addition & 1 deletion app/lib/activitypub/linked_data_signature.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def verify_account!
return unless type == 'RsaSignature2017'

creator = ActivityPub::TagManager.instance.uri_to_resource(creator_uri, Account)
creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false)
creator ||= ActivityPub::FetchRemoteKeyService.new.call(creator_uri)

return if creator.nil?

Expand Down
6 changes: 3 additions & 3 deletions app/services/activitypub/fetch_remote_account_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ class Error < StandardError; end
SUPPORTED_TYPES = %w(Application Group Organization Person Service).freeze

# Does a WebFinger roundtrip on each call, unless `only_key` is true
def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true)
def call(uri, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true)
return if domain_not_allowed?(uri)
return ActivityPub::TagManager.instance.uri_to_resource(uri, Account) if ActivityPub::TagManager.instance.local_uri?(uri)

@json = begin
if prefetched_body.nil?
fetch_resource(uri, id)
fetch_resource(uri, true)
else
body_to_json(prefetched_body, compare_id: id ? uri : nil)
body_to_json(prefetched_body, compare_id: uri)
end
rescue Oj::ParseError
raise Error, "Error parsing JSON-LD document #{uri}"
Expand Down
20 changes: 6 additions & 14 deletions app/services/activitypub/fetch_remote_key_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,14 @@ class ActivityPub::FetchRemoteKeyService < BaseService
class Error < StandardError; end

# Returns account that owns the key
def call(uri, id: true, prefetched_body: nil, suppress_errors: true)
def call(uri, suppress_errors: true)
raise Error, 'No key URI given' if uri.blank?

if prefetched_body.nil?
if id
@json = fetch_resource_without_id_validation(uri)
if person?
@json = fetch_resource(@json['id'], true)
elsif uri != @json['id']
raise Error, "Fetched URI #{uri} has wrong id #{@json['id']}"
end
else
@json = fetch_resource(uri, id)
end
else
@json = body_to_json(prefetched_body, compare_id: id ? uri : nil)
@json = fetch_resource_without_id_validation(uri)
if person?
@json = fetch_resource(@json['id'], true)
elsif uri != @json['id']
raise Error, "Fetched URI #{uri} has wrong id #{@json['id']}"
end

raise Error, "Unable to fetch key JSON at #{uri}" if @json.nil?
Expand Down
8 changes: 4 additions & 4 deletions app/services/activitypub/fetch_remote_status_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@ class ActivityPub::FetchRemoteStatusService < BaseService
include JsonLdHelper

# Should be called when uri has already been checked for locality
def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil)
def call(uri, prefetched_body: nil, on_behalf_of: nil)
@json = begin
if prefetched_body.nil?
fetch_resource(uri, id, on_behalf_of)
fetch_resource(uri, true, on_behalf_of)
else
body_to_json(prefetched_body, compare_id: id ? uri : nil)
body_to_json(prefetched_body, compare_id: uri)
end
end

Expand All @@ -29,7 +29,7 @@ def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil)
return if activity_json.nil? || !trustworthy_attribution?(@json['id'], actor_id)

actor = ActivityPub::TagManager.instance.uri_to_resource(actor_id, Account)
actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id, id: true) if actor.nil? || needs_update?(actor)
actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id) if actor.nil? || needs_update?(actor)

return if actor.nil? || actor.suspended?

Expand Down
2 changes: 1 addition & 1 deletion app/services/activitypub/process_account_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def collection_info(type)

def moved_account
account = ActivityPub::TagManager.instance.uri_to_resource(@json['movedTo'], Account)
account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], id: true, break_on_redirect: true)
account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], break_on_redirect: true)
account
end

Expand Down
10 changes: 9 additions & 1 deletion app/services/fetch_resource_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,15 @@ def process_response(response, terminal = false)
body = response.body_with_limit
json = body_to_json(body)

[json['id'], { prefetched_body: body, id: true }] if supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) || expected_type?(json))
return unless supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) || expected_type?(json))

if json['id'] != @url
return if terminal

return process(json['id'], terminal: true)
end

[@url, { prefetched_body: body }]
elsif !terminal
link_header = response['Link'] && parse_link_header(response)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
end

describe '#call' do
let(:account) { subject.call('https://example.com/alice', id: true) }
let(:account) { subject.call('https://example.com/alice') }

shared_examples 'sets profile data' do
it 'returns an account' do
Expand Down
10 changes: 5 additions & 5 deletions spec/services/fetch_resource_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@

let(:json) do
{
id: 1,
id: 'http://example.com/foo',
'@context': ActivityPub::TagManager::CONTEXT,
type: 'Note',
}.to_json
Expand All @@ -79,14 +79,14 @@
let(:content_type) { 'application/activity+json; charset=utf-8' }
let(:body) { json }

it { is_expected.to eq [1, { prefetched_body: body, id: true }] }
it { is_expected.to eq ['http://example.com/foo', { prefetched_body: body }] }
end

context 'when content type is ld+json with profile' do
let(:content_type) { 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"' }
let(:body) { json }

it { is_expected.to eq [1, { prefetched_body: body, id: true }] }
it { is_expected.to eq ['http://example.com/foo', { prefetched_body: body }] }
end

before do
Expand All @@ -97,14 +97,14 @@
context 'when link header is present' do
let(:headers) { { 'Link' => '<http://example.com/foo>; rel="alternate"; type="application/activity+json"', } }

it { is_expected.to eq [1, { prefetched_body: json, id: true }] }
it { is_expected.to eq ['http://example.com/foo', { prefetched_body: json }] }
end

context 'when content type is text/html' do
let(:content_type) { 'text/html' }
let(:body) { '<html><head><link rel="alternate" href="http://example.com/foo" type="application/activity+json"/></head></html>' }

it { is_expected.to eq [1, { prefetched_body: json, id: true }] }
it { is_expected.to eq ['http://example.com/foo', { prefetched_body: json }] }
end
end
end
Expand Down

0 comments on commit 766619a

Please sign in to comment.