FIX: `StaticController#favicon` reads from disk when using local store. (#7160)

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.

diff --git a/app/assets/javascripts/discourse.js.es6 b/app/assets/javascripts/discourse.js.es6
index e84865f..a393adb 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 e743676..3ba4864 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 f6ec174..288ee57 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
+
+      before do
+        SiteSetting.enable_s3_uploads = true
+        SiteSetting.s3_access_key_id = 'X'
+        SiteSetting.s3_secret_access_key = 'X'
+      end
 
-      get '/favicon/proxied'
+      it 'can proxy a favicon correctly' do
+        SiteSetting.favicon = upload
 
-      expect(response.status).to eq(200)
-      expect(response.content_type).to eq('image/png')
-      expect(response.body.bytesize).to eq(png.bytesize)
+        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

GitHub sha: 1c6a2262

it seems this change broke the favicon for us:

It falls back through to the default again.

Can you provide me with the following output?

SiteSetting.favicon
SiteSetting.favicon_url(warn: false)

On 2019-03-14 06:27, Guo Xiang Tan wrote:

Do you have a favicon configured?

We had yes

it was working until I updated our instance to discourse-2.3.0.beta5~git3.1bc96177dd

Hi can you pm me on meta to help you debug this? We’re not seeing the problem on our sites.

hmm there is a problem here @tgxworld … what happens when the favicon changes, there is no way of invalidating it with this scheme. Ideally the client should have the upload id and use it as a cache breaker.

/favicon/proxied?upload_id=11993

With the new upload site settings, the id will change if the favicon changes.

I know that… but the client will have this thing cached for a long time…

Admins will get super confused when they upload a new favicon and it does not change, client needs to know the upload id.

1 Like

Ah ok I get it now. I was wondering why we included the url in the params even though we don’t use it on the server side.

2 Likes

Fix favicon not updating on the client side when changed.