FIX: Secure upload post processing race condition (#23968)

* FIX: Secure upload post processing race condition

This commit fixes a couple of issues.

A little background -- when uploads are created in the composer
for posts, regardless of whether the upload will eventually be
marked secure or not, if secure_uploads is enabled we always mark
the upload secure at first. This is so the upload is by default
protected, regardless of post type (regular or PM) or category.

This was causing issues in some rare occasions though because
of the order of operations of our post creation and processing
pipeline. When creating a post, we enqueue a sidekiq job to
post-process the post which does various things including
converting images to lightboxes. We were also enqueuing a job
to update the secure status for all uploads in that post.

Sometimes the secure status job would run before the post process
job, marking uploads as _not secure_ in the background and changing
their ACL before the post processor ran, which meant the users
would see a broken image in their posts. This commit fixes that issue
by always running the upload security changes inline _within_ the
cooked_post_processor job.

The other issue was that the lightbox wrapper link for images in
the post would end up with a URL like this:

```
href="/secure-uploads/original/2X/4/4e1f00a40b6c952198bbdacae383ba77932fc542.jpeg"
```

Since we weren't actually using the `upload.url` to pass to
`UrlHelper.cook_url` here, we weren't converting this href to the CDN
URL if the post was not in a secure context (the UrlHelper does not
know how to convert a secure-uploads URL to a CDN one). Now we
always end up with the correct lightbox href. This was less of an issue
than the other one, since the secure-uploads URL works even when the
upload has become non-secure, but it was a good inconsistency to fix
anyway.

app/controllers/posts_controller.rb

@@ -647,11 +647,7 @@ class PostsController < ApplicationController
     guardian.ensure_can_rebake!
 
     post = find_post_from_params
-    post.rebake!(
-      invalidate_oneboxes: true,
-      invalidate_broken_images: true,
-      update_upload_security: true,
-    )
+    post.rebake!(invalidate_oneboxes: true, invalidate_broken_images: true)
 
     render body: nil
   end

app/models/post.rb

@@ -748,12 +748,7 @@ class Post < ActiveRecord::Base
     problems
   end
 
-  def rebake!(
-    invalidate_broken_images: false,
-    invalidate_oneboxes: false,
-    priority: nil,
-    update_upload_security: false
-  )
+  def rebake!(invalidate_broken_images: false, invalidate_oneboxes: false, priority: nil)
     new_cooked = cook(raw, topic_id: topic_id, invalidate_oneboxes: invalidate_oneboxes)
     old_cooked = cooked
 
@@ -770,10 +765,6 @@ class Post < ActiveRecord::Base
     TopicLink.extract_from(self)
     QuotedPost.extract_from(self)
 
-    # Settings may have changed before rebake, so any uploads linked to the post
-    # should have their secure status reexamined.
-    update_uploads_secure_status(source: "post rebake") if update_upload_security
-
     # make sure we trigger the post process
     trigger_post_process(bypass_bump: true, priority: priority)
 
@@ -1084,8 +1075,8 @@ class Post < ActiveRecord::Base
   end
 
   def update_uploads_secure_status(source:)
-    if Discourse.store.external? && SiteSetting.secure_uploads?
-      Jobs.enqueue(:update_post_uploads_secure_status, post_id: self.id, source: source)
+    if Discourse.store.external?
+      self.uploads.each { |upload| upload.update_secure_status(source: source) }
     end
   end
 

lib/cooked_post_processor.rb

@@ -37,6 +37,7 @@ class CookedPostProcessor
   def post_process(new_post: false)
     DistributedMutex.synchronize("post_process_#{@post.id}", validity: 10.minutes) do
       DiscourseEvent.trigger(:before_post_process_cooked, @doc, @post)
+      update_uploads_secure_status
       remove_full_quote_on_direct_reply if new_post
       post_process_oneboxes
       post_process_images
@@ -84,6 +85,10 @@ class CookedPostProcessor
       end
   end
 
+  def update_uploads_secure_status
+    @post.update_uploads_secure_status(source: "post processor")
+  end
+
   def remove_full_quote_on_direct_reply
     return if !SiteSetting.remove_full_quote
     return if @post.post_number == 1
@@ -276,7 +281,8 @@ class CookedPostProcessor
     lightbox.add_child(img)
 
     # then, the link to our larger image
-    src = UrlHelper.cook_url(img["src"], secure: @post.with_secure_uploads?)
+    src_url = Upload.secure_uploads_url?(img["src"]) ? upload&.url : img["src"]
+    src = UrlHelper.cook_url(src_url || img["src"], secure: @post.with_secure_uploads?)
     a = create_link_node("lightbox", src)
     img.add_next_sibling(a)
 

lib/post_creator.rb

@@ -193,7 +193,6 @@ class PostCreator
         update_user_counts
         create_embedded_topic
         @post.link_post_uploads
-        @post.update_uploads_secure_status(source: "post creator")
         delete_owned_bookmarks
         ensure_in_allowed_users if guardian.is_staff?
         unarchive_message if !@opts[:import_mode]

lib/post_revisor.rb

@@ -311,10 +311,6 @@ class PostRevisor
     # leading to corrupt state
     QuotedPost.extract_from(@post)
 
-    # This must be done before post_process_post, because that uses
-    # post upload security status to cook URLs.
-    @post.update_uploads_secure_status(source: "post revisor")
-
     post_process_post
 
     update_topic_word_counts

spec/lib/cooked_post_processor_spec.rb

@@ -516,8 +516,6 @@ RSpec.describe CookedPostProcessor do
 
           context "when the upload is attached to the correct post" do
             before do
-              FastImage.expects(:size).returns([1750, 2000])
-              OptimizedImage.expects(:resize).returns(true)
               Discourse
                 .store
                 .class
@@ -525,15 +523,51 @@ RSpec.describe CookedPostProcessor do
                 .expects(:has_been_uploaded?)
                 .at_least_once
                 .returns(true)
-              upload.update(secure: true, access_control_post: post)
+              upload.update!(secure: true, access_control_post: post)
+              post.link_post_uploads
             end
 
             # TODO fix this spec, it is sometimes getting CDN links when it runs concurrently
             xit "handles secure images with the correct lightbox link href" do
+              FastImage.expects(:size).returns([1750, 2000])
+              OptimizedImage.expects(:resize).returns(true)
               cpp.post_process
 
               expect(cpp.html).to match_html cooked_html
             end
+
+            context "when the upload was not secure" do
+              before { upload.update!(secure: false) }
+
+              it "changes the secure status" do
+                cpp.post_process
+                expect(upload.reload.secure).to eq(true)
+              end
+            end
+
+            context "when the upload should no longer be considered secure" do
+              before { SiteSetting.login_required = false }
+
+              it "changes the secure status" do
+                cpp.post_process
+                expect(upload.reload.secure).to eq(false)
+              end
+
+              it "does not use a secure-uploads URL for the lightbox href" do
+                SiteSetting.create_thumbnails = false
+                SiteSetting.max_image_width = 10
+                SiteSetting.max_image_height = 10
+
+                cpp.post_process
+                expect(cpp.html).not_to have_tag(
+                  "a",
+                  with: {
+                    class: "lightbox",
+                    href: "//test.localhost/secure-uploads/original/1X/#{upload.sha1}.png",
+                  },
+                )
+              end
+            end
           end
 
           context "when the upload is attached to a different post" do

spec/lib/email/sender_spec.rb

@@ -587,7 +587,7 @@ RSpec.describe Email::Sender do
         SiteSetting.secure_uploads_max_email_embed_image_size_kb = 5_000
 
         Jobs.run_immediately!
-        Jobs::PullHotlinkedImages.any_instance.expects(:execute)
+        Jobs::PullHotlinkedImages.any_instance.expects(:execute).at_least_once
         FileStore::S3Store.any_instance.expects(:has_been_uploaded?).returns(true).at_least_once
         CookedPostProcessor.any_instance.stubs(:get_size).returns([244, 66])
 
@@ -596,7 +596,9 @@ RSpec.describe Email::Sender do
           UploadCreator.new(@secure_image_file, "logo.png").create_for(Discourse.system_user.id)
         @secure_image.update_secure_status(override: true)
         @secure_image.update(access_control_post_id: reply.id)
-        reply.update(raw: reply.raw + "\n" + "#{UploadMarkdown.new(@secure_image).image_markdown}")
+        reply.update!(raw: reply.raw + "\n" + "#{UploadMarkdown.new(@secure_image).image_markdown}")
+        reply.uploads << @secure_image
+        reply.save
         reply.rebake!
       end
 
@@ -629,7 +631,14 @@ RSpec.describe Email::Sender do
           expect(message.to_s.scan(/cid:[\w\-@.]+/).uniq.length).to eq(2)
         end
 
-        it "does not attach images that are not marked as secure" do
+        it "does not attach images that are not marked as secure, in the case of a non-secure upload copied to a PM" do
+          SiteSetting.login_required = false
+          @secure_image.update_secure_status(override: false)
+          @secure_image.update!(access_control_post: Fabricate(:post))
+          pm_topic = Fabricate(:private_message_topic)
+          Fabricate(:post, topic: pm_topic)
+          reply.update(topic: pm_topic)
+          reply.rebake!
           Email::Sender.new(message, :valid_type).send
           expect(message.attachments.length).to eq(4)
         end
@@ -642,9 +651,9 @@ RSpec.describe Email::Sender do
 
         it "uses the email styles to inline secure images and attaches the secure image upload to the email" do
           Email::Sender.new(message, :valid_type).send
-          expect(message.attachments.length).to eq(4)
+          expect(message.attachments.length).to eq(5)
           expect(message.attachments.map(&:filename)).to contain_exactly(
-            *[small_pdf, large_pdf, csv_file, @secure_image].map(&:original_filename),
+            *[small_pdf, large_pdf, csv_file, image, @secure_image].map(&:original_filename),
           )
           expect(message.attachments["logo.png"].body.raw_source.force_encoding("UTF-8")).to eq(
             File.read(@secure_image_file),
@@ -656,7 +665,7 @@ RSpec.describe Email::Sender do
         it "embeds an image with a secure URL that has an upload that is not secure" do
           @secure_image.update_secure_status(override: false)
           Email::Sender.new(message, :valid_type).send
-          expect(message.attachments.length).to eq(4)
+          expect(message.attachments.length).to eq(5)
           expect(message.attachments["logo.png"].body.raw_source.force_encoding("UTF-8")).to eq(
             File.read(@secure_image_file),
           )
@@ -681,9 +690,9 @@ RSpec.describe Email::Sender do
 
           it "uses the email styles and the optimized image to inline secure images and attaches the secure image upload to the email" do
             Email::Sender.new(message, :valid_type).send
-            expect(message.attachments.length).to eq(4)
+            expect(message.attachments.length).to eq(5)
             expect(message.attachments.map(&:filename)).to contain_exactly(
-              *[small_pdf, large_pdf, csv_file, @secure_image].map(&:original_filename),
+              *[small_pdf, large_pdf, csv_file, image, @secure_image].map(&:original_filename),
             )
             expect(message.attachments["logo.png"].body.raw_source.force_encoding("UTF-8")).to eq(
               File.read(optimized_image_file),

spec/lib/post_creator_spec.rb

@@ -2091,7 +2091,7 @@ RSpec.describe PostCreator do
     end
   end
 
-  describe "secure uploads uploads" do
+  describe "secure uploads" do
     fab!(:image_upload) { Fabricate(:upload, secure: true) }
     fab!(:user2) { Fabricate(:user) }
     fab!(:public_topic) { Fabricate(:topic) }
@@ -2104,12 +2104,13 @@ RSpec.describe PostCreator do
     end
 
     it "links post uploads" do
-      _public_post =
+      public_post =
         PostCreator.create(
           user,
           topic_id: public_topic.id,
-          raw: "A public post with an image.\n![](#{image_upload.short_path})",
+          raw: "A public post with an image.\n![secure image](#{image_upload.short_path})",
         )
+      expect(public_post.reload.uploads.map(&:access_control_post_id)).to eq([public_post.id])
     end
   end
 

spec/lib/post_revisor_spec.rb

@@ -1549,7 +1549,7 @@ RSpec.describe PostRevisor do
 
           expect(image5.reload.secure).to eq(false)
           expect(image5.security_last_changed_reason).to eq(
-            "access control post dictates security | source: post revisor",
+            "access control post dictates security | source: post processor",
           )
         end
 
@@ -1563,7 +1563,7 @@ RSpec.describe PostRevisor do
 
           expect(image5.reload.secure).to eq(true)
           expect(image5.security_last_changed_reason).to eq(
-            "access control post dictates security | source: post revisor",
+            "access control post dictates security | source: post processor",
           )
         end
       end

spec/models/post_spec.rb

@@ -1392,37 +1392,6 @@ RSpec.describe Post do
     ensure
       InlineOneboxer.invalidate("http://testonebox.com/vvf22")
     end
-
-    context "when secure uploads are enabled" do
-      before do
-        setup_s3
-        SiteSetting.secure_uploads = true
-      end
-
-      it "does not enqueue job to update secure status by default" do
-        post = create_post
-        expect_not_enqueued_with(
-          job: :update_post_uploads_secure_status,
-          args: {
-            post_id: post.id,
-            source: "post rebake",
-          },
-        ) { post.rebake! }
-      end
-
-      context "when passing update_upload_security: true option" do
-        it "does enqueue job to update secure status" do
-          post = create_post
-          expect_enqueued_with(
-            job: :update_post_uploads_secure_status,
-            args: {
-              post_id: post.id,
-              source: "post rebake",
-            },
-          ) { post.rebake!(update_upload_security: true) }
-        end
-      end
-    end
   end
 
   describe "#set_owner" do