fix(apps): confine app file operations (#10481)

2026-07-21 06:35:21 -05:00
parent 18740b095f
commit 65e1e3d508
1 changed files with 160 additions and 4 deletions
@@ -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<GooseApp, String> {
-        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<PathBuf, String> {
+        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<String, String> {
        .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<T: serde::de::DeserializeOwned>(
    response: &Message,
    tool_name: &str,
@@ -691,3 +713,137 @@ fn extract_tool_response<T: serde::de::DeserializeOwned>(

    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("<html><body>test</body></html>".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<String> {
+        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());
+    }
+}