Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Rack::Lint to ActionDispatch::RemoteIp tests #48835

Merged
merged 1 commit into from Jul 31, 2023
Merged

Add Rack::Lint to ActionDispatch::RemoteIp tests #48835

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 34 additions & 29 deletions actionpack/test/dispatch/request_test.rb
View file
Open in desktop
Expand Up @@ -4,10 +4,6 @@

class BaseRequestTest < ActiveSupport::TestCase
def setup
@env = {
:ip_spoofing_check => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was a behavior change at first but I see why its not: ip_spoofing_check defaults to true in stub_request if the key isn't present. So this makes sense to me 👍

"rack.input" => "foo"
}
@original_tld_length = ActionDispatch::Http::URL.tld_length
end

Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this line earlier since using Rack::MockRequest also means that any symbol keys being passed to setup the env, will be discarded. Only string keys are copied.


uri = env["HTTPS"] == "on" ? "https://www.example.org" : "http://www.example.org"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly defining the uri based on env["HTTPS"] as Rack::MockRequest relies on the URI exclusively: https://github.com/rack/rack/blob/444dc8a130663159ed490f25948b73ff9b5fbba0/lib/rack/mock_request.rb#L110-L111

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does our custom value not get set at the end of env_for?

https://github.com/rack/rack/blob/444dc8a130663159ed490f25948b73ff9b5fbba0/lib/rack/mock_request.rb#L155-L157

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does set, but RACK_URL_SCHEME is not, causing the scheme to remain as http.

Without uri = env["HTTPS"] == "on" ? "https://www.example.org" : "http://www.example.org" the test errors with:

Failure:
RequestPort#test_standard_port? [/home/spin/src/github.com/rails/rails/actionpack/test/dispatch/request_test.rb:354]:
Expected #<ActionDispatch::Request GET "https://example.org:80/" for > to be standard_port?.

Here's the dump of env in mock_request for this test:

Stop by #0  BP - Line  /home/spin/.bundle/rails/gems/rack-3.0.8/lib/rack/mock_request.rb:163 (line)
(rdbg@bin/test#657085) env
{"rack.input"=>#<StringIO:0x00007f0461cd7848>,
 "rack.errors"=>#<StringIO:0x00007f0461cd7938>,
 "REQUEST_METHOD"=>"GET",
 "SERVER_NAME"=>"example.org",
 "SERVER_PORT"=>"80",
 "SERVER_PROTOCOL"=>"HTTP/1.1",
 "QUERY_STRING"=>"",
 "PATH_INFO"=>"/",
 "rack.url_scheme"=>"http",
 "HTTPS"=>"on",
 "SCRIPT_NAME"=>"",
 "CONTENT_LENGTH"=>"0"}

Here's the same structure for the same test, after setting the fix:

Stop by #0  BP - Line  /home/spin/.bundle/rails/gems/rack-3.0.8/lib/rack/mock_request.rb:163 (line)
(rdbg@bin/test#658127) env
{"rack.input"=>#<StringIO:0x00007fc6b73a4f00>,
 "rack.errors"=>#<StringIO:0x00007fc6b73a4ff0>,
 "REQUEST_METHOD"=>"GET",
 "SERVER_NAME"=>"www.example.org",
 "SERVER_PORT"=>"443",
 "SERVER_PROTOCOL"=>"HTTP/1.1",
 "QUERY_STRING"=>"",
 "PATH_INFO"=>"/",
 "rack.url_scheme"=>"https",
 "HTTPS"=>"on",
 "SCRIPT_NAME"=>"",
 "CONTENT_LENGTH"=>"0"}


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
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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"
}

Expand All @@ -658,13 +663,13 @@ class RequestRewind < BaseRequestTest
request.request_parameters

# Should have rewound the body.
assert_equal 0, request.body.pos
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails the Rack::Lint with:

NoMethodError: undefined method `pos' for #<Rack::Lint::InputWrapper:0x00007f89c1fec5c0 @input=#<Rack::Lint::InputWrapper:0x00007f89c1feafb8 @input=#<StringIO:0x00007f89c2052640>>>

Even for Rack v2, I guess the using the Linter highlighted how making too many assumptions on what the body responds to is wrong.

The conditional if Rack.release < "3" was added in #47077.
I was tempted to remove it, but the "body should be rewound" test fails for Rack V2 and the following test "raw_post rewinds rack.input if RAW_POST_DATA is nil" fails for Rack V3.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if instead of removing the test fully we can update the assertions to be more similar to the test below. I think the idea is that calling request_parameterss should allow the body to be ready again, so I think we can make the assertion assert_equal "rewind", request.body.read

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, updated! Ty

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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"

Expand Down Expand Up @@ -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 }
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 } })
Comment on lines -1401 to +1407
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stub_request already allows passing extra values for env via, arguments. No need to load it in @env.

end

test "when early hints is set in the env link headers are sent" do
Expand Down