You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by pm...@apache.org on 2026/06/13 18:02:00 UTC
(superset) branch master updated: fix(charts): evaluate access before rendering query filters; isolate macro env (#40982)
This is an automated email from the ASF dual-hosted git repository.
sha174n 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 e5986b32ffb fix(charts): evaluate access before rendering query filters; isolate macro env (#40982)
e5986b32ffb is described below
commit e5986b32ffb4a4f69e5554509c920e52fefe1348
Author: Shaitan <10...@users.noreply.github.com>
AuthorDate: Sat Jun 13 19:01:54 2026 +0100
fix(charts): evaluate access before rendering query filters; isolate macro env (#40982)
Co-authored-by: sha174n <pe...@preset.io>
Co-authored-by: Claude Opus 4.8 (1M context) <no...@anthropic.com>
---
superset/common/query_context_processor.py | 10 +++++--
superset/jinja_context.py | 33 +++++++++++++++-----
.../common/test_query_context_processor.py | 34 +++++++++++++++++++++
tests/unit_tests/jinja_context_test.py | 35 ++++++++++++++++++++++
4 files changed, 101 insertions(+), 11 deletions(-)
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.