diff --git a/superset/common/query_object.py b/superset/common/query_object.py index 989df5775b2e7..5109c465e0abe 100644 --- a/superset/common/query_object.py +++ b/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, diff --git a/superset/security/manager.py b/superset/security/manager.py index b29529edb2398..8268b19411b9c 100644 --- a/superset/security/manager.py +++ b/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 diff --git a/tests/integration_tests/security/guest_token_security_tests.py b/tests/integration_tests/security/guest_token_security_tests.py index 4cd8a77f7635c..b812929433c9b 100644 --- a/tests/integration_tests/security/guest_token_security_tests.py +++ b/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=[], ) } ) diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index 1843e7261cc9a..ad6e53e9930ce 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/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,