fix: check if guest user modified query

superset/security/manager.py

@@ -143,6 +143,56 @@
 RoleModelView.related_views = []
 
 
+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_
+
+    # sanity checks
+    if form_data is None or stored_chart is None:
+        return True
+
+    # cannot request a different chart
+    if form_data.get("slice_id") != stored_chart.id:
+        return True
+
+    # compare form_data
+    requested_metrics = {
+        frozenset(metric.items()) if isinstance(metric, dict) else metric
+        for metric in form_data.get("metrics") or []
+    }
+    stored_metrics = {
+        frozenset(metric.items()) if isinstance(metric, dict) else metric
+        for metric in stored_chart.params_dict.get("metrics") or []
+    }
+    if not requested_metrics.issubset(stored_metrics):
+        return True
+
+    # compare queries in query_context
+    queries_metrics = {
+        frozenset(metric.items()) if isinstance(metric, dict) else metric
+        for query in query_context.queries
+        for metric in query.metrics or []
+    }
+
+    if stored_chart.query_context:
+        stored_query_context = json.loads(cast(str, stored_chart.query_context))
+        for query in stored_query_context.get("queries") or []:
+            stored_metrics.update(
+                {
+                    frozenset(metric.items()) if isinstance(metric, dict) else metric
+                    for metric in query.get("metrics") or []
+                }
+            )
+
+    return not queries_metrics.issubset(stored_metrics)
+
+
 class SupersetSecurityManager(  # pylint: disable=too-many-public-methods
     SecurityManager
 ):
@@ -1941,29 +1991,20 @@
                     self.get_table_access_error_object(denied)
                 )
 
