Repository: falconry/falcon
Pull request #1527 (2019-05-06 21:40:13)

fix(API): return 500 response to server for uncaught exceptions
Per newly-understood WSGI spec, applications should trap internal errors instead of passing them on to the server. Add a default exception handler for the base Python ``Exception``, which is placed first in the error handler list (and is thus evaluated last, per LIFO operation).

The 500 error response sends no specific error information back to the client, but it does log a traceback to ``wsgi.errors``. This logging behavior currently has no tests.

Update error-handling documentation to reflect new default behavior. No "recipe" is included as yet -- need more details on requirements here.

Currently, errors are serialized in JSON or XML, depending on the Accept header in the request. It defaults to JSON if the request does not prefer XML, even if the app's default media type is XML. Changing this behavior appears to be non-trivial, so it is not attempted here, though it was discussed during development.

Addresses https://github.com/falconry/falcon/issues/1507

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=h1) Report
> Merging [#1527](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/a95e4663ddae0555e3ebf60f639146232e63347e?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1527/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=tree)

```diff
@@          Coverage Diff           @@
##           master   #1527   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          38      38           
  Lines        2561    2561           
  Branches      391     391           
======================================
  Hits         2561    2561
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=footer). Last update [a95e466...f078b2e](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=h1) Report
> Merging [#1527](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/a95e4663ddae0555e3ebf60f639146232e63347e?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1527/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=tree)

```diff
@@          Coverage Diff           @@
##           master   #1527   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          38      38           
  Lines        2561    2561           
  Branches      391     391           
======================================
  Hits         2561    2561
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=footer). Last update [a95e466...f078b2e](https://codecov.io/gh/falconry/falcon/pull/1527?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

csojinb: So, the hug tests are failing on this. Do I need to make a PR to there before this can be merged?
csojinb: Okay, so a brief scan of the test output suggests that a number of hug tests are depending on the behavior where falcon passes out I caught errors. 

I think that I could make a PR to hug that would override the now-default (in this PR) behavior to catch those exceptions for the purposes of their tests. (Hopefully I can do that with minimal disruption.) However, that does create the annoying situation where the default HTTPError and HTTPStatus handlers are also overridden, which is just kinda gross.

So, I’m thinking maybe it would make sense to resolve https://github.com/falconry/falcon/issues/1514 first and then come back to this PR? I looked into how werkzeug does it, and it’s super-simple, so I think I could change it pretty quickly. Catching all of the references to LIFO order in the docs seems like the hardest part :joy:. That issue is still in “Needs Decision” status, but I think it would be a significant usability win (assuming there are no objections to storing the error handlers in a dict).
kgriffs: Perhaps we should disable the hug test temporarily and open an issue over there? We can re-enable the hug tests in 3.1 once all of Falcon's breaking changes have been addressed. @timothycrosley what would you suggest?
timothycrosley: @kgriffs I think that sounds good! Once Falcon approaches a release candidate for the next version I'll start working on resolving any breaking issues that arise. Once I feel like hug is in good shape to support the new version I can make a pull request to uncomment the tests.
vytas7: Thanks for input @timothycrosley !
And :+1: from me for this path, we are probably going to hit more breaking changes on the road to 3.0 anyway.

Bandwidth permitting, I will try to submit a Hug PR if the changes are trivial (as I did for Falcon 2.0), otherwise we are going to need your help @timothycrosley :slightly_smiling_face: 
kgriffs: OK, sounds like a plan. I will disable the hug test for the moment.
kgriffs: Depends on #1599 
csojinb: Okay @vytas7, I modified the documentation to be more technically correct re: default error-handling. If it all looks good to you, then I'm happy with it.

Patch:
diff --git a/docs/api/errors.rst b/docs/api/errors.rst
index f0131e5..88f2e2e 100644
--- a/docs/api/errors.rst
+++ b/docs/api/errors.rst
@@ -66,6 +66,10 @@ at the global level by registering one or more custom error handlers.
 See also :meth:`~.API.add_error_handler` to learn more about this
 feature.
 
+.. note::
+    By default, any uncaught exceptions will return an HTTP 500 response and
+    log details of the exception to ``wsgi.errors``.
+
 Base Class
 ----------
 
diff --git a/docs/user/faq.rst b/docs/user/faq.rst
index 24b02c4..890b2e8 100644
--- a/docs/user/faq.rst
+++ b/docs/user/faq.rst
@@ -446,9 +446,15 @@ for this exception type. Alternatively, you may be able to configure your web
 server to transform the response for you (e.g., using Nginx's ``error_page``
 directive).
 
-500 errors are typically the result of an unhandled exception making its way
-up to the web server. To handle these errors more gracefully, you can add a
-custom error handler for Python's base :class:`Exception` type.
+By default, non-system-exiting exceptions that do not inherit from
+:class:`~.HTTPError` or :class:`~.HTTPStatus` are handled by Falcon with a
+plain HTTP 500 error. To provide your own 500 logic, you can add a custom error
+handler for Python's base :class:`Exception` type, though be aware that doing
+so will also override the default handlers for :class:`~.HTTPError` and
+:class:`~.HTTPStatus`.
+
+See :ref:`errors` and the :meth:`falcon.API.add_error_handler` docs for more
+details.
 
 Request Handling
 ~~~~~~~~~~~~~~~~
diff --git a/falcon/api.py b/falcon/api.py
index 2329cc5..59bd1ab 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -16,6 +16,7 @@
 
 from functools import wraps
 import re
+import traceback
 
 from falcon import api_helpers as helpers, DEFAULT_MEDIA_TYPE, routing
 from falcon.http_error import HTTPError
@@ -210,6 +211,7 @@ class API:
         self.resp_options.default_media_type = media_type
 
         # NOTE(kgriffs): Add default error handlers
+        self.add_error_handler(Exception, self._python_error_handler)
         self.add_error_handler(falcon.HTTPError, self._http_error_handler)
         self.add_error_handler(falcon.HTTPStatus, self._http_status_handler)
 
@@ -536,14 +538,25 @@ class API:
         will choose the one that was most recently registered.
         Therefore, more general error handlers (e.g., for the
         standard ``Exception`` type) should be added first, to avoid
-        masking more specific handlers for subclassed types.
+        masking more specific handlers for subclassed types. For example::
+
+            app = falcon.API()
+            app.add_error_handler(Exception, custom_handle_uncaught_exception)
+            app.add_error_handler(falcon.HTTPError, custom_handle_http_error)
+            app.add_error_handler(CustomException)
 
         .. Note::
 
-            By default, the framework installs two handlers, one for
-            :class:`~.HTTPError` and one for :class:`~.HTTPStatus`. These can
-            be overridden by adding a custom error handler method for the
-            exception type in question.
+            By default, the framework installs three handlers, one for
+            :class:`~.HTTPError`, one for :class:`~.HTTPStatus`, and one for
+            the standard ``Exception`` type, which prevents passing uncaught
+            exceptions to the WSGI server. These can be overridden by adding a
+            custom error handler method for the exception type in question.
+
+            Be aware that both :class:`~.HTTPError` and :class:`~.HTTPStatus`
+            inherit from the standard ``Exception`` type, so overriding the
+            default ``Exception`` handler will override all three default
+            handlers, due to the LIFO ordering of handler-matching.
 
         Args:
             exception (type or iterable of types): When handling a request,
@@ -556,7 +569,7 @@ class API:
                 If not specified explicitly, the handler will default to
                 ``exception.handle``, where ``exception`` is the error
                 type specified above, and ``handle`` is a static method
-                (i.e., decorated with @staticmethod) that accepts
+                (i.e., decorated with ``@staticmethod``) that accepts
                 the same params just described. For example::
 
                     class CustomException(CustomBaseException):
@@ -633,6 +646,11 @@ class API:
             property value, and it may choose to override the
             representation logic.
 
+        Note:
+            A custom serializer set with this method may not be called if the
+            default error handler for :class:`~.HTTPError` has been overriden.
+            See :meth:`~.add_error_handler` for more details.
+
         The :class:`~.HTTPError` class contains helper methods,
         such as `to_json()` and `to_dict()`, that can be used from
         within custom serializers. For example::
@@ -770,6 +788,11 @@ class API:
     def _http_error_handler(self, req, resp, error, params):
         self._compose_error_response(req, resp, error)
 
+    def _python_error_handler(self, req, resp, error, params):
+        req.log_error(traceback.format_exc())
+        self._compose_error_response(
+            req, resp, falcon.HTTPInternalServerError())
+
     def _handle_exception(self, req, resp, ex, params):
         """Handle an exception raised from mw or a responder.
 


Test patch:
diff --git a/tests/test_error_handlers.py b/tests/test_error_handlers.py
index c8dca25..e29e349 100644
--- a/tests/test_error_handlers.py
+++ b/tests/test_error_handlers.py
@@ -1,7 +1,7 @@
 import pytest
 
 import falcon
-from falcon import testing
+from falcon import constants, testing
 
 
 def capture_error(req, resp, ex, params):
@@ -61,14 +61,17 @@ class TestErrorHandler:
         assert result.status_code == 723
         assert not result.content
 
-    def test_uncaught_error(self, client):
-        client.app.add_error_handler(CustomException, capture_error)
-        with pytest.raises(Exception):
-            client.simulate_get()
-
-    def test_uncaught_error_else(self, client):
-        with pytest.raises(Exception):
-            client.simulate_get()
+    @pytest.mark.parametrize('get_headers, resp_content_type, resp_start', [
+        (None, constants.MEDIA_JSON, '{"'),
+        ({'accept': constants.MEDIA_JSON}, constants.MEDIA_JSON, '{"'),
+        ({'accept': constants.MEDIA_XML}, constants.MEDIA_XML, '<?xml'),
+    ])
+    def test_uncaught_python_error(self, client,
+                                   get_headers, resp_content_type, resp_start):
+        result = client.simulate_get(headers=get_headers)
+        assert result.status_code == 500
+        assert result.headers['content-type'] == resp_content_type
+        assert result.text.startswith(resp_start)
 
     def test_converted_error(self, client):
         client.app.add_error_handler(CustomException)
diff --git a/tests/test_middleware.py b/tests/test_middleware.py
index 008cf21..fd019a2 100644
--- a/tests/test_middleware.py
+++ b/tests/test_middleware.py
@@ -194,8 +194,8 @@ class TestRequestTimeMiddleware(TestMiddleware):
         app.add_route(TEST_ROUTE, MiddlewareClassResource())
         client = testing.TestClient(app)
 
-        with pytest.raises(Exception):
-            client.simulate_request(path=TEST_ROUTE)
+        result = client.simulate_request(path=TEST_ROUTE)
+        assert result.status_code == 500
 
     @pytest.mark.parametrize('independent_middleware', [True, False])
     def test_log_get_request(self, independent_middleware):
@@ -381,8 +381,8 @@ class TestSeveralMiddlewares(TestMiddleware):
         app.add_route(TEST_ROUTE, MiddlewareClassResource())
         client = testing.TestClient(app)
 
-        with pytest.raises(Exception):
-            client.simulate_request(path=TEST_ROUTE)
+        result = client.simulate_request(path=TEST_ROUTE)
+        assert result.status_code == 500
 
         # RequestTimeMiddleware process_response should be executed
         assert 'transaction_id' in context