Improve error pages (for Okta)

backend/ng/core/middleware/error_handler.py

@@ -3,7 +3,6 @@
 Provides a unified decorator and a global registration function.
 """
 
-import sys
 import traceback
 from functools import wraps
 
@@ -16,21 +15,12 @@
 
 from ..exceptions import APIException
 from ..utils import error_response
-from ..utils.logger import get_logger
+from ..utils.logger import TRACEBACK_FRAME_LIMIT, format_traceback, get_logger
 
 logger = get_logger(__name__)
 
-def _get_small_traceback(limit = 3) -> str:
-    exc_type, exc, tb = sys.exc_info()
-    if tb is None:
-        return ""
-
-    frames = traceback.extract_tb(tb)
-
-    # Most recent call first, limit frames
-    frames = frames[-limit:][::-1]
-
-    return ''.join(traceback.format_list(frames))
+def _get_small_traceback(limit: int = TRACEBACK_FRAME_LIMIT) -> str:
+    return format_traceback(limit=limit)
 
 
 def _get_request_context() -> dict:

backend/ng/core/routes/views.py

@@ -4,23 +4,14 @@
 
 from typing import Any
 
-from CTFd.utils import get_app_config
-from ..utils.current_user import get_current_user
-from flask import Blueprint, render_template
-from flask import current_app as app
+from flask import Blueprint
+
+from ..utils.frontend import render_frontend
 
 plugin_views = Blueprint("plugin_views", __name__)
 
 
 @plugin_views.route("/", defaults={"subpath": ""}, methods=["GET"], strict_slashes=False)
 @plugin_views.route("/<path:subpath>", methods=["GET"])
 def view_template(subpath: str) -> Any:
-    static_build_path = get_app_config("STATIC_BUILD_PATH")
-    user = get_current_user()
-    return render_template(
-        "dev_entrypoint.html" if app.debug else "prod_entrypoint.html",
-        static_build_path=static_build_path,
-        user_id=user.id if user else None,
-    )
-
-
+    return render_frontend()

backend/ng/core/tests/helpers.py

@@ -2,6 +2,9 @@
 Test helper functions for setting up the plugin's test environment.
 """
 
