Validation around error handler responses

[dcrodman]

Hi, I ran into an interesting issue the other day when debugging an ugly response from Grape when sending a request with missing parameters to one of our endpoints.

Here's a stripped down version of the V1::Endpoints::Base definition:

module V1::Endpoints::Base
  included do
    format :json

    rescue_from :all do |e|
      error! V1::Entities::Error.represent(e), 400
      Rails.logger.error "[API] -- #{e.message}\n #{e.backtrace.join("\n")}"
      Bugsnag.notify(e) if Rails.env.production?
    end
end

You'll notice that the error! call was mistakenly placed on the first line instead of the last line and since return was omitted, this method will return the result of Bugsnag#notify, which is nil. This is obviously a bug on our end, but it causes the request to fail with a 500 and returns this full stack trace to the requestor:

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'
grape (1.0.2) lib/grape/router.rb:137:in `with_optimization'
grape (1.0.2) lib/grape/router.rb:56:in `call'
grape (1.0.2) lib/grape/api.rb:119:in `call'
grape (1.0.2) lib/grape/api.rb:45:in `call!'
grape (1.0.2) lib/grape/api.rb:40:in `call'
actionpack (5.1.6) lib/action_dispatch/routing/mapper.rb:17:in `block in <class:Constraints>'
actionpack (5.1.6) lib/action_dispatch/routing/mapper.rb:46:in `serve'
actionpack (5.1.6) lib/action_dispatch/journey/router.rb:50:in `block in serve'
actionpack (5.1.6) lib/action_dispatch/journey/router.rb:33:in `each'
actionpack (5.1.6) lib/action_dispatch/journey/router.rb:33:in `serve'
actionpack (5.1.6) lib/action_dispatch/routing/route_set.rb:844:in `call'
rack (2.0.4) lib/rack/etag.rb:25:in `call'
rack (2.0.4) lib/rack/conditional_get.rb:38:in `call'
rack (2.0.4) lib/rack/head.rb:12:in `call'
activerecord (5.1.6) lib/active_record/migration.rb:556:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/callbacks.rb:26:in `block in call'
activesupport (5.1.6) lib/active_support/callbacks.rb:97:in `run_callbacks'
actionpack (5.1.6) lib/action_dispatch/middleware/callbacks.rb:24:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/executor.rb:12:in `call'
bugsnag (6.7.0) lib/bugsnag/integrations/rack.rb:46:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/debug_exceptions.rb:59:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/show_exceptions.rb:31:in `call'
railties (5.1.6) lib/rails/rack/logger.rb:36:in `call_app'
railties (5.1.6) lib/rails/rack/logger.rb:24:in `block in call'
activesupport (5.1.6) lib/active_support/tagged_logging.rb:69:in `block in tagged'
activesupport (5.1.6) lib/active_support/tagged_logging.rb:26:in `tagged'
activesupport (5.1.6) lib/active_support/tagged_logging.rb:69:in `tagged'
railties (5.1.6) lib/rails/rack/logger.rb:24:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/remote_ip.rb:79:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/request_id.rb:25:in `call'
rack (2.0.4) lib/rack/runtime.rb:22:in `call'
activesupport (5.1.6) lib/active_support/cache/strategy/local_cache_middleware.rb:27:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/executor.rb:12:in `call'
actionpack (5.1.6) lib/action_dispatch/middleware/static.rb:125:in `call'
rack (2.0.4) lib/rack/sendfile.rb:111:in `call'
railties (5.1.6) lib/rails/engine.rb:522:in `call'

IMO, there should be some validation of the return values of exception handlers when they're called by Grape::Middleware::Error#exec_handler to make sure the result is an error response since the status, headers, and body are passed directly back up to Rack. At best this is a fairly baffling message for a simple error, at worst it's a security vulnerability since this exposes which libraries a service is using along with their exact versions.

Please let me know if there's any other information I can provide.

[dblock]

Seems reasonable, feel free to write a test/PR for this.

[dblock]

Fixed in c117bff.