Merge commit from fork

app/lib/activitypub/activity/accept.rb

@@ -58,7 +58,7 @@ class ActivityPub::Activity::Accept < ActivityPub::Activity
 
   def accept_quote!(quote)
     approval_uri = value_or_id(first_of_value(@json['result']))
-    return if unsupported_uri_scheme?(approval_uri) || quote.quoted_account != @account || !quote.status.local? || !quote.pending?
+    return if unsupported_uri_scheme?(approval_uri) || non_matching_uri_hosts?(approval_uri, @account.uri) || quote.quoted_account != @account || !quote.status.local? || !quote.pending?
 
     # NOTE: we are not going through `ActivityPub::VerifyQuoteService` as the `Accept` is as authoritative
     # as the stamp, but this means we are not checking the stamp, which may lead to inconsistencies

app/lib/activitypub/activity/create.rb

@@ -48,6 +48,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
     @params               = {}
     @quote                = nil
     @quote_uri            = nil
+    @quote_approval_uri   = nil
 
     process_status_params
     process_tags
@@ -229,9 +230,9 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
     @quote_uri = @status_parser.quote_uri
     return unless @status_parser.quote?
 
-    approval_uri = @status_parser.quote_approval_uri
+    @quote_approval_uri = @status_parser.quote_approval_uri
-    approval_uri = nil if unsupported_uri_scheme?(approval_uri) || TagManager.instance.local_url?(approval_uri)
+    @quote_approval_uri = nil if unsupported_uri_scheme?(@quote_approval_uri) || TagManager.instance.local_url?(@quote_approval_uri)
-    @quote = Quote.new(account: @account, approval_uri: approval_uri, legacy: @status_parser.legacy_quote?, state: @status_parser.deleted_quote? ? :deleted : :pending)
+    @quote = Quote.new(account: @account, approval_uri: nil, legacy: @status_parser.legacy_quote?, state: @status_parser.deleted_quote? ? :deleted : :pending)
   end
 
   def process_hashtag(tag)
@@ -391,9 +392,9 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
     return if @quote.nil?
 
     embedded_quote = safe_prefetched_embed(@account, @status_parser.quoted_object, @json['context'])
-    ActivityPub::VerifyQuoteService.new.call(@quote, fetchable_quoted_uri: @quote_uri, prefetched_quoted_object: embedded_quote, request_id: @options[:request_id], depth: @options[:depth])
+    ActivityPub::VerifyQuoteService.new.call(@quote, @quote_approval_uri, fetchable_quoted_uri: @quote_uri, prefetched_quoted_object: embedded_quote, request_id: @options[:request_id], depth: @options[:depth])
   rescue Mastodon::RecursionLimitExceededError, Mastodon::UnexpectedResponseError, *Mastodon::HTTP_CONNECTION_ERRORS
-    ActivityPub::RefetchAndVerifyQuoteWorker.perform_in(rand(30..600).seconds, @quote.id, @quote_uri, { 'request_id' => @options[:request_id] })
+    ActivityPub::RefetchAndVerifyQuoteWorker.perform_in(rand(30..600).seconds, @quote.id, @quote_uri, { 'request_id' => @options[:request_id], 'approval_uri' => @quote_approval_uri })
   end
 
   def conversation_from_uri(uri)

app/models/quote.rb

@@ -45,8 +45,12 @@ class Quote < ApplicationRecord
   after_destroy_commit :decrement_counter_caches!
   after_update_commit :update_counter_caches!
 
-  def accept!
+  def accept!(approval_uri: nil)
+    if approval_uri.present?
+      update!(state: :accepted, approval_uri:)
+    else
       update!(state: :accepted)
+    end
 
     reset_parent_cache! if attribute_previously_changed?(:state)
   end

app/services/activitypub/process_status_update_service.rb

@@ -52,7 +52,7 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
         create_edits!
       end
 
