From 63a483492b4f372fb3472062f91f61278f62d3f3 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 8 Nov 2026 18:59:22 -0800 Subject: [PATCH] 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 | 10 +++--- crates/engine/src/store.rs | 49 ++++++++++++++++++++++-------- crates/engine/src/tests/plugins.rs | 43 +++++++++++++++++++++++--- docs/plugins.md | 14 +++++---- 4 files changed, 89 insertions(+), 27 deletions(-) diff --git a/crates/engine/src/cmd/plugin.rs b/crates/engine/src/cmd/plugin.rs index 03c9f97ab..213b167af 100644 --- a/crates/engine/src/cmd/plugin.rs +++ b/crates/engine/src/cmd/plugin.rs @@ -621,8 +621,9 @@ fn install(s: &mut Session, p: &Value) -> Result { 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 { 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` diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index 6ebdea6a7..92a6507de 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -111,15 +111,21 @@ fn list_files(dir: &Path, ext: &str) -> Vec { /// 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, 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> { + 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, } +/// 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 { +fn read_grants(dir: &Path) -> Result { 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::>(&bytes).map_err(|e| e.to_string()))?; + let entries = serde_json::from_slice::>(&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 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() } } diff --git a/crates/engine/src/tests/plugins.rs b/crates/engine/src/tests/plugins.rs index 4ced0fcb3..e367708a5 100644 --- a/crates/engine/src/tests/plugins.rs +++ b/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()); } diff --git a/docs/plugins.md b/docs/plugins.md index adbd128cc..178bfd927 100644 --- a/docs/plugins.md +++ b/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