security: linear vector import, stroking and rounding, validation, and cyclic dev snapshots (#1306)

* security: linear vector import, stroking and rounding, validation and a cycle check for dev snapshots

Closing a subpath, rounding corners and chaining stroke runs each rescanned the whole
network per step, and Document::validate searched a parent's children per child. They now
update only the current subpath's last segment, rebuild each region loop in one pass, walk
per-vertex adjacency lists, and check edges against a once-built index. A dev snapshot whose
layers contain a cycle under the root is refused instead of reconstructed.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

* fix(doc): address review findings for snapshot children and 2-segment rounded loops

---------

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>

crates/doc/src/handoff.rs

@@ -71,8 +71,38 @@ impl Document {
         if let (Some(now), Some(r)) = (self.node(root), d.nodes.get_mut(&root)) {
             Arc::make_mut(r).parent = now.parent;
         }
+        // A damaged file can give a historical layer itself (or an ancestor) as a child; a copy
+        // with a cycle under the root would send every walk of it round forever.
+        if !d.is_tree_under(root) {
+            return None;
+        }
         d.dev_snapshots.clear();
         d.dirty = crate::Dirty::default();
         Some(d)
     }
+
+    /// Whether every layer under `root` is reached once (no layer is its own descendant or listed
+    /// by two parents), walking iteratively and stopping at the first repeat.
+    fn is_tree_under(&self, root: NodeId) -> bool {
+        let mut seen = BTreeSet::new();
+        let mut stack = vec![root];
+        while let Some(id) = stack.pop() {
+            if !seen.insert(id) {
+                return false;
+            }
+            let Some(n) = self.nodes.get(&id) else {
+                return false;
+            };
+            for &c in &n.children {
+                let Some(child) = self.nodes.get(&c) else {
+                    return false;
+                };
+                if child.parent != Some(id) {
+                    return false;
+                }
+                stack.push(c);
+            }
+        }
+        true
+    }
 }

crates/doc/src/handoff_tests.rs

@@ -34,3 +34,54 @@ fn a_snapshot_shows_the_layer_as_it_was() {
     doc.dev_snapshots.clear();
     assert!(serde_json::to_value(&doc).unwrap().get("dev_snapshots").is_none());
 }
+
+#[test]
+fn a_snapshot_with_a_layer_inside_itself_is_refused() {
+    let mut doc = Document::new("d");
+    let page = doc.first_page().unwrap();
+    let f = doc.create(page, NodeKind::Frame, "Screen", 0.0, 0.0, 100.0, 100.0).unwrap();
+    let mut snap = doc.snapshot_subtree(f).unwrap();
+    // A damaged file: the historical frame lists itself as a child.
+    let mut bad = (*snap.nodes[0]).clone();
+    bad.children.push(f);
+    snap.nodes[0] = Arc::new(bad);
+    doc.dev_snapshots.insert(f, Arc::new(snap));
+    assert!(doc.with_dev_snapshot(f).is_none());
+}
+
+#[test]
+fn a_snapshot_with_missing_or_nonreciprocal_child_is_refused() {
+    use crate::NodeId;
+    let mut doc = Document::new("d");
+    let page = doc.first_page().unwrap();
+    let f = doc.create(page, NodeKind::Frame, "Screen", 0.0, 0.0, 100.0, 100.0).unwrap();
+    let _c1 = doc.create(f, NodeKind::Rectangle, "R1", 0.0, 0.0, 10.0, 10.0).unwrap();
+    let snap = doc.snapshot_subtree(f).unwrap();
+
+    let mut missing = snap.clone();
+    let mut bad_root = (*missing.nodes[0]).clone();
+    bad_root.children.push(NodeId(99999));
+    missing.nodes[0] = Arc::new(bad_root);
+    doc.dev_snapshots.insert(f, Arc::new(missing));
+    assert!(doc.with_dev_snapshot(f).is_none());
+
+    let mut nonrecip = snap.clone();
+    let mut bad_child = (*nonrecip.nodes[1]).clone();
+    bad_child.parent = Some(NodeId(88888));
+    nonrecip.nodes[1] = Arc::new(bad_child);
+    doc.dev_snapshots.insert(f, Arc::new(nonrecip));
+    assert!(doc.with_dev_snapshot(f).is_none());
+}
+
+#[test]
+fn validate_is_linear_in_a_wide_parent() {
+    let mut doc = Document::new("d");
+    let page = doc.first_page().unwrap();
+    for i in 0..5_000 {
+        doc.create(page, NodeKind::Rectangle, "R", f64::from(i), 0.0, 1.0, 1.0).unwrap();
+    }
+    assert!(doc.validate().is_empty());
+    let kid = doc.node(page).unwrap().children[0];
+    doc.get_mut(page).unwrap().children.retain(|c| *c != kid);
+    assert!(doc.validate().iter().any(|m| m.contains("not listed in parent")));
+}

