Merge pull request #6005 from dotCooCoo/fix/prefs-set-folder-check

Preferences: prefs.set refuses a missing or unusable recovery or plug-ins folder

crates/engine/src/cmd/prefscmds.rs

@@ -520,6 +520,50 @@ fn in_range(key: &str, x: Option<f64>, min: f64, max: f64) -> std::result::Resul
     }
 }
 
+/// A check of a folder path: `Err` says why the folder can't be the preference's value.
+pub type FolderCheck = fn(&str) -> std::result::Result<(), String>;
+
+/// The check a folder preference with a Choose… button gives a folder before it becomes the
+/// preference's value: Data Recovery keeps its copies in its folder, and the Additional Plug-ins
+/// Folder is read for plug-ins. `None` for any other preference.
+pub fn folder_check(key: &str) -> Option<FolderCheck> {
+    match key {
+        "recoveryFolder" => Some(writable_folder),
+        "pluginsFolder" => Some(plugins_folder),
+        _ => None,
+    }
+}
+
+/// `Ok` when `path` is a folder whose contents can be listed, as the plug-ins in it are found by
+/// listing it. A folder outside the automation roots in force is refused before it is looked at.
+fn plugins_folder(path: &str) -> std::result::Result<(), String> {
+    crate::file_access::check_read(path)?;
+    let dir = std::path::Path::new(path);
+    if !dir.is_dir() {
+        return Err(format!("{path} is not a folder"));
+    }
+    std::fs::read_dir(dir).map(drop).map_err(|e| format!("Plug-ins can't be loaded from {path}: {e}"))
+}
+
+/// `Ok` when `path` is a folder that can be listed and that a folder can be created in, as the
+/// recovery store lists its folders there and makes its own: the folder is listed, and an empty
+/// folder is made there and removed. A folder that can't be removed again is an error too. A
+/// folder outside the automation roots in force is refused before it is looked at.
+fn writable_folder(path: &str) -> std::result::Result<(), String> {
+    crate::file_access::check_write(path)?;
+    crate::file_access::check_read(path)?;
+    let dir = std::path::Path::new(path);
+    if !dir.is_dir() {
+        return Err(format!("{path} is not a folder"));
+    }
+    let refused = |e: std::io::Error| format!("Data Recovery can't keep copies in {path}: {e}");
+    std::fs::read_dir(dir).map_err(refused)?;
+    let nanos = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_or(0, |t| t.as_nanos());
+    let check = dir.join(format!(".vectorcraft-write-check-{}-{nanos}", std::process::id()));
+    std::fs::create_dir(&check).map_err(refused)?;
+    std::fs::remove_dir(&check).map_err(refused)
+}
+
 impl Prefs {
     pub fn to_json(&self) -> Value {
         serde_json::to_value(self).unwrap_or(Value::Null)
@@ -568,6 +612,25 @@ impl Prefs {
         refused
     }
 
+    /// The [`folder_check`] of each folder preference in `values`. An empty value (the default
+    /// folder) and a folder the preference already holds are not checked. The web has no folders to
+    /// check.
+    fn check_folders(&self, values: &Map<String, Value>) -> std::result::Result<(), String> {
+        if cfg!(target_arch = "wasm32") {
+            return Ok(());
+        }
+        let saved = self.to_json();
+        for (key, v) in values {
+            let (Some(check), Some(path)) = (folder_check(key), v.as_str()) else { continue };
+            let path = path.trim();
+            if path.is_empty() || saved[key.as_str()].as_str().is_some_and(|s| s.trim() == path) {
+                continue;
+            }
+            check(path)?;
+        }
+        Ok(())
+    }
+
     /// Values of one category (`None` = all) reset to defaults.
     pub fn reset(&mut self, category: Option<&str>) {
         let d = Prefs::default().to_json();
@@ -669,7 +732,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Set Preference",
             [],
             None,
-            "{key, value} or {values: {key: value, …}} set preferences (validated; see prefs.list; lengths in pt or strings with a unit; unitsGeneral also sets the active document's units, one undo step; the eyedropper group takes a partial object, as eyedropper.setOptions)",
+            "{key, value} or {values: {key: value, …}} set preferences (validated; see prefs.list; lengths in pt or strings with a unit; unitsGeneral also sets the active document's units, one undo step; the eyedropper group takes a partial object, as eyedropper.setOptions; a new recoveryFolder must be a folder that can be listed and written in, and a new pluginsFolder a folder that can be listed: otherwise an error, and nothing changes; an empty value is not checked)",
             always,
             set
         ),
@@ -700,6 +763,7 @@ fn set(s: &mut Session, p: &Value) -> Result<Value> {
     if let Some(e) = s.prefs.keep_folders(&mut next).into_iter().next() {
         return Err(EngineError::Other(e));
     }
+    s.prefs.check_folders(&values).map_err(EngineError::Other)?;
     s.apply_prefs(next);
     // Units ▸ General is also the open document's units (as Document Setup sets them).
     if values.contains_key("unitsGeneral") && s.active().is_some() {

crates/engine/src/tests/prefs.rs

@@ -173,6 +173,98 @@ fn reset_category_and_all() {
     assert!(s.execute("prefs.reset", &json!({"category": "Bogus"})).is_err());
 }
 
+/// `prefs.set` refuses a recovery or plug-ins folder that is missing or is a file. Nothing changes,
+/// other values in the same call included, and the document gets no undo step.
+#[test]
+fn set_refuses_a_missing_folder_or_a_file_as_a_recovery_or_plugins_folder() {
+    let folder = crate::tests_links::Folder::new("prefs-set-refused");
+    let file = folder.file("not-a-folder.txt");
+    std::fs::write(&file, b"x").unwrap();
+    let mut s = Session::new();
+    s.execute("file.new", &json!({"width": 100, "height": 100})).unwrap();
+    let before = s.prefs.clone();
+    let undo = s.doc().unwrap().history.undo.len();
+    for key in ["recoveryFolder", "pluginsFolder"] {
+        for path in [file.clone(), folder.file("missing")] {
+            let e = s.execute("prefs.set", &json!({"key": key, "value": path})).unwrap_err();
+            assert_eq!(e.to_string(), format!("{path} is not a folder"), "{key}");
+            let e = s.execute("prefs.set", &json!({"values": {key: path, "keyboardIncrement": 7, "unitsGeneral": "millimeters"}})).unwrap_err();
+            assert_eq!(e.to_string(), format!("{path} is not a folder"), "{key}");
+            assert_eq!(s.prefs, before, "{key}: no preference changes");
+            assert_eq!(s.doc().unwrap().history.undo.len(), undo, "{key}: no undo step");
+        }
+    }
+}
+
+/// `prefs.set` refuses a recovery folder that no folder can be created in or that can't be listed,
+/// and a plug-ins folder that can't be listed.
+#[cfg(unix)]
+#[test]
+fn set_refuses_a_recovery_or_plugins_folder_it_cannot_use() {
+    use std::os::unix::fs::PermissionsExt;
+    /// Gives a folder mode 0o755 again when dropped, also when an assertion fails, so the test
+    /// folder can be removed.
+    struct Unlock(std::path::PathBuf);
+    impl Drop for Unlock {
+        fn drop(&mut self) {
+            let _ = std::fs::set_permissions(&self.0, std::fs::Permissions::from_mode(0o755));
+        }
+    }
+    let folder = crate::tests_links::Folder::new("prefs-set-unusable");
+    let mut s = Session::new();
+    let before = s.prefs.clone();
+    let locked = folder.0.join("locked");
+    std::fs::create_dir(&locked).unwrap();
+    let unlock_locked = Unlock(locked.clone());
+    std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o555)).unwrap();
+    // An account that may write anyway (root) has nothing to refuse.
+    if std::fs::create_dir(locked.join("probe")).is_err() {
+        let e = s.execute("prefs.set", &json!({"key": "recoveryFolder", "value": locked.to_string_lossy()})).unwrap_err();
+        assert!(e.to_string().starts_with("Data Recovery can't keep copies in "), "{e}");
+    }
+    drop(unlock_locked);
+    let unlisted = folder.0.join("unlisted");
+    std::fs::create_dir(&unlisted).unwrap();
+    let unlock_unlisted = Unlock(unlisted.clone());
+    std::fs::set_permissions(&unlisted, std::fs::Permissions::from_mode(0o333)).unwrap();
+    // An account that may list anyway (root) has nothing to refuse.
+    if std::fs::read_dir(&unlisted).is_err() {
+        let e = s.execute("prefs.set", &json!({"key": "recoveryFolder", "value": unlisted.to_string_lossy()})).unwrap_err();
+        assert!(e.to_string().starts_with("Data Recovery can't keep copies in "), "{e}");
+        let e = s.execute("prefs.set", &json!({"key": "pluginsFolder", "value": unlisted.to_string_lossy()})).unwrap_err();
+        assert!(e.to_string().starts_with("Plug-ins can't be loaded from "), "{e}");
+    }
+    drop(unlock_unlisted);
+    assert_eq!(s.prefs, before);
+}
+
+/// `prefs.set` saves a folder that passes as given and leaves nothing behind in it. An empty value
+/// and a folder the preference already holds are not checked, nor is a held folder when the call
+/// leaves it out, so they work while the folder is unavailable.
+#[test]
+fn set_takes_a_usable_empty_or_already_held_recovery_folder() {
+    let folder = crate::tests_links::Folder::new("prefs-set-usable");
+    let mut s = Session::new();
+    let typed = format!(" {} ", folder.0.display());
+    s.execute("prefs.set", &json!({"key": "recoveryFolder", "value": typed})).unwrap();
+    assert_eq!(s.prefs.recovery_folder, typed);
+    assert_eq!(std::fs::read_dir(&folder.0).unwrap().count(), 0, "the write check leaves nothing behind");
+
+    let gone = folder.0.join("gone");
+    std::fs::create_dir(&gone).unwrap();
+    let gone = gone.to_string_lossy().into_owned();
+    s.execute("prefs.set", &json!({"key": "recoveryFolder", "value": gone})).unwrap();
+    std::fs::remove_dir(&gone).unwrap();
+    s.execute("prefs.set", &json!({"values": {"recoveryFolder": gone, "keyboardIncrement": 7}})).unwrap();
+    s.execute("prefs.set", &json!({"key": "keyboardIncrement", "value": 3})).unwrap();
+    assert_eq!((s.prefs.recovery_folder.as_str(), s.prefs.keyboard_increment), (gone.as_str(), 3.0));
+
+    for key in ["recoveryFolder", "pluginsFolder"] {
+        s.execute("prefs.set", &json!({"key": key, "value": ""})).unwrap();
+        assert_eq!(s.prefs.to_json()[key], json!(""), "{key}");
+    }
+}
+
 #[test]
 fn list_describes_every_pref() {
     let mut s = Session::new();

crates/ui-egui/src/prefs_dialog.rs

@@ -6,7 +6,7 @@
 
 use serde_json::{Map, Value, json};
 use vectorcraft_engine::Prefs;
-use vectorcraft_engine::cmd::prefscmds::{GPU_PREFERENCES, PREF_CATEGORIES, PREF_GROUPS, PREF_SPECS, PrefKind};
+use vectorcraft_engine::cmd::prefscmds::{GPU_PREFERENCES, PREF_CATEGORIES, PREF_GROUPS, PREF_SPECS, PrefKind, folder_check};
 
 use crate::state::Dialog;
 use crate::theme::{self, Accent, Brightness, Scheme, Tokens};
@@ -40,7 +40,6 @@ pub fn confirm(app: &mut VectorcraftApp) -> Result<Value, String> {
     let Some(d) = app.ui.dialog.clone() else { return Err("no dialog open".into()) };
     let values: Map<String, Value> = d.fields.iter().filter(|(k, _)| !k.starts_with("__")).map(|(k, v)| (k.clone(), v.clone())).collect();
     let touch_ws = values.get("touchWorkspace").and_then(Value::as_bool);
-    check_typed_folders(app, &values)?;
     app.run("prefs.set", json!({ "values": values }))?;
     if touch_ws.is_some() {
         crate::workspaces::sync_touch_workspace(app, app.touch_screen);
@@ -413,20 +412,6 @@ fn category_fields(ui: &mut egui::Ui, app: &mut VectorcraftApp, d: &mut Dialog,
     }
 }
 
-/// A check of a folder path: `Err` says why the folder can't be the preference's value.
-type FolderCheck = fn(&str) -> Result<(), String>;
-
-/// The check a folder preference with a Choose… button gives a folder before it becomes the
-/// preference's value: Data Recovery keeps its copies in its folder, and the Additional Plug-ins
-/// Folder is read for plug-ins. `None` for any other preference.
-fn folder_check(key: &str) -> Option<FolderCheck> {
-    match key {
-        "recoveryFolder" => Some(writable_folder),
-        "pluginsFolder" => Some(plugins_folder),
-        _ => None,
-    }
-}
-
 /// Choose… beside the folder preference `key`: the folder picked becomes the field's value when it
 /// passes the preference's [`folder_check`]. Otherwise the status bar says why and the field keeps
 /// its value, as it does when the picker is canceled. The answer fills the field even when the
@@ -444,55 +429,6 @@ fn choose_folder(app: &mut VectorcraftApp, d: &mut Dialog, key: &'static str) {
     }
 }
 
-/// OK's check of the folder preferences with a Choose… button: a folder typed into one of their
-/// fields gets the same [`folder_check`] as a picked one. An empty field (the default folder) and
-/// a folder the preference already holds are not checked. The web has no folders to check.
-fn check_typed_folders(app: &VectorcraftApp, fields: &Map<String, Value>) -> Result<(), String> {
-    if cfg!(target_arch = "wasm32") {
-        return Ok(());
-    }
-    let saved = app.session.prefs.to_json();
-    for (key, v) in fields {
-        let (Some(check), Some(path)) = (folder_check(key), v.as_str()) else { continue };
-        let path = path.trim();
-        if path.is_empty() || saved[key.as_str()].as_str().is_some_and(|s| s.trim() == path) {
-            continue;
-        }
-        check(path)?;
-    }
-    Ok(())
-}
-
-/// `Ok` when `path` is a folder whose contents can be listed, as the plug-ins in it are found by
-/// listing it. A folder outside the automation roots in force is refused before it is looked at.
-fn plugins_folder(path: &str) -> Result<(), String> {
-    vectorcraft_engine::file_access::check_read(path)?;
-    let dir = std::path::Path::new(path);
-    if !dir.is_dir() {
-        return Err(format!("{path} is not a folder"));
-    }
-    std::fs::read_dir(dir).map(drop).map_err(|e| format!("Plug-ins can't be loaded from {path}: {e}"))
-}
-
-/// `Ok` when `path` is a folder that can be listed and that a folder can be created in, as the
-/// recovery store lists its folders there and makes its own: the folder is listed, and an empty
-/// folder is made there and removed. A folder that can't be removed again is an error too. A
-/// folder outside the automation roots in force is refused before it is looked at.
-fn writable_folder(path: &str) -> Result<(), String> {
-    vectorcraft_engine::file_access::check_write(path)?;
-    vectorcraft_engine::file_access::check_read(path)?;
-    let dir = std::path::Path::new(path);
-    if !dir.is_dir() {
-        return Err(format!("{path} is not a folder"));
-    }
-    let refused = |e: std::io::Error| format!("Data Recovery can't keep copies in {path}: {e}");
-    std::fs::read_dir(dir).map_err(refused)?;
-    let nanos = std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).map_or(0, |t| t.as_nanos());
-    let check = dir.join(format!(".vectorcraft-write-check-{}-{nanos}", std::process::id()));
-    std::fs::create_dir(&check).map_err(refused)?;
-    std::fs::remove_dir(&check).map_err(refused)
-}
-
 /// Is one of the dialog's Units (General, Stroke, Type, East Asian Type) Picas?
 fn picas_in_use(d: &Dialog) -> bool {
     ["unitsGeneral", "unitsStroke", "unitsType", "unitsAsianType"].iter().any(|k| d.str(k) == vectorcraft_doc::Unit::Picas.key())

crates/ui-egui/src/prefs_dialog/tests.rs

@@ -615,6 +615,7 @@ fn ok_refuses_a_typed_folder_that_choose_would_refuse() {
             assert_eq!(confirm(&mut a), Err(format!("{path} is not a folder")), "{key}");
             assert!(a.ui.dialog.is_some(), "{key}: the dialog stays open");
             assert_eq!(a.session.prefs.to_json(), before, "{key}: no preference changes");
+            assert!(!a.prefs_changed, "{key}: nothing to save");
         }
         // A typed folder that passes is saved, with the spaces around it as typed.
         let typed = format!(" {} ", dir.to_string_lossy());
@@ -627,7 +628,9 @@ fn ok_refuses_a_typed_folder_that_choose_would_refuse() {
         confirm(&mut a).unwrap();
         assert_eq!(a.session.prefs.to_json()[key], json!(""), "{key}");
         // A folder already saved is not checked again, so OK works while it is unavailable.
+        std::fs::create_dir(&missing).unwrap();
         a.run("prefs.set", json!({"key": key, "value": missing})).unwrap();
+        std::fs::remove_dir(&missing).unwrap();
         open(&mut a, Some("File Handling"));
         a.ui.dialog.as_mut().unwrap().fields.insert("showToolTips".into(), json!(!a.session.prefs.show_tool_tips));
         confirm(&mut a).unwrap();

docs/mcp.md

@@ -4700,7 +4700,10 @@ Modified documents get recovery copies (`file.recovery.save`; the app runs it ev
 `autosaveRecovery` is on, skipping documents of more than 20,000 objects while `recoveryOffForComplex` is on). A copy
 goes when its document is saved, reverted or closed, so copies still there after the app quits were left by a crash.
 The copies live in the `recoveryFolder` preference's folder (default: `Data Recovery` beside the app's preferences;
-none when the app runs with `VECTORCRAFT_NO_PREFS`) or, on the web, in browser storage.
+none when the app runs with `VECTORCRAFT_NO_PREFS`) or, on the web, in browser storage. `prefs.set` returns an error
+and changes nothing when a new `recoveryFolder` is missing, is not a folder, can't be listed, or is a folder that a
+folder can't be created in. It does not check an empty value or the folder the preference already holds, and the web
+build does not check folders. The folder in the example below must exist before the call.
 
 Each running app (each browser tab) keeps its copies in an area of its own (`<area>/<name>`): a sub-folder whose
 `.lock` file it keeps locked while it runs, or on the web an area with a heartbeat it refreshes every minute and a Web
@@ -5910,7 +5913,9 @@ that no built-in format has. A failing plug-in returns an error and leaves the d
 no file). A filter whose manifest asks for a heavy budget (minutes rather than a minute, `budget: "heavy"` in its
 `plugin.inspect {path | dataBase64}` entry) installs only with `allowHeavy: true`: ask the user first. `plugin.run`
 runs it where it is called, so the call may take minutes; `notifications/cancelled` stops it.
-`plugin.install` rejects a `replace` or `allowHeavy` value that is not a boolean.
+`plugin.install` rejects a `replace` or `allowHeavy` value that is not a boolean. `prefs.set` returns an error and
+changes nothing when a new `pluginsFolder` is missing, is not a folder or can't be listed. It does not check an empty
+value or the folder the preference already holds, and the web build does not check folders.
 
 ```json
 {"name":"run_command","arguments":{"command":"plugin.install","params":{"path":"/plugins/desaturate.wasm"}}}

docs/plugins.md

@@ -52,7 +52,7 @@ background ([below](#heavy-filters)).
 **Preference folder.** Edit › Preferences › Plug-ins & Scratch Disks › *Additional Plug-ins Folder*: when set, every
 `*.wasm` in that folder (not recursive) is installed at startup and whenever the preference changes (desktop builds;
 the web build installs from bytes). On the desktop, Choose… beside the field picks the folder; the dialog refuses a
-picked or typed folder whose contents can't be listed.
+picked or typed folder whose contents can't be listed, and `prefs.set` refuses such a folder too.
 
 Plug-ins are installed **per process**, not per document. Documents store a live effect as an ordinary effect record
 (`{"id": "plugin.<id>", "params": {…}}`): while the plug-in is installed it is drawn, exported and expanded; without it

docs/target-app-parity.md

@@ -1195,6 +1195,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
     PDF export writes a spot color named All or None (in a Separation space, the names of every plate and of no plate: [ISO 32000-1:2008](https://opensource.adobe.com/dc-acrobat-sdk-docs/pdfstandards/PDF32000_2008.pdf), 8.6.6.4) as the ink All Spot or None Spot (All Spot 2 and on when a spot swatch already has that name) in its fills, strokes, gradients, colorized images and Lab alternates; Registration still prints on every plate (`pdf/src/tests/output.rs`).
   - `.vectorcraft` (lossless JSON, compressed, atomic saves, save down to v1), Save As with a format chooser, Save a Copy, Revert, templates, Data Recovery after a crash (browser tabs hold their copies with Web Locks too, so a background tab's copies are never taken for a crash's, and a tab repairs copies another removed when it resumes: #367), background save and export. The desktop app writes its warnings and errors (graphics adapter, a lost graphics device, panics the guard recovered from) to standard error and to `logs/vectorcraft.log` next to the preferences, the two previous runs kept and each file capped at 16 MiB, so a launch without a terminal still leaves something to attach to a bug report (#465). Native documents with a lot of path data (traced or generated art, in layers, symbols or patterns) are saved as compact JSON, about a fifth of the size; small documents stay pretty-printed (#1492). Saving to a network drive on macOS falls back to a plain fsync when the share refuses a full flush (#1625, part of #1620; not confirmed on a real share), and a save still succeeds when a folder refuses the temporary file, the rename over the document or copying permissions: the content is written elsewhere first and copied in, and a failed copy keeps the temporary file and names it (#1649, part of #1620). A document opened by a relative path finds its relatively linked images, and `vectorcraft-cli convert` lists what reading the input left out as `importWarnings` (#1606, fixes #1604).
     Preferences › File Handling › Data Recovery › Folder has a Choose… button on the desktop that picks the folder Data Recovery keeps its copies in ([Illustrator reference, Set Data Recovery preferences](https://web.archive.org/web/20210601162814/https://helpx.adobe.com/pdf/illustrator_reference.pdf)); a picked folder that Data Recovery cannot create its folders in or list is refused with a status message, and the field keeps its value. OK applies the same check to a folder typed into the field (VectorCraft's own rule): when it fails, the dialog stays open, the status bar says why and no preference changes; an empty field (the default folder) and the folder already saved are not checked. Preferences › Plug-ins & Scratch Disks is where additional plug-ins folders are set ([Illustrator preferences help](https://helpx.adobe.com/illustrator/desktop/get-started/preferences-and-settings/set-app-preferences.html)); its Additional Plug-ins Folder has a Choose… button on the desktop as well, VectorCraft's own, matching Data Recovery's, and VectorCraft's own rule refuses a picked or typed folder whose contents can't be listed. Additional Fonts Folder and Templates Folder stay typed only.
+    `prefs.set` applies the dialog's check to `recoveryFolder` and `pluginsFolder` for every caller (VectorCraft's own rule): a folder that fails returns an error and no preference changes; an empty value and the folder already saved are not checked.
     A name typed in the desktop save panel without the file type's extension gets it, and when a file of that name exists, which the panel did not ask about, Save, Save As, Save a Copy, Save as Template, the exports and every other write through a save panel ask before replacing it (dialog `replaceFile`; Cancel writes nothing).
     A name returned by the desktop save panel with the extension of a format the save does not write has it replaced with the saved format's: Save As of a new document saves `card.ait` and `card.png` as `card.vectorcraft`, and Save as Template saves `card.pdf` as `card.vctemplate`. An extension that names no format stays part of the name (`notes.v2` is saved as `notes.v2.vectorcraft`).
     Export As, Save as PDF, Save for Office Documents, Save for Web, Text Export Options, print to file, the presets' Export… and the library, swatch and Document Info saves replace a typed extension of another format than the one they write in the same way: Export As PNG writes `card.svg` as `card.png`, Save as PDF writes `card.png` as `card.pdf`, and `document.exportSelection` with a `format` suggests and adds that format's extension. An extension of the written format stays as typed in any letter case (`photo.JPEG` for JPEG), Export As SVG writes `art.svgz` compressed, and `document.exportSelection` without a `format` writes the format the typed extension names (`tests/saveext.rs`).