diff --git a/superset/common/query_context_processor.py b/superset/common/query_context_processor.py index 56c7ac4f5c8..bee3d6eb0df 100644 --- a/superset/common/query_context_processor.py +++ b/superset/common/query_context_processor.py @@ -541,10 +541,14 @@ class QueryContextProcessor: :raises SupersetSecurityException: If the user cannot access the resource """ - for query in self._query_context.queries: - query.validate() - + # Evaluate access before validating the queries: query validation + # renders the request's filter expressions, so the access decision must + # come first to avoid rendering caller-supplied input for a resource the + # caller is not allowed to access. if self._qc_datasource.type == DatasourceType.QUERY: security_manager.raise_for_access(query=self._qc_datasource) else: security_manager.raise_for_access(query_context=self._query_context) + + for query in self._query_context.queries: + query.validate() diff --git a/superset/jinja_context.py b/superset/jinja_context.py index f02dd7a299c..025b0cb0a61 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -735,6 +735,22 @@ def to_datetime( return datetime.strptime(value, format) +class SupersetSandboxedEnvironment(SandboxedEnvironment): + """ + Sandbox that denies attribute access to the base environment/template + classes and to the internals of ``functools.partial`` objects, none of + which templates need. Calling such objects is unaffected; only attribute + access is denied. + """ + + def is_safe_attribute(self, obj: Any, attr: str, value: Any) -> bool: + if attr in {"environment_class", "template_class"}: + return False + if isinstance(obj, partial): + return False + return super().is_safe_attribute(obj, attr, value) + + class BaseTemplateProcessor: """ Base class for database-specific jinja context @@ -765,7 +781,7 @@ class BaseTemplateProcessor: self._applied_filters = applied_filters self._removed_filters = removed_filters self._context: dict[str, Any] = {} - self.env: Environment = SandboxedEnvironment(undefined=DebugUndefined) + self.env: Environment = SupersetSandboxedEnvironment(undefined=DebugUndefined) self.set_context(**kwargs) # custom filters @@ -935,13 +951,14 @@ class JinjaTemplateProcessor(BaseTemplateProcessor): } ) - # The `metric` filter needs the full context, in order to expand other filters - self._context["metric"] = partial( - safe_proxy, - metric_macro, - self.env, - self._context, - ) + # The `metric` filter needs the env and full context to expand other + # filters. Bind them through a closure rather than positional args so the + # template environment is not reachable via the macro's public + # ``partial.args`` from inside a template. + def metric_with_context(metric_key: str, dataset_id: int | None = None) -> str: + return metric_macro(self.env, self._context, metric_key, dataset_id) + + self._context["metric"] = partial(safe_proxy, metric_with_context) class NoOpTemplateProcessor(BaseTemplateProcessor): diff --git a/tests/unit_tests/common/test_query_context_processor.py b/tests/unit_tests/common/test_query_context_processor.py index 56a5b3dd4f4..6e4acc08b41 100644 --- a/tests/unit_tests/common/test_query_context_processor.py +++ b/tests/unit_tests/common/test_query_context_processor.py @@ -1821,3 +1821,37 @@ def test_get_df_payload_no_warning_when_not_memory_limited() -> None: result = processor.get_df_payload(query_obj, force_cached=False) assert result["warning"] is None + + +def test_raise_for_access_evaluates_access_before_validate(): + """ + Access must be evaluated before the queries are validated, because query + validation renders the request's filter expressions. When access is denied, + no query is validated (so caller-supplied input is never rendered). + """ + from superset.errors import ErrorLevel, SupersetError, SupersetErrorType + from superset.exceptions import SupersetSecurityException + from superset.utils.core import DatasourceType + + query = MagicMock() + query_context = MagicMock() + query_context.queries = [query] + query_context.datasource.type = DatasourceType.TABLE + + processor = QueryContextProcessor(query_context) + + denied = SupersetSecurityException( + SupersetError( + message="denied", + error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR, + level=ErrorLevel.ERROR, + ) + ) + with patch( + "superset.common.query_context_processor.security_manager.raise_for_access", + side_effect=denied, + ): + with pytest.raises(SupersetSecurityException): + processor.raise_for_access() + + query.validate.assert_not_called() diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index 2c15c69ef3e..5559dcac3a5 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -25,6 +25,7 @@ from flask import current_app from flask_appbuilder.security.sqla.models import Role from freezegun import freeze_time from jinja2 import DebugUndefined +from jinja2.exceptions import SecurityError from jinja2.sandbox import SandboxedEnvironment from pytest_mock import MockerFixture from sqlalchemy.dialects import mysql @@ -992,6 +993,40 @@ def test_metric_macro_expansion(mocker: MockerFixture) -> None: assert processor.process_template("{{ metric('c') }}") == "42" +def test_metric_macro_does_not_expose_environment(mocker: MockerFixture) -> None: + """ + A template must not be able to read the template environment through the + ``metric`` macro's bound arguments. + """ + database = Database(id=1, database_name="my_database", sqlalchemy_uri="sqlite://") + mock_g = mocker.patch("superset.jinja_context.g") + mock_g.form_data = {"datasource": {"id": 1}} + processor = get_template_processor(database=database) + # Attribute access on the macro's partial is denied, so a reference to its + # bound args resolves to an undefined and any further use raises instead of + # yielding the environment object. + with pytest.raises(SecurityError): + processor.process_template("{{ metric.args[1] }}") + with pytest.raises(SecurityError): + processor.process_template( + "{{ metric.args[1].template_class.environment_class() }}" + ) + + +def test_supersetsandboxedenvironment_denies_unsafe_attributes() -> None: + """is_safe_attribute denies env/template class attrs and all attrs on partials.""" + from functools import partial + + from superset.jinja_context import SupersetSandboxedEnvironment + + env = SupersetSandboxedEnvironment() + assert env.is_safe_attribute(env, "environment_class", None) is False + assert env.is_safe_attribute(env, "template_class", None) is False + macro = partial(lambda value: value, 1) + assert env.is_safe_attribute(macro, "args", macro.args) is False + assert env.is_safe_attribute(macro, "func", macro.func) is False + + def test_metric_macro_recursive_compound(mocker: MockerFixture) -> None: """ Test the ``metric_macro`` when the definition is compound.