crates/doc/src/lib.rs

@@ -1023,6 +1023,10 @@ impl Document {
             }
         }
         let mut issues = Vec::new();
+        // Every (parent, child) edge and the page list, indexed once so each node's checks are
+        // lookups: a wide parent or a long page list doesn't make validation quadratic.
+        let edges: std::collections::BTreeSet<(NodeId, NodeId)> = self.nodes.iter().flat_map(|(p, n)| n.children.iter().map(|c| (*p, *c))).collect();
+        let page_ids: std::collections::BTreeSet<NodeId> = self.pages.iter().copied().collect();
         for (id, n) in self.nodes.iter() {
             if n.id != *id {
                 note(&mut issues, || format!("node key {id} has id {}", n.id));
@@ -1039,12 +1043,12 @@ impl Document {
                 }
             }
             if let Some(p) = n.parent
-                && !self.node(p).is_some_and(|pn| pn.children.contains(id))
+                && !(self.node(p).is_some() && edges.contains(&(p, *id)))
             {
                 note(&mut issues, || format!("{id} not listed in parent {p}"));
             }
             // The library store's root is a page that is never listed.
-            let listed = self.pages.contains(id) || (n.is_page() && self.libraries.store == Some(*id));
+            let listed = page_ids.contains(id) || (n.is_page() && self.libraries.store == Some(*id));
             if n.is_page() != listed {
                 note(&mut issues, || format!("page list mismatch for {id}"));
             }

crates/doc/src/tests.rs

@@ -559,3 +559,60 @@ fn is_ancestor_holds_for_very_deep_chains() {
     assert!(d.is_ancestor(root, leaf));
     assert!(matches!(d.reparent(root, leaf, None), Err(DocError::Cycle(..))));
 }
+
+#[test]
+fn many_small_vector_subpaths_build_and_stroke_without_rescans() {
+    // Many closed subpaths whose last point repeats the start (#F-332), a long open chain (#F-334)
+    // and a big rounded polygon (#F-333): each used to scan the whole network per step.
+    let mut p = figcraft_geom::kurbo::BezPath::new();
+    for i in 0..3_000 {
+        let x = f64::from(i) * 10.0;
+        p.move_to((x, 0.0));
+        p.line_to((x + 5.0, 0.0));
+        p.line_to((x + 5.0, 5.0));
+        p.line_to((x, 0.0));
+        p.close_path();
+    }
+    let net = VectorNetwork::from_path(&p);
+    assert_eq!(net.vertices.len(), 3 * 3_000);
+    assert_eq!(net.regions[0].loops.len(), 3_000);
+    assert!(net.segments.iter().all(|s| s.a < net.vertices.len() && s.b < net.vertices.len()));
+    assert_eq!(net.stroke_runs().len(), 3_000);
+
+    let pts: Vec<Point> = (0..20_000).map(|i| Point::new(f64::from(i), f64::from(i % 7))).collect();
+    let chain = VectorNetwork::from_points(&pts, false);
+    let runs = chain.stroke_runs();
+    assert_eq!(runs.len(), 1);
+    assert!(!runs[0].closed);
+
+    let ring: Vec<Point> = (0..4_000)
+        .map(|i| {
+            let t = f64::from(i) / 4_000.0 * std::f64::consts::TAU;
+            Point::new(1000.0 * t.cos(), 1000.0 * t.sin() * if i % 2 == 0 { 1.0 } else { 0.9 })
+        })
+        .collect();
+    let mut poly = VectorNetwork::from_points(&ring, true);
+    for v in &mut poly.vertices {
+        v.radius = 0.5;
+    }
+    let rounded = poly.rounded().unwrap();
+    assert!(rounded.regions[0].loops[0].len() > poly.segments.len(), "arcs are walked between the sides");
+    assert_eq!(rounded.regions[0].loops[0].len(), rounded.segments.len());
+}
+
+#[test]
+fn two_segment_closed_loop_rounded_corners() {
+    let mut v0 = Vertex::new(0.0, 0.0);
+    v0.radius = 2.0;
+    let mut v1 = Vertex::new(10.0, 0.0);
+    v1.radius = 2.0;
+    let net = VectorNetwork {
+        vertices: vec![v0, v1],
+        segments: vec![Segment { a: 0, b: 1, ta: (0.0, 0.0), tb: (0.0, 0.0) }, Segment { a: 1, b: 0, ta: (0.0, 5.0), tb: (0.0, -5.0) }],
+        regions: vec![Region { loops: vec![vec![0, 1]], winding: WindingRule::NonZero }],
+    };
+    let r = net.rounded().unwrap();
+    assert_eq!(r.regions[0].loops[0].len(), 4, "both arcs are in the loop");
+    let path = net.fill_path();
+    assert!(!path.elements().is_empty());
+}

crates/doc/src/vector.rs

@@ -134,6 +134,16 @@ pub struct VectorNetwork {
     pub regions: Vec<Region>,
 }
 
+/// Point the subpath's last segment (the only one ending at the fresh vertex `from`) at `to`,
+/// so merging a duplicate endpoint doesn't rescan the segments of earlier subpaths.
+fn redirect_last_segment(net: &mut VectorNetwork, loop_segs: &[usize], from: usize, to: usize) {
+    if let Some(s) = loop_segs.last().and_then(|i| net.segments.get_mut(*i))
+        && s.b == from
+    {
+        s.b = to;
+    }
+}
+
 impl VectorNetwork {
     /// A network from a polyline / polygon.
     pub fn from_points(points: &[Point], closed: bool) -> Self {
@@ -169,11 +179,7 @@ impl VectorNetwork {
                 if a == f || loop_segs.len() < 2 || (pa - pf).hypot() > 1e-6 || a + 1 != net.vertices.len() {
                     return;
                 }
-                for s in net.segments.iter_mut() {
-                    if s.b == a {
-                        s.b = f;
-                    }
-                }
+                redirect_last_segment(net, loop_segs, a, f);
                 net.vertices.pop();
                 all_loops.push(std::mem::take(loop_segs));
             };
@@ -219,11 +225,7 @@ impl VectorNetwork {
                     let same = matches!((close_pt, first_pt), (Some(c), Some(f)) if (c - f).hypot() <= 1e-6);
                     if same && a != f {
                         // Merge the duplicate end vertex into the first one.
-                        for s in net.segments.iter_mut() {
-                            if s.b == a {
-                                s.b = f;
-                            }
-                        }
+                        redirect_last_segment(&mut net, &loop_segs, a, f);
                         if a + 1 == net.vertices.len() {
                             net.vertices.pop();
                         }
@@ -385,6 +387,7 @@ impl VectorNetwork {
             s.ta = (q.p1.x - q.p0.x, q.p1.y - q.p0.y);
             s.tb = (q.p2.x - q.p3.x, q.p2.y - q.p3.y);
         }
+        let mut by_vertex: std::collections::BTreeMap<usize, usize> = std::collections::BTreeMap::new();
         for c in corners {
             if let Some(v) = out.vertices.get_mut(c.v) {
                 (v.x, v.y) = (c.a.x, c.a.y);
@@ -400,15 +403,28 @@ impl VectorNetwork {
             }
             out.segments.push(Segment { a: c.v, b: nb, ta: c.ta, tb: c.tb });
             let arc = out.segments.len() - 1;
-            // The arc goes between the two segments wherever a loop walks through the point.
-            for lp in out.regions.iter_mut().flat_map(|r| r.loops.iter_mut()) {
-                let n = lp.len();
-                let between =
-                    |k: usize| matches!((lp.get(k), lp.get((k + 1) % n)), (Some(&x), Some(&y)) if (x, y) == (c.s1, c.s2) || (x, y) == (c.s2, c.s1));
-                if let Some(k) = (0..n).find(|k| between(*k)) {
-                    lp.insert(k + 1, arc);
+            by_vertex.insert(c.v, arc);
+        }
+        // The arc goes between the two segments wherever a loop walks through the point. Each loop
+        // is rebuilt in one pass, so a polygon with many rounded corners isn't quadratic.
+        for lp in out.regions.iter_mut().flat_map(|r| r.loops.iter_mut()) {
+            let Some(&first_idx) = lp.first() else { continue };
+            let start_v = match (self.segments.get(first_idx), lp.get(1).and_then(|&i| self.segments.get(i))) {
+                (Some(first), Some(second)) if first.a == second.a || first.a == second.b => first.b,
+                (Some(first), _) => first.a,
+                _ => 0,
+            };
+            let mut rebuilt = Vec::with_capacity(lp.len() + by_vertex.len().min(lp.len()));
+            let mut cur = start_v;
+            for &x in lp.iter() {
+                rebuilt.push(x);
+                let Some(s) = self.segments.get(x) else { continue };
+                cur = if s.a == cur { s.b } else { s.a };
+                if let Some(&arc) = by_vertex.get(&cur) {
+                    rebuilt.push(arc);
                 }
             }
+            *lp = rebuilt;
         }
         Some(out)
     }
@@ -547,25 +563,40 @@ impl VectorNetwork {
             }
         }
         let degree = |v: usize| degrees.get(v).copied().unwrap_or(0);
-        let next_from = |v: usize, used: &[bool]| -> Option<(usize, bool)> {
-            self.segments.iter().enumerate().find_map(|(i, s)| {
-                if used.get(i).copied().unwrap_or(true) {
-                    return None;
+        // Each vertex's segments in segment order, and how far down that list the used ones reach,
+        // so finding the next unused segment never rescans the whole network.
+        let mut adjacent: Vec<Vec<(usize, bool)>> = vec![Vec::new(); self.vertices.len()];
+        for (i, s) in self.segments.iter().enumerate() {
+            if let Some(l) = adjacent.get_mut(s.a) {
+                l.push((i, false));
+            }
+            if s.b != s.a
+                && let Some(l) = adjacent.get_mut(s.b)
+            {
+                l.push((i, true));
+            }
+        }
+        let mut cursor = vec![0usize; self.vertices.len()];
+        let mut remaining = self.segments.len();
+        let mut next_from = |v: usize, used: &[bool]| -> Option<(usize, bool)> {
+            let list = adjacent.get(v)?;
+            let at = cursor.get_mut(v)?;
+            while let Some(&(i, rev)) = list.get(*at) {
+                if !used.get(i).copied().unwrap_or(true) {
+                    return Some((i, rev));
                 }
-                if s.a == v {
-                    Some((i, false))
-                } else if s.b == v {
-                    Some((i, true))
-                } else {
-                    None
-                }
-            })
+                *at += 1;
+            }
+            None
         };
         let stop = |v: usize| degree(v) != 2 || breaks(v);
         // Start runs at endpoints, junctions and breaks first, then whatever remains (pure loops).
         let mut starts: Vec<usize> = (0..self.vertices.len()).filter(|v| stop(*v)).collect();
         starts.extend((0..self.vertices.len()).filter(|v| !stop(*v)));
         for v0 in starts {
+            if remaining == 0 {
+                break;
+            }
             while let Some((si, rev)) = next_from(v0, &used) {
                 let Some(start) = self.vertices.get(v0) else { break };
                 let mut path = BezPath::new();
@@ -574,8 +605,11 @@ impl VectorNetwork {
                 let mut closed = false;
                 let mut seg = Some((si, rev));
                 while let Some((i, r)) = seg {
-                    if let Some(u) = used.get_mut(i) {
+                    if let Some(u) = used.get_mut(i)
+                        && !*u
+                    {
                         *u = true;
+                        remaining = remaining.saturating_sub(1);
                     }
                     let Some(s) = self.segments.get(i) else { break };
                     self.push_seg(&mut path, s, r);