diff --git a/crates/engine/src/cmd/plugin.rs b/crates/engine/src/cmd/plugin.rs index 9cad35cdf..5985c43d9 100644 --- a/crates/engine/src/cmd/plugin.rs +++ b/crates/engine/src/cmd/plugin.rs @@ -640,18 +640,16 @@ 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; 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. diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index 5738290d9..ae98bdf4f 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -176,6 +176,12 @@ fn lock_disabled(_: &Path) -> Option { 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 = 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, } +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 diff --git a/crates/engine/src/tests/plugins.rs b/crates/engine/src/tests/plugins.rs index 83b57a6f6..08ed19db1 100644 --- a/crates/engine/src/tests/plugins.rs +++ b/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::>(&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] diff --git a/docs/plugins.md b/docs/plugins.md index d5f4e8f4a..f197f96ac 100644 --- a/docs/plugins.md +++ b/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