Add bad_request error handler

Requests that raise an error when parameters are parsed will now respond with a 400 status instead of 500.
sinatra · Jan 21, 2016 · a297e70 · a297e70
1 parent b14df4a
commit a297e70
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 5 deletions.
diff --git a/README.md b/README.md
@@ -93,6 +93,7 @@ pick up if available.
         * [Available Settings](#available-settings)
     * [Environments](#environments)
     * [Error Handling](#error-handling)
+        * [Bad Request](#bad-request)
         * [Not Found](#not-found)
         * [Error](#error)
     * [Rack Middleware](#rack-middleware)
@@ -2295,6 +2296,17 @@ Error handlers run within the same context as routes and before filters, which
 means you get all the goodies it has to offer, like `haml`,
 `erb`, `halt`, etc.
 
+### Bad Request
+
+When a `Sinatra::BadRequest` exception is raised, or the response's status
+code is 400, the `bad_request` handler is invoked:
+
+```ruby
+bad_request do
+  'This is a bad request.'
+end
+```
+
 ### Not Found
 
 When a `Sinatra::NotFound` exception is raised, or the response's status

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
@@ -69,6 +69,12 @@ def unlink?
       request_method == "UNLINK"
     end
 
+    def params
+      super
+    rescue Exception => e
+      raise BadRequest, e.message
+    end
+
     private
 
     class AcceptEntry
@@ -221,6 +227,10 @@ def call(env)
     end
   end
 
+  class BadRequest < NameError #:nodoc:
+    def http_status; 400 end
+  end
+
   class NotFound < NameError #:nodoc:
     def http_status; 404 end
   end
@@ -580,6 +590,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
@@ -892,11 +907,10 @@ def call(env)
 
     def call!(env) # :nodoc:
       @env      = env
+      @params   = {}
       @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! }
@@ -1074,6 +1088,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
@@ -1108,9 +1125,9 @@ def handle_exception!(boom)
         raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler
       end
 
-      if not_found?
+      if bad_request? || not_found?
         headers['X-Cascade'] = 'pass' if settings.x_cascade?
-        body '<h1>Not Found</h1>'
+        body "<h1>#{HTTP_STATUS_CODES[status]}</h1>"
       end
 
       res = error_block!(boom.class, boom) || error_block!(status, boom)
@@ -1253,6 +1270,12 @@ def error(*codes, &block)
         codes.each { |c| (@errors[c] ||= []) << args }
       end
 
+      # Sugar for `error(400) { ... }`
+      def bad_request(&block)
+        error(400, &block)
+        error(Sinatra::BadRequest, &block)
+      end
+
       # Sugar for `error(404) { ... }`
       def not_found(&block)
         error(404, &block)
@@ -1978,7 +2001,7 @@ def self.delegate(*methods)
     end
 
     delegate :get, :patch, :put, :post, :delete, :head, :options, :link, :unlink,
-             :template, :layout, :before, :after, :error, :not_found, :configure,
+             :template, :layout, :before, :after, :error, :bad_request, :not_found, :configure,
              :set, :mime_type, :enable, :disable, :use, :development?, :test?,
              :production?, :helpers, :settings, :register
 

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,16 @@ 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 "raises Sinatra::BadRequest when request param contains an invalid byte" do
+    request = Sinatra::Request.new 'QUERY_STRING' => 'foo%81=1'
+    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
@@ -53,6 +53,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') { }