favicon proxy now uses hijack

app/controllers/static_controller.rb

@@ -100,34 +100,36 @@ class StaticController < ApplicationController
   # a huge expiry, we also cache these assets in nginx so it bypassed if needed
   def favicon
 
-    data = DistributedMemoizer.memoize('favicon' + SiteSetting.favicon_url, 60 * 30) do
-      begin
-        file = FileHelper.download(
-          SiteSetting.favicon_url,
-          max_file_size: 50.kilobytes,
-          tmp_file_name: "favicon.png",
-          follow_redirect: true
-        )
-        data = file.read
-        file.unlink
-        data
-      rescue => e
-        AdminDashboardData.add_problem_message('dashboard.bad_favicon_url', 1800)
-        Rails.logger.debug("Invalid favicon_url #{SiteSetting.favicon_url}: #{e}\n#{e.backtrace}")
-        ""
+    hijack do
+      data = DistributedMemoizer.memoize('favicon' + SiteSetting.favicon_url, 60 * 30) do
+        begin
+          file = FileHelper.download(
+            SiteSetting.favicon_url,
+            max_file_size: 50.kilobytes,
+            tmp_file_name: "favicon.png",
+            follow_redirect: true
+          )
+          data = file.read
+          file.unlink
+          data
+        rescue => e
+          AdminDashboardData.add_problem_message('dashboard.bad_favicon_url', 1800)
+          Rails.logger.debug("Invalid favicon_url #{SiteSetting.favicon_url}: #{e}\n#{e.backtrace}")
+          ""
+        end
       end
-    end
 
-    if data.bytesize == 0
-      @@default_favicon ||= File.read(Rails.root + "public/images/default-favicon.png")
-      response.headers["Content-Length"] = @@default_favicon.bytesize.to_s
-      render plain: @@default_favicon, content_type: "image/png"
-    else
-      immutable_for 1.year
-      response.headers["Expires"] = 1.year.from_now.httpdate
-      response.headers["Content-Length"] = data.bytesize.to_s
-      response.headers["Last-Modified"] = Time.new('2000-01-01').httpdate
-      render plain: data, content_type: "image/png"
+      if data.bytesize == 0
+        @@default_favicon ||= File.read(Rails.root + "public/images/default-favicon.png")
+        response.headers["Content-Length"] = @@default_favicon.bytesize.to_s
+        render body: @@default_favicon, content_type: "image/png"
+      else
+        immutable_for 1.year
+        response.headers["Expires"] = 1.year.from_now.httpdate
+        response.headers["Content-Length"] = data.bytesize.to_s
+        response.headers["Last-Modified"] = Time.new('2000-01-01').httpdate
+        render body: data, content_type: "image/png"
+      end
     end
   end
 

lib/hijack.rb

@@ -4,13 +4,26 @@
 # For cases where we are making remote calls like onebox or proxying files and so on this helps
 # free up a unicorn worker while the remote IO is happening
 module Hijack
+
+  class FakeResponse
+    attr_reader :headers
+    def initialize
+      @headers = {}
+    end
+  end
+
   class Binder
-    attr_reader :content_type, :body, :status
+    attr_reader :content_type, :body, :status, :response
 
     def initialize
       @content_type = 'text/plain'
       @status = 500
       @body = ""
+      @response = FakeResponse.new
+    end
+
+    def immutable_for(duration)
+      response.headers['Cache-Control'] = "max-age=#{duration}, public, immutable"
     end
 
     def render(opts)
@@ -24,6 +37,10 @@ module Hijack
         @body = opts[:body].to_s
       end
 
+      if opts.key?(:content_type)
+        @content_type = opts[:content_type]
+      end
+
       if opts.key?(:plain)
         @content_type = 'text/plain; charset=utf-8'
         @body = opts[:plain].to_s
@@ -57,10 +74,17 @@ module Hijack
             Rails.logger.warn("Failed to process hijacked response correctly #{e}")
           end
 
+          headers = binder.response.headers
+          headers['Content-Length'] = binder.body.bytesize
+          headers['Content-Type'] = binder.content_type
+          headers['Connection'] = "close"
+
           io.write "#{binder.status} OK\r\n"
-          io.write "Content-Length: #{binder.body.bytesize}\r\n"
-          io.write "Content-Type: #{binder.content_type}\r\n"
-          io.write "Connection: close\r\n"
+
+          headers.each do |name, val|
+            io.write "#{name}: #{val}\r\n"
+          end
+
           io.write "\r\n"
           io.write binder.body
           io.close
@@ -71,7 +95,18 @@ module Hijack
       # not leaked out, we use 418 ... I am a teapot to denote that we are hijacked
       render plain: "", status: 418
     else
-      blk.call
+      binder = Binder.new
+      binder.instance_eval(&blk)
+
+      binder.response.headers.each do |name, val|
+        response.headers[name] = val
+      end
+
+      render(
+        body: binder.body,
+        content_type: binder.content_type,
+        status: status
+      )
     end
   end
 end

spec/requests/static_controller_spec.rb

@@ -2,6 +2,53 @@ require 'rails_helper'
 
 describe StaticController do
 
+  context '#favicon' do
+    before do
+      # this is a mess in test, will fix in a future commit
+      FinalDestination.stubs(:lookup_ip).returns('1.2.3.4')
+    end
+
+    let(:png) { Base64.decode64("R0lGODlhAQABALMAAAAAAIAAAACAAICAAAAAgIAAgACAgMDAwICAgP8AAAD/AP//AAAA//8A/wD//wBiZCH5BAEAAA8ALAAAAAABAAEAAAQC8EUAOw==") }
+
+    it 'returns the default favicon for a missing download' do
+
+      url = "https://somewhere1.over.rainbow/#{SecureRandom.hex}.png"
+
+      stub_request(:head, url).
+        with(headers: { 'Host' => 'somewhere1.over.rainbow' }).
+        to_return(status: 404, body: "", headers: {})
+
+      SiteSetting.favicon_url = url
+
+      get '/favicon/proxied'
+
+      favicon = File.read(Rails.root + "public/images/default-favicon.png")
+
+      expect(response.status).to eq(200)
+      expect(response.content_type).to eq('image/png')
+      expect(response.body.bytesize).to eq(favicon.bytesize)
+    end
+
+    it 'can proxy a favicon correctly' do
+      url = "https://somewhere.over.rainbow/#{SecureRandom.hex}.png"
+
+      stub_request(:head, url).
+        with(headers: { 'Host' => 'somewhere.over.rainbow' }).
+        to_return(status: 200, body: "", headers: {})
+
+      stub_request(:get, url).
+        to_return(status: 200, body: png, headers: {})
+
+      SiteSetting.favicon_url = url
+
+      get '/favicon/proxied'
+
+      expect(response.status).to eq(200)
+      expect(response.content_type).to eq('image/png')
+      expect(response.body.bytesize).to eq(png.bytesize)
+    end
+  end
+
   context '#brotli_asset' do
     it 'returns a non brotli encoded 404 if asset is missing' do
 
@@ -9,7 +56,7 @@ describe StaticController do
 
       expect(response.status).to eq(404)
       expect(response.headers['Content-Encoding']).not_to eq('br')
-      expect(response.headers["Cache-Control"]).to match(/max-age=1/)
+      expect(response.headers['Cache-Control']).to match(/max-age=1/)
     end
 
     it 'can handle fallback brotli assets' do