Merge pull request #513 from kmycode/kb-draft-5.15-lts

Release: 5.15 LTS

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 All notable changes to this project will be documented in this file.
 
+## [4.2.5] - 2024-02-01
+
+### Security
+
+- Fix insufficient origin validation (CVE-2024-23832, [GHSA-3fjr-858r-92rw](https://github.com/mastodon/mastodon/security/advisories/GHSA-3fjr-858r-92rw))
+
 ## [4.2.4] - 2024-01-24
 
 ### Fixed

SECURITY.md

@@ -14,9 +14,7 @@ A "vulnerability in Mastodon" is a vulnerability in the code distributed through
 ## Supported Versions
 
 | Version | Supported |
-| ------- | ---------------- |
+| ------- | --------- |
 | 4.2.x   | Yes       |
 | 4.1.x   | Yes       |
-| 4.0.x   | No               |
+| < 4.1   | No        |
-| 3.5.x   | Until 2023-12-31 |
-| < 3.5   | No               |

app/controllers/activitypub/references_controller.rb

@@ -5,8 +5,6 @@ class ActivityPub::ReferencesController < ActivityPub::BaseController
   include Authorization
   include AccountOwnedConcern
 
-  REFERENCES_LIMIT = 5
-
   before_action :require_signature!, if: :authorized_fetch_mode?
   before_action :set_status
 
@@ -40,17 +38,21 @@ class ActivityPub::ReferencesController < ActivityPub::BaseController
     @results ||= begin
       references = @status.reference_objects.order(target_status_id: :asc)
       references = references.where('target_status_id > ?', page_params[:min_id]) if page_params[:min_id].present?
-      references = references.limit(limit_param(REFERENCES_LIMIT))
+      references = references.limit(limit_param(references_limit))
       references.pluck(:target_status_id)
     end
   end
 
+  def references_limit
+    StatusReference::REFERENCES_LIMIT
+  end
+
   def pagination_min_id
     results.last
   end
 
   def records_continue?
-    results.size == limit_param(REFERENCES_LIMIT)
+    results.size == limit_param(references_limit)
   end
 
   def references_collection_presenter

app/controllers/concerns/signature_verification.rb

@@ -266,7 +266,7 @@ module SignatureVerification
       stoplight_wrap_request { ResolveAccountService.new.call(key_id.delete_prefix('acct:'), suppress_errors: false) }
     elsif !ActivityPub::TagManager.instance.local_uri?(key_id)
       account   = ActivityPub::TagManager.instance.uri_to_actor(key_id)
-      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

app/helpers/jsonld_helper.rb

@@ -155,8 +155,8 @@ module JsonLdHelper
     end
   end
 
-  def fetch_resource(uri, id, on_behalf_of = nil, request_options: {})
+  def fetch_resource(uri, id_is_known, on_behalf_of = nil, request_options: {})
-    unless id
+    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'])

app/lib/activitypub/activity.rb

@@ -154,7 +154,7 @@ class ActivityPub::Activity
     if object_uri.start_with?('http')
       return if ActivityPub::TagManager.instance.local_uri?(object_uri)
 
-      ActivityPub::FetchRemoteStatusService.new.call(object_uri, id: true, on_behalf_of: @account.followers.local.first, request_id: @options[:request_id])
+      ActivityPub::FetchRemoteStatusService.new.call(object_uri, on_behalf_of: @account.followers.local.first, request_id: @options[:request_id])
     elsif @object['url'].present?
       ::FetchRemoteStatusService.new.call(@object['url'], request_id: @options[:request_id])
     end

app/lib/activitypub/linked_data_signature.rb

@@ -19,7 +19,7 @@ class ActivityPub::LinkedDataSignature
     return unless type == 'RsaSignature2017'
 
     creator = ActivityPub::TagManager.instance.uri_to_actor(creator_uri)
-    creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri, id: false) if creator&.public_key.blank?
+    creator = ActivityPub::FetchRemoteKeyService.new.call(creator_uri) if creator&.public_key.blank?
 
     return if creator.nil?
 

app/models/status_reference.rb

@@ -13,6 +13,8 @@
 #
 
 class StatusReference < ApplicationRecord
+  REFERENCES_LIMIT = 5
+
   belongs_to :status
   belongs_to :target_status, class_name: 'Status'
 

app/services/activitypub/fetch_references_service.rb

@@ -6,7 +6,7 @@ class ActivityPub::FetchReferencesService < BaseService
   def call(status, collection_or_uri)
     @account = status.account
 
-    collection_items(collection_or_uri)&.map { |item| value_or_id(item) }
+    collection_items(collection_or_uri)&.take(8)&.map { |item| value_or_id(item) }
   end
 
   private
@@ -20,9 +20,9 @@ class ActivityPub::FetchReferencesService < BaseService
 
     case collection['type']
     when 'Collection', 'CollectionPage'
-      collection['items']
+      as_array(collection['items'])
     when 'OrderedCollection', 'OrderedCollectionPage'
-      collection['orderedItems']
+      as_array(collection['orderedItems'])
     end
   end
 
@@ -31,6 +31,19 @@ class ActivityPub::FetchReferencesService < BaseService
     return if unsupported_uri_scheme?(collection_or_uri)
     return if ActivityPub::TagManager.instance.local_uri?(collection_or_uri)
 
+    # NOTE: For backward compatibility reasons, Mastodon signs outgoing
+    # queries incorrectly by default.
+    #
+    # While this is relevant for all URLs with query strings, this is
+    # the only code path where this happens in practice.
+    #
+    # Therefore, retry with correct signatures if this fails.
+    begin
       fetch_resource_without_id_validation(collection_or_uri, nil, true)
+    rescue Mastodon::UnexpectedResponseError => e
+      raise unless e.response && e.response.code == 401 && Addressable::URI.parse(collection_or_uri).query.present?
+
+      fetch_resource_without_id_validation(collection_or_uri, nil, true, request_options: { with_query_string: true })
+    end
   end
 end

app/services/activitypub/fetch_remote_account_service.rb

@@ -2,7 +2,7 @@
 
 class ActivityPub::FetchRemoteAccountService < ActivityPub::FetchRemoteActorService
   # 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, request_id: nil)
+  def call(uri, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil)
     actor = super
     return actor if actor.nil? || actor.is_a?(Account)
 

app/services/activitypub/fetch_remote_actor_service.rb

@@ -10,15 +10,15 @@ class ActivityPub::FetchRemoteActorService < BaseService
   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, request_id: nil)
