Validate response from the exception handler. Closes ruby-grape#1757.

darren987469 · dblock · commit c117bff7d229 · 2018-08-30T15:20:38.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -7,6 +7,7 @@
 #### Fixes
 
 * Your contribution here.
+* [#1776](https://github.com/ruby-grape/grape/pull/1776): Validate response returned by the exception handler - [@darren987469](https://github.com/darren987469).
 
 ### 1.1.0 (8/4/2018)
 
diff --git a/README.md b/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,9 +2254,9 @@ 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` handler must return a `Rack::Response` object, call `error!`, or raise an exception (either the original exception or another custom one). The exception raised in `rescue_from` will be handled outside Grape. For example, if you mount Grape in Rails, the exception will be handle by [Rails Action Controller](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.
+Alternately, use the `with` option in `rescue_from` to specify a method or a `proc`.
 
 ```ruby
 class Twitter::API < Grape::API
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -1,6 +1,25 @@
 Upgrading Grape
 ===============
 
+### Upgrading to >= 1.1.1
+
+#### Changes in rescue_from returned object
+
+Grape will now check the object returned from `rescue_from` and ensure that it is a `Rack::Response`. That makes sure response is valid and avoids exposing service information. Change any code that invoked `Rack::Response.new(...).finish` in a custom `rescue_from` block 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) and [#1776](https://github.com/ruby-grape/grape/pull/1776) for more information.
+
 ### Upgrading to >= 1.1.0
 
 #### Changes in HTTP Response Code for Unsupported Content Type
diff --git a/lib/grape.rb b/lib/grape.rb
@@ -75,6 +75,7 @@ module Exceptions
     autoload :InvalidAcceptHeader
     autoload :InvalidVersionHeader
     autoload :MethodNotAllowed
+    autoload :InvalidResponse
   end
 
   module Extensions
diff --git a/lib/grape/exceptions/invalid_response.rb b/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
diff --git a/lib/grape/locale/en.yml b/lib/grape/locale/en.yml
@@ -49,4 +49,5 @@ en:
         invalid_version_header:
           problem: 'Invalid version header'
           resolution: '%{message}'
+        invalid_response: 'Invalid response'
 
diff --git a/lib/grape/middleware/error.rb b/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
diff --git a/spec/grape/api_spec.rb b/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
diff --git a/spec/grape/exceptions/invalid_response_spec.rb b/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
diff --git a/spec/grape/middleware/exception_spec.rb b/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
