fix(embedded): fail closed when the guest-user check can't resolve a principal

Follow-up to #43834. That change guarded is_sanitization_required() so a
raising user loader could no longer turn error responses into bare 500s,
but it failed *open*: when the guest-user check raised it returned False
("do not sanitize"), which can disclose raw engine errors to a genuine
embedded guest.

The check can raise for a resolvable guest, not only an unresolvable
anonymous one: get_guest_user_from_token() calls find_role() (a metadata-DB
round trip) outside the request loader's try/except, and is_guest_user()
consults the EMBEDDED_SUPERSET feature hook first. On a request whose DB
session is already broken -- exactly the state a SQLAlchemyError handler
runs in -- a valid guest token then yields the raw driver error instead of
a redacted message.

Make the fallback incapable of raising and fail closed for anyone
presenting a token:

- is_sanitization_required(): on failure, inside a request context fall
  back to whether the request carries a guest token (reading the header or
  form field cannot raise); redact if it does, leave genuinely anonymous
  errors untouched. Outside a request context (Celery worker under
  override_user) fail closed and redact, since the payload is delivered to
  the embedded viewer. Reworded the docstring as an availability-over-
  confidentiality trade-off rather than a definitional truth.

- Resolve the decision once and thread it down: add an optional
  `required` param to the sanitize_* helpers so a list is looked up a
  single time instead of once per error (fixes the N+1 loader calls and
  WARNING log-spam on multi-error responses).

- charts/data/api.py: replace the raw outer is_guest_user() with the
  guarded, once-resolved decision and reuse it for both the stacktrace pop
  and the message sanitization, so the block can no longer half-redact
  (drop the stacktrace while leaking the raw error).

- views/error_handling.py: give the last-resort show_unexpected_exception
  the same try/except FileNotFoundError fallback its siblings have, so a
  missing 500.html no longer collapses the response to a bare 500 with no
  SIP-40 body.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Elizabeth Thompson <eschutho@gmail.com>

superset/charts/data/api.py

@@ -66,7 +66,10 @@ from superset.utils.core import (
     get_user_id,
 )
 from superset.utils.decorators import logs_context
-from superset.utils.error_sanitization import sanitize_error_message
+from superset.utils.error_sanitization import (
+    is_sanitization_required,
+    sanitize_error_message,
+)
 from superset.views.base import CsvResponse, generate_download_headers, XlsxResponse
 from superset.views.base_api import statsd_metrics
 
@@ -584,7 +587,13 @@ class ChartDataRestApi(ChartRestApi):
                 ):
                     query["timing"] = query_result.timing.as_public_dict()
 
-            if security_manager.is_guest_user():
+            # Resolve the redaction decision once so the stacktrace pop and the
+            # error-message sanitization below stay consistent: the guarded
+            # ``is_sanitization_required`` cannot raise the request into a 500 and
+            # fails closed for a guest whose principal can't be resolved, so the
+            # block never half-redacts (popping ``stacktrace`` while leaking the
+            # raw ``error``).
+            if sanitize_required := is_sanitization_required():
                 # Guests may see the generated SQL only when the role attached to
                 # their guest token has been granted "can view query on Dashboard",
                 # mirroring the permission the frontend uses to expose the
@@ -598,7 +607,9 @@ class ChartDataRestApi(ChartRestApi):
                         query.pop("query", None)
                     query.pop("stacktrace", None)
                     if query.get("error"):
-                        query["error"] = sanitize_error_message(query["error"])
+                        query["error"] = sanitize_error_message(
+                            query["error"], required=sanitize_required
+                        )
 
             payload: dict[str, Any] = {"result": queries}
             if dashboard_filter_context is not None:

superset/utils/error_sanitization.py

@@ -30,6 +30,7 @@ import dataclasses
 import logging
 from typing import Any
 
