Merge pull request #6747 from dotCooCoo/fix/plugin-grant-sealed-api

Plug-ins: only an approval records a grant, and installs need one

crates/plugins/src/effect.rs

@@ -552,7 +552,7 @@ mod tests {
         // Traps on a long input.
         let first = "(if (i32.gt_u (local.get $il) (i32.const 4096)) (then unreachable))";
         let src = crate::wat::with_imports(&manifest, "", first, r#"{"objects":[]}"#);
-        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap());
+        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap()).unwrap();
         let doc = Document::new(100.0, 100.0);
         let line = |n: usize| PathData::single(SubPath::polyline(&(0..n).map(|i| Point::new(i as f64, 0.0)).collect::<Vec<_>>(), false));
         let draw =
@@ -583,7 +583,7 @@ mod tests {
         // Traps on a long input.
         let first = "(if (i32.gt_u (local.get $il) (i32.const 4096)) (then unreachable))";
         let src = crate::wat::with_imports(&manifest, "", first, &output);
-        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap());
+        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap()).unwrap();
         let doc = Document::new(100.0, 100.0);
         let line = |n: usize| PathData::single(SubPath::polyline(&(0..n).map(|i| Point::new(i as f64, 0.0)).collect::<Vec<_>>(), false));
         let draw =
@@ -647,7 +647,7 @@ mod tests {
         crate::safe_mode::set_active(None);
         const ID: &str = "test.effect.budget";
         let src = crate::wat::returning(&crate::wat::manifest(ID, "effect", "{}"), r#"{"objects":[]}"#);
-        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap());
+        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap()).unwrap();
         // Each run takes 900 ms by this clock (it is read before and after each run).
         let view = crate::deferred::Deferrals::with_clock(stepping(450));
         let ran = view.view(|| (0..4).map(|i| drawn(ID, &line(i + 2), i)).collect::<Vec<_>>());
@@ -677,7 +677,7 @@ mod tests {
         const ID: &str = "test.effect.deferfailed";
         // Runs to its instruction budget.
         let src = crate::wat::spinning(&crate::wat::manifest(ID, "effect", "{}"), 1_000_000_000, r#"{"objects":[]}"#);
-        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap());
+        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap()).unwrap();
         let view = crate::deferred::Deferrals::with_clock(stepping(1));
         assert!(!view.view(|| drawn(ID, &line(2), 0)));
         assert!(last_error(ID).is_some());
@@ -712,9 +712,9 @@ mod tests {
         const FAST: &str = "test.effect.cutshort.fast";
         let src = crate::wat::spinning(&crate::wat::manifest(ID, "effect", "{}"), 10_000_000, r#"{"objects":[]}"#);
         let limits = Limits { fuel_base: 1_000_000_000, ..Limits::default() };
-        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), limits).unwrap());
+        registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), limits).unwrap()).unwrap();
         let fast = crate::wat::returning(&crate::wat::manifest(FAST, "effect", "{}"), r#"{"objects":[]}"#);
-        registry::install(crate::Plugin::load(&wat::parse_str(&fast).unwrap(), Limits::default()).unwrap());
+        registry::install(crate::Plugin::load(&wat::parse_str(&fast).unwrap(), Limits::default()).unwrap()).unwrap();
         // The first run starts the budget at 0 ms and ends at 1,999: the next has 1 ms left, and
         // ten million steps take longer.
         let reads = Arc::new(std::sync::atomic::AtomicU64::new(0));
@@ -737,7 +737,7 @@ mod tests {
         let _lock = crate::safe_mode::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner());
         crate::safe_mode::set_active(None);
         let src = crate::wat::spinning(&crate::wat::manifest("test.effect.cancelled", "effect", "{}"), 1_000_000_000, r#"{"objects":[]}"#);
-        let plugin = registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap());
+        let plugin = registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap()).unwrap();
         let ran_first =
             || RUNS.lock().unwrap_or_else(|e| e.into_inner()).as_ref().is_some_and(|r| matches!(r.get(&plugin.serial()), Some(Runs::Done)));
         let draw = || {
@@ -777,7 +777,7 @@ mod tests {
             crate::safe_mode::parse_marker(&std::fs::read(&json[0]).unwrap())
         };
         let src = crate::wat::returning(&crate::wat::manifest("test.effect.overlap", "effect", "{}"), r#"{"objects":[]}"#);
-        let plugin = registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap());
+        let plugin = registry::install(crate::Plugin::load(&wat::parse_str(&src).unwrap(), Limits::default()).unwrap()).unwrap();
         let named = Some(crate::safe_mode::Item::of(&plugin));
         assert_eq!(marker(), None);
 

crates/plugins/src/lib.rs

@@ -89,6 +89,10 @@ pub enum Error {
     /// Safe Mode is on ([`safe_mode`]): no plug-in loads or runs.
     #[error("Safe Mode is on: plug-ins don't load or run until VectorCraft starts normally")]
     SafeMode,
+    /// The module lists permissions that no recorded grant gives this very module
+    /// ([`registry::approved`]): it names them, comma-separated.
+    #[error("needs approval: it asks for {0}, which the user grants when installing it on its own")]
+    NeedsApproval(String),
     #[error("{0}")]
     Io(String),
 }

crates/plugins/src/permissions.rs

@@ -275,6 +275,32 @@ impl Grant {
     }
 }
 
