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

@@ -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"));
     }
   },

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
 

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