-      fetch_and_verify_quote!(@quote, @status_parser.quote_uri) if @quote.present?
+      fetch_and_verify_quote!(@quote, @quote_approval_uri, @status_parser.quote_uri) if @quote.present?
       download_media_files!
       queue_poll_notifications!
 
@@ -317,9 +317,9 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
     approval_uri = @status_parser.quote_approval_uri
     approval_uri = nil if unsupported_uri_scheme?(approval_uri) || TagManager.instance.local_url?(approval_uri)
 
-    quote.update(approval_uri: approval_uri, state: :pending, legacy: @status_parser.legacy_quote?) if quote.approval_uri != @status_parser.quote_approval_uri
+    quote.update(approval_uri: nil, state: :pending, legacy: @status_parser.legacy_quote?) if quote.approval_uri.present? && quote.approval_uri != @status_parser.quote_approval_uri
 
-    fetch_and_verify_quote!(quote, quote_uri)
+    fetch_and_verify_quote!(quote, approval_uri, quote_uri)
   end
 
   def update_quote!
@@ -335,18 +335,20 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
           # Revoke the quote while we get a chance… maybe this should be a `before_destroy` hook?
           RevokeQuoteService.new.call(@status.quote) if @status.quote.quoted_account&.local? && @status.quote.accepted?
           @status.quote.destroy
-          quote = Quote.create(status: @status, approval_uri: approval_uri, legacy: @status_parser.legacy_quote?, state: @status_parser.deleted_quote? ? :deleted : :pending)
+          quote = Quote.create(status: @status, approval_uri: nil, legacy: @status_parser.legacy_quote?, state: @status_parser.deleted_quote? ? :deleted : :pending)
           @quote_changed = true
         else
           quote = @status.quote
-          quote.update(approval_uri: approval_uri, state: :pending, legacy: @status_parser.legacy_quote?) if quote.approval_uri != approval_uri
+          quote.update(approval_uri: nil, state: :pending, legacy: @status_parser.legacy_quote?) if quote.approval_uri.present? && quote.approval_uri != approval_uri
         end
       else
-        quote = Quote.create(status: @status, approval_uri: approval_uri, legacy: @status_parser.legacy_quote?)
+        quote = Quote.create(status: @status, approval_uri: nil, legacy: @status_parser.legacy_quote?)
         @quote_changed = true
       end
 
       @quote = quote
+      @quote_approval_uri = approval_uri
+
       quote.save
     elsif @status.quote.present?
       @quote = nil
@@ -355,11 +357,11 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
     end
   end
 
-  def fetch_and_verify_quote!(quote, quote_uri)
+  def fetch_and_verify_quote!(quote, approval_uri, quote_uri)
     embedded_quote = safe_prefetched_embed(@account, @status_parser.quoted_object, @activity_json['context'])
-    ActivityPub::VerifyQuoteService.new.call(quote, fetchable_quoted_uri: quote_uri, prefetched_quoted_object: embedded_quote, request_id: @request_id)
+    ActivityPub::VerifyQuoteService.new.call(quote, approval_uri, fetchable_quoted_uri: quote_uri, prefetched_quoted_object: embedded_quote, request_id: @request_id)
   rescue Mastodon::UnexpectedResponseError, *Mastodon::HTTP_CONNECTION_ERRORS
-    ActivityPub::RefetchAndVerifyQuoteWorker.perform_in(rand(30..600).seconds, quote.id, quote_uri, { 'request_id' => @request_id })
+    ActivityPub::RefetchAndVerifyQuoteWorker.perform_in(rand(30..600).seconds, quote.id, quote_uri, { 'request_id' => @request_id, 'approval_uri' => approval_uri })
   end
 
   def update_counts!

app/services/activitypub/verify_quote_service.rb

@@ -6,20 +6,21 @@ class ActivityPub::VerifyQuoteService < BaseService
   MAX_SYNCHRONOUS_DEPTH = 2
 
   # Optionally fetch quoted post, and verify the quote is authorized
