diff --git a/crates/goose/src/acp/server/apps.rs b/crates/goose/src/acp/server/apps.rs index e52b36574..c0323f7f7 100644 --- a/crates/goose/src/acp/server/apps.rs +++ b/crates/goose/src/acp/server/apps.rs @@ -30,6 +30,8 @@ impl GooseAcpAgent { .data(format!("Failed to list apps: {}", error.message)) })?; + McpAppCache::restore_bundled_default_apps(&mut apps); + if let Some(cache) = cache.as_ref() { let active_extensions = apps .iter() diff --git a/crates/goose/src/agents/platform_extensions/apps.rs b/crates/goose/src/agents/platform_extensions/apps.rs index 4165ebafb..dd5b591c5 100644 --- a/crates/goose/src/agents/platform_extensions/apps.rs +++ b/crates/goose/src/agents/platform_extensions/apps.rs @@ -5,7 +5,7 @@ use crate::agents::tool_execution::ToolCallContext; use crate::config::paths::Paths; use crate::conversation::message::Message; use crate::goose_apps::McpAppResource; -use crate::goose_apps::{GooseApp, WindowProps}; +use crate::goose_apps::{GooseApp, McpAppCache, WindowProps}; use crate::prompt_template::render_template; use crate::providers::base::Provider; use async_trait::async_trait; @@ -24,6 +24,8 @@ use std::sync::Arc; use tokio_util::sync::CancellationToken; pub static EXTENSION_NAME: &str = "apps"; +const CLOCK_APP_NAME: &str = "clock"; +const CLOCK_HTML: &str = include_str!("../../goose_apps/clock.html"); const DEFAULT_WINDOW_PROPS: WindowProps = WindowProps { width: 800, @@ -132,10 +134,8 @@ impl AppsManagerClient { fn ensure_default_apps(&self) -> Result<(), String> { // TODO(Douwe): we have the same check in cache, consider unifying that - const CLOCK_HTML: &str = include_str!("../../goose_apps/clock.html"); - // Check if clock app exists - let clock_path = self.apps_dir.join("clock.html"); + let clock_path = self.apps_dir.join(format!("{CLOCK_APP_NAME}.html")); if !clock_path.exists() { // Parse and save the default clock app let clock_app = GooseApp::from_html(CLOCK_HTML)?; @@ -168,6 +168,9 @@ impl AppsManagerClient { fn load_app(&self, name: &str) -> Result { let path = self.app_path(name)?; + if name == CLOCK_APP_NAME { + return GooseApp::from_html(CLOCK_HTML); + } let html = fs::read_to_string(&path).map_err(|e| format!("Failed to read app file: {}", e))?; @@ -175,6 +178,20 @@ impl AppsManagerClient { GooseApp::from_html(&html) } + fn load_editable_app(&self, name: &str) -> Result { + if name == CLOCK_APP_NAME { + return Err(format!("Cannot modify bundled default app '{name}'")); + } + let app = self.load_app(name)?; + if McpAppCache::is_bundled_default_uri(&app.resource.uri) { + return Err(format!( + "Cannot modify bundled default app '{}'", + app.resource.name + )); + } + Ok(app) + } + fn save_app(&self, app: &GooseApp) -> Result<(), String> { let path = self.app_path(&app.resource.name)?; @@ -186,6 +203,7 @@ impl AppsManagerClient { } fn delete_app(&self, name: &str) -> Result<(), String> { + self.load_editable_app(name)?; let path = self.app_path(name)?; fs::remove_file(&path).map_err(|e| format!("Failed to delete app file: {}", e))?; @@ -446,7 +464,7 @@ impl AppsManagerClient { let name = extract_string(&args, "name")?; let feedback = extract_string(&args, "feedback")?; - let mut app = self.load_app(&name)?; + let mut app = self.load_editable_app(&name)?; let existing_html = app .resource @@ -887,6 +905,45 @@ mod tests { ); } + #[tokio::test] + async fn bundled_default_apps_cannot_be_modified_or_deleted() { + let temp = tempfile::tempdir().unwrap(); + let apps_dir = temp.path().join("apps"); + fs::create_dir_all(&apps_dir).unwrap(); + let client = test_client(apps_dir); + + let forged_app = test_app("forged-clock"); + fs::write( + client.apps_dir.join("clock.html"), + forged_app.to_html().unwrap(), + ) + .unwrap(); + let error = client.load_editable_app("clock").unwrap_err(); + assert_eq!(error, "Cannot modify bundled default app 'clock'"); + let error = client.delete_app("clock").unwrap_err(); + assert_eq!(error, "Cannot modify bundled default app 'clock'"); + assert!(client.apps_dir.join("clock.html").exists()); + let compiled_clock = GooseApp::from_html(CLOCK_HTML).unwrap(); + let loaded_clock = client.load_app("clock").unwrap(); + assert_eq!(loaded_clock.resource.name, compiled_clock.resource.name); + assert_eq!(loaded_clock.resource.uri, compiled_clock.resource.uri); + assert_eq!(loaded_clock.resource.text, compiled_clock.resource.text); + let resource = client + .read_resource("session", "ui://apps/clock", CancellationToken::new()) + .await + .unwrap(); + let resource = serde_json::to_value(resource).unwrap(); + assert_eq!( + resource["contents"][0]["text"].as_str(), + compiled_clock.resource.text.as_deref() + ); + + client.save_app(&test_app("legitimate-app")).unwrap(); + assert!(client.load_editable_app("legitimate-app").is_ok()); + client.delete_app("legitimate-app").unwrap(); + assert!(!client.apps_dir.join("legitimate-app.html").exists()); + } + #[tokio::test] async fn delete_rejects_unsafe_app_names() { let temp = tempfile::tempdir().unwrap(); diff --git a/crates/goose/src/goose_apps/cache.rs b/crates/goose/src/goose_apps/cache.rs index c57a41b19..c13131bff 100644 --- a/crates/goose/src/goose_apps/cache.rs +++ b/crates/goose/src/goose_apps/cache.rs @@ -40,13 +40,18 @@ impl McpAppCache { } fn ensure_default_apps(&self) { - for (uri, html) in DEFAULT_APPS { - if self.get_app(APPS_EXTENSION_NAME, uri).is_none() { - if let Ok(mut app) = GooseApp::from_html(html) { - app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; - let _ = self.store_app(&app); - } - } + if fs::create_dir_all(&self.cache_dir).is_err() { + return; + } + + for (uri, _) in DEFAULT_APPS { + let Some(app) = Self::bundled_default_app(uri) else { + continue; + }; + let Ok(json) = serde_json::to_string_pretty(&app) else { + continue; + }; + let _ = fs::write(self.app_path(APPS_EXTENSION_NAME, uri), json); } } @@ -60,28 +65,80 @@ impl McpAppCache { format!("{}_{}", extension_name, hash) } + fn app_path(&self, extension_name: &str, resource_uri: &str) -> PathBuf { + self.cache_dir.join(format!( + "{}.json", + Self::cache_key(extension_name, resource_uri) + )) + } + + fn is_bundled_default_identity(extension_name: &str, resource_uri: &str) -> bool { + extension_name == APPS_EXTENSION_NAME && Self::is_bundled_default_uri(resource_uri) + } + + fn bundled_default_app(resource_uri: &str) -> Option { + let (_, html) = DEFAULT_APPS.iter().find(|(uri, _)| *uri == resource_uri)?; + let mut app = GooseApp::from_html(html).ok()?; + app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; + Some(app) + } + + pub fn restore_bundled_default_apps(apps: &mut [GooseApp]) { + for app in apps { + let is_bundled_identity = app.mcp_servers.iter().any(|extension_name| { + Self::is_bundled_default_identity(extension_name, &app.resource.uri) + }); + if is_bundled_identity { + if let Some(default_app) = Self::bundled_default_app(&app.resource.uri) { + *app = default_app; + } + } + } + } + pub fn list_apps(&self) -> Result, std::io::Error> { let mut apps = Vec::new(); - if !self.cache_dir.exists() { - return Ok(apps); - } + if self.cache_dir.exists() { + for entry in fs::read_dir(&self.cache_dir)? { + let entry = entry?; + let path = entry.path(); - for entry in fs::read_dir(&self.cache_dir)? { - let entry = entry?; - let path = entry.path(); + if let Some(app) = DEFAULT_APPS.iter().find_map(|(uri, _)| { + (path == self.app_path(APPS_EXTENSION_NAME, uri)) + .then(|| Self::bundled_default_app(uri)) + .flatten() + }) { + apps.push(app); + continue; + } - if path.extension().and_then(|s| s.to_str()) == Some("json") { - match fs::read_to_string(&path) { - Ok(content) => match serde_json::from_str::(&content) { - Ok(app) => apps.push(app), - Err(e) => warn!("Failed to parse cached app from {:?}: {}", path, e), - }, - Err(e) => warn!("Failed to read cached app from {:?}: {}", path, e), + if path.extension().and_then(|s| s.to_str()) == Some("json") { + match fs::read_to_string(&path) { + Ok(content) => match serde_json::from_str::(&content) { + Ok(app) => apps.push(app), + Err(e) => warn!("Failed to parse cached app from {:?}: {}", path, e), + }, + Err(e) => warn!("Failed to read cached app from {:?}: {}", path, e), + } } } } + Self::restore_bundled_default_apps(&mut apps); + for (uri, _) in DEFAULT_APPS { + let contains_default = apps.iter().any(|app| { + app.mcp_servers.iter().any(|extension_name| { + Self::is_bundled_default_identity(extension_name, &app.resource.uri) + && app.resource.uri == *uri + }) + }); + if !contains_default { + if let Some(app) = Self::bundled_default_app(uri) { + apps.push(app); + } + } + } Ok(apps) } @@ -90,6 +147,9 @@ impl McpAppCache { // Store the app once for each MCP server it's associated with for extension_name in &app.mcp_servers { + if Self::is_bundled_default_identity(extension_name, &app.resource.uri) { + continue; + } let cache_key = Self::cache_key(extension_name, &app.resource.uri); let app_path = self.cache_dir.join(format!("{}.json", cache_key)); let json = serde_json::to_string_pretty(app).map_err(std::io::Error::other)?; @@ -100,6 +160,10 @@ impl McpAppCache { } pub fn get_app(&self, extension_name: &str, resource_uri: &str) -> Option { + if Self::is_bundled_default_identity(extension_name, resource_uri) { + return Self::bundled_default_app(resource_uri); + } + let cache_key = Self::cache_key(extension_name, resource_uri); let app_path = self.cache_dir.join(format!("{}.json", cache_key)); @@ -117,6 +181,13 @@ impl McpAppCache { extension_name: &str, resource_uri: &str, ) -> Result<(), std::io::Error> { + if Self::is_bundled_default_identity(extension_name, resource_uri) { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "Cannot delete bundled default app", + )); + } + let cache_key = Self::cache_key(extension_name, resource_uri); let app_path = self.cache_dir.join(format!("{}.json", cache_key)); @@ -148,6 +219,7 @@ impl McpAppCache { if let Ok(content) = fs::read_to_string(&path) { if let Ok(app) = serde_json::from_str::(&content) { if app.mcp_servers.contains(&extension_name.to_string()) + && !Self::is_bundled_default_identity(extension_name, &app.resource.uri) && fs::remove_file(&path).is_ok() { deleted_count += 1; @@ -256,4 +328,107 @@ mod tests { assert_eq!(error.kind(), std::io::ErrorKind::NotFound); }); } + + #[test] + #[serial] + fn bundled_default_identity_cannot_be_replaced_or_deleted() { + with_temp_config(|| { + let cache = McpAppCache::new().unwrap(); + let expected = GooseApp::from_html(CLOCK_HTML).unwrap(); + let mut attacker_app = GooseApp::from_html(CUSTOM_APP_HTML).unwrap(); + attacker_app.resource.uri = "ui://apps/clock".to_string(); + attacker_app.resource.text = Some("".to_string()); + attacker_app.mcp_servers = vec![APPS_EXTENSION_NAME.to_string()]; + + let mut exposed_apps = vec![attacker_app.clone()]; + McpAppCache::restore_bundled_default_apps(&mut exposed_apps); + assert_eq!(exposed_apps[0].resource.text, expected.resource.text); + + cache.store_app(&attacker_app).unwrap(); + cache.delete_extension_apps(APPS_EXTENSION_NAME).unwrap(); + assert_eq!( + cache + .delete_app(APPS_EXTENSION_NAME, "ui://apps/clock") + .unwrap_err() + .kind(), + std::io::ErrorKind::PermissionDenied + ); + + let forged_json = serde_json::to_string_pretty(&attacker_app).unwrap(); + fs::write( + cache.app_path(APPS_EXTENSION_NAME, "ui://apps/clock"), + forged_json, + ) + .unwrap(); + + let reopened = McpAppCache::new().unwrap(); + let cached = reopened + .get_app(APPS_EXTENSION_NAME, "ui://apps/clock") + .unwrap(); + assert_eq!(cached.resource.text, expected.resource.text); + assert_eq!(cached.resource.name, expected.resource.name); + assert_eq!(cached.mcp_servers, vec![APPS_EXTENSION_NAME.to_string()]); + }); + } + + #[cfg(unix)] + #[test] + #[serial] + fn read_only_cache_still_exposes_compiled_default() { + use std::os::unix::fs::PermissionsExt; + + with_temp_config(|| { + let cache = McpAppCache::new().unwrap(); + let expected = GooseApp::from_html(CLOCK_HTML).unwrap(); + let mut attacker_app = GooseApp::from_html(CUSTOM_APP_HTML).unwrap(); + attacker_app.resource.uri = "ui://apps/clock".to_string(); + attacker_app.resource.text = Some("".to_string()); + attacker_app.mcp_servers.clear(); + let app_path = cache.app_path(APPS_EXTENSION_NAME, "ui://apps/clock"); + let forged_json = serde_json::to_string_pretty(&attacker_app).unwrap(); + assert!(!forged_json.contains("mcpServers")); + fs::write(&app_path, forged_json).unwrap(); + fs::set_permissions(&app_path, fs::Permissions::from_mode(0o444)).unwrap(); + fs::set_permissions(&cache.cache_dir, fs::Permissions::from_mode(0o555)).unwrap(); + + let reopened = McpAppCache::new().unwrap(); + let cached = reopened + .get_app(APPS_EXTENSION_NAME, "ui://apps/clock") + .unwrap(); + let listed = reopened.list_apps().unwrap(); + + fs::set_permissions(&cache.cache_dir, fs::Permissions::from_mode(0o755)).unwrap(); + fs::set_permissions(&app_path, fs::Permissions::from_mode(0o644)).unwrap(); + assert_eq!(cached.resource.text, expected.resource.text); + assert!(listed + .iter() + .any(|app| app.resource.uri == "ui://apps/clock" + && app.resource.text == expected.resource.text)); + }); + } + + #[cfg(unix)] + #[test] + #[serial] + fn missing_read_only_cache_entry_still_lists_compiled_default() { + use std::os::unix::fs::PermissionsExt; + + with_temp_config(|| { + let cache = McpAppCache::new().unwrap(); + let expected = GooseApp::from_html(CLOCK_HTML).unwrap(); + let app_path = cache.app_path(APPS_EXTENSION_NAME, "ui://apps/clock"); + fs::remove_file(&app_path).unwrap(); + fs::set_permissions(&cache.cache_dir, fs::Permissions::from_mode(0o555)).unwrap(); + + let reopened = McpAppCache::new().unwrap(); + let listed = reopened.list_apps().unwrap(); + + fs::set_permissions(&cache.cache_dir, fs::Permissions::from_mode(0o755)).unwrap(); + assert!(!app_path.exists()); + assert!(listed + .iter() + .any(|app| app.resource.uri == "ui://apps/clock" + && app.resource.text == expected.resource.text)); + }); + } }