You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/06/19 04:47:22 UTC
(superset) 01/01: fix(embedded): allow guest users to sort table columns
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/guest-embedded-table-sort
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 27c481d75d989a8b36cfc5b87e1425a7e7b3066b
Author: Claude Code <no...@anthropic.com>
AuthorDate: Thu Jun 18 21:46:53 2026 -0700
fix(embedded): allow guest users to sort table columns
query_context_modified() compared a guest request's orderby against the
stored chart's orderby with a strict subset check, alongside columns,
metrics and groupby. Saved charts usually store an empty orderby, so when
a guest clicked a column header to sort a table in an embedded dashboard
the new orderby value failed the subset check and the request was
rejected with "Guest user cannot modify chart payload", even though
sorting only changes result ordering and not which data is read.
Handle orderby separately: a guest may sort by any column or metric the
stored chart already references, but order-by terms that are not present
in the stored chart (e.g. a free-form random() expression) are still
rejected, preserving the existing SQL-injection guard. The columns/
metrics/groupby comparison is extracted into a helper to keep
query_context_modified within the complexity budget, and the inner loop's
variable shadowing of `key` is fixed along the way.
Co-Authored-By: Claude Fable 5 <no...@anthropic.com>
---
superset/security/manager.py | 145 ++++++++++++++++++++++++------
tests/unit_tests/security/manager_test.py | 72 ++++++++++++++-
2 files changed, 189 insertions(+), 28 deletions(-)
diff --git a/superset/security/manager.py b/superset/security/manager.py
index c8a44a6bffb..30be689c446 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -520,42 +520,87 @@ def _native_filter_request_modified(query_context: "QueryContext") -> bool:
)
-def query_context_modified(query_context: "QueryContext") -> bool:
+def _collect_sortable_identifiers(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
"""
- Check if a query context has been modified.
-
- This is used to ensure guest users don't modify the payload and fetch data
- different from what was shared with them in dashboards.
+ Frozen column names and metric labels/definitions a guest may legitimately
+ sort by: every column or metric the stored chart already references.
+
+ Order-by only changes the ordering of the result, not which data is read, so
+ any column or metric already part of the chart is a safe sort target. A term
+ that is not present in the stored chart (for example a free-form ``random()``
+ expression) cannot be validated and must be rejected. Order-by entries are
+ ``(column_or_metric, ascending)`` pairs, so only their first element is
+ collected.
"""
- form_data = query_context.form_data
- stored_chart = query_context.slice_
+ allowed: set[str] = set()
- # Native-filter data requests have no associated chart (no slice_id). Rather
- # than accepting any payload, constrain them to the column(s) the dashboard's
- # native filter is allowed to target; other chartless paths keep prior
- # behavior (see _native_filter_request_modified).
- if stored_chart is None:
- return _native_filter_request_modified(query_context)
+ def add(values: Any) -> None:
+ for value in values or []:
+ allowed.add(freeze_value(value))
- if form_data is None:
- return False
+ def add_orderby(entries: Any) -> None:
+ for entry in entries or []:
+ if isinstance(entry, (list, tuple)) and entry:
+ allowed.add(freeze_value(entry[0]))
- # cannot request a different chart
- if form_data.get("slice_id") != stored_chart.id:
- return True
+ params = stored_chart.params_dict
+ for key in ("columns", "groupby", "metrics", "all_columns"):
+ add(params.get(key))
+ add_orderby(params.get("orderby"))
- stored_query_context = (
- json.loads(cast(str, stored_chart.query_context))
- if stored_chart.query_context
- else None
- )
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ for key in ("columns", "groupby", "metrics"):
+ add(query.get(key))
+ add_orderby(query.get("orderby"))
+
+ return allowed
- # compare columns and metrics in form_data with stored values
+
+def _orderby_modified(
+ query_context: "QueryContext",
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+ """
+ Whether any order-by clause sorts by a term the stored chart does not already
+ reference.
+
+ A guest reordering an embedded chart by one of its existing columns or
+ metrics is legitimate and must not read as tampering; introducing a new
+ expression is not, and is rejected.
+ """
+ allowed = _collect_sortable_identifiers(stored_chart, stored_query_context)
+ form_data = query_context.form_data or {}
+ requested = list(form_data.get("orderby") or [])
+ for query in query_context.queries:
+ requested.extend(getattr(query, "orderby", None) or [])
+
+ for entry in requested:
+ term = entry[0] if isinstance(entry, (list, tuple)) and entry else entry
+ if freeze_value(term) not in allowed:
+ return True
+ return False
+
+
+def _columns_metrics_modified(
+ query_context: "QueryContext",
+ form_data: dict[str, Any],
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+ """
+ Whether the requested columns/metrics/group-by read beyond what the stored
+ chart exposes. Each requested set must be a subset of the values stored on
+ the chart (params and, when present, the stored query context).
+ """
for key, equivalent in [
("metrics", ["metrics"]),
("columns", ["columns", "groupby"]),
("groupby", ["columns", "groupby"]),
- ("orderby", ["orderby"]),
]:
requested_values = {freeze_value(value) for value in form_data.get(key) or []}
stored_values = {
@@ -572,9 +617,9 @@ def query_context_modified(query_context: "QueryContext") -> bool:
}
if stored_query_context:
for query in stored_query_context.get("queries") or []:
- for key in equivalent:
+ for equiv_key in equivalent:
stored_values.update(
- {freeze_value(value) for value in query.get(key) or []}
+ {freeze_value(value) for value in query.get(equiv_key) or []}
)
if not queries_values.issubset(stored_values):
@@ -583,6 +628,52 @@ def query_context_modified(query_context: "QueryContext") -> bool:
return False
+def query_context_modified(query_context: "QueryContext") -> bool:
+ """
+ Check if a query context has been modified.
+
+ This is used to ensure guest users don't modify the payload and fetch data
+ different from what was shared with them in dashboards.
+ """
+ form_data = query_context.form_data
+ stored_chart = query_context.slice_
+
+ # Native-filter data requests have no associated chart (no slice_id). Rather
+ # than accepting any payload, constrain them to the column(s) the dashboard's
+ # native filter is allowed to target; other chartless paths keep prior
+ # behavior (see _native_filter_request_modified).
+ if stored_chart is None:
+ return _native_filter_request_modified(query_context)
+
+ if form_data is None:
+ return False
+
+ # cannot request a different chart
+ if form_data.get("slice_id") != stored_chart.id:
+ return True
+
+ stored_query_context = (
+ json.loads(cast(str, stored_chart.query_context))
+ if stored_chart.query_context
+ else None
+ )
+
+ # compare columns and metrics in form_data with stored values. Order-by is
+ # handled separately: a strict subset check there would reject a guest
+ # legitimately sorting an embedded chart by one of its existing columns.
+ if _columns_metrics_modified(
+ query_context, form_data, stored_chart, stored_query_context
+ ):
+ return True
+
+ # Order-by may sort only by columns/metrics already present in the stored
+ # chart; new expressions (e.g. ``random()``) are still rejected.
+ if _orderby_modified(query_context, stored_chart, stored_query_context):
+ return True
+
+ return False
+
+
class SupersetSecurityManager( # pylint: disable=too-many-public-methods
SecurityManager
):
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index f4441630f68..aa60a5f9502 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -19,7 +19,7 @@
import json # noqa: TID251
from types import SimpleNamespace
-from typing import Any
+from typing import Any, Optional
import pytest
from flask_appbuilder.security.sqla.models import Role, User
@@ -1218,6 +1218,76 @@ def test_query_context_modified_orderby(mocker: MockerFixture) -> None:
assert query_context_modified(query_context)
+def _table_sort_query_context(
+ mocker: MockerFixture,
+ orderby: list[Any],
+ *,
+ stored_metrics: Optional[list[Any]] = None,
+ with_stored_query_context: bool = True,
+) -> Any:
+ """
+ Build a minimal table-chart query context for a guest that sorts by
+ ``orderby``. The stored chart groups by ``gender`` and aggregates ``count``.
+ """
+ metrics: list[Any] = stored_metrics if stored_metrics is not None else ["count"]
+ query_context = mocker.MagicMock()
+ query_context.queries = [
+ QueryObject(columns=["gender"], metrics=metrics, orderby=orderby),
+ ]
+ query_context.form_data = {"slice_id": 101, "groupby": ["gender"]}
+ query_context.slice_.id = 101
+ query_context.slice_.params_dict = {"groupby": ["gender"], "metrics": metrics}
+ query_context.slice_.query_context = (
+ json.dumps({"queries": [{"columns": ["gender"], "metrics": metrics}]})
+ if with_stored_query_context
+ else None
+ )
+ return query_context
+
+
+def test_query_context_modified_orderby_sort_by_column(mocker: MockerFixture) -> None:
+ """A guest sorting an embedded table by an existing column is allowed."""
+ query_context = _table_sort_query_context(mocker, orderby=[("gender", True)])
+ assert not query_context_modified(query_context)
+
+
+def test_query_context_modified_orderby_sort_by_metric(mocker: MockerFixture) -> None:
+ """A guest sorting by an existing metric is allowed."""
+ query_context = _table_sort_query_context(mocker, orderby=[("count", False)])
+ assert not query_context_modified(query_context)
+
+
+def test_query_context_modified_orderby_sort_by_adhoc_metric(
+ mocker: MockerFixture,
+) -> None:
+ """A guest sorting by an existing adhoc metric definition is allowed."""
+ adhoc_metric: AdhocMetric = {
+ "aggregate": "SUM",
+ "column": {"column_name": "num"},
+ "expressionType": "SIMPLE",
+ "hasCustomLabel": False,
+ "label": "SUM(num)",
+ }
+ query_context = _table_sort_query_context(
+ mocker,
+ orderby=[(adhoc_metric, False)],
+ stored_metrics=[adhoc_metric],
+ )
+ assert not query_context_modified(query_context)
+
+
+def test_query_context_modified_orderby_unknown_column(mocker: MockerFixture) -> None:
+ """A guest sorting by a column the chart does not reference is rejected."""
+ query_context = _table_sort_query_context(mocker, orderby=[("salary", True)])
+ assert query_context_modified(query_context)
+
+
+def test_query_context_modified_orderby_empty(mocker: MockerFixture) -> None:
+ """An empty order-by is not a modification."""
+ query_context = _table_sort_query_context(mocker, orderby=[])
+ assert not query_context_modified(query_context)
+
+
def test_query_context_modified_time_grain_native_filter(
mocker: MockerFixture,
) -> None: