From cbf809a37eb83ddc4646ed8e90824fe348aa9acf Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Wed, 21 Oct 2026 12:03:42 +0000 Subject: [PATCH] Masks: mask-effect ids and component indexes that aren't whole numbers are errors mask.applyEffect, saveEffect and updateEffect read an `id` that isn't a mask id as the selected mask (applyEffect {"id": "1"} changed mask 2); mask.update, mask.component and mask.refineDetail cast `component` with as-casts and fell back to component 0 or the last selection. They share the strict opt_mask_id / opt_index helpers now. --- crates/engine/src/cmd/mask_effects.rs | 8 ++---- crates/engine/src/cmd/masks.rs | 10 +++---- crates/engine/src/tests_ids_params.rs | 40 +++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/mask_effects.rs b/crates/engine/src/cmd/mask_effects.rs index 270abeed5..549434e02 100644 --- a/crates/engine/src/cmd/mask_effects.rs +++ b/crates/engine/src/cmd/mask_effects.rs @@ -287,12 +287,8 @@ fn effect_index(s: &Session, name: &str, c: &str) -> Result { /// The selected mask's adjustments on the active photo. fn selected_mask(s: &Session, p: &Value, c: &str) -> Result { let id = s.active().ok_or_else(|| bad(c, "no active photo"))?; - let mid = p - .get("id") - .and_then(Value::as_u64) - .and_then(|v| u32::try_from(v).ok()) - .or(s.active_mask) - .ok_or_else(|| bad(c, "no mask selected (give `id`)"))?; + // an `id` that isn't a mask id is an error, not the selected mask + let mid = super::masks::opt_mask_id(p, c)?.or(s.active_mask).ok_or_else(|| bad(c, "no mask selected (give `id`)"))?; let d = s.develop_of(id).unwrap_or_default(); d.masks.iter().find(|m| m.id == mid).cloned().ok_or_else(|| bad(c, format!("no mask {mid}"))) } diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index 4f7191ceb..e39f8aa3d 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -202,7 +202,7 @@ fn connected_strokes(from: Point, last: &BrushStroke, to: BrushStroke) -> Vec
Result> { +pub(crate) fn opt_mask_id(p: &Value, c: &str) -> 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")), @@ -382,7 +382,7 @@ pub fn specs() -> Vec { cmd!("mask.update", "Update Mask Shape", [], None, "{id?, component?: index (0), shape: MaskShape JSON}", has_active, |s, p| { let shape: MaskShape = serde_json::from_value(p.get("shape").cloned().unwrap_or_default()).map_err(|e| bad("mask.update", e.to_string()))?; - let comp = p.get("component").and_then(Value::as_u64).unwrap_or(0) as usize; + let comp = opt_index(p, "component", "mask.update")?.unwrap_or(0); let mid = mask_id(p, s.active_mask, "mask.update")?; masks_edit(s, "mask.update", "Edit Mask", |masks, _| { let i = find(masks, mid, "mask.update")?; @@ -501,8 +501,8 @@ pub fn specs() -> Vec { let mid = mask_id(p, s.active_mask, c)?; let d = s.develop_of(id).unwrap_or_default(); let m = d.masks.iter().find(|m| m.id == mid).ok_or_else(|| bad(c, format!("no mask {mid}")))?; - let k = match p.get("component").and_then(Value::as_u64) { - Some(k) => k as usize, + let k = match opt_index(p, "component", c)? { + Some(k) => k, None => m .components .iter() @@ -703,7 +703,7 @@ pub fn specs() -> Vec { |s, p| { let c = "mask.component"; let mid = mask_id(p, s.active_mask, c)?; - let k = p.get("component").and_then(Value::as_u64).ok_or_else(|| bad(c, "missing component"))? as usize; + let k = opt_index(p, "component", c)?.ok_or_else(|| bad(c, "missing component"))?; let action = str_param(p, "action").ok_or_else(|| bad(c, "missing action"))?.to_string(); let op: Option = match p.get("op") { Some(v) => Some(serde_json::from_value(v.clone()).map_err(|e| bad(c, e.to_string()))?), diff --git a/crates/engine/src/tests_ids_params.rs b/crates/engine/src/tests_ids_params.rs index daff1f7ae..8ce772ded 100644 --- a/crates/engine/src/tests_ids_params.rs +++ b/crates/engine/src/tests_ids_params.rs @@ -163,3 +163,43 @@ fn photo_label_refuses_a_missing_or_non_text_label() { s.execute("photo.label", &json!({"label": "none"})).unwrap(); assert!(labels(&s).iter().all(Option::is_none)); } + +/// The mask-effect commands and a mask's `component` the same way: `mask.saveEffect {"id": "2"}` +/// saved the selected mask's sliders, and `mask.update {"component": "1"}` reshaped component 0. +#[test] +fn mask_effects_and_components_refuse_values_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("mask.saveEffect", &json!({"name": "Kept"})).unwrap(); + let state = |s: &Session| (format!("{:?}", s.develop_of(s.active().unwrap()).unwrap().masks), s.mask_effects.len()); + let before = state(&s); + let commits = s.commits(); + let d = s.develop_of(s.active().unwrap()).unwrap(); + let shape = serde_json::to_value(&d.masks.last().unwrap().components[0].shape).unwrap(); + for bad in [json!("1"), json!(-1), json!(1.5), json!([1]), json!(u64::from(u32::MAX) + 2)] { + for (cmd, params) in [ + ("mask.applyEffect", json!({"name": "Kept"})), + ("mask.saveEffect", json!({"name": "New"})), + ("mask.updateEffect", json!({"name": "Kept"})), + ] { + let mut params = params; + params["id"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`id`"), "{cmd} {bad}: {e}"); + } + if bad.is_u64() { + continue; // a well-formed index past the end is "no such component", not a type error + } + for (cmd, params) in + [("mask.update", json!({"shape": shape})), ("mask.component", json!({"action": "invert"})), ("mask.refineDetail", json!({}))] + { + let mut params = params; + params["component"] = bad.clone(); + let e = s.execute(cmd, ¶ms).unwrap_err(); + assert!(e.to_string().contains("`component`"), "{cmd} {bad}: {e}"); + } + } + assert_eq!(s.commits(), commits, "nothing was changed"); + assert_eq!(state(&s), before); +}