You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by el...@apache.org on 2026/09/04 20:05:44 UTC
(superset) 01/01: fix(embedded): fail closed when the guest-user check can't resolve a principal
This is an automated email from the ASF dual-hosted git repository.
eschutho pushed a commit to branch followup-error-sanitization-fail-open
in repository https://gitbox.apache.org/repos/asf/superset.git
commit eee8f3b3f4d87897cbe30f4dfa9f2230133e432d
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Fri Sep 4 20:05:29 2026 +0000
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 <no...@anthropic.com>
Signed-off-by: Elizabeth Thompson <es...@gmail.com>
---
superset/charts/data/api.py | 17 +++-
superset/utils/error_sanitization.py | 98 ++++++++++++++++++-----
superset/views/error_handling.py | 10 ++-
tests/unit_tests/charts/test_chart_data_api.py | 45 +++++++++++
tests/unit_tests/utils/test_error_sanitization.py | 77 ++++++++++++++++--
tests/unit_tests/views/test_error_handling.py | 34 ++++++++
6 files changed, 251 insertions(+), 30 deletions(-)
diff --git a/superset/charts/data/api.py b/superset/charts/data/api.py
index 74a471d15d5..40b9d1c0415 100644
--- a/superset/charts/data/api.py
+++ b/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:
diff --git a/superset/utils/error_sanitization.py b/superset/utils/error_sanitization.py
index 06b63434442..e68ebf0ab1d 100644
--- a/superset/utils/error_sanitization.py
+++ b/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.
-
- 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.
+ Whether the current request's error details should be redacted.
+
+ 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,
)
)
)
diff --git a/superset/views/error_handling.py b/superset/views/error_handling.py
index 2985275307a..9d8a5f34863 100644
--- a/superset/views/error_handling.py
+++ b/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(
[
diff --git a/tests/unit_tests/charts/test_chart_data_api.py b/tests/unit_tests/charts/test_chart_data_api.py
index b613e7b9661..51800c99878 100644
--- a/tests/unit_tests/charts/test_chart_data_api.py
+++ b/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(
diff --git a/tests/unit_tests/utils/test_error_sanitization.py b/tests/unit_tests/utils/test_error_sanitization.py
index ff34f3514fd..82cd17fc013 100644
--- a/tests/unit_tests/utils/test_error_sanitization.py
+++ b/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
diff --git a/tests/unit_tests/views/test_error_handling.py b/tests/unit_tests/views/test_error_handling.py
index aae3b16c808..46a2d033895 100644
--- a/tests/unit_tests/views/test_error_handling.py
+++ b/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
+ )