diff --git a/superset/security/manager.py b/superset/security/manager.py index 96acd3ea4618..ebbf0dbee6fd 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -347,6 +347,145 @@ def freeze_value(value: Any) -> str: return json.dumps(value, sort_keys=True) +def _native_filter_allowed_targets( + query_context: "QueryContext", form_data: dict[str, Any] +) -> Optional[tuple[set[str], set[str]]]: + """ + Return ``(allowed_columns, allowed_metrics)`` a native-filter data request + may read, or ``None`` when the request cannot be tied to a native filter on + the requesting dashboard (in which case the caller must fail closed). + + ``allowed_columns`` are the target column(s) of the filter identified by + ``native_filter_id`` that point at the request's datasource. ``allowed_metrics`` + are the saved-metric name(s) the filter is configured to sort its values by + (``controlValues.sortMetric``), which a legitimate value lookup sends. + """ + # pylint: disable=import-outside-toplevel + from superset import db + from superset.models.dashboard import Dashboard + + native_filter_id = form_data.get("native_filter_id") + dashboard_id = form_data.get("dashboardId") + if not native_filter_id or not dashboard_id: + return None + + dashboard = ( + db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one_or_none() + ) + if dashboard is None or not dashboard.json_metadata: + return None + try: + metadata = json.loads(dashboard.json_metadata) + except (TypeError, ValueError): + return None + + datasource = getattr(query_context, "datasource", None) + datasource_id = datasource.data.get("id") if datasource else None + + allowed_columns: set[str] = set() + allowed_metrics: set[str] = set() + for fltr in metadata.get("native_filter_configuration", []): + if fltr.get("id") != native_filter_id: + continue + for target in fltr.get("targets", []): + column = target.get("column") + if ( + target.get("datasetId") == datasource_id + and isinstance(column, dict) + and column.get("name") + ): + allowed_columns.add(column["name"]) + # The filter may be configured to sort its values by a saved metric; a + # legitimate value lookup then sends that metric name. + sort_metric = (fltr.get("controlValues") or {}).get("sortMetric") or fltr.get( + "sortMetric" + ) + if isinstance(sort_metric, str): + allowed_metrics.add(sort_metric) + # Filter ids are unique, so the matching filter is the only one. + break + + return allowed_columns, allowed_metrics + + +def _native_filter_term_allowed( + term: Any, allowed_columns: set[str], allowed_metrics: set[str] +) -> bool: + """ + Whether a value-returning term (metric or order-by expression) is allowed on + a native-filter request: a plain reference to a target column or the + configured sort metric, or a simple aggregate over a target column. Free-form + SQL terms and other saved metrics cannot be validated and are not allowed. + """ + if isinstance(term, str): + return term in allowed_columns or term in allowed_metrics + if isinstance(term, dict) and term.get("expressionType") == "SIMPLE": + return (term.get("column") or {}).get("column_name") in allowed_columns + return False + + +def _native_filter_query_modified( + query: Any, allowed_columns: set[str], allowed_metrics: set[str] +) -> bool: + """Whether a single query in a native-filter request reads beyond its targets.""" + # Columns and group-by may only reference target column(s); adhoc (free-form + # SQL) columns cannot be validated, so reject them. + for key in ("columns", "groupby"): + for col in getattr(query, key, None) or []: + if not isinstance(col, str) or col not in allowed_columns: + return True + for metric in getattr(query, "metrics", None) or []: + if not _native_filter_term_allowed(metric, allowed_columns, allowed_metrics): + return True + # order-by entries are ``(expression, asc)`` pairs. + for order in getattr(query, "orderby", None) or []: + expr = order[0] if isinstance(order, (list, tuple)) and order else order + if not _native_filter_term_allowed(expr, allowed_columns, allowed_metrics): + return True + return False + + +def _native_filter_request_modified(query_context: "QueryContext") -> bool: + """ + Validate a chartless data request that targets a native filter. + + Only requests identified as native-filter lookups (by the ``NATIVE_FILTER`` + type marker or a ``native_filter_id``) are constrained; other chartless + paths (drill-to-detail, drill-by, samples) carry neither and are validated by + the datasource-access checks in raise_for_access, so they are not treated as + modified here. + + A native filter may only read the column(s) it targets on the dashboard it + belongs to. The request is treated as modified (and therefore rejected for + guest users) when it cannot be tied to a native filter on the requesting + dashboard, or when any value-returning term (column, group-by, metric, or + order-by) references something other than a target column, a simple + aggregate over a target column, or the filter's configured sort metric. + Free-form SQL terms and saved metrics other than the configured sort metric + are rejected. Row-restricting clauses (``filter``/``extras``) are not + constrained here: cross-filters legitimately reference other columns and + they do not return column values; that blind-inference surface is a separate + concern shared with the chart path. + """ + form_data = query_context.form_data or {} + if not ( + form_data.get("type") == "NATIVE_FILTER" or form_data.get("native_filter_id") + ): + return False + targets = _native_filter_allowed_targets(query_context, form_data) + # Fail closed when the request cannot be tied to a native filter. + if targets is None: + return True + # Empty allowed sets (filter resolved but no matching column/metric target) + # intentionally deny every value-returning term below. + allowed_columns, allowed_metrics = targets + + return any( + _native_filter_query_modified(query, allowed_columns, allowed_metrics) + for query in query_context.queries + ) + + def query_context_modified(query_context: "QueryContext") -> bool: """ Check if a query context has been modified. @@ -357,8 +496,14 @@ def query_context_modified(query_context: "QueryContext") -> bool: form_data = query_context.form_data stored_chart = query_context.slice_ - # native filter requests - if form_data is None or stored_chart is None: + # Native-filter data requests have no associated chart (no slice_id). Rather + # than accepting any payload, constrain them to the column(s) the dashboard's + # native filter is allowed to target; other chartless paths keep prior + # behavior (see _native_filter_request_modified). + if stored_chart is None: + return _native_filter_request_modified(query_context) + + if form_data is None: return False # cannot request a different chart diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index f5e4190fa6bb..0d89bf88b8e1 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -19,6 +19,7 @@ import json # noqa: TID251 from types import SimpleNamespace +from typing import Any import pytest from flask_appbuilder.security.sqla.models import Role, User @@ -656,16 +657,188 @@ def test_query_context_modified_tampered( assert query_context_modified(query_context) -def test_query_context_modified_native_filter(mocker: MockerFixture) -> None: - """ - Test the `query_context_modified` function with a native filter request. +def _native_filter_ctx( + mocker: MockerFixture, + queries: list[Any], + *, + native_filter_id: str | None = "F1", + dashboard_id: int | None = 10, + dataset_id: int = 20, + targets: list[Any] | None = None, + control_values: dict[str, Any] | None = None, +) -> Any: + """Build a native-filter query context (no slice_) + patched dashboard.""" + if targets is None: + targets = [{"datasetId": dataset_id, "column": {"name": "region"}}] + qc = mocker.MagicMock() + qc.slice_ = None + qc.form_data = { + "type": "NATIVE_FILTER", + "native_filter_id": native_filter_id, + "dashboardId": dashboard_id, + } + qc.datasource.data = {"id": dataset_id} + qc.queries = queries + dash = mocker.MagicMock() + dash.json_metadata = json.dumps( + { + "native_filter_configuration": [ + { + "id": "F1", + "targets": targets, + "controlValues": control_values or {}, + } + ] + } + ) + query_chain = mocker.patch("superset.db.session.query") + query_chain.return_value.filter.return_value.one_or_none.return_value = dash + return qc + + +def test_query_context_modified_native_filter_target_column_allowed( + mocker: MockerFixture, +) -> None: + """A native-filter request reading only its target column is allowed.""" + query = SimpleNamespace(columns=["region"], metrics=[], groupby=[]) + qc = _native_filter_ctx(mocker, [query]) + assert not query_context_modified(qc) + + +def test_query_context_modified_native_filter_arbitrary_column_blocked( + mocker: MockerFixture, +) -> None: + """A native-filter request reading a non-target column is modified.""" + query = SimpleNamespace(columns=["ssn"], metrics=[], groupby=[]) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + +def test_query_context_modified_native_filter_simple_metric_on_target_allowed( + mocker: MockerFixture, +) -> None: + """A range-style request (simple aggregate over the target column) is allowed.""" + query = SimpleNamespace( + columns=[], + metrics=[ + { + "expressionType": "SIMPLE", + "column": {"column_name": "region"}, + "aggregate": "MIN", + } + ], + groupby=[], + ) + qc = _native_filter_ctx(mocker, [query]) + assert not query_context_modified(qc) + + +def test_query_context_modified_native_filter_adhoc_metric_blocked( + mocker: MockerFixture, +) -> None: + """A free-form SQL metric on the native-filter path is modified.""" + query = SimpleNamespace( + columns=[], + metrics=[{"expressionType": "SQL", "sqlExpression": "SUM(salary)"}], + groupby=[], + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + +def test_query_context_modified_native_filter_adhoc_column_blocked( + mocker: MockerFixture, +) -> None: + """An adhoc (free-form SQL) column on the native-filter path is modified.""" + query = SimpleNamespace( + columns=[{"sqlExpression": "ssn", "label": "x"}], metrics=[], groupby=[] + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) - A native filter request has no chart (slice) associated with it. + +def test_query_context_modified_native_filter_no_filter_context_blocked( + mocker: MockerFixture, +) -> None: + """Without a native_filter_id / dashboardId the request fails closed.""" + query = SimpleNamespace(columns=["region"], metrics=[], groupby=[]) + qc = _native_filter_ctx(mocker, [query], native_filter_id=None) + assert query_context_modified(qc) + + +def test_query_context_modified_native_filter_configured_sort_metric_allowed( + mocker: MockerFixture, +) -> None: + """A value lookup sorted by the filter's configured saved metric is allowed.""" + query = SimpleNamespace( + columns=["region"], + metrics=["total"], + groupby=[], + orderby=[["total", True]], + ) + qc = _native_filter_ctx(mocker, [query], control_values={"sortMetric": "total"}) + assert not query_context_modified(qc) + + +def test_query_context_modified_native_filter_arbitrary_saved_metric_blocked( + mocker: MockerFixture, +) -> None: + """A saved metric other than the filter's configured sort metric is modified.""" + query = SimpleNamespace(columns=["region"], metrics=["salary_total"], groupby=[]) + qc = _native_filter_ctx(mocker, [query], control_values={"sortMetric": "total"}) + assert query_context_modified(qc) + + +def test_query_context_modified_native_filter_orderby_arbitrary_column_blocked( + mocker: MockerFixture, +) -> None: + """Ordering by a non-target column on the native-filter path is modified.""" + query = SimpleNamespace( + columns=["region"], metrics=[], groupby=[], orderby=[["ssn", True]] + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + +def test_query_context_modified_native_filter_orderby_adhoc_blocked( + mocker: MockerFixture, +) -> None: + """Ordering by a free-form SQL expression on the native-filter path is modified.""" + query = SimpleNamespace( + columns=["region"], + metrics=[], + groupby=[], + orderby=[[{"sqlExpression": "ssn"}, True]], + ) + qc = _native_filter_ctx(mocker, [query]) + assert query_context_modified(qc) + + +def test_query_context_modified_chartless_non_native_filter_allowed( + mocker: MockerFixture, +) -> None: """ - query_context = mocker.MagicMock() - query_context.slice_ = None + A chartless request that is not a native filter (drill-to-detail, drill-by, + samples) is validated by the datasource-access checks in raise_for_access and + is not constrained here. + """ + qc = mocker.MagicMock() + qc.slice_ = None + qc.form_data = {"dashboardId": 10, "slice_id": 0, "groupby": ["ssn"]} + assert not query_context_modified(qc) - assert not query_context_modified(query_context) + +def test_query_context_modified_native_filter_without_type_marker_blocked( + mocker: MockerFixture, +) -> None: + """ + A request identified by native_filter_id is constrained even when the + NATIVE_FILTER type marker is absent. + """ + query = SimpleNamespace(columns=["ssn"], metrics=[], groupby=[]) + qc = _native_filter_ctx(mocker, [query]) + del qc.form_data["type"] + assert query_context_modified(qc) def test_query_context_modified_mixed_chart(mocker: MockerFixture) -> None: