fix(embedded): block custom SQL injection in guest user chart payloads

superset/security/manager.py

@@ -1177,6 +1177,290 @@ def _orderby_modified(
     return False
 
 
+# The frontend emits ``{expressionType: "SQL", sqlExpression: "1 = 0"}`` when
+# a native Select filter has "Filter value is required" enabled and no value
+# has been selected yet (superset-frontend/src/filters/utils.ts).  After
+# ``_sanitize_clause`` wraps it in parentheses the resulting ``extras.where``
+# clause is ``(1 = 0)``.  This is safe — it returns zero rows — and must be
+# allowed so that embedded charts are not rejected before the user picks a
+# filter value.
+_EMPTY_FILTER_SENTINEL = "1 = 0"
+
+#: Chart params keys that hold columns/group-bys a chart renders.  Defined
+#: here (above ``_collect_allowed_sql`` which is the first consumer) and
+#: reused by ``_columns_metrics_modified`` and ``_add_dashboard_column_expressions``.
+_STORED_COLUMN_PARAMS = (
+    "columns",
+    "groupby",
+    "all_columns",
+    "entity",
+    "series",
+    "series_columns",
+    "x_axis",
+    "granularity_sqla",
+)
+
+
+def _split_extras_clauses(composed: str) -> list[str]:
+    """
+    Extract raw SQL expressions from a composed ``extras.where`` /
+    ``extras.having`` string.
+
+    ``_sanitize_clause`` (``form_data_query_context.py:92``) /
+    ``processFilters.ts`` (``superset-ui-core/src/query/processFilters.ts``)
+    wraps each expression in one layer of parentheses and joins them with
+    ``' AND '``, producing strings like ``(expr1) AND (expr2)``.  This
+    reverses that: split on the ``)\\s+AND\\s+(`` boundary (case-insensitive,
+    tolerating whitespace variations), strip the outer parens, and return the
+    raw expressions.
+    """
+    if not composed:
+        return []
+    raw = re.split(r"\)\s+AND\s+\(", composed, flags=re.IGNORECASE)
+    # Strip exactly one outer paren added by _sanitize_clause.
+    if raw[0].startswith("("):
+        raw[0] = raw[0][1:]
+    if raw[-1].endswith(")"):
+        raw[-1] = raw[-1][:-1]
+    # _sanitize_clause appends ``\n`` inside the parens when the expression
+    # contains ``--`` (to terminate a trailing line comment).  Strip it so
+    # the result matches the stored raw expression.
+    return [expr.rstrip("\n") for expr in raw]
+
+
+def _add_allowed_sql_from_query_context(
+    allowed: set[str],
+    stored_query_context: dict[str, Any],
+) -> None:
+    """Add allowed SQL expressions from a stored query context."""
+    for query in stored_query_context.get("queries") or []:
+        for param in ("where", "having"):
+            composed = (query.get("extras") or {}).get(param, "")
+            for expr in _split_extras_clauses(composed):
+                allowed.add(expr)
+            # Keep the full composed value as a fallback in case a stored
+            # expression contains a literal ") AND (" that the split would
+            # incorrectly break apart.
+            if composed:
+                allowed.add(composed)
+        for key in ("columns", "groupby"):
+            for col in query.get(key) or []:
+                if isinstance(col, dict) and col.get("sqlExpression"):
+                    allowed.add(col["sqlExpression"])
+
+
+def _collect_allowed_sql(
+    stored_chart: "Slice",
+    stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
+    """
+    Collect every raw SQL expression a guest user is allowed to send,
+    derived from the stored chart's params and query context.
+
+    This single set validates all three SQL injection vectors:
+    ``extras.where``/``extras.having`` clauses, SQL-type adhoc filters, and
+    adhoc-column ``col`` values in structured filters.
+
+    The empty-filter sentinel ``1 = 0`` is always included.
+    """
+    allowed: set[str] = {_EMPTY_FILTER_SENTINEL}
+    params = stored_chart.params_dict
+
+    for flt in params.get("adhoc_filters") or []:
+        if (
+            isinstance(flt, dict)
+            and flt.get("expressionType") == "SQL"
+            and flt.get("sqlExpression")
+        ):
+            allowed.add(flt["sqlExpression"])
+
+    if params.get("where"):
+        allowed.add(params["where"])
+
+    _add_column_sql_expressions(allowed, params)
+
+    if stored_query_context:
+        _add_allowed_sql_from_query_context(allowed, stored_query_context)
+
+    return allowed
+
+
+def _add_column_sql_expressions(target: set[str], params: dict[str, Any]) -> None:
+    """Add ``sqlExpression`` values from column params to *target*.
+
+    Handles both list-valued controls (``columns``, ``groupby``) and
+    scalar-valued ones (``x_axis``, ``entity``, etc.).
+    """
+    for key in _STORED_COLUMN_PARAMS:
+        value = params.get(key)
+        if value is None:
+            continue
+        items = value if isinstance(value, (list, tuple)) else [value]
+        for col in items:
+            if isinstance(col, dict) and col.get("sqlExpression"):
+                target.add(col["sqlExpression"])
+
+
+def _query_has_novel_sql(query: Any, allowed: set[str]) -> bool:
+    """Whether a single query carries SQL not in the allowed set.
+
+    The full composed ``extras.where``/``extras.having`` value is checked
+    first; if it is in ``allowed`` (which includes full composed values from
+    the stored query context as a fallback) the split is skipped.  If a
+    stored expression contains a literal ``) AND (`` (e.g. a ``CASE WHEN``),
+    the split may break it into fragments that fail individually — a false
+    positive (403) rather than a bypass.  This is an acceptable trade-off:
+    such expressions in adhoc filters are rare, and the behavior fails closed.
+    """
+    extras = getattr(query, "extras", None) or {}
+    for param in ("where", "having"):
+        composed = extras.get(param, "")
+        if composed and composed not in allowed:
+            for expr in _split_extras_clauses(composed):
+                if expr not in allowed:
+                    return True
+
+    for flt in getattr(query, "filter", None) or []:
+        if isinstance(flt, dict):
+            col = flt.get("col")
+            if isinstance(col, dict) and col.get("sqlExpression"):
+                if col["sqlExpression"] not in allowed:
+                    return True
+    return False
+
+
+def _query_has_novel_extras(query: Any, allowed: set[str]) -> bool:
+    """Whether a query has novel ``extras.where``/``extras.having`` SQL."""
+    extras = getattr(query, "extras", None) or {}
+    for param in ("where", "having"):
+        composed = extras.get(param, "")
+        if composed and composed not in allowed:
+            for expr in _split_extras_clauses(composed):
+                if expr not in allowed:
+                    return True
+    return False
+
+
+def _query_has_novel_filter_col(query: Any, allowed: set[str]) -> bool:
+    """Whether a query has a structured filter ``col`` not in the allowed set.
+
+    Unlike ``_query_has_novel_sql`` this only checks the ``filter[].col``
+    vector — the cross-filter path — and intentionally ignores
+    ``extras.where``/``extras.having``.  Used for the scoped re-check after
+    expanding ``allowed`` with sibling dashboard chart expressions: those
+    borrowed expressions must only legitimize filter columns, not become
+    injectable as arbitrary WHERE/HAVING predicates.
+    """
+    for flt in getattr(query, "filter", None) or []:
+        if isinstance(flt, dict):
+            col = flt.get("col")
+            if isinstance(col, dict) and col.get("sqlExpression"):
+                if col["sqlExpression"] not in allowed:
+                    return True
+    return False
+
+
+def _add_dashboard_column_expressions(
+    allowed: set[str], dashboard_id: Any, target_chart_id: int
+) -> None:
+    """
+    Add ``sqlExpression`` values from adhoc columns on every chart of the
+    given dashboard (except the target chart, which is already covered).
+
+    This allows cross-filter structured filters whose ``col`` carries the
+    source chart's custom SQL dimension to pass validation.  Called lazily
+    (only when an unrecognized adhoc SQL col is found) to avoid a DB query
+    on the common path.
+
+    The dashboard is authorized via ``has_guest_access`` and the target chart
+    must belong to the dashboard; otherwise no expressions are added.
+    """
+    # pylint: disable=import-outside-toplevel
+    from superset import db, security_manager
+    from superset.models.dashboard import Dashboard
+
+    if not isinstance(dashboard_id, int):
+        return
+    dashboard = (
+        db.session.query(Dashboard).filter(Dashboard.id == dashboard_id).one_or_none()
+    )
+    if dashboard is None:
+        return
+
+    if not security_manager.has_guest_access(dashboard):
+        return
+
+    slice_ids = {s.id for s in dashboard.slices}
+    if target_chart_id not in slice_ids:
+        return
+
+    for slc in dashboard.slices:
+        if slc.id == target_chart_id:
+            continue
+        _add_column_sql_expressions(allowed, slc.params_dict)
+
+
+def _sql_filters_modified(
+    query_context: "QueryContext",
+    form_data: dict[str, Any],
+    stored_chart: "Slice",
+    stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+    """
+    Whether the request injects custom SQL not present on the stored chart.
+
+    Covers three vectors:
+
+    1. ``extras.where`` / ``extras.having`` — raw SQL strings.
+    2. Adhoc filters with ``expressionType == "SQL"`` in ``form_data``.
+    3. Structured ``{col, op, val}`` filters whose ``col`` carries a
+       ``sqlExpression`` (reaches ``adhoc_column_to_sqla``).
+
+    The ``(1 = 0)`` empty-filter sentinel injected by required-but-empty
+    native Select filters is always allowed.  For vector 3, SQL expressions
+    from all charts on the requesting dashboard are allowed so that
+    cross-filters referencing a sibling chart's custom SQL dimension pass.
+    """
+    allowed = _collect_allowed_sql(stored_chart, stored_query_context)
+
+    if any(_query_has_novel_sql(q, allowed) for q in query_context.queries):
+        # A novel SQL expression was found.  Before rejecting, check whether
+        # it comes from a sibling chart's custom SQL dimension (cross-filter).
+        # The dashboard lookup is deferred to here so that the common case
+        # (no cross-filter adhoc cols) pays no DB cost.
+
+        # Novel extras.where/having is always rejected — sibling chart
+        # expressions must never legitimize arbitrary WHERE/HAVING predicates.
+        if any(_query_has_novel_extras(q, allowed) for q in query_context.queries):
+            return True
+
+        # The only remaining novel SQL is in filter[].col.  Expand the
+        # allowed set with sibling chart column expressions (authorized
+        # dashboard only) and re-check just the filter col vector.
+        if dashboard_id := (form_data or {}).get("dashboardId"):
+            _add_dashboard_column_expressions(allowed, dashboard_id, stored_chart.id)
+            if not any(
+                _query_has_novel_filter_col(q, allowed) for q in query_context.queries
+            ):
+                return False
+        return True
+
+    stored_sql_filters: set[str] = {
+        freeze_value(flt)
+        for flt in stored_chart.params_dict.get("adhoc_filters") or []
+        if isinstance(flt, dict) and flt.get("expressionType") == "SQL"
+    }
+
+    for flt in form_data.get("adhoc_filters") or []:
+        if not isinstance(flt, dict):
+            continue
+        if flt.get("expressionType") == "SQL":
+            if freeze_value(flt) not in stored_sql_filters:
+                return True
+
+    return False
+
+
 #: Chart params keys that hold the metrics a chart renders. Different chart
 #: types store their metrics under control-specific keys (``metric`` for
 #: big number, ``x``/``y``/``size`` for bubble, and so on); a guest requesting
@@ -1193,20 +1477,6 @@ def _orderby_modified(
     "size",
 )
 
-#: Chart params keys that hold the columns/group-bys a chart renders, across
-#: the control names chart types use for them (``entity``/``series`` for
-#: bubble and world map, ``granularity_sqla`` for the temporal axis, etc.).
-_STORED_COLUMN_PARAMS = (
-    "columns",
-    "groupby",
-    "all_columns",
-    "entity",
-    "series",
-    "series_columns",
-    "x_axis",
-    "granularity_sqla",
-)
-
 
 def _stored_param_values(params: dict[str, Any], keys: tuple[str, ...]) -> set[str]:
     """
@@ -1306,6 +1576,13 @@ def query_context_modified(query_context: "QueryContext") -> bool:
     # 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).
+    #
+    # SQL extras (extras.where/having) are NOT validated on chartless paths:
+    # without a stored chart there is nothing to validate against, and
+    # tightening this would break legitimate chartless flows (native-filter
+    # pre-filtering, drill-to-detail) that carry SQL extras.  These paths
+    # are still protected by datasource-access checks in raise_for_access.
+    # The _sql_filters_modified check below covers chart payloads only.
     if stored_chart is None:
         return _native_filter_request_modified(query_context)
 
@@ -1375,6 +1652,19 @@ def query_context_modified(query_context: "QueryContext") -> bool:
         )
         return True
 
+    # SQL predicates (extras.where/having, SQL adhoc filters) must match
+    # what was saved on the chart; injected custom SQL is rejected.
+    if _sql_filters_modified(
+        query_context, form_data, stored_chart, stored_query_context
+    ):
+        logger.warning(
+            "Guest chart payload rejected for slice %s: SQL filter/extras "
+            "not on the stored chart (stored query_context %s)",
+            stored_chart.id,
+            stored_context_state,
+        )
+        return True
+
     return False