Validates response processed by exception handler

[darren987469]

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.

[dblock]

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

[dblock]

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

[dblock]

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

[darren987469]

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]

Should we re-raise the original error instead?

[darren987469]

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]

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]

Would love some second opinion, maybe @namusyaka?

[darren987469]

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:

1. Add documentation to tell what should be returned in rescue_from
2. 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]

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]

@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]

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]

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]

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]

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

[dblock]

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]

Changes:

1. Add behavior description when raise exception in rescue_from
2. 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]

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]

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]

@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]

@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]

@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]

Closed via c117bff. Thanks @darren987469!

[darren987469]

@dblock @namusyaka Thank you, too!