+from flask import current_app, has_request_context, request
 from flask_babel import lazy_gettext as _
 
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
@@ -80,15 +81,29 @@ ACCESS_STATUSES = frozenset({401, 403})
 
 def is_sanitization_required() -> bool:
     """
-    Whether the principal of the current request is an embedded guest viewer.
+    Whether the current request's error details should be redacted.
 
-    This runs inside Flask's HTTP error handler, which has no handler of its own:
-    if resolving the principal raises, Flask discards the intended status and
-    returns a bare 500. Some Flask-Login user loaders (e.g. a JWT request loader)
-    raise rather than fall back to an anonymous user when a request carries no
-    valid credential, so the lookup below must never be allowed to propagate. A
-    request whose principal cannot be resolved is by definition not an embedded
-    guest viewer, so there is nothing to redact and ``False`` is the safe answer.
+    Error details are redacted for embedded guest viewers. This runs inside
+    Flask's HTTP error handler, which has no handler of its own: if resolving the
+    principal raises, Flask discards the intended status and returns a bare 500.
+    The lookup can raise in more than one way -- a Flask-Login user loader (e.g.
+    a JWT request loader) may raise rather than fall back to an anonymous user;
+    resolving a *valid* guest token itself does a metadata-DB round trip
+    (``find_role``) and consults the ``EMBEDDED_SUPERSET`` feature hook, either of
+    which can raise on a request whose DB session is already broken -- and a
+    broken session is exactly the state the handler for a ``SQLAlchemyError`` runs
+    in. The lookup below must therefore never be allowed to propagate.
+
+    When it does raise the principal is unknown, so this makes a deliberate
+    availability-over-confidentiality trade-off rather than guessing "not a
+    guest". Inside a request context it falls back to whether the request even
+    carries a guest token: reading headers and form fields cannot raise, anyone
+    presenting a token is redacted (failing closed for the only principal whose
+    errors are redacted), and a genuinely anonymous request keeps its error so
+    ordinary failures are not over-sanitized. Outside a request context (e.g. a
+    Celery worker, where a guest principal may be active via ``override_user`` and
+    the error is delivered to the embedded viewer) there is no token to read and
+    no handler-of-a-handler concern, so it fails closed and redacts.
     """
     # pylint: disable=import-outside-toplevel
     from superset import security_manager
@@ -99,13 +114,28 @@ def is_sanitization_required() -> bool:
         # Never let identifying the principal break the error handler itself.
         logger.warning(
             "Could not resolve the request principal while deciding whether to "
-            "sanitize an error response; treating it as a non-guest request.",
+            "sanitize an error response; falling back to the presence of a guest "
+            "token.",
             exc_info=True,
         )
-        return False
+        if not has_request_context():
+            # No request to inspect (e.g. a Celery worker running under
+            # ``override_user``). Fail closed: a guest principal may be active
+            # and the redacted payload is delivered to the embedded viewer.
+            return True
+        # Reading the token header and form field cannot raise, so this fallback
+        # is itself incapable of breaking the error handler. ``.get`` on the
+        # config keeps that guarantee even if the key is somehow absent.
+        header_name = current_app.config.get("GUEST_TOKEN_HEADER_NAME")
+        return bool(
+            (header_name and request.headers.get(header_name))
+            or request.form.get("guest_token")
+        )
 
 
-def sanitize_error_message(message: str, status: int | None = None) -> str:
+def sanitize_error_message(
+    message: str, status: int | None = None, required: bool | None = None
+) -> str:
     """
     Replace an error message with a generic one for embedded guest viewers.
 
@@ -113,15 +143,22 @@ def sanitize_error_message(message: str, status: int | None = None) -> str:
     string carries no error type, so there is no way to tell an authorization
     denial from an engine error that happens to be reported as a 404. `status`
     only selects which generic message reads correctly.
+
+    `required` lets a caller that has already resolved the sanitization decision
+    thread it in so the principal is not looked up again (``None`` computes it).
     """
-    if not is_sanitization_required():
+    if required is None:
+        required = is_sanitization_required()
+    if not required:
         return message
     if status in ACCESS_STATUSES:
         return str(GENERIC_ACCESS_MESSAGE)
     return str(GENERIC_ERROR_MESSAGE)
 
 
