Merge pull request #1920 from storytold/fix/sec-plugin-policy

fix(engine): keep plugins out of branches, saved versions and library links

crates/engine/src/doc_list_paths_tests.rs

@@ -46,3 +46,37 @@ fn a_plugin_gets_names_but_no_paths() {
     assert!(out.contains("Front"), "{out}");
     assert!(!out.contains(SECRET) && !out.contains("path"), "{out}");
 }
+
+#[test]
+fn a_plugin_gets_library_names_but_not_their_links() {
+    const LINK: &str = "ws://tracker.example:9/?file=f&token=per-file-secret";
+    let mut s = Session::new();
+    s.doc_mut().unwrap().libraries_mut().enabled.push(figcraft_doc::LibraryLink {
+        id: "lib".into(),
+        name: "Kit".into(),
+        path: LINK.into(),
+        version: 3,
+    });
+    let tail = "(call $set_output (i32.const 40000) (call $reply_read (i32.const 40000) (i32.const 20000))) (return (i32.const 0))";
+    install(&mut s, manifest("org.test.libs", "read", &[]), &script(&[("library.list", json!({}))], OnRefusal::Fail, tail));
+    let out = ex(&mut s, "plugin.run", json!({"id": "org.test.libs"}))["output"].to_string();
+    assert!(out.contains("Kit"), "{out}");
+    assert!(!out.contains("per-file-secret") && !out.contains("\\\"path\\\""), "{out}");
+}
+
+#[test]
+fn a_plugin_gets_a_fixed_message_for_library_update_failures() {
+    const LINK: &str = "/home/someone/private/kits/kit.figcraft";
+    let mut s = Session::new();
+    s.doc_mut().unwrap().libraries_mut().enabled.push(figcraft_doc::LibraryLink {
+        id: "lib".into(),
+        name: "Kit".into(),
+        path: LINK.into(),
+        version: 3,
+    });
+    let tail = "(call $set_output (i32.const 40000) (call $reply_read (i32.const 40000) (i32.const 20000))) (return (i32.const 0))";
+    install(&mut s, manifest("org.test.upd", "read", &[]), &script(&[("library.updates", json!({}))], OnRefusal::Fail, tail));
+    let out = ex(&mut s, "plugin.run", json!({"id": "org.test.upd"}))["output"].to_string();
+    assert!(out.contains("could not be read"), "{out}");
+    assert!(!out.contains("someone") && !out.contains("private"), "{out}");
+}

crates/engine/src/extensions/plugins.rs

@@ -180,12 +180,21 @@ fn refusal(m: &Manifest, command: &str, params: &Value) -> Option<String> {
     if command == "variable.import" && get(params, "variables").and_then(Value::as_str).is_some_and(|t| !t.trim_start().starts_with('{')) {
         return Some("plugins can't read files: pass the variables JSON itself".into());
     }
+    // A saved version is the file's history, which plugins can't read (`version.` is denied).
+    if command == "dev.compare" && get(params, "version").is_some_and(|v| !v.is_null()) {
+        return Some("plugins can't read saved versions: leave out `version`".into());
+    }
     if !m.permissions.allows(command) {
         return Some(format!("`{command}` isn't in the commands {} asked permission for", m.name));
     }
     if m.permissions.access == Access::Read && find_command(command).is_some_and(|c| c.history) {
         return Some(format!("`{command}` edits the document, and {} may only read it", m.name));
     }
+    // Branches live beside the document (not in its undo history or its `Arc`), so the write
+    // checks below don't see them change.
+    if m.permissions.access == Access::Read && matches!(command, "branch.create" | "branch.updateFromMain" | "branch.merge" | "branch.archive") {
+        return Some(format!("`{command}` changes the file's branches, and {} may only read it", m.name));
+    }
     None
 }
 
@@ -263,10 +272,23 @@ impl figcraft_plugin::Host for EngineHost<'_> {
             Ok(v)
         });
         r.map(|mut v| {
-            // A plugin gets the open files' names, not where they are on disk (#1117).
-            if command == "doc.list" {
-                for e in v.as_array_mut().into_iter().flatten().filter_map(Value::as_object_mut) {
+            // A plugin gets the open files' and libraries' names, not where they are on disk or
+            // the share links (with their tokens) that reach them (#1117).
+            if matches!(command, "doc.list" | "library.list" | "library.updates") {
+                // `doc.list` is a list; `library.list` keeps its list under `libraries`, and
+                // `library.updates` its failures under `errors`.
+                let list = match command {
+                    "library.list" => v.get_mut("libraries"),
+                    "library.updates" => v.get_mut("errors"),
+                    _ => Some(&mut v),
+                };
+                for e in list.and_then(Value::as_array_mut).into_iter().flatten().filter_map(Value::as_object_mut) {
                     e.remove("path");
+                    // A failure's text may quote the path, a token or another form of it: the
+                    // plugin gets a fixed message.
+                    if let Some(err) = e.get_mut("error").filter(|v| v.is_string()) {
+                        *err = json!("the library could not be read");
+                    }
                 }
             }
             v.to_string()
@@ -708,6 +730,23 @@ mod refusal_tests {
         let r = refusal(&m, "library.publishToServer", &json!({}));
         assert!(r.is_some(), "library.publishToServer uploads the document and must be refused");
     }
+
+    #[test]
+    fn read_only_plugin_cannot_change_branches() {
+        let m = Manifest::parse(r#"{"id":"x.y","name":"X","main":"plugin.wasm"}"#).unwrap();
+        for cmd in ["branch.archive", "branch.updateFromMain", "branch.merge", "branch.create"] {
+            assert!(refusal(&m, cmd, &json!({})).is_some(), "{cmd} changes persisted branch state");
+        }
+        assert!(refusal(&m, "branch.list", &json!({})).is_none());
+    }
+
+    #[test]
+    fn plugins_cannot_compare_against_a_saved_version() {
+        let m = Manifest::parse(r#"{"id":"x.y","name":"X","main":"plugin.wasm","permissions":{"access":"write"}}"#).unwrap();
+        assert!(refusal(&m, "dev.compare", &json!({"version": "v1"})).is_some());
+        assert!(refusal(&m, "dev.compare", &json!({"version": null})).is_none());
+        assert!(refusal(&m, "dev.compare", &json!({})).is_none());
+    }
 }
 
 #[cfg(test)]