Fix correctness defects from security audit (GDS-321)

changelog.md

@@ -7,6 +7,11 @@
 ## Bug fixes
 
 * Fixed a stored cross-site scripting (XSS) vulnerability in `VG.render()`. Graph data was injected into an executable `<script>` block, so a node caption or property value containing `</script>` could break out and run arbitrary code in the browser of anyone opening a saved visualization. Data is now delivered as an inert `<script type="application/json">` block and read back with `JSON.parse`, with `<` escaped so no `</script>` can appear literally. The `render_widget` was unaffected.
+* Fixed `widget.remove_data` leaving dangling relationships when only nodes were removed.
+* Fixed `widget.remove_data` silently doing nothing when the id type differed (e.g. `Node(id=1)` vs `remove_data(nodes="1")`).
+* Fixed `VisualizationGraph.resize_nodes` and `color_nodes` crashing with `ValueError: min() iterable argument is empty` when no node has the requested property. They now raise a `ValueError` that names the missing property.
+* Fixed `VisualizationGraph.color_nodes` with `color_space=ColorSpace.CONTINUOUS` raising an unhelpful `TypeError` when colouring non-numeric (text) values. It now raises a `ValueError` suggesting `ColorSpace.DISCRETE`.
+* Fixed the `max_allowed_nodes` limit only being enforced at draw time. `GraphWidget.add_data` now rejects additions that would exceed the limit set when the widget was created (via `render_widget`/`from_graph_data`), so the graph can no longer be grown past it afterwards.
 
 ## Improvements
 

python-wrapper/src/neo4j_viz/_graph_entity_operations.py

