fix(recipes): confine library writes (#11482)

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<PathBuf> {
+    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<PathBuf>) -> anyhow::Result<PathBuf> {
     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"));
+    }
 }

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;

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<fs::File> {
+    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<fs::File> {
     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<fs::File> {
+    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<fs::File> {
+    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<u16> = 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::*;