From d5e7691ae9327685e8f4e0f76b017d6c394d8897 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 27 Nov 2017 14:50:57 +1100 Subject: [PATCH] favicon proxy now uses hijack --- app/controllers/static_controller.rb | 54 +++++++++++++------------ lib/hijack.rb | 45 ++++++++++++++++++--- spec/requests/static_controller_spec.rb | 49 +++++++++++++++++++++- 3 files changed, 116 insertions(+), 32 deletions(-) diff --git a/app/controllers/static_controller.rb b/app/controllers/static_controller.rb index cc04e232778..2b32131c9d9 100644 --- a/app/controllers/static_controller.rb +++ b/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 diff --git a/lib/hijack.rb b/lib/hijack.rb index 1eb27a0cf37..486609d3754 100644 --- a/lib/hijack.rb +++ b/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 diff --git a/spec/requests/static_controller_spec.rb b/spec/requests/static_controller_spec.rb index f3343505d65..f4376253ace 100644 --- a/spec/requests/static_controller_spec.rb +++ b/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