From 4646a38ae6595c97dc3736dd67ec2c1edd72de93 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 23 Jan 2020 09:31:46 +1000 Subject: [PATCH] FIX: Use presigned URL to avoid 403 when pulling hotlinked images for secure media (#8764) When we were pulling hotlinked images for oneboxes in the CookedPostProcessor, we were using the direct S3 URL, which returned a 403 error and thus did not set widths and heights of the images. We now cook the URL first based on whether the upload is secure before handing off to FastImage. --- lib/cooked_post_processor.rb | 17 +++-- spec/components/cooked_post_processor_spec.rb | 64 ++++++++++++++----- 2 files changed, 60 insertions(+), 21 deletions(-) diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index c4cd361dd1a..17af21d2f78 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -277,14 +277,18 @@ class CookedPostProcessor absolute_url = url absolute_url = Discourse.base_url_no_prefix + absolute_url if absolute_url =~ /^\/[^\/]/ - if url&.start_with?("/secure-media-uploads/") - absolute_url = Discourse.store.signed_url_for_path(url.sub("/secure-media-uploads/", "")) - end - return unless absolute_url # FastImage fails when there's no scheme absolute_url = SiteSetting.scheme + ":" + absolute_url if absolute_url.start_with?("//") + + # we can't direct FastImage to our secure-media-uploads url because it bounces + # anonymous requests with a 404 error + if url&.include?("/secure-media-uploads/") + secure_upload_s3_path = absolute_url.sub(Discourse.base_url, "").sub("/secure-media-uploads/", "") + absolute_url = Discourse.store.signed_url_for_path(secure_upload_s3_path) + end + return unless is_valid_image_url?(absolute_url) # we can *always* crawl our own images @@ -539,7 +543,10 @@ class CookedPostProcessor upload_id = downloaded_images[src] upload = Upload.find_by_id(upload_id) if upload_id - img["src"] = upload.url if upload.present? + + if upload.present? + img["src"] = UrlHelper.cook_url(upload.url, secure: @post.with_secure_media?) + end # make sure we grab dimensions for oneboxed images # and wrap in a div diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index f087616a936..8905905ba82 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -976,7 +976,8 @@ describe CookedPostProcessor do let(:cpp) { CookedPostProcessor.new(post, invalidate_oneboxes: true) } before do - Oneboxer.expects(:onebox) + Oneboxer + .expects(:onebox) .with("http://www.youtube.com/watch?v=9bZkp7q19f0", invalidate_oneboxes: true, user_id: nil, category_id: post.topic.category_id) .returns("
GANGNAM STYLE
") @@ -988,28 +989,59 @@ describe CookedPostProcessor do expect(cpp.html).to match_html "
GANGNAM STYLE
" end - it "replaces downloaded onebox image" do - url = 'https://image.com/my-avatar' - image_url = 'https://image.com/avatar.png' + describe "replacing downloaded onebox image" do + let(:url) { 'https://image.com/my-avatar' } + let(:image_url) { 'https://image.com/avatar.png' } - Oneboxer.stubs(:onebox).with(url, anything).returns("") + it "successfully replaces the image" do + Oneboxer.stubs(:onebox).with(url, anything).returns("") - post = Fabricate(:post, raw: url) - upload.update!(url: "https://test.s3.amazonaws.com/something.png") + post = Fabricate(:post, raw: url) + upload.update!(url: "https://test.s3.amazonaws.com/something.png") - post.custom_fields[Post::DOWNLOADED_IMAGES] = { "//image.com/avatar.png": upload.id } - post.save_custom_fields + post.custom_fields[Post::DOWNLOADED_IMAGES] = { "//image.com/avatar.png": upload.id } + post.save_custom_fields - cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true) - cpp.post_process_oneboxes + cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true) + cpp.post_process_oneboxes - expect(cpp.doc.to_s).to eq("

") + expect(cpp.doc.to_s).to eq("

") - upload.destroy! - cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true) - cpp.post_process_oneboxes + upload.destroy! + cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true) + cpp.post_process_oneboxes - expect(cpp.doc.to_s).to eq("

") + expect(cpp.doc.to_s).to eq("

") + Oneboxer.unstub(:onebox) + end + + context "when the post is with_secure_media and the upload is secure and secure media is enabled" do + before do + upload.update(secure: true) + SiteSetting.login_required = true + s3_setup + SiteSetting.secure_media = true + stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/") + end + + it "does not use the direct URL, uses the cooked URL instead (because of the private ACL preventing w/h fetch)" do + Oneboxer.stubs(:onebox).with(url, anything).returns("") + + post = Fabricate(:post, raw: url) + upload.update!(url: "https://test.s3.amazonaws.com/something.png") + + post.custom_fields[Post::DOWNLOADED_IMAGES] = { "//image.com/avatar.png": upload.id } + post.save_custom_fields + + cooked_url = "https://localhost/secure-media-uploads/test.png" + UrlHelper.expects(:cook_url).with(upload.url, secure: true).returns(cooked_url) + + cpp = CookedPostProcessor.new(post, invalidate_oneboxes: true) + cpp.post_process_oneboxes + + expect(cpp.doc.to_s).to eq("

") + end + end end it "replaces large image placeholder" do