From 1c6a2262b3b57d344a60402eee6b5cbcfab7cf6d Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 14 Mar 2019 04:17:36 +0800 Subject: [PATCH] FIX: `StaticController#favicon` reads from disk when using local store. (#7160) Since uploads site settings are now backed by an actual upload, we don't have to reach over the network just to fetch the favicon. Instead, we can just read the upload directly from disk. --- app/assets/javascripts/discourse.js.es6 | 5 +- app/controllers/static_controller.rb | 38 +++++++++------ spec/requests/static_controller_spec.rb | 65 +++++++++++++++++-------- 3 files changed, 69 insertions(+), 39 deletions(-) diff --git a/app/assets/javascripts/discourse.js.es6 b/app/assets/javascripts/discourse.js.es6 index e84865fb6bb..a393adbfc46 100644 --- a/app/assets/javascripts/discourse.js.es6 +++ b/app/assets/javascripts/discourse.js.es6 @@ -62,10 +62,7 @@ const Discourse = Ember.Application.extend({ @observes("notifyCount") faviconChanged() { if (Discourse.User.currentProp("dynamic_favicon")) { - let url = Discourse.SiteSettings.site_favicon_url; - if (/^http/.test(url)) { - url = Discourse.getURL("/favicon/proxied?" + encodeURIComponent(url)); - } + const url = Discourse.getURL("/favicon/proxied"); new window.Favcount(url).set(this.get("notifyCount")); } }, diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index e74367647bd..3ba48643559 100644 --- a/app/controllers/static_controller.rb +++ b/app/controllers/static_controller.rb @@ -124,22 +124,28 @@ class StaticController < ApplicationController is_asset_path hijack do - data = DistributedMemoizer.memoize(FAVICON + SiteSetting.site_favicon_url, 60 * 30) do - begin - file = FileHelper.download( - UrlHelper.absolute(SiteSetting.site_favicon_url), - max_file_size: 50.kilobytes, - tmp_file_name: FAVICON, - follow_redirect: true - ) - file ||= Tempfile.new([FAVICON, ".png"]) - data = file.read - file.unlink - data - rescue => e - AdminDashboardData.add_problem_message('dashboard.bad_favicon_url', 1800) - Rails.logger.debug("Invalid favicon_url #{SiteSetting.site_favicon_url}: #{e}\n#{e.backtrace}") - "" + data = DistributedMemoizer.memoize("FAVICON#{SiteSetting.favicon&.id}", 60 * 30) do + favicon = SiteSetting.favicon + next "" unless favicon + + if Discourse.store.external? + begin + file = FileHelper.download( + UrlHelper.absolute(favicon.url), + max_file_size: favicon.filesize, + tmp_file_name: FAVICON, + follow_redirect: true + ) + + file&.read || "" + rescue => e + AdminDashboardData.add_problem_message('dashboard.bad_favicon_url', 1800) + Rails.logger.warn("Failed to fetch faivcon #{favicon.url}: #{e}\n#{e.backtrace}") + ensure + file&.unlink + end + else + File.read(Rails.root.join("public", favicon.url[1..-1])) end end diff --git a/spec/requests/static_controller_spec.rb b/spec/requests/static_controller_spec.rb index f6ec174a24d..288ee57095e 100644 --- a/spec/requests/static_controller_spec.rb +++ b/spec/requests/static_controller_spec.rb @@ -4,38 +4,65 @@ describe StaticController do let(:upload) { Fabricate(:upload) } context '#favicon' do - let(:png) { Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==") } + let(:filename) { 'smallest.png' } + let(:file) { file_from_fixtures(filename) } - before { FinalDestination.stubs(:lookup_ip).returns("1.2.3.4") } + let(:upload) do + UploadCreator.new(file, filename).create_for(Discourse.system_user.id) + end after do DistributedMemoizer.flush! end - it 'returns the default favicon for a missing download' do - url = UrlHelper.absolute(upload.url) - SiteSetting.favicon = upload - stub_request(:get, url).to_return(status: 404) + describe 'local store' do + it 'returns the default favicon if favicon has not been configured' do + get '/favicon/proxied' - get '/favicon/proxied' + expect(response.status).to eq(200) + expect(response.content_type).to eq('image/png') + expect(response.body.bytesize).to eq(SiteSetting.favicon.filesize) + end - favicon = File.read(Rails.root + "public/images/default-favicon.png") + it 'returns the configured favicon' do + SiteSetting.favicon = upload - expect(response.status).to eq(200) - expect(response.content_type).to eq('image/png') - expect(response.body.bytesize).to eq(favicon.bytesize) + get '/favicon/proxied' + + expect(response.status).to eq(200) + expect(response.content_type).to eq('image/png') + expect(response.body.bytesize).to eq(upload.filesize) + end end - it 'can proxy a favicon correctly' do - url = UrlHelper.absolute(upload.url) - SiteSetting.favicon = upload - stub_request(:get, url).to_return(status: 200, body: png) + describe 'external store' do + let(:upload) do + Upload.create!( + url: '//s3-upload-bucket.s3-us-east-1.amazonaws.com/somewhere/a.png', + original_filename: filename, + filesize: file.size, + user_id: Discourse.system_user.id + ) + end - get '/favicon/proxied' + before do + SiteSetting.enable_s3_uploads = true + SiteSetting.s3_access_key_id = 'X' + SiteSetting.s3_secret_access_key = 'X' + end - expect(response.status).to eq(200) - expect(response.content_type).to eq('image/png') - expect(response.body.bytesize).to eq(png.bytesize) + it 'can proxy a favicon correctly' do + SiteSetting.favicon = upload + + stub_request(:get, "https:/#{upload.url}") + .to_return(status: 200, body: file) + + get '/favicon/proxied' + + expect(response.status).to eq(200) + expect(response.content_type).to eq('image/png') + expect(response.body.bytesize).to eq(upload.filesize) + end end end