-  def call(quote, fetchable_quoted_uri: nil, prefetched_quoted_object: nil, prefetched_approval: nil, request_id: nil, depth: nil)
+  def call(quote, approval_uri, fetchable_quoted_uri: nil, prefetched_quoted_object: nil, prefetched_approval: nil, request_id: nil, depth: nil)
     @request_id = request_id
     @depth = depth || 0
     @quote = quote
+    @approval_uri = approval_uri.presence || @quote.approval_uri
     @fetching_error = nil
 
     fetch_quoted_post_if_needed!(fetchable_quoted_uri, prefetched_body: prefetched_quoted_object)
     return if quote.quoted_account&.local?
-    return if fast_track_approval! || quote.approval_uri.blank?
+    return if fast_track_approval! || @approval_uri.blank?
 
-    @json = fetch_approval_object(quote.approval_uri, prefetched_body: prefetched_approval)
+    @json = fetch_approval_object(@approval_uri, prefetched_body: prefetched_approval)
     return quote.reject! if @json.nil?
 
-    return if non_matching_uri_hosts?(quote.approval_uri, value_or_id(@json['attributedTo']))
+    return if non_matching_uri_hosts?(@approval_uri, value_or_id(@json['attributedTo']))
     return unless matching_type? && matching_quote_uri?
 
     # Opportunistically import embedded posts if needed
@@ -30,7 +31,7 @@ class ActivityPub::VerifyQuoteService < BaseService
 
     return unless matching_quoted_post? && matching_quoted_author?
 
-    quote.accept!
+    quote.accept!(approval_uri: @approval_uri)
   end
 
   private
@@ -87,7 +88,7 @@ class ActivityPub::VerifyQuoteService < BaseService
     object = @json['interactionTarget'].merge({ '@context' => @json['@context'] })
 
     # It's not safe to fetch if the inlined object is cross-origin or doesn't match expectations
-    return if object['id'] != uri || non_matching_uri_hosts?(@quote.approval_uri, object['id'])
+    return if object['id'] != uri || non_matching_uri_hosts?(@approval_uri, object['id'])
 
     status = ActivityPub::FetchRemoteStatusService.new.call(object['id'], prefetched_body: object, on_behalf_of: @quote.account.followers.local.first, request_id: @request_id, depth: @depth)
 

app/workers/activitypub/quote_refresh_worker.rb

@@ -10,6 +10,6 @@ class ActivityPub::QuoteRefreshWorker
     return if quote.nil? || quote.updated_at > Quote::BACKGROUND_REFRESH_INTERVAL.ago
 
     quote.touch
-    ActivityPub::VerifyQuoteService.new.call(quote)
+    ActivityPub::VerifyQuoteService.new.call(quote, quote.approval_uri)
   end
 end

app/workers/activitypub/refetch_and_verify_quote_worker.rb

@@ -9,7 +9,7 @@ class ActivityPub::RefetchAndVerifyQuoteWorker
 
   def perform(quote_id, quoted_uri, options = {})
     quote = Quote.find(quote_id)
-    ActivityPub::VerifyQuoteService.new.call(quote, fetchable_quoted_uri: quoted_uri, request_id: options[:request_id])
+    ActivityPub::VerifyQuoteService.new.call(quote, options['approval_uri'], fetchable_quoted_uri: quoted_uri, request_id: options['request_id'])
     ::DistributionWorker.perform_async(quote.status_id, { 'update' => true }) if quote.state_previously_changed?
   rescue ActiveRecord::RecordNotFound
     # Do nothing

spec/services/activitypub/process_status_update_service_spec.rb

@@ -940,10 +940,10 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService do
       stub_request(:get, approval_uri).to_return(headers: { 'Content-Type': 'application/activity+json' }, body: quote_authorization_json.to_json)
     end
 