-def sanitize_superset_error(error: SupersetError) -> SupersetError:
+def sanitize_superset_error(
+    error: SupersetError, required: bool | None = None
+) -> SupersetError:
     """
     Replace a ``SupersetError`` with a generic one for embedded guest viewers.
 
@@ -129,8 +166,13 @@ def sanitize_superset_error(error: SupersetError) -> SupersetError:
     some error types, the offending SQL. An allowlisted error keeps its message
     and type, but its ``extra`` is still filtered to `SAFE_EXTRA_KEYS` -- an
     allowlisted type is not a promise that everything hanging off it is safe.
+
+    `required` lets a caller that has already resolved the sanitization decision
+    thread it in so the principal is not looked up again (``None`` computes it).
     """
-    if not is_sanitization_required():
+    if required is None:
+        required = is_sanitization_required()
+    if not required:
         return error
     if error.error_type in SAFE_ERROR_TYPES:
         if not error.extra:
@@ -151,23 +193,36 @@ def sanitize_superset_error(error: SupersetError) -> SupersetError:
     )
 
 
-def sanitize_superset_errors(errors: list[SupersetError]) -> list[SupersetError]:
+def sanitize_superset_errors(
+    errors: list[SupersetError], required: bool | None = None
+) -> list[SupersetError]:
     """
     Replace each leaky ``SupersetError`` with a generic one for guest viewers.
+
+    The sanitization decision is resolved once and threaded into the per-error
+    calls, so the principal is looked up a single time for the whole list rather
+    than once per error.
     """
-    if not is_sanitization_required():
+    if required is None:
+        required = is_sanitization_required()
+    if not required:
         return errors
-    return [sanitize_superset_error(error) for error in errors]
+    return [sanitize_superset_error(error, required=required) for error in errors]
 
 
-def sanitize_error_dicts(errors: list[Any]) -> list[Any]:
+def sanitize_error_dicts(errors: list[Any], required: bool | None = None) -> list[Any]:
     """
     Same as :func:`sanitize_superset_errors`, for already serialized errors.
 
     Entries that aren't ``SupersetError`` shaped — a bare string, or a dict with
     only a message — are treated as leaky and replaced wholesale.
+
+    The sanitization decision is resolved once and threaded into the per-error
+    calls, so the principal is looked up a single time for the whole list.
     """
-    if not is_sanitization_required():
+    if required is None:
+        required = is_sanitization_required()
+    if not required:
         return errors
 
     sanitized = []
@@ -190,7 +245,8 @@ def sanitize_error_dicts(errors: list[Any]) -> list[Any]:
                         error_type=error_type,
                         level=level,
                         extra=payload.get("extra"),
-                    )
+                    ),
+                    required=required,
                 )
             )
         )

superset/views/error_handling.py

@@ -294,7 +294,15 @@ def set_app_error_handlers(app: Flask) -> None:  # noqa: C901
 
         if "text/html" in request.accept_mimetypes and not app.config["DEBUG"]:
             path = files("superset") / "static/assets/500.html"
