rusackas commented on code in PR #41218:
URL: https://github.com/apache/superset/pull/41218#discussion_r3440546886
##########
superset/security/manager.py:
##########
@@ -520,42 +520,87 @@ def _native_filter_request_modified(query_context:
"QueryContext") -> bool:
)
-def query_context_modified(query_context: "QueryContext") -> bool:
+def _collect_sortable_identifiers(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
"""
- Check if a query context has been modified.
-
- This is used to ensure guest users don't modify the payload and fetch data
- different from what was shared with them in dashboards.
+ 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.
"""
- form_data = query_context.form_data
- stored_chart = query_context.slice_
+ allowed: set[str] = set()
- # Native-filter data requests have no associated chart (no slice_id).
Rather
- # 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).
- if stored_chart is None:
- return _native_filter_request_modified(query_context)
+ def add(values: Any) -> None:
+ for value in values or []:
+ allowed.add(freeze_value(value))
- if form_data is None:
- return False
+ def add_orderby(entries: Any) -> None:
+ for entry in entries or []:
+ if isinstance(entry, (list, tuple)) and entry:
+ allowed.add(freeze_value(entry[0]))
- # cannot request a different chart
- if form_data.get("slice_id") != stored_chart.id:
- return True
+ params = stored_chart.params_dict
+ for key in ("columns", "groupby", "metrics", "all_columns"):
+ add(params.get(key))
Review Comment:
Good catch on the legacy singular `metric` — added it to the allowed sort
targets and a test for it.
##########
superset/security/manager.py:
##########
@@ -520,42 +520,87 @@ def _native_filter_request_modified(query_context:
"QueryContext") -> bool:
)
-def query_context_modified(query_context: "QueryContext") -> bool:
+def _collect_sortable_identifiers(
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> set[str]:
"""
- Check if a query context has been modified.
-
- This is used to ensure guest users don't modify the payload and fetch data
- different from what was shared with them in dashboards.
+ 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.
"""
- form_data = query_context.form_data
- stored_chart = query_context.slice_
+ allowed: set[str] = set()
- # Native-filter data requests have no associated chart (no slice_id).
Rather
- # 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).
- if stored_chart is None:
- return _native_filter_request_modified(query_context)
+ def add(values: Any) -> None:
+ for value in values or []:
+ allowed.add(freeze_value(value))
- if form_data is None:
- return False
+ def add_orderby(entries: Any) -> None:
+ for entry in entries or []:
+ if isinstance(entry, (list, tuple)) and entry:
+ allowed.add(freeze_value(entry[0]))
- # cannot request a different chart
- if form_data.get("slice_id") != stored_chart.id:
- return True
+ params = stored_chart.params_dict
+ for key in ("columns", "groupby", "metrics", "all_columns"):
+ add(params.get(key))
+ add_orderby(params.get("orderby"))
- stored_query_context = (
- json.loads(cast(str, stored_chart.query_context))
- if stored_chart.query_context
- else None
- )
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ for key in ("columns", "groupby", "metrics"):
+ add(query.get(key))
+ add_orderby(query.get("orderby"))
+
+ return allowed
- # compare columns and metrics in form_data with stored values
+
+def _orderby_modified(
+ query_context: "QueryContext",
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+ """
+ Whether any order-by clause sorts by a term the stored chart does not
already
+ reference.
+
+ A guest reordering an embedded chart by one of its existing columns or
+ 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)
+ form_data = query_context.form_data or {}
+ requested = list(form_data.get("orderby") or [])
+ for query in query_context.queries:
+ requested.extend(getattr(query, "orderby", None) or [])
+
+ for entry in requested:
+ term = entry[0] if isinstance(entry, (list, tuple)) and entry else
entry
+ if freeze_value(term) not in allowed:
+ return True
Review Comment:
Fair — order-by entries now have to be `(term, bool)` pairs or they read as
tampering, so a bare/nested shape gets rejected before it can blow up query
building.
--
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]
