Merge pull request #847 from Silvertree2010/fix/h1767-shape-connect-and-shape-sites-po

Reject shape ids beyond u32 in shape.connect, sites and points

crates/engine/src/cmd/points.rs

@@ -355,8 +355,8 @@ fn refit(sh: &mut Shape) {
 /// The one shape whose points are edited: `id`, or the single selected shape.
 fn target(s: &Session, p: &Value, cmd: &str) -> Result<ShapeId> {
     let st = s.doc()?;
-    let id = match p.get("id").and_then(Value::as_u64) {
-        Some(i) => ShapeId(i as u32),
+    let id = match id_param(p, "id", cmd)? {
+        Some(i) => i,
         None => match st.selection.shapes.as_slice() {
             [one] => *one,
             _ => return Err(bad(cmd, "select one shape")),
@@ -429,8 +429,8 @@ pub fn slide_vertices(doc: &deckcraft_model::Presentation, sel: &crate::Selectio
 
 fn points(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
-    let id = match p.get("id").and_then(Value::as_u64) {
-        Some(i) => ShapeId(i as u32),
+    let id = match id_param(p, "id", "shape.points")? {
+        Some(i) => i,
         None => *st.selection.shapes.first().ok_or_else(|| bad("shape.points", "select a shape"))?,
     };
     let sh = st.shape(id).ok_or_else(|| bad("shape.points", "no such shape"))?;

crates/engine/src/cmd/shape.rs

@@ -1150,19 +1150,17 @@ fn media_options(s: &mut Session, p: &Value) -> Result<Value> {
     Ok(json!({}))
 }
 
-fn shape_id(p: &Value, key: &str) -> Option<deckcraft_model::ShapeId> {
-    p.get(key).and_then(Value::as_u64).map(|v| deckcraft_model::ShapeId(v as u32))
-}
-
 fn sites(s: &mut Session, p: &Value) -> Result<Value> {
-    let id = shape_id(p, "id").ok_or_else(|| bad("shape.sites", "missing `id`"))?;
+    let id = id_param(p, "id", "shape.sites")?.ok_or_else(|| bad("shape.sites", "missing `id`"))?;
     let st = s.doc()?;
     let sh = st.shape(id).ok_or_else(|| bad("shape.sites", "no such shape"))?;
     Ok(json!(crate::connect::sites(&st.doc, &st.selection, sh).iter().map(|q| [q.x, q.y]).collect::<Vec<_>>()))
 }
 
 fn connect(s: &mut Session, p: &Value) -> Result<Value> {
-    let (Some(from), Some(to)) = (shape_id(p, "from"), shape_id(p, "to")) else { return Err(bad("shape.connect", "missing `from` / `to`")) };
+    let (Some(from), Some(to)) = (id_param(p, "from", "shape.connect")?, id_param(p, "to", "shape.connect")?) else {
+        return Err(bad("shape.connect", "missing `from` / `to`"));
+    };
     let st = s.doc()?;
     let site_list = |id| st.shape(id).map(|sh| crate::connect::sites(&st.doc, &st.selection, sh)).unwrap_or_default();
     let (a, b) = (site_list(from), site_list(to));
@@ -1186,7 +1184,7 @@ fn connect(s: &mut Session, p: &Value) -> Result<Value> {
         }
     };
     let (p0, p1) = (a[ia], b[ib]);
-    let id = match shape_id(p, "id") {
+    let id = match id_param(p, "id", "shape.connect")? {
         Some(id) => id,
         None => {
             let preset = str_param(p, "preset").unwrap_or("straightConnector1");

crates/engine/src/tests.rs

@@ -3557,3 +3557,22 @@ fn an_exhausted_id_space_refuses_growth() {
     sorted.dedup();
     assert_eq!(sorted.len(), ids.len(), "no id repeats");
 }
+
+#[test]
+fn shape_ids_beyond_u32_are_rejected_not_wrapped() {
+    let (mut s, a, b) = two_shapes();
+    let wrap = |id: deckcraft_model::ShapeId| u64::from(id.0) + (1u64 << 32);
+    let shapes_before = s.doc().unwrap().shapes().len();
+    let r = s.execute("shape.connect", &json!({"from": wrap(a), "to": wrap(b)}));
+    assert!(r.is_err(), "connect must reject ids beyond u32, got {r:?}");
+    let r = s.execute("shape.sites", &json!({"id": wrap(a)}));
+    assert!(r.is_err(), "sites must reject ids beyond u32, got {r:?}");
+    let r = s.execute("shape.points", &json!({"id": wrap(a)}));
+    assert!(r.is_err(), "points must reject ids beyond u32, got {r:?}");
+    let r = s.execute("shape.movePoint", &json!({"id": wrap(a), "index": 0, "x": 1.0, "y": 1.0}));
+    assert!(r.is_err_and(|e| e.to_string().contains("not a valid shape id")), "movePoint must reject ids beyond u32");
+    assert_eq!(s.doc().unwrap().shapes().len(), shapes_before, "no connector was created");
+    // In-range ids still work.
+    assert!(s.execute("shape.sites", &json!({"id": a.0})).is_ok());
+    assert!(s.execute("shape.connect", &json!({"from": a.0, "to": b.0})).is_ok());
+}