-            return send_file(path, max_age=0), 500
+            # Try to serve HTML file; fall back to JSON if not built. This is the
+            # last-resort handler, so a missing ``500.html`` (a webpack artifact
+            # absent in API-only/unbuilt deployments) must not raise its own
+            # ``FileNotFoundError`` and collapse the response to a bare 500 with
+            # no SIP-40 body.
+            try:
+                return send_file(path, max_age=0), 500
+            except FileNotFoundError:
+                pass
 
         return json_error_response(
             [

tests/unit_tests/charts/test_chart_data_api.py

@@ -437,6 +437,51 @@ def test_send_chart_response_still_redacts_guest_errors_when_query_permitted(
     assert "stacktrace" not in query
 
 
+def test_send_chart_response_does_not_half_redact_guest_query_error(
+    app: SupersetApp,
+) -> None:
+    """
+    The redaction decision is resolved once and threaded down, so the block can
+    never pop ``stacktrace`` while leaking the raw ``error``. Previously the
+    outer guest check and the inner ``sanitize_error_message`` resolved the
+    principal independently: an ``is_guest_user`` that succeeded for the outer
+    check but raised for the inner one (e.g. a DB session that broke mid-request)
+    dropped the stacktrace yet kept the raw driver error, failing open. The
+    request carries a guest token so, even if the check raises, the shared
+    fallback fails closed and both are redacted.
+    """
+    result = _json_execution_result(
+        {
+            "error": "Table mydb.myschema.mytable was not found",
+            "stacktrace": "Traceback ...",
+            "query": "SELECT 1",
+        },
+        result_type=ChartDataResultType.QUERY,
+    )
+
+    header = app.config["GUEST_TOKEN_HEADER_NAME"]
+    api = ChartDataRestApi()
+    with (
+        app.test_request_context(
+            "/api/v1/chart/data", headers={header: "a.guest.token"}
+        ),
+        patch(
+            "superset.charts.data.api.security_manager.is_guest_user",
+            side_effect=[True, RuntimeError("session broke mid-request")],
+        ) as mock_is_guest_user,
+        patch(
+            "superset.charts.data.api.security_manager.can_access",
+            return_value=False,
+        ),
+    ):
+        response = api._send_chart_response(result)
+
+    query = json.loads(response.get_data(as_text=True))["result"][0]
+    assert query["error"] == str(GENERIC_ERROR_MESSAGE)
+    assert "stacktrace" not in query
+    assert mock_is_guest_user.called
+
+
 def test_get_data_response_redacts_guest_query_failure(app: SupersetApp) -> None:
     command = MagicMock()
     command.execute.side_effect = ChartDataQueryFailedError(

tests/unit_tests/utils/test_error_sanitization.py

@@ -179,17 +179,84 @@ def test_access_status_selects_the_denial_message(
     assert sanitize_error_message(DB_ERROR, 404) == str(GENERIC_ERROR_MESSAGE)
 
 
-def test_unresolvable_principal_is_treated_as_non_guest(app: SupersetApp) -> None:
+def test_unresolvable_anonymous_principal_is_not_redacted(app: SupersetApp) -> None:
     """
     ``is_sanitization_required`` runs inside the HTTP error handler, which has no
     handler of its own: a raising user loader (e.g. a JWT request loader that
     raises when no credential is present) would otherwise turn every error
-    response into a bare 500. The lookup must swallow the failure and fall back
-    to treating the request as a non-guest one, leaving the message untouched.
+    response into a bare 500. The lookup must swallow the failure -- but instead
+    of guessing "not a guest" it falls back to whether the request carries a
+    guest token. A genuinely anonymous request (no token) is left untouched, so
+    ordinary failures are not over-sanitized. This is a deliberate
+    availability-over-confidentiality trade-off, not a definitional truth: an
+    unresolvable principal is not "by definition" a non-guest.
     """
-    with patch(
+    is_guest_user = patch(
         "superset.security.SupersetSecurityManager.is_guest_user",
         side_effect=RuntimeError("no valid credential on request"),
-    ):
+    )
+    with app.test_request_context("/"), is_guest_user as mock_is_guest_user:
         assert is_sanitization_required() is False
         assert sanitize_error_message(DB_ERROR) == DB_ERROR
+        assert mock_is_guest_user.called
+
+
+def test_unresolvable_principal_with_guest_token_is_redacted(app: SupersetApp) -> None:
+    """
+    Locks the fail-closed direction: a request that *carries* a guest token whose
+    resolution raises (e.g. ``find_role`` hitting a broken DB session while
+    resolving a valid token, or the ``EMBEDDED_SUPERSET`` feature hook raising)
+    must still be redacted. Reading the token header cannot raise, so the guard
+    fails closed for the one principal whose errors it exists to protect rather
+    than disclosing the raw engine error to a genuine embedded guest.
+    """
+    header = app.config["GUEST_TOKEN_HEADER_NAME"]
+    is_guest_user = patch(
+        "superset.security.SupersetSecurityManager.is_guest_user",
+        side_effect=RuntimeError("find_role on a broken session"),
+    )
+    with (
+        app.test_request_context("/", headers={header: "a.guest.token"}),
+        is_guest_user as mock_is_guest_user,
+    ):
+        assert is_sanitization_required() is True
+        assert sanitize_error_message(DB_ERROR) == str(GENERIC_ERROR_MESSAGE)
+        assert mock_is_guest_user.called
+
+
+def test_unresolvable_principal_with_form_guest_token_is_redacted(
+    app: SupersetApp,
+) -> None:
+    """The token can also arrive in the ``guest_token`` form field, not a header."""
+    is_guest_user = patch(
+        "superset.security.SupersetSecurityManager.is_guest_user",
+        side_effect=RuntimeError("find_role on a broken session"),
+    )
+    with (
+        app.test_request_context(
+            "/", method="POST", data={"guest_token": "a.guest.token"}
+        ),
+        is_guest_user as mock_is_guest_user,
+    ):
+        assert is_sanitization_required() is True
+        assert sanitize_error_message(DB_ERROR) == str(GENERIC_ERROR_MESSAGE)
+        assert mock_is_guest_user.called
+
+
+def test_unresolvable_principal_without_request_context_fails_closed(
+    app: SupersetApp,
+) -> None:
+    """
+    In a Celery worker there is no request context: ``sanitize_error_dicts`` runs
+    inside ``override_user`` while writing the async job payload delivered to the
+    embedded viewer. There is no token to read and no handler-of-a-handler
+    concern, so an unresolvable principal fails closed and redacts.
+    """
+    is_guest_user = patch(
+        "superset.security.SupersetSecurityManager.is_guest_user",
+        side_effect=RuntimeError("no request context in a worker"),
+    )
+    with app.app_context(), is_guest_user as mock_is_guest_user:
+        assert is_sanitization_required() is True
+        assert sanitize_error_message(DB_ERROR) == str(GENERIC_ERROR_MESSAGE)
+        assert mock_is_guest_user.called

tests/unit_tests/views/test_error_handling.py

@@ -338,6 +338,10 @@ class TestErrorHandlerNeverTurnsErrorsInto500s:
     user loaders do (e.g. a JWT request loader that raises when a request carries
     no valid credential) -- Flask discards the intended status and returns a bare
     500. The check must therefore swallow that failure and keep the real status.
+    Swallowing it is a deliberate availability-over-confidentiality trade-off: an
+    anonymous request (no guest token) keeps its status and message here, while a
+    request that carries a token is still redacted (covered in
+    ``test_error_sanitization``).
     """
 
     def _build_app_with_handlers(self) -> Flask:
@@ -384,3 +388,33 @@ class TestErrorHandlerNeverTurnsErrorsInto500s:
 
         assert response.status_code == 504
         assert json.loads(response.data)["error"] == "upstream took too long"
+
+    def test_unexpected_exception_returns_json_body_when_500_html_is_absent(
+        self,
+    ) -> None:
+        """
+        The last-resort ``show_unexpected_exception`` handler serves ``500.html``
+        for HTML clients, but that webpack artifact is absent in API-only/unbuilt
+        deployments. Like its siblings it must fall back to a SIP-40 JSON body
+        rather than let ``send_file`` raise ``FileNotFoundError`` and collapse the
+        response to a bare 500 with no body.
+        """
+        test_app = self._build_app_with_handlers()
+
+        @test_app.route("/boom")
+        def boom() -> FlaskResponse:
+            raise RuntimeError("something unexpected")
+
+        client = test_app.test_client()
+        with patch(
+            "superset.views.error_handling.send_file",
+            side_effect=FileNotFoundError,
+        ):
+            response = client.get("/boom", headers={"Accept": "text/html"})
+
+        assert response.status_code == 500
+        payload = json.loads(response.data)
+        assert (
+            payload["errors"][0]["error_type"]
+            == SupersetErrorType.GENERIC_BACKEND_ERROR.value
+        )