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