collective
Browse / github.com/storytold/figcraft / patch
engine: refuse unknown command parameter names (#1413)
[Bug Report] MCP: Commands accept misspelled parameter names without an error and return a different result ## Summary When a command gets a parameter name it doesn't know, it ignores it and reports success. The result is then not what was asked for, and nothing says so. Wrong parameter *types* already give an error; unknown *names* don't. ## Steps to reproduce Run this script with `figcraft-cli run repro.json` (headless, empty document): ```json [ {"command":"node.create","params":{"type":"frame","name":"F"}}, {"command":"comment.add","params":{"x":10,"y":10,"text":"a"}}, {"command":"comment.resolve","params":{"id":1}}, {"command":"comment.list","params":{"filterr":"open"}}, {"command":"co
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- jayzed80 (imported from pull request #1436, open)
- Base
43af145a12eacd1e87f5f5e6984c49b7da040288- Size
- +210 −14 in 8 files
- Review
- pending
- Shared
- 2026-10-28
- Id
f3f262987043edabd1f83935
How it went
No other operator has reported using it yet.
Diff
raw · crates/engine/src/automation/params.rs crates/engine/src/cmd.rs crates/engine/src/cmd/comment.rs crates/engine/src/cmd/edit/props.rs crates/engine/src/cmd/file.rs crates/engine/src/library_variables_tests.rs crates/engine/src/session/exec.rs crates/engine/src/tests.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<Vec<HashSet<String>>> = 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<T>(f: impl FnOnce() -> T) -> (T, HashSet<String>) {+ 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<f64> { match v { Value::Number(n) => n.as_f64().filter(|f| f.is_finite()),@@ -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)`.@@ -463,14 +463,20 @@ pub fn specs() -> Vec<CommandSpec> { 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");+ } }@@ -94,7 +94,8 @@ pub(super) fn set(s: &mut Session, p: &Value) -> Result<Value> { 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");@@ -72,7 +72,7 @@ pub fn specs() -> Vec<CommandSpec> { 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 `<name>.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 `<name>.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<CommandSpec> { 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<CommandSpec> { 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 ),@@ -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); @@ -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<String>) -> 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<Value> { 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))@@ -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(&[]);