mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
fix: resolve telegram/slack name collision between tool and channel registries (#346)
When installing the Telegram WASM channel via the web UI, a name collision between registry/tools/telegram.json and registry/channels/telegram.json caused the tool entry to win, installing to ~/.ironclaw/tools/ instead of ~/.ironclaw/channels/. This made activation fail with "WASM runtime not available". - Add `get_with_kind()` to ExtensionRegistry for kind-aware lookup - Use `kind_hint` parameter in `install()` to resolve collisions - Rename tool entries to avoid future collisions: telegram → telegram-mtproto, slack → slack-tool - Fix `_bundles.json` stale reference (tools/slack → tools/slack-tool) - Fix `cache_discovered()` to deduplicate by (name, kind) consistently - Add path traversal validation to install/activate/remove entry points - Add tests for kind-aware lookup, discovery cache, and bundle resolution Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
b0b3a50fa3
commit
e9f32eaebe
@@ -32,7 +32,7 @@
|
|||||||
"tools/gmail",
|
"tools/gmail",
|
||||||
"tools/google-calendar",
|
"tools/google-calendar",
|
||||||
"tools/google-drive",
|
"tools/google-drive",
|
||||||
"tools/slack",
|
"tools/slack-tool",
|
||||||
"channels/telegram",
|
"channels/telegram",
|
||||||
"channels/slack"
|
"channels/slack"
|
||||||
],
|
],
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
{
|
{
|
||||||
"name": "slack",
|
"name": "slack-tool",
|
||||||
"display_name": "Slack",
|
"display_name": "Slack",
|
||||||
"kind": "tool",
|
"kind": "tool",
|
||||||
"version": "0.1.0",
|
"version": "0.1.0",
|
||||||
|
|||||||
@@ -1,5 +1,5 @@
|
|||||||
{
|
{
|
||||||
"name": "telegram",
|
"name": "telegram-mtproto",
|
||||||
"display_name": "Telegram",
|
"display_name": "Telegram",
|
||||||
"kind": "tool",
|
"kind": "tool",
|
||||||
"version": "0.1.0",
|
"version": "0.1.0",
|
||||||
|
|||||||
+16
-10
@@ -191,9 +191,10 @@ impl ExtensionManager {
|
|||||||
kind_hint: Option<ExtensionKind>,
|
kind_hint: Option<ExtensionKind>,
|
||||||
) -> Result<InstallResult, ExtensionError> {
|
) -> Result<InstallResult, ExtensionError> {
|
||||||
tracing::info!(extension = %name, url = ?url, kind = ?kind_hint, "Installing extension");
|
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 we have a registry entry, use it (prefer kind_hint to resolve collisions)
|
||||||
if let Some(entry) = self.registry.get(name).await {
|
if let Some(entry) = self.registry.get_with_kind(name, kind_hint).await {
|
||||||
return self.install_from_entry(&entry).await.map_err(|e| {
|
return self.install_from_entry(&entry).await.map_err(|e| {
|
||||||
tracing::error!(extension = %name, error = %e, "Extension install failed");
|
tracing::error!(extension = %name, error = %e, "Extension install failed");
|
||||||
e
|
e
|
||||||
@@ -245,6 +246,7 @@ impl ExtensionManager {
|
|||||||
|
|
||||||
/// Activate an installed (and optionally authenticated) extension.
|
/// Activate an installed (and optionally authenticated) extension.
|
||||||
pub async fn activate(&self, name: &str) -> Result<ActivateResult, ExtensionError> {
|
pub async fn activate(&self, name: &str) -> Result<ActivateResult, ExtensionError> {
|
||||||
|
Self::validate_extension_name(name)?;
|
||||||
let kind = self.determine_installed_kind(name).await?;
|
let kind = self.determine_installed_kind(name).await?;
|
||||||
|
|
||||||
match kind {
|
match kind {
|
||||||
@@ -399,6 +401,7 @@ impl ExtensionManager {
|
|||||||
|
|
||||||
/// Remove an installed extension.
|
/// Remove an installed extension.
|
||||||
pub async fn remove(&self, name: &str) -> Result<String, ExtensionError> {
|
pub async fn remove(&self, name: &str) -> Result<String, ExtensionError> {
|
||||||
|
Self::validate_extension_name(name)?;
|
||||||
let kind = self.determine_installed_kind(name).await?;
|
let kind = self.determine_installed_kind(name).await?;
|
||||||
|
|
||||||
match kind {
|
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
|
// Load the channel from files
|
||||||
let wasm_path = self.wasm_channels_dir.join(format!("{}.wasm", name));
|
let wasm_path = self.wasm_channels_dir.join(format!("{}.wasm", name));
|
||||||
let cap_path = self
|
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) {
|
async fn cleanup_expired_auths(&self) {
|
||||||
let mut pending = self.pending_auth.write().await;
|
let mut pending = self.pending_auth.write().await;
|
||||||
pending.retain(|_, auth| auth.created_at.elapsed() < std::time::Duration::from_secs(300));
|
pending.retain(|_, auth| auth.created_at.elapsed() < std::time::Duration::from_secs(300));
|
||||||
|
|||||||
+165
-2
@@ -108,6 +108,9 @@ impl ExtensionRegistry {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Look up an entry by exact name.
|
/// 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<RegistryEntry> {
|
pub async fn get(&self, name: &str) -> Option<RegistryEntry> {
|
||||||
if let Some(entry) = self.entries.iter().find(|e| e.name == name) {
|
if let Some(entry) = self.entries.iter().find(|e| e.name == name) {
|
||||||
return Some(entry.clone());
|
return Some(entry.clone());
|
||||||
@@ -116,6 +119,35 @@ impl ExtensionRegistry {
|
|||||||
cache.iter().find(|e| e.name == name).cloned()
|
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<ExtensionKind>,
|
||||||
|
) -> Option<RegistryEntry> {
|
||||||
|
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).
|
/// Return all registry entries (builtins + cached discoveries).
|
||||||
pub async fn all_entries(&self) -> Vec<RegistryEntry> {
|
pub async fn all_entries(&self) -> Vec<RegistryEntry> {
|
||||||
let mut entries = self.entries.clone();
|
let mut entries = self.entries.clone();
|
||||||
@@ -135,8 +167,11 @@ impl ExtensionRegistry {
|
|||||||
pub async fn cache_discovered(&self, entries: Vec<RegistryEntry>) {
|
pub async fn cache_discovered(&self, entries: Vec<RegistryEntry>) {
|
||||||
let mut cache = self.discovery_cache.write().await;
|
let mut cache = self.discovery_cache.write().await;
|
||||||
for entry in entries {
|
for entry in entries {
|
||||||
// Deduplicate by name
|
// Deduplicate by (name, kind) — same pair as new_with_catalog()
|
||||||
if !cache.iter().any(|e| e.name == entry.name) {
|
if !cache
|
||||||
|
.iter()
|
||||||
|
.any(|e| e.name == entry.name && e.kind == entry.kind)
|
||||||
|
{
|
||||||
cache.push(entry);
|
cache.push(entry);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -675,6 +710,134 @@ mod tests {
|
|||||||
assert_eq!(entry.unwrap().display_name, "Slack MCP");
|
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
|
// 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.
|
// to be loaded via new_with_catalog(). See test_new_with_catalog for catalog coverage.
|
||||||
}
|
}
|
||||||
|
|||||||
+27
-5
@@ -87,7 +87,7 @@ pub enum RegistryError {
|
|||||||
/// Central catalog loaded from the `registry/` directory.
|
/// Central catalog loaded from the `registry/` directory.
|
||||||
#[derive(Debug, Clone)]
|
#[derive(Debug, Clone)]
|
||||||
pub struct RegistryCatalog {
|
pub struct RegistryCatalog {
|
||||||
/// All loaded manifests, keyed by "<kind>/<name>" (e.g. "tools/slack").
|
/// All loaded manifests, keyed by "<kind>/<name>" (e.g. "tools/github").
|
||||||
manifests: HashMap<String, ExtensionManifest>,
|
manifests: HashMap<String, ExtensionManifest>,
|
||||||
|
|
||||||
/// Bundle definitions from `_bundles.json`.
|
/// Bundle definitions from `_bundles.json`.
|
||||||
@@ -274,11 +274,11 @@ impl RegistryCatalog {
|
|||||||
results
|
results
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Get a manifest by name. Tries exact key match first ("tools/slack"),
|
/// Get a manifest by name. Tries exact key match first ("tools/github"),
|
||||||
/// then searches by bare name ("slack").
|
/// then searches by bare name ("github").
|
||||||
///
|
///
|
||||||
/// If a bare name matches both a tool and a channel, returns `None`.
|
/// 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> {
|
pub fn get(&self, name: &str) -> Option<&ExtensionManifest> {
|
||||||
// Try exact key first
|
// Try exact key first
|
||||||
if let Some(m) = self.manifests.get(name) {
|
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<String> {
|
pub fn key_for(&self, name: &str) -> Option<String> {
|
||||||
if self.manifests.contains_key(name) {
|
if self.manifests.contains_key(name) {
|
||||||
return Some(name.to_string());
|
return Some(name.to_string());
|
||||||
@@ -682,4 +682,26 @@ mod tests {
|
|||||||
// At minimum, the embedded catalog from the repo should have entries
|
// At minimum, the embedded catalog from the repo should have entries
|
||||||
assert!(!catalog.all().is_empty() || !catalog.bundle_names().is_empty());
|
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
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user