From 3479ba0f2ffb8137a73a4caddb277f8654e996b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cedric=20Ro=CC=88ck?= Date: Wed, 21 Jan 2015 10:31:36 +0100 Subject: [PATCH] Fixes issue #904 by catching the parse error and raising an error that translates into a customized bad request response (+spec) --- CHANGELOG.md | 1 + lib/grape.rb | 1 + lib/grape/exceptions/invalid_message_body.rb | 10 ++ lib/grape/locale/en.yml | 6 + lib/grape/middleware/formatter.rb | 2 + lib/grape/parser/json.rb | 3 + lib/grape/parser/xml.rb | 3 + .../exceptions/body_parse_errors_spec.rb | 105 ++++++++++++++++++ 8 files changed, 131 insertions(+) create mode 100644 lib/grape/exceptions/invalid_message_body.rb create mode 100644 spec/grape/exceptions/body_parse_errors_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index c67c830fca..e8c2ecbfc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ Next Release * [#901](https://github.com/intridea/grape/pull/901): Fix: callbacks defined in a version block are only called for the routes defined in that block - [@kushkella](https://github.com/kushkella). * [#886](https://github.com/intridea/grape/pull/886): Group of parameters made to require an explicit type of Hash or Array - [@jrichter1](https://github.com/jrichter1). * [#912](https://github.com/intridea/grape/pull/912): Extended the `:using` feature for param documentation to `optional` fields - [@croeck](https://github.com/croeck). +* [#906](https://github.com/intridea/grape/pull/906): Fix: invalid body parse errors are not rescued by handlers - [@croeck](https://github.com/croeck). * Your contribution here. 0.10.1 (12/28/2014) diff --git a/lib/grape.rb b/lib/grape.rb index 12b347ae47..8720ce866c 100644 --- a/lib/grape.rb +++ b/lib/grape.rb @@ -57,6 +57,7 @@ module Exceptions autoload :IncompatibleOptionValues, 'grape/exceptions/incompatible_option_values' autoload :MissingGroupTypeError, 'grape/exceptions/missing_group_type' autoload :UnsupportedGroupTypeError, 'grape/exceptions/unsupported_group_type' + autoload :InvalidMessageBody, 'grape/exceptions/invalid_message_body' end module ErrorFormatter diff --git a/lib/grape/exceptions/invalid_message_body.rb b/lib/grape/exceptions/invalid_message_body.rb new file mode 100644 index 0000000000..f5866b1584 --- /dev/null +++ b/lib/grape/exceptions/invalid_message_body.rb @@ -0,0 +1,10 @@ +# encoding: utf-8 +module Grape + module Exceptions + class InvalidMessageBody < Base + def initialize(body_format) + super(message: compose_message('invalid_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 a061ba9343..1ef11648e1 100644 --- a/lib/grape/locale/en.yml +++ b/lib/grape/locale/en.yml @@ -35,4 +35,10 @@ en: all_or_none: 'provide all or none of parameters' missing_group_type: 'group type is required' unsupported_group_type: 'group type must be Array or Hash' + invalid_message_body: + problem: "message body does not match declared format" + resolution: + "when specifying %{body_format} as content-type, you must pass valid + %{body_format} in the request's 'body' + " diff --git a/lib/grape/middleware/formatter.rb b/lib/grape/middleware/formatter.rb index c3870bc807..55895a729c 100644 --- a/lib/grape/middleware/formatter.rb +++ b/lib/grape/middleware/formatter.rb @@ -81,6 +81,8 @@ def read_rack_input(body) end env['rack.request.form_input'] = env['rack.input'] end + rescue Grape::Exceptions::Base => e + raise e rescue StandardError => e throw :error, status: 400, message: e.message end diff --git a/lib/grape/parser/json.rb b/lib/grape/parser/json.rb index 404ae7b3bd..58bd874826 100644 --- a/lib/grape/parser/json.rb +++ b/lib/grape/parser/json.rb @@ -4,6 +4,9 @@ module Json class << self def call(object, env) MultiJson.load(object) + rescue MultiJson::ParseError + # handle JSON parsing errors via the rescue handlers or provide error message + raise Grape::Exceptions::InvalidMessageBody, 'application/json' end end end diff --git a/lib/grape/parser/xml.rb b/lib/grape/parser/xml.rb index b56642b0a5..2ee9368b26 100644 --- a/lib/grape/parser/xml.rb +++ b/lib/grape/parser/xml.rb @@ -4,6 +4,9 @@ module Xml class << self def call(object, env) MultiXml.parse(object) + rescue MultiXml::ParseError + # handle XML parsing errors via the rescue handlers or provide error message + raise Grape::Exceptions::InvalidMessageBody, 'application/xml' end end end diff --git a/spec/grape/exceptions/body_parse_errors_spec.rb b/spec/grape/exceptions/body_parse_errors_spec.rb new file mode 100644 index 0000000000..6f0230d16d --- /dev/null +++ b/spec/grape/exceptions/body_parse_errors_spec.rb @@ -0,0 +1,105 @@ +require 'spec_helper' + +describe Grape::Exceptions::ValidationErrors do + context 'api with rescue_from :all handler' do + subject { Class.new(Grape::API) } + before { + subject.rescue_from :all do |e| + rack_response 'message was processed', 400 + end + subject.params do + requires :beer + end + subject.post '/beer' do + 'beer received' + end + } + + def app + subject + end + + context 'with content_type json' do + it 'can recover from failed body parsing' do + post '/beer', 'test', 'CONTENT_TYPE' => 'application/json' + expect(last_response.status).to eq 400 + expect(last_response.body).to eq('message was processed') + end + end + + context 'with content_type xml' do + it 'can recover from failed body parsing' do + post '/beer', 'test', 'CONTENT_TYPE' => 'application/xml' + expect(last_response.status).to eq 400 + expect(last_response.body).to eq('message was processed') + end + end + + context 'with content_type text' do + it 'can recover from failed body parsing' do + post '/beer', 'test', 'CONTENT_TYPE' => 'text/plain' + expect(last_response.status).to eq 400 + expect(last_response.body).to eq('message was processed') + end + end + + context 'with no specific content_type' do + it 'can recover from failed body parsing' do + post '/beer', 'test', {} + expect(last_response.status).to eq 400 + expect(last_response.body).to eq('message was processed') + end + end + end + + context 'api without a rescue handler' do + subject { Class.new(Grape::API) } + before { + subject.params do + requires :beer + end + subject.post '/beer' do + 'beer received' + end + } + + def app + subject + end + + context 'and with content_type json' do + it 'can recover from failed body parsing' do + post '/beer', 'test', 'CONTENT_TYPE' => 'application/json' + expect(last_response.status).to eq 400 + expect(last_response.body).to include('message body does not match declared format') + expect(last_response.body).to include('application/json') + end + end + + context 'with content_type xml' do + it 'can recover from failed body parsing' do + post '/beer', 'test', 'CONTENT_TYPE' => 'application/xml' + expect(last_response.status).to eq 400 + expect(last_response.body).to include('message body does not match declared format') + expect(last_response.body).to include('application/xml') + end + end + + context 'with content_type text' do + it 'can recover from failed body parsing' do + post '/beer', 'test', 'CONTENT_TYPE' => 'text/plain' + expect(last_response.status).to eq 400 + expect(last_response.body).to eq('beer is missing') + end + end + + context 'and with no specific content_type' do + it 'can recover from failed body parsing' do + post '/beer', 'test', {} + expect(last_response.status).to eq 400 + # plain response with text/html + expect(last_response.body).to eq('beer is missing') + end + end + end +end