fix: prevent guest user from modifying metrics (apache#26749)

Author: betodealmeida

superset/common/query_object.py

@@ -125,7 +125,7 @@ def __init__(  # pylint: disable=too-many-locals
         order_desc: bool = True,
         orderby: list[OrderBy] | None = None,
         post_processing: list[dict[str, Any] | None] | None = None,
-        row_limit: int | None,
+        row_limit: int | None = None,
         row_offset: int | None = None,
         series_columns: list[Column] | None = None,
         series_limit: int = 0,

superset/security/manager.py

@@ -1819,7 +1819,7 @@ def get_exclude_users_from_lists() -> list[str]:
         return []
 
     def raise_for_access(
-        # pylint: disable=too-many-arguments,too-many-branches,too-many-locals
+        # pylint: disable=too-many-arguments,too-many-branches,too-many-locals,too-many-statements
         self,
         dashboard: Optional["Dashboard"] = None,
         database: Optional["Database"] = None,
@@ -1909,6 +1909,30 @@ def raise_for_access(
                     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,
+                    )
+                )
+
         if datasource or query_context or viz:
             form_data = None
 

tests/integration_tests/security/guest_token_security_tests.py

@@ -288,7 +288,10 @@ def test_raise_for_access__happy_path(self):
                         form_data={
                             "dashboardId": self.dash.id,
                             "slice_id": self.chart.id,
+                            "metrics": self.chart.params_dict["metrics"],
                         },
+                        slice_=self.chart,
+                        queries=[],
                     )
                 }
             )
@@ -304,7 +307,11 @@ def test_raise_for_access__native_filter_happy_path(self):
                             "dashboardId": self.dash.id,
                             "native_filter_id": "NATIVE_FILTER-ABCDEFGH",
                             "type": "NATIVE_FILTER",
+                            "slice_id": self.chart.id,
+                            "metrics": self.chart.params_dict["metrics"],
                         },
+                        slice_=self.chart,
+                        queries=[],
                     )
                 }
             )
@@ -382,7 +389,11 @@ def test_raise_for_access__native_filter_no_id_in_form_data(self):
                             form_data={
                                 "dashboardId": self.dash.id,
                                 "type": "NATIVE_FILTER",
+                                "slice_id": self.chart.id,
+                                "metrics": self.chart.params_dict["metrics"],
                             },
+                            slice_=self.chart,
+                            queries=[],
                         )
                     }
                 )
@@ -399,7 +410,11 @@ def test_raise_for_access__native_filter_datasource_not_associated(self):
                                 "dashboardId": self.dash.id,
                                 "native_filter_id": "NATIVE_FILTER-ABCDEFGH",
                                 "type": "NATIVE_FILTER",
+                                "slice_id": self.chart.id,
+                                "metrics": self.chart.params_dict["metrics"],
                             },
+                            slice_=self.chart,
+                            queries=[],
                         )
                     }
                 )

tests/unit_tests/security/manager_test.py

@@ -18,6 +18,7 @@
 import pytest
 from pytest_mock import MockFixture
 
+from superset.common.query_object import QueryObject
 from superset.exceptions import SupersetSecurityException
 from superset.extensions import appbuilder
 from superset.security.manager import SupersetSecurityManager
@@ -31,6 +32,81 @@ def test_security_manager(app_context: None) -> None:
     assert sm
 
 
+def test_raise_for_access_guest_user(
+    mocker: MockFixture,
+    app_context: None,
+) -> None:
+    """
+    Test that guest user can't modify 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_.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
+    query_context.form_data = {
+        "slice_id": 43,
+        "metrics": stored_metrics,
+    }
+    query_context.queries = [QueryObject(metrics=stored_metrics)]  # type: ignore
+    with pytest.raises(SupersetSecurityException):
+        sm.raise_for_access(query_context=query_context)
+
+    tampered_metrics = [
+        {
+            "aggregate": None,
+            "column": None,
+            "datasourceWarning": False,
+            "expressionType": "SQL",
+            "hasCustomLabel": False,
+            "label": "COUNT(*) + 2",
+            "optionName": "metric_ssa1gwimio_cxpyjc7vj3s",
+            "sqlExpression": "COUNT(*) + 2",
+        }
+    ]
+
+    query_context.form_data = {
+        "slice_id": 42,
+        "metrics": tampered_metrics,
+    }
+    with pytest.raises(SupersetSecurityException):
+        sm.raise_for_access(query_context=query_context)
+
+    query_context.form_data = {
+        "slice_id": 42,
+        "metrics": stored_metrics,
+    }
+    query_context.queries = [QueryObject(metrics=tampered_metrics)]  # type: ignore
+    with pytest.raises(SupersetSecurityException):
+        sm.raise_for_access(query_context=query_context)
+
+
 def test_raise_for_access_query_default_schema(
     mocker: MockFixture,
     app_context: None,