You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by jl...@apache.org on 2026/09/04 04:18:50 UTC
(superset) branch master updated: fix(embedded): don't let guest-user check turn error responses into 500s (#43834)
This is an automated email from the ASF dual-hosted git repository.
sadpandajoe pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new cceb52e776f fix(embedded): don't let guest-user check turn error responses into 500s (#43834)
cceb52e776f is described below
commit cceb52e776fa0b67138ce7e7ee43e28bf603d76e
Author: Elizabeth Thompson <es...@gmail.com>
AuthorDate: Thu Sep 3 21:18:43 2026 -0700
fix(embedded): don't let guest-user check turn error responses into 500s (#43834)
Signed-off-by: Elizabeth Thompson <es...@gmail.com>
Co-authored-by: Claude Opus 4.8 <no...@anthropic.com>
---
superset/utils/error_sanitization.py | 22 ++++++++-
tests/unit_tests/utils/test_error_sanitization.py | 17 +++++++
tests/unit_tests/views/test_error_handling.py | 56 +++++++++++++++++++++++
3 files changed, 94 insertions(+), 1 deletion(-)
diff --git a/superset/utils/error_sanitization.py b/superset/utils/error_sanitization.py
index 1ec37635d4c..06b63434442 100644
--- a/superset/utils/error_sanitization.py
+++ b/superset/utils/error_sanitization.py
@@ -27,12 +27,15 @@ message unless the error is one Superset authored itself.
from __future__ import annotations
import dataclasses
+import logging
from typing import Any
from flask_babel import lazy_gettext as _
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+logger = logging.getLogger(__name__)
+
GENERIC_ERROR_MESSAGE = _("An error occurred while fetching the data.")
GENERIC_ACCESS_MESSAGE = _("You don't have permission to access this resource.")
@@ -78,11 +81,28 @@ 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.
"""
# pylint: disable=import-outside-toplevel
from superset import security_manager
- return security_manager.is_guest_user()
+ try:
+ return security_manager.is_guest_user()
+ except Exception: # pylint: disable=broad-except
+ # 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.",
+ exc_info=True,
+ )
+ return False
def sanitize_error_message(message: str, status: int | None = None) -> str:
diff --git a/tests/unit_tests/utils/test_error_sanitization.py b/tests/unit_tests/utils/test_error_sanitization.py
index 5b6c82a847f..ff34f3514fd 100644
--- a/tests/unit_tests/utils/test_error_sanitization.py
+++ b/tests/unit_tests/utils/test_error_sanitization.py
@@ -25,6 +25,7 @@ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.utils.error_sanitization import (
GENERIC_ACCESS_MESSAGE,
GENERIC_ERROR_MESSAGE,
+ is_sanitization_required,
sanitize_error_dicts,
sanitize_error_message,
sanitize_superset_error,
@@ -176,3 +177,19 @@ def test_access_status_selects_the_denial_message(
) -> None:
assert sanitize_error_message("Forbidden", 403) == str(GENERIC_ACCESS_MESSAGE)
assert sanitize_error_message(DB_ERROR, 404) == str(GENERIC_ERROR_MESSAGE)
+
+
+def test_unresolvable_principal_is_treated_as_non_guest(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.
+ """
+ with patch(
+ "superset.security.SupersetSecurityManager.is_guest_user",
+ side_effect=RuntimeError("no valid credential on request"),
+ ):
+ assert is_sanitization_required() is False
+ assert sanitize_error_message(DB_ERROR) == DB_ERROR
diff --git a/tests/unit_tests/views/test_error_handling.py b/tests/unit_tests/views/test_error_handling.py
index 71c95e33c99..aae3b16c808 100644
--- a/tests/unit_tests/views/test_error_handling.py
+++ b/tests/unit_tests/views/test_error_handling.py
@@ -25,6 +25,7 @@ import sshtunnel
from flask import Flask, Response
from flask_babel import Babel
from sqlalchemy.exc import IntegrityError, OperationalError, ProgrammingError
+from werkzeug.exceptions import GatewayTimeout
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import QueryObjectValidationError, SupersetException
@@ -328,3 +329,58 @@ class TestGuestErrorSanitization:
)
assert payload["error"] == str(GENERIC_ERROR_MESSAGE)
+
+
+class TestErrorHandlerNeverTurnsErrorsInto500s:
+ """
+ The guest-user check runs *inside* the HTTP error handler, which has no
+ handler of its own. If resolving the principal raises -- as some Flask-Login
+ 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.
+ """
+
+ def _build_app_with_handlers(self) -> Flask:
+ # A fresh, minimal Flask app per test: `set_app_error_handlers` can only
+ # register handlers before the app has served its first request.
+ test_app = Flask(__name__)
+ test_app.config["DEBUG"] = False
+ Babel(test_app)
+ set_app_error_handlers(test_app)
+
+ @test_app.route("/gateway-timeout")
+ def gateway_timeout_view() -> FlaskResponse:
+ raise GatewayTimeout("upstream took too long")
+
+ return test_app
+
+ def test_gateway_timeout_keeps_its_status_when_principal_cannot_be_resolved(
+ self,
+ ) -> None:
+ client = self._build_app_with_handlers().test_client()
+
+ with patch(
+ "superset.security.SupersetSecurityManager.is_guest_user",
+ side_effect=RuntimeError("no valid credential on request"),
+ ):
+ response = client.get("/gateway-timeout")
+
+ # Without the guard the raising loader propagates out of the handler and
+ # Flask rewrites this to a bare 500.
+ assert response.status_code == 504
+
+ def test_direct_json_error_response_keeps_its_status(self, app: Flask) -> None:
+ with (
+ app.test_request_context(),
+ patch(
+ "superset.security.SupersetSecurityManager.is_guest_user",
+ side_effect=RuntimeError("no valid credential on request"),
+ ),
+ ):
+ response = cast(
+ Response,
+ json_error_response("upstream took too long", status=504),
+ )
+
+ assert response.status_code == 504
+ assert json.loads(response.data)["error"] == "upstream took too long"