-        if self.is_guest_user() and query_context:
-            # Guest users MUST not modify the payload so it's requesting a different
-            # chart or different ad-hoc metrics from what's saved.
-            form_data = query_context.form_data
-            stored_chart = query_context.slice_
-
-            if (
-                form_data is None
-                or stored_chart is None
-                or form_data.get("slice_id") != stored_chart.id
-                or form_data.get("metrics", []) != stored_chart.params_dict["metrics"]
-                or any(
-                    query.metrics != stored_chart.params_dict["metrics"]
-                    for query in query_context.queries
-                )
-            ):
-                raise SupersetSecurityException(
-                    SupersetError(
-                        error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
-                        message=_("Guest user cannot modify chart payload"),
-                        level=ErrorLevel.ERROR,
-                    )
+        # Guest users MUST not modify the payload so it's requesting a
+        # different chart or different ad-hoc metrics from what's saved.
+        if (
+            query_context
+            and self.is_guest_user()
+            and query_context_modified(query_context)
+        ):
+            raise SupersetSecurityException(
+                SupersetError(
+                    error_type=SupersetErrorType.DASHBOARD_SECURITY_ACCESS_ERROR,
+                    message=_("Guest user cannot modify chart payload"),
+                    level=ErrorLevel.ERROR,
                 )
+            )
 
         if datasource or query_context or viz:
             form_data = None

tests/unit_tests/security/manager_test.py

@@ -15,6 +15,8 @@
 # specific language governing permissions and limitations
 # under the License.
 
+# pylint: disable=invalid-name, unused-argument, redefined-outer-name
+
 import pytest
 from flask_appbuilder.security.sqla.models import Role, User
 from pytest_mock import MockFixture
@@ -25,6 +27,7 @@
 from superset.extensions import appbuilder
 from superset.models.slice import Slice
 from superset.security.manager import SupersetSecurityManager
+from superset.superset_typing import AdhocMetric
 from superset.utils.core import override_user
 
 
@@ -36,44 +39,68 @@ def test_security_manager(app_context: None) -> None:
     assert sm
 
 
-def test_raise_for_access_guest_user(
+@pytest.fixture
+def stored_metrics() -> list[AdhocMetric]:
+    """
+    Return a list of metrics.
+    """
+    return [
+        {
+            "column": None,
+            "expressionType": "SQL",
+            "hasCustomLabel": False,
+            "label": "COUNT(*) + 1",
+            "sqlExpression": "COUNT(*) + 1",
+        },
+    ]
+
+
+def test_raise_for_access_guest_user_ok(
     mocker: MockFixture,
     app_context: None,
+    stored_metrics: list[AdhocMetric],
 ) -> None:
     """
-    Test that guest user can't modify chart payload.
+    Test that guest user can submit an unmodified chart payload.
     """
     sm = SupersetSecurityManager(appbuilder)
     mocker.patch.object(sm, "is_guest_user", return_value=True)
     mocker.patch.object(sm, "can_access", return_value=True)
 
     query_context = mocker.MagicMock()
     query_context.slice_.id = 42
-    stored_metrics = [
-        {
-            "aggregate": None,
-            "column": None,
-            "datasourceWarning": False,
-            "expressionType": "SQL",
-            "hasCustomLabel": False,
-            "label": "COUNT(*) + 1",
-            "optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
-            "sqlExpression": "COUNT(*) + 1",
-        }
-    ]
+    query_context.slice_.query_context = None
     query_context.slice_.params_dict = {
         "metrics": stored_metrics,
     }
 
-    # normal request
     query_context.form_data = {
         "slice_id": 42,
         "metrics": stored_metrics,
     }
     query_context.queries = [QueryObject(metrics=stored_metrics)]  # type: ignore
     sm.raise_for_access(query_context=query_context)
 
-    # tampered requests
+
+def test_raise_for_access_guest_user_tampered_id(
+    mocker: MockFixture,
+    app_context: None,
+    stored_metrics: list[AdhocMetric],
+) -> None:
+    """
+    Test that guest user cannot modify the chart ID.
+    """
+    sm = SupersetSecurityManager(appbuilder)
+    mocker.patch.object(sm, "is_guest_user", return_value=True)
+    mocker.patch.object(sm, "can_access", return_value=True)
+
+    query_context = mocker.MagicMock()
+    query_context.slice_.id = 42
+    query_context.slice_.query_context = None
+    query_context.slice_.params_dict = {
+        "metrics": stored_metrics,
+    }
+
     query_context.form_data = {
         "slice_id": 43,
         "metrics": stored_metrics,
@@ -82,15 +109,32 @@ def test_raise_for_access_guest_user(
     with pytest.raises(SupersetSecurityException):
         sm.raise_for_access(query_context=query_context)
 
+
+def test_raise_for_access_guest_user_tampered_form_data(
+    mocker: MockFixture,
+    app_context: None,
+    stored_metrics: list[AdhocMetric],
+) -> None:
+    """
+    Test that guest user cannot modify metrics in the form data.
+    """
+    sm = SupersetSecurityManager(appbuilder)
+    mocker.patch.object(sm, "is_guest_user", return_value=True)
+    mocker.patch.object(sm, "can_access", return_value=True)
+
+    query_context = mocker.MagicMock()
+    query_context.slice_.id = 42
+    query_context.slice_.query_context = None
+    query_context.slice_.params_dict = {
+        "metrics": stored_metrics,
+    }
+
     tampered_metrics = [
         {
-            "aggregate": None,
             "column": None,
-            "datasourceWarning": False,
             "expressionType": "SQL",
             "hasCustomLabel": False,
             "label": "COUNT(*) + 2",
-            "optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
             "sqlExpression": "COUNT(*) + 2",
         }
     ]
@@ -102,6 +146,36 @@ def test_raise_for_access_guest_user(
     with pytest.raises(SupersetSecurityException):
         sm.raise_for_access(query_context=query_context)
 
+
+def test_raise_for_access_guest_user_tampered_queries(
+    mocker: MockFixture,
+    app_context: None,
+    stored_metrics: list[AdhocMetric],
+) -> None:
+    """
+    Test that guest user cannot modify metrics in the queries.
+    """
+    sm = SupersetSecurityManager(appbuilder)
+    mocker.patch.object(sm, "is_guest_user", return_value=True)
+    mocker.patch.object(sm, "can_access", return_value=True)
+
+    query_context = mocker.MagicMock()
+    query_context.slice_.id = 42
+    query_context.slice_.query_context = None
+    query_context.slice_.params_dict = {
+        "metrics": stored_metrics,
+    }
+
+    tampered_metrics = [
+        {
+            "column": None,
+            "expressionType": "SQL",
+            "hasCustomLabel": False,
+            "label": "COUNT(*) + 2",
+            "sqlExpression": "COUNT(*) + 2",
+        }
+    ]
+
     query_context.form_data = {
         "slice_id": 42,
         "metrics": stored_metrics,