FIX: `OptimizedImage#filesize` (#10095)

FIX: OptimizedImage#filesize (#10095)

OptimizedImage#filesize calls Discourse.store.download with an OptimizedImage as an argument. It would in turn attempt to call #original_filename and #secure? on that object. Both would fail as these methods do not exist on OptimizedImage, only on Upload. We didn’t know about these issues because:

  1. #calculate_filesize is not called often, because the filesize is saved on OptimizedImage creation, so it’s used mostly for manual filesize recalculation
  2. we were using rescue nil which swallows all errors
diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb
index 654d60a..9840207 100644
--- a/app/models/optimized_image.rb
+++ b/app/models/optimized_image.rb
@@ -156,9 +156,7 @@ class OptimizedImage < ActiveRecord::Base
     if size = read_attribute(:filesize)
       size
     else
-      # we may have a bad optimized image so just skip for now
-      # and do not break here
-      size = calculate_filesize rescue nil
+      size = calculate_filesize
 
       write_attribute(:filesize, size)
       if !new_record?
diff --git a/lib/file_store/base_store.rb b/lib/file_store/base_store.rb
index 504267e..44749f5 100644
--- a/lib/file_store/base_store.rb
+++ b/lib/file_store/base_store.rb
@@ -76,17 +76,19 @@ module FileStore
       not_implemented
     end
 
-    def download(upload, max_file_size_kb: nil)
-      DistributedMutex.synchronize("download_#{upload.sha1}", validity: 3.minutes) do
-        filename = "#{upload.sha1}#{File.extname(upload.original_filename)}"
+    def download(object, max_file_size_kb: nil)
+      DistributedMutex.synchronize("download_#{object.sha1}", validity: 3.minutes) do
+        extension = File.extname(object.respond_to?(:original_filename) ? object.original_filename : object.url)
+        filename = "#{object.sha1}#{extension}"
         file = get_from_cache(filename)
 
         if !file
           max_file_size_kb ||= [SiteSetting.max_image_size_kb, SiteSetting.max_attachment_size_kb].max.kilobytes
 
-          url = upload.secure? ?
-            Discourse.store.signed_url_for_path(upload.url) :
-            Discourse.store.cdn_url(upload.url)
+          secure = object.respond_to?(:secure) ? object.secure? : object.upload.secure?
+          url = secure ?
+            Discourse.store.signed_url_for_path(object.url) :
+            Discourse.store.cdn_url(object.url)
 
           url = SiteSetting.scheme + ":" + url if url =~ /^\/\//
           file = FileHelper.download(
@@ -95,6 +97,7 @@ module FileStore
             tmp_file_name: "discourse-download",
             follow_redirect: true
           )
+
           cache_file(file, filename)
           file = get_from_cache(filename)
         end
diff --git a/spec/models/optimized_image_spec.rb b/spec/models/optimized_image_spec.rb
index e90029b..2770290 100644
--- a/spec/models/optimized_image_spec.rb
+++ b/spec/models/optimized_image_spec.rb
@@ -292,68 +292,82 @@ describe OptimizedImage do
     end
 
     describe "external store" do
-      let(:s3_upload) { Fabricate(:upload_s3) }
-
       before do
         SiteSetting.enable_s3_uploads = true
         SiteSetting.s3_upload_bucket = "s3-upload-bucket"
         SiteSetting.s3_access_key_id = "some key"
         SiteSetting.s3_secret_access_key = "some secret key"
         SiteSetting.s3_region = "us-east-1"
-
-        tempfile = Tempfile.new(["discourse-external", ".png"])
-
-        %i{head get}.each do |method|
-          stub_request(method, "http://#{s3_upload.url}")
-            .to_return(
-              status: 200,
-              body: tempfile.read
-            )
-        end
       end
 
       context "when we have a bad file returned" do
         it "returns nil" do
-          # tempfile is empty
-          # this can not be resized
+          s3_upload = Fabricate(:upload_s3)
+          stub_request(:head, "http://#{s3_upload.url}").to_return(status: 200)
+          stub_request(:get, "http://#{s3_upload.url}").to_return(status: 200)
+
           expect(OptimizedImage.create_for(s3_upload, 100, 200)).to eq(nil)
         end
       end
 
       context "when the thumbnail is properly generated" do
-        before do
-          OptimizedImage.expects(:resize).returns(true)
-        end
+        context "secure media disabled" do
+          let(:s3_upload) { Fabricate(:upload_s3) }
+          let(:optimized_path) { "/optimized/1X/#{s3_upload.sha1}_2_100x200.png" }
+
+          before do
+            stub_request(:head, "http://#{s3_upload.url}").to_return(status: 200)
+            stub_request(:get, "http://#{s3_upload.url}").to_return(status: 200, body: file_from_fixtures("logo.png"))
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+            stub_request(:put, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com#{optimized_path}")
+              .to_return(status: 200, headers: { "ETag" => "someetag" })
+          end
 
-        it "downloads a copy of the original image" do
-          optimized_path = "/optimized/1X/#{s3_upload.sha1}_2_100x200.png"
+          it "downloads a copy of the original image" do
+            oi = OptimizedImage.create_for(s3_upload, 100, 200)
 
-          stub_request(
-            :head,
-            "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/"
-          )
+            expect(oi.sha1).to_not be_nil
+            expect(oi.extension).to eq(".png")
+            expect(oi.width).to eq(100)
+            expect(oi.height).to eq(200)
+            expect(oi.url).to eq("//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com#{optimized_path}")
+            expect(oi.filesize).to be > 0
+          end
 
-          stub_request(
-            :put,
-            "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com#{optimized_path}"
-          ).to_return(
-            status: 200,
-            headers: { "ETag" => "someetag" }
-          )
+          it "allows to recalculate the filesize" do
+            oi = OptimizedImage.create_for(s3_upload, 100, 200)
+            oi.filesize = nil
 
-          oi = OptimizedImage.create_for(s3_upload, 100, 200)
+            stub_request(
+              :get,
+              "http://#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com#{optimized_path}"
+            ).to_return(status: 200, body: file_from_fixtures("resized.png"))
 
-          expect(oi.sha1).to eq("da39a3ee5e6b4b0d3255bfef95601890afd80709")
-          expect(oi.extension).to eq(".png")
-          expect(oi.width).to eq(100)
-          expect(oi.height).to eq(200)
-          expect(oi.url).to eq("//#{SiteSetting.s3_upload_bucket}.s3.dualstack.us-east-1.amazonaws.com#{optimized_path}")
+            expect(oi.filesize).to be > 0
+          end
         end
 
-      end
+        context "secure uploads enabled" do
+          it "allows to recalculate the filesize" do
+            SiteSetting.secure_media = true
+            s3_upload = Fabricate(:secure_upload_s3)
 
-    end
+            stub_request(:head, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/")
+            stub_request(:get, Discourse.store.signed_url_for_path(s3_upload.url)).to_return(status: 200, body: file_from_fixtures("logo.png"))
+            stub_request(:put, "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/optimized/1X/#{s3_upload.sha1}_2_100x200.png")
+              .to_return(status: 200, headers: { "ETag" => "someetag" })
 
+            oi = OptimizedImage.create_for(s3_upload, 100, 200)
+            oi.filesize = nil
+
+            stub_request(:get, Discourse.store.signed_url_for_path(oi.url))
+              .to_return(status: 200, body: file_from_fixtures("resized.png"))
+
+            expect(oi.filesize).to be > 0
+          end
+        end
+      end
+    end
   end
 
   describe '#destroy' do

GitHub sha: 64ce12a7

This commit appears in #10095 which was approved by ZogStriP. It was merged by CvX.