From bda8c29d70619d53f5b1c181140638d340695514 Mon Sep 17 00:00:00 2001
From: Jordan Owens <jkowens@gmail.com>
Date: Thu, 31 Jan 2019 22:32:45 -0500
Subject: [PATCH] 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.
---
 lib/sinatra/base.rb       | 22 ++++++++++++++--------
 test/mapped_error_test.rb |  6 +++---
 test/result_test.rb       | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
index fd3ab4bc..827a778a 100644
--- a/lib/sinatra/base.rb
+++ b/lib/sinatra/base.rb
@@ -257,11 +257,14 @@ module Sinatra
     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 @@ module Sinatra
       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
index cb158a26..562e509d 100644
--- a/test/mapped_error_test.rb
+++ b/test/mapped_error_test.rb
@@ -6,15 +6,15 @@ end
 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
index cbb78131..67d163fc 100644
--- 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 @@ class ResultTest < Minitest::Test
     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