FIX: Post uploads setting access_control_post_id unnecessarily (#26627)

This commit addresses an issue for sites where secure_uploads
is turned on after the site has been operating without it for
some time.

When uploads are linked when they are used inside a post,
we were setting the access_control_post_id unconditionally
if it was NULL to that post ID and secure_uploads was true.

However this causes issues if an upload has been used in a
few different places, especially if a post was previously
used in a PM and marked secure, so we end up with a case of
the upload using a public post for its access control, which
causes URLs to not use the /secure-uploads/ path in the post,
breaking things like image uploads.

We should only set the access_control_post_id if the post is the first time the
upload is referenced so it cannot hijack uploads from other places.

app/models/post.rb

@@ -1093,11 +1093,28 @@ class Post < ActiveRecord::Base
       UploadReference.where(target: self).delete_all
       UploadReference.insert_all(upload_references) if upload_references.size > 0
 
+      # NOTE: The access_control_post_id needs to be set whenever a post is linked to an upload
+      # when secure uploads is enabled because it is later used for UploadSecurity checks.
+      # The caveat here is that access_control_post_id should only be set if this post is
+      # the first time the upload is referenced, otherwise we can end up hijacking upload
+      # ownership from other posts.
       if SiteSetting.secure_uploads?
+        uploads_in_post =
           Upload
+            .joins(:upload_references)
+            .includes(:upload_references)
             .where(id: upload_ids, access_control_post_id: nil)
-          .where("id NOT IN (SELECT upload_id FROM custom_emojis)")
-          .update_all(access_control_post_id: self.id)
+            .where("uploads.id NOT IN (SELECT upload_id FROM custom_emojis)")
+
+        access_control_will_change_upload_ids =
+          uploads_in_post.filter_map do |upl|
+            first_ref = upl.upload_references.min_by { |ur| [ur.created_at, ur.id] }
+            upl.id if first_ref.blank? || first_ref.target?(self)
+          end
+
+        Upload.where(id: access_control_will_change_upload_ids).update_all(
+          access_control_post_id: self.id,
+        )
       end
     end
   end

app/models/upload_reference.rb

@@ -6,6 +6,10 @@ class UploadReference < ActiveRecord::Base
 
   delegate :to_markdown, to: :upload
 
+  def target?(target_to_check)
+    self.target_id == target_to_check.id && self.target_type == target_to_check.class.to_s
+  end
+
   def self.ensure_exist!(upload_ids: [], target: nil, target_type: nil, target_id: nil)
     if !target && !(target_type && target_id)
       raise "target OR target_type and target_id are required"

spec/integration/secure_uploads_spec.rb

@@ -40,6 +40,7 @@ describe "Secure uploads" do
 
   it "does not convert an upload to secure when it was first used in a site setting then in a post" do
     upload = create_upload
+    stub_presign_upload_get(upload)
     SiteSetting.favicon = upload
     expect(upload.reload.upload_references.count).to eq(1)
     create_post(
@@ -55,6 +56,7 @@ describe "Secure uploads" do
 
   it "does not convert an upload to insecure when it was first used in a secure post then a site setting" do
     upload = create_upload
+    stub_presign_upload_get(upload)
     create_post(
       title: "Secure upload post",
       raw: "This is a new post <img src=\"#{upload.url}\" />",

spec/lib/post_revisor_spec.rb

@@ -1542,7 +1542,8 @@ RSpec.describe PostRevisor do
       end
 
       context "with secure uploads uploads" do
-        let!(:image5) { Fabricate(:secure_upload) }
+        let!(:image5) { Fabricate(:secure_upload, access_control_post: post) }
+
         before do
           Jobs.run_immediately!
           setup_s3

spec/models/post_spec.rb

@@ -1732,28 +1732,54 @@ RSpec.describe Post do
         expect(post.reload.upload_references.pluck(:id)).to_not contain_exactly(post_uploads_ids)
       end
 
-      context "when secure uploads is enabled" do
+      describe "setting access_control_post on linked uploads" do
+        fab!(:other_post) { Fabricate(:post) }
+
         before do
           setup_s3
           SiteSetting.authorized_extensions = "pdf|png|jpg|csv"
           SiteSetting.secure_uploads = true
+          SiteSetting.login_required = true
         end
 
-        it "sets the access_control_post_id on uploads in the post that don't already have the value set" do
-          other_post = Fabricate(:post)
-          video_upload.update(access_control_post_id: other_post.id)
-          audio_upload.update(access_control_post_id: other_post.id)
+        context "for uploads in the post which have a null access_control_post_id" do
+          before do
+            image_upload.update!(access_control_post_id: nil)
+            video_upload.update!(access_control_post_id: other_post.id)
+            audio_upload.update!(access_control_post_id: other_post.id)
+          end
 
+          it "does nothing if secure uploads is disabled" do
+            SiteSetting.secure_uploads = false
             post.link_post_uploads
-
             image_upload.reload
-          video_upload.reload
-          expect(image_upload.access_control_post_id).to eq(post.id)
+            expect(image_upload.access_control_post_id).to eq(nil)
+          end
+
+          it "sets access_control_post_id to the current post if it is the first time the upload is being referenced" do
+            post.link_post_uploads
+            expect(image_upload.reload.access_control_post_id).to eq(post.id)
+          end
+
+          it "does not set access_control_post_id if the current post is not the first time the upload is being referenced" do
+            UploadReference.create!(
+              upload: image_upload,
+              target: Fabricate(:post),
+              created_at: 1.year.ago,
+            )
+            post.link_post_uploads
+            expect(image_upload.reload.access_control_post_id).not_to eq(post.id)
+          end
+
+          it "does not set access_control_post_id for uploads that already have one" do
+            post.link_post_uploads
             expect(video_upload.access_control_post_id).not_to eq(post.id)
+            expect(audio_upload.access_control_post_id).not_to eq(post.id)
           end
 
           context "for custom emoji" do
             before { CustomEmoji.create(name: "meme", upload: image_upload) }
+
             it "never sets an access control post because they should not be secure" do
               post.link_post_uploads
               expect(image_upload.reload.access_control_post_id).to eq(nil)
@@ -1761,6 +1787,7 @@ RSpec.describe Post do
           end
         end
       end
+    end
 
     describe "#update_uploads_secure_status" do
       fab!(:user) { Fabricate(:user, trust_level: TrustLevel[0]) }