+from pathlib import Path
+
+import jinja2
 from flask import g
 from tests.helpers import (
     create_ctfd as create_ctfd_original,
@@ -28,11 +31,30 @@ def plugin_load(app):
         raise
 
 
+def register_frontend_templates(app):
+    """
+    Put the frontend entrypoint templates on the app's template path.
+
+    A deployed image copies `backend/views` into the active theme's template
+    directory (see `dockerfiles/ctfd.Dockerfile`), which is how the frontend
+    shell resolves at runtime. Tests run against the CTFd source tree, where
+    that copy never happened, so point Jinja straight at the directory.
+    """
+    views = Path(__file__).resolve().parents[3] / "views"
+
+    app.jinja_loader = jinja2.ChoiceLoader([
+        jinja2.FileSystemLoader(str(views)),
+        app.jinja_loader,
+    ])
+
+
 def create_ctfd():
     """Prepares the Flask app instance for the test session."""
 
     app = create_ctfd_original(enable_plugins=True, setup=False)
 
+    register_frontend_templates(app)
+
     # Disable rate limiters for testing
     app.config["RATELIMIT_ENABLED"] = False
 

backend/ng/core/tests/test_frontend_render.py

@@ -0,0 +1,25 @@
+"""
+Tests for the frontend entrypoint templates.
+"""
+
+import json
+import re
+
+import pytest
+from flask import render_template
+
+WINDOW_INIT_ERROR = re.compile(r"error:\s*(\{.*?\}|null)\s*\n", re.DOTALL)
+
+
+@pytest.mark.parametrize("template", ["dev_entrypoint.html", "prod_entrypoint.html"])
+def test_entrypoint_renders_without_an_error_argument(app, template):
+    """
+    Test that the entrypoint templates tolerate being rendered without `error`.
+
+    `tojson` raises on an undefined value, so a caller that does not know about
+    the argument would otherwise get a 500 instead of the app.
+    """
+    with app.test_request_context():
+        body = render_template(template)
+
+    assert json.loads(WINDOW_INIT_ERROR.search(body).group(1)) is None

backend/ng/core/tests/test_logger.py

@@ -0,0 +1,101 @@
+"""
+Tests for the JSON logger's record formatting.
+"""
+
+import json
+import logging
+import sys
+
+from ..utils.logger import JSONFormatter, format_traceback
+
+
+def formatted(record) -> dict:
+    """Run a record through the JSON formatter and parse the result."""
+
+    return json.loads(JSONFormatter().format(record))
+
+
+def make_record(**kwargs) -> logging.LogRecord:
+    """Build a log record the way `logger.error(...)` would."""
+
+    defaults = {
+        "name": "test",
+        "level": logging.ERROR,
+        "pathname": __file__,
+        "lineno": 1,
+        "msg": "something failed",
+        "args": (),
+        "exc_info": None,
+    }
+
+    return logging.LogRecord(**{**defaults, **kwargs})
+
+
+def test_traceback_is_recorded_for_exc_info():
+    """
+    Test that a record carrying exc_info keeps its traceback. The formatter
+    builds the entry field by field, so the traceback is dropped unless it is
+    read out explicitly.
+    """
+    try:
+        raise ValueError("the specific failure")
+    except ValueError:
+        record = make_record(exc_info=sys.exc_info())
+
+    trace = formatted(record)["trace"]
+
+    assert "ValueError: the specific failure" in trace
+    assert "test_traceback_is_recorded_for_exc_info" in trace
+
+
+def test_explicit_trace_wins_over_exc_info():
+    """
+    Test that a trace supplied on the record is kept as-is, so the existing
+    callers that pass their own trace are unaffected
+    """
+    try:
+        raise ValueError("ignored")
+    except ValueError:
+        record = make_record(exc_info=sys.exc_info())
+
+    record.trace = "supplied by the caller"
+
+    assert formatted(record)["trace"] == "supplied by the caller"
+
+
+def test_no_trace_without_an_exception():
+    """
+    Test that an ordinary record carries no trace field
+    """
+    assert "trace" not in formatted(make_record())
+
+
+def test_format_traceback_without_an_exception():
+    """
+    Test that formatting outside of an exception handler yields nothing rather
+    than raising
+    """
+    assert format_traceback() == ""
+
+
+def test_format_traceback_limits_frames():
+    """
+    Test that only the frames nearest the failure are kept
+    """
+    def depth_3():
+        raise RuntimeError("deep")
+
+    def depth_2():
+        depth_3()
+
+    def depth_1():
+        depth_2()
+
+    try:
+        depth_1()
+    except RuntimeError:
+        trace = format_traceback(limit=1)
+
+    assert "depth_3" in trace
+    assert "depth_1" not in trace
+    assert "RuntimeError: deep" in trace

backend/ng/core/utils/error_page.py

@@ -0,0 +1,76 @@
+"""
+Renders browser-facing failures as the frontend error page.
+
+Most plugin routes are called by the SPA over fetch, so a JSON body from
+`error_response` is the right answer. A handful of routes are loaded directly
+in the browser instead - OAuth callbacks, external redirects - and there a JSON
+body leaves the user staring at raw text. These serve the app itself with the
+failure attached, so it renders with the rest of the app's chrome.
+"""
+
+from uuid import uuid4
+
+from flask import Response
+from flask import current_app as app
+
+from .frontend import render_frontend
+from .logger import get_logger
+
+logger = get_logger(__name__)
+
+
+def render_error_page(
+    code: str,
+    *,
+    status: int = 500,
+    log_message: str | None = None,
+    context: dict | None = None,
+    detail: str | None = None,
+    exc_info: bool = False,
+) -> Response:
+    """
+    Log a browser-facing failure and render the frontend error page for it.
+
+    The failure travels in the document, as `window.init.error`, rather than in
+    the query string. Nothing about it is readable or editable in the URL, so a
+    hand-crafted link cannot be used to put a chosen error in front of a victim,
+    and the debug detail below is not exposed by a shared or logged URL.
+
+    Args:
+        code: Stable identifier for the failure. Must have a matching entry in
+            the frontend's ERRORS map, or the page falls back to generic copy.
+        status: HTTP status for the response, also shown on the page.
+        log_message: Line to log. Defaults to a description of the failure.
+        context: Extra structured fields to attach to the log entry.
+        detail: Internal specifics (exception text, validation failure). Shown
+            on the page only when the app is in debug mode, always logged.
+        exc_info: Attach the active exception's traceback to the log entry.
+
+    Returns:
+        An HTML response carrying the frontend app and the failure to display.
+        Returned as a `Response` so Flask-RESTX serves it as-is instead of
+        encoding the document as JSON.
+    """
+    reference = uuid4().hex[:12]
+
+    logger.error(
+        log_message or f"Rendering error page: {code}",
+        extra={
+            "context": {
+                "error_code": code,
+                "status_code": status,
+                "reference": reference,
+                **({"detail": detail} if detail else {}),
+                **(context or {}),
+            },
+        },
+        exc_info=exc_info,
+    )
+
+    error = {"code": code, "status": status, "reference": reference}
+
+    # `detail` can carry internal specifics, so it stays out of production pages.
+    if detail and app.debug:
+        error["detail"] = detail
+
+    return Response(render_frontend(error=error), status=status, mimetype="text/html")

backend/ng/core/utils/frontend.py

@@ -0,0 +1,35 @@
+"""
+Renders the frontend single-page application shell.
+
+Routes that a browser navigates to directly return this rather than JSON, so
+the user gets the app instead of a raw response body.
+"""
+
+from CTFd.utils import get_app_config
+from flask import current_app as app
+from flask import render_template
+
+from .current_user import get_current_user
+
+
+def render_frontend(*, error: dict | None = None) -> str:
+    """
+    Render the SPA entrypoint template.
+
+    Args:
+        error: A failure for the app to display instead of the routed page.
+            Serialized into `window.init.error`, where the frontend picks it up.
+            Passing it through the document rather than the query string keeps
+            it off the URL, where a visitor could edit it.
+
+    Returns:
+        The rendered HTML document.
+    """
+    user = get_current_user()
+
+    return render_template(
+        "dev_entrypoint.html" if app.debug else "prod_entrypoint.html",
+        static_build_path=get_app_config("STATIC_BUILD_PATH"),
+        user_id=user.id if user else None,
+        error=error,
+    )

backend/ng/core/utils/logger.py

@@ -6,6 +6,7 @@
 import logging
 import os
 import sys
+import traceback
 from datetime import UTC, datetime
 
 
@@ -17,6 +18,33 @@ def utc_now() -> datetime:
 PLUGIN_LOGGER_NAME = "ctfd_ng_plugin"
 logger = logging.getLogger(PLUGIN_LOGGER_NAME)
 
+# Frames kept from a traceback. A traceback is most of an entry's size, and the
+# frames nearest the failure are the ones worth reading.
+TRACEBACK_FRAME_LIMIT = 3
+
+
+def format_traceback(exc_info=None, limit: int = TRACEBACK_FRAME_LIMIT) -> str:
+    """
+    Render an exception as a short traceback, most recent frame first.
+
+    Args:
+        exc_info: A `(type, value, traceback)` triple, as carried by a log
+            record. Defaults to the exception currently being handled.
+        limit: Frames to keep, counting back from the failure.
+
+    Returns:
+        The formatted frames followed by the exception line, or an empty
+        string when there is no exception to report.
+    """
+    exc_type, exc, tb = exc_info or sys.exc_info()
+    if tb is None:
+        return ""
+
+    # Most recent call first, limited to the frames nearest the failure
+    frames = traceback.extract_tb(tb)[-limit:][::-1]
+
+    return "".join(traceback.format_list(frames) + traceback.format_exception_only(exc_type, exc))
+
 
 class JSONFormatter(logging.Formatter):
     def format(self, record):
@@ -33,6 +61,10 @@ def format(self, record):
 
         if hasattr(record, "trace") and record.trace:
             log_entry["trace"] = record.trace
+        elif record.exc_info:
+            # This formatter builds the entry field by field, so a traceback
+            # passed as `exc_info=True` is dropped unless it is picked up here.
+            log_entry["trace"] = format_traceback(record.exc_info)
 
         return json.dumps(log_entry)