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 #2161

Merged
merged 5 commits into from
Feb 10, 2021
Merged

Handle EOFError raised by Rack #2161

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f405ecb
Handle EOFError raised by Rack
bschmeck Feb 9, 2021
08bb693
Don't hardcode the content type when encountering an EOFError
bschmeck Feb 10, 2021
c1dd89a
Use Gem::Version instead of directly parsing Rack's version
bschmeck Feb 10, 2021
ca030b1
Use a custom EmptyMessageBody exception, instead of reusing InvalidMe…
bschmeck Feb 10, 2021
e9fb53e
Add Rack 2.2 to the test matrix
bschmeck Feb 10, 2021
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: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

* Your contribution here.

* [#2161](https://github.com/ruby-grape/grape/pull/2157): Handle EOFError from Rack when given an empty multipart body - [@bschmeck](https://github.com/bschmeck).

### 1.5.2 (2021/02/06)

#### Features
Expand Down
2 changes: 2 additions & 0 deletions lib/grape/request.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ def initialize(env, **options)

def params
@params ||= build_params
rescue EOFError
raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data'
Copy link
Member

Choose a reason for hiding this comment

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

I am concerned that this is hardcoding multipart/form-data, input doesn't have to be this way, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with reusing this error, it makes sense to me.

Copy link
Member

Choose a reason for hiding this comment

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

On a second thought, should we create a EmptyMessageBody error?

end

def headers
Expand Down
16 changes: 16 additions & 0 deletions spec/grape/endpoint_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,22 @@ def app
expect(last_response.status).to eq(201)
expect(last_response.body).to eq('Bob')
end

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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

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! I figured there had to be a better way.

next if major < 2 || major == 2 && minor < 2

subject.params do
requires :file, type: Rack::Multipart::UploadedFile
end
subject.post '/upload' do
params[:file][:filename]
end
post '/upload', { file: '' }, 'CONTENT_TYPE' => 'multipart/form-data; boundary=foobar'
expect(last_response.status).to eq(400)
expect(last_response.body).to include('multipart/form-data')
end
end

it 'responds with a 415 for an unsupported content-type' do
Expand Down