msyavuz commented on code in PR #43111:
URL: https://github.com/apache/superset/pull/43111#discussion_r3812072498
##########
superset/security/manager.py:
##########
@@ -1236,6 +1399,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.
Review Comment:
Datasource-access checks gate *which* dataset is queried, not what SQL runs
against it, so a guest can simply omit `slice_id` — `slice_` stays `None`,
`_native_filter_request_modified` returns `False` for any payload without the
`NATIVE_FILTER`/`native_filter_id` marker, and arbitrary `extras.where`
(including subqueries) executes against any dataset the dashboard grants. Is
leaving that path open intentional here?
##########
superset/security/manager.py:
##########
@@ -1107,6 +1107,169 @@ 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"
+
+
+def _split_extras_clauses(composed: str) -> list[str]:
+ """
+ Extract raw SQL expressions from a composed ``extras.where`` /
+ ``extras.having`` string.
+
+ ``_sanitize_clause`` / ``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 (tolerating whitespace variations),
+ strip the outer parens, and return the raw expressions.
+ """
+ if not composed:
+ return []
+ raw = re.split(r"\)\s+AND\s+\(", composed)
+ # 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"])
+
+ for key in _STORED_COLUMN_PARAMS:
+ for col in params.get(key) or []:
+ if isinstance(col, dict) and col.get("sqlExpression"):
+ allowed.add(col["sqlExpression"])
+
+ if stored_query_context:
+ _add_allowed_sql_from_query_context(allowed, stored_query_context)
+
+ return allowed
+
+
+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, "")
Review Comment:
`QueryObject._sanitize_filters` rewrites `extras["where"/"having"]` in place
and `get_payload_result` caches the rewritten value into
`cache_values["queries"]`, so the `GET /api/v1/chart/data/<cache_key>`
re-validation compares normalized SQL against the chart's raw stored
`sqlExpression` — with `GLOBAL_ASYNC_QUERIES` on, a saved custom SQL filter
containing `--` is cached as `(a > 0 /* x */)` (was `(a > 0 -- x\n)`) and the
guest's result fetch 403s.
--
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]
