sadpandajoe commented on code in PR #43391:
URL: https://github.com/apache/superset/pull/43391#discussion_r3993983487
##########
superset/security/manager.py:
##########
@@ -1615,6 +1662,146 @@ 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:
Review Comment:
This still treats an annotation layer found only in the stored query context
as authorized, so a params-only chart update that removes a layer can leave
embedded guests able to request that stale layer. Should this require the layer
to remain in the current chart params rather than unioning both stored
representations?
--
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]
