From 28be7b3e24c53b2d99e1fcb0d2a5beb47f0ea26b Mon Sep 17 00:00:00 2001 From: Yeachan-Heo Date: Wed, 1 Apr 2026 07:23:10 +0000 Subject: [PATCH] Tighten plugin manifest validation and installed-plugin discovery Expanded the Rust plugin loader coverage around manifest parsing so invalid permission values, invalid tool permissions, and multi-error manifests are validated in a structured way. Added scan-path coverage for installed plugin directories so both root and packaged manifests are discovered from the install root, independent of registry entries. Constraint: Keep plugin loader changes isolated to the plugins crate surface Rejected: Add a new manifest crate for shared schemas | unnecessary scope for this pass Confidence: high Scope-risk: narrow Reversibility: clean Directive: If manifest permissions or tool permission labels expand, update both the enums and validation tests together Tested: cargo fmt --all; cargo test -p plugins Not-tested: Cross-crate runtime consumption of any future expanded manifest permission variants --- rust/crates/plugins/src/lib.rs | 173 ++++++++++++++++++++++++++++++++- 1 file changed, 172 insertions(+), 1 deletion(-) diff --git a/rust/crates/plugins/src/lib.rs b/rust/crates/plugins/src/lib.rs index 8c859a5..1c2f134 100644 --- a/rust/crates/plugins/src/lib.rs +++ b/rust/crates/plugins/src/lib.rs @@ -2060,6 +2060,10 @@ mod tests { } fn write_tool_plugin(root: &Path, name: &str, version: &str) { + write_tool_plugin_with_name(root, name, version, "plugin_echo"); + } + + fn write_tool_plugin_with_name(root: &Path, name: &str, version: &str, tool_name: &str) { let script_path = root.join("tools").join("echo-json.sh"); write_file( &script_path, @@ -2076,7 +2080,7 @@ mod tests { write_file( root.join(MANIFEST_RELATIVE_PATH).as_path(), format!( - "{{\n \"name\": \"{name}\",\n \"version\": \"{version}\",\n \"description\": \"tool plugin\",\n \"tools\": [\n {{\n \"name\": \"plugin_echo\",\n \"description\": \"Echo JSON input\",\n \"inputSchema\": {{\"type\": \"object\", \"properties\": {{\"message\": {{\"type\": \"string\"}}}}, \"required\": [\"message\"], \"additionalProperties\": false}},\n \"command\": \"./tools/echo-json.sh\",\n \"requiredPermission\": \"workspace-write\"\n }}\n ]\n}}" + "{{\n \"name\": \"{name}\",\n \"version\": \"{version}\",\n \"description\": \"tool plugin\",\n \"tools\": [\n {{\n \"name\": \"{tool_name}\",\n \"description\": \"Echo JSON input\",\n \"inputSchema\": {{\"type\": \"object\", \"properties\": {{\"message\": {{\"type\": \"string\"}}}}, \"required\": [\"message\"], \"additionalProperties\": false}},\n \"command\": \"./tools/echo-json.sh\",\n \"requiredPermission\": \"workspace-write\"\n }}\n ]\n}}" ) .as_str(), ); @@ -2284,6 +2288,91 @@ mod tests { let _ = fs::remove_dir_all(root); } + #[test] + fn load_plugin_from_directory_rejects_invalid_tool_required_permission() { + let root = temp_dir("manifest-invalid-tool-permission"); + write_file( + root.join("tools").join("echo.sh").as_path(), + "#!/bin/sh\ncat\n", + ); + write_file( + root.join(MANIFEST_FILE_NAME).as_path(), + r#"{ + "name": "invalid-tool-permission", + "version": "1.0.0", + "description": "Invalid tool permission validation", + "tools": [ + { + "name": "echo_tool", + "description": "Echo tool", + "inputSchema": {"type": "object"}, + "command": "./tools/echo.sh", + "requiredPermission": "admin" + } + ] +}"#, + ); + + let error = + load_plugin_from_directory(&root).expect_err("invalid tool permission should fail"); + match error { + PluginError::ManifestValidation(errors) => { + assert!(errors.iter().any(|error| matches!( + error, + PluginManifestValidationError::InvalidToolRequiredPermission { + tool_name, + permission + } if tool_name == "echo_tool" && permission == "admin" + ))); + } + other => panic!("expected manifest validation errors, got {other}"), + } + + let _ = fs::remove_dir_all(root); + } + + #[test] + fn load_plugin_from_directory_accumulates_multiple_validation_errors() { + let root = temp_dir("manifest-multi-error"); + write_file( + root.join(MANIFEST_FILE_NAME).as_path(), + r#"{ + "name": "", + "version": "1.0.0", + "description": "", + "permissions": ["admin"], + "commands": [ + {"name": "", "description": "", "command": "./commands/missing.sh"} + ] +}"#, + ); + + let error = + load_plugin_from_directory(&root).expect_err("multiple manifest errors should fail"); + match error { + PluginError::ManifestValidation(errors) => { + assert!(errors.len() >= 4); + assert!(errors.iter().any(|error| matches!( + error, + PluginManifestValidationError::EmptyField { field } if *field == "name" + ))); + assert!(errors.iter().any(|error| matches!( + error, + PluginManifestValidationError::EmptyField { field } + if *field == "description" + ))); + assert!(errors.iter().any(|error| matches!( + error, + PluginManifestValidationError::InvalidPermission { permission } + if permission == "admin" + ))); + } + other => panic!("expected manifest validation errors, got {other}"), + } + + let _ = fs::remove_dir_all(root); + } + #[test] fn discovers_builtin_and_bundled_plugins() { let manager = PluginManager::new(PluginManagerConfig::new(temp_dir("discover"))); @@ -2375,6 +2464,24 @@ mod tests { let _ = fs::remove_dir_all(bundled_root); } + #[test] + fn default_bundled_root_loads_repo_bundles_as_installed_plugins() { + let config_home = temp_dir("default-bundled-home"); + let manager = PluginManager::new(PluginManagerConfig::new(&config_home)); + + let installed = manager + .list_installed_plugins() + .expect("default bundled plugins should auto-install"); + assert!(installed + .iter() + .any(|plugin| plugin.metadata.id == "example-bundled@bundled")); + assert!(installed + .iter() + .any(|plugin| plugin.metadata.id == "sample-hooks@bundled")); + + let _ = fs::remove_dir_all(config_home); + } + #[test] fn persists_bundled_plugin_enable_state_across_reloads() { let config_home = temp_dir("bundled-state-home"); @@ -2408,6 +2515,39 @@ mod tests { let _ = fs::remove_dir_all(bundled_root); } + #[test] + fn persists_bundled_plugin_disable_state_across_reloads() { + let config_home = temp_dir("bundled-disabled-home"); + let bundled_root = temp_dir("bundled-disabled-root"); + write_bundled_plugin(&bundled_root.join("starter"), "starter", "0.1.0", true); + + let mut config = PluginManagerConfig::new(&config_home); + config.bundled_root = Some(bundled_root.clone()); + let mut manager = PluginManager::new(config); + + manager + .disable("starter@bundled") + .expect("disable bundled plugin should succeed"); + assert_eq!( + load_enabled_plugins(&manager.settings_path()).get("starter@bundled"), + Some(&false) + ); + + let mut reloaded_config = PluginManagerConfig::new(&config_home); + reloaded_config.bundled_root = Some(bundled_root.clone()); + reloaded_config.enabled_plugins = load_enabled_plugins(&manager.settings_path()); + let reloaded_manager = PluginManager::new(reloaded_config); + let reloaded = reloaded_manager + .list_installed_plugins() + .expect("bundled plugins should still be listed"); + assert!(reloaded + .iter() + .any(|plugin| { plugin.metadata.id == "starter@bundled" && !plugin.enabled })); + + let _ = fs::remove_dir_all(config_home); + let _ = fs::remove_dir_all(bundled_root); + } + #[test] fn validates_plugin_source_before_install() { let config_home = temp_dir("validate-home"); @@ -2552,4 +2692,35 @@ mod tests { let _ = fs::remove_dir_all(config_home); let _ = fs::remove_dir_all(bundled_root); } + + #[test] + fn list_installed_plugins_scans_packaged_manifests_in_install_root() { + let config_home = temp_dir("installed-packaged-scan-home"); + let bundled_root = temp_dir("installed-packaged-scan-bundled"); + let install_root = config_home.join("plugins").join("installed"); + let installed_plugin_root = install_root.join("scan-packaged"); + write_file( + installed_plugin_root.join(MANIFEST_RELATIVE_PATH).as_path(), + r#"{ + "name": "scan-packaged", + "version": "1.0.0", + "description": "Packaged manifest in install root" +}"#, + ); + + let mut config = PluginManagerConfig::new(&config_home); + config.bundled_root = Some(bundled_root.clone()); + config.install_root = Some(install_root); + let manager = PluginManager::new(config); + + let installed = manager + .list_installed_plugins() + .expect("installed plugins should scan packaged manifests"); + assert!(installed + .iter() + .any(|plugin| plugin.metadata.id == "scan-packaged@external")); + + let _ = fs::remove_dir_all(config_home); + let _ = fs::remove_dir_all(bundled_root); + } }