+  def call(uri, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true, request_id: nil)
     return if domain_not_allowed?(uri)
     return ActivityPub::TagManager.instance.uri_to_actor(uri) 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}"

app/services/activitypub/fetch_remote_key_service.rb

@@ -6,23 +6,10 @@ class ActivityPub::FetchRemoteKeyService < BaseService
   class Error < StandardError; end
 
   # Returns actor 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?
+    @json = fetch_resource(uri, false)
-      if id
-        @json = fetch_resource_without_id_validation(uri)
-        if actor_type?
-          @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)
-    end
 
     raise Error, "Unable to fetch key JSON at #{uri}" if @json.nil?
     raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?(@json)

app/services/activitypub/fetch_remote_status_service.rb

@@ -8,14 +8,14 @@ class ActivityPub::FetchRemoteStatusService < BaseService
   DISCOVERIES_PER_REQUEST = 1000
 
   # Should be called when uri has already been checked for locality
-  def call(uri, id: true, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil)
+  def call(uri, prefetched_body: nil, on_behalf_of: nil, expected_actor_uri: nil, request_id: nil)
     return if domain_not_allowed?(uri)
 
     @request_id = request_id || "#{Time.now.utc.to_i}-status-#{uri}"
     @json = 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
 
     return unless supported_context?
@@ -65,7 +65,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService
 
   def account_from_uri(uri)
     actor = ActivityPub::TagManager.instance.uri_to_resource(uri, Account)
