From 5dc45b5dcf374765564a533dda03befb60500709 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 19 Oct 2023 09:48:01 +1000 Subject: [PATCH] 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 | 6 +--- app/models/post.rb | 15 ++-------- lib/cooked_post_processor.rb | 8 +++++- lib/post_creator.rb | 1 - lib/post_revisor.rb | 4 --- spec/lib/cooked_post_processor_spec.rb | 40 ++++++++++++++++++++++++-- spec/lib/email/sender_spec.rb | 25 ++++++++++------ spec/lib/post_creator_spec.rb | 7 +++-- spec/lib/post_revisor_spec.rb | 4 +-- spec/models/post_spec.rb | 31 -------------------- 10 files changed, 71 insertions(+), 70 deletions(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 538413b777a..9078f72cbc9 100644 --- a/app/controllers/posts_controller.rb +++ b/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 diff --git a/app/models/post.rb b/app/models/post.rb index f62016a45bb..b8b7460d286 100644 --- a/app/models/post.rb +++ b/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 diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 0364fc209aa..bc7fbcf2890 100644 --- a/lib/cooked_post_processor.rb +++ b/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) diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 62451a4694f..cba1c6300ee 100644 --- a/lib/post_creator.rb +++ b/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] diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 498ff537ab2..a38db7f792a 100644 --- a/lib/post_revisor.rb +++ b/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 diff --git a/spec/lib/cooked_post_processor_spec.rb b/spec/lib/cooked_post_processor_spec.rb index fbbdf419bb9..97283917adb 100644 --- a/spec/lib/cooked_post_processor_spec.rb +++ b/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 diff --git a/spec/lib/email/sender_spec.rb b/spec/lib/email/sender_spec.rb index fb79a136873..15bf98c59d1 100644 --- a/spec/lib/email/sender_spec.rb +++ b/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), diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb index 66068953b76..a27cea9b953 100644 --- a/spec/lib/post_creator_spec.rb +++ b/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 diff --git a/spec/lib/post_revisor_spec.rb b/spec/lib/post_revisor_spec.rb index 13aa0bbabbf..5079a3c99cf 100644 --- a/spec/lib/post_revisor_spec.rb +++ b/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 diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 2aeaefa6276..f251b8ef6dd 100644 --- a/spec/models/post_spec.rb +++ b/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