From 2492fe7715c3c76803c14a1fc0553e7d30b6d7ed Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Tue, 6 Aug 2024 07:12:42 +0800 Subject: [PATCH] FIX: Set sane default for `Net::HTTP` when processing a request (#28141) This commit patches `Net::HTTP` to reduce the default timeouts of 60 seconds when we are processing a request. There are certain routes in Discourse which makes external requests and if the proper timeouts are not set, we risk having the Unicorn master process force restarting the Unicorn workers once the `30` seconds timeout is reached. This can potentially become a vector for DoS attacks and this commit is aimed at reducing the risk here. --- app/controllers/test_requests_controller.rb | 17 +++++++++++++ config/initializers/200-first_middlewares.rb | 2 ++ config/routes.rb | 5 ++++ lib/freedom_patches/net_http.rb | 26 ++++++++++++++++++++ lib/middleware/processing_request.rb | 16 ++++++++++++ spec/integrity/middleware_order_spec.rb | 1 + spec/requests/net_http_timeout_spec.rb | 20 +++++++++++++++ 7 files changed, 87 insertions(+) create mode 100644 app/controllers/test_requests_controller.rb create mode 100644 lib/freedom_patches/net_http.rb create mode 100644 lib/middleware/processing_request.rb create mode 100644 spec/requests/net_http_timeout_spec.rb diff --git a/app/controllers/test_requests_controller.rb b/app/controllers/test_requests_controller.rb new file mode 100644 index 00000000000..b8d8d755c90 --- /dev/null +++ b/app/controllers/test_requests_controller.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +# This controller's actions are only available in the test environment to help with more complex testing requirements +class TestRequestsController < ApplicationController + if Rails.env.test? + def test_net_http_timeouts + net_http = Net::HTTP.new("example.com") + + render json: { + open_timeout: net_http.open_timeout, + read_timeout: net_http.read_timeout, + write_timeout: net_http.write_timeout, + max_retries: net_http.max_retries, + } + end + end +end diff --git a/config/initializers/200-first_middlewares.rb b/config/initializers/200-first_middlewares.rb index 60073484b7e..329704e69c0 100644 --- a/config/initializers/200-first_middlewares.rb +++ b/config/initializers/200-first_middlewares.rb @@ -7,6 +7,8 @@ # We aren't manipulating the middleware stack directly because of # https://github.com/rails/rails/pull/27936 +require "middleware/processing_request" +Rails.configuration.middleware.unshift(Middleware::ProcessingRequest) Rails.configuration.middleware.unshift(MessageBus::Rack::Middleware) # no reason to track this in development, that is 300+ redis calls saved per diff --git a/config/routes.rb b/config/routes.rb index c2e2c47365e..f20a2394957 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1649,5 +1649,10 @@ Discourse::Application.routes.draw do get "/form-templates/:id" => "form_templates#show" get "/form-templates" => "form_templates#index" + + if Rails.env.test? + # Routes that are only used for testing + get "/test_net_http_timeouts" => "test_requests#test_net_http_timeouts" + end end end diff --git a/lib/freedom_patches/net_http.rb b/lib/freedom_patches/net_http.rb new file mode 100644 index 00000000000..9e1df1aa8c2 --- /dev/null +++ b/lib/freedom_patches/net_http.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module NetHTTPPatch + OPEN_TIMEOUT = 5 + READ_TIMEOUT = 10 + WRITE_TIMEOUT = 5 + + # By default Net::HTTP will retry 1 time on idempotent requests but we can't afford that while processing a request + # so setting it to 0 + MAX_RETIRES = 0 + + def initialize(*args, &block) + super(*args, &block) + + ## START PATCH + if Thread.current[Middleware::ProcessingRequest::PROCESSING_REQUEST_THREAD_KEY] + self.open_timeout = OPEN_TIMEOUT + self.read_timeout = READ_TIMEOUT + self.write_timeout = WRITE_TIMEOUT + self.max_retries = 0 + end + ## END PATCH + end +end + +Net::HTTP.prepend(NetHTTPPatch) diff --git a/lib/middleware/processing_request.rb b/lib/middleware/processing_request.rb new file mode 100644 index 00000000000..fda6195bab1 --- /dev/null +++ b/lib/middleware/processing_request.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class Middleware::ProcessingRequest + PROCESSING_REQUEST_THREAD_KEY = "discourse.processing_request" + + def initialize(app) + @app = app + end + + def call(env) + Thread.current[PROCESSING_REQUEST_THREAD_KEY] = true + @app.call(env) + ensure + Thread.current[PROCESSING_REQUEST_THREAD_KEY] = nil + end +end diff --git a/spec/integrity/middleware_order_spec.rb b/spec/integrity/middleware_order_spec.rb index 9d7565e0303..1ae0e167f9b 100644 --- a/spec/integrity/middleware_order_spec.rb +++ b/spec/integrity/middleware_order_spec.rb @@ -8,6 +8,7 @@ RSpec.describe "Middleware order" do ActionDispatch::RemoteIp, Middleware::RequestTracker, MessageBus::Rack::Middleware, + Middleware::ProcessingRequest, Rack::Sendfile, ActionDispatch::Static, ActionDispatch::Executor, diff --git a/spec/requests/net_http_timeout_spec.rb b/spec/requests/net_http_timeout_spec.rb new file mode 100644 index 00000000000..aeace6e501d --- /dev/null +++ b/spec/requests/net_http_timeout_spec.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +RSpec.describe "Net::HTTP timeouts when processing a request" do + it "should set the right timeouts for any `Net::HTTP` instances intialized while processing a request" do + stub_const(NetHTTPPatch, :OPEN_TIMEOUT, 0.001) do + stub_const(NetHTTPPatch, :READ_TIMEOUT, 0.002) do + stub_const(NetHTTPPatch, :WRITE_TIMEOUT, 0.003) do + get "/test_net_http_timeouts.json" + + parsed = response.parsed_body + + expect(parsed["open_timeout"]).to eq(0.001) + expect(parsed["read_timeout"]).to eq(0.002) + expect(parsed["write_timeout"]).to eq(0.003) + expect(parsed["max_retries"]).to eq(0) + end + end + end + end +end