POST request with no body results in a rack exception

[thexa4]

Steps to reproduce:

1. rails new proofofconcept
2. cd proofofconcept
3. rails s &
4. curl -XPOST http://localhost:3000 -> '404'
5. Kill rails server
6. iodine &
7. curl -XPOST http://localhost:3000 -> '500'

Results in the following exception:

ERROR: Iodine caught an unprotected exception - NoMethodError: undefined method `[]' for nil:NilClass
/usr/lib/ruby/vendor_ruby/rack/request.rb:340:in `POST'
/usr/lib/ruby/vendor_ruby/rack/method_override.rb:43:in `method_override_param'
/usr/lib/ruby/vendor_ruby/rack/method_override.rb:27:in `method_override'
/usr/lib/ruby/vendor_ruby/rack/method_override.rb:15:in `call'
/usr/lib/ruby/vendor_ruby/rack/runtime.rb:22:in `call'
/usr/share/rubygems-integration/all/gems/activesupport-5.2.2.1/lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
/usr/share/rubygems-integration/all/gems/actionpack-5.2.2.1/lib/action_dispatch/middleware/executor.rb:14:in `call'
/usr/share/rubygems-integration/all/gems/actionpack-5.2.2.1/lib/action_dispatch/middleware/static.rb:127:in `call'
/usr/lib/ruby/vendor_ruby/rack/sendfile.rb:111:in `call'
/usr/share/rubygems-integration/all/gems/railties-5.2.2.1/lib/rails/engine.rb:524:in `call'
ERROR:

This seems to happen due to this line: https://github.com/rack/rack/blob/master/lib/rack/request.rb#L352 , form_vars is set to nil by get_header(RACK_INPUT).read which causes the safari fix check to throw a NoMethodError.

[SleeplessByte]

This should probably also check the content_length before performing any mutation.

[boazsegev]

@thexa4 , thank you very much for exposing this issue. 🎉👏🏻🙏🏻👍🏻

You're right, iodine should have returned an empty string ("") rather than nil. The issue occurred when an optimized "no content" path was taken instead of reaching EOF on the rack.input stream.

I posted an initial patch and I hope to release it soon, after running a few tests.

@SleeplessByte,

I'm not sure what you meant by suggesting to test the content_length?

I'm assuming you were suggesting a way to patch Rack... and although I think your instincts are great, 99.9% of all POST requests probably contain some data. I think it's fair that Rack would upstream the edge-case handling to the server.

[boazsegev]

Patch released. closing issue.

Thanks again! 🙏🏻👍🏻🎉

[SleeplessByte]

@boazsegev you're absolutely right on my intention with that comment, and I agree that this edge case can be handled here, in iodine :). Thank you for the blazingly fast update ❤️

The biggest issue I have with the rack implementation is that the comment for the rack functionality says it's supposed to check if there is data, whereas it's actually checking if there could be data -- and then it's further assumed that there is data.

[boazsegev]

@SleeplessByte - yes, I agree... the Rack documentation could be improved.

On the other hand, Rack was a huge undertaking and it's so relied upon that it became practically impossible to replace. It's battle tested and dependable. No matter how many of us complain about Rack's CGI core, it's probably here to stay and it's making many developers happy.

[SleeplessByte]

It makes me happy 😉 so it gets my upvote regardless of the 🆗 documentation.