From 613e8fd0a1f1a99c13be8b41bce28945dd9b225a Mon Sep 17 00:00:00 2001 From: Nuno Silva Date: Fri, 28 Jul 2023 14:44:32 +0000 Subject: [PATCH] Add Rack::Lint to `ActionDispatch::RemoteIp` tests To ensure Rails is and remains compliant with [the Rack 3 spec](https://github.com/rack/rack/blob/6d16306192349e665e4ec820a9bfcc0967009b6a/UPGRADE-GUIDE.md) we can add `Rack::Lint` to the Rails middleware tests. This adds additional test coverage to `ActionDispatch::RemoteIp` to validate that its input and output follow the Rack SPEC. The only code testing this middleware are the ones for `ActionDispatch::Request`. Several changes were required to make the tests pass: - `CONTENT_LENGTH` must be a string - `SERVER_PORT` must be a string - `HTTP_HOST` must be a string - `rack.input` must be an IO object, with ASCII-8BIT encoding - By leveraging `Rack::MockRequest`, we can pass the symbol :input, and the string value, and it will be converted to an IO object with the correct encoding. - See [definition here](https://github.com/rack/rack/blob/444dc8a130663159ed490f25948b73ff9b5fbba0/lib/rack/mock_request.rb#L89-L97) - using `Rack::MockRequest` also means that any symbol keys being passed to setup the env, will be discarded. [Only string keys are copied.]https://github.com/rack/rack/blob/444dc8a130663159ed490f25948b73ff9b5fbba0/lib/rack/mock_request.rb#L156 --- actionpack/test/dispatch/request_test.rb | 63 +++++++++++++----------- 1 file changed, 34 insertions(+), 29 deletions(-) diff --git a/actionpack/test/dispatch/request_test.rb b/actionpack/test/dispatch/request_test.rb index b51288dd03786..a4426468a4ebc 100644 --- a/actionpack/test/dispatch/request_test.rb +++ b/actionpack/test/dispatch/request_test.rb @@ -4,10 +4,6 @@ class BaseRequestTest < ActiveSupport::TestCase def setup - @env = { - :ip_spoofing_check => true, - "rack.input" => "foo" - } @original_tld_length = ActionDispatch::Http::URL.tld_length end @@ -23,14 +19,23 @@ def url_for(options = {}) private def stub_request(env = {}) ip_spoofing_check = env.key?(:ip_spoofing_check) ? env.delete(:ip_spoofing_check) : true + ActionDispatch::Http::URL.tld_length = env.delete(:tld_length) if env.key?(:tld_length) + + uri = env["HTTPS"] == "on" ? "https://www.example.org" : "http://www.example.org" + + env = Rack::MockRequest.env_for(uri, env) @additional_trusted_proxy ||= nil trusted_proxies = ActionDispatch::RemoteIp::TRUSTED_PROXIES + [@additional_trusted_proxy] - ip_app = ActionDispatch::RemoteIp.new(Proc.new { }, ip_spoofing_check, trusted_proxies) - ActionDispatch::Http::URL.tld_length = env.delete(:tld_length) if env.key?(:tld_length) + ip_app = Rack::Lint.new( + ActionDispatch::RemoteIp.new( + Rack::Lint.new(Proc.new { [200, {}, []] }), + ip_spoofing_check, + trusted_proxies, + ) + ) ip_app.call(env) - env = @env.merge(env) ActionDispatch::Request.new(env) end end @@ -306,7 +311,7 @@ class RequestDomain < BaseRequestTest assert_equal %w( 192 168 1 ), request.subdomains assert_equal "192.168.1", request.subdomain - request = stub_request "HTTP_HOST" => nil + request = stub_request "HTTP_HOST" => "" assert_equal [], request.subdomains assert_equal "", request.subdomain @@ -378,7 +383,7 @@ class RequestPort < BaseRequestTest request = stub_request "SERVER_PORT" => "80" assert_equal 80, request.server_port - request = stub_request "SERVER_PORT" => "" + request = stub_request "SERVER_PORT" => "0" assert_equal 0, request.server_port end end @@ -597,9 +602,9 @@ class RequestParamsParsing < BaseRequestTest test "doesn't break when content type has charset" do request = stub_request( "REQUEST_METHOD" => "POST", - "CONTENT_LENGTH" => "flamenco=love".length, + "CONTENT_LENGTH" => "flamenco=love".length.to_s, "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8", - "rack.input" => StringIO.new("flamenco=love") + :input => "flamenco=love" ) assert_equal({ "flamenco" => "love" }, request.request_parameters) @@ -616,7 +621,7 @@ class RequestParamsParsing < BaseRequestTest "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x", "CONTENT_LENGTH" => "9", # lower than data length "REQUEST_METHOD" => "POST", - "rack.input" => StringIO.new("0123456789") + :input => "0123456789" ) err = assert_raises(ActionController::BadRequest) do @@ -632,7 +637,7 @@ class RequestParamsParsing < BaseRequestTest "CONTENT_TYPE" => "multipart/form-data; boundary=AaB03x", "CONTENT_LENGTH" => "11", # higher than data length "REQUEST_METHOD" => "POST", - "rack.input" => StringIO.new("0123456789") + :input => "0123456789" ) err = assert_raises(ActionController::BadRequest) do @@ -648,8 +653,8 @@ class RequestRewind < BaseRequestTest test "body should be rewound" do data = "rewind" env = { - "rack.input" => StringIO.new(data), - "CONTENT_LENGTH" => data.length, + :input => data, + "CONTENT_LENGTH" => data.length.to_s, "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8" } @@ -658,13 +663,13 @@ class RequestRewind < BaseRequestTest request.request_parameters # Should have rewound the body. - assert_equal 0, request.body.pos + assert_equal "rewind", request.body.read end test "raw_post rewinds rack.input if RAW_POST_DATA is nil" do request = stub_request( - "rack.input" => StringIO.new("raw"), - "CONTENT_LENGTH" => 3 + :input => "raw", + "CONTENT_LENGTH" => "3" ) assert_equal "raw", request.raw_post assert_equal "raw", request.env["rack.input"].read @@ -919,11 +924,12 @@ class RequestFormat < BaseRequestTest test "format does not throw exceptions when invalid POST parameters" do body = "{record:{content:127.0.0.1}}" + request = stub_request( "REQUEST_METHOD" => "POST", - "CONTENT_LENGTH" => body.length, + "CONTENT_LENGTH" => body.length.to_s, "CONTENT_TYPE" => "application/json", - "rack.input" => StringIO.new(body), + :input => body, "action_dispatch.logger" => Logger.new(output = StringIO.new) ) assert request.formats @@ -1069,7 +1075,7 @@ class RequestMimeType < BaseRequestTest class RequestParameters < BaseRequestTest test "parameters" do request = stub_request "CONTENT_TYPE" => "application/json", - "CONTENT_LENGTH" => 9, + "CONTENT_LENGTH" => "9", "RAW_POST_DATA" => '{"foo":1}', "QUERY_STRING" => "bar=2" @@ -1131,9 +1137,9 @@ class RequestParameters < BaseRequestTest data = "foo=%81E" request = stub_request( "REQUEST_METHOD" => "POST", - "CONTENT_LENGTH" => data.length, + "CONTENT_LENGTH" => data.length.to_s, "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8", - "rack.input" => StringIO.new(data) + :input => data ) err = assert_raises(ActionController::BadRequest) { request.parameters } @@ -1154,9 +1160,9 @@ class RequestParameters < BaseRequestTest data = "foo=%81E" request = stub_request( "REQUEST_METHOD" => "POST", - "CONTENT_LENGTH" => data.length, + "CONTENT_LENGTH" => data.length.to_s, "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8", - "rack.input" => StringIO.new(data) + :input => data ) ActionDispatch::Request::Utils.stub(:set_binary_encoding, { "foo" => "\x81E".b }) do @@ -1167,9 +1173,9 @@ class RequestParameters < BaseRequestTest test "parameters not accessible after rack parse error 1" do request = stub_request( "REQUEST_METHOD" => "POST", - "CONTENT_LENGTH" => "a%=".length, + "CONTENT_LENGTH" => "a%=".length.to_s, "CONTENT_TYPE" => "application/x-www-form-urlencoded; charset=utf-8", - "rack.input" => StringIO.new("a%=") + :input => "a%=" ) assert_raises(ActionController::BadRequest) do @@ -1398,8 +1404,7 @@ class RequestFormData < BaseRequestTest class EarlyHintsRequestTest < BaseRequestTest def setup super - @env["rack.early_hints"] = lambda { |links| links } - @request = stub_request + @request = stub_request({ "rack.early_hints" => lambda { |links| links } }) end test "when early hints is set in the env link headers are sent" do