diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1fd3d2fc9c2..f91baa9c220 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -11,12 +11,14 @@ require_dependency 'distributed_cache' require_dependency 'global_path' require_dependency 'secure_session' require_dependency 'topic_query' +require_dependency 'hijack' class ApplicationController < ActionController::Base include CurrentUser include CanonicalURL::ControllerExtensions include JsonError include GlobalPath + include Hijack attr_reader :theme_key diff --git a/app/controllers/onebox_controller.rb b/app/controllers/onebox_controller.rb index 31fc3b97227..15b99a76486 100644 --- a/app/controllers/onebox_controller.rb +++ b/app/controllers/onebox_controller.rb @@ -4,7 +4,6 @@ class OneboxController < ApplicationController before_action :ensure_logged_in def show - unless params[:refresh] == 'true' preview = Oneboxer.cached_preview(params[:url]) preview.strip! if preview.present? @@ -14,19 +13,23 @@ class OneboxController < ApplicationController # only 1 outgoing preview per user return render(body: nil, status: 429) if Oneboxer.is_previewing?(current_user.id) - Oneboxer.preview_onebox!(current_user.id) + user_id = current_user.id + invalidate = params[:refresh] == 'true' + url = params[:url] - preview = Oneboxer.preview(params[:url], invalidate_oneboxes: params[:refresh] == 'true') - preview.strip! if preview.present? + hijack do + Oneboxer.preview_onebox!(user_id) - Scheduler::Defer.later("Onebox previewed") { - Oneboxer.onebox_previewed!(current_user.id) - } + preview = Oneboxer.preview(url, invalidate_oneboxes: invalidate) + preview.strip! if preview.present? - if preview.blank? - render body: nil, status: 404 - else - render plain: preview + Oneboxer.onebox_previewed!(user_id) + + if preview.blank? + render body: nil, status: 404 + else + render plain: preview + end end end diff --git a/lib/hijack.rb b/lib/hijack.rb new file mode 100644 index 00000000000..1d057336534 --- /dev/null +++ b/lib/hijack.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +# This module allows us to hijack a request and send it to the client in the deferred job queue +# 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 Binder + attr_reader :content_type, :body, :status + + def initialize + @content_type = 'text/plain' + @status = 500 + @body = "" + end + + def render(opts) + if opts[:status] + @status = opts[:status].to_i + else + @status = 200 + end + + if opts.key?(:body) + @body = opts[:body].to_s + end + + if opts.key?(:plain) + @content_type = 'text/plain' + @body = opts[:plain].to_s + end + end + end + + def hijack(&blk) + if hijack = request.env['rack.hijack'] + io = hijack.call + + Scheduler::Defer.later("hijack work") do + + begin + # do this first to confirm we have a working connection + # before doing any work + io.write "HTTP/1.1 " + + binder = Binder.new + begin + binder.instance_eval(&blk) + rescue => e + Rails.logger.warn("Failed to process hijacked response correctly #{e}") + end + + 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" + io.write "\r\n" + io.write binder.body + io.close + rescue Errno::EPIPE, IOError + # happens if client terminated before we responded, ignore + end + end + # not leaked out, we use 418 ... I am a teapot to denote that we are hijacked + render plain: "", status: 418 + else + blk.call + end + end +end diff --git a/spec/components/hijack_spec.rb b/spec/components/hijack_spec.rb new file mode 100644 index 00000000000..0b01595ccfa --- /dev/null +++ b/spec/components/hijack_spec.rb @@ -0,0 +1,70 @@ +require 'rails_helper' + +describe Hijack do + class Hijack::Tester + attr_reader :io + + include Hijack + def initialize + @io = StringIO.new + end + + def hijack_test(&blk) + hijack do + self.instance_eval(&blk) + end + end + + def request + @req ||= ActionController::TestRequest.new( + { "rack.hijack" => lambda { @io } }, + nil, + nil + ) + end + + def render(*opts) + # don't care + end + end + + let :tester do + Hijack::Tester.new + end + + it "renders non 200 status if asked for" do + tester.hijack_test do + render body: "hello world", status: 402 + end + + expect(tester.io.string).to include("402") + expect(tester.io.string).to include("world") + end + + it "renders stuff correctly if it works" do + tester.hijack_test do + render plain: "hello world" + end + + result = "HTTP/1.1 200 OK\r\nContent-Length: 11\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\nhello world" + expect(tester.io.string).to eq(result) + end + + it "returns 500 by default" do + tester.hijack_test + + expected = "HTTP/1.1 500 OK\r\nContent-Length: 0\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\n" + expect(tester.io.string).to eq(expected) + end + + it "does not run the block if io is closed" do + tester.io.close + + ran = false + tester.hijack_test do + ran = true + end + + expect(ran).to eq(false) + end +end diff --git a/spec/controllers/onebox_controller_spec.rb b/spec/controllers/onebox_controller_spec.rb index 403a6279fe8..8995a78710f 100644 --- a/spec/controllers/onebox_controller_spec.rb +++ b/spec/controllers/onebox_controller_spec.rb @@ -21,17 +21,37 @@ describe OneboxController do describe "cached onebox" do - let(:body) { "This is a cached onebox body" } - - before do - Oneboxer.expects(:cached_preview).with(url).returns(body) - Oneboxer.expects(:preview).never - get :show, params: { url: url, user_id: @user.id }, format: :json - end - it "returns the cached onebox response in the body" do + onebox_html = <<~HTML + + + + + + +

body

+ + + HTML + + url = "http://noodle.com/" + + stub_request(:head, url). + to_return(status: 200, body: "", headers: {}).then.to_raise + + stub_request(:get, url) + .to_return(status: 200, headers: {}, body: onebox_html).then.to_raise + + get :show, params: { url: url, user_id: @user.id, refresh: "true" }, format: :json + expect(response).to be_success - expect(response.body).to eq(body) + expect(response.body).to include('Fred') + expect(response.body).to include('bodycontent') + + get :show, params: { url: url, user_id: @user.id }, format: :json + expect(response).to be_success + expect(response.body).to include('Fred') + expect(response.body).to include('bodycontent') end end