From be0d9bc9e3fd4e55cbbafa51eb333b575f062a5e Mon Sep 17 00:00:00 2001
From: Evan <evan@preset.io>
Date: Tue, 23 Jun 2026 05:51:00 -0700
Subject: [PATCH] fix(embedded): include all_columns from stored query context
 in guest sort targets

Address review feedback: the stored query context loop in
_collect_sortable_identifiers omitted all_columns, silently rejecting
legitimate guest sorts on table charts configured with 'show all
columns'. Add it, plus a regression test and clarifying comments on the
order-by union and the intentional stored_values asymmetry.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 superset/security/manager.py              | 10 +++++++-
 tests/unit_tests/security/manager_test.py | 29 +++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/superset/security/manager.py b/superset/security/manager.py
index 759a88e4e22..8846be1deea 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -555,7 +555,7 @@ def _collect_sortable_identifiers(
 
     if stored_query_context:
         for query in stored_query_context.get("queries") or []:
-            for key in ("columns", "groupby", "metrics"):
+            for key in ("columns", "groupby", "metrics", "all_columns"):
                 add(query.get(key))
             add_orderby(query.get("orderby"))
 
@@ -577,6 +577,10 @@ def _orderby_modified(
     """
     allowed = _collect_sortable_identifiers(stored_chart, stored_query_context)
     form_data = query_context.form_data or {}
+    # Both ``form_data`` and each ``QueryObject`` can carry an order-by, and in
+    # the common frontend path they carry the same one. Either source could
+    # smuggle an unauthorized term, so validate the union of both rather than
+    # trusting one over the other; the duplication is harmless.
     requested = list(form_data.get("orderby") or [])
     for query in query_context.queries:
         requested.extend(getattr(query, "orderby", None) or [])
@@ -617,6 +621,10 @@ def _columns_metrics_modified(
         stored_values = {
             freeze_value(value) for value in stored_chart.params_dict.get(key) or []
         }
+        # ``form_data`` values are checked against ``params_dict`` alone;
+        # ``query_context`` values are checked below against the fuller set that
+        # also includes the stored query context. This asymmetry is intentional:
+        # each requested source is compared to its corresponding stored source.
         if not requested_values.issubset(stored_values):
             return True
 
diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py
index 7c28840aaf6..d4c3aa8fe9c 100644
--- a/tests/unit_tests/security/manager_test.py
+++ b/tests/unit_tests/security/manager_test.py
@@ -31,6 +31,8 @@ from superset.exceptions import SupersetSecurityException
 from superset.extensions import appbuilder
 from superset.models.slice import Slice
 from superset.security.manager import (
+    _collect_sortable_identifiers,
+    freeze_value,
     query_context_modified,
     SupersetSecurityManager,
 )
@@ -1314,6 +1316,33 @@ def test_query_context_modified_orderby_malformed_entry(
     assert query_context_modified(query_context)
 
 
+def test_query_context_modified_orderby_sort_by_stored_qc_only_column(
+    mocker: MockerFixture,
+) -> None:
+    """A column present only in the stored query context is an allowed sort target."""
+    query_context = _table_sort_query_context(mocker, orderby=[("age", True)])
+    # "age" is absent from params_dict but exposed via the stored query context,
+    # so sorting by it must be allowed.
+    query_context.slice_.params_dict = {"groupby": ["gender"], "metrics": ["count"]}
+    query_context.slice_.query_context = json.dumps(
+        {"queries": [{"columns": ["gender", "age"], "metrics": ["count"]}]}
+    )
+    assert not query_context_modified(query_context)
+
+
+def test_collect_sortable_identifiers_includes_stored_qc_all_columns(
+    mocker: MockerFixture,
+) -> None:
+    """``all_columns`` exposed via the stored query context is a valid sort target."""
+    stored_chart = mocker.MagicMock()
+    stored_chart.params_dict = {"groupby": ["gender"], "metrics": ["count"]}
+    allowed = _collect_sortable_identifiers(
+        stored_chart,
+        {"queries": [{"all_columns": ["gender", "age"], "metrics": ["count"]}]},
+    )
+    assert freeze_value("age") in allowed
+
+
 def test_query_context_modified_time_grain_native_filter(
     mocker: MockerFixture,
 ) -> None: