always pass InternalServerError instance to 500 handler

CHANGES.rst

@@ -11,6 +11,17 @@ Unreleased
 
 -   Bump minimum Werkzeug version to >= 0.15.
 -   Drop support for Python 3.4.
+-   Error handlers for ``InternalServerError`` or ``500`` will always be
+    passed an instance of ``InternalServerError``. If they are invoked
+    due to an unhandled exception, that original exception is now
+    available as ``e.original_exception`` rather than being passed
+    directly to the handler. The same is true if the handler is for the
+    base ``HTTPException``.This makes error handler behavior more
+    consistent. :pr:`3266`
+
+    -   :meth:`Flask.finalize_request` is called for all unhandled
+        exceptions even if there is no ``500`` error handler.
+
 -   :meth:`flask.RequestContext.copy` includes the current session
     object in the request context copy. This prevents ``session``
     pointing to an out-of-date object. (`#2935`_)

src/flask/app.py

@@ -1811,11 +1811,36 @@ class Flask(_PackageBoundObject):
         return handler(e)
 
     def handle_exception(self, e):
-        """Default exception handling that kicks in when an exception
+        """Handle an exception that did not have an error handler
-        occurs that is not caught.  In debug mode the exception will
+        associated with it, or that was raised from an error handler.
-        be re-raised immediately, otherwise it is logged and the handler
+        This always causes a 500 ``InternalServerError``.
-        for a 500 internal server error is used.  If no such handler
+
-        exists, a default 500 internal server error message is displayed.
+        Always sends the :data:`got_request_exception` signal.
+
+        If :attr:`propagate_exceptions` is ``True``, such as in debug
+        mode, the error will be re-raised so that the debugger can
+        display it. Otherwise, the original exception is logged, and
+        an :exc:`~werkzeug.exceptions.InternalServerError` is returned.
+
+        If an error handler is registered for ``InternalServerError`` or
+        ``500``, it will be used. For consistency, the handler will
+        always receive the ``InternalServerError``. The original
+        unhandled exception is available as ``e.original_exception``.
+
+        .. note::
+            Prior to Werkzeug 1.0.0, ``InternalServerError`` will not
+            always have a ``original_exception`` attribute. Use
+            ``getattr(e, "original_exception", None)`` to simulate the
+            behavior for compatibility.
+
+        .. versionchanged:: 1.1.0
+            Always passes the ``InternalServerError`` instance to the
+            handler, setting ``original_exception`` to the unhandled
+            error.
+
+        .. versionchanged:: 1.1.0
+            ``after_request`` functions and other finalization is done
+            even for the default 500 response when there is no handler.
 
         .. versionadded:: 0.3
         """
@@ -1833,10 +1858,16 @@ class Flask(_PackageBoundObject):
                 raise e
 
         self.log_exception((exc_type, exc_value, tb))
-        handler = self._find_error_handler(InternalServerError())
+        server_error = InternalServerError()
-        if handler is None:
+        # TODO: pass as param when Werkzeug>=1.0.0 is required
-            return InternalServerError()
+        # TODO: also remove note about this from docstring and docs
-        return self.finalize_request(handler(e), from_error_handler=True)
+        server_error.original_exception = e
+        handler = self._find_error_handler(server_error)
+
+        if handler is not None:
+            server_error = handler(server_error)
+
+        return self.finalize_request(server_error, from_error_handler=True)
 
     def log_exception(self, exc_info):
         """Logs an exception.  This is called by :meth:`handle_exception`

tests/test_user_error_handler.py

@@ -6,6 +6,7 @@ tests.test_user_error_handler
 :copyright: © 2010 by the Pallets team.
 :license: BSD, see LICENSE for more details.
 """
+import pytest
 from werkzeug.exceptions import Forbidden
 from werkzeug.exceptions import HTTPException
 from werkzeug.exceptions import InternalServerError
@@ -25,7 +26,13 @@ def test_error_handler_no_match(app, client):
 
     @app.errorhandler(500)
     def handle_500(e):
-        return type(e).__name__
+        assert isinstance(e, InternalServerError)
+        original = getattr(e, "original_exception", None)
+
+        if original is not None:
+            return "wrapped " + type(original).__name__
+
+        return "direct"
 
     @app.route("/custom")
     def custom_test():
@@ -35,9 +42,14 @@ def test_error_handler_no_match(app, client):
     def key_error():
         raise KeyError()
 
+    @app.route("/abort")
+    def do_abort():
+        flask.abort(500)
+
     app.testing = False
     assert client.get("/custom").data == b"custom"
-    assert client.get("/keyerror").data == b"KeyError"
+    assert client.get("/keyerror").data == b"wrapped KeyError"
+    assert client.get("/abort").data == b"direct"
 
 
 def test_error_handler_subclass(app):
@@ -194,3 +206,84 @@ def test_default_error_handler():
     assert c.get("/forbidden").data == b"forbidden"
     # Don't handle RequestRedirect raised when adding slash.
     assert c.get("/slash", follow_redirects=True).data == b"slash"
+
+
+class TestGenericHandlers(object):
+    """Test how very generic handlers are dispatched to."""
+
+    class Custom(Exception):
+        pass
+
+    @pytest.fixture()
+    def app(self, app):
+        @app.route("/custom")
+        def do_custom():
+            raise self.Custom()
+
+        @app.route("/error")
+        def do_error():
+            raise KeyError()
+
+        @app.route("/abort")
+        def do_abort():
+            flask.abort(500)
+
+        @app.route("/raise")
+        def do_raise():
+            raise InternalServerError()
+
+        app.config["PROPAGATE_EXCEPTIONS"] = False
+        return app
+
+    def report_error(self, e):
+        original = getattr(e, "original_exception", None)
+
+        if original is not None:
+            return "wrapped " + type(original).__name__
+
+        return "direct " + type(e).__name__
+
+    @pytest.mark.parametrize("to_handle", (InternalServerError, 500))
+    def test_handle_class_or_code(self, app, client, to_handle):
+        """``InternalServerError`` and ``500`` are aliases, they should
+        have the same behavior. Both should only receive
+        ``InternalServerError``, which might wrap another error.
+        """
+
+        @app.errorhandler(to_handle)
+        def handle_500(e):
+            assert isinstance(e, InternalServerError)
+            return self.report_error(e)
+
+        assert client.get("/custom").data == b"wrapped Custom"
+        assert client.get("/error").data == b"wrapped KeyError"
+        assert client.get("/abort").data == b"direct InternalServerError"
+        assert client.get("/raise").data == b"direct InternalServerError"
+
+    def test_handle_generic_http(self, app, client):
+        """``HTTPException`` should only receive ``HTTPException``
+        subclasses. It will receive ``404`` routing exceptions.
+        """
+
+        @app.errorhandler(HTTPException)
+        def handle_http(e):
+            assert isinstance(e, HTTPException)
+            return str(e.code)
+
+        assert client.get("/error").data == b"500"
+        assert client.get("/abort").data == b"500"
+        assert client.get("/not-found").data == b"404"
+
+    def test_handle_generic(self, app, client):
+        """Generic ``Exception`` will handle all exceptions directly,
+        including ``HTTPExceptions``.
+        """
+
+        @app.errorhandler(Exception)
+        def handle_exception(e):
+            return self.report_error(e)
+
+        assert client.get("/custom").data == b"direct Custom"
+        assert client.get("/error").data == b"direct KeyError"
+        assert client.get("/abort").data == b"direct InternalServerError"
+        assert client.get("/not-found").data == b"direct NotFound"