diff --git a/crates/engine/src/cmd/plugin.rs b/crates/engine/src/cmd/plugin.rs index dfa44a1436..3fa082bdb0 100644 --- a/crates/engine/src/cmd/plugin.rs +++ b/crates/engine/src/cmd/plugin.rs @@ -672,13 +672,53 @@ fn takes_over(s: &Session, plugin: &Plugin) -> bool { } } +#[cfg(test)] +thread_local! { + /// Tests: runs once in the next `plugin.install` on this thread after its checks and before it + /// installs, as another session changing the plug-ins in between would. + pub(crate) static BEFORE_INSTALL: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; + /// Tests: runs once in the next `plugin.install` on this thread right after it reads the grant + /// held for the id. + pub(crate) static AFTER_GRANT_READ: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; + /// Tests: runs once in the next `plugin.install` on this thread after it installs and before it + /// writes the store. + pub(crate) static BEFORE_STORE: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; + /// Tests: runs once on this thread when [`hold_plugin_changes`] finds the lock held and waits. + pub(crate) static WHILE_WAITING: std::cell::RefCell>> = const { std::cell::RefCell::new(None) }; +} + +/// Held by `plugin.install` from its first read of the grant until the store holds what it +/// installed, and throughout by `plugin.remove`, by loads of the stored plug-ins +/// ([`Session::load_stored_plugins`]) and by loads of a plug-ins folder: these run one after the +/// other across sessions. +static PLUGIN_CHANGES: Mutex<()> = Mutex::new(()); + +/// Takes [`PLUGIN_CHANGES`], waiting while another session holds it. +pub(crate) fn hold_plugin_changes() -> std::sync::MutexGuard<'static, ()> { + match PLUGIN_CHANGES.try_lock() { + Ok(held) => held, + Err(std::sync::TryLockError::Poisoned(e)) => e.into_inner(), + Err(std::sync::TryLockError::WouldBlock) => { + #[cfg(test)] + if let Some(waiting) = WHILE_WAITING.take() { + waiting(); + } + PLUGIN_CHANGES.lock().unwrap_or_else(|e| e.into_inner()) + } + } +} + /// `plugin.install`: loads and checks the module, refuses `grant` and `allowHeavy` that neither an /// [`InstallApproval`] for this module nor the session allows, one that asks for a heavy budget /// unless `allowHeavy` says a person agreed, one that lists permissions no person has granted this /// very module unless `grant` lists them all, one that would take another module's place -/// ([`takes_over`]) unless `replace` is true, and one whose SHA-256 isn't the `hash` given; records -/// the grant with the module's hash, keeps it for the next start and installs it → its -/// `plugin.list` entry. A module that lists no permissions ends any grant held for its id. +/// ([`takes_over`]) unless `replace` is true, and one whose SHA-256 isn't the `hash` given. It then +/// checks the plug-in installed under the id and its grant again while it records the grant with +/// the module's hash and installs it ([`registry::install_checked`]), and only then keeps both for +/// the next start → its `plugin.list` entry. A refused install changes neither the plug-in store +/// nor the plug-ins and grants of the process. A module that lists no permissions ends any grant +/// held for its id. A `plugin.remove` in another session waits until the install has written the +/// store. fn install(s: &mut Session, p: &Value) -> Result { const C: &str = "plugin.install"; // One use: spent whatever this install does. @@ -696,21 +736,36 @@ fn install(s: &mut Session, p: &Value) -> Result { { return Err(bad(C, "the module doesn't have the SHA-256 `hash` gives: nothing was installed")); } - if replace == Some(false) && registry::get(plugin.id()).is_some() { - return Err(bad(C, format!("a plug-in with id {:?} is already installed", plugin.id()))); + // A removal in another session waits until the store holds what this install wrote. + let _held = hold_plugin_changes(); + // The grant the checks below count on, the approval included. The install checks it again, + // with the plug-in installed under the id, before anything is stored: another session may + // change either in between. + let seen_grant = registry::granted(plugin.id()); + #[cfg(test)] + if let Some(between) = AFTER_GRANT_READ.take() { + between(); } - // Another module under the id gets the settings and cache kept for it: whoever installs it asks - // the user first. - if replace != Some(true) && takes_over(s, &plugin) { + let installed_already = || bad(C, format!("a plug-in with id {:?} is already installed", plugin.id())); + let taken_over = || { let name = permissions::display_text(&plugin.manifest().name, permissions::MAX_SHOWN_NAME); - return Err(bad( + bad( C, format!( "“{name}” is another module than the plug-in with id {:?} and would get the settings and cache kept for it: install it with `replace: true` once the user agrees", plugin.id() ), - )); + ) + }; + if replace == Some(false) && registry::get(plugin.id()).is_some() { + return Err(installed_already()); } + // Another module under the id gets the settings and cache kept for it: whoever installs it asks + // the user first. + if replace != Some(true) && takes_over(s, &plugin) { + return Err(taken_over()); + } + let (installed_already, taken_over) = (installed_already(), taken_over()); // `grant` and `allowHeavy` say that a person agreed, and a request alone can't say that. let approval = approval.filter(|a| a.covers(&plugin)); if !s.plugin_grants_from_requests && ((grant.is_some() && approval.is_none()) || (allow_heavy && !approval.is_some_and(|a| a.heavy))) { @@ -744,15 +799,55 @@ fn install(s: &mut Session, p: &Value) -> Result { // this very module already covers all of them. let m = plugin.manifest(); let wanted = m.permissions(); - if !s.plugin_approved(&plugin) && !grant.as_ref().is_some_and(|g| wanted.iter().all(|w| g.contains(w))) { + let needs_approval = || { let name = permissions::display_text(&m.name, permissions::MAX_SHOWN_NAME); let asked = permissions::names(&wanted).join(", "); - return Err(bad( + bad( C, format!( "“{name}” asks for the permissions {asked}: a person has to approve them in the desktop app, or pass `grant` in a session started with --allow-plugin-grants" ), - )); + ) + }; + let granted_here = grant.as_ref().is_some_and(|g| wanted.iter().all(|w| g.contains(w))); + // Without a plug-in store, the grant read above approves the module or doesn't. + let approved = match s.plugin_dir { + None => !m.asks_permissions() || seen_grant.as_ref().is_some_and(|g| g.covers(&m.id, plugin.hash(), &wanted)), + Some(_) => s.plugin_approved(&plugin), + }; + if !approved && !granted_here { + return Err(needs_approval()); + } + let needs_approval = needs_approval(); + let (id, hash, asks) = (m.id.clone(), plugin.hash().to_string(), m.asks_permissions()); + let g = Grant { id: id.clone(), hash: hash.clone(), permissions: wanted }; + let update = if asks { registry::GrantUpdate::Record(GrantToken::after_engine_approval(g.clone())) } else { registry::GrantUpdate::Revoke }; + // Another module installed under the id since the checks above takes the place they found free, + // or held by these bytes. A grant that changed since no longer approves what they approved, + // unless `grant` approves this module by itself or the new grant covers it. + let check = |installed: Option<&Plugin>, held: Option<&Grant>| { + if let Some(installed) = installed { + match replace { + Some(false) => return Err(installed_already), + None if installed.hash() != hash => return Err(taken_over), + _ => {} + } + } + if asks && !granted_here && held != seen_grant.as_ref() && !held.is_some_and(|h| h.covers(&id, &hash, &g.permissions)) { + return Err(needs_approval); + } + Ok(()) + }; + #[cfg(test)] + if let Some(between) = BEFORE_INSTALL.take() { + between(); + } + // The plug-in gets the settings and cache kept for its id. A module without permissions takes + // the id over, so nothing the user approved for the id applies any more. + let installed = registry::install_checked(plugin, update, check, || s.attach_plugin_storage(&id), err)?; + #[cfg(test)] + if let Some(between) = BEFORE_STORE.take() { + between(); } // 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 @@ -760,18 +855,14 @@ fn install(s: &mut Session, p: &Value) -> Result { // 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(GrantToken::after_engine_approval(g.clone())); - if stored && !s.store_grant(&m.id, Some(g)) { - stored = !s.put_back_plugin(plugin.id(), previous.ok().flatten().as_deref()) && holds_new(s); + let previous = s.stored_plugin_bytes(&id); + let mut stored = (previous.is_ok() || !asks) && s.store_plugin(&id, &bytes); + let holds_new = |s: &Session| s.stored_plugin_bytes(&id).is_ok_and(|held| held.as_deref() == Some(&bytes[..])); + if asks { + if stored && !s.store_grant(&id, Some(g)) { + stored = !s.put_back_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. - 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 module can't be read or written back, or there @@ -780,23 +871,21 @@ fn install(s: &mut Session, p: &Value) -> Result { // approves nothing, and the new module stays; one that can't be opened or read may approve // the old bytes once it reads again. let writes = s.grant_writes(); - 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())); + if stored && !s.store_grant(&id, None) && s.holds_plugin_grant(&id) { + let back = previous.as_ref().is_ok_and(|held| s.put_back_plugin(&id, held.as_deref())); stored = !back && holds_new(s); if !back || matches!(previous, Ok(None)) { - s.ignore_stored_grant(&m.id, writes); + s.ignore_stored_grant(&id, writes); } } } - // It gets the settings and cache kept for its id. - s.attach_plugin_storage(plugin.id()); // Installing a disabled plug-in again (an updated version) turns it on for the next launch too, // also when it is disabled by its stored file alone (it stopped a launch while loading). When // the new version could not replace the stored file, the old one stays disabled. - let file = s.stored_plugin_file(plugin.id()); + let file = s.stored_plugin_file(&id); let mut changed = false; if file.is_none() || stored { - changed = safe_mode::set_enabled(&safe_mode::Item::of(&plugin), true); + changed = safe_mode::set_enabled(&safe_mode::Item::of(&installed), true); if let Some(file) = file { changed |= safe_mode::set_enabled(&safe_mode::Item::file(file), true); } @@ -804,7 +893,6 @@ fn install(s: &mut Session, p: &Value) -> Result { if changed { s.store_disabled_plugins(); } - let installed = registry::install(plugin).map_err(err)?; let mut v = describe_installed(&installed); if !unknown.is_empty() { v["unknownCommands"] = json!(unknown); @@ -904,6 +992,8 @@ fn load_folder(s: &Session, folder: &str) -> Value { None if crate::file_access::current().is_none() => Ok(()), None => Err(format!("automation path rejected: {} isn't valid text", f.display())), }; + // A `plugin.install` in another session finishes writing the store first. + let _held = hold_plugin_changes(); match registry::load_folder_where(std::path::Path::new(folder), allow, s.plugin_folder_approver()) { Ok(r) => { for id in &r.loaded { @@ -1074,6 +1164,8 @@ fn info(_: &mut Session, p: &Value) -> Result { fn remove(s: &mut Session, p: &Value) -> Result { let id = str_param(p, "id").ok_or_else(|| bad("plugin.remove", "missing `id`"))?; + // An install in another session finishes its store writes first. + let _held = hold_plugin_changes(); 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); diff --git a/crates/engine/src/store.rs b/crates/engine/src/store.rs index 4943353ee0..8a18ce9824 100644 --- a/crates/engine/src/store.rs +++ b/crates/engine/src/store.rs @@ -421,6 +421,8 @@ impl Session { if safe_mode::active() { return; } + // A `plugin.install` in another session writes the store before this reads it. + let _held = crate::cmd::plugin::hold_plugin_changes(); let grants = self.counted_grants(&dir); let limits = vectorcraft_plugins::Limits::default(); let mut left = MAX_PLUGIN_STORE; diff --git a/crates/engine/src/tests/plugins.rs b/crates/engine/src/tests/plugins.rs index cc4460d250..2661e85787 100644 --- a/crates/engine/src/tests/plugins.rs +++ b/crates/engine/src/tests/plugins.rs @@ -2820,6 +2820,188 @@ fn a_failed_store_write_keeps_the_stored_module_with_its_grant() { vectorcraft_plugins::registry::revoke(id); } +/// An install refused because another session revoked the grant its checks relied on writes +/// nothing: a store that kept the grant and lost the module gets no module back, its grants stay as +/// they were, and the plug-in installed with the id and the other session's revoke stand. `grant` +/// approves the module by itself. +#[test] +fn a_grant_revoked_while_installing_leaves_the_store_as_it_was() { + use vectorcraft_plugins::permissions::sha256_hex; + use vectorcraft_plugins::registry; + let folder = crate::tests_links::Folder::new("plugin-store-revoked-while-installing"); + let id = "test.engine.permissions.revokedwhileinstalling"; + let bytes = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let data = vectorcraft_format::base64_encode(&bytes); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + run(&mut s, "plugin.install", json!({"dataBase64": data, "grant": ["doc.read"]})); + let grants = folder.0.join("grants.json"); + let recorded = std::fs::read(&grants).unwrap(); + let module = std::path::PathBuf::from(s.stored_plugin_file(id).unwrap()); + std::fs::remove_file(&module).unwrap(); + + crate::cmd::plugin::BEFORE_INSTALL.set(Some(Box::new(move || { + registry::revoke(id); + }))); + let e = s.execute("plugin.install", &json!({"dataBase64": data})).unwrap_err().to_string(); + assert!(e.contains("asks for the permissions doc.read"), "{e}"); + assert!(!module.exists(), "no module is stored"); + assert_eq!(std::fs::read(&grants).unwrap(), recorded, "the grants stay as they were"); + assert!(registry::granted(id).is_none(), "the other session's revoke stands"); + assert_eq!(registry::get(id).map(|p| p.hash().to_string()), Some(sha256_hex(&bytes)), "the installed plug-in stays"); + + crate::cmd::plugin::BEFORE_INSTALL.set(Some(Box::new(move || { + registry::revoke(id); + }))); + run(&mut s, "plugin.install", json!({"dataBase64": data, "grant": ["doc.read"]})); + assert_eq!(std::fs::read(&module).unwrap(), bytes, "a module `grant` approves is stored"); + assert!(registry::granted(id).is_some_and(|g| g.hash == sha256_hex(&bytes))); + run(&mut s, "plugin.remove", json!({"id": id})); +} + +/// A plug-in another session installs with the id while a module is checked takes the place the +/// checks found free. Without `replace`, or with `replace: false`, the install is refused, writes no +/// module and leaves that plug-in installed; with `replace: true` the module takes its place. +#[test] +fn a_plug_in_installed_while_installing_needs_replace() { + use vectorcraft_plugins::permissions::sha256_hex; + use vectorcraft_plugins::registry; + let folder = crate::tests_links::Folder::new("plugin-store-installed-while-installing"); + let id = "test.engine.installedwhileinstalling"; + let ours = asking(id, "[]", &json!({"objects": []})); + let theirs = asking(id, "[]", &json!({"objects": [], "documentData": null})); + let data = vectorcraft_format::base64_encode(&ours); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + let module = std::path::PathBuf::from(s.stored_plugin_file(id).unwrap()); + let installed = || registry::get(id).map(|p| p.hash().to_string()); + let meanwhile = |theirs: Vec| -> Option> { + Some(Box::new(move || { + registry::install_bytes(&theirs).unwrap(); + })) + }; + + for (replace, why) in [(Value::Null, "is another module than"), (json!(false), "is already installed")] { + crate::cmd::plugin::BEFORE_INSTALL.set(meanwhile(theirs.clone())); + let e = s.execute("plugin.install", &json!({"dataBase64": data, "replace": replace})).unwrap_err().to_string(); + assert!(e.contains(why), "{replace}: {e}"); + assert!(!module.exists(), "{replace}: no module is stored"); + assert_eq!(installed(), Some(sha256_hex(&theirs)), "{replace}: the other plug-in stays installed"); + registry::remove(id); + } + + crate::cmd::plugin::BEFORE_INSTALL.set(meanwhile(theirs)); + run(&mut s, "plugin.install", json!({"dataBase64": data, "replace": true})); + assert_eq!(std::fs::read(&module).unwrap(), ours); + assert_eq!(installed(), Some(sha256_hex(&ours))); + run(&mut s, "plugin.remove", json!({"id": id})); +} + +/// Without a plug-in store, the install's approval comes from the grant it read first, the one it +/// checks again before it installs: a grant another session records after that read approves +/// nothing, so a revoke that follows it can't be undone by the install recording it again. +#[test] +fn a_grant_recorded_and_revoked_while_installing_approves_nothing() { + use vectorcraft_plugins::permissions::{Grant, GrantToken, Permission, sha256_hex}; + use vectorcraft_plugins::registry; + let id = "test.engine.permissions.recordedwhileinstalling"; + let bytes = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let data = vectorcraft_format::base64_encode(&bytes); + let mut s = session(); + let g = Grant { id: id.into(), hash: sha256_hex(&bytes), permissions: vec![Permission::DocRead] }; + crate::cmd::plugin::AFTER_GRANT_READ.set(Some(Box::new(move || { + registry::grant(GrantToken::after_engine_approval(g)); + }))); + let e = s.execute("plugin.install", &json!({"dataBase64": data})).unwrap_err().to_string(); + assert!(e.contains("asks for the permissions doc.read"), "{e}"); + assert!(registry::get(id).is_none(), "nothing is installed"); + registry::revoke(id); +} + +/// A `plugin.remove` in another session while a module is installed waits until the install has +/// written the store, then removes the module, its grant and the plug-in: nothing of it stays in +/// the store for the next launch. +#[test] +fn a_removal_while_installing_leaves_nothing_in_the_store() { + use vectorcraft_plugins::registry; + let folder = crate::tests_links::Folder::new("plugin-store-removed-while-installing"); + let id = "test.engine.permissions.removedwhileinstalling"; + let bytes = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let data = vectorcraft_format::base64_encode(&bytes); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + let module = std::path::PathBuf::from(s.stored_plugin_file(id).unwrap()); + let grants = folder.0.join("grants.json"); + + let (said, heard) = std::sync::mpsc::channel::<&'static str>(); + let (handed, remover) = std::sync::mpsc::channel(); + let dir = folder.0.clone(); + crate::cmd::plugin::BEFORE_STORE.set(Some(Box::new(move || { + let thread = std::thread::spawn(move || { + let tell = said.clone(); + crate::cmd::plugin::WHILE_WAITING.set(Some(Box::new(move || { + let _ = tell.send("waiting"); + }))); + let mut other = session(); + other.attach_plugin_store(dir); + let removed = other.execute("plugin.remove", &json!({"id": id})).map(|_| ()).map_err(|e| e.to_string()); + let _ = said.send("removed"); + removed + }); + // The removal has finished, or waits for this install. + let first = heard.recv().unwrap_or("gone"); + let _ = handed.send((first, thread)); + }))); + run(&mut s, "plugin.install", json!({"dataBase64": data, "grant": ["doc.read"]})); + let (first, thread) = remover.recv().unwrap(); + assert_eq!(thread.join().unwrap(), Ok(())); + assert!(registry::get(id).is_none() && registry::granted(id).is_none(), "the plug-in and its grant are gone"); + assert!(!module.exists(), "no module stays in the store"); + assert!(!std::fs::read_to_string(&grants).is_ok_and(|g| g.contains(id)), "no grant stays in the store"); + assert_eq!(first, "waiting", "the removal waits for the install"); +} + +/// Another session that attaches the plug-in store while a new version is installed loads the +/// stored plug-ins once the install has written the store: the version that runs is the one the +/// store keeps for the next launch. +#[test] +fn a_store_loaded_while_installing_loads_the_new_version() { + use vectorcraft_plugins::permissions::sha256_hex; + use vectorcraft_plugins::registry; + let folder = crate::tests_links::Folder::new("plugin-store-loaded-while-installing"); + let id = "test.engine.permissions.loadedwhileinstalling"; + let old = asking(id, r#"["doc.read"]"#, &json!({"objects": []})); + let new = asking(id, r#"["doc.read"]"#, &json!({"objects": [], "documentData": null})); + let mut s = session(); + s.attach_plugin_store(folder.0.clone()); + run(&mut s, "plugin.install", json!({"dataBase64": vectorcraft_format::base64_encode(&old), "grant": ["doc.read"]})); + + let (said, heard) = std::sync::mpsc::channel::<&'static str>(); + let (handed, loader) = std::sync::mpsc::channel(); + let dir = folder.0.clone(); + crate::cmd::plugin::BEFORE_STORE.set(Some(Box::new(move || { + let thread = std::thread::spawn(move || { + let tell = said.clone(); + crate::cmd::plugin::WHILE_WAITING.set(Some(Box::new(move || { + let _ = tell.send("waiting"); + }))); + session().attach_plugin_store(dir); + let _ = said.send("loaded"); + }); + // The load has finished, or waits for this install. + let first = heard.recv().unwrap_or("gone"); + let _ = handed.send((first, thread)); + }))); + let data = vectorcraft_format::base64_encode(&new); + run(&mut s, "plugin.install", json!({"dataBase64": data, "grant": ["doc.read"], "replace": true})); + let (first, thread) = loader.recv().unwrap(); + thread.join().unwrap(); + assert_eq!(registry::get(id).map(|p| p.hash().to_string()), Some(sha256_hex(&new)), "the new version runs"); + assert_eq!(std::fs::read(s.stored_plugin_file(id).unwrap()).unwrap(), new, "the store keeps the new version"); + assert_eq!(first, "waiting", "the load waits for the install"); + run(&mut s, "plugin.remove", json!({"id": id})); +} + /// Runs the test `name` of this module again in a process of its own → whether this is that /// process. The list of disabled plug-ins is kept for the whole process, and the other tests here /// read it again each time they attach a plug-in store. diff --git a/crates/plugins/src/registry.rs b/crates/plugins/src/registry.rs index ee3732908c..1178f7c9ac 100644 --- a/crates/plugins/src/registry.rs +++ b/crates/plugins/src/registry.rs @@ -82,6 +82,57 @@ pub fn install(mut plugin: Plugin) -> Result> { Ok(p) } +/// What [`install_checked`] does to the grant recorded for the plug-in's id. +pub enum GrantUpdate { + /// Records this grant, replacing an earlier one for the id ([`grant`]). + Record(GrantToken), + /// Forgets the grant for the id ([`revoke`]). + Revoke, +} + +/// Installs `plugin` and updates the grant for its id as one step. While it holds the plug-ins and +/// then the grants, `check` is given the plug-in installed under the id and the grant recorded for +/// it. When `check` returns an error, nothing changes and that error is returned. A plug-in that +/// lists permissions the grant `update` records doesn't give this very module is refused the same +/// way, with [`Error::NeedsApproval`] passed through `refused`. Otherwise `attach` runs, the grant +/// is updated and the plug-in is installed, replacing one with the same id. `check` and `attach` +/// must not call the other functions of this module, which wait for the locks held here. +pub fn install_checked( + mut plugin: Plugin, + update: GrantUpdate, + check: impl FnOnce(Option<&Plugin>, Option<&Grant>) -> std::result::Result<(), E>, + attach: impl FnOnce(), + refused: impl FnOnce(Error) -> E, +) -> std::result::Result, E> { + // The same order as `install`, which reads the grants while it holds the plug-ins. + let mut plugins = PLUGINS.write().unwrap_or_else(|e| e.into_inner()); + let mut grants = GRANTS.write().unwrap_or_else(|e| e.into_inner()); + let id = plugin.id().to_string(); + check(plugins.get(&id).map(|p| &**p), grants.get(&id))?; + let m = plugin.manifest(); + let granted = match &update { + GrantUpdate::Record(token) => Some(token.grant()), + GrantUpdate::Revoke => None, + }; + if m.asks_permissions() && !granted.is_some_and(|g| g.covers(&m.id, plugin.hash(), &m.permissions())) { + return Err(refused(Error::NeedsApproval(crate::permissions::names(&m.permissions()).join(", ")))); + } + attach(); + match update { + GrantUpdate::Record(token) => { + let g = token.grant().clone(); + grants.insert(g.id.clone(), g); + } + GrantUpdate::Revoke => { + grants.remove(&id); + } + } + plugin.set_serial(REVISION.fetch_add(1, Ordering::Relaxed) + 1); + let p = Arc::new(plugin); + plugins.insert(id, p.clone()); + Ok(p) +} + /// Loads `bytes` with the default limits and installs the plug-in ([`install`]: one that lists /// permissions installs only when they were granted to this very module). pub fn install_bytes(bytes: &[u8]) -> Result> { @@ -345,4 +396,61 @@ mod tests { assert!(granted(id).is_none() && remove(id), "{listed:?}"); } } + + /// `install_checked` gives `check` the plug-in and grant held for the id. When `check` refuses, + /// or the grant it would record doesn't approve the module, the plug-in, the grant and the + /// revision stay as they were and `attach` doesn't run; otherwise it records the grant (or + /// forgets it) and installs the module. + #[test] + fn install_checked_changes_nothing_when_refused() { + use crate::permissions::Permission::DocRead; + let _lock = crate::safe_mode::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let id = "test.registry.checked"; + let old = module(id, Some(r#"["doc.read"]"#), 8); + let new = module(id, Some(r#"["doc.read"]"#), 9); + grant(GrantToken::after_engine_approval(grant_for(id, &old, vec![DocRead]))); + install_bytes(&old).unwrap(); + let load = |bytes: &[u8]| Plugin::load(bytes, Limits::default()).unwrap(); + let record = |bytes: &[u8]| GrantUpdate::Record(GrantToken::after_engine_approval(grant_for(id, bytes, vec![DocRead]))); + let unchanged = |why: &str| { + assert_eq!(get(id).map(|p| p.hash().to_string()), Some(crate::permissions::sha256_hex(&old)), "{why}"); + assert_eq!(granted(id), Some(grant_for(id, &old, vec![DocRead])), "{why}"); + }; + let attached = std::cell::Cell::new(false); + let before = revision(); + + let seen = std::cell::RefCell::new(None); + let e = install_checked( + load(&new), + record(&new), + |p, g| { + *seen.borrow_mut() = Some((p.map(|p| p.hash().to_string()), g.cloned())); + Err("refused") + }, + || attached.set(true), + |_| "not approved", + ) + .unwrap_err(); + assert_eq!(e, "refused"); + assert_eq!(seen.take(), Some((Some(crate::permissions::sha256_hex(&old)), Some(grant_for(id, &old, vec![DocRead]))))); + unchanged("check refused"); + + let same = std::convert::identity::; + let e = install_checked(load(&new), record(&old), |_, _| Ok(()), || attached.set(true), same).unwrap_err(); + assert_eq!(e, Error::NeedsApproval("doc.read".into())); + unchanged("a grant for other bytes"); + let e = install_checked(load(&new), GrantUpdate::Revoke, |_, _| Ok(()), || attached.set(true), same).unwrap_err(); + assert_eq!(e, Error::NeedsApproval("doc.read".into())); + unchanged("no grant"); + assert!(!attached.get() && revision() == before, "nothing ran or moved"); + + let p = install_checked(load(&new), record(&new), |_, _| Ok(()), || attached.set(true), same).unwrap(); + assert!(attached.get() && revision() > before); + assert_eq!(get(id).map(|p| p.hash().to_string()), Some(p.hash().to_string())); + assert_eq!(granted(id), Some(grant_for(id, &new, vec![DocRead]))); + let plain = module(id, Some("[]"), 10); + install_checked(load(&plain), GrantUpdate::Revoke, |_, _| Ok(()), || {}, same).unwrap(); + assert!(granted(id).is_none(), "a module without permissions forgets the grant"); + assert!(remove(id)); + } } diff --git a/crates/plugins/tests/grant_sites.rs b/crates/plugins/tests/grant_sites.rs index d0c0479337..a4bdb4b12b 100644 --- a/crates/plugins/tests/grant_sites.rs +++ b/crates/plugins/tests/grant_sites.rs @@ -1,7 +1,7 @@ //! The shipped code that records a plug-in grant: only the engine's install approval and its load -//! of the plug-in folder's grants file make a `GrantToken` and call `registry::grant`. A new call -//! site fails this test until it is added to the list. It checks the source, not what native code -//! can do at run time. +//! of the plug-in folder's grants file make a `GrantToken` and call `registry::grant` or +//! `registry::install_checked`. A new call site fails this test until it is added to the list. It +//! checks the source, not what native code can do at run time. #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use std::collections::BTreeMap; @@ -11,10 +11,13 @@ use std::path::{Path, PathBuf}; const EXPECTED: &[(&str, &str, usize)] = &[ ("GrantToken::after_engine_approval", "crates/engine/src/cmd/plugin.rs", 1), ("GrantToken::from_engine_grants_file", "crates/engine/src/store.rs", 2), - ("registry::grant", "crates/engine/src/cmd/plugin.rs", 1), ("registry::grant", "crates/engine/src/store.rs", 2), + ("registry::install_checked", "crates/engine/src/cmd/plugin.rs", 1), ]; +/// The names the audit counts. +const NAMES: [&str; 4] = ["GrantToken::after_engine_approval", "GrantToken::from_engine_grants_file", "registry::grant", "registry::install_checked"]; + /// Every `.rs` file under `dir`, without test files and folders. fn sources(dir: &Path, out: &mut Vec) { let Ok(entries) = std::fs::read_dir(dir) else { return }; @@ -61,7 +64,7 @@ fn grant_token_call_sites_are_limited() { let text = std::fs::read_to_string(file).unwrap(); let rel = file.strip_prefix(&root).unwrap().to_string_lossy().replace('\\', "/"); for line in shipped(&text) { - for name in ["GrantToken::after_engine_approval", "GrantToken::from_engine_grants_file", "registry::grant"] { + for name in NAMES { let n = count(line, name); if n > 0 { *found.entry((name, rel.clone())).or_default() += n; diff --git a/docs/plugins.md b/docs/plugins.md index 124cc76174..a932983acd 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -264,7 +264,13 @@ 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 an install of a module that lists no permissions can't + tries again. A refused install leaves the grant, the plug-in installed with the id, the `Plug-ins` folder and the + list of disabled plug-ins as they are. When another session installs a plug-in with the id or changes its grant + while `plugin.install` checks a module, the install is refused, before anything is written to the `Plug-ins` folder, + if the other plug-in would need `replace: true` or the changed grant no longer approves the module. A + `plugin.remove` in another session waits until the install has written the `Plug-ins` folder, and then removes the + module and its grant. Loading the `Plug-ins` folder or a plug-ins folder in another session waits for it too. 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 or written back, the new module stays in the folder, and diff --git a/docs/target-app-parity.md b/docs/target-app-parity.md index 6f12d24707..3f243b1385 100644 --- a/docs/target-app-parity.md +++ b/docs/target-app-parity.md @@ -1816,6 +1816,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur - **Plug-in approval needs a person (G17):** a plug-in's permissions and heavy budget are approved in a dialog that only a person can confirm: the control channel, MCP and injected input are refused, and `plugin.install` takes `grant` or `allowHeavy` only with that approval. `vectorcraft-cli mcp` and `run` refuse them unless started with `--allow-plugin-grants`. Regressions: `crates/engine/src/tests/plugins.rs`, `crates/ui-egui/src/tests/automation.rs`, `apps/vectorcraft-cli/tests/it/cli.rs`. Scores and hours unchanged. - **A dialog's OK runs what it was opened for:** `ui.dialog.set`, and MCP's `answer_dialog` through it, can set a `__` field only when it is a control of that kind of dialog (a section or tab shown, a link between two fields, a name or search being typed). Every other `__` field, such as the `__command` and `__params` of a confirmation or the generic parameter dialog, a plug-in command dialog's `__command` and `__plugin`, or the paste a swatch conflict waits to finish, is refused with "the `` dialog cannot set protected field ``"; the request sets no field, and with `action: "confirm"` OK does not run. Regressions: `crates/ui-egui/src/tests/automation.rs`, `crates/ui-egui/src/control/tests.rs`. Scores and hours unchanged. - **Only an approval records a plug-in grant (G17):** `vectorcraft_plugins::registry::grant` takes a `GrantToken`, which the engine makes after an approval and when it loads `grants.json`, and `registry::install`, `install_bytes` and folder loads refuse a module that lists permissions no recorded grant gives this very module (`Error::NeedsApproval`), leaving the plug-in under its id and the revision as they were. A module without `permissions`, or with an empty list, installs as before. `docs/plugins.md` says what the approval does not protect against (native code linked into the app, another program running as the same user, the unsigned `grants.json`). Regressions: `crates/plugins/src/registry.rs`, `crates/plugins/tests/grant_sites.rs`, `crates/engine/src/tests/plugins.rs`. Scores and hours unchanged. +- **A refused plug-in install writes nothing (G17):** `plugin.install` checks the plug-in installed under the module's id and its grant again while it records the grant and installs the module (`registry::install_checked`), and only then writes the module and its grant to the `Plug-ins` folder, attaches its settings and cache and turns a disabled plug-in on. When another session installed a plug-in with the id that would need `replace: true`, or changed the grant the checks relied on, the install is refused and the folder, `grants.json`, the disabled list and the installed plug-in stay as they were. Without a plug-in store, the approval comes from the grant the install checks again. A `plugin.remove`, or a load of the folder or of a plug-ins folder, in another session waits until the install has written the folder. VectorCraft's own rule. Regressions: `crates/engine/src/tests/plugins.rs`. Scores and hours unchanged. - **Plug-in approval waits before OK (G17):** the approval dialog's OK stays disabled until its question has been on screen in the focused window for half a second, VectorCraft's own value; a click, Enter, Space, or a press begun before then installs nothing, and the dialog stays open. The wait starts over for every new question, even one that reads the same, and when the window gets the focus back, so a question swapped in just before a click is not approved by it. Cancel works at once. Regressions: `crates/ui-egui/src/tests/plugins.rs`. Scores and hours unchanged. - **An install automation asked for needs the person to start it (G17):** an approval question that a control request, MCP, injected input or a file dialog one of them opened leads to keeps OK disabled and refuses every confirm, a person's click included; it says to choose Object › Plug-ins › Install Plug-in…, and `ui.installPlugin` answers `approval: "personMustStartInstall"`. The person's own Install Plug-in…, File › Open or drop inspects the module again and asks a new question. Automation's `replace: true` no longer skips the question about taking another module's place, and that question now opens in the approval dialog for everyone. VectorCraft's own rule. Regressions: `crates/ui-egui/src/tests/automation.rs`, `crates/ui-egui/src/tests/plugins.rs`. Scores and hours unchanged. - **A plug-in with no permissions ends its id's grant (G17):** installing a module that lists no permissions under a granted id forgets the grant, in the process and in `grants.json`, so the formerly approved bytes need a new approval. Attaching a `Plug-ins` folder whose module for that id lists no permissions forgets it too, and plug-in folders forget the process's grant. A refused install leaves the grant. Regressions: `crates/engine/src/tests/plugins.rs`, `crates/engine/tests/plugin_safe_mode.rs`. Scores and hours unchanged.