msyavuz commented on code in PR #43111:
URL: https://github.com/apache/superset/pull/43111#discussion_r3802759137
##########
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:
Review Comment:
`extras.where` is a `' AND '`-joined composition (`processFilters.ts:69`),
not a single clause, so exact-match plus a single-value sentinel rejects
legitimate dashboards: two required-empty Select filters send `(1 = 0) AND (1 =
0)`, and a chart with a saved SQL adhoc filter on such a dashboard sends
`(region = 'EMEA') AND (1 = 0)` — both 403. Should this split on the join and
validate each clause?
##########
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 []:
+ if flt.get("expressionType") == "SQL" and not flt.get("isExtra"):
Review Comment:
`isExtra` comes from guest-controlled `form_data` (`fields.Raw` in
`charts/schemas.py`), so adding `"isExtra": true` to an injected SQL filter
bypasses this check; on the embedded path `getSelectExtraFormData` never sets
it and `buildQueryContext` sends `form_data` before extra adhoc filters are
appended, so the allowance seems to cover no real request.
##########
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 []:
Review Comment:
Rejecting every adhoc `col` breaks cross-filters: `getCrossFilterDataMask`
(`plugin-chart-echarts/src/utils/eventHandlers.ts:67`) emits `{col, op, val}`
with the source chart's raw `QueryFormColumn`, which is an adhoc dict whenever
that chart's dimension is custom SQL — so a guest clicking such a chart gets a
403.
##########
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:
`flt` is guest-controlled and not necessarily a dict — `"adhoc_filters":
["x"]` raises `AttributeError` inside `raise_for_access`, i.e. a 500 instead of
a 403.
##########
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:
This guard is reachable only with a `slice_id`: omit it and
`QueryContextFactory` leaves `slice_` None (`query_context_factory.py:67`),
`query_context_modified` takes the chartless branch,
`_native_filter_request_modified` returns `False` with no native-filter marker,
and arbitrary `extras.where` is accepted — and the explicit `NATIVE_FILTER`
path never inspects `extras` either.
--
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]
