sadpandajoe commented on code in PR #43391:
URL: https://github.com/apache/superset/pull/43391#discussion_r3833378745
##########
superset/security/manager.py:
##########
@@ -1562,6 +1563,120 @@ def _columns_metrics_modified(
return False
+def _annotation_layer_identity(layer: Any) -> Optional[tuple[str, str]]:
+ """
+ Identity of an annotation layer for tamper comparison: the source type and
+ the underlying source it reads (a native annotation-layer id or a chart
+ id). Cosmetic keys (``name``, styling, overrides) are not part of the
+ identity. Returns ``None`` for a malformed (non-dict) layer.
+ """
+ if not isinstance(layer, dict):
+ return None
+ return (
+ freeze_value(layer.get("sourceType")),
+ freeze_value(layer.get("value")),
+ )
+
+
+def _annotation_layers_modified(
+ query_context: "QueryContext",
+ form_data: dict[str, Any],
+ stored_chart: "Slice",
+ stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+ """
+ Whether the request references annotation layers the stored chart does
+ not already carry.
+
+ ``annotation_layers`` is accepted on any query object, and native layers
+ resolve every annotation of each referenced layer id with no further
+ access check, so a guest injecting a layer the chart was not saved with
+ would read data that was never shared with them. Replaying the chart's
+ own stored layers is not tampering.
+ """
+ requested: set[Optional[tuple[str, str]]] = {
+ _annotation_layer_identity(layer)
+ for layer in form_data.get("annotation_layers") or []
+ }
+ requested.update(
+ _annotation_layer_identity(layer)
+ for query in query_context.queries
+ for layer in getattr(query, "annotation_layers", None) or []
+ )
+ if not requested:
+ return False
+ # A malformed (non-dict) layer is nothing the frontend produces from a
+ # stored chart; treat it as tampering rather than crashing on it later.
+ if None in requested:
+ return True
+
+ stored: set[Optional[tuple[str, str]]] = {
+ _annotation_layer_identity(layer)
+ for layer in stored_chart.params_dict.get("annotation_layers") or []
+ }
+ if stored_query_context:
+ for query in stored_query_context.get("queries") or []:
+ stored.update(
+ _annotation_layer_identity(layer)
+ for layer in query.get("annotation_layers") or []
+ )
+ return not requested.issubset(stored)
+
+
+#: Result types that make the server rewrite the query to return raw rows of
+#: every datasource column (``_prepare_samples_query`` and
+#: ``_prepare_drill_detail_query`` in ``superset.common.query_actions``).
+_ROW_EXPANDING_RESULT_TYPES = {
+ ChartDataResultType.SAMPLES.value,
+ ChartDataResultType.DRILL_DETAIL.value,
+}
+
+
+def _result_type_value(result_type: Any) -> str:
+ """Normalize a result type (enum member or raw string) to its value."""
+ return str(getattr(result_type, "value", result_type)).lower()
+
+
+def _result_type_modified(
+ query_context: "QueryContext",
+ stored_query_context: Optional[dict[str, Any]],
+) -> bool:
+ """
+ Whether the request asks for a result type that expands the stored chart's
+ query to raw datasource rows.
+
+ The ``samples`` and ``drill_detail`` preparers replace the query's columns
+ with every column on the datasource - and drop its metrics - *after*
+ ``raise_for_access`` has run, so the subset comparisons on columns and
+ metrics in ``query_context_modified`` still pass while the response
+ contains the full underlying table. A guest's entitlement is the chart's
+ rendered data, so these result types are rejected unless the chart's
+ stored query context itself uses them.
+ """
+ stored_result_types: set[str] = set()
+ if stored_query_context:
+ if stored_type := stored_query_context.get("result_type"):
+ stored_result_types.add(_result_type_value(stored_type))
+ for stored_query in stored_query_context.get("queries") or []:
+ if isinstance(stored_query, dict) and (
+ stored_type := stored_query.get("result_type")
+ ):
+ stored_result_types.add(_result_type_value(stored_type))
+
+ requested_result_types = {_result_type_value(query_context.result_type)}
+ requested_result_types.update(
+ _result_type_value(query.result_type)
+ for query in query_context.queries
+ if query.result_type
+ )
+
+ return any(
+ result_type in _ROW_EXPANDING_RESULT_TYPES
Review Comment:
Because this combines every saved result type into one set, a multi-query
chart with one saved `samples` query lets a guest change a different query to
`samples`; its preparer then returns that query's raw datasource rows. Could
this compare each requested query with its corresponding stored query instead?
--
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]
