Merge pull request #1070 from jkowens/fix-1058

Add error handling for requests with invalid params (Fixes #1058)
sinatra · May 9, 2016 · 939ce04 · 939ce04
2 parents bb40ef6 + 8f8df53
commit 939ce04
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 5 deletions.
diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
@@ -73,6 +73,12 @@ def unlink?
       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 @@ def call(env)
     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 @@ def server_error?
       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
@@ -900,9 +915,7 @@ def call!(env) # :nodoc:
       @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! }
@@ -1089,6 +1102,9 @@ def invoke
 
     # 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
@@ -1124,11 +1140,12 @@ def handle_exception!(boom)
       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
@@ -26,6 +26,23 @@ def status_app(code, &block)
     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
@@ -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
@@ -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') { }