alexandrusoare commented on code in PR #43111:
URL: https://github.com/apache/superset/pull/43111#discussion_r3803337008
##########
superset/security/manager.py:
##########
@@ -1305,6 +1429,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(
Review Comment:
Fixed
##########
superset/security/manager.py:
##########
@@ -1107,6 +1107,130 @@ def _orderby_modified(
return False
+def _collect_allowed_sql_extras(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> tuple[set[str], set[str]]:
+ """
+ Collect the ``extras.where`` and ``extras.having`` values that a guest user
+ is allowed to send, derived from the stored chart and its query context.
+ """
+ from superset.common.form_data_query_context import freeform_where_having
+
+ allowed_where: set[str] = set()
+ allowed_having: set[str] = set()
+
+ stored_extras = freeform_where_having(stored_chart.params_dict)
+ if stored_extras.get("where"):
+ allowed_where.add(stored_extras["where"])
+ if stored_extras.get("having"):
+ allowed_having.add(stored_extras["having"])
+
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ extras = query.get("extras") or {}
+ if extras.get("where"):
+ allowed_where.add(extras["where"])
+ if extras.get("having"):
+ allowed_having.add(extras["having"])
+
+ return allowed_where, allowed_having
+
+
+# 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``
+# value 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)"
+
+
+def _filter_has_adhoc_sql_col(flt: Any) -> bool:
+ """
+ Whether a structured ``{col, op, val}`` filter carries an adhoc column
+ with a ``sqlExpression``, which would reach ``adhoc_column_to_sqla``
+ and execute arbitrary SQL in the WHERE clause.
+ """
+ if not isinstance(flt, dict):
+ return False
+ col = flt.get("col")
+ return (
+ isinstance(col, dict)
+ and isinstance(col.get("sqlExpression"), str)
+ and bool(col.get("sqlExpression"))
+ )
+
+
+def _query_extras_sql_modified(
+ query: Any,
+ allowed_where: set[str],
+ allowed_having: set[str],
+) -> bool:
+ """
+ Whether a single query's ``extras.where``/``extras.having`` or structured
+ filters inject SQL not present on the stored chart.
+ """
+ extras = query.extras or {}
+ req_where = extras.get("where", "")
+ if req_where and req_where != _EMPTY_FILTER_SENTINEL:
+ if req_where not in allowed_where:
+ return True
+ req_having = extras.get("having", "")
+ if req_having and req_having != _EMPTY_FILTER_SENTINEL:
+ if req_having not in allowed_having:
+ return True
+ for flt in query.filter or []:
+ if _filter_has_adhoc_sql_col(flt):
+ return True
+ return False
+
+
+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 predicates that are 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`` is an adhoc column
+ carrying a ``sqlExpression`` (reaches ``adhoc_column_to_sqla``).
+
+ Dashboard native filters can inject the ``(1 = 0)`` empty-filter sentinel
+ and adhoc filters tagged ``isExtra`` via ``merge_extra_form_data``; both
+ are allowed so embedded charts with required-but-empty filters are not
+ rejected.
+ """
+ allowed_where, allowed_having = _collect_allowed_sql_extras(
+ stored_chart, stored_query_context
+ )
+
+ if any(
+ _query_extras_sql_modified(query, allowed_where, allowed_having)
+ for query in query_context.queries
+ ):
+ return True
+
+ stored_sql_filters: set[str] = {
+ freeze_value(flt)
+ for flt in stored_chart.params_dict.get("adhoc_filters") or []
+ if flt.get("expressionType") == "SQL"
+ }
+
+ for flt in form_data.get("adhoc_filters") or []:
Review Comment:
Addressed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
