From 714f127100a2abad0579f8a97f8282ba1ca60bd5 Mon Sep 17 00:00:00 2001 From: FredApps Date: Sun, 11 Oct 2026 20:39:28 +0200 Subject: [PATCH] M2: reject malformed explicit layer targets (#785) --- crates/engine/src/cmd/layer.rs | 36 +++---- crates/engine/src/cmd/layerpanel.rs | 25 +++-- crates/engine/src/tests_layers.rs | 157 ++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+), 29 deletions(-) diff --git a/crates/engine/src/cmd/layer.rs b/crates/engine/src/cmd/layer.rs index 81d4d4e4..4930f3ae 100644 --- a/crates/engine/src/cmd/layer.rs +++ b/crates/engine/src/cmd/layer.rs @@ -189,15 +189,16 @@ pub fn specs() -> Vec { /// The rows a panel command acts on: `ids` or `id`, else the highlighted rows, else the current /// layer (capped at [`MAX_ROWS`]). -pub(crate) fn rows_param(s: &Session, p: &Value) -> Result> { - let mut ids = match (ids_param(p, "ids"), id_param(p, "id")) { - (Some(ids), _) => ids, - (None, Some(id)) => vec![id], - (None, None) => { - let st = s.doc()?; - let rows = st.highlighted_rows(); - if rows.is_empty() { st.current_layer().into_iter().collect() } else { rows } - } +pub(crate) fn rows_param(s: &Session, p: &Value, command: &str) -> Result> { + // Presence is explicit intent: validate every target before fallback, truncation or edits. + let mut ids = if p.get("ids").is_some() { + checked_ids_param(s, p, "ids", command)? + } else if let Some(id) = p.get("id") { + vec![checked_id(s, id, command)?] + } else { + let st = s.doc()?; + let rows = st.highlighted_rows(); + if rows.is_empty() { st.current_layer().into_iter().collect() } else { rows } }; ids.truncate(MAX_ROWS); Ok(ids) @@ -457,7 +458,7 @@ fn new_sublayer(s: &mut Session, p: &Value) -> Result { } fn delete_rows(s: &mut Session, p: &Value) -> Result { - let ids = rows_param(s, p)?; + let ids = rows_param(s, p, "layer.delete")?; let st = s.doc()?; let ids = row_roots(&st.doc, &ids); if ids.is_empty() { @@ -484,7 +485,7 @@ fn delete_rows(s: &mut Session, p: &Value) -> Result { } fn duplicate_rows(s: &mut Session, p: &Value) -> Result { - let ids = rows_param(s, p)?; + let ids = rows_param(s, p, "layer.duplicate")?; let ids = row_roots(&s.doc()?.doc, &ids); if ids.is_empty() { return Err(bad("layer.duplicate", "no such rows")); @@ -551,7 +552,7 @@ fn highlight(s: &mut Session, p: &Value) -> Result { fn set_props(s: &mut Session, p: &Value) -> Result { const C: &str = "layer.setProps"; check_options(C, p)?; - let ids = rows_param(s, p)?; + let ids = rows_param(s, p, C)?; if ids.is_empty() { return Err(bad(C, "no rows")); } @@ -567,12 +568,11 @@ fn set_props(s: &mut Session, p: &Value) -> Result { fn collect(s: &mut Session, p: &Value) -> Result { const C: &str = "layer.collectInNew"; - let ids = match ids_param(p, "ids") { - Some(ids) => ids, - None => { - let rows = s.doc()?.highlighted_rows(); - if rows.is_empty() { super::edit::selected_roots(s)? } else { rows } - } + let ids = if p.get("ids").is_some() { + checked_ids_param(s, p, "ids", C)? + } else { + let rows = s.doc()?.highlighted_rows(); + if rows.is_empty() { super::edit::selected_roots(s)? } else { rows } }; let ids = row_roots(&s.doc()?.doc, &ids); let Some(top) = ids.last().copied() else { return Err(bad(C, "select rows or objects to collect")) }; diff --git a/crates/engine/src/cmd/layerpanel.rs b/crates/engine/src/cmd/layerpanel.rs index c069c33b..a3d85970 100644 --- a/crates/engine/src/cmd/layerpanel.rs +++ b/crates/engine/src/cmd/layerpanel.rs @@ -84,7 +84,7 @@ pub fn specs() -> Vec { None, "{ids?} hide every top-level layer but those holding the highlighted rows (else the current layer), which show. One undo step → {count}", has_doc, - |s, p| others(s, p, Flag::Visible, false) + |s, p| others(s, p, Flag::Visible, false, "layer.hideOthers") ), cmd!("layer.showAll", "Show All Layers", ["Window", "Layers"], None, "{} show every layer and sublayer → {count}", has_doc, |s, _| all( s, @@ -98,7 +98,7 @@ pub fn specs() -> Vec { None, "{ids?} turn Preview off (outline view) for every top-level layer but those holding the highlighted rows (else the current layer), which preview → {count}", has_doc, - |s, p| others(s, p, Flag::Preview, false) + |s, p| others(s, p, Flag::Preview, false, "layer.outlineOthers") ), cmd!( "layer.previewAll", @@ -116,7 +116,7 @@ pub fn specs() -> Vec { None, "{ids?} lock every top-level layer but those holding the highlighted rows (else the current layer), which unlock → {count}", has_doc, - |s, p| others(s, p, Flag::Locked, true) + |s, p| others(s, p, Flag::Locked, true, "layer.lockOthers") ), cmd!( "layer.unlockAll", @@ -196,16 +196,16 @@ fn set_flag(s: &mut Session, changes: Vec<(NodeId, bool)>, flag: Flag, label: &s } /// The top-level layers holding the rows a command acts on. -fn kept_layers(s: &Session, p: &Value) -> Result> { - let ids = rows_param(s, p)?; +fn kept_layers(s: &Session, p: &Value, command: &str) -> Result> { + let ids = rows_param(s, p, command)?; let d = &s.doc()?.doc; Ok(ids.iter().filter_map(|id| d.layer_of(*id)).collect()) } /// Every top-level layer but the kept ones set to `v`, and the kept ones to the opposite: Alt-click /// on a hidden layer's eye shows just that layer (#1158). -fn others(s: &mut Session, p: &Value, flag: Flag, v: bool) -> Result { - let keep = kept_layers(s, p)?; +fn others(s: &mut Session, p: &Value, flag: Flag, v: bool, command: &str) -> Result { + let keep = kept_layers(s, p, command)?; let changes = s.doc()?.doc.layers.iter().map(|l| (l.id, !keep.contains(&l.id) == v)).collect(); set_flag(s, changes, flag, flag.label(v)) } @@ -221,7 +221,7 @@ fn all(s: &mut Session, flag: Flag, v: bool) -> Result { } fn template(s: &mut Session, p: &Value) -> Result { - let ids = rows_param(s, p)?; + let ids = rows_param(s, p, "layer.template")?; let d = &s.doc()?.doc; let ids: Vec = ids.into_iter().filter(|id| d.node(*id).is_some_and(Node::is_layer)).collect(); let Some(first) = ids.first().and_then(|id| d.node(*id)) else { return Err(bad("layer.template", "select a layer")) }; @@ -268,7 +268,7 @@ fn merge_into(d: &mut Document, into: NodeId, others: &[NodeId]) -> Result<()> { fn merge(s: &mut Session, p: &Value) -> Result { const C: &str = "layer.merge"; - let ids = rows_param(s, p)?; + let ids = rows_param(s, p, C)?; let st = s.doc()?; let d = &st.doc; let mut seen = HashSet::new(); @@ -300,7 +300,10 @@ fn flatten(s: &mut Session, p: &Value) -> Result { const C: &str = "layer.flatten"; let st = s.doc()?; let d = &st.doc; - let from = id_param(p, "id").or_else(|| st.highlighted_rows().last().copied()).or(st.current_layer()); + let from = match p.get("id") { + Some(id) => Some(checked_id(s, id, C)?), + None => st.highlighted_rows().last().copied().or(st.current_layer()), + }; let into = from.and_then(|id| d.layer_of(id)).ok_or_else(|| bad(C, "no layer"))?; if !d.node(into).is_some_and(|n| n.visible) { return Err(EngineError::Other("the layer to flatten into is hidden".into())); @@ -389,7 +392,7 @@ fn release(s: &mut Session, p: &Value) -> Result { fn reverse(s: &mut Session, p: &Value) -> Result { const C: &str = "layer.reverse"; - let ids = rows_param(s, p)?; + let ids = rows_param(s, p, C)?; let d = &s.doc()?.doc; let ids = row_roots(d, &ids); // Rows grouped by the layer or group holding them (None: top-level layers). diff --git a/crates/engine/src/tests_layers.rs b/crates/engine/src/tests_layers.rs index 6f3111b7..36006d72 100644 --- a/crates/engine/src/tests_layers.rs +++ b/crates/engine/src/tests_layers.rs @@ -614,3 +614,160 @@ fn the_clipping_mask_button_acts_on_the_highlighted_group() { assert_eq!(run(&mut s, "layer.clippingMask.toggle", json!({}))["clip"], true); assert!(node(&s, g).clips()); } + +/// #785: explicit malformed layer targets must never become the current-layer default. +#[test] +fn layer_delete_rejects_invalid_explicit_targets() { + let mut unexpected = vec![]; + for params in [json!({"id": "1"}), json!({"ids": [1, "2"]}), json!({"id": "$1.id"})] { + let mut s = session(); + run(&mut s, "layer.new", json!({})); + run(&mut s, "layer.new", json!({})); + let before = layer_target_snapshot(&s); + let result = s.execute("layer.delete", ¶ms); + if result.is_ok() { + let names: Vec<_> = s.doc().unwrap().doc.layers.iter().map(|l| l.display_name()).collect(); + unexpected.push(format!("{params}: {result:?}; remaining layers {names:?}")); + } else { + assert_eq!(layer_target_snapshot(&s), before, "{params}"); + } + } + assert!(unexpected.is_empty(), "malformed requests succeeded: {}", unexpected.join("; ")); +} + +fn layer_target_snapshot(s: &Session) -> Value { + let st = s.doc().unwrap(); + let history = + |entries: &[HistoryEntry]| entries.iter().map(|e| json!({"label": e.label, "doc": e.doc, "selection": e.selection})).collect::>(); + json!({"doc": st.doc, "selection": st.selection, "revision": st.revision, "activeLayer": st.active_layer, + "rows": st.layer_rows, "undo": history(&st.history.undo), "redo": history(&st.history.redo)}) +} + +fn layer_target_fixture() -> (Session, Vec) { + let mut s = session(); + let first = s.doc().unwrap().doc.layers[0].id; + rect(&mut s, 0.0); + let second = id(&run(&mut s, "layer.new", json!({}))); + rect(&mut s, 40.0); + let third = id(&run(&mut s, "layer.new", json!({}))); + rect(&mut s, 80.0); + run(&mut s, "layer.highlight", json!({"ids": [first.0, second.0]})); + (s, vec![first, second, third]) +} + +fn malformed_layer_targets(first: NodeId, second: NodeId) -> Vec { + let mut params = vec![]; + for bad in [json!("1"), json!("$1.id"), json!(1.0), json!(1.5), json!(-1), Value::Null, json!(true), json!({}), json!([])] { + params.push(json!({"id": bad})); + params.push(json!({"ids": [first.0, bad]})); + } + for bad in [Value::Null, json!("1"), json!(1), json!(false), json!({})] { + params.push(json!({"ids": bad, "id": first.0})); + } + for missing in [0, u64::MAX] { + params.push(json!({"id": missing})); + params.push(json!({"ids": [first.0, second.0, missing]})); + } + params +} + +fn assert_layer_target_error(s: &mut Session, command: &str, params: Value) { + let before = layer_target_snapshot(s); + let error = s.execute(command, ¶ms).unwrap_err(); + assert!(error.to_string().contains(command), "{command} {params}: {error}"); + assert_eq!(layer_target_snapshot(s), before, "{command} {params}: rejected targets must leave all state unchanged"); +} + +/// #785: shared row commands reject malformed or unknown targets before any partial edits. +#[test] +fn layer_target_validation_keeps_document_selection_revision_and_history_unchanged() { + let (_, rows) = layer_target_fixture(); + for command in [ + "layer.delete", + "layer.duplicate", + "layer.setProps", + "layer.template", + "layer.merge", + "layer.reverse", + "layer.hideOthers", + "layer.outlineOthers", + "layer.lockOthers", + ] { + for mut params in malformed_layer_targets(rows[0], rows[1]) { + let (mut s, _) = layer_target_fixture(); + if command == "layer.setProps" { + params["name"] = json!("Changed"); + } + assert_layer_target_error(&mut s, command, params); + } + } +} + +/// These commands have their own selectors, not the shared row-default parser. +#[test] +fn layer_collect_and_flatten_validate_their_own_explicit_targets() { + let (_, rows) = layer_target_fixture(); + for params in malformed_layer_targets(rows[0], rows[1]) { + let command = if params.get("ids").is_some() { "layer.collectInNew" } else { "layer.flatten" }; + let (mut s, _) = layer_target_fixture(); + assert_layer_target_error(&mut s, command, params); + } +} + +/// Truncation limits work, not validation: a malformed or unknown tail cannot be ignored. +#[test] +fn layer_target_arrays_are_fully_validated_before_the_row_limit() { + // The existing panel row limit is 100,000; exercise the public command beyond it. + const ROW_LIMIT: usize = 100_000; + for tail in [json!("$1.id"), json!(u64::MAX)] { + let (mut s, rows) = layer_target_fixture(); + let mut values = vec![json!(rows[0].0); ROW_LIMIT + 1]; + *values.last_mut().unwrap() = tail; + assert_layer_target_error(&mut s, "layer.delete", json!({"ids": values})); + } + let (mut s, rows) = layer_target_fixture(); + let mut values = vec![json!(rows[0].0); ROW_LIMIT + 1]; + *values.last_mut().unwrap() = json!(rows[1].0); + run(&mut s, "layer.setProps", json!({"ids": values, "name": "Within limit"})); + assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Within limit")); + assert_ne!(node(&s, rows[1]).name.as_deref(), Some("Within limit"), "the valid tail remains beyond the work cap"); +} + +#[test] +fn layer_target_defaults_and_valid_explicit_precedence_are_preserved() { + let (mut s, rows) = layer_target_fixture(); + run(&mut s, "layer.setProps", json!({"name": "Highlighted"})); + assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Highlighted")); + assert_eq!(node(&s, rows[1]).name.as_deref(), Some("Highlighted")); + run(&mut s, "layer.setProps", json!({"id": rows[2].0, "name": "Single"})); + assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Single")); + let before = undos(&s); + run(&mut s, "layer.setProps", json!({"ids": [rows[1].0, rows[0].0, rows[1].0], "id": "$1.id", "name": "Explicit"})); + assert_eq!(node(&s, rows[0]).name.as_deref(), Some("Explicit")); + assert_eq!(node(&s, rows[1]).name.as_deref(), Some("Explicit")); + assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Single")); + assert_eq!(undos(&s), before + 1, "duplicates remain one edit"); + assert_layer_target_error(&mut s, "layer.setProps", json!({"ids": [], "id": rows[2].0, "name": "Must not fall back"})); + run(&mut s, "layer.setCurrent", json!({"id": rows[2].0})); + run(&mut s, "layer.highlight", json!({"ids": []})); + run(&mut s, "layer.setProps", json!({"name": "Current"})); + assert_eq!(node(&s, rows[2]).name.as_deref(), Some("Current")); + let before = undos(&s); + run(&mut s, "layer.delete", json!({})); + assert!(s.doc().unwrap().doc.node(rows[2]).is_none()); + assert_eq!(undos(&s), before + 1); +} + +#[test] +fn layer_collect_and_flatten_keep_valid_and_omitted_target_behavior() { + let (mut s, rows) = layer_target_fixture(); + let r = run(&mut s, "layer.flatten", json!({"id": rows[2].0})); + assert_eq!(r["id"], json!(rows[2].0)); + assert_eq!(s.doc().unwrap().doc.layers.len(), 1); + let (mut s, _) = layer_target_fixture(); + let selected = s.doc().unwrap().selection.objects.clone(); + s.doc_mut().unwrap().layer_rows.clear(); + let r = run(&mut s, "layer.collectInNew", json!({})); + let collected = id(&r); + assert!(selected.iter().all(|id| parent(&s, *id) == Some(collected))); +}