Plug-ins: a grant left behind stops counting when its module can't go back

plugin.install and plugin.remove count an entry of grants.json that this
version can't use when it can't be removed. plugin.install stops counting
a grant it can't remove when the old module can't be written back to the
Plug-ins folder.

crates/engine/src/cmd/plugin.rs

@@ -640,18 +640,16 @@ fn install(s: &mut Session, p: &Value) -> Result<Value> {
         registry::revoke(&m.id);
         // When the write failed, the old stored module stays in the folder and needs its grant at
         // 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; when there is no module to go with
-        // the grant, the grant stops counting for this session. A grants file that is too large or
-        // doesn't hold a list approves nothing, and the new module stays; one that can't be opened or
-        // read may approve the old bytes once it reads again.
+        // gets the module it held back. When that module can't be read or written back, or there
+        // was none, no module goes with the grant, and the grant stops counting for this session. A
+        // grants file that is too large or doesn't hold a list approves nothing, and the new module
+        // stays; one that can't be opened or read may approve the old bytes once it reads again.
         if stored && !s.store_grant(&m.id, None) && s.holds_plugin_grant(&m.id) {
-            if !matches!(previous, Ok(Some(_))) {
+            let back = previous.as_ref().is_ok_and(|held| s.put_back_plugin(plugin.id(), held.as_deref()));
+            stored = !back;
+            if !back || matches!(previous, Ok(None)) {
                 s.ignore_stored_grant(&m.id);
             }
-            if let Ok(held) = previous {
-                s.put_back_plugin(plugin.id(), held.as_deref());
-                stored = false;
-            }
         }
     }
     // It gets the settings and cache kept for its id.

crates/engine/src/store.rs

@@ -176,6 +176,12 @@ fn lock_disabled(_: &Path) -> Option<std::fs::File> {
     None
 }
 
+#[cfg(test)]
+thread_local! {
+    /// Tests: [`Session::put_back_plugin`] fails on this thread, as on a full volume.
+    pub(crate) static PUT_BACK_FAILS: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
+}
+
 /// 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);
@@ -363,9 +369,7 @@ impl Session {
         for id in &asks_nothing {
             vectorcraft_plugins::registry::revoke(id);
             let named = match &kept {
-                Ok(file) => {
-                    file.grants.contains_key(id) || file.other.iter().any(|e| e.get("id").and_then(serde_json::Value::as_str) == Some(id.as_str()))
-                }
+                Ok(file) => file.names(id),
                 Err(e) => e.unread,
             };
             if named && !self.store_grant(id, None) && self.holds_plugin_grant(id) {
@@ -458,12 +462,15 @@ impl Session {
     }
 
     /// Return the store's module for the plug-in `id` to `previous`, the bytes it held before a
-    /// write ([`Session::stored_plugin_bytes`]): with `None`, the file is removed.
-    pub(crate) fn put_back_plugin(&self, id: &str, previous: Option<&[u8]>) {
+    /// write ([`Session::stored_plugin_bytes`]): with `None`, the file is removed. → whether the
+    /// store now holds `previous`.
+    pub(crate) fn put_back_plugin(&self, id: &str, previous: Option<&[u8]>) -> bool {
+        #[cfg(test)]
+        if PUT_BACK_FAILS.get() {
+            return false;
+        }
         match previous {
-            Some(bytes) => {
-                self.store_plugin(id, bytes);
-            }
+            Some(bytes) => self.store_plugin(id, bytes),
             None => self.unstore_plugin(id),
         }
     }
@@ -531,21 +538,20 @@ impl Session {
         }
     }
 
-    /// Whether the store holds, or may hold, a grant this version can use for the plug-in `id`.
-    /// `true` when the grants file can't be opened or read, because it may name `id` once it reads
-    /// again; `false` without a folder, and when the file is too large or doesn't hold a list.
+    /// Whether the store holds, or may hold, an entry for the plug-in `id`, including one this
+    /// version can't use ([`GrantsFile::names`]). `true` when the grants file can't be opened or
+    /// read, because it may name `id` once it reads again; `false` without a folder, and when the
+    /// file is too large or doesn't hold a list.
     pub(crate) fn holds_plugin_grant(&self, id: &str) -> bool {
         self.plugin_dir.as_deref().is_some_and(|dir| match read_grants(dir) {
-            Ok(file) => file.grants.contains_key(id),
+            Ok(file) => file.names(id),
             Err(e) => e.unread,
         })
     }
 
-    /// Remove the plug-in `id` from the store.
-    pub(crate) fn unstore_plugin(&self, id: &str) {
-        if let Some(dir) = &self.plugin_dir {
-            take(dir, &format!("{}.{PLUGIN_EXT}", file_stem(id)));
-        }
+    /// Remove the plug-in `id` from the store → whether its file is gone (`false` without a folder).
+    pub(crate) fn unstore_plugin(&self, id: &str) -> bool {
+        self.plugin_dir.as_deref().is_some_and(|dir| take(dir, &format!("{}.{PLUGIN_EXT}", file_stem(id))))
     }
 
     /// Keep the settings and cache of plug-in `id` in the attached plug-in store from now on; without
@@ -1042,6 +1048,13 @@ struct GrantsFile {
     other: Vec<serde_json::Value>,
 }
 
+impl GrantsFile {
+    /// Whether the file has an entry for the plug-in `id`, usable by this version or not.
+    fn names(&self, id: &str) -> bool {
+        self.grants.contains_key(id) || self.other.iter().any(|e| e.get("id").and_then(serde_json::Value::as_str) == Some(id))
+    }
+}
+
 /// Why the grants file of a plug-in store gave no grants.
 struct GrantsError {
     /// Opening or reading the file failed, so what it holds is unknown. When `false`, the file is

crates/engine/src/tests/plugins.rs

@@ -1517,6 +1517,74 @@ fn a_grant_that_cannot_be_removed_keeps_the_stored_module() {
     run(&mut s, "plugin.remove", json!({"id": id}));
 }
 
+/// A grant that can't be removed stops counting for the session when the module it covers can't be
+/// written back either: the store holds the new module, and the old bytes would otherwise install
+/// without approval.
+#[test]
+fn a_grant_left_behind_when_its_module_cannot_go_back_stops_counting() {
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-put-back");
+    let id = "test.engine.permissions.failedputback";
+    let empty = json!({"objects": []});
+    let approved = asking(id, r#"["doc.read"]"#, &empty);
+    let approved_data = vectorcraft_format::base64_encode(&approved);
+    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": approved_data, "grant": ["doc.read"]}));
+    let file = s.stored_plugin_file(id).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>>(&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();
+
+    crate::store::PUT_BACK_FAILS.set(true);
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&plain), "replace": true}));
+    crate::store::PUT_BACK_FAILS.set(false);
+    assert_eq!(std::fs::read(&file).unwrap(), plain, "the store keeps the new module");
+    assert_eq!(std::fs::read(&grants).unwrap(), full);
+    assert_eq!(run(&mut s, "plugin.inspect", json!({"dataBase64": approved_data}))["needsApproval"], true, "the grant left behind stops counting");
+    let refused = s.execute("plugin.install", &json!({"dataBase64": approved_data, "replace": true})).unwrap_err().to_string();
+    assert!(refused.contains("doc.read"), "{refused}");
+
+    std::fs::remove_file(&grants).unwrap();
+    run(&mut s, "plugin.remove", json!({"id": id}));
+}
+
+/// An entry of `grants.json` that this version can't use is kept for a version that can, so when it
+/// can't be removed, neither `plugin.remove` nor a replacing module gets past it.
+#[test]
+fn an_unusable_entry_that_cannot_be_removed_stops_plugin_remove() {
+    let folder = crate::tests_links::Folder::new("plugin-store-failed-remove-other");
+    let id = "test.engine.permissions.removeother";
+    let empty = json!({"objects": []});
+    let first = asking(id, "null", &empty);
+    let second = asking(id, "[]", &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(&first)}));
+    let file = s.stored_plugin_file(id).unwrap();
+    let entry = json!({"id": id, "hash": "0".repeat(64), "permissions": ["no.such.permission"]});
+    // `1e9` is written back as `1000000000.0`, which makes the rewritten file larger than the 1 MiB that is read.
+    let full = format!(r#"[{entry},{{"n":[{}]}}]"#, vec!["1e9"; 200_000].join(",")).into_bytes();
+    std::fs::write(&grants, &full).unwrap();
+
+    let refused = s.execute("plugin.remove", &json!({"id": id})).unwrap_err().to_string();
+    assert!(refused.contains(crate::store::GRANT_NOT_REMOVED), "{refused}");
+    assert!(vectorcraft_plugins::registry::get(id).is_some(), "the plug-in stays installed");
+    assert_eq!(std::fs::read(&file).unwrap(), first, "the store keeps the module");
+    assert_eq!(std::fs::read(&grants).unwrap(), full);
+
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&second), "replace": true}));
+    assert_eq!(std::fs::read(&file).unwrap(), first, "the store gets its module back");
+
+    std::fs::write(&grants, serde_json::to_vec(&json!([entry])).unwrap()).unwrap();
+    run(&mut s, "plugin.remove", json!({"id": id}));
+    assert!(vectorcraft_plugins::registry::get(id).is_none());
+    assert!(!grants.exists(), "the removal takes the entry out of the file");
+}
+
 /// A grant that can't be removed when no module is stored for its id has no module to go with, so
 /// it stops counting for the session.
 #[test]

