diff --git a/crates/goose/src/agents/platform_extensions/apps.rs b/crates/goose/src/agents/platform_extensions/apps.rs index 80383a6a4..f3646fd47 100644 --- a/crates/goose/src/agents/platform_extensions/apps.rs +++ b/crates/goose/src/agents/platform_extensions/apps.rs @@ -19,7 +19,7 @@ use serde::{Deserialize, Serialize}; use serde_json::json; use std::collections::HashMap; use std::fs; -use std::path::PathBuf; +use std::path::{Component, Path, PathBuf}; use std::sync::Arc; use tokio_util::sync::CancellationToken; @@ -167,7 +167,7 @@ impl AppsManagerClient { } fn load_app(&self, name: &str) -> Result { - let path = self.apps_dir.join(format!("{}.html", name)); + let path = self.app_path(name)?; let html = fs::read_to_string(&path).map_err(|e| format!("Failed to read app file: {}", e))?; @@ -176,7 +176,7 @@ impl AppsManagerClient { } fn save_app(&self, app: &GooseApp) -> Result<(), String> { - let path = self.apps_dir.join(format!("{}.html", app.resource.name)); + let path = self.app_path(&app.resource.name)?; let html_content = app.to_html()?; @@ -186,13 +186,21 @@ impl AppsManagerClient { } fn delete_app(&self, name: &str) -> Result<(), String> { - let path = self.apps_dir.join(format!("{}.html", name)); + let path = self.app_path(name)?; fs::remove_file(&path).map_err(|e| format!("Failed to delete app file: {}", e))?; Ok(()) } + fn app_path(&self, name: &str) -> Result { + if !is_single_file_name(name) { + return Err("Invalid app name: expected a single file name".to_string()); + } + + Ok(self.apps_dir.join(format!("{name}.html"))) + } + fn with_platform_notification( &self, result: CallToolResult, @@ -663,6 +671,20 @@ fn extract_string(args: &JsonObject, key: &str) -> Result { .ok_or_else(|| format!("Missing or invalid '{}'", key)) } +fn is_single_file_name(name: &str) -> bool { + if name.contains(['/', '\\']) || has_windows_drive_prefix(name) { + return false; + } + + let mut components = Path::new(name).components(); + matches!(components.next(), Some(Component::Normal(_))) && components.next().is_none() +} + +fn has_windows_drive_prefix(name: &str) -> bool { + let bytes = name.as_bytes(); + bytes.len() >= 2 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':' +} + fn extract_tool_response( response: &Message, tool_name: &str, @@ -691,3 +713,137 @@ fn extract_tool_response( Err(format!("LLM did not call the required tool: {}", tool_name)) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::session::SessionManager; + use std::path::Path; + + fn test_client(apps_dir: PathBuf) -> AppsManagerClient { + AppsManagerClient { + info: AppsManagerClient::create_info(), + context: PlatformExtensionContext { + extension_manager: None, + session_manager: Arc::new(SessionManager::new(apps_dir.join("sessions"))), + session: None, + use_login_shell_path: false, + }, + apps_dir, + } + } + + fn test_app(name: &str) -> GooseApp { + GooseApp { + resource: McpAppResource { + uri: format!("ui://apps/{name}"), + name: name.to_string(), + description: Some("Test app".to_string()), + mime_type: "text/html;profile=mcp-app".to_string(), + text: Some("test".to_string()), + blob: None, + meta: None, + }, + mcp_servers: vec![EXTENSION_NAME.to_string()], + window_props: None, + prd: None, + deletable: true, + } + } + + fn invalid_app_names(temp_dir: &Path) -> Vec { + vec![ + "../outside".to_string(), + "nested/outside".to_string(), + "nested/".to_string(), + "nested/.".to_string(), + r"nested\outside".to_string(), + temp_dir.join("absolute").to_string_lossy().into_owned(), + "C:outside".to_string(), + ] + } + + fn assert_invalid_name(error: String) { + assert!( + error.contains("Invalid app name"), + "expected app-name validation error, got: {error}" + ); + } + + #[tokio::test] + async fn load_and_resource_read_reject_unsafe_app_names() { + 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); + fs::write( + temp.path().join("outside.html"), + test_app("outside").to_html().unwrap(), + ) + .unwrap(); + + for name in invalid_app_names(temp.path()) { + assert_invalid_name(client.load_app(&name).unwrap_err()); + + let result = client + .read_resource( + "session", + &format!("ui://apps/{name}"), + CancellationToken::new(), + ) + .await; + assert!( + result.is_err(), + "resource read accepted unsafe name: {name}" + ); + } + + client.save_app(&test_app("legitimate-app")).unwrap(); + assert!(client.load_app("legitimate-app").is_ok()); + assert!(client + .read_resource( + "session", + "ui://apps/legitimate-app", + CancellationToken::new(), + ) + .await + .is_ok()); + } + + #[tokio::test] + async fn provider_generated_save_rejects_unsafe_app_names() { + 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); + + for name in invalid_app_names(temp.path()) { + assert_invalid_name(client.save_app(&test_app(&name)).unwrap_err()); + } + + client.save_app(&test_app("legitimate-app")).unwrap(); + assert_eq!( + client.load_app("legitimate-app").unwrap().resource.name, + "legitimate-app" + ); + } + + #[tokio::test] + async fn delete_rejects_unsafe_app_names() { + 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 outside_path = temp.path().join("outside.html"); + fs::write(&outside_path, "outside").unwrap(); + + for name in invalid_app_names(temp.path()) { + assert_invalid_name(client.delete_app(&name).unwrap_err()); + } + + assert!(outside_path.exists()); + client.save_app(&test_app("legitimate-app")).unwrap(); + client.delete_app("legitimate-app").unwrap(); + assert!(!client.apps_dir.join("legitimate-app.html").exists()); + } +}