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

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,