Handle EOFError raised by Rack and raise BadRequest (and lock Rack version to 2.0 to pass tests)

[tamazon]

This fixes #1739. Now Sinatra returns 400 Bad Request instead of 500.

[tamazon]

I can't understand why this case fails. I need help.

  1) Failure:
RoutingTest#test_handles_params_without_a_value_0 [/home/runner/work/sinatra/sinatra/test/routing_test.rb:317]:
Expected "" to be nil.

1. I also tried running this test on master branch and older releases and it still fails.
2. It works when I change Rack version from 2.3.0 to 2.2.3.

I assume this is problem of Rack but I have no idea why it happens and why Gemfile specifies rack's main branch to install.

[tamazon]

rack/rack#1696
This is probably the change that causes this behavior.

from CHANGELOG of 3.0.0

BREAKING CHANGE: Query parsing now treats parameters without = as having the empty string value instead of nil value,
to conform to the URL spec. (#1696, @jeremyevans)

[tamazon]

I guess changing this line from: assert_nil params.fetch('foo') to: assert_equal '', params.fetch('foo') fix this problem.
I'm not sure it's appropriate to do so.

[jkowens]

I think we can lock the Gemfile to rack '~> 2.0' for now.

[ioquatix]

It's not clear to me why you escape the HTML here, rather than on the output formatting side.

[dentarg]

I can only provide background info, not speak to if this is the best solution. The introduction of handling of EOFError probably just followed what was already there: the handling of invalid query parameters – escaping those was introduced in #1432 to address an reported XSS #1428 (the initial handling of invalid query parameters was added in #1070)

Feel free to make a PR if you have a more appropriate solution.

[dentarg]

I think it makes sense to escape here as otherwise every application outputting the BadRequest message would need to do it.

[ioquatix]

You should escape any error messages regardless because you don't control every error message generation, right? It should be escaped when generating HTML from the error message, not when creating an exception. Otherwise, how do you print this to the command-line, log file, etc. Maybe I'm misunderstanding something.

[dentarg]

Sure, what you say makes sense. Feel free to contribute and maybe others will weigh in as well.

[tamazon]

Thanks for mentioning. To be honest, I just copied lines above and changed error type and message. I didn't think about whether this usage of escape_html was correct or not.