Merge pull request #316 from ajvondrak/prevent-params-from-swallowing…

…-errors

Prevent Goliath::Rack::Params from swallowing downstream errors

lib/goliath/rack/params.rb

@@ -60,10 +60,11 @@ def initialize(app)
       end
 
       def call(env)
-        Goliath::Rack::Validator.safely(env) do
+        error_response = Goliath::Rack::Validator.safely(env) do
           env['params'] = retrieve_params(env)
-          @app.call(env)
+          nil
         end
+        error_response || @app.call(env)
       end
     end
   end

spec/integration/exception_handling_spec.rb

@@ -0,0 +1,202 @@
+# This integration spec isn't *quite* about exception handling. It's more of a
+# regression test, since Goliath::Rack::Params used to swallow any exception
+# generated downstream from it in the middleware chain (even though
+# Goliath::API#call handles exceptions already).
+
+require 'spec_helper'
+
+class ExceptionHandlingMiddleware
+  include Goliath::Rack::AsyncMiddleware
+  include Goliath::Constants
+
+  def call(env)
+    # This kind of relies on downstream middleware raising their own exceptions
+    # if something goes wrong. Alternatively, they could rescue the exception
+    # and set env[RACK_EXCEPTION]. See #post_process.
+    super
+  rescue Exception => e
+    handle_exception(e)
+  end
+
+  def post_process(env, status, headers, body)
+    # If an exception was raised by Goliath::API#call, it will store the
+    # exception in env[RACK_EXCEPTION] and automatically generate an error
+    # response. We circumvent the usual error response by returning our own.
+    return handle_exception(env[RACK_EXCEPTION]) if env[RACK_EXCEPTION]
+    [status, headers, body]
+  end
+
+  private
+
+  def handle_exception(e)
+    # You could imagine this middleware having some sort of side effect, like
+    # sending your team an email alert detailing the exception. For the
+    # purposes of the spec, we'll just return some text.
+    [200, {}, "Exception raised: #{e.message}"]
+  end
+end
+
+class ExceptionRaisingMiddleware
+  def initialize(app)
+    @app = app
+  end
+
+  def call(env)
+    raise env.params['raise'] if env.params['raise']
+    @app.call(env)
+  end
+end
+
+class ExceptionHandlingAPI < Goliath::API
+  # The exception handling middleware should come first in the chain so that
+  # any exception in the rest of the chain gets rescued.
+  use ExceptionHandlingMiddleware
+
+  # Require param parsing before the exception raising middleware, because the
+  # latter depends on params being parsed. Herein lies the regression:
+  # Goliath::Rack::Params used to swallow any exceptions raised downstream from
+  # it (here, ExceptionRaisingMiddleware), so any upstream middleware (here,
+  # ExceptionHandlingMiddleware) would be none the wiser. Really, the only
+  # thing Goliath::Rack::Params should be worried about is whether it can parse
+  # the params.
+  use Goliath::Rack::Params
+
+  # Allow us to raise an exception on demand with the param raise=<message>.
+  # You could imagine something less dumb for a real-life application that
+  # incidentally raises an exception, if you'd like.
+  use ExceptionRaisingMiddleware
+
+  def response(env)
+    # Goliath::API#call ensures that any exceptions raised here get rescued and
+    # stored in env[RACK_EXCEPTION].
+    fail env.params['fail'] if env.params['fail']
+    [200, {}, 'No exceptions raised']
+  end
+end
+
+describe ExceptionHandlingAPI do
+  let(:err) { Proc.new { fail 'API request failed' } }
+
+  context 'when no exceptions get raised' do
+    let(:query) { '' }
+
+    it 'returns a normal response' do
+      with_api(ExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 200
+          c.response.should == 'No exceptions raised'
+        end
+      end
+    end
+  end
+
+  context 'when a middleware raises an exception' do
+    let(:query) { 'raise=zoinks' }
+
+    it 'handles the exception using ExceptionHandlingMiddleware' do
+      with_api(ExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 200
+          c.response.should == 'Exception raised: zoinks'
+        end
+      end
+    end
+  end
+
+  context 'when the API raises an exception' do
+    let(:query) { 'fail=jinkies' }
+
+    it 'handles the exception using ExceptionHandlingMiddleware' do
+      with_api(ExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 200
+          c.response.should == 'Exception raised: jinkies'
+        end
+      end
+    end
+  end
+
+  context 'when param parsing raises an exception' do
+    let(:query) { 'ambiguous[]=&ambiguous[4]=' }
+
+    it 'returns a validation error generated by Goliath::Rack::Params' do
+      with_api(ExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 400
+          c.response.should == "[:error, \"Invalid parameters: Rack::Utils::ParameterTypeError\"]"
+        end
+      end
+    end
+  end
+end
+
+# This API has the same setup as ExceptionHandlingAPI, except for the crucial
+# difference that it does *not* use ExceptionHandlingMiddleware. This is to
+# make sure that exceptions are still rescued somewhere in an API call, even
+# though they aren't getting swallowed by Goliath::Rack::Params anymore.
+
+class PassiveExceptionHandlingAPI < Goliath::API
+  use Goliath::Rack::Params
+  use ExceptionRaisingMiddleware
+
+  def response(env)
+    fail env.params['fail'] if env.params['fail']
+    [200, {}, 'No exceptions raised']
+  end
+end
+
+describe PassiveExceptionHandlingAPI do
+  let(:err) { Proc.new { fail 'API request failed' } }
+
+  context 'when no exceptions get raised' do
+    let(:query) { '' }
+
+    it 'returns a normal response' do
+      with_api(PassiveExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 200
+          c.response.should == 'No exceptions raised'
+        end
+      end
+    end
+  end
+
+  context 'when a middleware raises an exception' do
+    let(:query) { 'raise=ruh-roh' }
+
+    it 'returns the server error generated by Goliath::Request#process' do
+      with_api(PassiveExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 500
+          c.response.should == 'ruh-roh'
+        end
+      end
+    end
+  end
+
+  context 'when the API raises an exception' do
+    let(:query) { 'fail=puppy-power' }
+
+    it 'returns the validation error generated by Goliath::API#call' do
+      with_api(PassiveExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 500
+          c.response.should == '[:error, "puppy-power"]'
+        end
+      end
+    end
+  end
+
+  context 'when param parsing raises an exception' do
+    let(:query) { 'ambiguous[]=&ambiguous[4]=' }
+
+    it 'returns a validation error generated by Goliath::Rack::Params' do
+      with_api(PassiveExceptionHandlingAPI) do
+        get_request({ query: query }, err) do |c|
+          c.response_header.status.should == 400
+          c.response.should == '[:error, "Invalid parameters: Rack::Utils::ParameterTypeError"]'
+        end
+      end
+    end
+  end
+end

spec/unit/rack/params_spec.rb

@@ -21,6 +21,13 @@
       }.to raise_error(Goliath::Validation::BadRequestError)
     end
 
+    it 'handles ambiguous query strings' do
+      @env['QUERY_STRING'] = 'ambiguous[]=&ambiguous[4]='
+      expect {
+        @params.retrieve_params(@env)
+      }.to raise_error(Goliath::Validation::Error)
+    end
+
     it 'parses the query string' do
       @env['QUERY_STRING'] = 'foo=bar&baz=bonkey'
 
@@ -128,6 +135,37 @@
       body.should == app_body
     end
 
+    it 'returns a validation error if one is raised while parsing' do
+      expect(@app).to_not receive(:call)
+      params_exception = Goliath::Validation::Error.new(423, 'invalid')
+      expect(@params).to receive(:retrieve_params).and_raise(params_exception)
+      status, headers, body = @params.call(@env)
+      expect(status).to eq 423
+      expect(headers).to eq({})
+      expect(body).to eq(error: 'invalid')
+    end
+
+    it 'returns a 500 error if an unexpected error is raised while parsing' do
+      expect(@app).to_not receive(:call)
+
+      params_exception = Exception.new('uh oh')
+      expect(@params).to receive(:retrieve_params).and_raise(params_exception)
+
+      logger = double('logger').as_null_object
+      expect(@env).to receive(:logger).twice.and_return(logger)
+
+      status, headers, body = @params.call(@env)
+      expect(status).to eq 500
+      expect(headers).to eq({})
+      expect(body).to eq(error: 'uh oh')
+    end
+
+    it 'does not swallow exceptions from the app' do
+      app_exception = Class.new(Exception)
+      expect(@app).to receive(:call).and_raise(app_exception)
+      expect { @params.call(@env) }.to raise_error(app_exception)
+    end
+
     context 'content type' do
       it "parses application/x-www-form-urlencoded" do
         @env['CONTENT_TYPE'] = 'application/x-www-form-urlencoded; charset=utf-8'