diff --git a/crates/engine/src/automation/params.rs b/crates/engine/src/automation/params.rs --- a/crates/engine/src/automation/params.rs +++ b/crates/engine/src/automation/params.rs @@ -1,16 +1,58 @@ //! Parameter parsing helpers shared by commands (lenient: agents send numbers as strings, ids as //! "12" or "0:12", colours as hex or CSS). +use std::cell::RefCell; +use std::collections::HashSet; + use figcraft_color::{BlendMode, Color, Paint}; use figcraft_doc::NodeId; use serde_json::{Value, json}; use crate::{EngineError, Result, Session, bad}; +thread_local! { + /// The parameter names each running command asked for, innermost command last. + static READ: RefCell>> = const { RefCell::new(Vec::new()) }; +} + pub fn get<'a>(p: &'a Value, key: &str) -> Option<&'a Value> { + note_read(key); p.get(key).filter(|v| !v.is_null()) } +fn note_read(key: &str) { + READ.with_borrow_mut(|r| { + if let Some(top) = r.last_mut() + && !top.contains(key) + { + top.insert(key.to_string()); + } + }); +} + +/// Count every key of `p` as read, for a command that takes keys it doesn't name (flat properties). +pub fn read_all(p: &Value) { + for key in p.as_object().into_iter().flat_map(|m| m.keys()) { + note_read(key); + } +} + +/// Run `f` and return what it returned with the parameter names it asked for through [`get`]. +pub(crate) fn tracking_reads(f: impl FnOnce() -> T) -> (T, HashSet) { + struct Frame; + impl Drop for Frame { + fn drop(&mut self) { + READ.with_borrow_mut(|r| r.pop()); + } + } + READ.with_borrow_mut(|r| r.push(HashSet::new())); + let frame = Frame; + let out = f(); + let read = READ.with_borrow_mut(|r| r.last_mut().map(std::mem::take).unwrap_or_default()); + drop(frame); + (out, read) +} + pub fn f64_of(v: &Value) -> Option { match v { Value::Number(n) => n.as_f64().filter(|f| f.is_finite()), diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -50,6 +50,40 @@ impl CommandSpec { pub fn shortcut(&self) -> Option<&'static str> { crate::catalog::shortcut(self.id) } + /// The parameter names `params` documents: the first word of each entry at the top level of + /// its `{..}` groups (`{ids?, index | name, effect {..}}` → ids, index, name, effect). + pub fn param_names(&self) -> Vec<&'static str> { + let doc = self.params; + let mut names = Vec::new(); + let mut depth = 0usize; + let mut entry_start = None; + for (i, c) in doc.char_indices() { + match c { + '{' | '(' | '[' => { + if depth == 0 && c == '{' { + entry_start = Some(i + 1); + } + depth += 1; + } + '}' | ')' | ']' => depth = depth.saturating_sub(1), + ',' | '|' if depth == 1 => entry_start = Some(i + 1), + _ => {} + } + if let Some(start) = entry_start + && depth == 1 + && !c.is_whitespace() + && i >= start + { + entry_start = None; + let rest = doc.get(i..).unwrap_or_default(); + let len = rest.find(|ch: char| !(ch.is_ascii_alphanumeric() || ch == '_')).unwrap_or(rest.len()); + if let Some(name) = rest.get(..len).filter(|n| !n.is_empty() && !names.contains(n)) { + names.push(name); + } + } + } + names + } } /// Define a command spec: `spec!(id, label, params_doc, history, run)`. diff --git a/crates/engine/src/cmd/comment.rs b/crates/engine/src/cmd/comment.rs --- a/crates/engine/src/cmd/comment.rs +++ b/crates/engine/src/cmd/comment.rs @@ -463,14 +463,20 @@ pub fn specs() -> Vec { Ok(json!({"on": on})) } ), - spec!("comment.resolve", "Resolve comment", "{id, resolved? (default true)}", true, |s, p| { - let id = req_comment("comment.resolve", s, p)?; - let v = boolean(p, "resolved").unwrap_or(true); - if let Some(c) = s.doc_mut()?.comment_mut(id) { - c.resolved = v; + spec!( + "comment.resolve", + "Resolve comment", + "{id, resolved? (default true), author? (accepted, ignored: anyone can resolve)}", + true, + |s, p| { + let id = req_comment("comment.resolve", s, p)?; + let v = boolean(p, "resolved").unwrap_or(true); + if let Some(c) = s.doc_mut()?.comment_mut(id) { + c.resolved = v; + } + ok() } - ok() - }), + ), spec!( "comment.move", "Move comment", @@ -659,4 +665,23 @@ mod tests { assert!(s.undo().unwrap()); assert_eq!(s.doc().unwrap().comments.len(), 1); } + + /// A parameter name the command doesn't know is refused, not ignored: a misspelt `filterr` made + /// `comment.list` report every thread as if no filter was given. + #[test] + fn misspelt_parameter_names_are_refused() { + let mut s = Session::new(); + let c = s.execute("comment.add", &json!({"x": 10, "y": 10, "text": "a"})).unwrap(); + let cid = c["id"].as_u64().unwrap(); + s.execute("comment.resolve", &json!({"id": cid})).unwrap(); + let err = s.execute("comment.list", &json!({"filterr": "open"})).unwrap_err().to_string(); + assert!(err.contains("filterr") && err.contains("filter"), "{err}"); + // The correct name still works, and finds nothing open. + let open = s.execute("comment.list", &json!({"filter": "open"})).unwrap(); + assert!(open["comments"].as_array().unwrap().is_empty()); + // A refused edit changes nothing. + let before = s.doc().unwrap().comments.len(); + assert!(s.execute("comment.add", &json!({"x": 1, "y": 1, "text": "b", "textt": "c"})).is_err()); + assert_eq!(s.doc().unwrap().comments.len(), before, "nothing stored"); + } } diff --git a/crates/engine/src/cmd/edit/props.rs b/crates/engine/src/cmd/edit/props.rs --- a/crates/engine/src/cmd/edit/props.rs +++ b/crates/engine/src/cmd/edit/props.rs @@ -94,7 +94,8 @@ pub(super) fn set(s: &mut Session, p: &Value) -> Result { let props = match get(p, "props") { Some(Value::Object(m)) => m.clone(), _ => { - // Allow flat params: {"id": 3, "fill": "#f00"}. + // Allow flat params: {"id": 3, "fill": "#f00"}. `apply` refuses keys it doesn't know. + crate::params::read_all(p); let mut m = p.as_object().cloned().unwrap_or_default(); m.remove("id"); m.remove("ids"); diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -72,7 +72,7 @@ pub fn specs() -> Vec { spec!( "file.open", "Open", - "{path | data (base64) + name (tells the format; opens as a new unsaved document), variables? (REST: the local variables JSON as an object, JSON text or a path; default `.variables.json` or `variables.json` next to the file)} (.figcraft, .json: FigCraft or REST API file JSON, .fig (best effort), .sketch (Sketch 48 or newer, best effort), .svg, .png, .jpg …); REST, .fig and .sketch imports return `warnings`; a file opened anew returns `summary` {name, pages, layers, images} (one that was already open just returns its `index`)", + "{path | data (base64), name (tells the format; opens as a new unsaved document), variables? (REST: the local variables JSON as an object, JSON text or a path; default `.variables.json` or `variables.json` next to the file)} (.figcraft, .json: FigCraft or REST API file JSON, .fig (best effort), .sketch (Sketch 48 or newer, best effort), .svg, .png, .jpg …); REST, .fig and .sketch imports return `warnings`; a file opened anew returns `summary` {name, pages, layers, images} (one that was already open just returns its `index`)", false, open ), @@ -107,7 +107,7 @@ pub fn specs() -> Vec { spec!( "file.export", "Export", - "{ids? (default selection, else the page), format? (png|jpg|svg|pdf|webp|xml (an Android Vector Drawable, with `warnings` for what it changed or left out), or fig: the whole file as .fig, experimental, with `warnings` for what was left out; other apps may not open every file), scale? (default 1) | size? (2x|512w|512h or {by: scale|width|height, value}: a scale, or a width or height in pixels), path? (else returns base64 `data`), bleed? (PDF: points, or with a unit: \"3mm\", \"0.125in\", \"9pt\"), cropMarks?, bleedMarks?, cmyk? (PDF print options), svgIdAttribute? (SVG ids, XML android:name), vdCompat? (XML: bestEffort (default: approximate with a warning)|exact (leave out what can't be exact, with a warning)|ignore (no warnings))}", + "{ids? (default selection, else the page), format? (png|jpg|svg|pdf|webp|xml (an Android Vector Drawable, with `warnings` for what it changed or left out), or fig: the whole file as .fig, experimental, with `warnings` for what was left out; other apps may not open every file), scale? (default 1) | size? (2x|512w|512h or {by: scale|width|height, value}: a scale, or a width or height in pixels), path? (else returns base64 `data`), bleed? (PDF: points, or with a unit: \"3mm\", \"0.125in\", \"9pt\"), cropMarks?, bleedMarks?, cmyk? (PDF print options), contentsOnly?, includeBoundingBox?, svgOutlineText?, svgSimplifyStroke? (true or false, as in the layer's export settings), svgIdAttribute? (SVG ids, XML android:name), vdCompat? (XML: bestEffort (default: approximate with a warning)|exact (leave out what can't be exact, with a warning)|ignore (no warnings))}", false, export ), @@ -135,7 +135,7 @@ pub fn specs() -> Vec { spec!( "file.import", "Import", - "{path | data (base64) + name, x?, y?, center? (SVG: centre it on x, y), variables? (REST: the local variables JSON as an object, JSON text or a path)} SVG becomes an editable frame (shapes, text, gradients, masks, effects; with `warnings` for what it left out), images become image layers, .figcraft, REST API file JSON, .fig and .sketch pages are appended with their variables and flows (REST, .fig and .sketch: with `warnings`; those return `summary` {name, pages, layers, images} of the file read)", + "{path | data (base64), name, x?, y?, center? (SVG: centre it on x, y), variables? (REST: the local variables JSON as an object, JSON text or a path)} SVG becomes an editable frame (shapes, text, gradients, masks, effects; with `warnings` for what it left out), images become image layers, .figcraft, REST API file JSON, .fig and .sketch pages are appended with their variables and flows (REST, .fig and .sketch: with `warnings`; those return `summary` {name, pages, layers, images} of the file read)", true, import ), diff --git a/crates/engine/src/library_variables_tests.rs b/crates/engine/src/library_variables_tests.rs --- a/crates/engine/src/library_variables_tests.rs +++ b/crates/engine/src/library_variables_tests.rs @@ -332,7 +332,7 @@ fn library_style_bindings_ignore_unrelated_local_variables() { assert_eq!(local, var, "the ids must collide for this test"); let r = idv(&ex(&mut s, "shape.rectangle", json!({"width": 40, "height": 40}))); ex(&mut s, "library.add", json!({"path": path})); - ex(&mut s, "library.applyStyle", json!({"library": lid, "style": "Brand", "id": r})); + ex(&mut s, "library.applyStyle", json!({"library": lid, "style": "Brand", "ids": [r]})); assert_eq!(fill(&s, r), "0000FF"); healthy(&s); diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -1,4 +1,7 @@ +use std::collections::HashSet; + use super::*; +use crate::cmd::CommandSpec; /// Whether a command, or a batch wrapping one, imports a file passed as Base64 `data`. fn command_carries_import_bytes(id: &str, params: &Value) -> bool { @@ -13,12 +16,42 @@ fn command_carries_import_bytes(id: &str, params: &Value) -> bool { } } +/// A parameter the command neither asked for nor documents changed nothing: refuse it (the edit is +/// rolled back) rather than report success for something that wasn't done. A misspelt `filterr` +/// made `comment.list` list every thread. +fn check_params_read(spec: &CommandSpec, params: &Value, read: &HashSet) -> Result<()> { + refuse_unknown_params(spec, params, |k| read.contains(k)) +} + +/// Commands whose effects outlive a rollback (tabs, branches, versions, libraries, preferences, the undo history, plugin publishing, written files, the font scan): their +/// unknown parameter names are refused before they run, by their documented names. +fn effects_outlive_rollback(id: &str) -> bool { + const FAMILIES: [&str; 11] = + ["file.", "doc.", "version.", "branch.", "library.", "libraries.", "pref.", "window.", "app.", "board.", "plugin.directory."]; + FAMILIES.iter().any(|f| id.starts_with(f)) || matches!(id, "command.batch" | "edit.undo" | "edit.redo" | "image.place" | "text.rescanFonts") +} + +fn refuse_unknown_params(spec: &CommandSpec, params: &Value, read: impl Fn(&str) -> bool) -> Result<()> { + let Some(given) = params.as_object() else { return Ok(()) }; + let documented = spec.param_names(); + let unknown: Vec<&str> = + given.iter().filter(|(k, v)| !v.is_null() && !read(k.as_str()) && !documented.contains(&k.as_str())).map(|(k, _)| k.as_str()).collect(); + if unknown.is_empty() { + return Ok(()); + } + let takes = if documented.is_empty() { "takes no parameters".to_string() } else { format!("takes {}", documented.join(", ")) }; + Err(bad(spec.id, format!("unknown parameters: {} (`{}` {takes})", unknown.join(", "), spec.id))) +} + impl Session { /// Run a command by id. Mutations become one undo step; a failure or panic leaves the document /// as it was. pub fn execute(&mut self, id: &str, params: &Value) -> Result { let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?; board::refuse_on_board(self, id)?; + if effects_outlive_rollback(id) { + refuse_unknown_params(spec, params, |_| false)?; + } // Revert replaces the document's uid: refuse before ending previews or syncing branches. if id == "file.revert" && self.live().doc_uid.is_some() { return Err(EngineError::Disabled(id.into(), "leave the live session before reverting to the saved file".into())); @@ -27,7 +60,8 @@ impl Session { self.apply_view_prefs(); if textpreview::COMMANDS.contains(&id) { // A live preview is no edit: no undo step, normalisation or rollback here. - let r = guard::run(id, || (spec.run)(self, params)); + let (r, read) = params::tracking_reads(|| guard::run(id, || (spec.run)(self, params))); + let r = r.and_then(|v| check_params_read(spec, params, &read).map(|()| v)); if r.is_err() { textpreview::end(self); } @@ -44,7 +78,9 @@ impl Session { let before = self.ds().ok().map(|d| (d.doc.clone(), d.selection.clone(), d.page, d.uid)); // Cut and copy write the clipboard before a later check can refuse the command. let clipboard_before = self.clipboard.clone(); - let result = guard::run(id, || (spec.run)(self, params)) + let (result, read) = params::tracking_reads(|| guard::run(id, || (spec.run)(self, params))); + let result = result + .and_then(|v| check_params_read(spec, params, &read).map(|()| v)) .and_then(|v| self.check_instance_structure(id, &before).map(|()| v)) .and_then(|v| self.check_preview(id, &before).map(|()| v)) .and_then(|v| self.check_live_access(id, &before).map(|()| v)) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -488,6 +488,64 @@ fn every_command_survives_junk_params() { } } +/// No command reports success for a parameter name it doesn't know: it used to ignore it, so a +/// misspelt option gave a different result with nothing saying so. +#[test] +fn every_command_refuses_an_unknown_parameter_name() { + let mut accepted = Vec::new(); + for spec in all_commands() { + // These write or read files, or walk the font folders; the check is the same for them. + if spec.id.starts_with("file.") || spec.id == "image.place" || spec.id == "text.rescanFonts" { + continue; + } + let mut s = sess(); + let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})); + if s.execute(spec.id, &json!({"zzNotAParameter": 1})).is_ok() { + accepted.push(spec.id); + } + } + assert!(accepted.is_empty(), "these commands accepted an unknown parameter name: {accepted:?}"); +} + +#[test] +fn an_unknown_parameter_name_stops_a_command_before_its_effects() { + let mut s = sess(); + let tabs = s.execute("doc.list", &json!({})).unwrap().to_string(); + assert!(s.execute("file.new", &json!({"nmae": "x"})).is_err()); + assert_eq!(s.execute("doc.list", &json!({})).unwrap().to_string(), tabs, "no tab was opened"); + assert!(s.execute("file.new", &json!({"name": "x"})).is_ok()); + // Tab switches and copies change the session before a post-run check could refuse them. + s.execute("file.new", &json!({"name": "y"})).unwrap(); + let state = |s: &mut Session| s.execute("doc.list", &json!({})).unwrap().to_string(); + s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap(); + s.execute("version.autosave", &json!({})).unwrap(); + let versions = s.execute("version.list", &json!({})).unwrap().to_string(); + assert!(versions.len() > 2, "the test needs a saved version: {versions}"); + assert!(s.execute("version.delete", &json!({"index": 0, "zzNotAParameter": 1})).is_err()); + assert_eq!(s.execute("version.list", &json!({})).unwrap().to_string(), versions, "version.delete removed a version"); + let before = state(&mut s); + // A batch's nested steps (here a new tab) run before its own unknown name could be refused. + let nested = json!({"commands": [{"command": "file.new", "params": {"name": "z"}}], "zzNotAParameter": 1}); + assert!(s.execute("command.batch", &nested).is_err()); + assert_eq!(state(&mut s), before, "command.batch opened a tab"); + assert!(s.execute("branch.create", &json!({"name": "b", "zzNotAParameter": 1})).is_err()); + assert_eq!(state(&mut s), before, "branch.create opened a tab"); + assert!(s.execute("branch.list", &json!({})).unwrap().as_array().is_some_and(|b| b.is_empty()), "a branch was recorded"); + // A refused undo must not consume the undo step. + s.execute("shape.rectangle", &json!({"x": 5, "y": 5, "width": 10, "height": 10})).unwrap(); + let layers = |s: &mut Session| s.ds().map(|d| d.doc.nodes().count()).unwrap_or(0); + let with_rect = layers(&mut s); + assert!(s.execute("edit.undo", &json!({"zzNotAParameter": 1})).is_err()); + assert_eq!(layers(&mut s), with_rect); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(layers(&mut s), with_rect - 1, "the refused undo used up the step"); + let before = state(&mut s); + for id in ["doc.switch", "doc.nextTab", "doc.previousTab", "version.duplicate"] { + assert!(s.execute(id, &json!({"index": 0, "zzNotAParameter": 1})).is_err(), "{id}"); + assert_eq!(state(&mut s), before, "{id} changed the tabs"); + } +} + #[test] fn parity_catalog_mostly_covered() { let (done, total, missing) = catalog::parity(&[]);