Plug-ins: a grant that can't be removed keeps the module it covers

plugin.install of a module that lists no permissions ignored the result
of removing the id's grant. When grants.json could not be updated, the
formerly approved bytes could still be installed without approval. The
store now gets the old module back, so the grant and the module it
covers stay together. The new module stays when the old one can't be
read back and when grants.json can't be read, since such a file approves
nothing. Deleting the last grant now reports a failure.

plugin.remove had the same fault. It now moves the settings and cache
aside and removes the grant before anything is deleted. When the grant
can't be removed, the folder is moved back, the command returns an
error and the plug-in stays installed.

crates/engine/src/cmd/plugin.rs

@@ -608,9 +608,12 @@ fn install(s: &mut Session, p: &Value) -> Result<Value> {
         // This module takes the id over, so nothing the user approved for the id applies any more.
         registry::revoke(&m.id);
         // When the write failed, the old stored module stays in the folder and needs its grant at
-        // the next launch.
-        if stored {
-            s.store_grant(&m.id, None);
+        // the next launch. A grant that can't be removed still approves the old bytes, so the store
+        // gets the module it held back, when that could be read. A grants file that can't be read
+        // approves nothing, and the new module stays.
+        if stored && !s.store_grant(&m.id, None) && previous.is_ok() && s.holds_plugin_grant(&m.id) {
+            s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref());
+            stored = false;
         }
     }
     // It gets the settings and cache kept for its id.
