Right now agent extensions can specify icon paths that point anywhere. This changes it so that they can only specify icon paths that are subdirectories of the extension's root dir. Release Notes: - Restrict agent server extension icon paths to subdirectories of the extension's root directory
Unified · Split
| @@ -241,6 +241,123 @@ mod ext_agent_tests { · 240 unmodified lines | |||
| 241 | .collect(); | 241 | .collect(); |
| 242 | assert_eq!(remaining, vec!["custom".to_string()]); | 242 | assert_eq!(remaining, vec!["custom".to_string()]); |
| 243 | } | 243 | } |
| 244 | |||
| 245 | #[test] | ||
| 246 | fn resolve_extension_icon_path_allows_valid_paths() { | ||
| 247 | // Create a temporary directory structure for testing | ||
| 248 | let temp_dir = tempfile::tempdir().unwrap(); | ||
| 249 | let extensions_dir = temp_dir.path(); | ||
| 250 | let ext_dir = extensions_dir.join("my-extension"); | ||
| 251 | std::fs::create_dir_all(&ext_dir).unwrap(); | ||
| 252 | |||
| 253 | // Create a valid icon file | ||
| 254 | let icon_path = ext_dir.join("icon.svg"); | ||
| 255 | std::fs::write(&icon_path, "<svg></svg>").unwrap(); | ||
| 256 | |||
| 257 | // Test that a valid relative path works | ||
| 258 | let result = super::resolve_extension_icon_path(extensions_dir, "my-extension", "icon.svg"); | ||
| 259 | assert!(result.is_some()); | ||
| 260 | assert!(result.unwrap().ends_with("icon.svg")); | ||
| 261 | } | ||
| 262 | |||
| 263 | #[test] | ||
| 264 | fn resolve_extension_icon_path_allows_nested_paths() { | ||
| 265 | let temp_dir = tempfile::tempdir().unwrap(); | ||
| 266 | let extensions_dir = temp_dir.path(); | ||
| 267 | let ext_dir = extensions_dir.join("my-extension"); | ||
| 268 | let icons_dir = ext_dir.join("assets").join("icons"); | ||
| 269 | std::fs::create_dir_all(&icons_dir).unwrap(); | ||
| 270 | |||
| 271 | let icon_path = icons_dir.join("logo.svg"); | ||
| 272 | std::fs::write(&icon_path, "<svg></svg>").unwrap(); | ||
| 273 | |||
| 274 | let result = super::resolve_extension_icon_path( | ||
| 275 | extensions_dir, | ||
| 276 | "my-extension", | ||
| 277 | "assets/icons/logo.svg", | ||
| 278 | ); | ||
| 279 | assert!(result.is_some()); | ||
| 280 | assert!(result.unwrap().ends_with("logo.svg")); | ||
| 281 | } | ||
| 282 | |||
| 283 | #[test] | ||
| 284 | fn resolve_extension_icon_path_blocks_path_traversal() { | ||
| 285 | let temp_dir = tempfile::tempdir().unwrap(); | ||
| 286 | let extensions_dir = temp_dir.path(); | ||
| 287 | |||
| 288 | // Create two extension directories | ||
| 289 | let ext1_dir = extensions_dir.join("extension1"); | ||
| 290 | let ext2_dir = extensions_dir.join("extension2"); | ||
| 291 | std::fs::create_dir_all(&ext1_dir).unwrap(); | ||
| 292 | std::fs::create_dir_all(&ext2_dir).unwrap(); | ||
| 293 | |||
| 294 | // Create a file in extension2 | ||
| 295 | let secret_file = ext2_dir.join("secret.svg"); | ||
| 296 | std::fs::write(&secret_file, "<svg>secret</svg>").unwrap(); | ||
| 297 | |||
| 298 | // Try to access extension2's file from extension1 using path traversal | ||
| 299 | let result = super::resolve_extension_icon_path( | ||
| 300 | extensions_dir, | ||
| 301 | "extension1", | ||
| 302 | "../extension2/secret.svg", | ||
| 303 | ); | ||
| 304 | assert!( | ||
| 305 | result.is_none(), | ||
| 306 | "Path traversal to sibling extension should be blocked" | ||
| 307 | ); | ||
| 308 | } | ||
| 309 | |||
| 310 | #[test] | ||
| 311 | fn resolve_extension_icon_path_blocks_absolute_escape() { | ||
| 312 | let temp_dir = tempfile::tempdir().unwrap(); | ||
| 313 | let extensions_dir = temp_dir.path(); | ||
| 314 | let ext_dir = extensions_dir.join("my-extension"); | ||
| 315 | std::fs::create_dir_all(&ext_dir).unwrap(); | ||
| 316 | |||
| 317 | // Create a file outside the extensions directory | ||
| 318 | let outside_file = temp_dir.path().join("outside.svg"); | ||
| 319 | std::fs::write(&outside_file, "<svg>outside</svg>").unwrap(); | ||
| 320 | |||
| 321 | // Try to escape to parent directory | ||
| 322 | let result = | ||
| 323 | super::resolve_extension_icon_path(extensions_dir, "my-extension", "../outside.svg"); | ||
| 324 | assert!( | ||
| 325 | result.is_none(), | ||
| 326 | "Path traversal to parent directory should be blocked" | ||
| 327 | ); | ||
| 328 | } | ||
| 329 | |||
| 330 | #[test] | ||
| 331 | fn resolve_extension_icon_path_blocks_deep_traversal() { | ||
| 332 | let temp_dir = tempfile::tempdir().unwrap(); | ||
| 333 | let extensions_dir = temp_dir.path(); | ||
| 334 | let ext_dir = extensions_dir.join("my-extension"); | ||
| 335 | std::fs::create_dir_all(&ext_dir).unwrap(); | ||
| 336 | |||
| 337 | // Try deep path traversal | ||
| 338 | let result = super::resolve_extension_icon_path( | ||
| 339 | extensions_dir, | ||
| 340 | "my-extension", | ||
| 341 | "../../../../../../etc/passwd", | ||
| 342 | ); | ||
| 343 | assert!( | ||
| 344 | result.is_none(), | ||
| 345 | "Deep path traversal should be blocked (file doesn't exist)" | ||
| 346 | ); | ||
| 347 | } | ||
| 348 | |||
| 349 | #[test] | ||
| 350 | fn resolve_extension_icon_path_returns_none_for_nonexistent() { | ||
| 351 | let temp_dir = tempfile::tempdir().unwrap(); | ||
| 352 | let extensions_dir = temp_dir.path(); | ||
| 353 | let ext_dir = extensions_dir.join("my-extension"); | ||
| 354 | std::fs::create_dir_all(&ext_dir).unwrap(); | ||
| 355 | |||
| 356 | // Try to access a file that doesn't exist | ||
| 357 | let result = | ||
| 358 | super::resolve_extension_icon_path(extensions_dir, "my-extension", "nonexistent.svg"); | ||
| 359 | assert!(result.is_none(), "Nonexistent file should return None"); | ||
| 360 | } | ||
| 244 | } | 361 | } |
| 245 | 362 | ||
| 246 | impl AgentServerStore { | 363 | impl AgentServerStore { |
| @@ -278,7 +395,6 @@ impl AgentServerStore { · 31 unmodified lines | |||
| 278 | extension_agents.clear(); | 395 | extension_agents.clear(); |
| 279 | for (ext_id, manifest) in manifests { | 396 | for (ext_id, manifest) in manifests { |
| 280 | for (agent_name, agent_entry) in &manifest.agent_servers { | 397 | for (agent_name, agent_entry) in &manifest.agent_servers { |
| 281 | // Store absolute icon path if provided, resolving symlinks for dev extensions | ||
| 282 | // Store display name from manifest | 398 | // Store display name from manifest |
| 283 | self.agent_display_names.insert( | 399 | self.agent_display_names.insert( |
| 284 | ExternalAgentServerName(agent_name.clone().into()), | 400 | ExternalAgentServerName(agent_name.clone().into()), |
| @@ -286,18 +402,17 @@ impl AgentServerStore { · 1 unmodified line | |||
| 286 | ); | 402 | ); |
| 287 | 403 | ||
| 288 | let icon_path = if let Some(icon) = &agent_entry.icon { | 404 | let icon_path = if let Some(icon) = &agent_entry.icon { |
| 289 | let icon_path = extensions_dir.join(ext_id).join(icon); | 405 | if let Some(absolute_icon_path) = |
| 290 | // Canonicalize to resolve symlinks (dev extensions are symlinked) | 406 | resolve_extension_icon_path(&extensions_dir, ext_id, icon) |
| 291 | let absolute_icon_path = icon_path | 407 | { |
| 292 | .canonicalize() | 408 | self.agent_icons.insert( |
| 293 | .unwrap_or(icon_path) | 409 | ExternalAgentServerName(agent_name.clone().into()), |
| 294 | .to_string_lossy() | 410 | SharedString::from(absolute_icon_path.clone()), |
| 295 | .to_string(); | 411 | ); |
| 296 | self.agent_icons.insert( | 412 | Some(absolute_icon_path) |
| 297 | ExternalAgentServerName(agent_name.clone().into()), | 413 | } else { |
| 298 | SharedString::from(absolute_icon_path.clone()), | 414 | None |
| 299 | ); | 415 | } |
| 300 | Some(absolute_icon_path) | ||
| 301 | } else { | 416 | } else { |
| 302 | None | 417 | None |
| 303 | }; | 418 | }; |
| @@ -326,23 +441,18 @@ impl AgentServerStore { · 22 unmodified lines | |||
| 326 | SharedString::from(agent_entry.name.clone()), | 441 | SharedString::from(agent_entry.name.clone()), |
| 327 | ); | 442 | ); |
| 328 | 443 | ||
| 329 | // Store absolute icon path if provided, resolving symlinks for dev extensions | ||
| 330 | let icon = if let Some(icon) = &agent_entry.icon { | 444 | let icon = if let Some(icon) = &agent_entry.icon { |
| 331 | let icon_path = extensions_dir.join(ext_id).join(icon); | 445 | if let Some(absolute_icon_path) = |
| 332 | // Canonicalize to resolve symlinks (dev extensions are symlinked) | 446 | resolve_extension_icon_path(&extensions_dir, ext_id, icon) |
| 333 | let absolute_icon_path = icon_path | 447 | { |
| 334 | .canonicalize() | 448 | self.agent_icons.insert( |
| 335 | .unwrap_or(icon_path) | 449 | ExternalAgentServerName(agent_name.clone().into()), |
| 336 | .to_string_lossy() | 450 | SharedString::from(absolute_icon_path.clone()), |
| 337 | .to_string(); | 451 | ); |
| 338 | 452 | Some(absolute_icon_path) | |
| 339 | // Store icon locally for remote client | 453 | } else { |
| 340 | self.agent_icons.insert( | 454 | None |
| 341 | ExternalAgentServerName(agent_name.clone().into()), | 455 | } |
| 342 | SharedString::from(absolute_icon_path.clone()), | ||
| 343 | ); | ||
| 344 | |||
| 345 | Some(absolute_icon_path) | ||
| 346 | } else { | 456 | } else { |
| 347 | None | 457 | None |
| 348 | }; | 458 | }; |
| @@ -384,7 +494,49 @@ impl AgentServerStore { · 35 unmodified lines | |||
| 384 | pub fn agent_icon(&self, name: &ExternalAgentServerName) -> Option<SharedString> { | 494 | pub fn agent_icon(&self, name: &ExternalAgentServerName) -> Option<SharedString> { |
| 385 | self.agent_icons.get(name).cloned() | 495 | self.agent_icons.get(name).cloned() |
| 386 | } | 496 | } |
| 497 | } | ||
| 498 | |||
| 499 | /// Safely resolves an extension icon path, ensuring it stays within the extension directory. | ||
| 500 | /// Returns `None` if the path would escape the extension directory (path traversal attack). | ||
| 501 | fn resolve_extension_icon_path( | ||
| 502 | extensions_dir: &Path, | ||
| 503 | extension_id: &str, | ||
| 504 | icon_relative_path: &str, | ||
| 505 | ) -> Option<String> { | ||
| 506 | let extension_root = extensions_dir.join(extension_id); | ||
| 507 | let icon_path = extension_root.join(icon_relative_path); | ||
| 508 | |||
| 509 | // Canonicalize both paths to resolve symlinks and normalize the paths. | ||
| 510 | // For the extension root, we need to handle the case where it might be a symlink | ||
| 511 | // (common for dev extensions). | ||
| 512 | let canonical_extension_root = extension_root.canonicalize().unwrap_or(extension_root); | ||
| 513 | let canonical_icon_path = match icon_path.canonicalize() { | ||
| 514 | Ok(path) => path, | ||
| 515 | Err(err) => { | ||
| 516 | log::warn!( | ||
| 517 | "Failed to canonicalize icon path for extension '{}': {} (path: {})", | ||
| 518 | extension_id, | ||
| 519 | err, | ||
| 520 | icon_relative_path | ||
| 521 | ); | ||
| 522 | return None; | ||
| 523 | } | ||
| 524 | }; | ||
| 387 | 525 | ||
| 526 | // Verify the resolved icon path is within the extension directory | ||
| 527 | if canonical_icon_path.starts_with(&canonical_extension_root) { | ||
| 528 | Some(canonical_icon_path.to_string_lossy().to_string()) | ||
| 529 | } else { | ||
| 530 | log::warn!( | ||
| 531 | "Icon path '{}' for extension '{}' escapes extension directory, ignoring for security", | ||
| 532 | icon_relative_path, | ||
| 533 | extension_id | ||
| 534 | ); | ||
| 535 | None | ||
| 536 | } | ||
| 537 | } | ||
| 538 | |||
| 539 | impl AgentServerStore { | ||
| 388 | pub fn agent_display_name(&self, name: &ExternalAgentServerName) -> Option<SharedString> { | 540 | pub fn agent_display_name(&self, name: &ExternalAgentServerName) -> Option<SharedString> { |
| 389 | self.agent_display_names.get(name).cloned() | 541 | self.agent_display_names.get(name).cloned() |
| 390 | } | 542 | } |