alexandrusoare commented on code in PR #43111:
URL: https://github.com/apache/superset/pull/43111#discussion_r3812155907
##########
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:
Checked — not an issue. The async cache stores the original form_data, not
the sanitized QueryObject.extras. On cache fetch, the QueryContext is rebuilt
from that original form_data, so our validation sees the pre-sanitization value.
--
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]
