No tracebacks on normal errors and prettier error pages (sanic-org#1768)

* Default error handler now only logs traceback on 500 errors and all responses are HTML formatted. * Tests passing. * Ability to flag any exception object with self.quiet = True following @ashleysommer suggestion. * Refactor HTML formatting into errorpages.py. String escapes for debug tracebacks. * Remove extra includes * Auto-set quiet flag also when decorator is used. * Cleanup, make error pages (probably) HTML5-compliant and similar for debug and non-debug modes. * Fix lookup of non-existant status codes * No logging of 503 errors after all. * lint
Sts0mrg0 · Jan 20, 2020 · ba9b432 · ba9b432
1 parent b565072
commit ba9b432
Show file tree

Hide file tree

Showing 14 changed files with 165 additions and 214 deletions.
diff --git a/sanic/errorpages.py b/sanic/errorpages.py
@@ -0,0 +1,117 @@
+import sys
+
+from traceback import extract_tb
+
+from sanic.exceptions import SanicException
+from sanic.helpers import STATUS_CODES
+from sanic.response import html
+
+
+# Here, There Be Dragons (custom HTML formatting to follow)
+
+
+def escape(text):
+    """Minimal HTML escaping, not for attribute values (unlike html.escape)."""
+    return f"{text}".replace("&", "&amp;").replace("<", "&lt;")
+
+
+def exception_response(request, exception, debug):
+    status = 500
+    text = (
+        "The server encountered an internal error "
+        "and cannot complete your request."
+    )
+
+    headers = {}
+    if isinstance(exception, SanicException):
+        text = f"{exception}"
+        status = getattr(exception, "status_code", status)
+        headers = getattr(exception, "headers", headers)
+    elif debug:
+        text = f"{exception}"
+
+    status_text = STATUS_CODES.get(status, b"Error Occurred").decode()
+    title = escape(f"{status} — {status_text}")
+    text = escape(text)
+
+    if debug and not getattr(exception, "quiet", False):
+        return html(
+            f"<!DOCTYPE html><meta charset=UTF-8><title>{title}</title>"
+            f"<style>{TRACEBACK_STYLE}</style>\n"
+            f"<h1>⚠️ {title}</h1><p>{text}\n"
+            f"{_render_traceback_html(request, exception)}",
+            status=status,
+        )
+
+    # Keeping it minimal with trailing newline for pretty curl/console output
+    return html(
+        f"<!DOCTYPE html><meta charset=UTF-8><title>{title}</title>"
+        "<style>html { font-family: sans-serif }</style>\n"
+        f"<h1>⚠️ {title}</h1><p>{text}\n",
+        status=status,
+        headers=headers,
+    )
+
+
+def _render_exception(exception):
+    frames = extract_tb(exception.__traceback__)
+    frame_html = "".join(TRACEBACK_LINE_HTML.format(frame) for frame in frames)
+    return TRACEBACK_WRAPPER_HTML.format(
+        exc_name=escape(exception.__class__.__name__),
+        exc_value=escape(exception),
+        frame_html=frame_html,
+    )
+
+
+def _render_traceback_html(request, exception):
+    exc_type, exc_value, tb = sys.exc_info()
+    exceptions = []
+    while exc_value:
+        exceptions.append(_render_exception(exc_value))
+        exc_value = exc_value.__cause__
+
+    traceback_html = TRACEBACK_BORDER.join(reversed(exceptions))
+    appname = escape(request.app.name)
+    name = escape(exception.__class__.__name__)
+    value = escape(exception)
+    path = escape(request.path)
+    return (
+        f"<h2>Traceback of {appname} (most recent call last):</h2>"
+        f"{traceback_html}"
+        "<div class=summary><p>"
+        f"<b>{name}: {value}</b> while handling path <code>{path}</code>"
+    )
+
+
+TRACEBACK_STYLE = """
+    html { font-family: sans-serif }
+    h2 { color: #888; }
+    .tb-wrapper p { margin: 0 }
+    .frame-border { margin: 1rem }
+    .frame-line > * { padding: 0.3rem 0.6rem }
+    .frame-line { margin-bottom: 0.3rem }
+    .frame-code { font-size: 16px; padding-left: 4ch }
+    .tb-wrapper { border: 1px solid #eee }
+    .tb-header { background: #eee; padding: 0.3rem; font-weight: bold }
+    .frame-descriptor { background: #e2eafb; font-size: 14px }
+"""
+
+TRACEBACK_WRAPPER_HTML = (
+    "<div class=tb-header>{exc_name}: {exc_value}</div>"
+    "<div class=tb-wrapper>{frame_html}</div>"
+)
+
+TRACEBACK_BORDER = (
+    "<div class=frame-border>"
+    "The above exception was the direct cause of the following exception:"
+    "</div>"
+)
+
+TRACEBACK_LINE_HTML = (
+    "<div class=frame-line>"
+    "<p class=frame-descriptor>"
+    "File {0.filename}, line <i>{0.lineno}</i>, "
+    "in <code><b>{0.name}</b></code>"
+    "<p class=frame-code><code>{0.line}</code>"
+    "</div>"
+)
diff --git a/sanic/exceptions.py b/sanic/exceptions.py
@@ -1,146 +1,35 @@
 from sanic.helpers import STATUS_CODES
 
 
-TRACEBACK_STYLE = """
-    <style>
-        body {
-            padding: 20px;
-            font-family: Arial, sans-serif;
-        }
-
-        p {
-            margin: 0;
-        }
-
-        .summary {
-            padding: 10px;
-        }
-
-        h1 {
-            margin-bottom: 0;
-        }
-
-        h3 {
-            margin-top: 10px;
-        }
-
-        h3 code {
-            font-size: 24px;
-        }
-
-        .frame-line > * {
-            padding: 5px 10px;
-        }
-
-        .frame-line {
-            margin-bottom: 5px;
-        }
-
-        .frame-code {
-            font-size: 16px;
-            padding-left: 30px;
-        }
-
-        .tb-wrapper {
-            border: 1px solid #f3f3f3;
-        }
-
-        .tb-header {
-            background-color: #f3f3f3;
-            padding: 5px 10px;
-        }
-
-        .tb-border {
-            padding-top: 20px;
-        }
-
-        .frame-descriptor {
-            background-color: #e2eafb;
-        }
-
-        .frame-descriptor {
-            font-size: 14px;
-        }
-    </style>
-"""
-
-TRACEBACK_WRAPPER_HTML = """
-    <html>
-        <head>
-            {style}
-        </head>
-        <body>
-            {inner_html}
-            <div class="summary">
-                <p>
-                <b>{exc_name}: {exc_value}</b>
-                    while handling path <code>{path}</code>
-                </p>
-            </div>
-        </body>
-    </html>
-"""
-
-TRACEBACK_WRAPPER_INNER_HTML = """
-    <h1>{exc_name}</h1>
-    <h3><code>{exc_value}</code></h3>
-    <div class="tb-wrapper">
-        <p class="tb-header">Traceback (most recent call last):</p>
-        {frame_html}
-    </div>
-"""
-
-TRACEBACK_BORDER = """
-    <div class="tb-border">
-        <b><i>
-            The above exception was the direct cause of the
-            following exception:
-        </i></b>
-    </div>
-"""
-
-TRACEBACK_LINE_HTML = """
-    <div class="frame-line">
-        <p class="frame-descriptor">
-            File {0.filename}, line <i>{0.lineno}</i>,
-            in <code><b>{0.name}</b></code>
-        </p>
-        <p class="frame-code"><code>{0.line}</code></p>
-    </div>
-"""
-
-INTERNAL_SERVER_ERROR_HTML = """
-    <h1>Internal Server Error</h1>
-    <p>
-        The server encountered an internal error and cannot complete
-        your request.
-    </p>
-"""
-
-
 _sanic_exceptions = {}
 
 
-def add_status_code(code):
+def add_status_code(code, quiet=None):
     """
     Decorator used for adding exceptions to :class:`SanicException`.
     """
 
     def class_decorator(cls):
         cls.status_code = code
+        if quiet or quiet is None and code != 500:
+            cls.quiet = True
         _sanic_exceptions[code] = cls
         return cls
 
     return class_decorator
 
 
 class SanicException(Exception):
-    def __init__(self, message, status_code=None):
+    def __init__(self, message, status_code=None, quiet=None):
         super().__init__(message)
 
         if status_code is not None:
             self.status_code = status_code
 
+        # quiet=None/False/True with None meaning choose by status
+        if quiet or quiet is None and status_code not in (None, 500):
+            self.quiet = True
+
 
 @add_status_code(404)
 class NotFound(SanicException):

diff --git a/sanic/handlers.py b/sanic/handlers.py
@@ -1,21 +1,13 @@
-import sys
-
-from traceback import extract_tb, format_exc
+from traceback import format_exc
 
+from sanic.errorpages import exception_response
 from sanic.exceptions import (
-    INTERNAL_SERVER_ERROR_HTML,
-    TRACEBACK_BORDER,
-    TRACEBACK_LINE_HTML,
-    TRACEBACK_STYLE,
-    TRACEBACK_WRAPPER_HTML,
-    TRACEBACK_WRAPPER_INNER_HTML,
     ContentRangeError,
     HeaderNotFound,
     InvalidRangeType,
-    SanicException,
 )
 from sanic.log import logger
-from sanic.response import html, text
+from sanic.response import text
 
 
 class ErrorHandler:
@@ -40,35 +32,6 @@ def __init__(self):
         self.cached_handlers = {}
         self.debug = False
 
-    def _render_exception(self, exception):
-        frames = extract_tb(exception.__traceback__)
-
-        frame_html = []
-        for frame in frames:
-            frame_html.append(TRACEBACK_LINE_HTML.format(frame))
-
-        return TRACEBACK_WRAPPER_INNER_HTML.format(
-            exc_name=exception.__class__.__name__,
-            exc_value=exception,
-            frame_html="".join(frame_html),
-        )
-
-    def _render_traceback_html(self, exception, request):
-        exc_type, exc_value, tb = sys.exc_info()
-        exceptions = []
-
-        while exc_value:
-            exceptions.append(self._render_exception(exc_value))
-            exc_value = exc_value.__cause__
-
-        return TRACEBACK_WRAPPER_HTML.format(
-            style=TRACEBACK_STYLE,
-            exc_name=exception.__class__.__name__,
-            exc_value=exception,
-            inner_html=TRACEBACK_BORDER.join(reversed(exceptions)),
-            path=request.path,
-        )
-
     def add(self, exception, handler):
         """
         Add a new exception handler to an already existing handler object.
@@ -166,27 +129,17 @@ def default(self, request, exception):
             :class:`Exception`
         :return:
         """
-        self.log(format_exc())
-        try:
-            url = repr(request.url)
-        except AttributeError:
-            url = "unknown"
-
-        response_message = "Exception occurred while handling uri: %s"
-        logger.exception(response_message, url)
-
-        if issubclass(type(exception), SanicException):
-            return text(
-                "Error: {}".format(exception),
-                status=getattr(exception, "status_code", 500),
-                headers=getattr(exception, "headers", dict()),
-            )
-        elif self.debug:
-            html_output = self._render_traceback_html(exception, request)
+        quiet = getattr(exception, "quiet", False)
+        if quiet is False:
+            try:
+                url = repr(request.url)
+            except AttributeError:
+                url = "unknown"
 
-            return html(html_output, status=500)
-        else:
-            return html(INTERNAL_SERVER_ERROR_HTML, status=500)
+            self.log(format_exc())
+            logger.exception("Exception occurred while handling uri: %s", url)
+
+        return exception_response(request, exception, self.debug)
 
 
 class ContentRangeHandler:

diff --git a/tests/test_app.py b/tests/test_app.py
@@ -105,10 +105,7 @@ def handler(request):
 
     request, response = app.test_client.get("/test")
 
-    assert (
-        response.text
-        == "Error: 'None' was returned while requesting a handler from the router"
-    )
+    assert "'None' was returned while requesting a handler from the router" in response.text
 
 
 @pytest.mark.parametrize("websocket_enabled", [True, False])
@@ -189,7 +186,7 @@ def handler(request):
     with caplog.at_level(logging.ERROR):
         request, response = app.test_client.get("/")
     assert response.status == 500
-    assert response.text == "Error: Mock SanicException"
+    assert "Mock SanicException" in response.text
     assert (
         "sanic.root",
         logging.ERROR,

diff --git a/tests/test_bad_request.py b/tests/test_bad_request.py
@@ -18,4 +18,4 @@ async def _request(sanic, loop):
 
     app.run(host="127.0.0.1", port=42101, debug=False)
     assert lines[0] == b"HTTP/1.1 400 Bad Request\r\n"
-    assert lines[-1] == b"Error: Bad Request"
+    assert b"Bad Request" in lines[-1]