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.

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

Validates response processed by exception handler #1776

1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
* [#1758](/~https://github.com/ruby-grape/grape/pull/1758): Fix expanding load_path in gemspec - [@2maz](/~https://github.com/2maz).
* [#1765](/~https://github.com/ruby-grape/grape/pull/1765): Use 415 when request body is of an unsupported media type - [@jdmurphy](/~https://github.com/jdmurphy).
* [#1771](/~https://github.com/ruby-grape/grape/pull/1771): Fix param aliases with 'given' blocks - [@jereynolds](/~https://github.com/jereynolds).
* [#1776](/~https://github.com/ruby-grape/grape/pull/1776): Validates response processed by exception handler - [@darren987469](/~https://github.com/darren987469).
Copy link
Member

Choose a reason for hiding this comment

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

We released 1.1.0, so this belongs in the next release above.


### 1.0.3 (4/23/2018)

Expand Down
8 changes: 7 additions & 1 deletion lib/grape/middleware/error.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,13 @@ def run_rescue_handler(handler, error)
handler = public_method(handler)
end

handler.arity.zero? ? instance_exec(&handler) : instance_exec(error, &handler)
response = handler.arity.zero? ? instance_exec(&handler) : instance_exec(error, &handler)
valid_response?(response) ? response : error!('Internal Server Error(Invalid Response)')
Copy link
Member

Choose a reason for hiding this comment

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

Should we re-raise the original error instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I will change the code to use default_rescue_handler to handle the original error. That means, if custom handlers failed to do their job (return valid response), default_rescue_handler will handle it.

end

def valid_response?(response)
Copy link
Member

Choose a reason for hiding this comment

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

Do we do anything similar elsewhere? Seems a bit fishy, possibly belongs in Rack (maybe there's a method somewhere to check that the response is valid)?

Copy link
Contributor Author

@darren987469 darren987469 Aug 7, 2018

Choose a reason for hiding this comment

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

After searching, I don't find anything similar.

I know this is a bad smell. The better solution is let exception handler return a Rack:: Response object, rather than an array (currently returned by Rack::Response.new.finish). Then we can check whether the handler return Rack:: Response object. However, that break the existing behavior and needs a lot of corresponding changes not only for grape but also for users of grape.

What do you think? @dblock

# Rack::Response.new(...).finish generates an array with size 3
response.is_a?(Array) && response.size == 3
end
end
end
Expand Down
20 changes: 20 additions & 0 deletions spec/grape/api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1723,6 +1723,26 @@ class CustomError < Grape::Exceptions::Base; end
expect(last_response.status).to eql 500
expect(last_response.body).to eq('Formatter Error')
end

it 'validates response processed by exception handler' do
subject.rescue_from ArgumentError do
error!('rain!')
nil # invalid response caused by return nil
end
subject.rescue_from :all do
error!('rain!')
end
subject.get('/invalid_response') { raise ArgumentError }
subject.get('/valid_response') { raise 'rain!' }

get '/invalid_response'
expect(last_response.status).to eql 500
expect(last_response.body).to eq('Internal Server Error(Invalid Response)')

get '/valid_response'
expect(last_response.status).to eql 500
expect(last_response.body).to eq('rain!')
end
end

describe '.rescue_from klass, block' do
Expand Down