@@ -132,6 +132,12 @@ def resize_nodes(
                 if node.size is not None:
                     all_sizes[node.id] = node.size
 
+        if property is not None and not all_sizes:
+            raise ValueError(
+                f"No node has the property {property!r}, so sizes cannot be computed. "
+                "Check the spelling, or pass explicit `sizes`."
+            )
+
         # Validate node sizes
         for id, size in all_sizes.items():
             if size is None:
@@ -207,6 +213,9 @@ def resize_relationships(
     def _normalize_values(
         node_map: dict[NodeIdType, RealNumber], min_max: tuple[float, float] = (0, 1)
     ) -> dict[NodeIdType, RealNumber]:
+        if not node_map:
+            return {}
+
         unscaled_min_size = min(node_map.values())
         unscaled_max_size = max(node_map.values())
         unscaled_size_range = float(unscaled_max_size - unscaled_min_size)
@@ -262,6 +271,21 @@ def node_to_attr(node: Node) -> Any:
                 colors = NEO4J_COLORS_DISCRETE
         else:
             node_map = {node.id: node_to_attr(node) for node in self.nodes if node_to_attr(node) is not None}
+
+            if not node_map:
+                raise ValueError(
+                    f"No node has a value for {attribute!r}, so continuous coloring cannot be computed. "
+                    "Check the spelling, or use `color_space=ColorSpace.DISCRETE` for categorical values."
+                )
+
+            for node_id, value in node_map.items():
+                if isinstance(value, bool) or not isinstance(value, (int, float)):
+                    raise ValueError(
+                        f"Continuous coloring needs numeric values, but node {node_id!r} has "
+                        f"{attribute}={value!r} ({type(value).__name__}). "
+                        "Use `color_space=ColorSpace.DISCRETE` for non-numeric values."
+                    )
+
             normalized_map = self._normalize_values(node_map)
 
             if colors is None:

python-wrapper/src/neo4j_viz/visualization_graph.py

@@ -572,4 +572,5 @@ def render_widget(
             options=render_options,
             theme=theme,
             legend=self.legend,
+            max_allowed_nodes=max_allowed_nodes,
         )

python-wrapper/src/neo4j_viz/widget.py

@@ -125,6 +125,8 @@ class GraphWidget(anywidget.AnyWidget):
         from_json=lambda value, widget: Legend.model_validate(value),
     )
 
+    _max_allowed_nodes: int = 10_000
+
     def on_selection_change(self, callback: Callable[[GraphSelection], None]) -> Callable[[dict[str, Any]], None]:
         """
         Register a callback that fires whenever the widget's `selected` trait changes.
@@ -173,9 +175,10 @@ def from_graph_data(
         options: RenderOptions | None = None,
         theme: str = "auto",
         legend: Legend | None = None,
+        max_allowed_nodes: int = 10_000,
     ) -> GraphWidget:
         """Create a GraphWidget from Node and Relationship lists."""
-        return cls(
+        widget = cls(
             nodes=nodes,
             relationships=relationships,
             width=width,
@@ -184,6 +187,8 @@ def from_graph_data(
             theme=theme,
             legend=legend if legend is not None else Legend(),
         )
+        widget._max_allowed_nodes = max_allowed_nodes
+        return widget
 
     def __str__(self) -> str:
         return f"GraphWidget(nodes={len(self.nodes)}, relationships={len(self.relationships)}, options={self.options}, theme={self.theme}, width={self.width}, height={self.height})"
@@ -622,6 +627,14 @@ def add_data(
         if isinstance(relationships, Relationship):
             relationships = [relationships]
 
+        if nodes and len(self.nodes) + len(nodes) > self._max_allowed_nodes:
+            raise ValueError(
+                f"Adding {len(nodes)} nodes would result in {len(self.nodes) + len(nodes)} nodes, "
+                f"which exceeds the maximum of {self._max_allowed_nodes} nodes set when this widget "
+                "was created. It can be increased by overriding `max_allowed_nodes` in "
+                "`render_widget`, but rendering could then take a long time."
+            )
+
         if nodes:
             self.nodes = self.nodes + nodes
         if relationships:
@@ -644,33 +657,39 @@ def remove_data(
         relationships:
             Relationships to remove from the graph widget.
         """
+        # Compare ids as strings on both sides, matching how ids are serialized for the
+        # frontend (see _validation.check_dangling_relationships): ``Node(id=1)`` and a
+        # request to remove ``"1"`` refer to the same node.
         if isinstance(nodes, Node):
-            node_ids_to_remove = {nodes.id}
+            node_ids_to_remove = {str(nodes.id)}
         elif isinstance(nodes, NodeIdType):
-            node_ids_to_remove = {nodes}
+            node_ids_to_remove = {str(nodes)}
         elif nodes is None:
             node_ids_to_remove = set()
         else:
-            node_ids_to_remove = {n.id if isinstance(n, Node) else n for n in nodes}
+            node_ids_to_remove = {str(n.id) if isinstance(n, Node) else str(n) for n in nodes}
 
         if isinstance(relationships, Relationship):
-            rel_ids_to_remove = {relationships.id}
+            rel_ids_to_remove = {str(relationships.id)}
         elif isinstance(relationships, RelationshipIdType):
-            rel_ids_to_remove = {relationships}
+            rel_ids_to_remove = {str(relationships)}
         elif relationships is None:
             rel_ids_to_remove = set()
         else:
-            rel_ids_to_remove = {r.id if isinstance(r, Relationship) else r for r in relationships}
+            rel_ids_to_remove = {str(r.id) if isinstance(r, Relationship) else str(r) for r in relationships}
 
         if node_ids_to_remove:
-            self.nodes = [n for n in self.nodes if n.id not in node_ids_to_remove]
+            self.nodes = [n for n in self.nodes if str(n.id) not in node_ids_to_remove]
 
         def keep_rel(r: Relationship) -> bool:
             return (
-                r.id not in rel_ids_to_remove
-                and r.source not in node_ids_to_remove
-                and r.target not in node_ids_to_remove
+                str(r.id) not in rel_ids_to_remove
+                and str(r.source) not in node_ids_to_remove
+                and str(r.target) not in node_ids_to_remove
             )
 
-        if rel_ids_to_remove:
+        # Run the cleanup whenever anything is being removed. A node-only delete must also
+        # drop the relationships that pointed at it, otherwise the frontend silently renders
+        # an empty graph (see _validation.check_dangling_relationships).
+        if node_ids_to_remove or rel_ids_to_remove:
             self.relationships = [r for r in self.relationships if keep_rel(r)]

python-wrapper/tests/test_color_nodes.py

@@ -290,3 +290,21 @@ def test_color_nodes_override_false() -> None:
     assert VG.nodes[0].color == Color("#ff0000")  # Should keep existing color
     assert VG.nodes[1].color == Color("#ff0000")  # Should keep existing color
     assert VG.nodes[2].color == Color("#00ff00")  # Should get new color (no existing color)
+
+
+def test_color_nodes_continuous_missing_property_raises() -> None:
+    VG = VisualizationGraph(nodes=[Node(id="1"), Node(id="2")], relationships=[])
+
+    with pytest.raises(ValueError, match="No node has a value for 'name'"):
+        VG.color_nodes(property="name", color_space=ColorSpace.CONTINUOUS)
+
+
+def test_color_nodes_continuous_non_numeric_raises() -> None:
+    nodes = [
+        Node(id="1", properties={"name": "Alice"}),
+        Node(id="2", properties={"name": "Bob"}),
+    ]
+    VG = VisualizationGraph(nodes=nodes, relationships=[])
+
+    with pytest.raises(ValueError, match="Continuous coloring needs numeric values"):
+        VG.color_nodes(property="name", color_space=ColorSpace.CONTINUOUS)

python-wrapper/tests/test_resize_nodes.py

@@ -158,3 +158,10 @@ def test_resize_nodes_no_args_failure() -> None:
 
     with pytest.raises(ValueError, match="At least one of `sizes`, `property` or `node_radius_min_max` must be given"):
         VG.resize_nodes(node_radius_min_max=None)
+
+
+def test_resize_nodes_missing_property_raises() -> None:
+    VG = VisualizationGraph(nodes=[Node(id="1"), Node(id="2")], relationships=[])
+
+    with pytest.raises(ValueError, match=re.escape("No node has the property 'pagerank'")):
+        VG.resize_nodes(property="pagerank")

python-wrapper/tests/test_widget.py

@@ -213,6 +213,57 @@ def test_remove_data(self) -> None:
         assert {n.id for n in widget.nodes} == {"n3"}
         assert {r.id for r in widget.relationships} == {43}
 
+    def test_remove_data_nodes_only_deletes_dangling_relationships(self) -> None:
+        nodes = [Node(id="n1"), Node(id="n2")]
+        rels = [
+            Relationship(source="n1", target="n2"),
+            Relationship(source="n2", target="n2"),
+        ]
+        widget = GraphWidget.from_graph_data(nodes, rels)
+
+        widget.remove_data(nodes=["n1"])
+        assert {n.id for n in widget.nodes} == {"n2"}
+        # The relationship that pointed at the removed node is deleted, not left dangling.
+        assert {(r.source, r.target) for r in widget.relationships} == {("n2", "n2")}
+
+    def test_remove_data_relationships_only(self) -> None:
+        nodes = [Node(id="n1"), Node(id="n2")]
+        rels = [Relationship(id="r1", source="n1", target="n2"), Relationship(id="r2", source="n2", target="n1")]
+        widget = GraphWidget.from_graph_data(nodes, rels)
+
+        widget.remove_data(relationships=["r1"])
+        assert {n.id for n in widget.nodes} == {"n1", "n2"}
+        assert {r.id for r in widget.relationships} == {"r2"}
+
+    def test_remove_data_id_type_mismatch(self) -> None:
+        widget = GraphWidget.from_graph_data([Node(id=1), Node(id=2)], [Relationship(source=1, target=2)])
+
+        widget.remove_data(nodes="1")
+        assert {n.id for n in widget.nodes} == {2}
+        # Relationship pointing at the removed node is also deleted.
+        assert widget.relationships == []
+
+    def test_add_data_exceeds_max_allowed_nodes(self) -> None:
+        widget = GraphWidget.from_graph_data([Node(id="n1")], [], max_allowed_nodes=10)
+
+        with pytest.raises(ValueError, match="exceeds the maximum of 10 nodes"):
+            widget.add_data(nodes=[Node(id=f"x{i}") for i in range(10)])
+
+        # The graph must be left unchanged when the limit would be exceeded.
+        assert {n.id for n in widget.nodes} == {"n1"}
+
+    def test_add_data_max_allowed_nodes_threaded_from_render_widget(self) -> None:
+        """A custom max_allowed_nodes passed to render_widget is honored by add_data (L-03)."""
+        vg = VisualizationGraph(nodes=[Node(id="n1")], relationships=[])
+        widget = vg.render_widget(max_allowed_nodes=3)
+
+        # Up to the limit is fine.
+        widget.add_data(nodes=[Node(id="n2")])
+        assert len(widget.nodes) == 2
+
+        with pytest.raises(ValueError, match="exceeds the maximum of 3 nodes"):
+            widget.add_data(nodes=[Node(id=f"x{i}") for i in range(3)])
+
     def test_add_data_dangling_warns_by_default(self) -> None:
         widget = GraphWidget.from_graph_data([Node(id="n1")], [])
         with pytest.warns(UserWarning, match=re.escape("reference node ids that are not in the graph")):