-    it 'updates the approval URI but does not verify the quote' do
+    it 'does not update the approval URI and does not verify the quote' do
       expect { subject.call(status, json, json) }
         .to change(status, :quote).from(nil)
-      expect(status.quote.approval_uri).to eq approval_uri
+      expect(status.quote.approval_uri).to be_nil
       expect(status.quote.state).to_not eq 'accepted'
       expect(status.quote.quoted_status).to be_nil
     end

spec/services/activitypub/verify_quote_service_spec.rb

@@ -9,8 +9,9 @@ RSpec.describe ActivityPub::VerifyQuoteService do
   let(:quoted_account) { Fabricate(:account, domain: 'b.example.com') }
   let(:quoted_status) { Fabricate(:status, account: quoted_account) }
   let(:status) { Fabricate(:status, account: account) }
-  let(:quote) { Fabricate(:quote, status: status, quoted_status: quoted_status, approval_uri: approval_uri) }
+  let(:quote) { Fabricate(:quote, status: status, quoted_status: quoted_status, approval_uri: approval_uri_record) }
 
+  shared_examples 'common behavior' do
     context 'with an unfetchable approval URI' do
       let(:approval_uri) { 'https://b.example.com/approvals/1234' }
 
@@ -21,16 +22,16 @@ RSpec.describe ActivityPub::VerifyQuoteService do
 
       context 'with an already-fetched post' do
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to change(quote, :state).to('rejected')
         end
       end
 
       context 'with an already-verified quote' do
-      let(:quote) { Fabricate(:quote, status: status, quoted_status: quoted_status, approval_uri: approval_uri, state: :accepted) }
+        let(:quote) { Fabricate(:quote, status: status, quoted_status: quoted_status, approval_uri: approval_uri_record, state: :accepted) }
 
         it 'rejects the quote' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to change(quote, :state).to('revoked')
         end
       end
@@ -81,7 +82,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
 
       context 'with a valid activity for already-fetched posts' do
         it 'updates the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to change(quote, :state).to('accepted')
 
           expect(a_request(:get, approval_uri))
@@ -110,7 +111,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         end
 
         it 'updates the status' do
-        expect { subject.call(quote, fetchable_quoted_uri: 'https://b.example.com/unknown-quoted', prefetched_quoted_object: prefetched_object) }
+          expect { subject.call(quote, approval_uri_arg, fetchable_quoted_uri: 'https://b.example.com/unknown-quoted', prefetched_quoted_object: prefetched_object) }
             .to change(quote, :state).to('accepted')
 
           expect(a_request(:get, approval_uri))
@@ -139,7 +140,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         end
 
         it 'updates the status' do
-        expect { subject.call(quote, fetchable_quoted_uri: 'https://b.example.com/unknown-quoted') }
+          expect { subject.call(quote, approval_uri_arg, fetchable_quoted_uri: 'https://b.example.com/unknown-quoted') }
             .to change(quote, :state).to('accepted')
 
           expect(a_request(:get, approval_uri))
@@ -168,7 +169,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         end
 
         it 'does not update the status' do
-        expect { subject.call(quote, fetchable_quoted_uri: 'https://example.com/unknown-quoted') }
+          expect { subject.call(quote, approval_uri_arg, fetchable_quoted_uri: 'https://example.com/unknown-quoted') }
             .to not_change(quote, :state)
             .and not_change(quote, :quoted_status)
 
@@ -179,7 +180,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
 
       context 'with a valid activity for already-fetched posts, with a pre-fetched approval' do
         it 'updates the status without fetching the activity' do
-        expect { subject.call(quote, prefetched_approval: json.to_json) }
+          expect { subject.call(quote, approval_uri_arg, prefetched_approval: JSON.generate(json)) }
             .to change(quote, :state).to('accepted')
 
           expect(a_request(:get, approval_uri))
@@ -191,7 +192,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         let(:approval_uri) { 'https://evil.com/approvals/1234' }
 
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state)
         end
       end
