Fix: フレンドサーバー申請時、ドメインを偽装して無関係のInboxを指定できる脆弱性 (#934)

app/lib/activitypub/activity/follow.rb

@@ -60,19 +60,13 @@ class ActivityPub::Activity::Follow < ActivityPub::Activity
       already_accepted = friend.accepted?
       friend.update!(passive_state: :pending, active_state: :idle, passive_follow_activity_id: @json['id'])
     else
-      @friend = FriendDomain.new(domain: @account.domain, passive_state: :pending, passive_follow_activity_id: @json['id'])
+      @friend = FriendDomain.create!(domain: @account.domain, passive_state: :pending, passive_follow_activity_id: @json['id'], inbox_url: @account.preferred_inbox_url)
-      @friend.inbox_url = @json['inboxUrl'].presence || @friend.default_inbox_url
-      @friend.save!
     end
 
-    if already_accepted || Setting.unlocked_friend
+    friend.accept! if already_accepted || Setting.unlocked_friend
-      friend.accept!
 
-      # Notify for admin even if unlocked
+    # Notify for admin
     notify_staff_about_pending_friend_server! unless already_accepted
-    else
-      notify_staff_about_pending_friend_server!
-    end
   end
 
   def friend

app/models/friend_domain.rb

@@ -116,6 +116,7 @@ class FriendDomain < ApplicationRecord
       object: ActivityPub::TagManager::COLLECTIONS[:public],
 
       # Cannot use inbox_url method because this model also has inbox_url column
+      # This is deprecated property. Newer version's kmyblue will ignore it.
       inboxUrl: "https://#{Rails.configuration.x.web_domain}/inbox",
     }
   end

spec/lib/activitypub/activity/follow_spec.rb

@@ -380,11 +380,10 @@ RSpec.describe ActivityPub::Activity::Follow do
   context 'when given a friend server' do
     subject { described_class.new(json, sender) }
 
-    let(:sender) { Fabricate(:account, domain: 'abc.com', url: 'https://abc.com/#actor') }
+    let(:sender) { Fabricate(:account, domain: 'abc.com', url: 'https://abc.com/#actor', shared_inbox_url: 'https://abc.com/shared_inbox') }
     let!(:friend) { Fabricate(:friend_domain, domain: 'abc.com', inbox_url: 'https://example.com/inbox', passive_state: :idle) }
     let!(:owner_user) { Fabricate(:user, role: UserRole.find_by(name: 'Owner')) }
     let!(:patch_user) { Fabricate(:user, role: Fabricate(:user_role, name: 'OhagiOps', permissions: UserRole::FLAGS[:manage_federation])) }
-    let(:inbox_url) { nil }
 
     let(:json) do
       {
@@ -393,7 +392,6 @@ RSpec.describe ActivityPub::Activity::Follow do
         type: 'Follow',
         actor: ActivityPub::TagManager.instance.uri_for(sender),
         object: 'https://www.w3.org/ns/activitystreams#Public',
-        inboxUrl: inbox_url,
       }.with_indifferent_access
     end
 
@@ -415,25 +413,34 @@ RSpec.describe ActivityPub::Activity::Follow do
         expect(friend).to_not be_nil
         expect(friend.they_are_pending?).to be true
         expect(friend.passive_follow_activity_id).to eq 'foo'
-        expect(friend.inbox_url).to eq 'https://abc.com/inbox'
+        expect(friend.inbox_url).to eq 'https://abc.com/shared_inbox'
       end
     end
 
-    context 'when no record and inbox_url is specified' do
+    context 'when old spec which no record and inbox_url is specified' do
-      let(:inbox_url) { 'https://ohagi.com/inbox' }
+      let(:json) do
+        {
+          '@context': 'https://www.w3.org/ns/activitystreams',
+          id: 'foo',
+          type: 'Follow',
+          actor: ActivityPub::TagManager.instance.uri_for(sender),
+          object: 'https://www.w3.org/ns/activitystreams#Public',
+          inboxUrl: 'https://evil.org/bad_inbox',
+        }.with_indifferent_access
+      end
 
       before do
         friend.destroy!
       end
 
-      it 'marks the friend as pending' do
+      it 'marks the friend as pending but inboxUrl is not working' do
         subject.perform
 
         friend = FriendDomain.find_by(domain: 'abc.com')
         expect(friend).to_not be_nil
         expect(friend.they_are_pending?).to be true
         expect(friend.passive_follow_activity_id).to eq 'foo'
-        expect(friend.inbox_url).to eq 'https://ohagi.com/inbox'
+        expect(friend.inbox_url).to eq 'https://abc.com/shared_inbox'
       end
     end
 

spec/models/friend_domain_spec.rb

@@ -21,7 +21,6 @@ RSpec.describe FriendDomain do
         type: 'Follow',
         actor: 'https://cb6e6126.ngrok.io/actor',
         object: 'https://www.w3.org/ns/activitystreams#Public',
-        inboxUrl: 'https://cb6e6126.ngrok.io/inbox',
       }))).to have_been_made.once
     end
   end