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

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

Merged
merged 2 commits into from Feb 2, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df65a73
Handle EOFError raised by Rack
tamazon Jan 30, 2022
c29749d
lock rack version to 2.0 for now
tamazon Feb 1, 2022
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
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Expand Up @@ -11,7 +11,7 @@ source 'https://rubygems.org' unless ENV['QUICK']
gemspec

gem 'rake'
gem 'rack', git: 'https://github.com/rack/rack.git'
gem 'rack', '~> 2.0'
jkowens marked this conversation as resolved.
Show resolved Hide resolved
gem 'rack-test', '>= 0.6.2'
gem "minitest", "~> 5.0"
gem 'yard'
Expand Down
2 changes: 2 additions & 0 deletions lib/sinatra/base.rb
View file
Open in desktop
Expand Up @@ -78,6 +78,8 @@ def params
super
rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
raise BadRequest, "Invalid query parameters: #{Rack::Utils.escape_html(e.message)}"
rescue EOFError => e
raise BadRequest, "Invalid multipart/form-data: #{Rack::Utils.escape_html(e.message)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

end

class AcceptEntry
Expand Down
9 changes: 9 additions & 0 deletions test/request_test.rb
View file
Open in desktop
Expand Up @@ -17,6 +17,15 @@ class RequestTest < Minitest::Test
assert_equal 'bar', request.params['foo']
end

it 'raises Sinatra::BadRequest when multipart/form-data request has no content' do
request = Sinatra::Request.new(
'REQUEST_METHOD' => 'POST',
'CONTENT_TYPE' => 'multipart/form-data; boundary=dummy',
'rack.input' => StringIO.new('')
)
assert_raises(Sinatra::BadRequest) { request.params }
end

it 'is secure when the url scheme is https' do
request = Sinatra::Request.new('rack.url_scheme' => 'https')
assert request.secure?
Expand Down