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"