+/// A [`Grant`] that [`crate::registry::grant`] records. The engine makes one where it records what
+/// a person approved and where it loads the grants kept in its plug-in folder; tests that set up a
+/// grant use the same constructors. `tests/grant_sites.rs` fails when other shipped code makes one.
+/// Native code linked into the app can still call these constructors, so the token does not
+/// protect against such code.
+#[derive(Debug)]
+pub struct GrantToken(Grant);
+
+impl GrantToken {
+    /// The grant the engine records after a person approved the module it covers, or in a session
+    /// whose starter allowed requests to grant permissions (`--allow-plugin-grants`).
+    pub fn after_engine_approval(grant: Grant) -> Self {
+        Self(grant)
+    }
+
+    /// A grant the engine read from the grants file of its plug-in folder, for a module it loads.
+    pub fn from_engine_grants_file(grant: Grant) -> Self {
+        Self(grant)
+    }
+
+    /// The grant it holds.
+    pub fn grant(&self) -> &Grant {
+        &self.0
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

crates/plugins/src/registry.rs

@@ -7,9 +7,8 @@ use std::collections::{BTreeMap, HashSet};
 use std::sync::atomic::{AtomicU64, Ordering};
 use std::sync::{Arc, Mutex, RwLock};
 
-#[cfg(not(target_arch = "wasm32"))]
 use crate::Error;
-use crate::permissions::Grant;
+use crate::permissions::{Grant, GrantToken};
 use crate::storage::Storage;
 use crate::{Limits, Plugin, Result};
 
@@ -35,8 +34,11 @@ pub fn storage(id: &str) -> Option<Arc<dyn Storage>> {
     STORAGE.read().unwrap_or_else(|e| e.into_inner()).get(id).cloned()
 }
 
-/// Records what the user approved for a plug-in, replacing an earlier grant for its id.
-pub fn grant(g: Grant) {
+/// Records what the user approved for a plug-in, replacing an earlier grant for its id. Only a
+/// [`GrantToken`] is recorded, which the engine makes after an approval or when it loads its grants
+/// file.
+pub fn grant(token: GrantToken) {
+    let g = token.grant().clone();
     GRANTS.write().unwrap_or_else(|e| e.into_inner()).insert(g.id.clone(), g);
 }
 
@@ -63,20 +65,27 @@ pub fn approved(plugin: &Plugin) -> bool {
     !m.asks_permissions() || granted(&m.id).is_some_and(|g| g.covers(&m.id, plugin.hash(), &m.permissions()))
 }
 
-/// Installs `plugin`, replacing one with the same id.
-pub fn install(mut plugin: Plugin) -> Arc<Plugin> {
+/// Installs `plugin`, replacing one with the same id, when it is [`approved`]: a module that lists
+/// permissions no recorded grant gives this very module is refused ([`Error::NeedsApproval`]), and
+/// the plug-in installed under its id, the grants and the revision stay as they were.
+pub fn install(mut plugin: Plugin) -> Result<Arc<Plugin>> {
     // The revision moves while the write lock is held, so a cache that reads the new revision and
-    // then the plug-ins finds the new plug-in among them.
+    // then the plug-ins finds the new plug-in among them. The approval is checked while it is held
+    // too.
     let mut plugins = PLUGINS.write().unwrap_or_else(|e| e.into_inner());
+    if !approved(&plugin) {
+        return Err(Error::NeedsApproval(crate::permissions::names(&plugin.manifest().permissions()).join(", ")));
+    }
     plugin.set_serial(REVISION.fetch_add(1, Ordering::Relaxed) + 1);
     let p = Arc::new(plugin);
     plugins.insert(p.id().to_string(), p.clone());
-    p
+    Ok(p)
 }
 
-/// Loads `bytes` with the default limits and installs the plug-in.
+/// 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<Arc<Plugin>> {
-    Ok(install(Plugin::load(bytes, Limits::default())?))
+    install(Plugin::load(bytes, Limits::default())?)
 }
 
 pub fn get(id: &str) -> Option<Arc<Plugin>> {
@@ -162,7 +171,8 @@ pub fn load_folder(dir: &std::path::Path) -> Result<FolderReport> {
 }
 
 /// [`load_folder`], loading only the files `allow` accepts, and of the plug-ins that list
-/// permissions only those `approve` accepts; the others are reported as failed with the reason. A plug-in the user disabled ([`crate::safe_mode::skipped`]) is reported and
+/// permissions only those `approve` accepts and a recorded grant covers ([`install`]); the others
+/// are reported as failed with the reason. A plug-in the user disabled ([`crate::safe_mode::skipped`]) is reported and
 /// skipped, by its file before it loads or by its id after (its file is then disabled too, so the
 /// next load skips it before it runs); the launch marker names each file while it loads
 /// ([`crate::safe_mode::loading`]). In Safe Mode nothing loads ([`Error::SafeMode`]). When the
@@ -206,12 +216,14 @@ pub fn load_folder_where(
             }
             Ok(p) if p.manifest().asks_permissions() && !approve(&p) => {
                 let asked = crate::permissions::names(&p.manifest().permissions()).join(", ");
-                report.failed.push((
-                    f.display().to_string(),
-                    format!("needs approval: it asks for {asked}, which the user grants when installing it on its own"),
-                ));
+                report.failed.push((f.display().to_string(), Error::NeedsApproval(asked).to_string()));
             }
-            Ok(p) => report.loaded.push(install(p).id().to_string()),
+            // `approve` only narrows what installs: `install` still refuses a module that no
+            // recorded grant covers.
+            Ok(p) => match install(p) {
+                Ok(p) => report.loaded.push(p.id().to_string()),
+                Err(e) => report.failed.push((f.display().to_string(), e.to_string())),
+            },
             Err(e) => report.failed.push((f.display().to_string(), e.to_string())),
         }
     }
@@ -231,6 +243,7 @@ mod tests {
     /// waiting for the set leaves the revision alone until it holds the set.
     #[test]
     fn an_install_changes_the_revision_only_while_it_holds_the_set() {
+        let _lock = crate::safe_mode::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner());
         let plugin = Plugin::load(include_bytes!("../tests/fixtures/desaturate.wasm"), Limits::default()).unwrap();
         let id = plugin.id().to_string();
         let held = PLUGINS.read().unwrap_or_else(|e| e.into_inner());
@@ -241,8 +254,95 @@ mod tests {
             revision() != before
         });
         drop(held);
-        installing.join().unwrap();
+        installing.join().unwrap().unwrap();
         assert!(!moved, "the revision changed while the install waited for the set");
         assert!(revision() > before && remove(&id));
     }
+
+    /// A module of a filter `id` whose manifest lists `permissions` (a JSON list, or nothing), with
+    /// `marker` in its code so that two markers give two SHA-256s.
+    fn module(id: &str, permissions: Option<&str>, marker: i64) -> Vec<u8> {
+        let listed = permissions.map(|p| format!(r#","permissions":{p}"#)).unwrap_or_default();
+        let manifest = format!(r#"{{"id":"{id}","name":"T","kind":"filter"{listed}}}"#);
+        wat::parse_str(crate::wat::module(&manifest, &format!("(i64.const {marker}) (drop) (i64.const 0)"), "", 1)).unwrap()
+    }
+
+    fn grant_for(id: &str, bytes: &[u8], permissions: Vec<crate::permissions::Permission>) -> Grant {
+        Grant { id: id.into(), hash: crate::permissions::sha256_hex(bytes), permissions }
+    }
+
+    /// A module that lists permissions installs only once a recorded grant gives this very module
+    /// all of them: refused, nothing changes, neither the revision nor the plug-in under its id.
+    #[test]
+    fn install_bytes_refuses_unapproved_manifest_permissions() {
+        // The other tests that install or remove plug-ins hold it, so the revision moves only here.
+        let _lock = crate::safe_mode::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner());
+        let id = "test.registry.unapproved";
+        let plain = install_bytes(&module(id, None, 1)).unwrap();
+        let before = revision();
+        let e = install_bytes(&module(id, Some(r#"["doc.read"]"#), 2)).unwrap_err();
+        assert_eq!(e, Error::NeedsApproval("doc.read".into()));
+        assert_eq!(revision(), before, "nothing installed");
+        assert_eq!(get(id).map(|p| p.hash().to_string()), Some(plain.hash().to_string()), "the plug-in before it stays");
+        assert!(granted(id).is_none());
+        assert!(remove(id));
+    }
+
+    /// `install` itself and a folder load whose `approve` accepts everything refuse the module too.
+    #[cfg(not(target_arch = "wasm32"))]
+    #[test]
+    fn install_and_folder_callback_cannot_bypass_approval() {
+        let _lock = crate::safe_mode::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner());
+        crate::safe_mode::set_active(None);
+        let id = "test.registry.bypass";
+        let plain = install_bytes(&module(id, Some("[]"), 3)).unwrap();
+        let asking = module(id, Some(r#"["doc.read","doc.write"]"#), 4);
+        let e = install(Plugin::load(&asking, Limits::default()).unwrap()).unwrap_err();
+        assert_eq!(e, Error::NeedsApproval("doc.read, doc.write".into()));
+        let dir = std::env::temp_dir().join(format!("vc-registry-bypass-{}", std::process::id()));
+        std::fs::create_dir_all(&dir).unwrap();
+        std::fs::write(dir.join("asking.wasm"), &asking).unwrap();
+        let r = load_folder_where(&dir, |_| Ok(()), |_| true).unwrap();
+        std::fs::remove_dir_all(&dir).unwrap();
+        assert!(r.loaded.is_empty(), "{r:?}");
+        assert_eq!(r.failed.len(), 1);
+        assert!(r.failed[0].1.starts_with("needs approval: it asks for doc.read, doc.write"), "{r:?}");
+        assert_eq!(get(id).map(|p| p.hash().to_string()), Some(plain.hash().to_string()), "the plug-in before it stays");
+        assert!(remove(id));
+    }
+
+    /// A recorded grant lets in only the module with its id and SHA-256 that lists nothing it
+    /// doesn't grant; a module whose manifest lists no permissions, or an empty list, needs none.
+    #[test]
+    fn matching_grant_allows_only_exact_bytes() {
+        use crate::permissions::Permission::{DocRead, DocWrite};
+        let _lock = crate::safe_mode::TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner());
+        let id = "test.registry.exact";
+        let bytes = module(id, Some(r#"["doc.read","doc.write"]"#), 5);
+        let plugin = Plugin::load(&bytes, Limits::default()).unwrap();
+        assert_eq!(plugin.hash(), crate::permissions::sha256_hex(&bytes), "a grant's hash is the loaded module's");
+        let refused = |g: Grant, why: &str| {
+            let other = g.id.clone();
+            revoke(id);
+            grant(GrantToken::after_engine_approval(g));
+            assert!(matches!(install_bytes(&bytes), Err(Error::NeedsApproval(_))), "{why}");
+            assert!(get(id).is_none(), "{why}");
+            revoke(&other);
+        };
+        refused(grant_for(id, &module(id, Some(r#"["doc.read","doc.write"]"#), 6), vec![DocRead, DocWrite]), "other bytes");
+        refused(grant_for("test.registry.other", &bytes, vec![DocRead, DocWrite]), "another id");
+        refused(Grant { id: id.into(), hash: "not a hash".into(), permissions: vec![DocRead, DocWrite] }, "a malformed hash");
+        refused(
+            Grant { id: id.into(), hash: crate::permissions::sha256_hex(&bytes).to_uppercase(), permissions: vec![DocRead, DocWrite] },
+            "an upper-case hash",
+        );
+        refused(grant_for(id, &bytes, vec![DocRead]), "a missing permission");
+        grant(GrantToken::from_engine_grants_file(grant_for(id, &bytes, vec![DocRead, DocWrite])));
+        assert_eq!(install_bytes(&bytes).unwrap().hash(), plugin.hash());
+        assert!(remove(id) && revoke(id));
+        for listed in [None, Some("[]")] {
+            assert!(install_bytes(&module(id, listed, 7)).is_ok(), "{listed:?}");
+            assert!(granted(id).is_none() && remove(id), "{listed:?}");
+        }
+    }
 }