@@ -822,19 +825,24 @@ fn remove(s: &mut Session, p: &Value) -> Result<Value> {
     let installed = registry::get(id).is_some();
     // Settings and cache the store keeps for an id that isn't installed are deleted too.
     let stored = !installed && vectorcraft_plugins::manifest::valid_id(id) && s.has_plugin_storage(id);
-    // Its settings and cache go first: when they can't be moved aside, the plug-in stays installed,
-    // as it was, and a second `plugin.remove` tries again.
-    if (installed || stored)
-        && let Err(e) = s.remove_plugin_storage(id)
-    {
+    // Installed again, it asks again. A grant that can't be removed still approves the module; a
+    // grants file that can't be read approves nothing and doesn't stop the removal.
+    let forget = || s.store_grant(id, None) || !s.holds_plugin_grant(id);
+    // Its settings and cache are moved aside and its grant removed before anything is deleted: when
+    // either can't be done, the plug-in stays installed, as it was, and a second `plugin.remove`
+    // tries again.
+    let failed = if installed || stored {
+        s.remove_plugin_storage(id, forget).err().map(|e| e.to_string())
+    } else {
+        (!forget()).then(|| crate::store::GRANT_NOT_REMOVED.to_string())
+    };
+    if let Some(e) = failed {
         if installed {
             s.attach_plugin_storage(id);
         }
-        return Err(EngineError::Other(e.to_string()));
+        return Err(EngineError::Other(e));
     }
-    // Installed again, it asks again.
     registry::revoke(id);
-    s.store_grant(id, None);
     s.unstore_plugin(id);
     if !stored && (!installed || !registry::remove(id)) {
         return Err(err(vectorcraft_plugins::Error::NotFound(id.into())));
@@ -913,7 +921,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Remove Plug-in",
             [],
             None,
-            "{id} uninstall a plug-in (its live effects then leave the geometry as it is) and delete the settings and cache it kept (when their folder can't be moved aside, the command fails and the plug-in stays installed); for an id that isn't installed, deletes the settings and cache the plug-in store still keeps for it → {removed}",
+            "{id} uninstall a plug-in (its live effects then leave the geometry as it is) and delete the settings and cache it kept (when their folder can't be moved aside, or its grant can't be removed from the plug-in store, the command fails and the plug-in stays installed); for an id that isn't installed, deletes the settings and cache the plug-in store still keeps for it → {removed}",
             always,
             remove
         ),

crates/engine/src/store.rs

@@ -61,6 +61,8 @@ pub const DISABLED_FILE: &str = "disabled.json";
 pub const DISABLED_LOCK: &str = "disabled.lock";
 /// The file in the plug-in store that holds the permissions the user granted.
 const GRANTS_FILE: &str = "grants.json";
+/// The error of a `plugin.remove` that can't remove the plug-in's grant from [`GRANTS_FILE`].
+pub(crate) const GRANT_NOT_REMOVED: &str = "the plug-in's grant can't be removed from the plug-in store";
 /// The largest grants file read.
 const MAX_GRANTS_FILE: u64 = 1 << 20;
 /// The folder in the plug-in store that holds each plug-in's settings and cache.
@@ -167,13 +169,15 @@ fn lock_disabled(_: &Path) -> Option<std::fs::File> {
     None
 }
 
-/// Remove `dir/file`; a file that isn't there is what was wanted.
-fn take(dir: &Path, file: &str) {
+/// Remove `dir/file` → whether it is gone; a file that isn't there is what was wanted.
+fn take(dir: &Path, file: &str) -> bool {
     let path = dir.join(file);
-    if let Err(e) = std::fs::remove_file(&path)
-        && e.kind() != std::io::ErrorKind::NotFound
-    {
-        log::warn!("can't remove {}: {e}", path.display());
+    match std::fs::remove_file(&path) {
+        Err(e) if e.kind() != std::io::ErrorKind::NotFound => {
+            log::warn!("can't remove {}: {e}", path.display());
+            false
+        }
+        _ => true,
     }
 }
 
@@ -470,8 +474,7 @@ impl Session {
         let mut entries: Vec<serde_json::Value> = file.grants.into_values().filter_map(|g| serde_json::to_value(g).ok()).collect();
         entries.extend(file.other);
         if entries.is_empty() {
-            take(dir, GRANTS_FILE);
-            return true;
+            return take(dir, GRANTS_FILE);
         }
         // The file is read back only up to its cap: written compactly when pretty-printed is larger,
         // and not at all when that is too.
@@ -485,6 +488,12 @@ impl Session {
         }
     }
 
+    /// Whether the store holds a grant this version can use for the plug-in `id` (`false` without a
+    /// folder, and when the grants file can't be read).
+    pub(crate) fn holds_plugin_grant(&self, id: &str) -> bool {
+        self.plugin_dir.as_deref().is_some_and(|dir| stored_grants(dir).contains_key(id))
+    }
+
     /// Remove the plug-in `id` from the store.
     pub(crate) fn unstore_plugin(&self, id: &str) {
         if let Some(dir) = &self.plugin_dir {
@@ -510,10 +519,15 @@ impl Session {
     /// Delete the settings and cache of plug-in `id` from the store, and stop keeping any for it.
     /// The folder is first renamed out of the way (`<folder>.removed`), and that fails, leaving it
     /// as it was, when the rename does; what the deletion then leaves stays under that name, with a
-    /// log record, and is deleted at the plug-in's next removal.
-    pub(crate) fn remove_plugin_storage(&self, id: &str) -> Result<(), StorageError> {
+    /// log record, and is deleted at the plug-in's next removal. `forget` runs once the folder is out
+    /// of the way, before anything is deleted: when it returns `false`, the folder is renamed back
+    /// and this fails.
+    pub(crate) fn remove_plugin_storage(&self, id: &str, forget: impl FnOnce() -> bool) -> Result<(), StorageError> {
+        let not_forgotten = || StorageError::Io(GRANT_NOT_REMOVED.to_string());
         vectorcraft_plugins::registry::set_storage(id, None);
-        let Some(dir) = &self.plugin_dir else { return Ok(()) };
+        let Some(dir) = &self.plugin_dir else {
+            return if forget() { Ok(()) } else { Err(not_forgotten()) };
+        };
         let _held = PLUGIN_DATA_LOCK.lock().unwrap_or_else(|e| e.into_inner());
         let path = plugin_data_dir(dir, id);
         // Every storage handed out for the folder, including those runs still hold, is closed once
@@ -528,6 +542,9 @@ impl Session {
         let data = dir.join(PLUGIN_DATA_DIR);
         if is_link(&data) {
             log::warn!("{} is a link: the plug-in settings and cache it leads to are left", data.display());
+            if !forget() {
+                return Err(not_forgotten());
+            }
             close();
             return Ok(());
         }
@@ -540,6 +557,14 @@ impl Session {
             Err(e) => return Err(StorageError::Io(format!("{}: {e}", path.display()))),
             Ok(()) => {}
         }
+        if !forget() {
+            if std::fs::symlink_metadata(&aside).is_ok()
+                && let Err(e) = std::fs::rename(&aside, &path)
+            {
+                log::warn!("can't move {} back: {e}", aside.display());
+            }
+            return Err(not_forgotten());
+        }
         close();
         if std::fs::symlink_metadata(&aside).is_ok()
             && let Err(e) = std::fs::remove_dir_all(&aside)

crates/engine/src/tests/plugins.rs

@@ -811,6 +811,133 @@ fn a_failed_store_write_of_a_module_without_permissions_keeps_the_stored_grant()
     run(&mut s, "plugin.remove", json!({"id": id}));
 }
 
+/// A module listing no permissions whose id's grant can't be removed from `grants.json` doesn't stay
+/// in the store, so the grant and the module it covers stay together. It does stay when the module
+/// before it can't be read back, and when the grants file can't be read and so approves nothing.
+#[test]
+fn a_grant_that_cannot_be_removed_keeps_the_stored_module() {
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-forget");
+    let id = "test.engine.permissions.forgetfail";
+    let empty = json!({"objects": []});
+    let approved = asking(id, r#"["doc.read"]"#, &empty);
+    let plain = asking(id, "null", &empty);
+    let replace = json!({"dataBase64": vectorcraft_format::base64_encode(&plain), "replace": true});
+    let grants = folder.0.join("grants.json");
+    let mut s = session();
+    s.attach_plugin_store(folder.0.clone());
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&approved), "grant": ["doc.read"]}));
+    let file = s.stored_plugin_file(id).unwrap();
+    // An entry this version can't use is written back as it was read: `1e9` as `1000000000.0`, which
+    // makes the rewritten file larger than the 1 MiB that is read.
+    let entry = serde_json::from_slice::<Vec<Value>>(&std::fs::read(&grants).unwrap()).unwrap().remove(0);
+    let full = format!(r#"[{entry},{{"n":[{}]}}]"#, vec!["1e9"; 200_000].join(",")).into_bytes();
+    std::fs::write(&grants, &full).unwrap();
+
+    run(&mut s, "plugin.install", replace.clone());
+    assert_eq!(std::fs::read(&file).unwrap(), approved, "the store gets the module the grant covers back");
+    assert_eq!(std::fs::read(&grants).unwrap(), full);
+    assert!(vectorcraft_plugins::registry::granted(id).is_none(), "the process runs the module that lists nothing");
+
+    let oversized = vectorcraft_plugins::Limits::default().max_module_bytes as u64 + 1;
+    std::fs::File::options().write(true).open(&file).unwrap().set_len(oversized).unwrap();
+    run(&mut s, "plugin.install", replace.clone());
+    assert_eq!(std::fs::read(&file).unwrap(), plain, "a module that can't be read back can't be put back");
+
+    std::fs::write(&file, &approved).unwrap();
+    std::fs::write(&grants, b"not a list").unwrap();
+    run(&mut s, "plugin.install", replace);
+    assert_eq!(std::fs::read(&file).unwrap(), plain, "a grants file that can't be read approves nothing");
+
+    std::fs::remove_file(&grants).unwrap();
+    run(&mut s, "plugin.remove", json!({"id": id}));
+}
+
+/// `plugin.remove` returns an error and keeps the plug-in when its grant can't be removed from
+/// `grants.json`; a second one tries again. A grants file that can't be read doesn't stop it.
+#[test]
+fn a_grant_that_cannot_be_removed_stops_plugin_remove() {
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-remove");
+    let id = "test.engine.permissions.removefail";
+    let approved = asking(id, r#"["doc.read"]"#, &json!({"objects": []}));
+    let approved_data = vectorcraft_format::base64_encode(&approved);
+    let grants = folder.0.join("grants.json");
+    let mut s = session();
+    s.attach_plugin_store(folder.0.clone());
+    run(&mut s, "plugin.install", json!({"dataBase64": approved_data, "grant": ["doc.read"]}));
+    let file = s.stored_plugin_file(id).unwrap();
+    let recorded = std::fs::read(&grants).unwrap();
+    // `1e9` is written back as `1000000000.0`, which makes the rewritten file larger than the 1 MiB that is read.
+    let entry = serde_json::from_slice::<Vec<Value>>(&recorded).unwrap().remove(0);
+    let full = format!(r#"[{entry},{{"n":[{}]}}]"#, vec!["1e9"; 200_000].join(",")).into_bytes();
+    std::fs::write(&grants, &full).unwrap();
+    let data = crate::store::plugin_data_dir(&folder.0, id);
+    std::fs::create_dir_all(&data).unwrap();
+    std::fs::write(data.join("settings.json"), b"{}").unwrap();
+
+    assert!(s.execute("plugin.remove", &json!({"id": id})).is_err());
+    assert!(vectorcraft_plugins::registry::get(id).is_some(), "the plug-in stays installed");
+    assert!(vectorcraft_plugins::registry::granted(id).is_some(), "it keeps its grant");
+    assert_eq!(std::fs::read(&file).unwrap(), approved, "the store keeps the module the grant covers");
+    assert_eq!(std::fs::read(&grants).unwrap(), full);
+    assert!(data.join("settings.json").is_file(), "it keeps its settings");
+
+    // Settings that can't be moved aside leave the grant recorded. A file at the aside path stops
+    // the move on Unix, and a file held open in the folder stops it on Windows.
+    std::fs::write(&grants, &recorded).unwrap();
+    let mut aside = data.clone().into_os_string();
+    aside.push(".removed");
+    let aside = std::path::PathBuf::from(aside);
+    std::fs::write(&aside, b"in the way").unwrap();
+    #[cfg(windows)]
+    let held = {
+        use std::os::windows::fs::OpenOptionsExt;
+        std::fs::File::options().read(true).share_mode(1).open(data.join("settings.json")).unwrap()
+    };
+    assert!(s.execute("plugin.remove", &json!({"id": id})).is_err());
+    #[cfg(windows)]
+    drop(held);
+    assert_eq!(std::fs::read(&grants).unwrap(), recorded, "the grant stays while the settings do");
+    assert!(vectorcraft_plugins::registry::get(id).is_some());
+    let _ = std::fs::remove_file(&aside);
+
+    run(&mut s, "plugin.remove", json!({"id": id}));
+    assert!(vectorcraft_plugins::registry::get(id).is_none());
+    assert!(!grants.exists(), "the second removal forgets the grant");
+
+    run(&mut s, "plugin.install", json!({"dataBase64": approved_data, "grant": ["doc.read"]}));
+    std::fs::write(&grants, b"not a list").unwrap();
+    run(&mut s, "plugin.remove", json!({"id": id}));
+    assert!(vectorcraft_plugins::registry::get(id).is_none(), "a grants file that can't be read approves nothing");
+    assert!(!std::path::Path::new(&file).exists());
+}
+
+/// Windows keeps a file another program holds open without delete sharing, so the last grant can't
+/// be removed by deleting `grants.json`.
+#[cfg(windows)]
+#[test]
+fn a_grants_file_that_cannot_be_deleted_keeps_the_stored_module() {
+    use std::os::windows::fs::OpenOptionsExt;
+    const FILE_SHARE_READ: u32 = 1;
+    let folder = crate::tests_links::Folder::new("plugin-store-held-grants");
+    let id = "test.engine.permissions.heldgrants";
+    let empty = json!({"objects": []});
+    let approved = asking(id, r#"["doc.read"]"#, &empty);
+    let plain = asking(id, "null", &empty);
+    let grants = folder.0.join("grants.json");
+    let mut s = session();
+    s.attach_plugin_store(folder.0.clone());
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&approved), "grant": ["doc.read"]}));
+    let file = s.stored_plugin_file(id).unwrap();
+
+    let held = std::fs::File::options().read(true).share_mode(FILE_SHARE_READ).open(&grants).unwrap();
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&plain), "replace": true}));
+    drop(held);
+    assert_eq!(std::fs::read(&file).unwrap(), approved, "the store gets the module the grant covers back");
+    assert!(grants.exists());
+
+    run(&mut s, "plugin.remove", json!({"id": id}));
+}
+
 /// A plug-in folder that leaves a module listing no permissions under an id forgets the process's
 /// grant for it, unless a later file in the folder leaves a granted module there. A grant kept in
 /// the plug-in store stays, with the stored module it covers.
@@ -1893,7 +2020,7 @@ fn plugin_storage_is_capped_and_survives_a_damaged_settings_file() {
     assert_eq!(st.set_cache_entry("tile", b"again", m), Err(StorageError::Unavailable));
     assert!(!dir.exists());
     assert_eq!(upper.setting("k0", m).unwrap(), Some(b"upper".to_vec()));
-    s.remove_plugin_storage(upper_id).unwrap();
+    s.remove_plugin_storage(upper_id, || true).unwrap();
 }
 
 /// Files larger than the storage reads are not read: a settings file larger than 1 MiB, although

docs/mcp.md

@@ -6570,7 +6570,8 @@ an error naming the permissions needed. The grant is kept with the module's hash
 same bytes install again without `grant`, and a module with other bytes has `needsApproval: true` and needs a new
 `grant` even when it lists the same permissions. `plugin.install` does not check that a person approved `grant`: it
 trusts the client that sends it. `plugin.remove` forgets the grant, and so does installing a module that lists no
-permissions under that id: the earlier bytes then need a new `grant`. A
+permissions under that id: the earlier bytes then need a new `grant`. When `plugin.remove` can't remove the grant from
+the plug-in store, it returns an error and the plug-in stays installed. A
 filter without `doc.write` or `state` that returns output needing them gets an error and the document is unchanged.
 A plug-in of API level 2 (`api: 2` in its entry) that lists `progress` may report its progress, see Cancel and write
 log lines: `notifications/cancelled` makes its `vc_cancelled` return 1, and its log lines go to the server's log

docs/plugins.md

@@ -181,10 +181,14 @@ capability model proposed for PhotoCraft plug-ins
   fails as well, the folder holds the new module with the old grant, the grant does not cover that module, and the
   plug-in is skipped at the next start until it is approved again. A grant whose hash is not 64 lower-case
   hexadecimal digits covers nothing. `plugin.remove` forgets the grant, and so does a successful `plugin.install` of a
-  module that lists no permissions under that id, so the earlier bytes need a new approval. A refused install leaves
-  the grant as it is. When such an install can't write the module to the `Plug-ins` folder, the module already stored
-  there stays and `grants.json` keeps its grant. When VectorCraft loads the `Plug-ins` folder, it forgets any grant
-  for an id whose stored module lists no permissions. In Safe Mode, and for a stored module that the user disabled,
+  module that lists no permissions under that id, so the earlier bytes need a new approval. When `plugin.remove` can't
+  remove the grant from `grants.json`, it returns an error, the plug-in stays installed, and a second `plugin.remove`
+  tries again. A refused install leaves
+  the grant as it is. When such an install can't write the module to the `Plug-ins` folder, or can't remove the grant
+  from `grants.json`, the folder keeps the module it held and `grants.json` keeps the grant. The new module runs until
+  the session ends. When the grant can't be removed and the module already stored can't be read back, the new module
+  stays in the folder. A `grants.json` that can't be read grants nothing, and the new module stays in the folder then
+  too. When VectorCraft loads the `Plug-ins` folder, it forgets any grant for an id whose stored module lists no permissions. In Safe Mode, and for a stored module that the user disabled,
   VectorCraft does not read the module, and `grants.json` keeps the grant for its id. The Additional Plug-ins Folder
   and `plugin.reload` do not change the `Plug-ins` folder: when they leave a module that lists no permissions under an
   id, the process forgets its own grant for that id, and a grant kept in `grants.json` stays with the stored module it