fix(dashboard): validate native-filter data requests against filter targets

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

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: