From 622988d7f71ed8765d4474a20aa853e096ec40f6 Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Wed, 21 Oct 2026 01:39:29 +0000 Subject: [PATCH] Masks and spots: an id or index that isn't one is an error, not the selected one mask_id read `id` with `as_u64().map(|v| v as u32)`, so text fell back to the selected mask (mask.delete {"id": "1"} deleted it) and an id past u32 wrapped onto another mask (4294967297 as u32 is 1). spot_index and spot.select did the same with `index`, and a colour range's `component` fell back to the first colour range part. They now refuse such values, as the photo ids do; a missing or null value still means the selection. --- crates/engine/src/cmd/masks.rs | 33 ++++++++++++++++++----- crates/engine/src/tests_ids_params.rs | 39 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 0efb3b658..4f7191ceb 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -199,8 +199,27 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec
Result> { + match p.get("id") { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().and_then(|n| u32::try_from(n).ok()).map(Some).ok_or_else(|| bad(c, "`id` must be a mask id")), + } +} + fn mask_id(p: &Value, active: Option, c: &str) -> Result { - p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)")) + opt_mask_id(p, c)?.or(active).ok_or_else(|| bad(c, "no mask selected (give `id`)")) +} + +/// A whole-number index parameter `key`: `None` when it is missing or null, else an error unless it +/// is one (an index that isn't must not fall back to the selected item). +fn opt_index(p: &Value, key: &str, c: &str) -> Result> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).map(Some).ok_or_else(|| bad(c, format!("`{key}` must be a whole number"))), + } } fn find(masks: &mut [Mask], id: u32, c: &str) -> Result { @@ -307,7 +326,7 @@ pub fn specs() -> Vec { let connect = bool_or(p, "connect", false); let mut connected = false; let next = s.active().and_then(|id| s.develop_of(id)).map(|d| d.next_mask_id()).unwrap_or(1); - let want = p.get("id").and_then(Value::as_u64).map(|v| v as u32).or(s.active_mask); + let want = opt_mask_id(p, "mask.brushStroke")?.or(s.active_mask); let adjust = super::mask_effects::new_mask_adjust(s, "brush"); let mut out = masks_edit(s, "mask.brushStroke", "Brush", |masks, active| { let i = match want.and_then(|m| masks.iter().position(|x| x.id == m)) { @@ -391,7 +410,7 @@ pub fn specs() -> Vec { let lab = lightcraft_pipeline::color_range_sample(&src, &info, &d, &req, lightcraft_geom::Point::new(x, y)) .ok_or_else(|| bad(c, "the point is outside the photo"))?; let mid = mask_id(p, s.active_mask, c)?; - let comp = p.get("component").and_then(Value::as_u64).map(|v| v as usize); + let comp = opt_index(p, "component", c)?; let mut out = Vec::new(); masks_edit(s, c, "Color Range", |masks, _| { let i = find(masks, mid, c)?; @@ -604,7 +623,7 @@ pub fn specs() -> Vec { } ), cmd!("mask.select", "Select Mask", [], None, "{id|null}", has_active, |s, p| { - s.active_mask = p.get("id").and_then(Value::as_u64).map(|v| v as u32); + s.active_mask = opt_mask_id(p, "mask.select")?; ok() }), cmd!("mask.delete", "Delete Mask", [], None, "{id?}", has_active, |s, p| { @@ -879,8 +898,8 @@ pub fn specs() -> Vec { ), cmd!("spot.select", "Select Spot", [], None, "{index|null}", has_active, |s, p| { let n = spots_len(s); - s.active_spot = match p.get("index").and_then(Value::as_u64) { - Some(i) if (i as usize) < n => Some(i as usize), + s.active_spot = match opt_index(p, "index", "spot.select")? { + Some(i) if i < n => Some(i), Some(_) => return Err(bad("spot.select", "no such spot")), None => None, }; @@ -1040,7 +1059,7 @@ fn spots_len(s: &Session) -> usize { /// `index` from the params, else the selected spot. fn spot_index(s: &Session, p: &Value, c: &str) -> Result { - let i = p.get("index").and_then(Value::as_u64).map(|v| v as usize).or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?; + let i = opt_index(p, "index", c)?.or(s.active_spot).ok_or_else(|| bad(c, "no spot selected (give `index`)"))?; if i >= spots_len(s) { return Err(bad(c, "no such spot")); } diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs index 1eae90473..824dbedc2 100644 --- a/crates/engine/src/tests_ids_params.rs +++ b/crates/engine/src/tests_ids_params.rs @@ -104,3 +104,42 @@ fn single_photo_commands_refuse_an_id_that_is_not_a_photo_id() { assert_eq!(source(&s), before); assert_eq!(s.catalog.photo(active).unwrap().meta.regions.len(), 1); } + +/// Mask ids and spot indexes the same way: `mask.delete {"id": "1"}` deleted the selected mask, and an +/// id past `u32` wrapped (`4294967297 as u32` is 1) onto another mask. +#[test] +fn mask_and_spot_commands_refuse_ids_that_are_not_ids() { + let (mut s, _, _) = session(); + s.execute("mask.add", &json!({"kind": "linear"})).unwrap(); + s.execute("mask.add", &json!({"kind": "radial"})).unwrap(); + s.execute("spot.add", &json!({"mode": "heal", "points": [[0.4, 0.5]], "size": 0.02, "source": [0.1, 0.0]})).unwrap(); + let state = |s: &Session| { + let d = s.develop_of(s.active().unwrap()).unwrap(); + (d.masks.iter().map(|m| (m.id, m.name.clone(), m.visible)).collect::>(), d.spots.len()) + }; + let before = state(&s); + assert_eq!((before.0.len(), before.1), (2, 1)); + let first = before.0[0].0; + let commits = s.commits(); + for bad in [json!(u64::from(u32::MAX) + 1 + u64::from(first)), json!("1"), json!(-1), json!(1.5), json!([first])] { + for (cmd, params) in [ + ("mask.delete", json!({})), + ("mask.rename", json!({"name": "renamed"})), + ("mask.visible", json!({"visible": false})), + ("mask.select", json!({})), + ] { + let mut params = params; + params["id"] = bad.clone(); + assert!(s.execute(cmd, ¶ms).is_err(), "{cmd} {bad}"); + } + for cmd in ["spot.delete", "spot.select"] { + assert!(s.execute(cmd, &json!({"index": bad})).is_err(), "{cmd} {bad}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(state(&s), before); + // well-formed ids and a null still work + s.execute("mask.delete", &json!({"id": first})).unwrap(); + s.execute("spot.select", &json!({"index": null})).unwrap(); + assert_eq!(state(&s).0.len(), 1); +}