Internal Sinatra errors now extend Sinatra::Error

By extending Sinatra::Error, an error class can set the http status code on the response to a value other than 500. This commit fixes issues #1204 and #1518 where an error raised by a third party library that responded to http_status could set the status on the response. Any error outside of Sinatra errors will now always return a 500 status. This fixes an issue where an exception could leak sensitive data in the message to the browser. Errors that have http_status code 400 or 404 use the message as the body of the response. This is why it is imperative that these errors extend Sinatra::Error so that this is an explicit decision.
sinatra · Feb 2, 2022 · bda8c29 · bda8c29
1 parent 9a924aa
commit bda8c29
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 11 deletions.
diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
@@ -257,11 +257,14 @@ def call(env)
     end
   end
 
-  class BadRequest < TypeError #:nodoc:
+  class Error < StandardError #:nodoc:
+  end
+
+  class BadRequest < Error #:nodoc:
     def http_status; 400 end
   end
 
-  class NotFound < NameError #:nodoc:
+  class NotFound < Error #:nodoc:
     def http_status; 404 end
   end
 
@@ -1151,14 +1154,17 @@ def handle_exception!(boom)
       end
       @env['sinatra.error'] = boom
 
-      if boom.respond_to? :http_status and boom.http_status.between? 400, 599
-        status(boom.http_status)
-      elsif settings.use_code? and boom.respond_to? :code and boom.code.between? 400, 599
-        status(boom.code)
-      else
-        status(500)
+      http_status = if boom.kind_of? Sinatra::Error
+        if boom.respond_to? :http_status
+          boom.http_status
+        elsif settings.use_code? && boom.respond_to?(:code)
+          boom.code
+        end
       end
 
+      http_status = 500 unless http_status && http_status.between?(400, 599)
+      status(http_status)
+
       if server_error?
         dump_errors! boom if settings.dump_errors?
         raise boom if settings.show_exceptions? and settings.show_exceptions != :after_handler

diff --git a/test/mapped_error_test.rb b/test/mapped_error_test.rb
@@ -6,15 +6,15 @@ class FooError < RuntimeError
 class FooNotFound < Sinatra::NotFound
 end
 
-class FooSpecialError < RuntimeError
+class FooSpecialError < Sinatra::Error
   def http_status; 501 end
 end
 
-class FooStatusOutOfRangeError < RuntimeError
+class FooStatusOutOfRangeError < Sinatra::Error
   def code; 4000 end
 end
 
-class FooWithCode < RuntimeError
+class FooWithCode < Sinatra::Error
   def code; 419 end
 end
 

diff --git a/test/result_test.rb b/test/result_test.rb
@@ -1,5 +1,9 @@
 require File.expand_path('../helper', __FILE__)
 
+class ThirdPartyError < RuntimeError
+  def http_status; 400 end
+end
+
 class ResultTest < Minitest::Test
   it "sets response.body when result is a String" do
     mock_app { get('/') { 'Hello World' } }
@@ -73,4 +77,15 @@ def res.each ; yield call ; end
     assert_equal 205, status
     assert_equal '', body
   end
+
+  it "sets status to 500 when raised error is not Sinatra::Error" do
+    mock_app do
+      set :raise_errors, false
+      get('/') { raise ThirdPartyError }
+    end
+
+    get '/'
+    assert_equal 500, status
+    assert_equal '<h1>Internal Server Error</h1>', body
+  end
 end