-    actor = ActivityPub::FetchRemoteAccountService.new.call(uri, id: true, request_id: @request_id) if actor.nil? || actor.possibly_stale?
+    actor = ActivityPub::FetchRemoteAccountService.new.call(uri, request_id: @request_id) if actor.nil? || actor.possibly_stale?
     actor
   end
 

app/services/activitypub/process_account_service.rb

@@ -383,7 +383,7 @@ class ActivityPub::ProcessAccountService < BaseService
 
   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, request_id: @options[:request_id])
+    account ||= ActivityPub::FetchRemoteAccountService.new.call(@json['movedTo'], break_on_redirect: true, request_id: @options[:request_id])
     account
   end
 

app/services/fetch_resource_service.rb

@@ -48,7 +48,15 @@ class FetchResourceService < BaseService
       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::FetchRemoteActorService::SUPPORTED_TYPES) || expected_type?(json))
+      return unless supported_context?(json) && (equals_or_includes_any?(json['type'], ActivityPub::FetchRemoteActorService::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)
 

docker-compose.yml

@@ -56,7 +56,7 @@ services:
 
   web:
     build: .
-    image: ghcr.io/mastodon/mastodon:v4.2.4
+    image: ghcr.io/mastodon/mastodon:v4.2.5
     restart: always
     env_file: .env.production
     command: bash -c "rm -f /mastodon/tmp/pids/server.pid; bundle exec rails s -p 3000"
@@ -77,7 +77,7 @@ services:
 
   streaming:
     build: .
-    image: ghcr.io/mastodon/mastodon:v4.2.4
+    image: ghcr.io/mastodon/mastodon:v4.2.5
     restart: always
     env_file: .env.production
     command: node ./streaming
@@ -95,7 +95,7 @@ services:
 
   sidekiq:
     build: .
-    image: ghcr.io/mastodon/mastodon:v4.2.4
+    image: ghcr.io/mastodon/mastodon:v4.2.5
     restart: always
     env_file: .env.production
     command: bundle exec sidekiq

lib/mastodon/version.rb

@@ -9,7 +9,7 @@ module Mastodon
     end
 
     def kmyblue_minor
-      14
+      15
     end
 
     def kmyblue_flag
@@ -25,7 +25,7 @@ module Mastodon
     end
 
     def patch
-      4
+      5
     end
 
     def default_prerelease

spec/lib/activitypub/linked_data_signature_spec.rb

@@ -60,7 +60,7 @@ RSpec.describe ActivityPub::LinkedDataSignature do
 
         allow(ActivityPub::FetchRemoteKeyService).to receive(:new).and_return(service_stub)
 
-        allow(service_stub).to receive(:call).with('http://example.com/alice', id: false) do
+        allow(service_stub).to receive(:call).with('http://example.com/alice') do
           sender.update!(public_key: old_key)
           sender
         end
@@ -68,7 +68,7 @@ RSpec.describe ActivityPub::LinkedDataSignature do
 
       it 'fetches key and returns creator' do
         expect(subject.verify_actor!).to eq sender
-        expect(service_stub).to have_received(:call).with('http://example.com/alice', id: false).once
+        expect(service_stub).to have_received(:call).with('http://example.com/alice').once
       end
     end
 

spec/services/activitypub/fetch_references_service_spec.rb

@@ -0,0 +1,128 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe ActivityPub::FetchReferencesService, type: :service do
+  subject { described_class.new.call(status, payload) }
+
+  let(:actor)          { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/account') }
+  let(:status)         { Fabricate(:status, account: actor) }
+  let(:collection_uri) { 'http://example.com/references/1' }
+
+  let(:items) do
+    [
+      'http://example.com/self-references-1',
+      'http://example.com/self-references-2',
+      'http://example.com/self-references-3',
+      'http://other.com/other-references-1',
+      'http://other.com/other-references-2',
+      'http://other.com/other-references-3',
+      'http://example.com/self-references-4',
+      'http://example.com/self-references-5',
+      'http://example.com/self-references-6',
+      'http://example.com/self-references-7',
+      'http://example.com/self-references-8',
+    ]
+  end
+
+  let(:payload) do
+    {
+      '@context': 'https://www.w3.org/ns/activitystreams',
+      type: 'Collection',
+      id: collection_uri,
+      items: items,
+    }.with_indifferent_access
+  end
+
+  describe '#call' do
+    context 'when the payload is a Collection with inlined replies' do
+      context 'when there is a single reference, with the array compacted away' do
+        let(:items) { 'http://example.com/self-references-1' }
+
+        it 'a item is returned' do
+          expect(subject).to eq ['http://example.com/self-references-1']
+        end
+      end
+
+      context 'when passing the collection itself' do
+        it 'first 8 items are returned' do
+          expect(subject).to eq items.take(8)
+        end
+      end
+
+      context 'when passing the URL to the collection' do
+        subject { described_class.new.call(status, collection_uri) }
+
+        before do
+          stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload))
+        end
+
+        it 'first 8 items are returned' do
+          expect(subject).to eq items.take(8)
+        end
+      end
+    end
+
+    context 'when the payload is an OrderedCollection with inlined references' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          type: 'OrderedCollection',
+          id: collection_uri,
+          orderedItems: items,
+        }.with_indifferent_access
+      end
+
+      context 'when passing the collection itself' do
+        it 'first 8 items are returned' do
+          expect(subject).to eq items.take(8)
+        end
+      end
+
+      context 'when passing the URL to the collection' do
+        subject { described_class.new.call(status, collection_uri) }
+
+        before do
+          stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload))
+        end
+
+        it 'first 8 items are returned' do
+          expect(subject).to eq items.take(8)
+        end
+      end
+    end
+
+    context 'when the payload is a paginated Collection with inlined references' do
+      let(:payload) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          type: 'Collection',
+          id: collection_uri,
+          first: {
+            type: 'CollectionPage',
+            partOf: collection_uri,
+            items: items,
+          },
+        }.with_indifferent_access
+      end
+
+      context 'when passing the collection itself' do
+        it 'first 8 items are returned' do
+          expect(subject).to eq items.take(8)
+        end
+      end
+
+      context 'when passing the URL to the collection' do
+        subject { described_class.new.call(status, collection_uri) }
+
+        before do
+          stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload))
+        end
+
+        it 'first 8 items are returned' do
+          expect(subject).to eq items.take(8)
+        end
+      end
+    end
+  end
+end

