From 13a062a5d9f05d5ca476c2159d560261f7011b2b Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Tue, 14 Aug 2018 20:24:47 +0200 Subject: [PATCH 1/4] Upgrade Doorkeeper to 4.4.1 (#8197) --- Gemfile | 2 +- Gemfile.lock | 4 ++-- ..._change_account_id_nonnullable_in_lists.rb | 2 -- ..._confidential_to_doorkeeper_application.rb | 23 +++++++++++++++++++ db/schema.rb | 3 ++- 5 files changed, 28 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20180814171349_add_confidential_to_doorkeeper_application.rb diff --git a/Gemfile b/Gemfile index 7a6e1568d..c9e74455a 100644 --- a/Gemfile +++ b/Gemfile @@ -41,7 +41,7 @@ gem 'omniauth-cas', '~> 1.1' gem 'omniauth-saml', '~> 1.10' gem 'omniauth', '~> 1.2' -gem 'doorkeeper', '~> 4.2', '< 4.3' +gem 'doorkeeper', '~> 4.4' gem 'fast_blank', '~> 1.0' gem 'fastimage' gem 'goldfinger', '~> 2.1' diff --git a/Gemfile.lock b/Gemfile.lock index e1929a05c..213a4418e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -181,7 +181,7 @@ GEM docile (1.3.0) domain_name (0.5.20180417) unf (>= 0.0.5, < 1.0.0) - doorkeeper (4.2.6) + doorkeeper (4.4.1) railties (>= 4.2) dotenv (2.2.2) dotenv-rails (2.2.2) @@ -670,7 +670,7 @@ DEPENDENCIES devise (~> 4.4) devise-two-factor (~> 3.0) devise_pam_authenticatable2 (~> 9.1) - doorkeeper (~> 4.2, < 4.3) + doorkeeper (~> 4.4) dotenv-rails (~> 2.2, < 2.3) fabrication (~> 2.20) faker (~> 1.8) diff --git a/db/migrate/20171201000000_change_account_id_nonnullable_in_lists.rb b/db/migrate/20171201000000_change_account_id_nonnullable_in_lists.rb index 120f74402..3369e3b9e 100644 --- a/db/migrate/20171201000000_change_account_id_nonnullable_in_lists.rb +++ b/db/migrate/20171201000000_change_account_id_nonnullable_in_lists.rb @@ -1,5 +1,3 @@ -require Rails.root.join('lib', 'mastodon', 'migration_helpers') - class ChangeAccountIdNonnullableInLists < ActiveRecord::Migration[5.1] def change change_column_null :lists, :account_id, false diff --git a/db/migrate/20180814171349_add_confidential_to_doorkeeper_application.rb b/db/migrate/20180814171349_add_confidential_to_doorkeeper_application.rb new file mode 100644 index 000000000..7077a4e65 --- /dev/null +++ b/db/migrate/20180814171349_add_confidential_to_doorkeeper_application.rb @@ -0,0 +1,23 @@ +require Rails.root.join('lib', 'mastodon', 'migration_helpers') + +class AddConfidentialToDoorkeeperApplication < ActiveRecord::Migration[5.2] + include Mastodon::MigrationHelpers + + disable_ddl_transaction! + + def up + safety_assured do + add_column_with_default( + :oauth_applications, + :confidential, + :boolean, + allow_null: false, + default: true # maintaining backwards compatibility: require secrets + ) + end + end + + def down + remove_column :oauth_applications, :confidential + end +end diff --git a/db/schema.rb b/db/schema.rb index 02032c548..a7d81d57e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2018_07_07_154237) do +ActiveRecord::Schema.define(version: 2018_08_14_171349) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -339,6 +339,7 @@ ActiveRecord::Schema.define(version: 2018_07_07_154237) do t.string "website" t.string "owner_type" t.bigint "owner_id" + t.boolean "confidential", default: true, null: false t.index ["owner_id", "owner_type"], name: "index_oauth_applications_on_owner_id_and_owner_type" t.index ["uid"], name: "index_oauth_applications_on_uid", unique: true end From 31a209cb015022733cdc6e6f5f2fac614a1e70b3 Mon Sep 17 00:00:00 2001 From: ThibG Date: Mon, 20 Aug 2018 22:42:02 +0200 Subject: [PATCH 2/4] Upgrade doorkeeper to 4.4.2 (#8321) --- Gemfile.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 213a4418e..2da3fe324 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -181,7 +181,7 @@ GEM docile (1.3.0) domain_name (0.5.20180417) unf (>= 0.0.5, < 1.0.0) - doorkeeper (4.4.1) + doorkeeper (4.4.2) railties (>= 4.2) dotenv (2.2.2) dotenv-rails (2.2.2) @@ -269,7 +269,7 @@ GEM httplog (1.0.2) colorize (~> 0.8) rack (>= 1.0) - i18n (1.0.1) + i18n (1.1.0) concurrent-ruby (~> 1.0) i18n-tasks (0.9.21) activesupport (>= 4.0.2) @@ -347,7 +347,7 @@ GEM net-ssh (>= 2.6.5) net-ssh (4.2.0) nio4r (2.3.0) - nokogiri (1.8.2) + nokogiri (1.8.4) mini_portile2 (~> 2.3.0) nokogumbo (1.5.0) nokogiri @@ -415,7 +415,7 @@ GEM puma (3.11.4) pundit (1.1.0) activesupport (>= 3.0.0) - rack (2.0.4) + rack (2.0.5) rack-attack (5.2.0) rack rack-cors (1.0.2) @@ -423,7 +423,7 @@ GEM rack rack-proxy (0.6.4) rack - rack-test (1.0.0) + rack-test (1.1.0) rack (>= 1.0, < 3) rails (5.2.0) actioncable (= 5.2.0) @@ -764,4 +764,4 @@ RUBY VERSION ruby 2.5.0p0 BUNDLED WITH - 1.16.2 + 1.16.3 From f100e843725cc6e2dfdbb6dc2b1bfc2fa939bb63 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 22 Aug 2018 20:55:14 +0200 Subject: [PATCH 3/4] Improve federated ID validation (#8372) * Fix URI not being sufficiently validated with prefetched JSON * Add additional id validation to OStatus documents, when possible --- app/helpers/jsonld_helper.rb | 6 ++- app/lib/ostatus/activity/creation.rb | 11 +++- .../fetch_remote_account_service.rb | 2 +- .../activitypub/fetch_remote_key_service.rb | 2 +- .../fetch_remote_status_service.rb | 2 +- app/services/fetch_remote_account_service.rb | 7 ++- .../fetch_remote_account_service_spec.rb | 7 ++- .../fetch_remote_status_service_spec.rb | 22 ++++++++ .../fetch_remote_account_service_spec.rb | 20 ++++++- .../fetch_remote_status_service_spec.rb | 52 +++++++++++++++++++ 10 files changed, 122 insertions(+), 9 deletions(-) diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index 9d2b6cf00..532397272 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -73,8 +73,10 @@ module JsonLdHelper 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 nil end diff --git a/app/lib/ostatus/activity/creation.rb b/app/lib/ostatus/activity/creation.rb index d3a303a0c..8f8c70052 100644 --- a/app/lib/ostatus/activity/creation.rb +++ b/app/lib/ostatus/activity/creation.rb @@ -7,7 +7,7 @@ class OStatus::Activity::Creation < OStatus::Activity::Base return [nil, false] end - return [nil, false] if @account.suspended? + return [nil, false] if @account.suspended? || invalid_origin? RedisLock.acquire(lock_options) do |lock| if lock.acquired? @@ -204,6 +204,15 @@ class OStatus::Activity::Creation < OStatus::Activity::Base 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 { redis: Redis.current, key: "create:#{id}" } end diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index 867e70876..6fa4e9a1b 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/app/services/activitypub/fetch_remote_account_service.rb @@ -11,7 +11,7 @@ class ActivityPub::FetchRemoteAccountService < BaseService @json = if prefetched_body.nil? fetch_resource(uri, id) else - body_to_json(prefetched_body) + body_to_json(prefetched_body, compare_id: id ? uri : nil) end return unless supported_context? && expected_type? diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb index 505baccd4..df17d9079 100644 --- a/app/services/activitypub/fetch_remote_key_service.rb +++ b/app/services/activitypub/fetch_remote_key_service.rb @@ -17,7 +17,7 @@ class ActivityPub::FetchRemoteKeyService < BaseService @json = fetch_resource(uri, id) end else - @json = body_to_json(prefetched_body) + @json = body_to_json(prefetched_body, compare_id: id ? uri : nil) end return unless supported_context?(@json) && expected_type? diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index 2b447abb3..469821032 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -8,7 +8,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService @json = if prefetched_body.nil? fetch_resource(uri, id, on_behalf_of) else - body_to_json(prefetched_body) + body_to_json(prefetched_body, compare_id: id ? uri : nil) end return unless supported_context? && expected_type? diff --git a/app/services/fetch_remote_account_service.rb b/app/services/fetch_remote_account_service.rb index a0f031a44..cfc560022 100644 --- a/app/services/fetch_remote_account_service.rb +++ b/app/services/fetch_remote_account_service.rb @@ -27,7 +27,7 @@ class FetchRemoteAccountService < BaseService 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 rescue TypeError @@ -37,4 +37,9 @@ class FetchRemoteAccountService < BaseService Rails.logger.debug 'Invalid XML or missing namespace' nil 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 diff --git a/spec/services/activitypub/fetch_remote_account_service_spec.rb b/spec/services/activitypub/fetch_remote_account_service_spec.rb index dba55c034..aa13f0a9b 100644 --- a/spec/services/activitypub/fetch_remote_account_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_account_service_spec.rb @@ -59,7 +59,6 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do it 'returns nil' do expect(account).to be_nil end - end 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' 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 diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index 549eb80fa..9ae409996 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -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" 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 diff --git a/spec/services/fetch_remote_account_service_spec.rb b/spec/services/fetch_remote_account_service_spec.rb index 1c3abe8f3..20dd505d0 100644 --- a/spec/services/fetch_remote_account_service_spec.rb +++ b/spec/services/fetch_remote_account_service_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe FetchRemoteAccountService, type: :service do - let(:url) { 'https://example.com' } + let(:url) { 'https://example.com/alice' } let(:prefetched_body) { nil } let(:protocol) { :ostatus } subject { FetchRemoteAccountService.new.call(url, prefetched_body, protocol) } @@ -46,6 +46,24 @@ RSpec.describe FetchRemoteAccountService, type: :service do end include_examples 'return Account' + + it 'does not update account information if XML comes from an unverified domain' do + feed_xml = <<-XML.squish + + + + http://activitystrea.ms/schema/1.0/person + http://kickass.zone/users/localhost + localhost + localhost + Villain!!! + + + 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 context 'when prefetched_body is nil' do diff --git a/spec/services/fetch_remote_status_service_spec.rb b/spec/services/fetch_remote_status_service_spec.rb index 0df9c329a..f9db024b9 100644 --- a/spec/services/fetch_remote_status_service_spec.rb +++ b/spec/services/fetch_remote_status_service_spec.rb @@ -32,4 +32,56 @@ RSpec.describe FetchRemoteStatusService, type: :service do expect(status.text).to eq 'Lorem ipsum' 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 + + + tag:real.domain,2017-04-27:objectId=4487555:objectType=Status + 2017-04-27T13:49:25Z + 2017-04-27T13:49:25Z + http://activitystrea.ms/schema/1.0/note + http://activitystrea.ms/schema/1.0/post + + https://real.domain/users/tracer + http://activitystrea.ms/schema/1.0/person + https://real.domain/users/tracer + tracer + + Overwatch rocks + + 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 + + + https://other-real.domain/statuses/123 + 2017-04-27T13:49:25Z + 2017-04-27T13:49:25Z + http://activitystrea.ms/schema/1.0/note + http://activitystrea.ms/schema/1.0/post + + https://real.domain/users/tracer + http://activitystrea.ms/schema/1.0/person + https://real.domain/users/tracer + tracer + + Overwatch rocks + + XML + + expect(subject.call('https://real.domain/statuses/456', status_body, :ostatus)).to be_nil + end + end end From 612d02028c9b33cda250207778ed3cb7fb24e317 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 22 Aug 2018 20:56:43 +0200 Subject: [PATCH 4/4] Bump version to 2.4.4 --- lib/mastodon/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index 04894d34d..87099675b 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -13,7 +13,7 @@ module Mastodon end def patch - 3 + 4 end def pre