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

@@ -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

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("<div>GANGNAM STYLE</div>")
 
@@ -988,28 +989,59 @@ describe CookedPostProcessor do
       expect(cpp.html).to match_html "<div>GANGNAM STYLE</div>"
     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("<img class='onebox' src='#{image_url}' />")
+      it "successfully replaces the image" do
+        Oneboxer.stubs(:onebox).with(url, anything).returns("<img class='onebox' src='#{image_url}' />")
 
-      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("<p><img class=\"onebox\" src=\"#{upload.url}\" width=\"\" height=\"\"></p>")
+        expect(cpp.doc.to_s).to eq("<p><img class=\"onebox\" src=\"#{upload.url}\" width=\"\" height=\"\"></p>")
 
-      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("<p><img class=\"onebox\" src=\"#{image_url}\" width=\"\" height=\"\"></p>")
+        expect(cpp.doc.to_s).to eq("<p><img class=\"onebox\" src=\"#{image_url}\" width=\"\" height=\"\"></p>")
+        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("<img class='onebox' src='#{image_url}' />")
+
+          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("<p><img class=\"onebox\" src=\"#{cooked_url}\" width=\"\" height=\"\"></p>")
+        end
+      end
     end
 
     it "replaces large image placeholder" do