diff --git a/CHANGES.rst b/CHANGES.rst index 0f0b5b76..f32148bf 100644 --- a/CHANGES.rst +++ b/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`_) diff --git a/src/flask/app.py b/src/flask/app.py index 6e0647e3..ccde3a6f 100644 --- a/src/flask/app.py +++ b/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 - occurs that is not caught. In debug mode the exception will - be re-raised immediately, otherwise it is logged and the handler - for a 500 internal server error is used. If no such handler - exists, a default 500 internal server error message is displayed. + """Handle an exception that did not have an error handler + associated with it, or that was raised from an error handler. + This always causes a 500 ``InternalServerError``. + + 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()) - if handler is None: - return InternalServerError() - return self.finalize_request(handler(e), from_error_handler=True) + server_error = InternalServerError() + # TODO: pass as param when Werkzeug>=1.0.0 is required + # TODO: also remove note about this from docstring and docs + 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` diff --git a/tests/test_user_error_handler.py b/tests/test_user_error_handler.py index f26826d2..51e99e43 100644 --- a/tests/test_user_error_handler.py +++ b/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"