Plug-ins: a grants file that can't be read keeps the grant it may hold

A grants file that can't be opened or read may name the old module once
it reads again. plugin.remove and an install of a module listing no
permissions now fail on it and keep the module. A file over 1 MiB or one
that doesn't hold a list still approves nothing and doesn't stop them.

crates/engine/src/cmd/plugin.rs

@@ -621,8 +621,9 @@ 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. A grants file that can't be read
-        // approves nothing, and the new module stays.
+        // gets the module it held back, when that could be read. 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) && previous.is_ok() && s.holds_plugin_grant(&m.id) {
             s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref());
             stored = false;
@@ -837,8 +838,9 @@ 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);
-    // 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.
+    // Installed again, it asks again. A grant that can't be removed still approves the module, and
+    // so may a grants file that can't be opened or read. One that is too large or doesn't hold a
+    // list 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`

crates/engine/src/store.rs

@@ -111,15 +111,21 @@ fn list_files(dir: &Path, ext: &str) -> Vec<PathBuf> {
 
 /// The content of `path` when it is at most `max` bytes; the file is never read past `max`.
 pub(crate) fn read_capped(path: &Path, max: u64) -> Result<Vec<u8>, String> {
-    let file = std::fs::File::open(path).map_err(|e| e.to_string())?;
-    let mut bytes = vec![];
-    file.take(max + 1).read_to_end(&mut bytes).map_err(|e| e.to_string())?;
+    let bytes = read_past_cap(path, max).map_err(|e| e.to_string())?;
     if bytes.len() as u64 > max {
         return Err(too_large(max));
     }
     Ok(bytes)
 }
 
+/// The first `max + 1` bytes of `path` at most, so that a result longer than `max` shows the file is
+/// over its cap.
+fn read_past_cap(path: &Path, max: u64) -> std::io::Result<Vec<u8>> {
+    let mut bytes = vec![];
+    std::fs::File::open(path)?.take(max + 1).read_to_end(&mut bytes)?;
+    Ok(bytes)
+}
+
 /// What a file over its cap is told (a detail after the file's name, in the log or an error).
 fn too_large(max: u64) -> String {
     format!("larger than {max} bytes")
@@ -458,7 +464,7 @@ impl Session {
         let mut file = match read_grants(dir) {
             Ok(file) => file,
             Err(e) => {
-                log::warn!("not updating the stored plug-in grants {}: {e}", dir.join(GRANTS_FILE).display());
+                log::warn!("not updating the stored plug-in grants {}: {}", dir.join(GRANTS_FILE).display(), e.why);
                 return false;
             }
         };
@@ -488,10 +494,14 @@ 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).
+    /// 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.
     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))
+        self.plugin_dir.as_deref().is_some_and(|dir| match read_grants(dir) {
+            Ok(file) => file.grants.contains_key(id),
+            Err(e) => e.unread,
+        })
     }
 
     /// Remove the plug-in `id` from the store.
@@ -901,16 +911,29 @@ struct GrantsFile {
     other: Vec<serde_json::Value>,
 }
 
+/// 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
+    /// too large or doesn't hold a list, and approves nothing.
+    unread: bool,
+    /// The detail for a log record.
+    why: String,
+}
+
 /// The grants file of the plug-in store `dir`: empty when there is none, an error when it is too
 /// large, can't be read or doesn't hold a list. An entry that doesn't parse as a grant, or names an
 /// invalid id, is kept in [`GrantsFile::other`] with a log record.
-fn read_grants(dir: &Path) -> Result<GrantsFile, String> {
+fn read_grants(dir: &Path) -> Result<GrantsFile, GrantsError> {
     let path = dir.join(GRANTS_FILE);
-    if !path.exists() {
-        return Ok(GrantsFile::default());
+    let bytes = match read_past_cap(&path, MAX_GRANTS_FILE) {
+        Ok(bytes) => bytes,
+        Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(GrantsFile::default()),
+        Err(e) => return Err(GrantsError { unread: true, why: e.to_string() }),
+    };
+    if bytes.len() as u64 > MAX_GRANTS_FILE {
+        return Err(GrantsError { unread: false, why: too_large(MAX_GRANTS_FILE) });
     }
-    let entries =
-        read_capped(&path, MAX_GRANTS_FILE).and_then(|bytes| serde_json::from_slice::<Vec<serde_json::Value>>(&bytes).map_err(|e| e.to_string()))?;
+    let entries = serde_json::from_slice::<Vec<serde_json::Value>>(&bytes).map_err(|e| GrantsError { unread: false, why: e.to_string() })?;
     let mut file = GrantsFile::default();
     let mut first_error = None;
     for entry in entries {
@@ -938,7 +961,7 @@ fn stored_grants(dir: &Path) -> BTreeMap<String, vectorcraft_plugins::permission
     match read_grants(dir) {
         Ok(file) => file.grants,
         Err(e) => {
-            log::warn!("skipping the stored plug-in grants {}: {e}", dir.join(GRANTS_FILE).display());
+            log::warn!("skipping the stored plug-in grants {}: {}", dir.join(GRANTS_FILE).display(), e.why);
             BTreeMap::new()
         }
     }

crates/engine/src/tests/plugins.rs

@@ -813,7 +813,7 @@ fn a_failed_store_write_of_a_module_without_permissions_keeps_the_stored_grant()
 
 /// 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.
+/// before it can't be read back, and when the grants file doesn't hold a list 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");
@@ -846,14 +846,49 @@ fn a_grant_that_cannot_be_removed_keeps_the_stored_module() {
     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");
+    assert_eq!(std::fs::read(&file).unwrap(), plain, "a grants file that isn't a list approves nothing");
 
     std::fs::remove_file(&grants).unwrap();
     run(&mut s, "plugin.remove", json!({"id": id}));
 }
 
+/// A grants file that can't be opened or read may name the old module once it reads again, so
+/// neither a module listing no permissions nor `plugin.remove` gets past it. A directory at the
+/// file's path gives a read error on every platform.
+#[test]
+fn a_grants_file_that_cannot_be_read_keeps_the_grant_and_the_module_it_covers() {
+    let folder = crate::tests_links::Folder::new("plugin-store-unread-grants");
+    let id = "test.engine.permissions.unreadgrants";
+    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 recorded = std::fs::read(&grants).unwrap();
+    std::fs::remove_file(&grants).unwrap();
+    std::fs::create_dir(&grants).unwrap();
+
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&plain), "replace": true}));
+    assert_eq!(std::fs::read(&file).unwrap(), approved, "the store gets the module the grant covers back");
+
+    run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&approved), "grant": ["doc.read"]}));
+    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(), approved, "the store keeps the module");
+
+    std::fs::remove_dir(&grants).unwrap();
+    std::fs::write(&grants, &recorded).unwrap();
+    run(&mut s, "plugin.remove", json!({"id": id}));
+    assert!(vectorcraft_plugins::registry::get(id).is_none());
+    assert!(!grants.exists(), "the file reads again and the grant goes");
+}
+
 /// `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.
+/// `grants.json`; a second one tries again. A grants file that doesn't hold a list 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");
@@ -907,7 +942,7 @@ fn a_grant_that_cannot_be_removed_stops_plugin_remove() {
     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!(vectorcraft_plugins::registry::get(id).is_none(), "a grants file that isn't a list approves nothing");
     assert!(!std::path::Path::new(&file).exists());
 }
 

docs/plugins.md

@@ -214,12 +214,14 @@ capability model proposed for PhotoCraft plug-ins
   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. 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,
+  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. 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 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