From 004d015cfe3827ca9dd91ebeec5574a381d891e2 Mon Sep 17 00:00:00 2001 From: Nigel Ihza Firdausy <106657905+Ni-zav@users.noreply.github.com> Date: Sat, 10 Oct 2026 23:39:32 +0700 Subject: [PATCH 1/2] fix(place): reject malformed rectangle and position parameters --- crates/engine/src/cmd/place/mod.rs | 11 ++++++--- crates/engine/src/tests_place.rs | 37 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/cmd/place/mod.rs b/crates/engine/src/cmd/place/mod.rs index 9fee1ce5..33e8dc47 100644 --- a/crates/engine/src/cmd/place/mod.rs +++ b/crates/engine/src/cmd/place/mod.rs @@ -309,9 +309,11 @@ fn transformed(mut node: Node, m: Affine) -> Node { /// `[x, y, width, height]` with a positive size. fn rect_param(p: &Value) -> Result> { let Some(v) = p.get("rect") else { return Ok(None) }; - let n: Vec = v.as_array().map(|a| a.iter().filter_map(Value::as_f64).collect()).unwrap_or_default(); - match n[..] { - [x, y, w, h] if w > 0.0 && h > 0.0 => Ok(Some(Rect::new(x, y, x + w, y + h))), + // Reject invalid components instead of dropping them: filtering would turn a malformed + // five-element box such as [0, "bad", 10, 40, 50] into a different valid rectangle. + let numbers = v.as_array().filter(|a| a.len() == 4).and_then(|a| a.iter().map(Value::as_f64).collect::>>()); + match numbers.as_deref() { + Some([x, y, w, h]) if *w > 0.0 && *h > 0.0 => Ok(Some(Rect::new(*x, *y, x + w, y + h))), _ => Err(bad(PLACE, "rect must be [x, y, width, height] with a positive width and height")), } } @@ -343,6 +345,9 @@ fn place(s: &mut Session, p: &Value) -> Result { let replace = bool_or(p, "replace", false); let template = bool_or(p, "template", false); let at = point_param(p, "at"); + if p.get("at").is_some() && (at.is_none() || p["at"].as_array().is_none_or(|a| a.len() != 2)) { + return Err(bad(PLACE, "at must be [x, y] with two numeric coordinates")); + } let rect = rect_param(p)?; if replace && (template || at.is_some() || rect.is_some()) { return Err(bad(PLACE, "replace keeps the replaced object's place and transform: drop template, at and rect")); diff --git a/crates/engine/src/tests_place.rs b/crates/engine/src/tests_place.rs index aba974e5..daab2d57 100644 --- a/crates/engine/src/tests_place.rs +++ b/crates/engine/src/tests_place.rs @@ -57,6 +57,43 @@ fn serialize(s: &mut Session, format: &str) -> Vec { vectorcraft_format::base64_decode(v["dataBase64"].as_str().unwrap()).unwrap() } + +#[test] +fn malformed_place_coordinates_are_rejected_instead_of_silently_changed() { + let mut s = session(); + let png = png(4, 2, [20, 40, 60, 255], None); + let original = s.doc().unwrap().doc.layers.clone(); + // Previously filter_map dropped the string from a five-element rectangle and + // treated the remaining four values as a different, valid placement. + for rect in [ + json!([0, "ignored", 10, 40, 50]), + json!([10, 20, 40, null, 50]), + json!([10, 20, 40, 50, 60]), + ] { + let mut params = file("tiny.png", &png); + params["rect"] = rect; + assert!(s.execute("file.place", ¶ms).is_err(), "{params}"); + assert_eq!(s.doc().unwrap().doc.layers, original, "failed Place must not add art"); + } + // An explicitly supplied invalid position formerly behaved as though no + // position were supplied and centred the image on the artboard. + for at in [ + json!(["not-a-number", 20]), + json!([10, null]), + json!([10, 20, 30]), + json!("invalid"), + ] { + let mut params = file("tiny.png", &png); + params["at"] = at; + assert!(s.execute("file.place", ¶ms).is_err(), "{params}"); + assert_eq!(s.doc().unwrap().doc.layers, original); + } + let mut params = file("tiny.png", &png); + params["rect"] = json!([10, 20, 80, 60]); + let placed = s.execute("file.place", ¶ms).unwrap(); + assert_eq!(placed["ids"].as_array().unwrap().len(), 1); +} + #[test] fn a_300_px_png_at_300_ppi_places_as_72_pt_centred_selected_and_undoable() { let mut s = session(); From ac6d72b44a23ee6551478d955373f7775876e8b8 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Sun, 11 Oct 2026 01:44:11 +0700 Subject: [PATCH 2/2] Review #985: one parser for exact number lists; null `at` is no position - `finite_numbers::` (cmd/mod.rs) reads an array of exactly N finite numbers, nothing dropped or padded; `rect` uses it, and the rect cut, Free Distort and anchor handles reuse it in the PRs stacked on this one. - `at` is parsed once (`at_param`) instead of point_param plus a second shape check that indexed the params; `at: null` means no position, as null does for the other optional params, and gets a test. - rustfmt. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/mod.rs | 11 +++++++++++ crates/engine/src/cmd/place/mod.rs | 22 +++++++++++++--------- crates/engine/src/tests_place.rs | 18 ++++++------------ 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 228410de..a3d142eb 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -324,6 +324,17 @@ pub(crate) fn checked_id(s: &Session, value: &Value, command: &str) -> Result(v: &Value) -> Option<[f64; N]> { + let a = v.as_array().filter(|a| a.len() == N)?; + let mut out = [0.0; N]; + for (o, n) in out.iter_mut().zip(a) { + *o = n.as_f64().filter(|n| n.is_finite())?; + } + Some(out) +} + pub(crate) fn point_param(p: &Value, key: &str) -> Option { let a = p.get(key)?.as_array()?; Some(Point::new(a.first()?.as_f64()?, a.get(1)?.as_f64()?)) diff --git a/crates/engine/src/cmd/place/mod.rs b/crates/engine/src/cmd/place/mod.rs index 33e8dc47..b881d52b 100644 --- a/crates/engine/src/cmd/place/mod.rs +++ b/crates/engine/src/cmd/place/mod.rs @@ -309,15 +309,22 @@ fn transformed(mut node: Node, m: Affine) -> Node { /// `[x, y, width, height]` with a positive size. fn rect_param(p: &Value) -> Result> { let Some(v) = p.get("rect") else { return Ok(None) }; - // Reject invalid components instead of dropping them: filtering would turn a malformed - // five-element box such as [0, "bad", 10, 40, 50] into a different valid rectangle. - let numbers = v.as_array().filter(|a| a.len() == 4).and_then(|a| a.iter().map(Value::as_f64).collect::>>()); - match numbers.as_deref() { - Some([x, y, w, h]) if *w > 0.0 && *h > 0.0 => Ok(Some(Rect::new(*x, *y, x + w, y + h))), + // All four or nothing: a malformed entry dropped would shift the others ([0, "bad", 10, 40, 50]). + match finite_numbers(v) { + Some([x, y, w, h]) if w > 0.0 && h > 0.0 => Ok(Some(Rect::new(x, y, x + w, y + h))), _ => Err(bad(PLACE, "rect must be [x, y, width, height] with a positive width and height")), } } +/// `at`, the `[x, y]` the art is centred on (absent or null: none); anything else fails rather than +/// placing the art as if no point were given. +fn at_param(p: &Value) -> Result> { + p.get("at") + .filter(|v| !v.is_null()) + .map(|v| finite_numbers(v).map(|[x, y]| Point::new(x, y)).ok_or_else(|| bad(PLACE, "at must be [x, y]"))) + .transpose() +} + /// The transform that puts new art with the natural box `natural` where `old` is: an image's scale /// and rotation relative to its 100% size (its resolution's) and its centre are kept (with /// `image_xf`); other objects keep their centre. @@ -344,10 +351,7 @@ fn fit(natural: Rect, r: Rect) -> Affine { fn place(s: &mut Session, p: &Value) -> Result { let replace = bool_or(p, "replace", false); let template = bool_or(p, "template", false); - let at = point_param(p, "at"); - if p.get("at").is_some() && (at.is_none() || p["at"].as_array().is_none_or(|a| a.len() != 2)) { - return Err(bad(PLACE, "at must be [x, y] with two numeric coordinates")); - } + let at = at_param(p)?; let rect = rect_param(p)?; if replace && (template || at.is_some() || rect.is_some()) { return Err(bad(PLACE, "replace keeps the replaced object's place and transform: drop template, at and rect")); diff --git a/crates/engine/src/tests_place.rs b/crates/engine/src/tests_place.rs index daab2d57..0fb2c5d7 100644 --- a/crates/engine/src/tests_place.rs +++ b/crates/engine/src/tests_place.rs @@ -57,7 +57,6 @@ fn serialize(s: &mut Session, format: &str) -> Vec { vectorcraft_format::base64_decode(v["dataBase64"].as_str().unwrap()).unwrap() } - #[test] fn malformed_place_coordinates_are_rejected_instead_of_silently_changed() { let mut s = session(); @@ -65,11 +64,7 @@ fn malformed_place_coordinates_are_rejected_instead_of_silently_changed() { let original = s.doc().unwrap().doc.layers.clone(); // Previously filter_map dropped the string from a five-element rectangle and // treated the remaining four values as a different, valid placement. - for rect in [ - json!([0, "ignored", 10, 40, 50]), - json!([10, 20, 40, null, 50]), - json!([10, 20, 40, 50, 60]), - ] { + for rect in [json!([0, "ignored", 10, 40, 50]), json!([10, 20, 40, null, 50]), json!([10, 20, 40, 50, 60])] { let mut params = file("tiny.png", &png); params["rect"] = rect; assert!(s.execute("file.place", ¶ms).is_err(), "{params}"); @@ -77,12 +72,7 @@ fn malformed_place_coordinates_are_rejected_instead_of_silently_changed() { } // An explicitly supplied invalid position formerly behaved as though no // position were supplied and centred the image on the artboard. - for at in [ - json!(["not-a-number", 20]), - json!([10, null]), - json!([10, 20, 30]), - json!("invalid"), - ] { + for at in [json!(["not-a-number", 20]), json!([10, null]), json!([10, 20, 30]), json!("invalid")] { let mut params = file("tiny.png", &png); params["at"] = at; assert!(s.execute("file.place", ¶ms).is_err(), "{params}"); @@ -92,6 +82,10 @@ fn malformed_place_coordinates_are_rejected_instead_of_silently_changed() { params["rect"] = json!([10, 20, 80, 60]); let placed = s.execute("file.place", ¶ms).unwrap(); assert_eq!(placed["ids"].as_array().unwrap().len(), 1); + // A null position is no position, as for the other optional params. + let mut params = file("tiny.png", &png); + params["at"] = Value::Null; + assert_eq!(s.execute("file.place", ¶ms).unwrap()["ids"].as_array().unwrap().len(), 1); } #[test]