diff --git a/crates/engine/src/cmd/plugin.rs b/crates/engine/src/cmd/plugin.rs index 94b8f7f582..456d130a93 100644 --- a/crates/engine/src/cmd/plugin.rs +++ b/crates/engine/src/cmd/plugin.rs @@ -742,15 +742,17 @@ fn install(s: &mut Session, p: &Value) -> Result { // Installed plug-ins stay installed after a restart (#2541). The module is written first and its // grant only once it is there; when the grant can't be written, the store gets the module it // held back. A failed write of that module is only logged: the store then holds the new module, - // which the grant doesn't cover. A stored module that can't be read back isn't replaced by one - // that needs a grant. + // which the grant doesn't cover. A write that reports a failure may still have put the old + // module back, so the store is read again to tell which module it holds. A stored module that + // can't be read back isn't replaced by one that needs a grant. let previous = s.stored_plugin_bytes(plugin.id()); let mut stored = (previous.is_ok() || !m.asks_permissions()) && s.store_plugin(plugin.id(), &bytes); + let holds_new = |s: &Session| s.stored_plugin_bytes(plugin.id()).is_ok_and(|held| held.as_deref() == Some(&bytes[..])); if m.asks_permissions() { let g = Grant { id: m.id.clone(), hash: plugin.hash().to_string(), permissions: wanted }; registry::grant(g.clone()); if stored && !s.store_grant(&m.id, Some(g)) { - stored = !s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref()); + stored = !s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref()) && holds_new(s); } } else { // This module takes the id over, so nothing the user approved for the id applies any more. @@ -763,7 +765,7 @@ fn install(s: &mut Session, p: &Value) -> Result { // 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) { let back = previous.as_ref().is_ok_and(|held| s.put_back_plugin(plugin.id(), held.as_deref())); - stored = !back; + stored = !back && holds_new(s); if !back || matches!(previous, Ok(None)) { s.ignore_stored_grant(&m.id); } diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index 01ebc46526..a8a474fc98 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -190,6 +190,9 @@ fn lock_disabled(_: &Path) -> Option { 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) }; + /// Tests: [`Session::put_back_plugin`] writes the old module back on this thread and then + /// reports a failure, as when the last flush of a copy fails. + pub(crate) static PUT_BACK_FAILS_AFTER_WRITE: 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. @@ -498,16 +501,21 @@ 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. → whether the - /// store now holds `previous`. + /// write reported success; a write that reports a failure may still have put `previous` back. pub(crate) fn put_back_plugin(&self, id: &str, previous: Option<&[u8]>) -> bool { #[cfg(test)] if PUT_BACK_FAILS.get() { return false; } - match previous { + let back = match previous { Some(bytes) => self.store_plugin(id, bytes), None => self.unstore_plugin(id), + }; + #[cfg(test)] + if PUT_BACK_FAILS_AFTER_WRITE.get() { + return false; } + back } /// The file the plug-in `id` is stored in, spelled as [`Session::load_stored_plugins`] names it diff --git a/crates/engine/src/tests/plugins.rs b/crates/engine/src/tests/plugins.rs index 4839a1c457..51163d3d63 100644 --- a/crates/engine/src/tests/plugins.rs +++ b/crates/engine/src/tests/plugins.rs @@ -2243,6 +2243,91 @@ fn an_update_left_in_the_store_by_a_failed_put_back_turns_a_disabled_plugin_on() safe_mode::set_disabled(vec![]); } +/// An update whose grant can't be written, and whose put-back writes the old module and then +/// reports a failure, leaves the old module in the store: a disabled plug-in stays disabled, and the +/// next start doesn't load it. +#[test] +fn an_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off() { + if !in_own_process("an_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off") { + return; + } + use vectorcraft_plugins::{registry, safe_mode}; + let folder = crate::tests_links::Folder::new("plugin-store-put-back-fails-after-write"); + let id = "test.engine.permissions.putbacklate"; + let first = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let changed = asking(id, r#"["doc.read","doc.write"]"#, &json!({"objects": []})); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&first), "grant": ["doc.read"]})); + run(&mut s, "plugin.setEnabled", json!({"id": id, "enabled": false})); + let disabled = safe_mode::disabled(); + assert!(!disabled.is_empty()); + let grants = folder.0.join("grants.json"); + let recorded = std::fs::read(&grants).unwrap(); + std::fs::remove_file(&grants).unwrap(); + std::fs::create_dir_all(grants.join("block")).unwrap(); + + crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(true); + let update = json!({"dataBase64": vectorcraft_format::base64_encode(&changed), "grant": ["doc.read", "doc.write"], "replace": true}); + let installed = s.execute("plugin.install", &update); + crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(false); + installed.unwrap(); + assert_eq!(std::fs::read(s.stored_plugin_file(id).unwrap()).unwrap(), first, "the store holds the old module"); + assert_eq!(safe_mode::disabled(), disabled, "the plug-in stays disabled"); + let saved = std::fs::read(folder.0.join(crate::store::DISABLED_FILE)).unwrap(); + assert!(!safe_mode::parse_disabled_file(&saved).is_empty(), "the disabled list on disk is kept"); + + std::fs::remove_dir_all(&grants).unwrap(); + std::fs::write(&grants, &recorded).unwrap(); + registry::remove(id); + registry::revoke(id); + let mut next = session(); + next.attach_plugin_store(folder.0.clone()); + assert!(registry::get(id).is_none(), "the disabled old module doesn't load"); + safe_mode::set_disabled(vec![]); +} + +/// A module without permissions whose install can't remove the old grant, and whose put-back writes +/// the old module and then reports a failure, leaves the old module in the store: a disabled plug-in +/// stays disabled. +#[test] +fn a_plain_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off() { + if !in_own_process("a_plain_update_put_back_that_fails_after_writing_keeps_a_disabled_plugin_off") { + return; + } + use vectorcraft_plugins::safe_mode; + let folder = crate::tests_links::Folder::new("plugin-store-plain-put-back-fails-after-write"); + let id = "test.engine.permissions.plainputbacklate"; + 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"]})); + run(&mut s, "plugin.setEnabled", json!({"id": id, "enabled": false})); + let disabled = safe_mode::disabled(); + assert!(!disabled.is_empty()); + 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_AFTER_WRITE.set(true); + let installed = s.execute("plugin.install", &json!({"dataBase64": vectorcraft_format::base64_encode(&plain), "replace": true})); + crate::store::PUT_BACK_FAILS_AFTER_WRITE.set(false); + installed.unwrap(); + assert_eq!(std::fs::read(&file).unwrap(), approved, "the store holds the old module"); + assert_eq!(safe_mode::disabled(), disabled, "the plug-in stays disabled"); + let saved = std::fs::read(folder.0.join(crate::store::DISABLED_FILE)).unwrap(); + assert!(!safe_mode::parse_disabled_file(&saved).is_empty(), "the disabled list on disk is kept"); + + std::fs::remove_file(&grants).unwrap(); + run(&mut s, "plugin.remove", json!({"id": id})); + safe_mode::set_disabled(vec![]); +} + /// A first install whose grant can't be written, and whose module can't be taken out of the store /// again, stays in the store: the plug-in's file is turned on. No grant covers the module, so the /// next start skips it until it is approved.