Handle EOFError raised by Rack#2161
Conversation
2676741 to
a723b9f
Compare
In v2.2.0, Rack started raising an EOFError when given an empty body with a multipart upload - rack/rack#1572 Previously, Rack had swallowed this error.
a723b9f to
f405ecb
Compare
| def params | ||
| @params ||= build_params | ||
| rescue EOFError | ||
| raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data' |
There was a problem hiding this comment.
I am concerned that this is hardcoding multipart/form-data, input doesn't have to be this way, right?
There was a problem hiding this comment.
I don't know when Rack would raise this error other than when given a multipart/form-data, but I'm not a Rack expert. I've pushed a commit to use the object's content_type method instead.
This error class seemed the most appropriated, but it requires a content type. I'm happy to create a different error class if that would be preferable.
There was a problem hiding this comment.
I'm ok with reusing this error, it makes sense to me.
There was a problem hiding this comment.
On a second thought, should we create a EmptyMessageBody error?
We most likely are here due to a multipart/form-data Content-Type, but there's no guarantee of that.
|
|
||
| it 'returns a 400 if given an invalid multipart body' do | ||
| # Rack swallowed this error until v2.2.0 | ||
| major, minor, _patch = Rack.release.split('.').map(&:to_i) |
There was a problem hiding this comment.
The "correct" way to do this is to use Gem::Version.new(Rack.release) >= ..., see https://code.dblock.org/2020/07/16/comparing-version-numbers-in-ruby.html if it's helpful.
Let's also externalize the if as a spec condition so we see it properly skipped.
There was a problem hiding this comment.
Thanks! I figured there had to be a better way.
| def params | ||
| @params ||= build_params | ||
| rescue EOFError | ||
| raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data' |
There was a problem hiding this comment.
On a second thought, should we create a EmptyMessageBody error?
dblock
left a comment
There was a problem hiding this comment.
Let's also add a gemfile and run it as part of the matrix explicitly with this version of Rack, ala https://github.com/ruby-grape/grape/blob/master/gemfiles/rack2.gemfile
Also move the conditional to a spec condition, to properly skip the spec on versions of Rack that don't raise an error.
|
I've cleaned up how the test is skipped for older versions of Rack, added a custom I think that addresses all of your comments, but let me know if I missed something. |
|
Thanks for hanging in here, great job. Merged. |
|
Related, we had #2130, maybe you care to resolve it one way or another? I do think a note on all Rack-related issues could be useful somewhere. |
In v2.2.0, Rack started raising an EOFError when given an empty body with a multipart upload: rack/rack#1572. Previously, Rack swallowed this error and execution continued normally. This PR translates that error to an
InvalidMessageBodyexception, letting Grape return a 400 class error, instead of a 500 class error.Note that the error raised by Rack will change to
Rack::Multipart::EmptyContentErrorin v3 - rack/rack#1688