fix: check if guest user modified query (#27484)

superset/security/manager.py

@@ -143,6 +143,56 @@ def __init__(self, **kwargs: Any) -> None:
 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 @@ 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,
- )
+ # 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,