spec/services/activitypub/fetch_remote_account_service_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do
   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

spec/services/activitypub/fetch_remote_actor_service_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe ActivityPub::FetchRemoteActorService, type: :service do
   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

spec/services/activitypub/fetch_remote_key_service_spec.rb

@@ -56,7 +56,7 @@ RSpec.describe ActivityPub::FetchRemoteKeyService, type: :service do
   end
 
   describe '#call' do
-    let(:account) { subject.call(public_key_id, id: false) }
+    let(:account) { subject.call(public_key_id) }
 
     context 'when the key is a sub-object from the actor' do
       before do

spec/services/fetch_resource_service_spec.rb

@@ -57,7 +57,7 @@ RSpec.describe FetchResourceService, type: :service do
 
       let(:json) do
         {
-          id: 1,
+          id: 'http://example.com/foo',
           '@context': ActivityPub::TagManager::CONTEXT,
           type: 'Note',
         }.to_json
@@ -83,27 +83,27 @@ RSpec.describe FetchResourceService, type: :service do
         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
 
       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

spec/services/resolve_url_service_spec.rb

@@ -139,6 +139,7 @@ describe ResolveURLService, type: :service do
         stub_request(:get, url).to_return(status: 302, headers: { 'Location' => status_url })
         body = ActiveModelSerializers::SerializableResource.new(status, serializer: ActivityPub::NoteSerializer, adapter: ActivityPub::Adapter).to_json
         stub_request(:get, status_url).to_return(body: body, headers: { 'Content-Type' => 'application/activity+json' })
+        stub_request(:get, uri).to_return(body: body, headers: { 'Content-Type' => 'application/activity+json' })
       end
 
       it 'returns status by url' do