diff --git a/crates/goose/src/recipe/local_recipes.rs b/crates/goose/src/recipe/local_recipes.rs index e4edc3370..02e3a0322 100644 --- a/crates/goose/src/recipe/local_recipes.rs +++ b/crates/goose/src/recipe/local_recipes.rs @@ -7,6 +7,7 @@ use crate::config::paths::Paths; use crate::recipe::read_recipe_file_content::{read_recipe_file, RecipeFile}; use crate::recipe::Recipe; use crate::recipe::RECIPE_FILE_EXTENSIONS; +use crate::skills::{create_source_file, write_source_file}; const GOOSE_RECIPE_PATH_ENV_VAR: &str = "GOOSE_RECIPE_PATH"; @@ -168,33 +169,175 @@ fn generate_recipe_filename(title: &str, recipe_library_dir: &Path) -> PathBuf { }; let mut candidate = recipe_library_dir.join(format!("{}.yaml", filename)); - if !candidate.exists() { + if fs::symlink_metadata(&candidate).is_err() { return candidate; } let mut counter = 1; loop { candidate = recipe_library_dir.join(format!("{}-{}.yaml", filename, counter)); - if !candidate.exists() { + if fs::symlink_metadata(&candidate).is_err() { return candidate; } counter += 1; } } +fn save_new_recipe_to_dir( + title: &str, + recipe_library_dir: &Path, + yaml_content: &[u8], +) -> anyhow::Result { + fs::create_dir_all(recipe_library_dir)?; + let recipe_library_dir = recipe_library_dir.canonicalize()?; + loop { + let file_path = generate_recipe_filename(title, &recipe_library_dir); + let file_name = file_path + .file_name() + .ok_or_else(|| anyhow!("Recipe path has no file name: {}", file_path.display()))?; + match create_source_file(&recipe_library_dir, Path::new(file_name), yaml_content) { + Ok(()) => return Ok(file_path), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(error.into()), + } + } +} + pub fn save_recipe_to_file(recipe: Recipe, file_path: Option) -> anyhow::Result { let recipe_library_dir = get_recipe_library_dir(true); + let yaml_content = recipe.to_yaml()?; - let file_path_value = match file_path { - Some(path) => path, - None => generate_recipe_filename(&recipe.title, &recipe_library_dir), - }; - - if let Some(parent) = file_path_value.parent() { - fs::create_dir_all(parent)?; + if let Some(file_path) = file_path { + let parent = file_path + .parent() + .ok_or_else(|| anyhow!("Recipe path has no parent: {}", file_path.display()))?; + let file_name = file_path + .file_name() + .ok_or_else(|| anyhow!("Recipe path has no file name: {}", file_path.display()))?; + write_source_file(parent, Path::new(file_name), yaml_content.as_bytes())?; + return Ok(file_path); } - let yaml_content = recipe.to_yaml()?; - fs::write(&file_path_value, yaml_content)?; - Ok(file_path_value) + save_new_recipe_to_dir(&recipe.title, &recipe_library_dir, yaml_content.as_bytes()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn recipe(title: &str, instructions: &str) -> Recipe { + Recipe::builder() + .title(title) + .description("Test recipe") + .instructions(instructions) + .build() + .unwrap() + } + + fn write_recipe(path: &Path, title: &str, instructions: &str) { + fs::write(path, recipe(title, instructions).to_yaml().unwrap()).unwrap(); + } + + #[cfg(unix)] + #[test] + fn listed_recipe_replaced_by_symlink_is_not_saved() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let recipe_root = root.path().canonicalize().unwrap(); + let recipe_path = recipe_root.join("listed.yaml"); + let outside_path = outside.path().join("outside.yaml"); + write_recipe(&recipe_path, "Listed", "original"); + write_recipe(&outside_path, "Outside", "must stay unchanged"); + let outside_content = fs::read_to_string(&outside_path).unwrap(); + + let listed_path = scan_directory_for_recipes(&recipe_root).unwrap()[0] + .0 + .clone(); + fs::remove_file(&recipe_path).unwrap(); + std::os::unix::fs::symlink(&outside_path, &recipe_path).unwrap(); + + let result = save_recipe_to_file(recipe("Listed", "updated"), Some(listed_path)); + + assert!(result.is_err()); + assert_eq!(fs::read_to_string(outside_path).unwrap(), outside_content); + } + + #[test] + fn listed_recipe_replaced_by_hard_link_is_not_saved() { + let root = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let recipe_root = root.path().canonicalize().unwrap(); + let recipe_path = recipe_root.join("listed.yaml"); + let outside_path = outside.path().join("outside.yaml"); + write_recipe(&recipe_path, "Listed", "original"); + write_recipe(&outside_path, "Outside", "must stay unchanged"); + let outside_content = fs::read_to_string(&outside_path).unwrap(); + + let listed_path = scan_directory_for_recipes(&recipe_root).unwrap()[0] + .0 + .clone(); + fs::remove_file(&recipe_path).unwrap(); + fs::hard_link(&outside_path, &recipe_path).unwrap(); + + let result = save_recipe_to_file(recipe("Listed", "updated"), Some(listed_path)); + + assert!(result.is_err()); + assert_eq!(fs::read_to_string(outside_path).unwrap(), outside_content); + } + + #[cfg(unix)] + #[test] + fn listed_recipe_with_replaced_ancestor_is_not_saved() { + let parent = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let parent = parent.path().canonicalize().unwrap(); + let recipes = parent.join("recipes"); + let moved_recipes = parent.join("moved-recipes"); + fs::create_dir(&recipes).unwrap(); + let recipe_path = recipes.join("listed.yaml"); + let outside_path = outside.path().join("listed.yaml"); + write_recipe(&recipe_path, "Listed", "original"); + write_recipe(&outside_path, "Outside", "must stay unchanged"); + let outside_content = fs::read_to_string(&outside_path).unwrap(); + + let listed_path = scan_directory_for_recipes(&recipes).unwrap()[0].0.clone(); + fs::rename(&recipes, moved_recipes).unwrap(); + std::os::unix::fs::symlink(outside.path(), &recipes).unwrap(); + + let result = save_recipe_to_file(recipe("Listed", "updated"), Some(listed_path)); + + assert!(result.is_err()); + assert_eq!(fs::read_to_string(outside_path).unwrap(), outside_content); + } + + #[cfg(unix)] + #[test] + fn new_recipe_creation_does_not_follow_dangling_symlink_collision() { + let recipes = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let recipe_root = recipes.path().canonicalize().unwrap(); + let outside_path = outside.path().join("missing.yaml"); + std::os::unix::fs::symlink(&outside_path, recipe_root.join("new-recipe.yaml")).unwrap(); + + let saved = save_new_recipe_to_dir("New Recipe", &recipe_root, b"safe recipe").unwrap(); + + assert_eq!(saved, recipe_root.join("new-recipe-1.yaml")); + assert_eq!(fs::read_to_string(saved).unwrap(), "safe recipe"); + assert!(!outside_path.exists()); + } + + #[test] + fn listed_regular_recipe_can_be_updated() { + let recipes = tempfile::tempdir().unwrap(); + let recipe_root = recipes.path().canonicalize().unwrap(); + let recipe_path = recipe_root.join("listed.yaml"); + write_recipe(&recipe_path, "Listed", "original"); + let listed_path = scan_directory_for_recipes(&recipe_root).unwrap()[0] + .0 + .clone(); + + save_recipe_to_file(recipe("Listed", "updated"), Some(listed_path)).unwrap(); + + assert!(fs::read_to_string(recipe_path).unwrap().contains("updated")); + } } diff --git a/crates/goose/src/skills/mod.rs b/crates/goose/src/skills/mod.rs index 6c641e43f..3e2a8180c 100644 --- a/crates/goose/src/skills/mod.rs +++ b/crates/goose/src/skills/mod.rs @@ -8,7 +8,9 @@ pub mod client; mod supporting_files; pub use client::{SkillsClient, EXTENSION_NAME}; -pub(crate) use supporting_files::{load_supporting_file, read_source_file}; +pub(crate) use supporting_files::{ + create_source_file, load_supporting_file, read_source_file, write_source_file, +}; use crate::config::{paths::Paths, Config}; use crate::plugins::discovery::PluginScope; diff --git a/crates/goose/src/skills/supporting_files.rs b/crates/goose/src/skills/supporting_files.rs index d383f0b10..88f57f84d 100644 --- a/crates/goose/src/skills/supporting_files.rs +++ b/crates/goose/src/skills/supporting_files.rs @@ -1,5 +1,5 @@ use std::fs; -use std::io::{self, Read}; +use std::io::{self, Read, Write}; use std::path::{Component, Path}; const LOADED_FILE_PREFIX: &str = "# Loaded: "; @@ -69,6 +69,22 @@ pub(crate) fn read_source_file(source_dir: &Path, relative: &Path) -> io::Result ) } +pub(crate) fn write_source_file( + source_dir: &Path, + relative: &Path, + content: &[u8], +) -> io::Result<()> { + write_confined_file_with_hook(source_dir, relative, content, false, |_| {}) +} + +pub(crate) fn create_source_file( + source_dir: &Path, + relative: &Path, + content: &[u8], +) -> io::Result<()> { + write_confined_file_with_hook(source_dir, relative, content, true, |_| {}) +} + fn read_supporting_file_with_hook( skill_dir: &Path, relative: &Path, @@ -299,6 +315,58 @@ fn read_confined_file_with_hook( read_opened_file(file, limit) } +#[cfg(unix)] +fn write_confined_file_with_hook( + source_dir: &Path, + relative: &Path, + content: &[u8], + create_new: bool, + mut after_opened_component: impl FnMut(&Path), +) -> io::Result<()> { + let components = validated_relative_components(relative)?; + let (file_name, ancestors) = components.split_last().unwrap(); + let mut directory = open_skill_root(source_dir, &mut after_opened_component)?; + + let mut opened_path = std::path::PathBuf::new(); + for ancestor in ancestors { + directory = open_at(&directory, ancestor, directory_traversal_flags())?; + opened_path.push(ancestor); + after_opened_component(&opened_path); + } + + let mut flags = + libc::O_WRONLY | libc::O_CREAT | libc::O_NONBLOCK | libc::O_NOFOLLOW | libc::O_CLOEXEC; + if create_new { + flags |= libc::O_EXCL; + } + let mut file = open_at_with_mode(&directory, file_name, flags, 0o666)?; + let metadata = file.metadata()?; + if !metadata.is_file() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "source path is not a regular file", + )); + } + ensure_source_file_has_single_link(&file, &metadata)?; + if !create_new { + file.set_len(0)?; + } + file.write_all(content) +} + +#[cfg(unix)] +fn ensure_source_file_has_single_link(_file: &fs::File, metadata: &fs::Metadata) -> io::Result<()> { + use std::os::unix::fs::MetadataExt; + + if metadata.nlink() != 1 { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "source path must have exactly one hard link", + )); + } + Ok(()) +} + #[cfg(unix)] fn open_at( directory: &fs::File, @@ -324,6 +392,39 @@ fn open_at( Ok(unsafe { fs::File::from_raw_fd(descriptor) }) } +#[cfg(unix)] +fn open_at_with_mode( + directory: &fs::File, + name: &std::ffi::OsStr, + flags: libc::c_int, + mode: libc::mode_t, +) -> io::Result { + use std::ffi::CString; + use std::os::fd::{AsRawFd, FromRawFd}; + use std::os::unix::ffi::OsStrExt; + + let name = CString::new(name.as_bytes()).map_err(|_| { + io::Error::new( + io::ErrorKind::InvalidInput, + "source file path contains a NUL byte", + ) + })?; + // SAFETY: openat does not retain the name pointer, and mode is supplied for O_CREAT. + let descriptor = unsafe { + libc::openat( + directory.as_raw_fd(), + name.as_ptr(), + flags, + libc::c_uint::from(mode), + ) + }; + if descriptor < 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: openat returned a new owned descriptor on success. + Ok(unsafe { fs::File::from_raw_fd(descriptor) }) +} + #[cfg(windows)] fn open_skill_root( skill_dir: &Path, @@ -430,6 +531,67 @@ fn read_confined_file_with_hook( read_opened_file(file, limit) } +#[cfg(windows)] +fn write_confined_file_with_hook( + source_dir: &Path, + relative: &Path, + content: &[u8], + create_new: bool, + mut after_opened_component: impl FnMut(&Path), +) -> io::Result<()> { + let components = validated_relative_components(relative)?; + let (file_name, ancestors) = components.split_last().unwrap(); + let mut directory = open_skill_root(source_dir, &mut after_opened_component)?; + + let mut opened_path = std::path::PathBuf::new(); + for ancestor in ancestors { + directory = windows_open_at(&directory, ancestor, true)?; + let metadata = directory.metadata()?; + if windows_metadata_is_reparse_point(&metadata) || !metadata.is_dir() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "source path ancestor is not a regular directory", + )); + } + opened_path.push(ancestor); + after_opened_component(&opened_path); + } + + let mut file = windows_open_file_at(&directory, file_name, create_new)?; + let metadata = file.metadata()?; + if windows_metadata_is_reparse_point(&metadata) || !metadata.is_file() { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "source path is not a regular file", + )); + } + ensure_source_file_has_single_link(&file, &metadata)?; + if !create_new { + file.set_len(0)?; + } + file.write_all(content) +} + +#[cfg(windows)] +fn ensure_source_file_has_single_link(file: &fs::File, _metadata: &fs::Metadata) -> io::Result<()> { + use std::os::windows::io::AsRawHandle; + use winapi::um::fileapi::{GetFileInformationByHandle, BY_HANDLE_FILE_INFORMATION}; + + // SAFETY: BY_HANDLE_FILE_INFORMATION is a plain C data structure initialized before the call. + let mut information: BY_HANDLE_FILE_INFORMATION = unsafe { std::mem::zeroed() }; + // SAFETY: the file owns a valid handle and information points to writable initialized storage. + if unsafe { GetFileInformationByHandle(file.as_raw_handle(), &mut information) } == 0 { + return Err(io::Error::last_os_error()); + } + if information.nNumberOfLinks != 1 { + return Err(io::Error::new( + io::ErrorKind::InvalidInput, + "source path must have exactly one hard link", + )); + } + Ok(()) +} + #[cfg(windows)] fn windows_open_at( directory: &fs::File, @@ -437,18 +599,63 @@ fn windows_open_at( directory_only: bool, ) -> io::Result { use ntapi::ntioapi::{ - NtCreateFile, FILE_DIRECTORY_FILE, FILE_OPEN, FILE_OPEN_REPARSE_POINT, - FILE_SYNCHRONOUS_IO_NONALERT, IO_STATUS_BLOCK, + FILE_DIRECTORY_FILE, FILE_OPEN, FILE_OPEN_REPARSE_POINT, FILE_SYNCHRONOUS_IO_NONALERT, }; + use winapi::um::winnt::{FILE_GENERIC_READ, FILE_READ_ATTRIBUTES, FILE_TRAVERSE, SYNCHRONIZE}; + + let mut create_options = FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT; + if directory_only { + create_options |= FILE_DIRECTORY_FILE; + } + let desired_access = if directory_only { + FILE_TRAVERSE | FILE_READ_ATTRIBUTES | SYNCHRONIZE + } else { + FILE_GENERIC_READ + }; + windows_open_at_with_options(directory, name, desired_access, FILE_OPEN, create_options) +} + +#[cfg(windows)] +fn windows_open_file_at( + directory: &fs::File, + name: &std::ffi::OsStr, + create_new: bool, +) -> io::Result { + use ntapi::ntioapi::{ + FILE_CREATE, FILE_NON_DIRECTORY_FILE, FILE_OPEN_IF, FILE_OPEN_REPARSE_POINT, + FILE_SYNCHRONOUS_IO_NONALERT, + }; + use winapi::um::winnt::{FILE_GENERIC_WRITE, FILE_READ_ATTRIBUTES, SYNCHRONIZE}; + + let create_disposition = if create_new { + FILE_CREATE + } else { + FILE_OPEN_IF + }; + windows_open_at_with_options( + directory, + name, + FILE_GENERIC_WRITE | FILE_READ_ATTRIBUTES | SYNCHRONIZE, + create_disposition, + FILE_NON_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT, + ) +} + +#[cfg(windows)] +fn windows_open_at_with_options( + directory: &fs::File, + name: &std::ffi::OsStr, + desired_access: u32, + create_disposition: u32, + create_options: u32, +) -> io::Result { + use ntapi::ntioapi::{NtCreateFile, IO_STATUS_BLOCK}; use std::os::windows::ffi::OsStrExt; use std::os::windows::io::{AsRawHandle, FromRawHandle}; use winapi::shared::ntdef::{ HANDLE, NT_SUCCESS, OBJECT_ATTRIBUTES, OBJ_CASE_INSENSITIVE, UNICODE_STRING, }; - use winapi::um::winnt::{ - FILE_GENERIC_READ, FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, FILE_SHARE_READ, - FILE_SHARE_WRITE, FILE_TRAVERSE, SYNCHRONIZE, - }; + use winapi::um::winnt::{FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE}; let mut name: Vec = name.encode_wide().collect(); let name_bytes = name @@ -477,15 +684,6 @@ fn windows_open_at( let mut handle: HANDLE = std::ptr::null_mut(); // SAFETY: IO_STATUS_BLOCK is a plain C data structure initialized before the synchronous call. let mut io_status: IO_STATUS_BLOCK = unsafe { std::mem::zeroed() }; - let mut create_options = FILE_OPEN_REPARSE_POINT | FILE_SYNCHRONOUS_IO_NONALERT; - if directory_only { - create_options |= FILE_DIRECTORY_FILE; - } - let desired_access = if directory_only { - FILE_TRAVERSE | FILE_READ_ATTRIBUTES | SYNCHRONIZE - } else { - FILE_GENERIC_READ - }; // SAFETY: all pointers reference initialized values for the duration of the synchronous call. let status = unsafe { NtCreateFile( @@ -496,7 +694,7 @@ fn windows_open_at( std::ptr::null_mut(), 0, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - FILE_OPEN, + create_disposition, create_options, std::ptr::null_mut(), 0, @@ -538,6 +736,21 @@ fn read_confined_file_with_hook( )) } +#[cfg(not(any(unix, windows)))] +fn write_confined_file_with_hook( + _source_dir: &Path, + relative: &Path, + _content: &[u8], + _create_new: bool, + _after_opened_component: impl FnMut(&Path), +) -> io::Result<()> { + validated_relative_components(relative)?; + Err(io::Error::new( + io::ErrorKind::Unsupported, + "secure source file writes are not supported on this platform", + )) +} + #[cfg(test)] mod tests { use super::*;