-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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.
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
Add Rack::Lint to ActionDispatch::RemoteIp
tests
#48835
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicitly defining the uri based on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does our custom value not get set at the end of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Here's the dump of
Here's the same structure for the same test, after setting the fix:
|
||
|
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fails the
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 } }) | ||
Comment on lines
-1401
to
+1407
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
end | ||
|
||
test "when early hints is set in the env link headers are sent" do | ||
|
There was a problem hiding this comment.
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 totrue
instub_request
if the key isn't present. So this makes sense to me 👍