diff --git a/registry/_bundles.json b/registry/_bundles.json index bf332a58..c7adf1cd 100644 --- a/registry/_bundles.json +++ b/registry/_bundles.json @@ -32,7 +32,7 @@ "tools/gmail", "tools/google-calendar", "tools/google-drive", - "tools/slack", + "tools/slack-tool", "channels/telegram", "channels/slack" ], diff --git a/registry/tools/slack.json b/registry/tools/slack.json index b7bedf53..8e33cba5 100644 --- a/registry/tools/slack.json +++ b/registry/tools/slack.json @@ -1,5 +1,5 @@ { - "name": "slack", + "name": "slack-tool", "display_name": "Slack", "kind": "tool", "version": "0.1.0", diff --git a/registry/tools/telegram.json b/registry/tools/telegram.json index d7df228e..07e51f66 100644 --- a/registry/tools/telegram.json +++ b/registry/tools/telegram.json @@ -1,5 +1,5 @@ { - "name": "telegram", + "name": "telegram-mtproto", "display_name": "Telegram", "kind": "tool", "version": "0.1.0", diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 52814340..7c2ef0a3 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -191,9 +191,10 @@ impl ExtensionManager { kind_hint: Option, ) -> Result { tracing::info!(extension = %name, url = ?url, kind = ?kind_hint, "Installing extension"); + Self::validate_extension_name(name)?; - // If we have a registry entry, use it - if let Some(entry) = self.registry.get(name).await { + // If we have a registry entry, use it (prefer kind_hint to resolve collisions) + if let Some(entry) = self.registry.get_with_kind(name, kind_hint).await { return self.install_from_entry(&entry).await.map_err(|e| { tracing::error!(extension = %name, error = %e, "Extension install failed"); e @@ -245,6 +246,7 @@ impl ExtensionManager { /// Activate an installed (and optionally authenticated) extension. pub async fn activate(&self, name: &str) -> Result { + Self::validate_extension_name(name)?; let kind = self.determine_installed_kind(name).await?; match kind { @@ -399,6 +401,7 @@ impl ExtensionManager { /// Remove an installed extension. pub async fn remove(&self, name: &str) -> Result { + Self::validate_extension_name(name)?; let kind = self.determine_installed_kind(name).await?; match kind { @@ -1732,14 +1735,6 @@ impl ExtensionManager { ))); } - // Validate name to prevent path traversal - if name.contains('/') || name.contains('\\') || name.contains("..") || name.contains('\0') { - return Err(ExtensionError::ActivationFailed(format!( - "Invalid channel name '{}': contains path separator or traversal characters", - name - ))); - } - // Load the channel from files let wasm_path = self.wasm_channels_dir.join(format!("{}.wasm", name)); let cap_path = self @@ -2033,6 +2028,17 @@ impl ExtensionManager { ))) } + /// Reject names containing path separators or traversal sequences. + fn validate_extension_name(name: &str) -> Result<(), ExtensionError> { + if name.contains('/') || name.contains('\\') || name.contains("..") || name.contains('\0') { + return Err(ExtensionError::InstallFailed(format!( + "Invalid extension name '{}': contains path separator or traversal characters", + name + ))); + } + Ok(()) + } + async fn cleanup_expired_auths(&self) { let mut pending = self.pending_auth.write().await; pending.retain(|_, auth| auth.created_at.elapsed() < std::time::Duration::from_secs(300)); diff --git a/src/extensions/registry.rs b/src/extensions/registry.rs index ef6e46e6..0f79b6f8 100644 --- a/src/extensions/registry.rs +++ b/src/extensions/registry.rs @@ -108,6 +108,9 @@ impl ExtensionRegistry { } /// Look up an entry by exact name. + /// + /// NOTE: Prefer [`get_with_kind`] when a kind hint is available, to avoid + /// returning the wrong entry when two entries share a name but differ in kind. pub async fn get(&self, name: &str) -> Option { if let Some(entry) = self.entries.iter().find(|e| e.name == name) { return Some(entry.clone()); @@ -116,6 +119,35 @@ impl ExtensionRegistry { cache.iter().find(|e| e.name == name).cloned() } + /// Look up an entry by exact name, filtering by kind when provided. + /// + /// When `kind` is `Some(...)`, only returns an entry matching both name and + /// kind — never falls back to a different kind. When `kind` is `None`, + /// returns the first name match (same as [`get`]). + pub async fn get_with_kind( + &self, + name: &str, + kind: Option, + ) -> Option { + if let Some(kind) = kind { + if let Some(entry) = self + .entries + .iter() + .find(|e| e.name == name && e.kind == kind) + { + return Some(entry.clone()); + } + let cache = self.discovery_cache.read().await; + if let Some(entry) = cache.iter().find(|e| e.name == name && e.kind == kind) { + return Some(entry.clone()); + } + // Kind was specified but no entry matches — don't fall back to a + // different kind, as that would silently misroute the install. + return None; + } + self.get(name).await + } + /// Return all registry entries (builtins + cached discoveries). pub async fn all_entries(&self) -> Vec { let mut entries = self.entries.clone(); @@ -135,8 +167,11 @@ impl ExtensionRegistry { pub async fn cache_discovered(&self, entries: Vec) { let mut cache = self.discovery_cache.write().await; for entry in entries { - // Deduplicate by name - if !cache.iter().any(|e| e.name == entry.name) { + // Deduplicate by (name, kind) — same pair as new_with_catalog() + if !cache + .iter() + .any(|e| e.name == entry.name && e.kind == entry.kind) + { cache.push(entry); } } @@ -675,6 +710,134 @@ mod tests { assert_eq!(entry.unwrap().display_name, "Slack MCP"); } + #[tokio::test] + async fn test_get_with_kind_resolves_collision() { + // Two entries with the same name but different kinds (the telegram collision scenario) + let catalog_entries = vec![ + RegistryEntry { + name: "telegram".to_string(), + display_name: "Telegram Tool".to_string(), + kind: ExtensionKind::WasmTool, + description: "Telegram MTProto tool".to_string(), + keywords: vec!["messaging".into()], + source: ExtensionSource::WasmBuildable { + repo_url: "tools-src/telegram".to_string(), + build_dir: Some("tools-src/telegram".to_string()), + crate_name: Some("telegram-tool".to_string()), + }, + fallback_source: None, + auth_hint: AuthHint::CapabilitiesAuth, + }, + RegistryEntry { + name: "telegram".to_string(), + display_name: "Telegram Channel".to_string(), + kind: ExtensionKind::WasmChannel, + description: "Telegram Bot API channel".to_string(), + keywords: vec!["messaging".into(), "bot".into()], + source: ExtensionSource::WasmBuildable { + repo_url: "channels-src/telegram".to_string(), + build_dir: Some("channels-src/telegram".to_string()), + crate_name: Some("telegram-channel".to_string()), + }, + fallback_source: None, + auth_hint: AuthHint::CapabilitiesAuth, + }, + ]; + + let registry = ExtensionRegistry::new_with_catalog(catalog_entries); + + // Without kind hint, get() returns the first match (WasmTool) + let entry = registry.get("telegram").await; + assert!(entry.is_some()); + assert_eq!(entry.unwrap().kind, ExtensionKind::WasmTool); + + // With kind hint for WasmChannel, get_with_kind() returns the channel entry + let entry = registry + .get_with_kind("telegram", Some(ExtensionKind::WasmChannel)) + .await; + assert!(entry.is_some()); + let entry = entry.unwrap(); + assert_eq!(entry.kind, ExtensionKind::WasmChannel); + assert_eq!(entry.display_name, "Telegram Channel"); + + // With kind hint for WasmTool, get_with_kind() returns the tool entry + let entry = registry + .get_with_kind("telegram", Some(ExtensionKind::WasmTool)) + .await; + assert!(entry.is_some()); + let entry = entry.unwrap(); + assert_eq!(entry.kind, ExtensionKind::WasmTool); + assert_eq!(entry.display_name, "Telegram Tool"); + + // Without kind hint (None), get_with_kind() falls back to first match + let entry = registry.get_with_kind("telegram", None).await; + assert!(entry.is_some()); + assert_eq!(entry.unwrap().kind, ExtensionKind::WasmTool); + + // Kind mismatch: no McpServer named "telegram" exists — must return None, + // not silently fall back to the WasmTool entry. + let entry = registry + .get_with_kind("telegram", Some(ExtensionKind::McpServer)) + .await; + assert!( + entry.is_none(), + "Should return None when kind doesn't match, not fall back to wrong kind" + ); + } + + #[tokio::test] + async fn test_get_with_kind_discovery_cache() { + let registry = ExtensionRegistry::new(); + + // Add two entries with the same name but different kinds to the discovery cache + let tool_entry = RegistryEntry { + name: "cached-ext".to_string(), + display_name: "Cached Tool".to_string(), + kind: ExtensionKind::WasmTool, + description: "A cached tool".to_string(), + keywords: vec![], + source: ExtensionSource::WasmBuildable { + repo_url: "tools-src/cached".to_string(), + build_dir: None, + crate_name: None, + }, + fallback_source: None, + auth_hint: AuthHint::None, + }; + let channel_entry = RegistryEntry { + name: "cached-ext".to_string(), + display_name: "Cached Channel".to_string(), + kind: ExtensionKind::WasmChannel, + description: "A cached channel".to_string(), + keywords: vec![], + source: ExtensionSource::WasmBuildable { + repo_url: "channels-src/cached".to_string(), + build_dir: None, + crate_name: None, + }, + fallback_source: None, + auth_hint: AuthHint::None, + }; + + registry + .cache_discovered(vec![tool_entry, channel_entry]) + .await; + + // Kind-aware lookup should find the channel in the cache + let entry = registry + .get_with_kind("cached-ext", Some(ExtensionKind::WasmChannel)) + .await; + assert!(entry.is_some()); + assert_eq!(entry.unwrap().display_name, "Cached Channel"); + + // Kind-aware lookup should find the tool in the cache + let entry = registry + .get_with_kind("cached-ext", Some(ExtensionKind::WasmTool)) + .await; + assert!(entry.is_some()); + assert_eq!(entry.unwrap().display_name, "Cached Tool"); + } + // Channel tests (telegram, slack, discord, whatsapp) require the embedded catalog // to be loaded via new_with_catalog(). See test_new_with_catalog for catalog coverage. } diff --git a/src/registry/catalog.rs b/src/registry/catalog.rs index 36f75d59..64b03704 100644 --- a/src/registry/catalog.rs +++ b/src/registry/catalog.rs @@ -87,7 +87,7 @@ pub enum RegistryError { /// Central catalog loaded from the `registry/` directory. #[derive(Debug, Clone)] pub struct RegistryCatalog { - /// All loaded manifests, keyed by "/" (e.g. "tools/slack"). + /// All loaded manifests, keyed by "/" (e.g. "tools/github"). manifests: HashMap, /// Bundle definitions from `_bundles.json`. @@ -274,11 +274,11 @@ impl RegistryCatalog { results } - /// Get a manifest by name. Tries exact key match first ("tools/slack"), - /// then searches by bare name ("slack"). + /// Get a manifest by name. Tries exact key match first ("tools/github"), + /// then searches by bare name ("github"). /// /// If a bare name matches both a tool and a channel, returns `None`. - /// Use a qualified key ("tools/slack" or "channels/slack") to disambiguate. + /// Use a qualified key ("tools/github" or "channels/telegram") to disambiguate. pub fn get(&self, name: &str) -> Option<&ExtensionManifest> { // Try exact key first if let Some(m) = self.manifests.get(name) { @@ -322,7 +322,7 @@ impl RegistryCatalog { } } - /// Get the full key ("tools/slack" or "channels/telegram") for a manifest. + /// Get the full key ("tools/github" or "channels/telegram") for a manifest. pub fn key_for(&self, name: &str) -> Option { if self.manifests.contains_key(name) { return Some(name.to_string()); @@ -682,4 +682,26 @@ mod tests { // At minimum, the embedded catalog from the repo should have entries assert!(!catalog.all().is_empty() || !catalog.bundle_names().is_empty()); } + + #[test] + fn test_bundle_entries_resolve_against_real_registry() { + // Load the actual registry/ directory (catches stale bundle refs after renames) + let catalog = RegistryCatalog::load_or_embedded().unwrap(); + + for bundle_name in catalog.bundle_names() { + let (manifests, missing) = catalog.resolve_bundle(bundle_name).unwrap(); + assert!( + missing.is_empty(), + "Bundle '{}' has unresolved entries: {:?}. \ + Check that _bundles.json entries match manifest name fields.", + bundle_name, + missing + ); + assert!( + !manifests.is_empty(), + "Bundle '{}' resolved to zero manifests", + bundle_name + ); + } + } }