From 4c9163ec3144fd9eb7712391c393bff2f0488c01 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:20:30 -0500 Subject: [PATCH 01/12] Capture Rack's "too many multipart files" error with a spec --- spec/grape/endpoint_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 9fee973cab..7e31850838 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -436,6 +436,19 @@ def app end end + it 'returns a 400 if given too many multipart files' do + Rack::Utils.multipart_part_limit = 1 + + subject.params do + requires :file, type: Rack::Multipart::UploadedFile + end + subject.post '/upload' do + params[:file][:filename] + end + post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, "text/plain"), extra: Rack::Test::UploadedFile.new(__FILE__, "text/plain") } + expect(last_response.status).to eq(400) + end + it 'responds with a 415 for an unsupported content-type' do subject.format :json # subject.content_type :json, "application/json" From c1d73f4563d62ee898170d4ebdf79c534c2c6029 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:31:52 -0500 Subject: [PATCH 02/12] Create a custom error class to handle Rack's multipart error --- lib/grape.rb | 1 + lib/grape/exceptions/too_many_multipart_files.rb | 11 +++++++++++ lib/grape/locale/en.yml | 1 + 3 files changed, 13 insertions(+) create mode 100644 lib/grape/exceptions/too_many_multipart_files.rb diff --git a/lib/grape.rb b/lib/grape.rb index 49b3ec4037..c8824eff72 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -81,6 +81,7 @@ module Exceptions autoload :MethodNotAllowed autoload :InvalidResponse autoload :EmptyMessageBody + autoload :TooManyMultipartFiles autoload :MissingGroupTypeError, 'grape/exceptions/missing_group_type' autoload :UnsupportedGroupTypeError, 'grape/exceptions/unsupported_group_type' end diff --git a/lib/grape/exceptions/too_many_multipart_files.rb b/lib/grape/exceptions/too_many_multipart_files.rb new file mode 100644 index 0000000000..073f79d267 --- /dev/null +++ b/lib/grape/exceptions/too_many_multipart_files.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Grape + module Exceptions + class TooManyMultipartFiles < Base + def initialize + super(message: compose_message(:too_many_multipart_files), status: 400) + end + end + end +end diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml index 036eb93b80..b4f5c331ae 100644 --- a/lib/grape/locale/en.yml +++ b/lib/grape/locale/en.yml @@ -45,6 +45,7 @@ en: %{body_format} in the request's 'body' " empty_message_body: 'Empty message body supplied with %{body_format} content-type' + too_many_multipart_files: "The number of uploaded files exceeded the system's configured limit" invalid_accept_header: problem: 'Invalid accept header' resolution: '%{message}' From 45b8bad527408f382e33d8949cd856a72dc0086f Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:33:16 -0500 Subject: [PATCH 03/12] fixup! Capture Rack's "too many multipart files" error with a spec --- spec/grape/endpoint_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 7e31850838..305db3a9a7 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -445,7 +445,7 @@ def app subject.post '/upload' do params[:file][:filename] end - post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, "text/plain"), extra: Rack::Test::UploadedFile.new(__FILE__, "text/plain") } + post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') } expect(last_response.status).to eq(400) end From 8293f10f110ecc6e069f093999a7ceb8f5059d6e Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:33:33 -0500 Subject: [PATCH 04/12] Use our custom error class when Rack raises a multipart limit error --- lib/grape/request.rb | 2 ++ spec/grape/endpoint_spec.rb | 1 + 2 files changed, 3 insertions(+) diff --git a/lib/grape/request.rb b/lib/grape/request.rb index e3ed4492d6..7bf0fff8df 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -17,6 +17,8 @@ def params @params ||= build_params rescue EOFError raise Grape::Exceptions::EmptyMessageBody.new(content_type) + rescue Rack::Multipart::MultipartPartLimitError + raise Grape::Exceptions::TooManyMultipartFiles end def headers diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 305db3a9a7..fb2e07e782 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -447,6 +447,7 @@ def app end post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') } expect(last_response.status).to eq(400) + expect(last_response.body).to eq("The number of uploaded files exceeded the system's configured limit") end it 'responds with a 415 for an unsupported content-type' do From b5a47cd50dd778210ca33a4b3f33b6f7126dea6f Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:39:34 -0500 Subject: [PATCH 05/12] Use a Payload Too Large status code for TooManyMultipartFiles errors --- lib/grape/exceptions/too_many_multipart_files.rb | 2 +- spec/grape/endpoint_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/grape/exceptions/too_many_multipart_files.rb b/lib/grape/exceptions/too_many_multipart_files.rb index 073f79d267..aa2a6f1fd3 100644 --- a/lib/grape/exceptions/too_many_multipart_files.rb +++ b/lib/grape/exceptions/too_many_multipart_files.rb @@ -4,7 +4,7 @@ module Grape module Exceptions class TooManyMultipartFiles < Base def initialize - super(message: compose_message(:too_many_multipart_files), status: 400) + super(message: compose_message(:too_many_multipart_files), status: 413) end end end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index fb2e07e782..a1c4f04271 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -436,7 +436,7 @@ def app end end - it 'returns a 400 if given too many multipart files' do + it 'returns a 413 if given too many multipart files' do Rack::Utils.multipart_part_limit = 1 subject.params do @@ -446,7 +446,7 @@ def app params[:file][:filename] end post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') } - expect(last_response.status).to eq(400) + expect(last_response.status).to eq(413) expect(last_response.body).to eq("The number of uploaded files exceeded the system's configured limit") end From bc884201a7bc955e225700bd9bf0472ca695eca6 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:47:13 -0500 Subject: [PATCH 06/12] Restore Rack's multipart limit after testing the failure --- spec/grape/endpoint_spec.rb | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index a1c4f04271..3cf30456af 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -436,18 +436,25 @@ def app end end - it 'returns a 413 if given too many multipart files' do - Rack::Utils.multipart_part_limit = 1 - - subject.params do - requires :file, type: Rack::Multipart::UploadedFile + context 'when the limit on multipart files is exceeded' do + around do |example| + limit = Rack::Utils.multipart_part_limit + Rack::Utils.multipart_part_limit = 1 + example.run + Rack::Utils.multipart_part_limit = limit end - subject.post '/upload' do - params[:file][:filename] + + it 'returns a 413 if given too many multipart files' do + subject.params do + requires :file, type: Rack::Multipart::UploadedFile + end + subject.post '/upload' do + params[:file][:filename] + end + post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') } + expect(last_response.status).to eq(413) + expect(last_response.body).to eq("The number of uploaded files exceeded the system's configured limit") end - post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') } - expect(last_response.status).to eq(413) - expect(last_response.body).to eq("The number of uploaded files exceeded the system's configured limit") end it 'responds with a 415 for an unsupported content-type' do From dad121c66c7c6b8ad1b1cac44801d60fe2b11de9 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Thu, 21 Apr 2022 11:51:01 -0500 Subject: [PATCH 07/12] Upadate CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 10544bd24f..ab22f8f6ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ * [#2227](https://github.com/ruby-grape/grape/pull/2222): Rename `MissingGroupType` and `UnsupportedGroupType` exceptions - [@ericproulx](https://github.com/ericproulx). * [#2244](https://github.com/ruby-grape/grape/pull/2244): Fix a breaking change in `Grape::Validations` provided in 1.6.1 - [@dm1try](https://github.com/dm1try). * [#2250](https://github.com/ruby-grape/grape/pull/2250): Add deprecation warning for `UnsupportedGroupTypeError` and `MissingGroupTypeError` - [@ericproulx](https://github.com/ericproulx). +* [#2256](https://github.com/ruby-grape/grape/pull/2256): Handle MultipartPartLimitError from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck). * Your contribution here. ### 1.6.2 (2021/12/30) From 2cdb1b0fd357c16de9cfb8fbfb5eb1ac78e021af Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Mon, 25 Apr 2022 11:13:35 -0500 Subject: [PATCH 08/12] Reword CHANGELOG entry for MultipartPartLimitError change --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab22f8f6ef..154374acd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ * [#2227](https://github.com/ruby-grape/grape/pull/2222): Rename `MissingGroupType` and `UnsupportedGroupType` exceptions - [@ericproulx](https://github.com/ericproulx). * [#2244](https://github.com/ruby-grape/grape/pull/2244): Fix a breaking change in `Grape::Validations` provided in 1.6.1 - [@dm1try](https://github.com/dm1try). * [#2250](https://github.com/ruby-grape/grape/pull/2250): Add deprecation warning for `UnsupportedGroupTypeError` and `MissingGroupTypeError` - [@ericproulx](https://github.com/ericproulx). -* [#2256](https://github.com/ruby-grape/grape/pull/2256): Handle MultipartPartLimitError from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck). +* [#2256](https://github.com/ruby-grape/grape/pull/2256): Raise Grape::Exceptions::MultipartPartLimitError from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck). * Your contribution here. ### 1.6.2 (2021/12/30) From 190db464482c74e097b9f773c90ba5364cf276e4 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Mon, 25 Apr 2022 16:47:00 -0500 Subject: [PATCH 09/12] Backticks around classname in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 154374acd4..e0fa959add 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ * [#2227](https://github.com/ruby-grape/grape/pull/2222): Rename `MissingGroupType` and `UnsupportedGroupType` exceptions - [@ericproulx](https://github.com/ericproulx). * [#2244](https://github.com/ruby-grape/grape/pull/2244): Fix a breaking change in `Grape::Validations` provided in 1.6.1 - [@dm1try](https://github.com/dm1try). * [#2250](https://github.com/ruby-grape/grape/pull/2250): Add deprecation warning for `UnsupportedGroupTypeError` and `MissingGroupTypeError` - [@ericproulx](https://github.com/ericproulx). -* [#2256](https://github.com/ruby-grape/grape/pull/2256): Raise Grape::Exceptions::MultipartPartLimitError from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck). +* [#2256](https://github.com/ruby-grape/grape/pull/2256): Raise `Grape::Exceptions::MultipartPartLimitError` from Rack when too many files are uploaded - [@bschmeck](https://github.com/bschmeck). * Your contribution here. ### 1.6.2 (2021/12/30) From 2d8da411f20e9a3f2c5fe39b1af13735fa0b3dc8 Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Mon, 25 Apr 2022 16:48:59 -0500 Subject: [PATCH 10/12] Change next version of Grape from 1.6.3 to 1.7.0 The introduction of the TooManyMultipartFiles changes API behavior, so bump the minor version. --- CHANGELOG.md | 2 +- README.md | 2 +- UPGRADING.md | 2 +- lib/grape/version.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0fa959add..e360bffa17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ -### 1.6.3 (Next) +### 1.7.0 (Next) #### Features diff --git a/README.md b/README.md index e5a921474e..fb83a68c9f 100644 --- a/README.md +++ b/README.md @@ -158,7 +158,7 @@ content negotiation, versioning and much more. ## Stable Release -You're reading the documentation for the next release of Grape, which should be **1.6.3**. +You're reading the documentation for the next release of Grape, which should be **1.7.0**. Please read [UPGRADING](UPGRADING.md) when upgrading from a previous version. The current stable release is [1.6.2](https://github.com/ruby-grape/grape/blob/v1.6.2/README.md). diff --git a/UPGRADING.md b/UPGRADING.md index 969fc1347c..9e5549d65d 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,7 +1,7 @@ Upgrading Grape =============== -### Upgrading to >= 1.6.3 +### Upgrading to >= 1.7.0 #### Exceptions renaming diff --git a/lib/grape/version.rb b/lib/grape/version.rb index af7c1a7cde..3a0817e8db 100644 --- a/lib/grape/version.rb +++ b/lib/grape/version.rb @@ -2,5 +2,5 @@ module Grape # The current version of Grape. - VERSION = '1.6.3' + VERSION = '1.7.0' end From 46cbf077b72fae524608e960d630c4678ecc4a6e Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Mon, 25 Apr 2022 16:56:15 -0500 Subject: [PATCH 11/12] Include the system's configured multipart file limit in the error message The number of allowed multipart files is a configurable value in Rack, pull that limit and include it in the generated error message. --- lib/grape/exceptions/too_many_multipart_files.rb | 4 ++-- lib/grape/locale/en.yml | 2 +- lib/grape/request.rb | 2 +- spec/grape/endpoint_spec.rb | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/grape/exceptions/too_many_multipart_files.rb b/lib/grape/exceptions/too_many_multipart_files.rb index aa2a6f1fd3..72deffb39a 100644 --- a/lib/grape/exceptions/too_many_multipart_files.rb +++ b/lib/grape/exceptions/too_many_multipart_files.rb @@ -3,8 +3,8 @@ module Grape module Exceptions class TooManyMultipartFiles < Base - def initialize - super(message: compose_message(:too_many_multipart_files), status: 413) + def initialize(limit) + super(message: compose_message(:too_many_multipart_files, limit: limit), status: 413) end end end diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml index b4f5c331ae..546529a3e8 100644 --- a/lib/grape/locale/en.yml +++ b/lib/grape/locale/en.yml @@ -45,7 +45,7 @@ en: %{body_format} in the request's 'body' " empty_message_body: 'Empty message body supplied with %{body_format} content-type' - too_many_multipart_files: "The number of uploaded files exceeded the system's configured limit" + too_many_multipart_files: "The number of uploaded files exceeded the system's configured limit (%{limit})" invalid_accept_header: problem: 'Invalid accept header' resolution: '%{message}' diff --git a/lib/grape/request.rb b/lib/grape/request.rb index 7bf0fff8df..c45df98761 100644 --- a/lib/grape/request.rb +++ b/lib/grape/request.rb @@ -18,7 +18,7 @@ def params rescue EOFError raise Grape::Exceptions::EmptyMessageBody.new(content_type) rescue Rack::Multipart::MultipartPartLimitError - raise Grape::Exceptions::TooManyMultipartFiles + raise Grape::Exceptions::TooManyMultipartFiles.new(Rack::Utils.multipart_part_limit) end def headers diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 3cf30456af..a599abdf40 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -453,7 +453,7 @@ def app end post '/upload', { file: Rack::Test::UploadedFile.new(__FILE__, 'text/plain'), extra: Rack::Test::UploadedFile.new(__FILE__, 'text/plain') } expect(last_response.status).to eq(413) - expect(last_response.body).to eq("The number of uploaded files exceeded the system's configured limit") + expect(last_response.body).to eq("The number of uploaded files exceeded the system's configured limit (1)") end end From 7bfd055e9d5aa4a4b74533a536e71e1606d9268a Mon Sep 17 00:00:00 2001 From: Ben Schmeckpeper Date: Mon, 25 Apr 2022 17:39:18 -0500 Subject: [PATCH 12/12] Add a note to UPGRADING about the new TooManyMultipartFiles exception --- UPGRADING.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index 9e5549d65d..620e1d6ec9 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -12,6 +12,10 @@ The following exceptions has been renamed for consistency through exceptions nam See [#2227](https://github.com/ruby-grape/grape/pull/2227) for more information. +#### Handling Multipart Limit Errors + +Rack supports a configurable limit on the number of files created from multipart parameters (`Rack::Utils.multipart_part_limit`) and raises an error if params are received that create too many files. If you were handling the Rack error directly, Grape now wraps that error in `Grape::Execeptions::TooManyMultipartFiles`. Additionally, Grape will return a 413 status code if the exception goes unhandled. + ### Upgrading to >= 1.6.0 #### Parameter renaming with :as