fixup! Exclude tracebacks from error responses (#6420)

src/azul/chalice.py

@@ -40,7 +40,6 @@
     NotFoundError,
     Request,
     Response,
-    RestAPIEventHandler,
 )
 import chevron
 from furl import (
@@ -124,23 +123,6 @@ def aws_name(self) -> str:
 C = TypeVar('C', bound='AppController')
 
 
-def handle_uncaught_exception(self: RestAPIEventHandler) -> Response:
-    request = self.current_request
-    path = getattr(request, 'path', 'unknown')
-    log.error('Caught exception for path %r', path, exc_info=True)
-    body = {
-        'Code': 'InternalServerError',
-        'Message': 'An internal server error occurred.',
-        'RequestId': request.lambda_context.aws_request_id
-    }
-    return Response(body=body, status_code=500)
-
-
-# Ensures that stacktraces are never included in the response, even when
-# `app.debug` is set to true
-RestAPIEventHandler._unhandled_exception_to_response = handle_uncaught_exception
-
-
 class AzulChaliceApp(Chalice):
     # FIXME: Remove these two class attributes once upstream issue is fixed
     #        https://github.com/DataBiosphere/azul/issues/4558
@@ -161,7 +143,8 @@ def __init__(self,
         assert 'paths' not in spec, 'The top-level spec must not define paths'
         self._specs = copy_json(spec)
         self._specs['paths'] = {}
-        super().__init__(app_name, debug=config.debug > 0, configure_logs=False)
+        # `debug=False` prevents stacktraces from appearing in the response
+        super().__init__(app_name, debug=False, configure_logs=False)
         # Middleware is invoked in order of registration
         self.register_middleware(self._logging_middleware, 'http')
         self.register_middleware(self._security_headers_middleware, 'http')

test/test_app_logging.py

@@ -63,8 +63,9 @@ def fail():
                     server_thread.start()
                     try:
                         host, port = server_thread.address
-                        with self.assertLogs(azul.log, level=log_level) as azul_log:
-                            response = requests.get(f'http://{host}:{port}{path}')
+                        with self.assertLogs(app.log, level=log_level) as app_log:
+                            with self.assertLogs(azul.log, level=log_level) as azul_log:
+                                response = requests.get(f'http://{host}:{port}{path}')
                     finally:
                         server_thread.kill_thread()
                         server_thread.join(timeout=10)
@@ -73,8 +74,8 @@ def fail():
 
                     self.assertEqual(response.status_code, 500)
 
-                    # The request and exception are always logged
-                    self.assertEqual(len(azul_log.output), 4)
+                    # The request is always logged
+                    self.assertEqual(len(azul_log.output), 3)
                     headers = {
                         'host': f'{host}:{port}',
                         'user-agent': 'python-requests/2.32.3',
@@ -88,36 +89,46 @@ def fail():
                     self.assertEqual(azul_log.output[1],
                                      'INFO:azul.chalice:Did not authenticate request.')
 
-                    err_log = f'ERROR:azul.chalice:Caught exception for path {path!r}'
-                    self.assertTrue(azul_log.output[2].startswith(err_log))
-                    self.assertIn(magic_message, azul_log.output[2])
-                    self.assertIn(traceback_header, azul_log.output[2])
+                    # The exception is always logged
+                    self.assertEqual(len(app_log.output), 1)
+                    err_log = f'ERROR:test_app_logging:Caught exception for path {path}'
+                    self.assertTrue(app_log.output[0].startswith(err_log))
+                    self.assertIn(magic_message, app_log.output[0])
+                    self.assertIn(traceback_header, app_log.output[0])
+
+                    content = response.content.decode()
+                    # The response should never contain a stacktrace
+                    self.assertNotIn(traceback_header, content)
+                    self.assertNotIn(magic_message, content)
+                    json_content = json.loads(content)
 
-                    response_json = response.json()
-                    response_json.pop('RequestId')
-                    self.assertEqual(response_json, {
-                        'Code': 'InternalServerError',
-                        'Message': 'An internal server error occurred.'
-                    })
                     if debug:
                         # In debug mode, the response is logged.
                         headers = {
                             **app.security_headers(),
                             'Cache-Control': 'no-store'
                         }
-                        self.assertEqual(
-                            azul_log.output[3],
+                        expected = (
                             'DEBUG:azul.chalice:Returning 500 response with headers ' +
                             json.dumps(headers) + '. ' +
                             'See next line for the first 1024 characters of the body.\n' +
-                            # Normalize whitespace
-                            json.dumps(json.loads(response.content.decode()))
+                            # Yes, we already called `loads`, but this is needed
+                            # to normalize whitespace
+                            json.dumps(json_content)
                         )
                     else:
-                        self.assertEqual(
-                            azul_log.output[3],
-                            'INFO:azul.chalice:Returning 500 response. To log headers and body, set AZUL_DEBUG to 1.'
-                        )
+                        self.maxDiff = None
+                        # Otherwise, a generic error message is logged.
+                        expected = ('INFO:azul.chalice:Returning 500 response. ' +
+                                    'To log headers and body, set AZUL_DEBUG to 1.')
+                    self.assertEqual(azul_log.output[2], expected)
+
+                    # Can't be known in advance; different for each invocation
+                    json_content.pop('RequestId')
+                    self.assertEqual(json_content, {
+                        'Code': 'InternalServerError',
+                        'Message': 'An internal server error occurred.',
+                    })
 
 
 class TestPermittedWarnings(AzulUnitTestCase):