From 3a8d61527aa5e6717b2fbdf3b01c09ef6947fd97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Florentin=20D=C3=B6rre?= Date: Fri, 14 Aug 2026 13:44:57 +0200 Subject: [PATCH] Fix correctness defects from security audit (GDS-321) Address five findings from the python-graph-visualization security audit (F-01 XSS and L-04 parser recursion are handled separately / deferred): - L-01: remove_data now deletes relationships left dangling by a node-only removal, instead of leaving them and silently rendering an empty graph. - L-02: remove_data compares ids as strings on both sides, so a numeric id (Node(id=1)) matches its text form (remove_data(nodes="1")). - F-02: resize_nodes/color_nodes raise a clear ValueError naming the missing property instead of crashing with min() iterable argument is empty. - F-03: color_nodes continuous raises a clear ValueError suggesting ColorSpace.DISCRETE for non-numeric values instead of a TypeError. - L-03: max_allowed_nodes is threaded into GraphWidget and enforced by add_data, so the draw-time node limit cannot be bypassed afterwards. # Conflicts: # changelog.md --- changelog.md | 5 ++ .../src/neo4j_viz/_graph_entity_operations.py | 24 +++++++++ .../src/neo4j_viz/visualization_graph.py | 1 + python-wrapper/src/neo4j_viz/widget.py | 43 +++++++++++----- python-wrapper/tests/test_color_nodes.py | 18 +++++++ python-wrapper/tests/test_resize_nodes.py | 7 +++ python-wrapper/tests/test_widget.py | 51 +++++++++++++++++++ 7 files changed, 137 insertions(+), 12 deletions(-) diff --git a/changelog.md b/changelog.md index 86779d3e..64b349d9 100644 --- a/changelog.md +++ b/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 `` could break out and run arbitrary code in the browser of anyone opening a saved visualization. Data is now delivered as an inert `` 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 diff --git a/python-wrapper/src/neo4j_viz/_graph_entity_operations.py b/python-wrapper/src/neo4j_viz/_graph_entity_operations.py index 2fa8343e..f9d2ac7c 100644 --- a/python-wrapper/src/neo4j_viz/_graph_entity_operations.py +++ b/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: diff --git a/python-wrapper/src/neo4j_viz/visualization_graph.py b/python-wrapper/src/neo4j_viz/visualization_graph.py index c7b00347..e0635522 100644 --- a/python-wrapper/src/neo4j_viz/visualization_graph.py +++ b/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, ) diff --git a/python-wrapper/src/neo4j_viz/widget.py b/python-wrapper/src/neo4j_viz/widget.py index 2791e606..5d0b5110 100644 --- a/python-wrapper/src/neo4j_viz/widget.py +++ b/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)] diff --git a/python-wrapper/tests/test_color_nodes.py b/python-wrapper/tests/test_color_nodes.py index dff4d2c8..59038846 100644 --- a/python-wrapper/tests/test_color_nodes.py +++ b/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) diff --git a/python-wrapper/tests/test_resize_nodes.py b/python-wrapper/tests/test_resize_nodes.py index 251fd351..156115d6 100644 --- a/python-wrapper/tests/test_resize_nodes.py +++ b/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") diff --git a/python-wrapper/tests/test_widget.py b/python-wrapper/tests/test_widget.py index c0623e99..37951be2 100644 --- a/python-wrapper/tests/test_widget.py +++ b/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")):