fix: prevent guest users from changing columns

superset/security/manager.py

@@ -145,9 +145,9 @@ def __init__(self, **kwargs: Any) -> None:
 RoleModelView.related_views = []
 
 
-def freeze_metric(metric: Metric) -> str:
+def freeze_value(metric: Metric) -> str:
     """
-    Used to compare metric sets.
+    Used to compare column and metric sets.
     """
     return json.dumps(metric, sort_keys=True)
 
@@ -170,32 +170,37 @@ def query_context_modified(query_context: "QueryContext") -> bool:
     if form_data.get("slice_id") != stored_chart.id:
         return True
 
-    # compare form_data
-    requested_metrics = {
-        freeze_metric(metric) for metric in form_data.get("metrics") or []
-    }
-    stored_metrics = {
-        freeze_metric(metric)
-        for metric in stored_chart.params_dict.get("metrics") or []
-    }
-    if not requested_metrics.issubset(stored_metrics):
-        return True
+    stored_query_context = (
+        json.loads(cast(str, stored_chart.query_context))
+        if stored_chart.query_context
+        else None
+    )
 
-    # compare queries in query_context
-    queries_metrics = {
-        freeze_metric(metric)
-        for query in query_context.queries
-        for metric in query.metrics or []
-    }
+    # compare columns and metrics in form_data with stored values
+    for key in ["metrics", "columns"]:
+        requested_values = {freeze_value(value) for value in form_data.get(key) or []}
+        stored_values = {
+            freeze_value(value) for value in stored_chart.params_dict.get(key) or []
+        }
+        if not requested_values.issubset(stored_values):
+            return True
 
-    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(
-                {freeze_metric(metric) for metric in query.get("metrics") or []}
-            )
+        # compare queries in query_context
+        queries_values = {
+            freeze_value(value)
+            for query in query_context.queries
+            for value in getattr(query, key) or []
+        }
+        if stored_query_context:
+            for query in stored_query_context.get("queries") or []:
+                stored_values.update(
+                    {freeze_value(value) for value in query.get(key) or []}
+                )
+
+        if not queries_values.issubset(stored_values):
+            return True
 
-    return not queries_metrics.issubset(stored_metrics)
+    return False
 
 
 class SupersetSecurityManager(  # pylint: disable=too-many-public-methods

tests/unit_tests/security/manager_test.py

@@ -31,7 +31,7 @@
     SupersetSecurityManager,
 )
 from superset.sql_parse import Table
-from superset.superset_typing import AdhocMetric
+from superset.superset_typing import AdhocColumn, AdhocMetric
 from superset.utils.core import override_user
 
 
@@ -59,10 +59,24 @@ def stored_metrics() -> list[AdhocMetric]:
     ]
 
 
+@pytest.fixture
+def stored_columns() -> list[AdhocColumn]:
+    """
+    Return a list of columns.
+    """
+    return [
+        {
+            "label": "My column",
+            "sqlExpression": "UPPER(name)",
+        },
+    ]
+
+
 def test_raise_for_access_guest_user_ok(
     mocker: MockerFixture,
     app_context: None,
     stored_metrics: list[AdhocMetric],
+    stored_columns: list[AdhocColumn],
 ) -> None:
     """
     Test that guest user can submit an unmodified chart payload.
@@ -76,11 +90,43 @@ def test_raise_for_access_guest_user_ok(
     query_context.slice_.query_context = None
     query_context.slice_.params_dict = {
         "metrics": stored_metrics,
+        "columns": stored_columns,
     }
 
     query_context.form_data = {
         "slice_id": 42,
         "metrics": stored_metrics,
+        "columns": stored_columns,
+    }
+    query_context.queries = [QueryObject(metrics=stored_metrics)]  # type: ignore
+    sm.raise_for_access(query_context=query_context)
+
+
+def test_raise_for_access_guest_user_ok_subset(
+    mocker: MockerFixture,
+    app_context: None,
+    stored_metrics: list[AdhocMetric],
+    stored_columns: list[AdhocColumn],
+) -> None:
+    """
+    Test that guest user can submit a request of a subset of the metrics/columns.
+    """
+    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,
+        "columns": stored_columns,
+    }
+
+    query_context.form_data = {
+        "slice_id": 42,
+        "metrics": [],
+        "columns": [],
     }
     query_context.queries = [QueryObject(metrics=stored_metrics)]  # type: ignore
     sm.raise_for_access(query_context=query_context)
@@ -114,7 +160,7 @@ def test_raise_for_access_guest_user_tampered_id(
         sm.raise_for_access(query_context=query_context)
 
 
-def test_raise_for_access_guest_user_tampered_form_data(
+def test_raise_for_access_guest_user_tampered_form_data_metrics(
     mocker: MockerFixture,
     app_context: None,
     stored_metrics: list[AdhocMetric],
@@ -151,7 +197,42 @@ def test_raise_for_access_guest_user_tampered_form_data(
         sm.raise_for_access(query_context=query_context)
 
 
-def test_raise_for_access_guest_user_tampered_queries(
+def test_raise_for_access_guest_user_tampered_form_data_columns(
+    mocker: MockerFixture,
+    app_context: None,
+    stored_columns: list[AdhocColumn],
+) -> None:
+    """
+    Test that guest user cannot modify columns 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 = {
+        "columns": stored_columns,
+    }
+
+    tampered_columns = [
+        {
+            "label": "My column",
+            "sqlExpression": "list_secret()",
+            "expressionType": "SQL",
+        },
+    ]
+
+    query_context.form_data = {
+        "slice_id": 42,
+        "columns": tampered_columns,
+    }
+    with pytest.raises(SupersetSecurityException):
+        sm.raise_for_access(query_context=query_context)
+
+
+def test_raise_for_access_guest_user_tampered_queries_metrics(
     mocker: MockerFixture,
     app_context: None,
     stored_metrics: list[AdhocMetric],
@@ -189,6 +270,42 @@ def test_raise_for_access_guest_user_tampered_queries(
         sm.raise_for_access(query_context=query_context)
 
 
+def test_raise_for_access_guest_user_tampered_queries_columns(
+    mocker: MockerFixture,
+    app_context: None,
+    stored_columns: list[AdhocColumn],
+) -> None:
+    """
+    Test that guest user cannot modify columns 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 = {
+        "columns": stored_columns,
+    }
+
+    tampered_columns = [
+        {
+            "label": "My column",
+            "sqlExpression": "list_secret()",
+            "expressionType": "SQL",
+        }
+    ]
+
+    query_context.form_data = {
+        "slice_id": 42,
+        "columns": stored_columns,
+    }
+    query_context.queries = [QueryObject(metrics=tampered_columns)]  # type: ignore
+    with pytest.raises(SupersetSecurityException):
+        sm.raise_for_access(query_context=query_context)
+
+
 def test_raise_for_access_query_default_schema(
     mocker: MockerFixture,
     app_context: None,