Validates response processed by exception handler

CHANGELOG.md

@@ -7,6 +7,7 @@
 #### Fixes
 
 * Your contribution here.
+* [#1776](https://github.com/ruby-grape/grape/pull/1776): Validates response processed by exception handler - [@darren987469](https://github.com/darren987469).
 
 ### 1.1.0 (8/4/2018)
 

README.md

@@ -2183,7 +2183,7 @@ You can also rescue all exceptions with a code block and handle the Rack respons
 ```ruby
 class Twitter::API < Grape::API
   rescue_from :all do |e|
-    Rack::Response.new([ e.message ], 500, { 'Content-type' => 'text/error' }).finish
+    Rack::Response.new([ e.message ], 500, { 'Content-type' => 'text/error' })
   end
 end
 ```
@@ -2254,7 +2254,7 @@ class Twitter::API < Grape::API
 end
 ```
 
-The `rescue_from` block must return a `Rack::Response` object, call `error!` or re-raise an exception.
+The `rescue_from` block must return a `Rack::Response` object, or call `error!` or raise an exception, either original exception or another custom exception. The exception raised in `rescue_from` would be handled outside grape. For example, if you mount grape in Rails, the exception would be handle by [Rails](https://guides.rubyonrails.org/action_controller_overview.html#rescue).
 
 The `with` keyword is available as `rescue_from` options, it can be passed method name or Proc object.
 

UPGRADING.md

@@ -1,6 +1,25 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 1.1.1
+
+#### Changes in rescue_from returned object
+
+Grape add a validation to check object returned from `rescue_from` block is a Rack::Response. That makes sure response is valid and prevent exposing service information. If you return `Rack::Response.new(...).finish` in `rescue_from` block, change it to `Rack::Response.new(...)` to comply with the validation.
+
+```ruby
+class Twitter::API < Grape::API
+  rescue_from :all do |e|
+    # version prior to 1.1.1
+    Rack::Response.new([ e.message ], 500, { 'Content-type' => 'text/error' }).finish
+    # 1.1.1 version
+    Rack::Response.new([ e.message ], 500, { 'Content-type' => 'text/error' })
+  end
+end
+```
+
+See [#1757](https://github.com/ruby-grape/grape/pull/1757), [#1776](https://github.com/ruby-grape/grape/pull/1776) for more details.
+
 ### Upgrading to >= 1.1.0
 
 #### Changes in HTTP Response Code for Unsupported Content Type

lib/grape.rb

@@ -75,6 +75,7 @@ module Exceptions
     autoload :InvalidAcceptHeader
     autoload :InvalidVersionHeader
     autoload :MethodNotAllowed
+    autoload :InvalidResponse
   end
 
   module Extensions

lib/grape/exceptions/invalid_response.rb

@@ -0,0 +1,9 @@
+module Grape
+  module Exceptions
+    class InvalidResponse < Base
+      def initialize
+        super(message: compose_message(:invalid_response))
+      end
+    end
+  end
+end

lib/grape/locale/en.yml

@@ -49,4 +49,5 @@ en:
         invalid_version_header:
           problem: 'Invalid version header'
           resolution: '%{message}'
+        invalid_response: 'Invalid response'
 

lib/grape/middleware/error.rb

@@ -73,7 +73,7 @@ def rack_response(message, status = options[:default_status], headers = { Grape:
         if headers[Grape::Http::Headers::CONTENT_TYPE] == TEXT_HTML
           message = ERB::Util.html_escape(message)
         end
-        Rack::Response.new([message], status, headers).finish
+        Rack::Response.new([message], status, headers)
       end
 
       def format_message(message, backtrace, original_exception = nil)
@@ -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)
+
+        if response.is_a?(Rack::Response)
+          response
+        else
+          run_rescue_handler(:default_rescue_handler, Grape::Exceptions::InvalidResponse.new)
+        end
       end
     end
   end

spec/grape/api_spec.rb

@@ -1723,6 +1723,16 @@ class CustomError < Grape::Exceptions::Base; end
       expect(last_response.status).to eql 500
       expect(last_response.body).to eq('Formatter Error')
     end
+
+    it 'uses default_rescue_handler to handle invalid response from rescue_from' do
+      subject.rescue_from(:all) { 'error' }
+      subject.get('/') { raise }
+
+      expect_any_instance_of(Grape::Middleware::Error).to receive(:default_rescue_handler).and_call_original
+      get '/'
+      expect(last_response.status).to eql 500
+      expect(last_response.body).to eql 'Invalid response'
+    end
   end
 
   describe '.rescue_from klass, block' do

spec/grape/exceptions/invalid_response_spec.rb

@@ -0,0 +1,11 @@
+require 'spec_helper'
+
+describe Grape::Exceptions::InvalidResponse do
+  describe '#message' do
+    let(:error) { described_class.new }
+
+    it 'contains the problem in the message' do
+      expect(error.message).to include('Invalid response')
+    end
+  end
+end

spec/grape/middleware/exception_spec.rb

@@ -128,7 +128,7 @@ def app
       subject do
         Rack::Builder.app do
           use Spec::Support::EndpointFaker
-          use Grape::Middleware::Error, rescue_handlers: { NotImplementedError => -> { [200, {}, 'rescued'] } }
+          use Grape::Middleware::Error, rescue_handlers: { NotImplementedError => -> { Rack::Response.new('rescued', 200, {}) } }
           run ExceptionSpec::OtherExceptionApp
         end
       end