From 64ce12a7587600f0e7de1c8f7b624e527ded682d Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Mon, 6 Jul 2020 17:01:29 +0200 Subject: [PATCH] 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 --- app/models/optimized_image.rb | 4 +- lib/file_store/base_store.rb | 15 +++-- spec/models/optimized_image_spec.rb | 90 +++++++++++++++++------------ 3 files changed, 62 insertions(+), 47 deletions(-) diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 654d60a35cc..98402079cc8 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 504267ebf4d..44749f586ab 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 e90029bec3c..27702902b33 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) + 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 + oi = OptimizedImage.create_for(s3_upload, 100, 200) + + 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 + + it "allows to recalculate the filesize" do + oi = OptimizedImage.create_for(s3_upload, 100, 200) + oi.filesize = nil + + 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.filesize).to be > 0 + end end - it "downloads a copy of the original image" do - optimized_path = "/optimized/1X/#{s3_upload.sha1}_2_100x200.png" + context "secure uploads enabled" do + it "allows to recalculate the filesize" do + SiteSetting.secure_media = true + s3_upload = Fabricate(:secure_upload_s3) - stub_request( - :head, - "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com/" - ) + 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" }) - stub_request( - :put, - "https://#{SiteSetting.s3_upload_bucket}.s3.amazonaws.com#{optimized_path}" - ).to_return( - status: 200, - headers: { "ETag" => "someetag" } - ) + oi = OptimizedImage.create_for(s3_upload, 100, 200) + oi.filesize = nil - oi = OptimizedImage.create_for(s3_upload, 100, 200) + stub_request(:get, Discourse.store.signed_url_for_path(oi.url)) + .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 - end - end describe '#destroy' do