Add Rack::Lint to ActionDispatch::RemoteIp tests
#48835
Conversation
| @@ -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.
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.
| @@ -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" | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Does our custom value not get set at the end of env_for?
There was a problem hiding this comment.
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.early_hints"] = lambda { |links| links } | ||
| @request = stub_request | ||
| @request = stub_request({ "rack.early_hints" => lambda { |links| links } }) |
There was a problem hiding this comment.
stub_request already allows passing extra values for env via, arguments. No need to load it in @env.
| @@ -4,10 +4,6 @@ | |||
|
|
|||
| class BaseRequestTest < ActiveSupport::TestCase | |||
| def setup | |||
| @env = { | |||
| :ip_spoofing_check => true, | |||
There was a problem hiding this comment.
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 👍
| @@ -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" | |||
There was a problem hiding this comment.
Does our custom value not get set at the end of env_for?
|
Gonna go ahead and try to fix the tests for rack 2.x |
nunosilva800
force-pushed
the
ns-rack-lint-remote-ip
branch
2 times, most recently
from
7cd6e77
to
1c44ad1
Compare
| request.request_parameters | ||
|
|
||
| # Should have rewound the body. | ||
| assert_equal 0, request.body.pos |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Nice, updated! Ty
nunosilva800
force-pushed
the
ns-rack-lint-remote-ip
branch
from
1c44ad1
to
00a5f89
Compare
nunosilva800
force-pushed
the
ns-rack-lint-remote-ip
branch
from
00a5f89
to
54e1d1e
Compare
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
nunosilva800
force-pushed
the
ns-rack-lint-remote-ip
branch
from
54e1d1e
to
613e8fd
Compare
Motivation / Background
To ensure Rails is and remains compliant with the Rack 3 spec we can add
Rack::Lintto the Rails middleware tests.This adds additional test coverage to
ActionDispatch::RemoteIpto validate that its input and output follow the Rack SPEC.Detail
The only code testing this middleware are the ones for
ActionDispatch::Request.Several changes were required to make the tests pass:
CONTENT_LENGTHmust be a stringSERVER_PORTmust be a stringHTTP_HOSTmust be a stringrack.inputmust be an IO object, with ASCII-8BIT encoding - By leveragingRack::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 hereRack::MockRequestalso means that any symbol keys being passed to setup the env, will be discarded. Only string keys are copied.Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]