@@ -201,7 +202,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
 
         it 'does not accept the quote' do
           # NOTE: maybe we want to skip that instead of rejecting it?
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to change(quote, :state).to('rejected')
         end
       end
@@ -210,7 +211,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         let(:approval_attributed_to) { ActivityPub::TagManager.instance.uri_for(Fabricate(:account, domain: 'b.example.com')) }
 
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state)
         end
       end
@@ -219,7 +220,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         let(:approval_interaction_target) { ActivityPub::TagManager.instance.uri_for(Fabricate(:status, account: quoted_account)) }
 
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state)
         end
       end
@@ -228,7 +229,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         let(:approval_interacting_object) { ActivityPub::TagManager.instance.uri_for(Fabricate(:status, account: account)) }
 
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state)
         end
       end
@@ -237,7 +238,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         let(:approval_type) { 'ReplyAuthorization' }
 
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state)
         end
       end
@@ -248,7 +249,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
 
       context 'without any fast-track condition' do
         it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state)
         end
       end
@@ -257,7 +258,7 @@ RSpec.describe ActivityPub::VerifyQuoteService do
         let(:quoted_account) { account }
 
         it 'updates the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to change(quote, :state).to('accepted')
         end
       end
@@ -267,10 +268,25 @@ RSpec.describe ActivityPub::VerifyQuoteService do
           quoted_status.mentions << Mention.new(account: account)
         end
 
-      it 'does not the status' do
+        it 'does not update the status' do
-        expect { subject.call(quote) }
+          expect { subject.call(quote, approval_uri_arg) }
             .to_not change(quote, :state).from('pending')
         end
       end
     end
   end
+
+  context 'when approval URI is passed as argument' do
+    let(:approval_uri_arg) { approval_uri }
+    let(:approval_uri_record) { nil }
+
+    it_behaves_like 'common behavior'
+  end
+
+  context 'when approval URI is stored in the record (legacy)' do
+    let(:approval_uri_arg) { nil }
+    let(:approval_uri_record) { approval_uri }
+
+    it_behaves_like 'common behavior'
+  end
+end

spec/workers/activitypub/quote_refresh_worker_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe ActivityPub::QuoteRefreshWorker do
         expect { worker.perform(quote.id) }
           .to(change { quote.reload.updated_at })
 
-        expect(service).to have_received(:call).with(quote)
+        expect(service).to have_received(:call).with(quote, quote.approval_uri)
       end
     end
 
@@ -31,7 +31,7 @@ RSpec.describe ActivityPub::QuoteRefreshWorker do
         expect { worker.perform(quote.id) }
           .to_not(change { quote.reload.updated_at })
 
-        expect(service).to_not have_received(:call).with(quote)
+        expect(service).to_not have_received(:call).with(quote, quote.approval_uri)
       end
     end
   end

spec/workers/activitypub/refetch_and_verify_quote_worker_spec.rb

@@ -13,11 +13,20 @@ RSpec.describe ActivityPub::RefetchAndVerifyQuoteWorker do
     let(:status)  { Fabricate(:status, account: account) }
     let(:quote)   { Fabricate(:quote, status: status, quoted_status: nil) }
     let(:url) { 'https://example.com/quoted-status' }
+    let(:approval_uri) { 'https://example.com/approval-uri' }
 
+    it 'sends the status to the service' do
+      worker.perform(quote.id, url, { 'approval_uri' => approval_uri })
+
+      expect(service).to have_received(:call).with(quote, approval_uri, fetchable_quoted_uri: url, request_id: anything)
+    end
+
+    context 'with the old format' do
       it 'sends the status to the service' do
         worker.perform(quote.id, url)
 
-      expect(service).to have_received(:call).with(quote, fetchable_quoted_uri: url, request_id: anything)
+        expect(service).to have_received(:call).with(quote, nil, fetchable_quoted_uri: url, request_id: anything)
+      end
     end
 
     it 'returns nil for non-existent record' do