diff --git a/superset/charts/data/api.py b/superset/charts/data/api.py index 250a0713cb64..fc0feaf31dfe 100644 --- a/superset/charts/data/api.py +++ b/superset/charts/data/api.py @@ -413,6 +413,12 @@ def data_from_cache(self, cache_key: str) -> Response: # for async queries with jinja context set_form_data(cached_data) query_context = self._create_query_context_from_form(cached_data) + # Mark as a cache replay so _sql_filters_modified skips the + # SQL-extras check. The original request already passed the + # full security check, cache keys are opaque SHA-256 hashes + # (unguessable), and force_cached only serves pre-computed + # data — no new SQL is executed. + query_context._from_cache_replay = True command = ChartDataCommand(query_context) command.validate() except ChartDataCacheLoadError: diff --git a/superset/security/manager.py b/superset/security/manager.py index 79c262dfddca..c6cf873ba58c 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1177,6 +1177,276 @@ 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`` (``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 [] + # Unbalanced parens can't be a valid composed clause — fail closed so + # the malformed string lands in the allowed-set check as-is (→ 403) + # instead of splitting into fragments that might individually pass. + if composed.count("(") != composed.count(")"): + return [composed] + 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( + extras_allowed: set[str], + col_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): + extras_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: + extras_allowed.add(composed) + for key in ("columns", "groupby"): + for col in query.get(key) or []: + if isinstance(col, dict) and col.get("sqlExpression"): + col_allowed.add(col["sqlExpression"]) + + +def _collect_allowed_sql( + stored_chart: "Slice", + stored_query_context: Optional[dict[str, Any]], +) -> tuple[set[str], set[str]]: + """ + Collect the SQL expressions a guest user is allowed to send. + + Returns ``(extras_allowed, col_allowed)``: + + * ``extras_allowed`` — for validating ``extras.where``/``extras.having``: + adhoc-filter SQL, legacy ``where`` param, stored query-context extras, + and the ``1 = 0`` empty-filter sentinel. + * ``col_allowed`` — for validating structured-filter ``col.sqlExpression``: + everything in ``extras_allowed`` plus column SQL expressions from the + chart's dimensions (which cross-filters legitimately reference). + """ + extras_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") + ): + extras_allowed.add(flt["sqlExpression"]) + + if params.get("where"): + extras_allowed.add(params["where"]) + + # Column expressions go only into col_allowed — they must not be + # injectable as WHERE/HAVING predicates. + col_allowed: set[str] = set(extras_allowed) + _add_column_sql_expressions(col_allowed, params) + + if stored_query_context: + _add_allowed_sql_from_query_context( + extras_allowed, col_allowed, stored_query_context + ) + + return extras_allowed, col_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_extras(query: Any, allowed: set[str]) -> bool: + """Whether a query has novel ``extras.where``/``extras.having`` SQL. + + The full composed 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 (`` the split may break it into fragments that fail individually — + a false positive (403) rather than a bypass, and an acceptable trade-off. + """ + 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_extras`` 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 + + try: + dashboard_id = int(dashboard_id) + except (TypeError, ValueError): + 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. + + Cache-replay requests (``/data/``) are skipped: the original + request already passed the full check, and ``_sanitize_filters`` may have + rewritten ``extras`` in place before caching (comment normalization, + Jinja rendering), making byte-equality comparison unreliable. + """ + if getattr(query_context, "_from_cache_replay", False) is True: + return False + + extras_allowed, col_allowed = _collect_allowed_sql( + stored_chart, stored_query_context + ) + + # Vector 1: extras.where / extras.having + if any(_query_has_novel_extras(q, extras_allowed) for q in query_context.queries): + return True + + # Vector 3: structured filter col with adhoc SQL. + # Sibling chart column expressions (cross-filter) are allowed; the + # dashboard lookup is deferred so the common case pays no DB cost. + if any(_query_has_novel_filter_col(q, col_allowed) for q in query_context.queries): + if dashboard_id := (form_data or {}).get("dashboardId"): + _add_dashboard_column_expressions( + col_allowed, dashboard_id, stored_chart.id + ) + if any( + _query_has_novel_filter_col(q, col_allowed) for q in query_context.queries + ): + return True + + # Vector 2: SQL adhoc filters in form_data + 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 @@ -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 diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index 5f19477ddedc..1c19219a818f 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -36,6 +36,7 @@ from superset.models.slice import Slice from superset.security.manager import ( _collect_sortable_identifiers, + _sql_filters_modified, freeze_value, query_context_modified, SupersetSecurityManager, @@ -3791,3 +3792,683 @@ def test_validate_guest_token_resources_accepts_embedded_int_id( sm.validate_guest_token_resources( [{"type": GuestTokenResourceType.DASHBOARD, "id": 5}] ) + + +# --------------------------------------------------------------------------- +# _sql_filters_modified – block custom SQL injection by guest users +# --------------------------------------------------------------------------- + + +def test_sql_filters_extras_where_injected_blocked( + mocker: MockerFixture, +) -> None: + """Injecting extras.where when the chart has no SQL filters is blocked.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"metrics": ["count"]} + + query = QueryObject(extras={"where": "1=1"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_extras_having_injected_blocked( + mocker: MockerFixture, +) -> None: + """Injecting extras.having when the chart has no SQL filters is blocked.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject(extras={"having": "COUNT(*) > 0"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_extras_where_replay_allowed( + mocker: MockerFixture, +) -> None: + """Replaying the chart's own SQL WHERE filter is allowed.""" + sql_filter = { + "expressionType": "SQL", + "sqlExpression": "region = 'EMEA'", + "clause": "WHERE", + } + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"adhoc_filters": [sql_filter]} + + # freeform_where_having wraps each clause in parens + query = QueryObject(extras={"where": "(region = 'EMEA')"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_extras_having_replay_allowed( + mocker: MockerFixture, +) -> None: + """Replaying the chart's own SQL HAVING filter is allowed.""" + sql_filter = { + "expressionType": "SQL", + "sqlExpression": "SUM(sales) > 100", + "clause": "HAVING", + } + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"adhoc_filters": [sql_filter]} + + query = QueryObject(extras={"having": "(SUM(sales) > 100)"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_adhoc_sql_filter_injected_blocked( + mocker: MockerFixture, +) -> None: + """Injecting a new SQL adhoc filter not on the stored chart is blocked.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject() + query_context.queries = [query] + + injected_filter = { + "expressionType": "SQL", + "sqlExpression": "1=1", + "clause": "WHERE", + } + form_data: dict[str, Any] = {"slice_id": 1, "adhoc_filters": [injected_filter]} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_adhoc_sql_filter_replay_allowed( + mocker: MockerFixture, +) -> None: + """Replaying the exact stored SQL adhoc filter is allowed.""" + sql_filter = { + "expressionType": "SQL", + "sqlExpression": "region = 'EMEA'", + "clause": "WHERE", + } + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"adhoc_filters": [sql_filter]} + + query = QueryObject() + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1, "adhoc_filters": [sql_filter]} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_empty_extras_always_allowed( + mocker: MockerFixture, +) -> None: + """No SQL in extras is always allowed, even when the chart has SQL filters.""" + sql_filter = { + "expressionType": "SQL", + "sqlExpression": "region = 'EMEA'", + "clause": "WHERE", + } + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"adhoc_filters": [sql_filter]} + + query = QueryObject() + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_from_stored_qc_allowed( + mocker: MockerFixture, +) -> None: + """extras.where from stored query_context is allowed.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + stored_qc = { + "queries": [{"extras": {"where": "(col > 5)"}}], + } + + query = QueryObject(extras={"where": "(col > 5)"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, stored_qc) + + +def test_sql_filters_multi_query_stored_predicate_allowed( + mocker: MockerFixture, +) -> None: + """Multiple queries replaying predicates from the stored chart are allowed. + + The allowed set is global across all stored queries — per-query pinning is + intentionally not applied because there is no stable identity linking a + request query to a stored query, and all queries share the same + chart/datasource so predicates only restrict rows, never expand access. + """ + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + stored_qc = { + "queries": [ + {"extras": {"where": "(region = 'EMEA')"}}, + {"extras": {"where": "(status = 'active')"}}, + ], + } + + # Both request queries use predicates from the stored chart. + query_context.queries = [ + QueryObject(extras={"where": "(region = 'EMEA')"}), + QueryObject(extras={"where": "(status = 'active')"}), + ] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, stored_qc) + + +def test_sql_filters_multi_query_novel_predicate_blocked( + mocker: MockerFixture, +) -> None: + """A novel predicate on any query is blocked even when others are valid.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + stored_qc = { + "queries": [{"extras": {"where": "(region = 'EMEA')"}}], + } + + query_context.queries = [ + QueryObject(extras={"where": "(region = 'EMEA')"}), + QueryObject(extras={"where": "(1=1)"}), # not stored + ] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, stored_qc) + + +def test_sql_filters_different_sql_blocked( + mocker: MockerFixture, +) -> None: + """Modified SQL (appending extra predicates) is blocked.""" + sql_filter = { + "expressionType": "SQL", + "sqlExpression": "col > 5", + "clause": "WHERE", + } + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"adhoc_filters": [sql_filter]} + + # Attacker appends extra predicate + query = QueryObject( + extras={"where": "(col > 5) AND (1=1)"}, + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_simple_filters_not_blocked( + mocker: MockerFixture, +) -> None: + """SIMPLE structured filters (from dashboard native filters) are not blocked.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject( + filters=[{"col": "country", "op": "==", "val": "US"}], + ) + query_context.queries = [query] + + simple_adhoc_filter = { + "expressionType": "SIMPLE", + "subject": "country", + "operator": "==", + "comparator": "US", + "clause": "WHERE", + } + form_data: dict[str, Any] = { + "slice_id": 1, + "adhoc_filters": [simple_adhoc_filter], + } + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_structured_filter_adhoc_col_blocked( + mocker: MockerFixture, +) -> None: + """Structured filter with an adhoc SQL column in ``col`` is blocked. + + ``ChartDataFilterSchema.col`` is ``fields.Raw``, so an attacker can pass + an adhoc column dict that reaches ``adhoc_column_to_sqla`` and executes + arbitrary SQL in the WHERE clause. + """ + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + adhoc_col: Any = { + "expressionType": "SQL", + "sqlExpression": "1; DROP TABLE users--", + "label": "x", + } + query = QueryObject( + filters=[{"col": adhoc_col, "op": "!=", "val": "z"}], + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_structured_filter_stored_adhoc_col_allowed( + mocker: MockerFixture, +) -> None: + """Cross-filter with an adhoc SQL column matching a stored chart dimension + is allowed.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = { + "columns": [ + {"sqlExpression": "YEAR(order_date)", "label": "order_year"}, + ], + } + + adhoc_col: Any = { + "sqlExpression": "YEAR(order_date)", + "label": "order_year", + } + query = QueryObject( + filters=[{"col": adhoc_col, "op": "==", "val": "2024"}], + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_cross_filter_adhoc_col_from_sibling_chart_allowed( + mocker: MockerFixture, +) -> None: + """Cross-filter with an adhoc SQL column from a sibling chart on the same + dashboard is allowed.""" + from superset.models.dashboard import Dashboard + + # Target chart (chart B) has no custom SQL columns. + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.id = 2 + stored_chart.params_dict = {"metrics": ["count"]} + + # Source chart (chart A) has the custom SQL dimension. + sibling_chart = mocker.MagicMock() + sibling_chart.id = 1 + sibling_chart.params_dict = { + "columns": [ + {"sqlExpression": "YEAR(order_date)", "label": "order_year"}, + ], + } + + # Dashboard contains both charts. + dashboard = mocker.MagicMock(spec=Dashboard) + dashboard.slices = [sibling_chart, stored_chart] + + mocker.patch("superset.db.session.query") + db_query = mocker.patch("superset.db.session.query").return_value + db_query.filter.return_value.one_or_none.return_value = dashboard + mocker.patch( + "superset.security_manager.has_guest_access", + return_value=True, + ) + + adhoc_col: Any = { + "sqlExpression": "YEAR(order_date)", + "label": "order_year", + } + query = QueryObject( + filters=[{"col": adhoc_col, "op": "==", "val": "2024"}], + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 2, "dashboardId": 10} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_cross_filter_rejected_for_unauthorized_dashboard( + mocker: MockerFixture, +) -> None: + """Cross-filter lookup must not use a dashboard the guest has no access to.""" + from superset.models.dashboard import Dashboard + + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.id = 2 + stored_chart.params_dict = {} + + sibling_chart = mocker.MagicMock() + sibling_chart.id = 1 + sibling_chart.params_dict = { + "columns": [{"sqlExpression": "YEAR(order_date)", "label": "order_year"}], + } + + dashboard = mocker.MagicMock(spec=Dashboard) + dashboard.slices = [sibling_chart, stored_chart] + + mocker.patch("superset.db.session.query") + db_query = mocker.patch("superset.db.session.query").return_value + db_query.filter.return_value.one_or_none.return_value = dashboard + mocker.patch( + "superset.security_manager.has_guest_access", + return_value=False, + ) + + adhoc_col: Any = {"sqlExpression": "YEAR(order_date)", "label": "order_year"} + query = QueryObject( + filters=[{"col": adhoc_col, "op": "==", "val": "2024"}], + ) + query_context.queries = [query] + form_data: dict[str, Any] = {"slice_id": 2, "dashboardId": 999} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_cross_filter_rejected_when_chart_not_on_dashboard( + mocker: MockerFixture, +) -> None: + """Cross-filter lookup must verify the target chart belongs to the dashboard.""" + from superset.models.dashboard import Dashboard + + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.id = 99 # not on the dashboard + stored_chart.params_dict = {} + + sibling_chart = mocker.MagicMock() + sibling_chart.id = 1 + sibling_chart.params_dict = { + "columns": [{"sqlExpression": "YEAR(order_date)", "label": "order_year"}], + } + + dashboard = mocker.MagicMock(spec=Dashboard) + dashboard.slices = [sibling_chart] # stored_chart not here + + mocker.patch("superset.db.session.query") + db_query = mocker.patch("superset.db.session.query").return_value + db_query.filter.return_value.one_or_none.return_value = dashboard + mocker.patch( + "superset.security_manager.has_guest_access", + return_value=True, + ) + + adhoc_col: Any = {"sqlExpression": "YEAR(order_date)", "label": "order_year"} + query = QueryObject( + filters=[{"col": adhoc_col, "op": "==", "val": "2024"}], + ) + query_context.queries = [query] + form_data: dict[str, Any] = {"slice_id": 99, "dashboardId": 10} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_sibling_expressions_cannot_inject_where_having( + mocker: MockerFixture, +) -> None: + """Sibling chart column expressions must not legitimize novel WHERE/HAVING.""" + from superset.models.dashboard import Dashboard + + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.id = 2 + stored_chart.params_dict = {} + + # Sibling has a column expression that an attacker tries to use as WHERE. + sibling_chart = mocker.MagicMock() + sibling_chart.id = 1 + sibling_chart.params_dict = { + "columns": [ + {"sqlExpression": "(SELECT secret FROM users LIMIT 1)", "label": "x"}, + ], + } + + dashboard = mocker.MagicMock(spec=Dashboard) + dashboard.slices = [sibling_chart, stored_chart] + + mocker.patch("superset.db.session.query") + db_query = mocker.patch("superset.db.session.query").return_value + db_query.filter.return_value.one_or_none.return_value = dashboard + + # Attacker injects the sibling expression into extras.where. + query = QueryObject( + extras={"where": "(SELECT secret FROM users LIMIT 1)"}, + ) + query_context.queries = [query] + form_data: dict[str, Any] = {"slice_id": 2, "dashboardId": 10} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_collect_allowed_sql_includes_scalar_column_params( + mocker: MockerFixture, +) -> None: + """Scalar column params like x_axis contribute their sqlExpression.""" + from superset.security.manager import _collect_allowed_sql + + stored_chart = mocker.MagicMock() + stored_chart.params_dict = { + "x_axis": {"sqlExpression": "DATE_TRUNC('month', ts)", "label": "m"}, + "groupby": [{"sqlExpression": "UPPER(country)", "label": "c"}], + } + + _, col_allowed = _collect_allowed_sql(stored_chart, None) + + assert "DATE_TRUNC('month', ts)" in col_allowed + assert "UPPER(country)" in col_allowed + + +def test_sql_filters_structured_filter_string_col_allowed( + mocker: MockerFixture, +) -> None: + """Structured filter with a plain string column is allowed.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject( + filters=[{"col": "status", "op": "==", "val": "active"}], + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_empty_filter_sentinel_allowed( + mocker: MockerFixture, +) -> None: + """The ``(1 = 0)`` sentinel from a required-but-empty native filter is allowed.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject(extras={"where": "(1 = 0)"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_double_sentinel_allowed( + mocker: MockerFixture, +) -> None: + """Two required-but-empty filters compose ``(1 = 0) AND (1 = 0)``.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject(extras={"where": "(1 = 0) AND (1 = 0)"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_stored_clause_plus_sentinel_allowed( + mocker: MockerFixture, +) -> None: + """A stored SQL filter composed with the empty-filter sentinel is allowed.""" + sql_filter = { + "expressionType": "SQL", + "sqlExpression": "region = 'EMEA'", + "clause": "WHERE", + } + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {"adhoc_filters": [sql_filter]} + + query = QueryObject( + extras={"where": "(region = 'EMEA') AND (1 = 0)"}, + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_non_dict_adhoc_filter_skipped( + mocker: MockerFixture, +) -> None: + """Non-dict items in adhoc_filters are skipped, not 500.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject() + query_context.queries = [query] + + form_data: dict[str, Any] = { + "slice_id": 1, + "adhoc_filters": ["not_a_dict", 42, None], + } + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_raise_for_access_guest_user_sql_filter_injection_blocked( + mocker: MockerFixture, + app_context: None, + stored_metrics: list[AdhocMetric], +) -> None: + """Guest user injecting SQL via extras.where is rejected by raise_for_access.""" + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "is_guest_user", return_value=True) + mocker.patch.object(sm, "can_access", return_value=True) + + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = {"metrics": stored_metrics} + + query_context.form_data = {"slice_id": 42, "metrics": stored_metrics} + query_context.queries = [ + QueryObject( + metrics=stored_metrics, # type: ignore + extras={"where": "1=1 UNION SELECT password FROM users"}, + ) + ] + + with pytest.raises(SupersetSecurityException): + sm.raise_for_access(query_context=query_context) + + +def test_sql_filters_cache_replay_skips_check( + mocker: MockerFixture, +) -> None: + """Cache-replay requests skip the SQL filter check.""" + query_context = mocker.MagicMock() + query_context._from_cache_replay = True + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject(extras={"where": "(injected SQL)"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert not _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_column_expression_cannot_become_where( + mocker: MockerFixture, +) -> None: + """A chart's column sqlExpression must not be injectable as extras.where.""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = { + "columns": [ + { + "sqlExpression": "(SELECT secret FROM users LIMIT 1)", + "label": "x", + }, + ], + } + + query = QueryObject( + extras={"where": "((SELECT secret FROM users LIMIT 1))"}, + ) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None) + + +def test_sql_filters_unbalanced_parens_rejected( + mocker: MockerFixture, +) -> None: + """Unbalanced parens in extras.where are rejected (403, not 500).""" + query_context = mocker.MagicMock() + stored_chart = mocker.MagicMock() + stored_chart.params_dict = {} + + query = QueryObject(extras={"where": "(a) AND (b"}) + query_context.queries = [query] + + form_data: dict[str, Any] = {"slice_id": 1} + + assert _sql_filters_modified(query_context, form_data, stored_chart, None)