diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index af768284b0..e75c7656de 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -784,7 +784,7 @@ pub use docmenu::guide_indexes; pub(crate) use edit::{settle_interaction, typing_in_progress}; pub(crate) use object::bounds_rewrite; pub(crate) use object::{keep_aligned_in, refresh_pixel_aligned}; -pub use plugin_call::{Location, key_location, location_in, location_keys}; +pub use plugin_call::{Location, ParamsPlace, key_location, location_in, location_keys}; pub(crate) use vectorcraft_doc::range::parse_range; /// A date param in Unix seconds that defaults to the clock: `None` when absent (now, see diff --git a/crates/engine/src/cmd/params.rs b/crates/engine/src/cmd/params.rs index c38854ae76..40e097e205 100644 --- a/crates/engine/src/cmd/params.rs +++ b/crates/engine/src/cmd/params.rs @@ -174,7 +174,7 @@ fn accepted(doc: &'static str, depth: usize) -> Option> { /// Does the description `doc` name `key` as a parameter (its own keys, and those of the commands /// its `…` entries name)? -pub(crate) fn takes_key(doc: &'static str, key: &str) -> bool { +pub fn takes_key(doc: &'static str, key: &str) -> bool { doc_keys(doc).is_ok_and(|parsed| parsed.keys.contains(&key)) || accepted(doc, 0).is_some_and(|keys| keys.contains(&key)) } diff --git a/crates/engine/src/cmd/plugin_call.rs b/crates/engine/src/cmd/plugin_call.rs index 71fbcc8540..7001cf2ebd 100644 --- a/crates/engine/src/cmd/plugin_call.rs +++ b/crates/engine/src/cmd/plugin_call.rs @@ -101,12 +101,30 @@ const FILE_WORDS: &[&str] = &[ "clipboard", ]; -/// Param keys that name a file or a folder. -const FILE_KEYS: &[&str] = - &["path", "paths", "file", "files", "folder", "folders", "dir", "directory", "destinationFolder", "sourceFolder", "url", "diffPath"]; +/// Param keys that name a file or a folder, each by its whole name, the folder preferences +/// `prefs.set {values}` takes included: a command that adds a key naming one adds it here. A key +/// that only ends like one (`alignToPath`, `linksFolder`, `sortPaths`) names none, nor do `folders` +/// (how an export groups its files) and `url` (a web link). +const FILE_KEYS: &[&str] = &[ + "path", + "paths", + "file", + "files", + "folder", + "dir", + "directory", + "destinationFolder", + "sourceFolder", + "diffPath", + "fontsFolder", + "pluginsFolder", + "recoveryFolder", + "templatesFolder", +]; /// The keys in [`FILE_KEYS`] that name a folder. -const FOLDER_KEYS: &[&str] = &["folder", "folders", "dir", "directory", "destinationFolder", "sourceFolder"]; +const FOLDER_KEYS: &[&str] = + &["folder", "dir", "directory", "destinationFolder", "sourceFolder", "fontsFolder", "pluginsFolder", "recoveryFolder", "templatesFolder"]; /// What a param key that names a place on disk names there. #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -132,7 +150,83 @@ pub fn key_location(key: &str) -> Option { /// What `key` names on disk in command `spec`: [`key_location`], or `None` when [`NOT_FILES`] /// names it for `spec` (it names part of the document there). pub fn location_in(spec: &CommandSpec, key: &str) -> Option { - if NOT_FILES.contains(&(spec.id, key)) { None } else { key_location(key) } + ParamsPlace::top(Some(spec)).member(key, None).1 +} + +/// A place in the params of a command. What a key names on disk depends on the command and on the +/// keys above it: [`key_location`], less a key [`NOT_FILES`] names for the command at the top of its +/// params and one at the end of a [`NESTED_NOT_FILES`] key list. A walk of params starts at +/// [`ParamsPlace::top`] and goes down with [`ParamsPlace::member`] and [`ParamsPlace::item`]. +#[derive(Clone, Copy, Debug)] +pub struct ParamsPlace { + /// The command whose params these are; `None` for a command that isn't known. + command: Option<&'static str>, + /// Whether this is the top of the params. + top: bool, + /// The keys of a [`NESTED_NOT_FILES`] list still to follow below this place. + trail: Option<&'static [&'static str]>, +} + +impl ParamsPlace { + /// The top of the params of `spec`, or of a command that isn't known (`None`), where every key in + /// [`FILE_KEYS`] names a file or a folder. + pub fn top(spec: Option<&CommandSpec>) -> ParamsPlace { + ParamsPlace { command: spec.map(|s| s.id), top: true, trail: None } + } + + /// The top of the params of the command with id `id` ([`ParamsPlace::top`]). + pub fn of(id: &str) -> ParamsPlace { + ParamsPlace::top(super::find_command(id)) + } + + /// The top of the params of the command that object `fields` calls, when it is a call + /// (`{command | id: , params}`, as a `command.batch` step or a control request is): + /// the place of its `params` member ([`ParamsPlace::member`]). + pub fn carried(fields: &serde_json::Map) -> Option { + if !fields.contains_key("params") { + return None; + } + let id = fields.get("command").or_else(|| fields.get("id")).and_then(Value::as_str)?; + super::find_command(id).map(|spec| ParamsPlace::top(Some(spec))) + } + + /// The top of the params of the command that array `items` records, when it is a recorded step + /// (`[command id, params]`): the place of its item 1 ([`ParamsPlace::item`]). + pub fn step(items: &[Value]) -> Option { + match items { + [Value::String(id), Value::Object(_)] => super::find_command(id).map(|spec| ParamsPlace::top(Some(spec))), + _ => None, + } + } + + /// The place of member `key` of an object here, with what `key` names on disk there. `carried` is + /// [`ParamsPlace::carried`] of that object: its `params` member is then the top of that command's + /// params, and names no file. + pub fn member(self, key: &str, carried: Option) -> (ParamsPlace, Option) { + if key == "params" + && let Some(inner) = carried + { + return (inner, None); + } + let rest = if self.top { + let id = self.command; + NESTED_NOT_FILES.iter().find(|(c, keys)| Some(*c) == id && keys.first() == Some(&key)).map(|(_, keys)| keys.get(1..).unwrap_or_default()) + } else { + self.trail.filter(|keys| keys.first() == Some(&key)).map(|keys| keys.get(1..).unwrap_or_default()) + }; + let document = (self.top && self.command.is_some_and(|id| NOT_FILES.contains(&(id, key)))) || rest.is_some_and(<[&str]>::is_empty); + let place = ParamsPlace { command: self.command, top: false, trail: rest.filter(|keys| !keys.is_empty()) }; + (place, if document { None } else { key_location(key) }) + } + + /// The place of item `index` of an array here. `step` is [`ParamsPlace::step`] of that array: its + /// item 1 is then the top of that command's params. + pub fn item(self, index: usize, step: Option) -> ParamsPlace { + match step { + Some(inner) if index == 1 => inner, + _ => ParamsPlace { command: self.command, top: false, trail: None }, + } + } } /// The param keys of `spec` that name a file or a folder, with what they name: the keys in @@ -162,11 +256,8 @@ pub(crate) const NOT_FILES: &[(&str, &str)] = &[ ("symbol.instance.setAppearance", "path"), ("symbol.instance.selectPart", "path"), ("object.objectsOnPath.attach", "path"), - ("attributes.set", "url"), ("svgInteractivity.info", "files"), ("addon.cleanup.rejoin", "paths"), - ("object.slice.options", "url"), - ("document.exportForScreens", "folders"), ]; /// Commands and the keys from the top of their params down to a key in [`FILE_KEYS`] that, there, @@ -234,34 +325,24 @@ pub(crate) fn names_key(text: &str, key: &str) -> bool { }) } -/// Does `args`, the arguments of a call of command `id`, carry a key in [`FILE_KEYS`] at any depth -/// (in objects and in arrays), such as an effect's `textureImage.path`? Only a key [`NOT_FILES`] -/// names for `id`, at the top level, and one at the end of a [`NESTED_NOT_FILES`] key list for -/// `id`, aren't counted. The walk keeps its own stack, so deep arguments can't overflow the -/// thread's. +/// Does `args`, the arguments of a call of command `id`, carry a key that names a file or a folder at +/// any depth (in objects and in arrays, [`ParamsPlace`]), such as an effect's `textureImage.path`? +/// Only a key [`NOT_FILES`] names for `id`, at the top level, and one at the end of a +/// [`NESTED_NOT_FILES`] key list for `id`, aren't counted. A call or a recorded step inside `args` +/// is read as part of `id`'s arguments, not as the command it names. The walk keeps its own stack, +/// so deep arguments can't overflow the thread's. fn names_file(id: &str, args: &Value) -> bool { - // Each value, whether it is the params themselves, and the keys of a NESTED_NOT_FILES list - // still to follow below it. - let mut stack: Vec<(&Value, bool, Option<&[&str]>)> = vec![(args, true, None)]; - while let Some((v, top, trail)) = stack.pop() { + let mut stack: Vec<(&Value, ParamsPlace)> = vec![(args, ParamsPlace::of(id))]; + while let Some((v, place)) = stack.pop() { match v { - Value::Array(a) => stack.extend(a.iter().map(|x| (x, false, None))), + Value::Array(a) => stack.extend(a.iter().enumerate().map(|(i, x)| (x, place.item(i, None)))), Value::Object(o) => { for (k, x) in o { - let k = k.as_str(); - let rest = if top { - NESTED_NOT_FILES - .iter() - .find(|(c, keys)| *c == id && keys.first() == Some(&k)) - .map(|(_, keys)| keys.get(1..).unwrap_or_default()) - } else { - trail.filter(|keys| keys.first() == Some(&k)).map(|keys| keys.get(1..).unwrap_or_default()) - }; - let document = (top && NOT_FILES.contains(&(id, k))) || rest.is_some_and(<[&str]>::is_empty); - if FILE_KEYS.contains(&k) && !document { + let (below, location) = place.member(k, None); + if location.is_some() { return true; } - stack.push((x, false, rest.filter(|keys| !keys.is_empty()))); + stack.push((x, below)); } } _ => {} @@ -595,7 +676,13 @@ mod tests { assert!(names_file("effect.apply", &texture)); assert!(names_file("effect.setParams", &texture)); assert!(names_file("effect.apply", &json!({"params": {"layers": [{"name": "a"}, {"folder": "/tmp"}]}}))); - assert!(names_file("select.all", &json!([[[{"url": "file:///etc/hosts"}]]]))); + assert!(names_file("select.all", &json!([[[{"file": "file:///etc/hosts"}]]]))); + // A web link and an export's grouping name no file. + assert!(!names_file("attributes.set", &json!({"url": "https://example.com/a"}))); + assert!(!names_file("object.move", &json!({"opts": {"folders": "rows", "alignToPath": "baseline"}}))); + // A call nested in the arguments doesn't vouch for its own keys. + assert!(names_file("object.move", &json!({"opts": {"command": "object.setLiveShape", "params": {"path": "/tmp/x"}}}))); + assert!(names_file("object.move", &json!({"opts": ["object.setLiveShape", {"path": "/tmp/x"}]}))); assert!(!names_file("effect.apply", &json!({"effect": "distort.glass", "params": {"distortion": 5, "textureImage": "frosted"}}))); assert!(!names_file("object.move", &json!({"dx": 5, "dy": 0, "ids": [1, 2], "opts": {"copy": false}}))); // `path` is a shape's path in object.setLiveShape, but only as its own param. @@ -670,11 +757,48 @@ mod tests { assert!(found.iter().all(|(k, l)| (*l == Location::Folder) == FOLDER_KEYS.contains(k)), "{}: {found:?}", spec.id); assert!(denied(spec).is_some() || found.is_empty(), "{} names {found:?} but a plug-in may call it", spec.id); } - assert_eq!(key_location("url"), Some(Location::File)); + assert_eq!(key_location("url"), None); + assert_eq!(key_location("folders"), None); + assert_eq!(key_location("linksFolder"), None); assert_eq!(key_location("sourceFolder"), Some(Location::Folder)); assert_eq!(key_location("pathData"), None); } + /// A key keeps the meaning its command gives it at every depth: a call (`{command, params}`) and a + /// recorded step (`[command id, params]`) carry their command's exceptions down, and a custom + /// arrowhead's outline is part of the document only at its own place in `stroke.set`. + #[test] + fn a_params_place_follows_its_command() { + use serde_json::json; + let at = |place: ParamsPlace, keys: &[&str]| { + let mut place = place; + let mut location = None; + for k in keys { + (place, location) = place.member(k, None); + } + location + }; + assert_eq!(at(ParamsPlace::of("object.setLiveShape"), &["path"]), None); + assert_eq!(at(ParamsPlace::of("object.setLiveShape"), &["options", "path"]), Some(Location::File)); + assert_eq!(at(ParamsPlace::of("stroke.set"), &["endArrow", "Custom", "path"]), None); + assert_eq!(at(ParamsPlace::of("stroke.set"), &["endArrow", "Custom", "file"]), Some(Location::File)); + assert_eq!(at(ParamsPlace::of("no.such.command"), &["path"]), Some(Location::File)); + assert_eq!(at(ParamsPlace::top(None), &["destinationFolder"]), Some(Location::Folder)); + let call = json!({"command": "object.setLiveShape", "params": {"path": [[0, 0]]}}); + let carried = call.as_object().and_then(ParamsPlace::carried); + let (inner, location) = ParamsPlace::of("command.batch").member("params", carried); + assert_eq!(location, None); + assert_eq!(inner.member("path", None).1, None); + assert_eq!(ParamsPlace::carried(&serde_json::Map::new()).map(|_| ()), None); + let unknown = json!({"command": "no.such.command", "params": {}}); + assert!(unknown.as_object().and_then(ParamsPlace::carried).is_none()); + let step = [json!("object.setLiveShape"), json!({"path": [[0, 0]]})]; + let place = ParamsPlace::top(None).item(1, ParamsPlace::step(&step)); + assert_eq!(place.member("path", None).1, None); + assert_eq!(ParamsPlace::top(None).item(0, ParamsPlace::step(&step)).member("path", None).1, Some(Location::File)); + assert!(ParamsPlace::step(&[json!("object.setLiveShape")]).is_none()); + } + #[test] fn keys_are_found_as_whole_keys() { assert!(names_key("{path: a .wasm file}", "path")); diff --git a/crates/engine/src/file_access.rs b/crates/engine/src/file_access.rs index 6ca724d810..9f676e737c 100644 --- a/crates/engine/src/file_access.rs +++ b/crates/engine/src/file_access.rs @@ -222,11 +222,11 @@ pub fn written_elsewhere(path: &str) -> bool { !cfg!(windows) && foreign && current().is_some() } -/// Does a command param named `key` hold a file or folder path: `path`, `paths`, `file`, `files`, -/// `folder`, or a name ending in `Path`, `Paths`, `File`, `Files` or `Folder` (`diffPath`, -/// `destinationFolder`)? +/// Does a command param named `key` hold a file or folder path in a command that gives it no other +/// meaning ([`crate::cmd::key_location`]: `path`, `file`, `diffPath`, `destinationFolder` and the +/// like)? [`crate::cmd::ParamsPlace`] tells what it holds in a given command. pub fn is_path_key(key: &str) -> bool { - matches!(key, "path" | "paths" | "file" | "files" | "folder") || ["Path", "Paths", "File", "Files", "Folder"].iter().any(|end| key.ends_with(end)) + crate::cmd::key_location(key).is_some() } /// May `path` be written here? Always, without roots in force. diff --git a/crates/engine/tests/command_sweep.rs b/crates/engine/tests/command_sweep.rs index 6577754de0..0cc67e074c 100644 --- a/crates/engine/tests/command_sweep.rs +++ b/crates/engine/tests/command_sweep.rs @@ -5,6 +5,7 @@ #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use serde_json::{Value, json}; +use vectorcraft_engine::cmd::{Location, ParamsPlace}; use vectorcraft_engine::{Session, command_specs}; use vectorcraft_testkit::catch_quiet; use vectorcraft_testkit::fixtures::Fixture; @@ -271,44 +272,46 @@ fn tool_edit_cases(c: &vectorcraft_engine::CommandSpec, path: &str) -> Vec) { +/// The strings in `v`, the value at `at` (found at `place`) in a case of command `id`, that name a +/// place on disk outside `dir`, as `id at: value`: `v` itself and the members of arrays in it when +/// `named` (it is under a key that names a file or folder), and in objects at any depth, the values +/// under such keys as the command reads them (`vectorcraft_engine::cmd::ParamsPlace`). +fn outside_locations( + id: &str, + at: &str, + v: &Value, + place: ParamsPlace, + named: bool, + dir: &std::path::Path, + out: &mut std::collections::BTreeSet, +) { match v { Value::String(s) if named && !std::path::Path::new(s).starts_with(dir) => { out.insert(format!("{id} {at}: {v}")); } Value::Array(a) => { + let step = ParamsPlace::step(a); for (i, x) in a.iter().enumerate() { - outside_locations(id, &format!("{at}[{i}]"), x, named, dir, out); + outside_locations(id, &format!("{at}[{i}]"), x, place.item(i, step), named, dir, out); } } Value::Object(o) => { + let carried = ParamsPlace::carried(o); for (k, x) in o { - outside_locations(id, &format!("{at}.{k}"), x, names_location(None, k), dir, out); + let (below, location) = place.member(k, carried); + outside_locations(id, &format!("{at}.{k}"), x, below, location.is_some(), dir, out); } } _ => {} } } -/// Does `key` name a file or folder, in command `spec` when given: a key in -/// `vectorcraft_engine::cmd::key_location`'s list that `vectorcraft_engine::cmd::location_in` keeps -/// for `spec`. -fn names_location(spec: Option<&vectorcraft_engine::CommandSpec>, key: &str) -> bool { - match spec { - Some(spec) => vectorcraft_engine::cmd::location_in(spec, key).is_some(), - None => vectorcraft_engine::cmd::key_location(key).is_some(), - } -} - /// Every string the sweeps send under a key that names a file or folder, in the command catalog /// (`vectorcraft_engine::cmd::location_keys`, so a command added later is checked too) or inside /// an object or an array at any depth, is inside the sweep's temp folder, so no run reads or /// writes in the working directory. `every_command_with_junk_params` sends every such key of every -/// command alone in at least one case ([`junk_cases`]). +/// command alone in at least one case ([`junk_cases`]). Which catalog keys name a file or folder is +/// checked on its own by [`catalog_path_keys_match_location_policy`]. #[test] fn sweep_cases_name_files_only_inside_the_temp_folder() { let path = safe_path(); @@ -329,7 +332,8 @@ fn sweep_cases_name_files_only_inside_the_temp_folder() { for p in &cases { let Some(o) = p.as_object() else { continue }; for (k, v) in o { - outside_locations(c.id, k, v, names_location(Some(c), k), &dir, &mut outside); + let (below, location) = ParamsPlace::top(Some(c)).member(k, None); + outside_locations(c.id, k, v, below, location.is_some(), &dir, &mut outside); } } } @@ -338,6 +342,105 @@ fn sweep_cases_name_files_only_inside_the_temp_folder() { assert!(outside.is_empty(), "{} cases name a location outside {}:\n{}", outside.len(), dir.display(), outside.join("\n")); } +/// The param keys that name a file or a folder, and which. +const LOCATION_KEYS: &[(&str, Location)] = &[ + ("path", Location::File), + ("paths", Location::File), + ("file", Location::File), + ("files", Location::File), + ("diffPath", Location::File), + ("folder", Location::Folder), + ("dir", Location::Folder), + ("directory", Location::Folder), + ("destinationFolder", Location::Folder), + ("sourceFolder", Location::Folder), + ("fontsFolder", Location::Folder), + ("pluginsFolder", Location::Folder), + ("recoveryFolder", Location::Folder), + ("templatesFolder", Location::Folder), +]; + +/// Param keys that read like a file or folder key ([`reads_like_a_location`]) and name neither: +/// web links, an export's grouping of its files, and choices and switches. +const NOT_LOCATIONS: &[&str] = &[ + "url", + "copyrightUrl", + "folders", + "alignToPath", + "wholePaths", + "emptyTextPaths", + "sortPaths", + "alterPaths", + "includeLinkedFiles", + "linksFolder", +]; + +/// Commands and the key in [`LOCATION_KEYS`] that, at the top of their params, names part of the +/// document (a path of its art, a result field), not a file. +const DOCUMENT_PARTS: &[(&str, &str)] = &[ + ("object.setLiveShape", "path"), + ("path.reverse", "paths"), + ("object.pathfinder.options", "paths"), + ("object.maskImage", "path"), + ("text.createInPath", "path"), + ("imageTrace.make", "paths"), + ("symbol.instance.setAppearance", "path"), + ("symbol.instance.selectPart", "path"), + ("object.objectsOnPath.attach", "path"), + ("svgInteractivity.info", "files"), + ("addon.cleanup.rejoin", "paths"), +]; + +/// Does param key `k` read like one that names a file, a folder or a link: a bare name for one, or a +/// name that ends like one? +fn reads_like_a_location(k: &str) -> bool { + const ENDS: &[&str] = &["Path", "Paths", "File", "Files", "Folder", "Folders", "Dir", "Directory", "Url", "URL"]; + matches!(k, "path" | "paths" | "file" | "files" | "folder" | "folders" | "dir" | "directory" | "url") || ENDS.iter().any(|end| k.ends_with(end)) +} + +/// Every key of every command that reads like a file or folder key (in its params description, the +/// commands its `…` entries name and its schema) is in [`LOCATION_KEYS`] or [`NOT_LOCATIONS`], so a +/// command that adds one fails here until it is sorted. The generic classifications +/// (`vectorcraft_engine::cmd::key_location`, `vectorcraft_engine::file_access::is_path_key`) agree +/// with those lists, and each command reads such a key as they say (`location_in`), less +/// [`DOCUMENT_PARTS`]. +#[test] +fn catalog_path_keys_match_location_policy() { + use vectorcraft_engine::cmd::{key_location, location_in, params::takes_key}; + let catalog: Vec<(&vectorcraft_engine::CommandSpec, Vec)> = command_specs().iter().map(|c| (c, command_param_keys(c))).collect(); + let mut keys: std::collections::BTreeSet = + catalog.iter().flat_map(|(_, keys)| keys.iter().filter(|k| reads_like_a_location(k)).cloned()).collect(); + keys.extend(LOCATION_KEYS.iter().map(|(k, _)| k.to_string()).chain(NOT_LOCATIONS.iter().map(|k| k.to_string()))); + let mut wrong = vec![]; + for k in &keys { + let want = LOCATION_KEYS.iter().find(|(l, _)| l == k).map(|(_, l)| *l); + if want.is_none() && !NOT_LOCATIONS.contains(&k.as_str()) { + wrong.push(format!("`{k}` is in neither LOCATION_KEYS nor NOT_LOCATIONS")); + continue; + } + if key_location(k) != want { + wrong.push(format!("key_location(`{k}`) is {:?}, not {want:?}", key_location(k))); + } + if vectorcraft_engine::file_access::is_path_key(k) != want.is_some() { + wrong.push(format!("is_path_key(`{k}`) is {}", want.is_none())); + } + for (c, named) in &catalog { + if !named.contains(k) && !(c.params.contains('…') && takes_key(c.params, k)) { + continue; + } + let here = if DOCUMENT_PARTS.contains(&(c.id, k.as_str())) { None } else { want }; + if location_in(c, k) != here { + wrong.push(format!("{}: location_in(`{k}`) is {:?}, not {here:?}", c.id, location_in(c, k))); + } + } + } + for (id, key) in DOCUMENT_PARTS { + let named = catalog.iter().any(|(c, _)| c.id == *id && c.params.contains(key)); + assert!(named, "{id} doesn't name `{key}`"); + } + assert!(wrong.is_empty(), "{} keys are classified unlike the reviewed lists:\n{}", wrong.len(), wrong.join("\n")); +} + /// The cases sweep keys the command catalog names as files and folders: `document.open`'s /// `{path | name …}`, `addon.document.saveTimestampedCopy`'s `{folder? (…)}`, `document.compare`'s /// `diffPath`, `file.saveAs`'s `path` after `{…as document.save}` and `links.relink`'s diff --git a/crates/testkit/src/strategies.rs b/crates/testkit/src/strategies.rs index 153ede8f02..ac1c380cc3 100644 --- a/crates/testkit/src/strategies.rs +++ b/crates/testkit/src/strategies.rs @@ -2,7 +2,7 @@ use proptest::prelude::*; use serde_json::{Value, json}; -use vectorcraft_engine::cmd::{Location, key_location, location_in, location_keys}; +use vectorcraft_engine::cmd::{Location, ParamsPlace, key_location, location_keys}; use vectorcraft_engine::{CommandSpec, Session, command_specs}; use vectorcraft_geom::{Anchor, PathData, Point, Rect, SubPath}; @@ -518,20 +518,8 @@ pub fn command_param_keys(spec: &CommandSpec) -> Vec { keys } -/// Parameter-name keys that name a file or folder in some command ([`key_location`]): fuzzing these -/// with arbitrary strings could write files anywhere, so callers substitute a temp path. -pub fn is_path_key(k: &str) -> bool { - key_location(k).is_some() -} - -/// Path keys that name a folder a command reads, creates or writes files into ([`key_location`]): -/// `folder`, `folders`, `dir`, `directory`, `destinationFolder` and `sourceFolder`. -pub fn is_folder_key(k: &str) -> bool { - key_location(k) == Some(Location::Folder) -} - -/// The location to send under the path key `k`: `safe_path` for a file, a folder next to it (in -/// the same temp folder) for a folder key. +/// The location to send under the path key `k` ([`key_location`]): `safe_path` for a file, a folder +/// next to it (in the same temp folder) for a folder key. pub fn safe_location(k: &str, safe_path: &str) -> String { location(key_location(k).unwrap_or(Location::File), safe_path) } @@ -542,55 +530,74 @@ fn location(l: Location, safe_path: &str) -> String { if l == Location::Folder { format!("{safe_path}-folder") } else { safe_path.to_string() } } -/// `v` with every string in it that can name a place on disk sent to the temp location: `v` -/// itself and the members of arrays in it, at any depth, when `kind` is `Some`, and in an object -/// at any depth, the strings under a path key ([`is_path_key`]). Other values keep their junk -/// shape. -fn redirect(kind: Option, v: &Value, safe_path: &str) -> Value { +/// `v`, found at `place` in a command's params, with every string in it that can name a place on +/// disk sent to the temp location: `v` itself and the members of arrays in it, at any depth, when +/// `kind` is `Some`, and in an object at any depth, the strings under a key that names a file or a +/// folder there ([`ParamsPlace::member`]: a call or a recorded step inside `v` keeps its command's +/// meaning). Other values keep their junk shape. +fn redirect(place: ParamsPlace, kind: Option, v: &Value, safe_path: &str) -> Value { match v { Value::String(_) => kind.map_or_else(|| v.clone(), |l| json!(location(l, safe_path))), - Value::Array(a) => Value::Array(a.iter().map(|x| redirect(kind, x, safe_path)).collect()), - Value::Object(o) => Value::Object(o.iter().map(|(k, x)| (k.clone(), redirect(key_location(k), x, safe_path))).collect()), + Value::Array(a) => { + let step = ParamsPlace::step(a); + Value::Array(a.iter().enumerate().map(|(i, x)| redirect(place.item(i, step), kind, x, safe_path)).collect()) + } + Value::Object(o) => { + let carried = ParamsPlace::carried(o); + let member = |k: &String, x: &Value| { + let (below, l) = place.member(k, carried); + (k.clone(), redirect(below, l, x, safe_path)) + }; + Value::Object(o.iter().map(|(k, x)| member(k, x)).collect()) + } _ => v.clone(), } } -/// `v`, with its strings sent to the temp location when `k` is a path key in any command -/// ([`is_path_key`]), and the strings under path keys in objects inside it too. -pub fn safe_value(k: &str, v: &Value, safe_path: &str) -> Value { - redirect(key_location(k), v, safe_path) +/// `v`, sent under key `k` at `place`, as [`redirect`] makes it. +fn redirect_under(place: ParamsPlace, k: &str, v: &Value, safe_path: &str) -> Value { + let (below, kind) = place.member(k, None); + redirect(below, kind, v, safe_path) } -/// `v` as [`safe_value`] makes it for command `spec` ([`location_in`]): a key that names part of -/// the document in it keeps its strings. +/// `v`, with its strings sent to the temp location when `k` is a path key in a command that gives it +/// no other meaning ([`key_location`]), and the strings under path keys in objects inside it too. +pub fn safe_value(k: &str, v: &Value, safe_path: &str) -> Value { + redirect_under(ParamsPlace::top(None), k, v, safe_path) +} + +/// `v` as [`safe_value`] makes it for command `spec` ([`vectorcraft_engine::cmd::location_in`]): a key that names part of +/// the document in it keeps its strings, at the top level and below. pub fn safe_value_for_command(spec: &CommandSpec, k: &str, v: &Value, safe_path: &str) -> Value { - redirect(location_in(spec, k), v, safe_path) + redirect_under(ParamsPlace::top(Some(spec)), k, v, safe_path) } /// A deterministic list of junk param objects for `doc` (the command's params doc): each /// documented key set to each junk value, all keys junk at once, wrong top-level types. A string -/// under a key that names a file or folder in any command ([`is_path_key`]) is a temp location. +/// under a key that names a file or folder in a command that gives it no other meaning +/// ([`key_location`]) is a temp location. pub fn junk_params(doc: &str, safe_path: &str) -> Vec { - junk_params_with(¶m_keys(doc), |k, v| safe_value(k, v, safe_path), key_location, safe_path) + junk_params_with(¶m_keys(doc), ParamsPlace::top(None), safe_path) } /// [`junk_params`] for command `spec`, with the keys of its schema and its file and folder keys too /// ([`command_param_keys`]), and a string under each key that names a file or folder in it -/// ([`location_in`]) a temp location, as are the strings in an array of them and under the path +/// ([`vectorcraft_engine::cmd::location_in`]) a temp location, as are the strings in an array of them and under the path /// keys of objects in one (`paths: [{ids, path}]`), which it also sends. pub fn junk_params_for_command(spec: &CommandSpec, safe_path: &str) -> Vec { - junk_params_with(&command_param_keys(spec), |k, v| safe_value_for_command(spec, k, v, safe_path), |k| location_in(spec, k), safe_path) + junk_params_with(&command_param_keys(spec), ParamsPlace::top(Some(spec)), safe_path) } -/// The junk cases for `keys`: `safe(k, v)` is what is sent for junk value `v` under `k`, and -/// `kind(k)` what `k` names on disk, if anything, whose keys also get a list of locations and a -/// list of objects with a `path`. -fn junk_params_with(keys: &[String], safe: impl Fn(&str, &Value) -> Value, kind: impl Fn(&str) -> Option, safe_path: &str) -> Vec { +/// The junk cases for `keys` at `place`, the top of a command's params: what is sent for a junk value +/// under a key is [`redirect_under`] it, and a key that names a file or a folder there also gets a +/// list of locations and a list of objects with a `path`. +fn junk_params_with(keys: &[String], place: ParamsPlace, safe_path: &str) -> Vec { let junk = junk_values(); let mut out = vec![Value::Null, json!([]), json!("str"), json!(42), json!({"unknownKey": 1})]; for k in keys { + let (below, kind) = place.member(k, None); for (i, v) in junk.iter().enumerate() { - let v = safe(k, v); + let v = redirect(below, kind, v, safe_path); // Sparse: one key at a time with every junk value, plus a few combos. let mut o = serde_json::Map::new(); o.insert(k.clone(), v.clone()); @@ -598,17 +605,56 @@ fn junk_params_with(keys: &[String], safe: impl Fn(&str, &Value) -> Value, kind: if i % 5 == 0 { let mut all = serde_json::Map::new(); for k2 in keys { - let v2 = kind(k2).map_or_else(|| v.clone(), |l| json!(location(l, safe_path))); + let v2 = place.member(k2, None).1.map_or_else(|| v.clone(), |l| json!(location(l, safe_path))); all.insert(k2.clone(), v2); } out.push(Value::Object(all)); } } - if let Some(l) = kind(k) { + if let Some(l) = kind { for v in [json!(["junk", "", 1]), json!([{"ids": [1], "path": "junk"}, {"path": null}])] { - out.push(json!({ k.as_str(): redirect(Some(l), &v, safe_path) })); + out.push(json!({ k.as_str(): redirect(below, Some(l), &v, safe_path) })); } } } out } + +#[cfg(test)] +mod tests { + use super::*; + + /// The strings the sweeps send keep the meaning the command gives their key, at every depth: an + /// export's folder grouping and a web link stay as sent, also inside a `command.batch` step and + /// a recorded step, and a real file or folder key there goes to the temp location. + #[test] + fn nested_locations_keep_command_meaning() { + let spec = |id: &str| command_specs().iter().find(|c| c.id == id).unwrap_or_else(|| panic!("no command {id}")); + let safe = "/tmp/sweep/out.bin"; + let settings = json!({"folders": "rows", "formats": [{"format": "png", "folder": "junk"}]}); + assert_eq!( + safe_value_for_command(spec("document.exportPresets.save"), "settings", &settings, safe), + json!({"folders": "rows", "formats": [{"format": "png", "folder": "/tmp/sweep/out.bin-folder"}]}) + ); + let steps = json!([ + {"command": "attributes.set", "params": {"url": "https://example.com/a"}}, + {"command": "document.save", "params": {"path": "junk"}}, + {"command": "object.path.simplify", "params": {"wholePaths": "junk"}}, + ]); + assert_eq!( + safe_value_for_command(spec("command.batch"), "commands", &steps, safe), + json!([ + {"command": "attributes.set", "params": {"url": "https://example.com/a"}}, + {"command": "document.save", "params": {"path": safe}}, + {"command": "object.path.simplify", "params": {"wholePaths": "junk"}}, + ]) + ); + let recorded = json!([["object.setLiveShape", {"path": "junk"}], ["document.compare", {"diffPath": "junk"}]]); + assert_eq!( + safe_value("steps", &recorded, safe), + json!([["object.setLiveShape", {"path": "junk"}], ["document.compare", {"diffPath": safe}]]) + ); + assert_eq!(safe_value("url", &json!("https://example.com/a"), safe), json!("https://example.com/a")); + assert_eq!(safe_value("destinationFolder", &json!("junk"), safe), json!("/tmp/sweep/out.bin-folder")); + } +} diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 89ab7e37b4..2032995b11 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -41,6 +41,7 @@ use std::sync::atomic::{AtomicU8, AtomicU64, Ordering}; use std::sync::mpsc::Sender; use serde_json::{Value, json}; +use vectorcraft_engine::cmd::ParamsPlace; use vectorcraft_render::encode::{RasterExportOptions, RasterFormat}; use vectorcraft_tools::{Mods, PointerEvent, PointerKind}; @@ -424,17 +425,19 @@ impl Hiding { } /// The paths outside the read roots that `v`'s strings and keys name at any depth, written as paths -/// ([`unreadable_path_like`]) or held by a path param -/// ([`vectorcraft_engine::file_access::is_path_key`]), a relative one too, each as a hidden library id -/// (`loaded/`) with `loaded/` shown. A reply hides them as it hides a hidden library's -/// path, with that file name also for a hidden library shown as `loaded/ (2)`, so that it -/// reads the same whether or not a library was loaded from one of them. +/// ([`unreadable_path_like`]) or held by a path param, a relative one too, each as a hidden library id +/// (`loaded/`) with `loaded/` shown. A param is a path param as the command it is +/// for reads it ([`ParamsPlace`]: `engine.execute`'s `params`, a `command.batch` step's and a +/// recorded step's), so a web link or an export's folder grouping is not one. A reply hides them as +/// it hides a hidden library's path, with that file name also for a hidden library shown as +/// `loaded/ (2)`, so that it reads the same whether or not a library was loaded from one +/// of them. fn sent_paths(v: &Value, out: &mut Vec<(String, String)>) { - collect_sent_paths(v, false, out); + collect_sent_paths(v, ParamsPlace::top(None), false, out); } -/// [`sent_paths`] on `v`, which a path param holds when `path_param` is set. -fn collect_sent_paths(v: &Value, path_param: bool, out: &mut Vec<(String, String)>) { +/// [`sent_paths`] on `v`, found at `place`, which a path param holds when `path_param` is set. +fn collect_sent_paths(v: &Value, place: ParamsPlace, path_param: bool, out: &mut Vec<(String, String)>) { fn add(s: &str, path_param: bool, out: &mut Vec<(String, String)>) { if !unreadable_path_like(s) && !(path_param && !readable_value(s)) { return; @@ -456,11 +459,18 @@ fn collect_sent_paths(v: &Value, path_param: bool, out: &mut Vec<(String, String } match v { Value::String(s) => add(s, path_param, out), - Value::Array(items) => items.iter().for_each(|item| collect_sent_paths(item, path_param, out)), + Value::Array(items) => { + let step = ParamsPlace::step(items); + for (i, item) in items.iter().enumerate() { + collect_sent_paths(item, place.item(i, step), path_param, out); + } + } Value::Object(fields) => { + let carried = ParamsPlace::carried(fields); for (key, item) in fields { add(key, false, out); - collect_sent_paths(item, vectorcraft_engine::file_access::is_path_key(key), out); + let (below, location) = place.member(key, carried); + collect_sent_paths(item, below, location.is_some(), out); } } _ => {} @@ -1045,45 +1055,55 @@ fn percent_decoded(s: &str) -> String { String::from_utf8_lossy(&out).into_owned() } -/// The params of each step of each action of action set `set` (`ui.action_sets`). -fn steps_mut(set: &mut Value) -> impl Iterator> { +/// The params of each step of each action of action set `set` (`ui.action_sets`), each with the top +/// of its command's params ([`ParamsPlace::of`] the step's command id). +fn steps_mut(set: &mut Value) -> impl Iterator)> { let actions = set.get_mut("actions").and_then(Value::as_array_mut).into_iter().flatten(); let steps = actions.filter_map(|action| action.get_mut("steps").and_then(Value::as_array_mut)).flatten(); - steps.filter_map(|step| step.get_mut(1).and_then(Value::as_object_mut)) + steps.filter_map(|step| { + let place = ParamsPlace::of(step.get(0).and_then(Value::as_str).unwrap_or_default()); + step.get_mut(1).and_then(Value::as_object_mut).map(|params| (place, params)) + }) } -/// Takes out of a recorded step's params, at any depth (the commands of a `command.batch` step), the -/// paths the request may not read: the value of a path param -/// ([`vectorcraft_engine::file_access::is_path_key`]: `path`, `file`, `diffPath`, `destinationFolder` -/// and the like), each path of a list under such a param, and under any other param, as a key too, a -/// string written as a path ([`unreadable_path_like`]). -fn leave_out_unreadable_params(params: &mut serde_json::Map) { - use vectorcraft_engine::file_access::is_path_key; +/// Takes out of a recorded step's params at `place`, at any depth (the commands of a `command.batch` +/// step), the paths the request may not read: the value of a path param as the command reads it +/// ([`ParamsPlace`]: `path`, `file`, `diffPath`, `destinationFolder` and the like, but not a key that +/// names part of the document in that command), each path of a list under such a param, and under +/// any other param, as a key too, a string written as a path ([`unreadable_path_like`]). A web link, +/// a choice or a switch whose name ends like a path param's (`alignToPath`, `linksFolder`) stays. +fn leave_out_unreadable_params(params: &mut serde_json::Map, place: ParamsPlace) { + let carried = ParamsPlace::carried(params); params.retain(|key, v| { !unreadable_path_like(key) && match v.as_str() { - Some(p) => !unreadable_path_like(p) && !(is_path_key(key) && !readable_value(p)), + Some(p) => !unreadable_path_like(p) && !(place.member(key, carried).1.is_some() && !readable_value(p)), None => true, } }); + let carried = ParamsPlace::carried(params); for (key, v) in params.iter_mut() { + let (below, location) = place.member(key, carried); if let Value::Array(paths) = v - && is_path_key(key) + && location.is_some() { paths.retain(|p| p.as_str().is_none_or(readable_value)); } - leave_out_unreadable_values(v); + leave_out_unreadable_values(v, below); } } -/// [`leave_out_unreadable_params`] on each object `v` holds, and a string written as a path that the -/// request may not read ([`unreadable_path_like`]) taken out of each list. -fn leave_out_unreadable_values(v: &mut Value) { +/// [`leave_out_unreadable_params`] on each object `v`, found at `place`, holds, and a string written +/// as a path that the request may not read ([`unreadable_path_like`]) taken out of each list. +fn leave_out_unreadable_values(v: &mut Value, place: ParamsPlace) { match v { - Value::Object(fields) => leave_out_unreadable_params(fields), + Value::Object(fields) => leave_out_unreadable_params(fields, place), Value::Array(items) => { items.retain(|item| !item.as_str().is_some_and(unreadable_path_like)); - items.iter_mut().for_each(leave_out_unreadable_values); + let step = ParamsPlace::step(items); + for (i, item) in items.iter_mut().enumerate() { + leave_out_unreadable_values(item, place.item(i, step)); + } } _ => {} } @@ -1103,8 +1123,8 @@ fn leave_out_unreadable_paths(ui: &mut serde_json::Map) { } } } - for step in ui.get_mut("action_sets").and_then(Value::as_array_mut).into_iter().flatten().flat_map(steps_mut) { - leave_out_unreadable_params(step); + for (place, step) in ui.get_mut("action_sets").and_then(Value::as_array_mut).into_iter().flatten().flat_map(steps_mut) { + leave_out_unreadable_params(step, place); } if let Some(Value::Object(prefs)) = ui.get_mut("engine_prefs") { leave_out_unreadable_folders(prefs); @@ -1231,7 +1251,7 @@ fn hide_in_reply(app: &VectorcraftApp, reply: &mut Value, sent: &Value, before: return; } let mut given = Vec::new(); - written_paths(sent, false, &mut given); + written_paths(sent, ParamsPlace::top(None), false, &mut given); let kept = kept_written_paths(reply, &given); let mut hiding = unreadable_library_ids(app); hiding.merge(before); @@ -1242,16 +1262,24 @@ fn hide_in_reply(app: &VectorcraftApp, reply: &mut Value, sent: &Value, before: restore_written_paths(reply, kept); } -/// The strings that `v`'s path params hold at any depth ([`vectorcraft_engine::file_access::is_path_key`]) -/// that name a file inside the write roots; `v` is a path param's value when `path_param` is set. -fn written_paths(v: &Value, path_param: bool, out: &mut Vec) { - use vectorcraft_engine::file_access::{Access, current, is_path_key}; +/// The strings that `v`'s path params hold at any depth, as the command they are for reads them +/// ([`ParamsPlace`]), that name a file inside the write roots; `v`, found at `place`, is a path +/// param's value when `path_param` is set. +fn written_paths(v: &Value, place: ParamsPlace, path_param: bool, out: &mut Vec) { + use vectorcraft_engine::file_access::{Access, current}; match v { Value::String(s) if path_param && !s.is_empty() && current().is_some_and(|r| r.check(s, Access::Write).is_ok()) => out.push(s.clone()), - Value::Array(items) => items.iter().for_each(|item| written_paths(item, path_param, out)), + Value::Array(items) => { + let step = ParamsPlace::step(items); + for (i, item) in items.iter().enumerate() { + written_paths(item, place.item(i, step), path_param, out); + } + } Value::Object(fields) => { + let carried = ParamsPlace::carried(fields); for (key, item) in fields { - written_paths(item, is_path_key(key), out); + let (below, location) = place.member(key, carried); + written_paths(item, below, location.is_some(), out); } } _ => {} diff --git a/crates/ui-egui/src/control/tests.rs b/crates/ui-egui/src/control/tests.rs index ac8fbfa3eb..ec412ce413 100644 --- a/crates/ui-egui/src/control/tests.rs +++ b/crates/ui-egui/src/control/tests.rs @@ -301,10 +301,71 @@ fn a_url_naming_another_host_stays_without_a_read_root() { assert!(super::readable_value("file://host/share/a.svg")); let mut params = json!({"path": "file://host/share/a.svg", "list": ["file://host/share/b.svg"]}); let fields = params.as_object_mut().unwrap(); - super::leave_out_unreadable_params(fields); + super::leave_out_unreadable_params(fields, vectorcraft_engine::cmd::ParamsPlace::top(None)); assert_eq!(params, json!({"path": "file://host/share/a.svg", "list": ["file://host/share/b.svg"]})); } +/// Under a read root, a recorded step loses a relative path outside it under a key that names a file +/// or a folder as its command reads it (also inside a `command.batch` step), and a `file:` URL outside +/// it under any key. A web link, a choice or a switch whose name ends like a path key's, and a key +/// that names part of the document in its command, keep their values. +#[test] +fn recorded_steps_filter_only_unreadable_locations() { + use vectorcraft_engine::cmd::ParamsPlace; + let root = std::env::temp_dir().join(format!("vc-recorded-locations-{}", std::process::id())); + std::fs::create_dir_all(&root).unwrap(); + let roots = vectorcraft_engine::file_access::AutomationRoots::new(&[&root], &[]).unwrap(); + let steps = [ + ("document.compare", json!({"diffPath": "elsewhere/a.svg", "directory": "elsewhere", "keep": 1})), + ("attributes.set", json!({"url": "https://example.com/elsewhere/a.html", "note": "kept"})), + ("object.slice.options", json!({"url": "file:///elsewhere/a.html"})), + ("object.path.simplify", json!({"wholePaths": "elsewhere/a", "alignToPath": "elsewhere/b", "linksFolder": "elsewhere/c"})), + ("document.exportPresets.save", json!({"settings": {"folders": "item/format", "formats": [{"folder": "elsewhere/png"}]}})), + ("object.setLiveShape", json!({"path": "elsewhere/shape"})), + ("command.batch", json!({"commands": [{"command": "document.save", "params": {"path": "elsewhere/b.svg"}}]})), + ]; + let filtered: Vec = vectorcraft_engine::file_access::confine(roots.as_ref(), || { + steps + .iter() + .map(|(id, params)| { + let mut params = params.clone(); + super::leave_out_unreadable_params(params.as_object_mut().unwrap(), ParamsPlace::of(id)); + params + }) + .collect() + }); + let _ = std::fs::remove_dir_all(root); + let want = [ + json!({"keep": 1}), + json!({"url": "https://example.com/elsewhere/a.html", "note": "kept"}), + json!({}), + json!({"wholePaths": "elsewhere/a", "alignToPath": "elsewhere/b", "linksFolder": "elsewhere/c"}), + json!({"settings": {"folders": "item/format", "formats": [{}]}}), + json!({"path": "elsewhere/shape"}), + json!({"commands": [{"command": "document.save", "params": {}}]}), + ]; + assert_eq!(filtered, want); +} + +/// A request's relative path outside the read root is hidden in a reply where the command it calls +/// reads it as a file, and a web link or a key that names part of the document there is not. +#[test] +fn a_request_sends_paths_as_its_command_reads_them() { + let root = std::env::temp_dir().join(format!("vc-sent-command-paths-{}", std::process::id())); + std::fs::create_dir_all(&root).unwrap(); + let roots = vectorcraft_engine::file_access::AutomationRoots::new(&[&root], &[]).unwrap(); + let mut sent = Vec::new(); + vectorcraft_engine::file_access::confine(roots.as_ref(), || { + super::sent_paths(&json!({"command": "attributes.set", "params": {"url": "https://example.com/a/page.html"}}), &mut sent); + super::sent_paths(&json!({"command": "object.path.simplify", "params": {"wholePaths": "out/w.svg"}}), &mut sent); + super::sent_paths(&json!({"command": "document.compare", "params": {"diffPath": "out/d.svg"}}), &mut sent); + super::sent_paths(&json!({"command": "file.place", "params": {"directory": "out/folder"}}), &mut sent); + }); + let _ = std::fs::remove_dir_all(root); + let pair = |path: &str, name: &str| (format!("loaded/{path}"), format!("loaded/{name}")); + assert_eq!(sent, [pair("out/d.svg", "d.svg"), pair("out/folder", "folder")]); +} + /// A hidden path quoted with `{:?}` is hidden with its quotes and control characters escaped. #[test] fn a_reply_hides_a_quoted_path_with_escaped_quotes() {