docs/plugins.md

@@ -218,8 +218,9 @@ capability model proposed for PhotoCraft plug-ins
   tries again. A refused install leaves the grant as it is. When an install of a module that lists no permissions 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, and the grant does not
-  count for the rest of the session. The grant also does not count when the folder held no module for that id. A
+  removed and the module already stored can't be read back or written back, the new module stays in the folder, and
+  the grant does not count for the rest of the session. The grant also does not count when the folder held no module
+  for that id. A
   `grants.json` larger than 1 MiB, or one that is not a JSON list, grants nothing: the new module stays in the folder,
   and `plugin.remove` removes the plug-in. A `grants.json` that can't be opened or read counts as a grant that can't be
   removed. When VectorCraft loads the `Plug-ins` folder, it forgets any grant for an id whose stored module lists no
@@ -232,7 +233,8 @@ capability model proposed for PhotoCraft plug-ins
   id, the process forgets its own grant for that id, and a grant kept in `grants.json` stays with the stored module it
   covers. An entry of `grants.json` that this version can't use (a permission it doesn't know, an invalid id) is
   skipped and stays in the file until its plug-in is granted again, replaced by a module that lists no permissions, or
-  removed. A `grants.json` that is not a JSON list is never overwritten, and the file is never written larger than
+  removed. When such an entry can't be removed from the file, `plugin.install` and `plugin.remove` treat it as a grant
+  that can't be removed. A `grants.json` that is not a JSON list is never overwritten, and the file is never written larger than
   1 MiB, the most that is read. When a `Plug-ins` folder is in use, only the grants kept
   in it count for `plugin.install`, `plugin.inspect`, `plugin.reload` and the Additional Plug-ins Folder. At startup,
   a stored plug-in is skipped when no grant covers its bytes and the permissions it lists. The Additional Plug-ins