Validates response processed by exception handler #1776

darren987469 · 2018-08-05T08:36:57Z

Try to fix #1757.

Problem

class Api < Grape::API
  rescue_from :all do
    error!('rain')
    nil
  end
end

The rescue_from block return nil, that cause response like

500,
NoMethodError (undefined method `[]' for nil:NilClass):

grape (1.0.2) lib/grape/router.rb:163:in `cascade?'
grape (1.0.2) lib/grape/router.rb:95:in `transaction'
grape (1.0.2) lib/grape/router.rb:72:in `identity'
grape (1.0.2) lib/grape/router.rb:57:in `block in call'
...

Response exposes the system information.

After PR

Response would like

500, Internal Server Error(Invalid Response)

Not sure the message is proper or not.

If block of return nil, the response would be nil. That causes system information leak. Add a validation to prevent it.

dblock

Looks legit, see my questions/requests below. Thanks.

dblock · 2018-08-06T16:33:13Z

CHANGELOG.md

@@ -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).


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

dblock · 2018-08-06T16:34:52Z

lib/grape/middleware/error.rb

+        valid_response?(response) ? response : error!('Internal Server Error(Invalid Response)')
+      end
+
+      def valid_response?(response)


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)?

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

dblock · 2018-08-06T16:35:16Z

lib/grape/middleware/error.rb

@@ -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)')


Should we re-raise the original error instead?

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.

dblock · 2018-08-08T01:29:38Z

Looking at this more carefully, do we really want this behavior?

The problem I see is that with this change we allow any kind of response from rescue_from and just swallow anything that doesn't look like something compatible with Rack. That seems too loose for an API framework.

The current documentation doesn't document what rescue_from should return. Maybe we want to instead document what that should be just like we document that anything returned from an API gets passed to a formatter and serialized?

dblock · 2018-08-08T01:30:00Z

Would love some second opinion, maybe @namusyaka?

darren987469 · 2018-08-08T14:21:09Z

The problem stated in #1757 is: if rescue_from returns an invalid response, then the final response will expose which library and which version the service is using. My current proposed solution catches invalid response and handles original error with default_rescue_handler. That may not be a good way to handle this problem, but I think the problem is important.

Indeed, add documentation to tell what should be returned in rescue_from can prevent developers from the problem. But in the worst case, the developer makes a mistake, we can take care of it.

Another way I have come up is: if rescue_from returns an invalid response, we still use default_rescue_handler, but the error is for an invalid response in rescue_from. Maybe call it Grape::Exceptions::InvalidRescueFromResponse. So the developer can know the mistake.

In summary, I think we should do two things:

Add documentation to tell what should be returned in rescue_from
Chose a way to handle developer mistake in rescue_from
1. Use default_rescue_handler to handle original error
2. Use default_rescue_handler to handle mistake in rescue_from
3. Any other idea?

dblock · 2018-08-08T18:26:35Z

I totally agree that this is an important problem. Lets fix it.

Does anyone actually expect to get a response from rescue_from? I bet most people think of it as a final exception handler.

Are we breaking any existing specs if we start ignoring the response from rescue_from altogether? If the answer is no, then we should just do that and document it. There's always a backward compatible way to return data (call error! like most examples, call body, etc.). Otherwise lets look at the specs and figure out whether they make sense and whether we want to break backwards compat.

darren987469 · 2018-08-09T16:01:40Z

@dblock I’m not sure if I get what you mean.

Are we breaking any existing specs if we start ignoring the response from rescue_from altogether? If the answer is no, then we should just do that and document it.

Do you mean if we start ignoring what returned from rescue_from? If so, I added a fail spec.

dblock · 2018-08-09T21:30:15Z

I think "yes". I am reading the spec and it says error! which means I don't expect the next line to execute, maybe for spec purposes you should return 'whatever' to demonstrate that it can be anything? What's the fix for the spec as written?

darren987469 · 2018-08-10T04:56:27Z

I add documentation to suggest developers call error! in the last line of rescue_from block. Developers call also return sting, array, hash, and Rack::Response or even nil in the rescue_from block. The returned object would be the error message in response. If developer return nil in the rescue_from block, default_message and default_status would be responded.

What do you think about this version?

dblock · 2018-08-10T12:06:57Z

This feels too complicated to me, I feel like your previous version may have been better.

Re-reading README we say The rescue_from block must return a Rack::Response object, call error! or re-raise an exception. So the original problem is when you don't do this you end up with a weird error, and my problem with that was the whole check for an array. What if enforced this exact spec by checking for is_a?(Rack::Response) instead for an array?

I totally understand that we have been going back and forth on this. I can try the above as an alternate PR if you're tired of me ;)

darren987469 · 2018-08-10T12:52:26Z

@dblock What is the behavior you image if the response from rescue_from is not a Rack::Response?

dblock · 2018-08-10T19:14:27Z

I think as is the code works well.

If the response inside a rescue handler is garbage I expect unexpected things to happen :)

What happens when you throw an exception from a rescue handler? That can always happen and I think we should explain the behavior and make it predictable in both README and specs.

Try to write an UPGRADING to catch any changes users may beed to do to handle the changes?

I'm worried (unnecessarily?) about those .finish being removed turning a Rack response into an array and what consequences that might have when grape is mounted in different places.

@namusyaka I could use some help with another pair of eyes here

darren987469 · 2018-08-11T14:39:53Z

Changes:

Add behavior description when raise exception in rescue_from
Add UPGRADING

@dblock I use version 1.1.1 as next version since this is a fix in a security issue. But this PR also break grape usage. What do you think?

For your concern about the removal of .finish in Rack response in Grape::Middleware::Error, I found Grape::Middleware::Base response return Rack::Response without .finishlink. Maybe it is the real object we expected in response?

dblock · 2018-08-11T18:44:33Z

Looks good. I'll edit the language in UPGRADING a bit, but for now lets just wait for another maintainer to take a look in case I missed something, hopefully soon.

namusyaka · 2018-08-26T14:36:50Z

Sorry for the late response.

@darren987469
Is there a merit to remove finish with this change?
Your referrenced block isn't actually used for returning HTTP response.

You removed .finish from rescue_from explicity. If the developer look at the upgrade.md, what do developers expect for?

In many cases, the change won't be no matter. I'm not saying that this change is bad.
However, as we are concerned about that, we should explain the removal of reason or concept.

darren987469 · 2018-08-27T14:33:37Z

@namusyaka Thanks for your response. Your are right, my referrenced block is not actually used for returning HTTP response. After digging more, I think it is ok to remove .finish method:

response = Rack::Response.new('body', 200)
status, body, headers = response

The code above announces a Rack::Response object. When doing assignment, it will trigger Rack::Response#finish method.

Since grape uses Rack::Head, Grape::Middleware::Error and Grape::Middleware::Formmatter in endpoint, the Rack::Head middleware will do the assignment and call finish for the response of Grape::Middleware::Error or Grape::Middleware::Formmatter. You can also find Grape::Middleware return a Rack::Response object without calling finish here.

Current documentation says we should return Rack::Response in rescue_from block, but also says we can return Rack::Response#finish here. That confuses developers. This change not only fix the mismatch, but also add a validation to prevent problem stated in #1757.

dblock · 2018-08-29T12:16:52Z

@darren987469 That makes a lot of sense, lets try to .finish this PR by removing all those .finish and seeing if specs still pass?

darren987469 · 2018-08-30T01:47:39Z

@dblock There is no more .finish call in grape. I wrote a spec to show .finish is called when doing assignment. Rack::Response#finish source code

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'rack'
  gem 'rspec'
end

require 'rack'
require 'rspec/autorun'

RSpec.describe Rack::Response do
  it 'calls to_ary when doing assignment' do
    response = Rack::Response.new('body', 200)

    expect(response).to receive(:to_ary)

    status, headers, body = response
  end

  it 'call finish when doing assignment' do
    # mock finish method
    Rack::Response.class_eval do
      def finish(&block)
        [1, 2, 3]
      end
      alias to_ary finish
    end

    response = Rack::Response.new('body', 200)

    status, headers, body = response
    expect(status).to eq 1
    expect(headers).to eq 2
    expect(body).to eq 3
  end
end

# Rack::Response
#    calls to_ary when doing assignment
#    call finish when doing assignment

# Finished in 0.00745 seconds (files took 0.10826 seconds to load)
# 2 examples, 0 failures

namusyaka

Looks good to me

dblock · 2018-08-30T13:21:11Z

Closed via c117bff. Thanks @darren987469!

darren987469 · 2018-08-30T13:45:14Z

@dblock @namusyaka Thank you, too!

darren987469 force-pushed the validates_response_processed_by_exception_handler branch from 52e9dea to f38213e Compare August 5, 2018 08:43

Validates response processed by exception handler

9e945a6

If block of return nil, the response would be nil. That causes system information leak. Add a validation to prevent it.

darren987469 force-pushed the validates_response_processed_by_exception_handler branch from f38213e to 9e945a6 Compare August 5, 2018 08:46

dblock requested changes Aug 6, 2018

View reviewed changes

darren987469 force-pushed the validates_response_processed_by_exception_handler branch 2 times, most recently from a1d7bee to bbb20ca Compare August 7, 2018 06:13

Use default_rescue_handler when reponse is invalid

429295f

darren987469 force-pushed the validates_response_processed_by_exception_handler branch from bbb20ca to 429295f Compare August 7, 2018 11:33

Add fail spec for ruby-grape#1757

3f95eb2

Takes object returned by rescue_from as error message

dfffd7f

Add rescue_from description in README

96d3138

darren987469 force-pushed the validates_response_processed_by_exception_handler branch from add9989 to 96d3138 Compare August 10, 2018 05:00

darren987469 added 2 commits August 10, 2018 22:55

Add InvalidResposne exception

c32c977

Use default_rescue_handler to handle invalid response

d2bc1b6

darren987469 added 2 commits August 11, 2018 21:33

Add raise exception behavior in rescue_from in README

641e746

Add upgrading

553bf07

namusyaka self-requested a review August 26, 2018 14:17

namusyaka approved these changes Aug 30, 2018

View reviewed changes

dblock closed this Aug 30, 2018

darren987469 deleted the validates_response_processed_by_exception_handler branch August 30, 2018 13:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validates response processed by exception handler #1776

Validates response processed by exception handler #1776

darren987469 commented Aug 5, 2018

dblock left a comment

dblock Aug 6, 2018

dblock Aug 6, 2018

darren987469 Aug 7, 2018 •

edited

Loading

dblock Aug 6, 2018

darren987469 Aug 7, 2018

dblock commented Aug 8, 2018

dblock commented Aug 8, 2018

darren987469 commented Aug 8, 2018

dblock commented Aug 8, 2018 •

edited

Loading

darren987469 commented Aug 9, 2018

dblock commented Aug 9, 2018

darren987469 commented Aug 10, 2018

dblock commented Aug 10, 2018

darren987469 commented Aug 10, 2018

dblock commented Aug 10, 2018 •

edited

Loading

darren987469 commented Aug 11, 2018

dblock commented Aug 11, 2018 •

edited

Loading

namusyaka commented Aug 26, 2018

darren987469 commented Aug 27, 2018

dblock commented Aug 29, 2018

darren987469 commented Aug 30, 2018

namusyaka left a comment

dblock commented Aug 30, 2018

darren987469 commented Aug 30, 2018

Validates response processed by exception handler #1776

Validates response processed by exception handler #1776

Conversation

darren987469 commented Aug 5, 2018

Problem

After PR

dblock left a comment

Choose a reason for hiding this comment

dblock Aug 6, 2018

Choose a reason for hiding this comment

dblock Aug 6, 2018

Choose a reason for hiding this comment

darren987469 Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

dblock Aug 6, 2018

Choose a reason for hiding this comment

darren987469 Aug 7, 2018

Choose a reason for hiding this comment

dblock commented Aug 8, 2018

dblock commented Aug 8, 2018

darren987469 commented Aug 8, 2018

dblock commented Aug 8, 2018 • edited Loading

darren987469 commented Aug 9, 2018

dblock commented Aug 9, 2018

darren987469 commented Aug 10, 2018

dblock commented Aug 10, 2018

darren987469 commented Aug 10, 2018

dblock commented Aug 10, 2018 • edited Loading

darren987469 commented Aug 11, 2018

dblock commented Aug 11, 2018 • edited Loading

namusyaka commented Aug 26, 2018

darren987469 commented Aug 27, 2018

dblock commented Aug 29, 2018

darren987469 commented Aug 30, 2018

namusyaka left a comment

Choose a reason for hiding this comment

dblock commented Aug 30, 2018

darren987469 commented Aug 30, 2018

darren987469 Aug 7, 2018 •

edited

Loading

dblock commented Aug 8, 2018 •

edited

Loading

dblock commented Aug 10, 2018 •

edited

Loading

dblock commented Aug 11, 2018 •

edited

Loading