From 8f8df53ff29938ace79b31097c27d9cdac803b44 Mon Sep 17 00:00:00 2001
From: Jordan Owens <jkowens@gmail.com>
Date: Tue, 19 Jan 2016 19:50:41 -0500
Subject: [PATCH] Add error handling for bad requests (invalid params)

Requests that raise an error when parameters are parsed will
now respond with a 400 status instead of 500.
---
 lib/sinatra/base.rb  | 27 ++++++++++++++++++++++-----
 test/helpers_test.rb | 17 +++++++++++++++++
 test/request_test.rb |  5 +++++
 test/routing_test.rb | 10 ++++++++++
 4 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
index 2f7a818d..5a9f4862 100644
--- a/lib/sinatra/base.rb
+++ b/lib/sinatra/base.rb
@@ -73,6 +73,12 @@ module Sinatra
       request_method == "UNLINK"
     end
 
+    def params
+      super
+    rescue Rack::Utils::ParameterTypeError, Rack::Utils::InvalidParameterError => e
+      raise BadRequest, "Invalid query parameters: #{e.message}"
+    end
+
     private
 
     class AcceptEntry
@@ -225,6 +231,10 @@ module Sinatra
     end
   end
 
+  class BadRequest < TypeError #:nodoc:
+    def http_status; 400 end
+  end
+
   class NotFound < NameError #:nodoc:
     def http_status; 404 end
   end
@@ -593,6 +603,11 @@ module Sinatra
       status.between? 500, 599
     end
 
+    # whether or not the status is set to 400
+    def bad_request?
+      status == 400
+    end
+
     # whether or not the status is set to 404
     def not_found?
       status == 404
@@ -897,9 +912,7 @@ module Sinatra
       @env      = env
       @request  = Request.new(env)
       @response = Response.new
-      @params   = indifferent_params(@request.params)
       template_cache.clear if settings.reload_templates
-      force_encoding(@params)
 
       @response['Content-Type'] = nil
       invoke { dispatch! }
@@ -1086,6 +1099,9 @@ module Sinatra
 
     # Dispatch a request with error handling.
     def dispatch!
+      @params = indifferent_params(@request.params)
+      force_encoding(@params)
+
       invoke do
         static! if settings.static? && (request.get? || request.head?)
         filter! :before
@@ -1121,11 +1137,12 @@ module Sinatra
       if server_error?
         dump_errors! boom if settings.dump_errors?
         raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler
-      end
-
-      if not_found?
+      elsif not_found?
         headers['X-Cascade'] = 'pass' if settings.x_cascade?
         body '<h1>Not Found</h1>'
+      elsif bad_request?
+        dump_errors! boom if settings.dump_errors?
+        halt status
       end
 
       res = error_block!(boom.class, boom) || error_block!(status, boom)
diff --git a/test/helpers_test.rb b/test/helpers_test.rb
index f56fe24c..1e51dffc 100644
--- a/test/helpers_test.rb
+++ b/test/helpers_test.rb
@@ -26,6 +26,23 @@ class HelpersTest < Minitest::Test
     end
   end
 
+  describe 'bad_request?' do
+    it 'is true for status == 400' do
+      status_app(400) { bad_request? }
+      assert_body 'true'
+    end
+
+    it 'is false for status gt 400' do
+      status_app(401) { bad_request? }
+      assert_body 'false'
+    end
+
+    it 'is false for status lt 400' do
+      status_app(399) { bad_request? }
+      assert_body 'false'
+    end
+  end
+
   describe 'not_found?' do
     it 'is true for status == 404' do
       status_app(404) { not_found? }
diff --git a/test/request_test.rb b/test/request_test.rb
index 4434589c..5c97b028 100644
--- a/test/request_test.rb
+++ b/test/request_test.rb
@@ -50,6 +50,11 @@ class RequestTest < Minitest::Test
     assert_equal({ 'compress' => '0.25' }, request.preferred_type.params)
   end
 
+  it "raises Sinatra::BadRequest when params contain conflicting types" do
+    request = Sinatra::Request.new 'QUERY_STRING' => 'foo=&foo[]='
+    assert_raises(Sinatra::BadRequest) { request.params }
+  end
+
   it "makes accept types behave like strings" do
     request = Sinatra::Request.new('HTTP_ACCEPT' => 'image/jpeg; compress=0.25')
     assert                     request.accept?('image/jpeg')
diff --git a/test/routing_test.rb b/test/routing_test.rb
index c595ca9a..8fab248d 100644
--- a/test/routing_test.rb
+++ b/test/routing_test.rb
@@ -47,6 +47,16 @@ class RoutingTest < Minitest::Test
     assert_equal '', response.body
   end
 
+  it "400s when request params contain conflicting types" do
+    mock_app {
+      get('/foo') { }
+    }
+
+    request = Rack::MockRequest.new(@app)
+    response = request.request('GET', '/foo?bar=&bar[]=', {})
+    assert response.bad_request?
+  end
+
   it "404s when no route satisfies the request" do
     mock_app {
       get('/foo') { }