From f405ecb1763249f74a93b3cff7ba43d1f6dbcb63 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Tue, 9 Feb 2021 15:16:58 -0600 Subject: [PATCH 1/5] Handle EOFError raised by Rack In v2.2.0, Rack started raising an EOFError when given an empty body with a multipart upload - https://github.com/rack/rack/pull/1572 Previously, Rack had swallowed this error. --- CHANGELOG.md | 2 ++ lib/grape/request.rb | 2 ++ spec/grape/endpoint_spec.rb | 16 ++++++++++++++++ 3 files changed, 20 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2793496880..b610254e7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/lib/grape/request.rb b/lib/grape/request.rb index b547797482..3163aa7989 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -15,6 +15,8 @@ def initialize(env, **options) def params @params ||= build_params + rescue EOFError + raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data' end def headers diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 4bbeb070c3..f121577918 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -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) + 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 From 08bb6939b3d775e2ac3d60093057f3b6794ed4eb Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Tue, 9 Feb 2021 20:04:57 -0600 Subject: [PATCH 2/5] Don't hardcode the content type when encountering an EOFError We most likely are here due to a multipart/form-data Content-Type, but there's no guarantee of that. --- lib/grape/request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/grape/request.rb b/lib/grape/request.rb index 3163aa7989..94fe33b6a9 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -16,7 +16,7 @@ def initialize(env, **options) def params @params ||= build_params rescue EOFError - raise Grape::Exceptions::InvalidMessageBody, 'multipart/form-data' + raise Grape::Exceptions::InvalidMessageBody, content_type end def headers From c1dd89ab3ead13790f2aa337ced9254e33c4ad75 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Wed, 10 Feb 2021 09:35:26 -0600 Subject: [PATCH 3/5] Use Gem::Version instead of directly parsing Rack's version Also move the conditional to a spec condition, to properly skip the spec on versions of Rack that don't raise an error. --- spec/grape/endpoint_spec.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index f121577918..731a7a901e 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -421,11 +421,8 @@ def app 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) - next if major < 2 || major == 2 && minor < 2 - + # Rack swallowed this error until v2.2.0 + it 'returns a 400 if given an invalid multipart body', if: Gem::Version.new(Rack.release) >= Gem::Version.new('2.2.0') do subject.params do requires :file, type: Rack::Multipart::UploadedFile end From ca030b148d54758cab42062d2f60d68badd27b3a Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Wed, 10 Feb 2021 10:07:21 -0600 Subject: [PATCH 4/5] Use a custom EmptyMessageBody exception, instead of reusing InvalidMessageBody --- lib/grape.rb | 1 + lib/grape/exceptions/empty_message_body.rb | 11 +++++++++++ lib/grape/locale/en.yml | 2 +- lib/grape/request.rb | 2 +- spec/grape/endpoint_spec.rb | 2 +- 5 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 lib/grape/exceptions/empty_message_body.rb diff --git a/lib/grape.rb b/lib/grape.rb index c1f313c2f9..818c8f6bc2 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -76,6 +76,7 @@ module Exceptions autoload :InvalidVersionHeader autoload :MethodNotAllowed autoload :InvalidResponse + autoload :EmptyMessageBody end end diff --git a/lib/grape/exceptions/empty_message_body.rb b/lib/grape/exceptions/empty_message_body.rb new file mode 100644 index 0000000000..c4fd431767 --- /dev/null +++ b/lib/grape/exceptions/empty_message_body.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Grape + module Exceptions + class EmptyMessageBody < Base + def initialize(body_format) + super(message: compose_message(:empty_message_body, body_format: body_format), status: 400) + end + end + end +end diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml index 10fa381f7d..036eb93b80 100644 --- a/lib/grape/locale/en.yml +++ b/lib/grape/locale/en.yml @@ -44,6 +44,7 @@ en: "when specifying %{body_format} as content-type, you must pass valid %{body_format} in the request's 'body' " + empty_message_body: 'Empty message body supplied with %{body_format} content-type' invalid_accept_header: problem: 'Invalid accept header' resolution: '%{message}' @@ -51,4 +52,3 @@ en: problem: 'Invalid version header' resolution: '%{message}' invalid_response: 'Invalid response' - diff --git a/lib/grape/request.rb b/lib/grape/request.rb index 94fe33b6a9..0357477d50 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -16,7 +16,7 @@ def initialize(env, **options) def params @params ||= build_params rescue EOFError - raise Grape::Exceptions::InvalidMessageBody, content_type + raise Grape::Exceptions::EmptyMessageBody, content_type end def headers diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 731a7a901e..487fbb0d3e 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -431,7 +431,7 @@ def app 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') + expect(last_response.body).to eq('Empty message body supplied with multipart/form-data; boundary=foobar content-type') end end From e9fb53ef5cffb6e7ae6c82b5e5be034410ddd1fc Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Wed, 10 Feb 2021 10:11:30 -0600 Subject: [PATCH 5/5] Add Rack 2.2 to the test matrix --- .github/workflows/test.yml | 5 +++-- gemfiles/rack2_2.gemfile | 39 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 gemfiles/rack2_2.gemfile diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 95b941260e..9fe44d5f2b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -33,6 +33,7 @@ jobs: - Gemfile - gemfiles/rack1.gemfile - gemfiles/rack2.gemfile + - gemfiles/rack2_2.gemfile - gemfiles/rack_edge.gemfile - gemfiles/rails_edge.gemfile - gemfiles/rails_5.gemfile @@ -74,7 +75,7 @@ jobs: - name: Run tests run: bundle exec rake spec - + - name: Run tests (spec/integration/eager_load) if: ${{ matrix.gemfile == 'Gemfile' }} run: bundle exec rspec spec/integration/eager_load @@ -82,7 +83,7 @@ jobs: - name: Run tests (spec/integration/multi_json) if: ${{ matrix.gemfile == 'gemfiles/multi_json.gemfile' }} run: bundle exec rspec spec/integration/multi_json - + - name: Run tests (spec/integration/multi_xml) if: ${{ matrix.gemfile == 'gemfiles/multi_xml.gemfile' }} run: bundle exec rspec spec/integration/multi_xml diff --git a/gemfiles/rack2_2.gemfile b/gemfiles/rack2_2.gemfile new file mode 100644 index 0000000000..88cfa240d0 --- /dev/null +++ b/gemfiles/rack2_2.gemfile @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +# This file was generated by Appraisal + +source 'https://rubygems.org' + +gem 'rack', '~> 2.2' + +group :development, :test do + gem 'bundler' + gem 'hashie' + gem 'rake' + gem 'rubocop', '1.7.0' + gem 'rubocop-ast', '1.3.0' + gem 'rubocop-performance', '1.9.1', require: false +end + +group :development do + gem 'appraisal' + gem 'benchmark-ips' + gem 'benchmark-memory' + gem 'guard' + gem 'guard-rspec' + gem 'guard-rubocop' +end + +group :test do + gem 'cookiejar' + gem 'coveralls_reborn' + gem 'grape-entity', '~> 0.6' + gem 'maruku' + gem 'mime-types' + gem 'rack-jsonp', require: 'rack/jsonp' + gem 'rack-test', '~> 1.1.0' + gem 'rspec', '~> 3.0' + gem 'ruby-grape-danger', '~> 0.2.0', require: false +end + +gemspec path: '../'