diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 6cdf23506630..735adb0817b5 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1687,6 +1687,12 @@ def adhoc_metric_to_sqla( label = utils.get_metric_name(metric, self.verbose_map) if expression_type == utils.AdhocMetricExpressionType.SIMPLE: + aggregate: Any = metric.get("aggregate") + if ( + not isinstance(aggregate, str) + or aggregate not in self.sqla_aggregations + ): + raise QueryObjectValidationError(_("Adhoc metric aggregate is invalid")) metric_column = metric.get("column") or {} column_name = cast(str, metric_column.get("column_name")) table_column: TableColumn | None = columns_by_name.get(column_name) @@ -1696,9 +1702,13 @@ def adhoc_metric_to_sqla( ) else: sqla_column = column(column_name) - sqla_metric = self.sqla_aggregations[metric["aggregate"]](sqla_column) + sqla_metric = self.sqla_aggregations[aggregate](sqla_column) elif expression_type == utils.AdhocMetricExpressionType.SQL: - expression = metric.get("sqlExpression") + expression: str | None = metric.get("sqlExpression") + if not isinstance(expression, str) or not expression.strip(): + raise QueryObjectValidationError( + _("Adhoc metric SQL expression is invalid") + ) if not processed: try: diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 77963bdd2884..9ca87780be97 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -102,6 +102,7 @@ from superset.jinja_context import BaseTemplateProcessor from superset.sql.parse import sanitize_clause, SQLScript, SQLStatement from superset.superset_typing import ( + AdhocColumn, AdhocMetric, Column as ColumnTyping, FilterValue, @@ -2585,16 +2586,26 @@ def adhoc_metric_to_sqla( label = utils.get_metric_name(metric) if expression_type == utils.AdhocMetricExpressionType.SIMPLE: + aggregate: Any = metric.get("aggregate") + if ( + not isinstance(aggregate, str) + or aggregate not in self.sqla_aggregations + ): + raise QueryObjectValidationError(_("Adhoc metric aggregate is invalid")) metric_column = metric.get("column") or {} column_name = cast(str, metric_column.get("column_name")) sqla_column = sa.column(column_name) - sqla_metric = self.sqla_aggregations[metric["aggregate"]](sqla_column) + sqla_metric = self.sqla_aggregations[aggregate](sqla_column) elif expression_type == utils.AdhocMetricExpressionType.SQL: - expression = metric.get("sqlExpression") + expression: Any = metric.get("sqlExpression") + if not isinstance(expression, str) or not expression.strip(): + raise QueryObjectValidationError( + _("Adhoc metric SQL expression is invalid") + ) if not processed: expression = self._process_select_expression( - expression=metric["sqlExpression"], + expression=expression, database_id=self.database_id, engine=self.database.backend, schema=self.schema, @@ -2753,7 +2764,7 @@ def _reapply_query_filters( def adhoc_column_to_sqla( self, - col: "AdhocColumn", # type: ignore # noqa: F821 + col: AdhocColumn, force_type_check: bool = False, template_processor: Optional[BaseTemplateProcessor] = None, ) -> tuple[ColumnElement, Optional[GenericDataType]]: @@ -3282,6 +3293,13 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma # use the key of the ColumnClause for the expected label metrics_exprs_by_label = {m.key: m for m in metrics_exprs} metrics_exprs_by_expr = {str(m): m for m in metrics_exprs} + adhoc_columns_by_label: dict[str, AdhocColumn] = {} + for selected in columns: + if not utils.is_adhoc_column(selected): + continue + selected_label = selected.get("label") + if isinstance(selected_label, str) and selected_label: + adhoc_columns_by_label[selected_label] = selected # Since orderby may use adhoc metrics, too; we need to process them first orderby_exprs: list[ColumnElement] = [] @@ -3313,6 +3331,11 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma elif col in metrics_exprs_by_label: col = metrics_exprs_by_label[col] need_groupby = True + elif isinstance(col, str) and col in adhoc_columns_by_label: + col, _unused = self.adhoc_column_to_sqla( + col=adhoc_columns_by_label[col], + template_processor=template_processor, + ) elif col in metrics_by_name: col = metrics_by_name[col].get_sqla_col( template_processor=template_processor @@ -3322,21 +3345,6 @@ def get_sqla_query( # pylint: disable=too-many-arguments,too-many-locals,too-ma col = self.convert_tbl_column_to_sqla_col( columns_by_name[col], template_processor=template_processor ) - elif isinstance(col, str) and columns: - # Check if this is a label reference to an adhoc column - adhoc_col = next( - ( - c - for c in columns - if utils.is_adhoc_column(c) and c.get("label") == col - ), - None, - ) - if adhoc_col: - col, _unused = self.adhoc_column_to_sqla( - col=adhoc_col, - template_processor=template_processor, - ) if isinstance(col, ColumnElement): orderby_exprs.append(col) diff --git a/superset/security/manager.py b/superset/security/manager.py index 78180893e2ca..352edf7e9c3f 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -23,7 +23,15 @@ from collections import defaultdict from math import ceil from types import SimpleNamespace -from typing import Any, Callable, cast, NamedTuple, Optional, TYPE_CHECKING, Union +from typing import ( + Any, + Callable, + cast, + NamedTuple, + Optional, + TYPE_CHECKING, + Union, +) from flask import current_app, Flask, g, Request from flask_appbuilder import Model @@ -83,6 +91,8 @@ from superset.utils.core import ( DatasourceName, DatasourceType, + get_column_name, + get_metric_name, get_user_id, get_username, RowLevelSecurityFilterType, @@ -520,48 +530,240 @@ def _native_filter_request_modified(query_context: "QueryContext") -> bool: ) +def _get_form_data_item_label(item: Any, is_metric: bool) -> str | None: + """ + Return the result-key label Superset uses for a column or metric definition. + """ + label: Any + try: + label = get_metric_name(item) if is_metric else get_column_name(item) + except (AttributeError, KeyError, TypeError, ValueError): + return None + return label if isinstance(label, str) and label else None + + +def _is_hidden_table_column(column_config: Any, name: str) -> bool: + """ + Whether Table column_config marks a result column as hidden. + """ + config = column_config.get(name) if isinstance(column_config, dict) else None + return isinstance(config, dict) and config.get("visible") is False + + +def _stored_sort_target_identifiers(item: Any, is_metric: bool) -> set[str]: + """ + Identifiers that can refer to a stored column/metric in orderby. + + The exact frozen value preserves dict-shaped adhoc references. The label + matches result keys sent by Table server pagination and labels accepted by + SQL query building for adhoc columns/metrics. + """ + identifiers = {freeze_value(item)} + if label := _get_form_data_item_label(item, is_metric=is_metric): + identifiers.add(freeze_value(label)) + return identifiers + + +def _requested_sort_target_identifiers(item: Any) -> set[str]: + """ + Identifiers a requested orderby term may use. + + String terms are result keys. Dict-shaped terms carry expression bodies, so + they authorize only by exact stored identity and never by a reused label. + """ + if isinstance(item, (str, dict)): + return {freeze_value(item)} + return set() + + +def _add_visible_sort_targets( + allowed: set[str], + values: Any, + column_config: Any, + *, + is_metric: bool, +) -> None: + """ + Add visible column/metric orderby identifiers from a stored control value. + """ + if not isinstance(values, (list, tuple)): + return + for value in values: + label = _get_form_data_item_label(value, is_metric=is_metric) + if label is not None and _is_hidden_table_column(column_config, label): + continue + allowed.update(_stored_sort_target_identifiers(value, is_metric=is_metric)) + + def _collect_sortable_identifiers( stored_chart: "Slice", stored_query_context: Optional[dict[str, Any]], ) -> set[str]: """ - Frozen column names and metric labels/definitions a guest may legitimately - sort by: every column or metric the stored chart already references. - - Order-by only changes the ordering of the result, not which data is read, so - any column or metric already part of the chart is a safe sort target. A term - that is not present in the stored chart (for example a free-form ``random()`` - expression) cannot be validated and must be rejected. Order-by entries are - ``(column_or_metric, ascending)`` pairs, so only their first element is - collected. + Identifiers a guest may use for a new sort target. + + These are the visible columns/metrics the stored chart exposes. Exact + owner-defined orderby replay is handled separately because saved orderby may + intentionally reference a non-visible helper term, while guest-initiated + sorting should be limited to visible result columns. """ allowed: set[str] = set() + params = stored_chart.params_dict + column_config = params.get("column_config") + + for key in ("columns", "groupby", "all_columns"): + _add_visible_sort_targets( + allowed, + params.get(key), + column_config, + is_metric=False, + ) + _add_visible_sort_targets( + allowed, + params.get("metrics"), + column_config, + is_metric=True, + ) + # Legacy charts store a single metric under the singular ``metric`` key. + if params.get("metric") is not None: + _add_visible_sort_targets( + allowed, + [params["metric"]], + column_config, + is_metric=True, + ) + + if stored_query_context: + for query in stored_query_context.get("queries") or []: + for key in ("columns", "groupby", "all_columns"): + _add_visible_sort_targets( + allowed, + query.get(key), + column_config, + is_metric=False, + ) + _add_visible_sort_targets( + allowed, + query.get("metrics"), + column_config, + is_metric=True, + ) + + return allowed + + +def _collect_stored_orderby_entries( + stored_chart: "Slice", + stored_query_context: Optional[dict[str, Any]], +) -> set[str]: + """ + Frozen saved orderby entries a guest may replay exactly. + """ + allowed: set[str] = { + freeze_value(entry) for entry in stored_chart.params_dict.get("orderby") or [] + } + if stored_query_context: + for query in stored_query_context.get("queries") or []: + allowed.update(freeze_value(entry) for entry in query.get("orderby") or []) + return allowed + + +def _metric_control_values(value: Any) -> list[Any]: + """ + Return non-empty values from a metric-valued control. + """ + if value is None or value == "": + return [] + if isinstance(value, (list, tuple)): + return [item for item in value if item is not None and item != ""] + return [value] - def add(values: Any) -> None: - for value in values or []: - allowed.add(freeze_value(value)) - def add_orderby(entries: Any) -> None: - for entry in entries or []: - if isinstance(entry, (list, tuple)) and entry: - allowed.add(freeze_value(entry[0])) +def _add_frozen_metric_control_values(allowed: set[str], value: Any) -> None: + """ + Add exact metric-control values to an authorization set. + """ + allowed.update(freeze_value(metric) for metric in _metric_control_values(value)) + +def _collect_stored_series_limit_metric_identifiers( + stored_chart: "Slice", + stored_query_context: Optional[dict[str, Any]], +) -> set[str]: + """ + Exact metric selectors a guest may use for series limiting. + """ + allowed: set[str] = set() params = stored_chart.params_dict - for key in ("columns", "groupby", "metrics", "all_columns"): - add(params.get(key)) - # Legacy charts store a single metric under the singular ``metric`` key. - add([params["metric"]] if params.get("metric") is not None else None) - add_orderby(params.get("orderby")) + + _add_frozen_metric_control_values(allowed, params.get("metrics")) + _add_frozen_metric_control_values(allowed, params.get("metric")) + for key in ("series_limit_metric", "timeseries_limit_metric"): + _add_frozen_metric_control_values(allowed, params.get(key)) if stored_query_context: for query in stored_query_context.get("queries") or []: - for key in ("columns", "groupby", "metrics", "all_columns"): - add(query.get(key)) - add_orderby(query.get("orderby")) + _add_frozen_metric_control_values(allowed, query.get("metrics")) + for key in ("series_limit_metric", "timeseries_limit_metric"): + _add_frozen_metric_control_values(allowed, query.get(key)) return allowed +def _series_limit_metric_value_modified(value: Any, allowed: set[str]) -> bool: + """ + Whether a requested series-limit metric is absent from stored metric controls. + """ + for metric in _metric_control_values(value): + if not isinstance(metric, (str, dict)) or not metric: + return True + if freeze_value(metric) not in allowed: + return True + return False + + +def _series_limit_metric_modified( + query_context: "QueryContext", + form_data: dict[str, Any], + stored_chart: "Slice", + stored_query_context: Optional[dict[str, Any]], +) -> bool: + """ + Whether series-limit metric selectors introduce a metric not stored on chart. + + Series limiting uses the selector to rank top-N groups, so guest requests may + only reuse stored metric controls. Dict-shaped selectors must match exactly; + labels are not an authorization key for expression objects. + """ + allowed: set[str] = _collect_stored_series_limit_metric_identifiers( + stored_chart, + stored_query_context, + ) + for key in ("series_limit_metric", "timeseries_limit_metric"): + if _series_limit_metric_value_modified(form_data.get(key), allowed): + return True + + for query in query_context.queries: + for key in ("series_limit_metric", "timeseries_limit_metric"): + if _series_limit_metric_value_modified(getattr(query, key, None), allowed): + return True + + return False + + +def _is_valid_orderby_entry(entry: Any) -> bool: + """ + Whether an orderby entry has the expected ``[term, ascending]`` shape. + """ + return ( + isinstance(entry, (list, tuple)) + and len(entry) == 2 + and isinstance(entry[0], (str, dict)) + and bool(entry[0]) + and isinstance(entry[1], bool) + ) + + def _orderby_modified( query_context: "QueryContext", stored_chart: "Slice", @@ -575,28 +777,35 @@ def _orderby_modified( metrics is legitimate and must not read as tampering; introducing a new expression is not, and is rejected. """ - allowed = _collect_sortable_identifiers(stored_chart, stored_query_context) + visible_targets = _collect_sortable_identifiers(stored_chart, stored_query_context) + stored_orderby_entries = _collect_stored_orderby_entries( + stored_chart, stored_query_context + ) form_data = query_context.form_data or {} # Both ``form_data`` and each ``QueryObject`` can carry an order-by, and in # the common frontend path they carry the same one. Either source could # smuggle an unauthorized term, so validate the union of both rather than # trusting one over the other; the duplication is harmless. - requested = list(form_data.get("orderby") or []) + form_orderby = form_data.get("orderby") + if form_orderby is not None and not isinstance(form_orderby, list): + return True + requested: list[Any] = list(form_orderby or []) for query in query_context.queries: - requested.extend(getattr(query, "orderby", None) or []) + query_orderby = getattr(query, "orderby", None) + if query_orderby is not None and not isinstance(query_orderby, list): + return True + requested.extend(query_orderby or []) for entry in requested: # Order-by entries must be ``(column_or_metric, ascending)`` pairs. A # malformed shape (e.g. a bare string or nested list) is not a valid # sort the chart could have produced, so treat it as tampering rather # than letting it crash query building when it is later unpacked. - if not ( - isinstance(entry, (list, tuple)) - and len(entry) == 2 - and isinstance(entry[1], bool) - ): + if not _is_valid_orderby_entry(entry): return True - if freeze_value(entry[0]) not in allowed: + if freeze_value(entry) in stored_orderby_entries: + continue + if not _requested_sort_target_identifiers(entry[0]) & visible_targets: return True return False @@ -667,7 +876,6 @@ def query_context_modified(query_context: "QueryContext") -> bool: if form_data is None: return False - # cannot request a different chart if form_data.get("slice_id") != stored_chart.id: return True @@ -685,6 +893,14 @@ def query_context_modified(query_context: "QueryContext") -> bool: ): return True + if _series_limit_metric_modified( + query_context, + form_data, + stored_chart, + stored_query_context, + ): + return True + # Order-by may sort only by columns/metrics already present in the stored # chart; new expressions (e.g. ``random()``) are still rejected. if _orderby_modified(query_context, stored_chart, stored_query_context): diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py index 58411fcfae76..9603e4cd0f51 100644 --- a/tests/unit_tests/models/helpers_test.py +++ b/tests/unit_tests/models/helpers_test.py @@ -21,7 +21,7 @@ import copy from contextlib import contextmanager -from typing import cast, TYPE_CHECKING +from typing import Any, cast, TYPE_CHECKING from unittest.mock import MagicMock, patch import pytest @@ -2057,6 +2057,106 @@ def test_orderby_adhoc_column(database: Database) -> None: assert "ORDER BY" in sql.upper() +def test_orderby_adhoc_column_label_takes_precedence_over_saved_metric( + database: Database, +) -> None: + """ + Test that orderby by an adhoc column label resolves to the selected column. + """ + from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn + + table: SqlaTable = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[ + TableColumn(column_name="a"), + TableColumn(column_name="b"), + ], + metrics=[ + SqlMetric(metric_name="custom_col", expression="SUM(a)"), + ], + ) + + result = table.get_sqla_query( + columns=[ + {"expressionType": "SQL", "label": "custom_col", "sqlExpression": "a + 1"}, + "b", + ], + orderby=[("custom_col", False)], + metrics=[], + extras={}, + filter=[], + granularity=None, + is_timeseries=False, + ) + + sql = str(result.sqla_query).upper() + assert "ORDER BY" in sql + assert "SUM(A)" not in sql + + +@pytest.mark.parametrize("aggregate", [None, "MEDIAN", ["SUM"], {"op": "SUM"}]) +def test_adhoc_metric_to_sqla_invalid_simple_aggregate_raises_validation_error( + database: Database, + aggregate: Any, +) -> None: + """ + Test that malformed SIMPLE adhoc metrics fail with a validation error. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.exceptions import QueryObjectValidationError + + table: SqlaTable = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[ + TableColumn(column_name="a"), + ], + ) + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "a"}, + "label": "Invalid metric", + } + if aggregate is not None: + metric["aggregate"] = aggregate + + with pytest.raises(QueryObjectValidationError): + table.adhoc_metric_to_sqla(metric, {}) + + +@pytest.mark.parametrize("sql_expression", [None, "", " "]) +def test_adhoc_metric_to_sqla_invalid_sql_expression_raises_validation_error( + database: Database, + sql_expression: str | None, +) -> None: + """ + Test that malformed SQL adhoc metrics fail with a validation error. + """ + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.exceptions import QueryObjectValidationError + + table = SqlaTable( + database=database, + schema=None, + table_name="t", + columns=[ + TableColumn(column_name="a"), + ], + ) + metric: AdhocMetric = { + "expressionType": "SQL", + "label": "Invalid metric", + } + if sql_expression is not None: + metric["sqlExpression"] = sql_expression + + with pytest.raises(QueryObjectValidationError): + table.adhoc_metric_to_sqla(metric, {}) + + def test_extras_where_is_parenthesized( database: Database, ) -> None: diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index a25375445a68..5a2722501ec4 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -20,6 +20,7 @@ import json # noqa: TID251 from types import SimpleNamespace from typing import Any, Optional +from unittest.mock import MagicMock import pytest from flask_appbuilder.security.sqla.models import Role, User @@ -1247,6 +1248,38 @@ def _table_sort_query_context( return query_context +def _series_limit_metric_query_context( + mocker: MockerFixture, + requested_metric: Any, + *, + stored_metrics: Optional[list[Any]] = None, + form_metric_key: str = "series_limit_metric", +) -> Any: + """ + Build a minimal chart query context with a series-limit metric selector. + """ + metrics: list[Any] = stored_metrics if stored_metrics is not None else ["count"] + query_kwargs: dict[str, Any] = {"metrics": metrics} + if form_metric_key == "series_limit_metric": + query_kwargs["series_limit_metric"] = requested_metric + + query_context = mocker.MagicMock() + query_context.queries = [ + QueryObject(**query_kwargs), + ] + query_context.form_data = { + "slice_id": 101, + "metrics": metrics, + form_metric_key: requested_metric, + } + query_context.slice_.id = 101 + query_context.slice_.params_dict = { + "metrics": metrics, + } + query_context.slice_.query_context = json.dumps({"queries": [{"metrics": metrics}]}) + return query_context + + def test_query_context_modified_orderby_sort_by_column(mocker: MockerFixture) -> None: """A guest sorting an embedded table by an existing column is allowed.""" query_context = _table_sort_query_context(mocker, orderby=[("gender", True)]) @@ -1412,6 +1445,31 @@ def test_query_context_modified_time_grain_native_filter( assert not query_context_modified(query_context) +def test_query_context_modified_orderby_visible_column_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that guest user can sort by a visible column (whitelist approach). + """ + query_context: MagicMock = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name", "country"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [["name", True]], # Sort by visible column + } + query_context.queries = [] + + assert not query_context_modified(query_context) + + def test_query_context_modified_time_grain_with_tampered_column( mocker: MockerFixture, ) -> None: @@ -1467,6 +1525,31 @@ def test_query_context_modified_time_grain_with_tampered_column( assert query_context_modified(query_context) +def test_query_context_modified_orderby_hidden_column_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that guest user cannot sort by a hidden column (data exfiltration prevention). + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [["credit_card_number", True]], # Hidden column - blocked! + } + query_context.queries = [] + + assert query_context_modified(query_context) + + def test_query_context_modified_time_grain_in_orderby( mocker: MockerFixture, ) -> None: @@ -1519,6 +1602,442 @@ def test_query_context_modified_time_grain_in_orderby( assert not query_context_modified(query_context) +def test_query_context_modified_orderby_direction_change_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that changing sort direction (ASC/DESC) is allowed for visible columns. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + "orderby": [["name", True]], # Original: ASC + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [["name", False]], # Changed to DESC - should be allowed + } + query_context.queries = [] + + assert not query_context_modified(query_context) + + +def test_query_context_modified_orderby_raw_table_all_columns_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that a raw-records Table chart can be sorted by its `all_columns`. + + The Table plugin's raw "Query mode" stores its selected columns under + `all_columns` rather than `columns`, so guests must be able to sort by them. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "query_mode": "raw", + "all_columns": ["name", "country"], + "orderby": [], + } + query_context.form_data = { + "slice_id": 42, + "orderby": [["country", True]], # Sort by a raw-mode column + } + query_context.queries = [] + + assert not query_context_modified(query_context) + + +def test_query_context_modified_orderby_column_config_hidden_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that hidden Table columns cannot be used for guest sorting. + + Table renders only columns whose column_config entry is not visible=false, + so a manually supplied sort by a hidden-but-selected column must be blocked. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "query_mode": "raw", + "all_columns": ["name", "secret_column"], + "column_config": { + "secret_column": { + "visible": False, + }, + }, + } + query_context.form_data = { + "slice_id": 42, + "orderby": [["secret_column", True]], + } + query_context.queries = [ + QueryObject( + orderby=[("secret_column", True)], + ), + ] + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_adhoc_metric_without_label_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that guests may sort by a visible adhoc SIMPLE metric without a label. + """ + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "sales", "type": "BIGINT"}, + "aggregate": "SUM", + } + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "metrics": [metric], + "orderby": [], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": [metric], + "orderby": [["SUM(sales)", True]], + } + query_context.queries = [ + QueryObject( + columns=["name"], + metrics=[metric], + orderby=[("SUM(sales)", True)], + ), + ] + + assert not query_context_modified(query_context) + + +def test_query_context_modified_orderby_simple_metric_reused_metric_label_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that dict-shaped orderby terms cannot pass by reusing a metric label. + """ + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "secret_sales"}, + "aggregate": "SUM", + "label": "count", + } + query_context = _table_sort_query_context( + mocker, + orderby=[(metric, True)], + stored_metrics=["count"], + ) + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_simple_metric_reused_column_label_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that dict-shaped orderby terms cannot pass by reusing a column label. + """ + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "secret_sales"}, + "aggregate": "SUM", + "label": "name", + } + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [[metric, True]], + } + query_context.queries = [ + QueryObject( + columns=["name"], + metrics=["count"], + orderby=[(metric, True)], + ), + ] + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_simple_metric_bad_aggregate_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that malformed SIMPLE metric orderby cannot pass through label spoofing. + """ + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "secret_sales"}, + "label": "count", + } + query_context = _table_sort_query_context( + mocker, + orderby=[(metric, True)], + stored_metrics=["count"], + ) + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_adhoc_column_label_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that guests may sort by a visible adhoc column result key. + """ + column: AdhocColumn = { + "label": "Full Name", + "sqlExpression": "CONCAT(first_name, last_name)", + } + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": [column], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": [column], + "metrics": ["count"], + "orderby": [["Full Name", True]], + } + query_context.queries = [ + QueryObject( + columns=[column], + metrics=["count"], + orderby=[("Full Name", True)], + ), + ] + + assert not query_context_modified(query_context) + + +def test_query_context_modified_orderby_hidden_stored_orderby_replay_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that replaying an exact owner-defined orderby is not tampering. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "all_columns": ["name", "secret_column"], + "column_config": { + "secret_column": { + "visible": False, + }, + }, + "orderby": [["secret_column", True]], + } + query_context.form_data = { + "slice_id": 42, + "orderby": [["secret_column", True]], + } + query_context.queries = [ + QueryObject( + orderby=[("secret_column", True)], + ), + ] + + assert not query_context_modified(query_context) + + +def test_query_context_modified_orderby_hidden_stored_orderby_direction_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that guests cannot change direction on a hidden owner-defined sort. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "all_columns": ["name", "secret_column"], + "column_config": { + "secret_column": { + "visible": False, + }, + }, + "orderby": [["secret_column", True]], + } + query_context.form_data = { + "slice_id": 42, + "orderby": [["secret_column", False]], + } + query_context.queries = [ + QueryObject( + orderby=[("secret_column", False)], + ), + ] + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_sql_expression_reused_label_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that a new SQL expression object cannot pass by reusing a visible label. + """ + sql_expression: AdhocColumn = { + "label": "name", + "sqlExpression": "random()", + } + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [[sql_expression, True]], + } + query_context.queries = [ + QueryObject( + columns=["name"], + metrics=["count"], + orderby=[(sql_expression, True)], + ), + ] + + assert query_context_modified(query_context) + + +def test_query_context_modified_series_limit_metric_stored_metric_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that a stored metric can be reused as the series-limit selector. + """ + query_context = _series_limit_metric_query_context( + mocker, + requested_metric="count", + stored_metrics=["count"], + ) + + assert not query_context_modified(query_context) + + +def test_query_context_modified_timeseries_limit_metric_stored_metric_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that the deprecated form-data control name follows the same guard. + """ + query_context = _series_limit_metric_query_context( + mocker, + requested_metric="count", + stored_metrics=["count"], + form_metric_key="timeseries_limit_metric", + ) + + assert not query_context_modified(query_context) + + +def test_query_context_modified_series_limit_metric_exact_adhoc_metric_allowed( + mocker: MockerFixture, +) -> None: + """ + Test that a stored adhoc metric can be reused as the series-limit selector. + """ + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "sales"}, + "aggregate": "SUM", + "label": "SUM(sales)", + } + query_context = _series_limit_metric_query_context( + mocker, + requested_metric=metric, + stored_metrics=[metric], + ) + + assert not query_context_modified(query_context) + + +def test_query_context_modified_series_limit_metric_off_chart_metric_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that a guest cannot introduce an off-chart series-limit metric. + """ + query_context = _series_limit_metric_query_context( + mocker, + requested_metric="revenue", + stored_metrics=["count"], + ) + + assert query_context_modified(query_context) + + +def test_query_context_modified_series_limit_metric_sql_expression_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that a guest cannot introduce a SQL expression as series-limit metric. + """ + metric: AdhocMetric = { + "expressionType": "SQL", + "sqlExpression": "random()", + "label": "count", + } + query_context = _series_limit_metric_query_context( + mocker, + requested_metric=metric, + stored_metrics=["count"], + ) + + assert query_context_modified(query_context) + + +def test_query_context_modified_series_limit_metric_bad_aggregate_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that malformed series-limit metrics cannot pass through label spoofing. + """ + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "column": {"column_name": "secret_sales"}, + "label": "count", + } + query_context = _series_limit_metric_query_context( + mocker, + requested_metric=metric, + stored_metrics=["count"], + ) + + assert query_context_modified(query_context) + + def test_get_catalog_perm() -> None: """ Test the `get_catalog_perm` method. @@ -2166,3 +2685,204 @@ def test_reset_password_self_service_pk_string_clears_flag( # Coerced to int when clearing, regardless of the inbound id type. mock_clear.assert_called_once_with(5) + + +# ----------------------------------------------------------------------------- +# Tests for orderby with invalid formats - unit tests +# ----------------------------------------------------------------------------- + + +def test_query_context_modified_orderby_sql_expression_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that SQL expressions in orderby are blocked. + + Security: prevents SQL injection via sorting. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [[{"expressionType": "SQL", "sqlExpression": "random()"}, True]], + } + query_context.queries = [] + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_invalid_format_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that invalid orderby formats are blocked (not crash). + + The invalid format {"column": "string"} should be blocked, + not cause AttributeError. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + # Invalid format - should be blocked, not crash + "orderby": [[{"column": "country"}, True]], + } + query_context.queries = [] + + # Should return True (modified/blocked), not raise exception + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_string_instead_of_list_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that orderby as string (instead of list) is blocked. + + Defensive barrier: if orderby is not a list, block (fail-closed). + Without this barrier, iterating over string would yield characters, + each would be skipped, and the check would pass (fail-open). + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + # Invalid: string instead of list + "orderby": "malicious_string", + } + query_context.queries = [] + + # Should return True (blocked), not pass through + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_element_not_tuple_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that orderby element that is not tuple/list is blocked. + + Defensive barrier: each orderby element must be [column, bool]. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + # Invalid: element is string, not tuple + "orderby": ["name"], + } + query_context.queries = [] + + # Should return True (blocked) + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_empty_tuple_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that empty orderby tuple is blocked. + + Defensive barrier: empty tuples are invalid. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + # Invalid: empty tuple + "orderby": [[]], + } + query_context.queries = [] + + # Should return True (blocked) + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_missing_direction_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that orderby entries without a direction are blocked. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [["name"]], + } + query_context.queries = [] + + assert query_context_modified(query_context) + + +def test_query_context_modified_orderby_non_bool_direction_blocked( + mocker: MockerFixture, +) -> None: + """ + Test that orderby direction must be a boolean for new guest sort terms. + """ + query_context = mocker.MagicMock() + query_context.slice_.id = 42 + query_context.slice_.query_context = None + query_context.slice_.params_dict = { + "columns": ["name"], + "groupby": [], + "metrics": ["count"], + } + query_context.form_data = { + "slice_id": 42, + "columns": ["name"], + "metrics": ["count"], + "orderby": [["name", "true"]], + } + query_context.queries = [] + + assert query_context_modified(query_context)