diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 599836a40d..5c2397ba5c 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3210,6 +3210,7 @@ dependencies = [ "codex-cloud-config", "codex-config", "codex-core", + "codex-exec-server", "codex-features", "codex-feedback", "codex-git-utils", diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index 6d77d325a2..f1fdbddde3 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -140,6 +140,7 @@ pub use fs_helper::CODEX_FS_HELPER_ARG1; pub use fs_helper_main::main as run_fs_helper_main; pub use local_file_system::LOCAL_FS; pub use local_file_system::LocalFileSystem; +pub use no_follow::BoundOutput; pub use noise_channel::NoiseChannelError; pub use noise_channel::NoiseChannelIdentity; pub use noise_channel::NoiseChannelPublicKey; diff --git a/codex-rs/exec-server/src/no_follow/mod.rs b/codex-rs/exec-server/src/no_follow/mod.rs index d7abd7ff9c..8e701f9eea 100644 --- a/codex-rs/exec-server/src/no_follow/mod.rs +++ b/codex-rs/exec-server/src/no_follow/mod.rs @@ -16,6 +16,79 @@ use unix as imp; #[cfg(windows)] use windows as imp; +fn invalid_output(reason: &'static str) -> io::Error { + io::Error::new(io::ErrorKind::InvalidInput, reason) +} + +/// A destination bound before sandboxed tools run. Only ordinary path-based files are truncated. +pub struct BoundOutput { + file: std::fs::File, + destination: Destination, +} + +enum Destination { + Path(std::path::PathBuf), + Stream { is_stdout: bool }, +} + +impl BoundOutput { + pub fn open(path: &Path) -> io::Result { + // Check raw syntax before absolute() or components() discards directory markers. + let bytes = path.as_os_str().as_encoded_bytes(); + let leaf = bytes + .rsplit(|b| *b == b'/' || (cfg!(windows) && *b == b'\\')) + .next(); + if matches!(leaf, None | Some(b"" | b"." | b"..")) { + return Err(invalid_output("output must name a file")); + } + #[cfg(windows)] + if bytes.eq_ignore_ascii_case(b"NUL") || bytes.eq_ignore_ascii_case(br"\\.\NUL") { + return Ok(Self { + file: windows::open_null_output()?, + destination: Destination::Stream { is_stdout: false }, + }); + } + #[cfg(unix)] + if let Some(output) = unix::inherited_output(path)? { + return Ok(output); + } + #[cfg(unix)] + let path = unix::output_path(path)?; + #[cfg(windows)] + let path = std::path::absolute(path)?; + let file = imp::open_output(&path)?; + if file.metadata()?.is_file() { + imp::reject_multiple_links(&file)?; + } + Ok(Self { + file, + destination: Destination::Path(path), + }) + } + + /// Whether the caller explicitly selected the inherited standard output stream. + pub fn is_stdout(&self) -> bool { + matches!(self.destination, Destination::Stream { is_stdout: true }) + } + + pub fn write(mut self, contents: &[u8]) -> io::Result<()> { + use std::io::Write; + if let Destination::Path(path) = &self.destination { + imp::verify_output_path(&self.file, path)?; + if self.file.metadata()?.is_file() { + imp::reject_multiple_links(&self.file)?; + self.file.set_len(0)?; + } + } + self.file.write_all(contents)?; + // A caller may reopen the pathname after a successful exit. Detect a visible swap. + if let Destination::Path(path) = &self.destination { + imp::verify_output_path(&self.file, path)?; + } + Ok(()) + } +} + pub(crate) fn open_file(path: &Path) -> io::Result { imp::open_file_sync(path) } @@ -58,3 +131,237 @@ pub(crate) async fn create_directory(path: &Path, recursive: bool) -> io::Result pub(crate) async fn remove(path: &Path, recursive: bool, force: bool) -> io::Result<()> { imp::remove(path.to_path_buf(), recursive, force).await } + +#[cfg(test)] +mod output_tests { + //! Exercise the destination as a caller of the final-message writer. + use super::BoundOutput; + use pretty_assertions::assert_eq; + use std::fs; + + #[test] + fn regular_output_is_truncated_only_when_written() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("message"); + fs::write(&path, "old message")?; + let output = BoundOutput::open(&path)?; + assert_eq!(fs::read_to_string(&path)?, "old message"); + output.write(b"new")?; + assert_eq!(fs::read_to_string(&path)?, "new"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn rejects_added_links_and_replaced_paths_before_writing() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + let path = dir.path().join("message"); + fs::write(&path, "new")?; + let output = BoundOutput::open(&path)?; + let alias = dir.path().join("alias"); + fs::hard_link(&path, &alias)?; + assert!(BoundOutput::open(&alias).is_err()); + assert!(output.write(b"overwritten").is_err()); + assert_eq!(fs::read(&alias)?, b"new"); + fs::remove_file(&alias)?; + let output = BoundOutput::open(&path)?; + fs::rename(&path, &alias)?; + fs::write(&path, "replacement")?; + assert!(output.write(b"overwritten").is_err()); + assert_eq!(fs::read(&path)?, b"replacement"); + assert_eq!(fs::read(&alias)?, b"new"); + Ok(()) + } + + #[cfg(windows)] + #[test] + fn windows_output_binds_the_chosen_stream_until_write() -> std::io::Result<()> { + use std::os::windows::fs::OpenOptionsExt; + use windows_sys::Win32::Foundation::ERROR_SHARING_VIOLATION; + use windows_sys::Win32::Storage::FileSystem::DELETE; + let dir = tempfile::tempdir()?; + let base = dir.path().join("message"); + fs::write(&base, "base")?; + let fs_name = super::windows::filesystem_name(&fs::File::open(&base)?)?; + let ntfs = fs_name.eq_ignore_ascii_case("NTFS"); + let sibling = dir.path().join("message:sibling"); + if ntfs { + fs::write(&sibling, "keep")?; + let alias = dir.path().join("alias"); + fs::hard_link(&base, &alias)?; + assert!(BoundOutput::open(&base).is_err()); + fs::remove_file(alias)?; + } + let names: &[&str] = if ntfs { + &[ + "message", + "message::$DATA", + "message:answer", + "message:answer:$DATA", + ] + } else { + &["message"] + }; + for name in names { + let path = dir.path().join(name); + fs::write(&base, "base")?; + fs::write(&path, "previous contents")?; + let open_for_rename = || fs::OpenOptions::new().access_mode(DELETE).open(&path); + let rename_handle = open_for_rename()?; + assert!( + matches!(BoundOutput::open(&path), Err(e) if e.raw_os_error() == Some(ERROR_SHARING_VIOLATION as i32)) + ); + drop(rename_handle); + + let output = BoundOutput::open(&path)?; + assert!( + matches!(open_for_rename(), Err(e) if e.raw_os_error() == Some(ERROR_SHARING_VIOLATION as i32)) + ); + output.write(b"done")?; + assert_eq!(fs::read(&path)?, b"done"); + drop(open_for_rename()?); + if name.starts_with("message:answer") { + assert_eq!(fs::read(&base)?, b"base"); + } + if ntfs { + assert_eq!(fs::read(&sibling)?, b"keep"); + } + } + Ok(()) + } + + #[cfg(windows)] + #[test] + fn nul_discards_output_without_accepting_other_devices() -> std::io::Result<()> { + for name in ["NUL", "nul", r"\\.\NUL"] { + BoundOutput::open(name.as_ref())?.write(b"discarded")?; + } + assert!(BoundOutput::open(r"\\.\CON".as_ref()).is_err()); + Ok(()) + } + + #[cfg(windows)] + #[test] + fn unavailable_full_ids_use_directory_entries_only_on_fat_volumes() -> std::io::Result<()> { + use windows_sys::Win32::Foundation::ERROR_INVALID_PARAMETER; + let dir = tempfile::tempdir()?; + let path = dir.path().join("message"); + let output = BoundOutput::open(&path)?; + let other = dir.path().join("other"); + fs::write(&other, "other")?; + // Legacy IDs on a ReFS temp volume are not a valid stand-in for FAT directory entries. + let host = super::windows::filesystem_name(&output.file)?; + let legacy_fixture = ["NTFS", "FAT", "FAT32", "exFAT"] + .iter() + .any(|name| host.eq_ignore_ascii_case(name)); + let verify = |candidate: &std::path::Path, volume: &str, named: Option<&str>| { + super::windows::verify_output_path_with( + &output.file, + candidate, + |_| { + Err(std::io::Error::from_raw_os_error( + ERROR_INVALID_PARAMETER as i32, + )) + }, + |file| { + if std::ptr::eq(file, &output.file) { + return Ok(volume.to_owned()); + } + named + .map(str::to_owned) + .ok_or(std::io::ErrorKind::Unsupported.into()) + }, + ) + }; + for volume in ["FAT", "FAT32", "exFAT"] { + // Reaching the second volume query proves the first one selected the fallback. + assert!( + matches!(verify(&path, volume, None), Err(e) if e.kind() == std::io::ErrorKind::Unsupported) + ); + if legacy_fixture { + verify(&path, volume, Some(volume))?; + assert!(verify(&other, volume, Some(volume)).is_err()); + } + } + for (bound, named) in [("ReFS", "exFAT"), ("exFAT", "ReFS")] { + assert!( + matches!(verify(&path, bound, Some(named)), Err(e) if e.raw_os_error() == Some(ERROR_INVALID_PARAMETER as i32)) + ); + } + Ok(()) + } + + #[cfg(windows)] + #[test] + #[ignore = "requires CODEX_TEST_EXFAT_DIR on a writable exFAT volume"] + fn writes_on_exfat() -> anyhow::Result<()> { + let dir = tempfile::tempdir_in(std::env::var("CODEX_TEST_EXFAT_DIR")?)?; + let path = dir.path().join("message"); + fs::write(&path, "previous message")?; + assert!( + super::windows::filesystem_name(&fs::File::open(&path)?)?.eq_ignore_ascii_case("exFAT") + ); + BoundOutput::open(&path)?.write(b"done")?; + assert_eq!(fs::read(&path)?, b"done"); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn output_only_accepts_leading_parent_components() -> std::io::Result<()> { + let dir = tempfile::tempdir()?; + fs::create_dir_all(dir.path().join("a/b"))?; + let path = dir.path().join("a/b/../../message"); + assert!( + matches!(BoundOutput::open(&path), Err(e) if e.kind() == std::io::ErrorKind::InvalidInput) + ); + assert!(!dir.path().join("message").exists()); + let mut expected = std::env::current_dir()?; + expected.pop(); + assert_eq!( + super::unix::output_path("./../message".as_ref())?, + expected.join("message") + ); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn writes_fifo_device_and_inherited_pipe() -> anyhow::Result<()> { + use std::io::Read; + use std::os::fd::AsRawFd; + use std::os::unix::fs::OpenOptionsExt; + let dir = tempfile::tempdir()?; + let fifo = dir.path().join("pipe"); + assert!( + std::process::Command::new("mkfifo") + .arg(&fifo) + .status()? + .success() + ); + let mut reader = fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NONBLOCK) + .open(&fifo)?; + BoundOutput::open(&fifo)?.write(b"fifo")?; + let mut buf = [0; 4]; + reader.read_exact(&mut buf)?; + assert_eq!(buf, *b"fifo"); + BoundOutput::open(std::path::Path::new("/dev/null"))?.write(b"device")?; + let stdout = BoundOutput::open(std::path::Path::new("/dev/stdout"))?; + assert!(stdout.is_stdout()); + let read_only = rustix::io::dup(&reader)?; + let alias = format!("/dev/fd/{}", read_only.as_raw_fd()); + assert!( + matches!(BoundOutput::open(alias.as_ref()), Err(e) if e.kind() == std::io::ErrorKind::InvalidInput) + ); + let writer = fs::OpenOptions::new().write(true).open(&fifo)?; + // Match the inheritable pipe descriptor passed by shell process substitution. + let inherited = rustix::io::dup(&writer)?; + let alias = format!("/dev/fd/{}", inherited.as_raw_fd()); + BoundOutput::open(alias.as_ref())?.write(b"pipe")?; + reader.read_exact(&mut buf)?; + assert_eq!(buf, *b"pipe"); + Ok(()) + } +} diff --git a/codex-rs/exec-server/src/no_follow/unix.rs b/codex-rs/exec-server/src/no_follow/unix.rs index 330a316ab0..24d5feb901 100644 --- a/codex-rs/exec-server/src/no_follow/unix.rs +++ b/codex-rs/exec-server/src/no_follow/unix.rs @@ -68,7 +68,7 @@ fn root() -> io::Result { .map_err(io::Error::from) } -fn open_directory(parent: &OwnedFd, name: &OsString) -> io::Result { +fn open_directory(parent: &OwnedFd, name: &std::ffi::OsStr) -> io::Result { openat( parent, name, @@ -90,6 +90,124 @@ fn parent(path: &Path) -> io::Result<(OwnedFd, OsString)> { Ok((directory, leaf)) } +pub(super) fn inherited_output(path: &Path) -> io::Result> { + use std::os::fd::FromRawFd; + let name = path.to_str().unwrap_or_default(); + let fd = match name { + "/dev/stdout" => libc::STDOUT_FILENO, + "/dev/stderr" => libc::STDERR_FILENO, + _ => { + let number = name + .strip_prefix("/dev/fd/") + .or_else(|| name.strip_prefix("/proc/self/fd/")); + let Some(fd) = number + .filter(|s| s.bytes().all(|b| b.is_ascii_digit())) + .and_then(|s| s.parse().ok()) + else { + return Ok(None); + }; + fd + } + }; + // Only caller-supplied descriptors may be selected, including shell process substitution. + // SAFETY: fcntl validates the integer descriptor; F_DUPFD_CLOEXEC returns a new owned fd. + let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) }; + if flags < 0 { + return Err(io::Error::last_os_error()); + } + if flags & libc::FD_CLOEXEC != 0 { + return Err(super::invalid_output("output descriptor is not inherited")); + } + let copy = unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, 3) }; + if copy < 0 { + return Err(io::Error::last_os_error()); + } + let file = unsafe { std::fs::File::from_raw_fd(copy) }; + let access = rustix::fs::fcntl_getfl(&file)? & OFlags::ACCMODE; + if !matches!(access, OFlags::WRONLY | OFlags::RDWR) { + return Err(super::invalid_output("output descriptor is not writable")); + } + Ok(Some(super::BoundOutput { + file, + destination: super::Destination::Stream { + is_stdout: fd == libc::STDOUT_FILENO, + }, + })) +} + +pub(super) fn reject_multiple_links(file: &std::fs::File) -> io::Result<()> { + use std::os::unix::fs::MetadataExt; + if file.metadata()?.nlink() > 1 { + return Err(super::invalid_output("output has multiple hard links")); + } + Ok(()) +} + +pub(super) fn output_path(path: &Path) -> io::Result { + // Resolve leading ".." against the caller's cwd, never against a movable directory fd. + let mut suffix = path.strip_prefix(".").unwrap_or(path); + let mut base = if suffix.is_absolute() { + PathBuf::new() + } else { + std::env::current_dir()? + }; + while let Ok(rest) = suffix.strip_prefix("..") { + base.pop(); + suffix = rest; + } + let path = base.join(suffix); + #[cfg(target_os = "macos")] + { + // Expand one known root link only. Never canonicalize its target or the caller's suffix. + if let Some(physical) = ["tmp", "var", "etc"].into_iter().find_map(|name| { + let root = Path::new("/").join(name); + let suffix = path.strip_prefix(&root).ok()?; + let expected = Path::new("private").join(name); + (std::fs::read_link(root).ok()? == expected) + .then(|| Path::new("/").join(expected).join(suffix)) + }) { + return Ok(physical); + } + } + Ok(path) +} + +pub(super) fn verify_output_path(file: &std::fs::File, path: &Path) -> io::Result<()> { + let (dir, leaf) = parent(path)?; + let named = statat(dir, leaf, AtFlags::SYMLINK_NOFOLLOW)?; + let bound = fstat(file)?; + if (named.st_dev, named.st_ino) != (bound.st_dev, bound.st_ino) { + return Err(super::invalid_output("output path was replaced")); + } + Ok(()) +} + +pub(super) fn open_output(path: &Path) -> io::Result { + let (directory, leaf) = parent(path)?; + let file = open_writable(&directory, &leaf)?; + if !file.metadata()?.is_file() { + let flags = rustix::fs::fcntl_getfl(&file)?; + rustix::fs::fcntl_setfl(&file, flags & !OFlags::NONBLOCK)?; + } + Ok(file) +} + +// Opening the leaf must not truncate it or hang on a FIFO without a reader. +fn open_writable(directory: &OwnedFd, leaf: &std::ffi::OsStr) -> io::Result { + let file = openat( + directory, + leaf, + OFlags::WRONLY + | OFlags::CREATE + | OFlags::NOFOLLOW + | OFlags::CLOEXEC + | OFlags::NOCTTY + | OFlags::NONBLOCK, + Mode::from_raw_mode(0o666), + )?; + Ok(std::fs::File::from(file)) +} + fn open_entry_sync(path: &Path) -> io::Result { let (parent, leaf) = parent(path)?; let file = openat( @@ -115,17 +233,8 @@ pub(super) fn open_file_sync(path: &Path) -> io::Result { pub(super) async fn write_file(path: PathBuf, contents: Vec) -> io::Result<()> { tokio::task::spawn_blocking(move || { - let (parent, leaf) = parent(&path)?; - // Prevent FIFOs and devices from blocking during open before the - // descriptor can be validated as a regular file below. - let file = openat( - &parent, - leaf, - OFlags::WRONLY | OFlags::CREATE | OFlags::NOFOLLOW | OFlags::NONBLOCK | OFlags::CLOEXEC, - Mode::from_raw_mode(0o666), - ) - .map_err(io::Error::from)?; - let mut file = std::fs::File::from(file); + let (directory, leaf) = parent(&path)?; + let mut file = open_writable(&directory, &leaf)?; if !file.metadata()?.is_file() { return Err(io::Error::new( io::ErrorKind::InvalidInput, diff --git a/codex-rs/exec-server/src/no_follow/windows.rs b/codex-rs/exec-server/src/no_follow/windows.rs index 3492979b5a..5d07b47c3e 100644 --- a/codex-rs/exec-server/src/no_follow/windows.rs +++ b/codex-rs/exec-server/src/no_follow/windows.rs @@ -23,16 +23,22 @@ use windows_sys::Win32::Foundation::RtlNtStatusToDosError; use windows_sys::Win32::Foundation::UNICODE_STRING; use windows_sys::Win32::Security::SECURITY_QUALITY_OF_SERVICE; use windows_sys::Win32::Security::SecurityIdentification; +use windows_sys::Win32::Storage::FileSystem::BY_HANDLE_FILE_INFORMATION; use windows_sys::Win32::Storage::FileSystem::DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_NORMAL; use windows_sys::Win32::Storage::FileSystem::FILE_DISPOSITION_INFO; use windows_sys::Win32::Storage::FileSystem::FILE_GENERIC_READ; +use windows_sys::Win32::Storage::FileSystem::FILE_ID_INFO; use windows_sys::Win32::Storage::FileSystem::FILE_READ_ATTRIBUTES; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_DELETE; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_READ; use windows_sys::Win32::Storage::FileSystem::FILE_SHARE_WRITE; use windows_sys::Win32::Storage::FileSystem::FILE_WRITE_DATA; use windows_sys::Win32::Storage::FileSystem::FileDispositionInfo; +use windows_sys::Win32::Storage::FileSystem::FileIdInfo; +use windows_sys::Win32::Storage::FileSystem::GetFileInformationByHandle; +use windows_sys::Win32::Storage::FileSystem::GetFileInformationByHandleEx; +use windows_sys::Win32::Storage::FileSystem::GetVolumeInformationByHandleW; use windows_sys::Win32::Storage::FileSystem::SetFileInformationByHandle; use windows_sys::Win32::System::IO::IO_STATUS_BLOCK; use windows_sys::Win32::System::IO::IO_STATUS_BLOCK_0; @@ -40,6 +46,7 @@ use windows_sys::Win32::System::IO::IO_STATUS_BLOCK_0; #[path = "windows_volume_fallback.rs"] mod volume_fallback; +const SHARE_ALL: u32 = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE; const FILE_DIRECTORY_FILE: u32 = 0x0000_0001; const FILE_SYNCHRONOUS_IO_NONALERT: u32 = 0x0000_0020; const FILE_NON_DIRECTORY_FILE: u32 = 0x0000_0040; @@ -128,6 +135,22 @@ fn open_handle( desired_access: u32, create_disposition: u32, create_options: u32, +) -> io::Result { + open_handle_with_sharing( + path, + desired_access, + create_disposition, + create_options, + SHARE_ALL, + ) +} + +fn open_handle_with_sharing( + path: &Path, + desired_access: u32, + create_disposition: u32, + create_options: u32, + share_access: u32, ) -> io::Result { let mut name = nt_path(path)?; if let Some(handle) = open_nt_handle( @@ -137,6 +160,7 @@ fn open_handle( create_disposition, create_options, OBJ_DONT_REPARSE, + share_access, )? { return Ok(handle); } @@ -151,6 +175,7 @@ fn open_handle( desired_access, create_disposition, create_options, + share_access, ); } Err(reparse_error()) @@ -168,6 +193,7 @@ fn open_nt_handle( create_disposition: u32, create_options: u32, attributes: u32, + share_access: u32, ) -> io::Result> { let maximum_length = u16::try_from(size_of_val(path)) .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "filesystem path is too long"))?; @@ -203,7 +229,7 @@ fn open_nt_handle( &mut io_status_block, ptr::null(), FILE_ATTRIBUTE_NORMAL, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + share_access, create_disposition, create_options | FILE_SYNCHRONOUS_IO_NONALERT, ptr::null(), @@ -253,16 +279,164 @@ fn validate_regular_file(file: &std::fs::File, path: &Path) -> io::Result<()> { Ok(()) } +fn file_information(file: &std::fs::File) -> io::Result { + let mut info = std::mem::MaybeUninit::uninit(); + // SAFETY: the file owns the handle and a successful call initializes info. + if unsafe { GetFileInformationByHandle(file.as_raw_handle(), info.as_mut_ptr()) } == 0 { + return Err(io::Error::last_os_error()); + } + Ok(unsafe { info.assume_init() }) +} + +pub(super) fn reject_multiple_links(file: &std::fs::File) -> io::Result<()> { + if file_information(file)?.nNumberOfLinks > 1 { + return Err(super::invalid_output("output has multiple hard links")); + } + Ok(()) +} + +fn file_identity(file: &std::fs::File) -> io::Result<(u64, [u8; 16])> { + // ReFS requires the full 128-bit identifier to distinguish files on the same volume. + let mut info = std::mem::MaybeUninit::::uninit(); + // SAFETY: the open handle is valid and FileIdInfo fills this exact structure on success. + if unsafe { + GetFileInformationByHandleEx( + file.as_raw_handle(), + FileIdInfo, + info.as_mut_ptr().cast(), + size_of::() as u32, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + let info = unsafe { info.assume_init() }; + // A filesystem may report success with an all-zero ID when it cannot identify the file. + if info.FileId.Identifier == [0; 16] { + return Err(super::invalid_output( + "filesystem did not provide an output file ID", + )); + } + Ok((info.VolumeSerialNumber, info.FileId.Identifier)) +} + +pub(super) fn verify_output_path(file: &std::fs::File, path: &Path) -> io::Result<()> { + verify_output_path_with(file, path, file_identity, filesystem_name) +} + +pub(super) fn verify_output_path_with( + file: &std::fs::File, + path: &Path, + file_id: impl Fn(&std::fs::File) -> io::Result<(u64, [u8; 16])>, + volume_name: impl Fn(&std::fs::File) -> io::Result, +) -> io::Result<()> { + let current = open_entry(path)?; + let same = match file_id(file) { + Ok(id) => id == file_id(¤t)?, + Err(error) => { + let is_fat = |name: String| { + ["FAT", "FAT32", "exFAT"] + .iter() + .any(|fs| name.eq_ignore_ascii_case(fs)) + }; + if !is_fat(volume_name(file)?) || !is_fat(volume_name(¤t)?) { + return Err(error); + } + same_fat_file(file, ¤t)? + } + }; + if !same { + return Err(super::invalid_output("output path was replaced")); + } + Ok(()) +} + +pub(super) fn filesystem_name(file: &std::fs::File) -> io::Result { + let mut name = [0u16; 32]; + // SAFETY: only the filesystem name is requested, in the supplied writable buffer. + if unsafe { + GetVolumeInformationByHandleW( + file.as_raw_handle(), + ptr::null_mut(), + /*nvolumenamesize*/ 0, + ptr::null_mut(), + ptr::null_mut(), + ptr::null_mut(), + name.as_mut_ptr(), + name.len() as u32, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + let name = name.split(|unit| *unit == 0).next().unwrap_or_default(); + Ok(String::from_utf16_lossy(name)) +} + +fn same_fat_file(file: &std::fs::File, current: &std::fs::File) -> io::Result { + // The bound output denies delete sharing, so its FAT directory slot cannot be recycled. + let id = |file| -> io::Result<_> { + let info = file_information(file)?; + if info.nFileIndexHigh == 0 && info.nFileIndexLow == 0 { + return Err(super::invalid_output( + "filesystem did not provide an output file ID", + )); + } + Ok(( + info.dwVolumeSerialNumber, + info.nFileIndexHigh, + info.nFileIndexLow, + )) + }; + Ok(id(file)? == id(current)?) +} + +pub(super) fn open_null_output() -> io::Result { + // Use the kernel null device directly so no user-controlled DOS alias is followed. + let mut name: Vec<_> = r"\Device\Null".encode_utf16().chain([0]).collect(); + let handle = open_nt_handle( + &mut name, + /*root_directory*/ None, + FILE_WRITE_DATA, + FILE_OPEN, + /*create_options*/ 0, + OBJ_DONT_REPARSE, + SHARE_ALL, + )? + .ok_or_else(reparse_error)?; + Ok(handle.into()) +} + +fn open_writable_regular_file(path: &Path, share_access: u32) -> io::Result { + let handle = open_handle_with_sharing( + path, + FILE_READ_ATTRIBUTES | FILE_WRITE_DATA, + FILE_OPEN_IF, + FILE_NON_DIRECTORY_FILE, + share_access, + )?; + let file = std::fs::File::from(handle); + validate_regular_file(&file, path)?; + Ok(file) +} + +pub(super) fn open_output(path: &Path) -> io::Result { + if matches!(path.components().next(), Some(Component::Prefix(p)) if p.kind().is_verbatim()) + && path.as_os_str().as_encoded_bytes().contains(&b'/') + { + return Err(super::invalid_output("invalid verbatim output path")); + } + // Full file IDs identify the base file, not a particular data stream. Keep the + // selected stream bound from the first open, including the unnamed default stream. + let file = open_writable_regular_file(path, FILE_SHARE_READ | FILE_SHARE_WRITE)?; + // Fail before starting a turn if this filesystem cannot verify the output. + verify_output_path(&file, path)?; + Ok(file) +} + pub(super) async fn write_file(path: PathBuf, contents: Vec) -> io::Result<()> { tokio::task::spawn_blocking(move || { - let handle = open_handle( - &path, - FILE_READ_ATTRIBUTES | FILE_WRITE_DATA, - FILE_OPEN_IF, - FILE_NON_DIRECTORY_FILE, - )?; - let mut file = std::fs::File::from(handle); - validate_regular_file(&file, &path)?; + let mut file = open_writable_regular_file(&path, SHARE_ALL)?; file.set_len(0)?; file.write_all(&contents) }) diff --git a/codex-rs/exec-server/src/no_follow/windows_tests.rs b/codex-rs/exec-server/src/no_follow/windows_tests.rs index 6274990971..c5750eb967 100644 --- a/codex-rs/exec-server/src/no_follow/windows_tests.rs +++ b/codex-rs/exec-server/src/no_follow/windows_tests.rs @@ -15,7 +15,13 @@ fn relative_volume_opens_files_and_rejects_junctions() -> io::Result<()> { let temp = tempfile::tempdir()?; let real = std::fs::canonicalize(temp.path())?; let open = |path: &Path, access, disposition, options| { - volume_fallback::open_relative_to_volume(&mut nt_path(path)?, access, disposition, options) + volume_fallback::open_relative_to_volume( + &mut nt_path(path)?, + access, + disposition, + options, + SHARE_ALL, + ) }; let existing = real.join("existing.txt"); std::fs::File::from(open( diff --git a/codex-rs/exec-server/src/no_follow/windows_volume_fallback.rs b/codex-rs/exec-server/src/no_follow/windows_volume_fallback.rs index aee889f1c3..a832253a84 100644 --- a/codex-rs/exec-server/src/no_follow/windows_volume_fallback.rs +++ b/codex-rs/exec-server/src/no_follow/windows_volume_fallback.rs @@ -16,6 +16,7 @@ pub(super) fn open_relative_to_volume( desired_access: u32, create_disposition: u32, create_options: u32, + share_access: u32, ) -> io::Result { let mut root_name = name[..DOS_DRIVE_ROOT_LEN].to_vec(); root_name.push(0); @@ -27,6 +28,7 @@ pub(super) fn open_relative_to_volume( create_disposition, create_options, OBJ_DONT_REPARSE, + share_access, )? .ok_or_else(reparse_error) } @@ -39,6 +41,7 @@ pub(super) fn open_volume_root(name: &mut [u16]) -> io::Result { FILE_OPEN, FILE_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT, /*attributes*/ 0, + SHARE_ALL, )? .ok_or_else(reparse_error)?; let root = std::fs::File::from(handle); diff --git a/codex-rs/exec-server/tests/file_system_unix.rs b/codex-rs/exec-server/tests/file_system_unix.rs index 6e45cd2dfc..8423fcff85 100644 --- a/codex-rs/exec-server/tests/file_system_unix.rs +++ b/codex-rs/exec-server/tests/file_system_unix.rs @@ -151,6 +151,7 @@ async fn file_system_operations_can_reject_symlinks_in_any_path_component( symlink(&real, &directory_link)?; let file_link = tmp_path.join("file-link"); symlink(&existing, &file_link)?; + assert!(codex_exec_server::BoundOutput::open(&directory_link.join("existing.txt")).is_err()); let no_follow_read = ReadFileOptions { follow_symlinks: false, diff --git a/codex-rs/exec-server/tests/file_system_windows.rs b/codex-rs/exec-server/tests/file_system_windows.rs index bcb4c462ec..aa043d371a 100644 --- a/codex-rs/exec-server/tests/file_system_windows.rs +++ b/codex-rs/exec-server/tests/file_system_windows.rs @@ -103,6 +103,9 @@ async fn file_system_operations_can_reject_junctions_in_any_path_component( std::fs::write(&removable, "keep")?; let directory_junction = tmp.path().join("directory-junction"); create_directory_junction(&real, &directory_junction)?; + assert!( + codex_exec_server::BoundOutput::open(&directory_junction.join("existing.txt")).is_err() + ); let no_follow_read = ReadFileOptions { follow_symlinks: false, diff --git a/codex-rs/exec/Cargo.toml b/codex-rs/exec/Cargo.toml index d5190c2caa..c4bd6e4995 100644 --- a/codex-rs/exec/Cargo.toml +++ b/codex-rs/exec/Cargo.toml @@ -30,6 +30,7 @@ codex-app-server-protocol = { workspace = true } codex-cloud-config = { workspace = true } codex-config = { workspace = true } codex-core = { workspace = true } +codex-exec-server = { workspace = true } codex-features = { workspace = true } codex-feedback = { workspace = true } codex-git-utils = { workspace = true } diff --git a/codex-rs/exec/src/event_processor.rs b/codex-rs/exec/src/event_processor.rs index 4d1da09001..a7a14bf957 100644 --- a/codex-rs/exec/src/event_processor.rs +++ b/codex-rs/exec/src/event_processor.rs @@ -1,7 +1,9 @@ +use anyhow::Context; use std::path::Path; use codex_app_server_protocol::ServerNotification; use codex_core::config::Config; +use codex_exec_server::BoundOutput; use codex_protocol::protocol::SessionConfiguredEvent; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -25,24 +27,21 @@ pub(crate) trait EventProcessor { /// Handle a local exec warning that is not represented as an app-server notification. fn process_warning(&mut self, message: String) -> CodexStatus; - fn print_final_output(&mut self) {} + fn print_final_output(&mut self, output: Option) -> anyhow::Result<()>; } -pub(crate) fn handle_last_message(last_agent_message: Option<&str>, output_file: &Path) { - let message = last_agent_message.unwrap_or_default(); - write_last_message_file(message, Some(output_file)); +pub(crate) fn handle_last_message( + last_agent_message: Option<&str>, + path: &Path, + file: BoundOutput, +) -> anyhow::Result<()> { + file.write(last_agent_message.unwrap_or_default().as_bytes()) + .with_context(|| format!("Failed to write last message file {path:?}"))?; if last_agent_message.is_none() { eprintln!( "Warning: no last agent message; wrote empty content to {}", - output_file.display() + path.display() ); } -} - -fn write_last_message_file(contents: &str, last_message_path: Option<&Path>) { - if let Some(path) = last_message_path - && let Err(e) = std::fs::write(path, contents) - { - eprintln!("Failed to write last message file {path:?}: {e}"); - } + Ok(()) } diff --git a/codex-rs/exec/src/event_processor_with_human_output.rs b/codex-rs/exec/src/event_processor_with_human_output.rs index 0a6ceeabce..1f960b26ac 100644 --- a/codex-rs/exec/src/event_processor_with_human_output.rs +++ b/codex-rs/exec/src/event_processor_with_human_output.rs @@ -9,6 +9,7 @@ use codex_app_server_protocol::ThreadItemPayload; use codex_app_server_protocol::ThreadTokenUsage; use codex_app_server_protocol::TurnStatus; use codex_core::config::Config; +use codex_exec_server::BoundOutput; use codex_model_provider_info::WireApi; use codex_protocol::num_format::format_with_separators; use codex_protocol::protocol::SessionConfiguredEvent; @@ -390,12 +391,22 @@ impl EventProcessor for EventProcessorWithHumanOutput { CodexStatus::Running } - fn print_final_output(&mut self) { - if self.emit_final_message_on_shutdown - && let Some(path) = self.last_message_path.as_deref() + fn print_final_output(&mut self, output: Option) -> anyhow::Result<()> { + let print_to_stdout = should_print_final_message_to_stdout( + self.emit_final_message_on_shutdown + .then_some(self.final_message.as_deref()) + .flatten(), + std::io::stdout().is_terminal(), + std::io::stderr().is_terminal(), + ); + let output_result = if self.emit_final_message_on_shutdown + && let (Some(path), Some(file)) = (self.last_message_path.as_deref(), output) + && !(print_to_stdout && file.is_stdout()) { - handle_last_message(self.final_message.as_deref(), path); - } + handle_last_message(self.final_message.as_deref(), path, file) + } else { + Ok(()) + }; if let Some(usage) = &self.last_total_token_usage { eprintln!( @@ -406,14 +417,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { } #[allow(clippy::print_stdout)] - if should_print_final_message_to_stdout( - self.emit_final_message_on_shutdown - .then_some(self.final_message.as_deref()) - .flatten(), - std::io::stdout().is_terminal(), - std::io::stderr().is_terminal(), - ) && let Some(message) = self.final_message.as_deref() - { + if print_to_stdout && let Some(message) = self.final_message.as_deref() { println!("{message}"); } else if should_print_final_message_to_tty( self.emit_final_message_on_shutdown @@ -430,6 +434,7 @@ impl EventProcessor for EventProcessorWithHumanOutput { message ); } + output_result } } diff --git a/codex-rs/exec/src/event_processor_with_jsonl_output.rs b/codex-rs/exec/src/event_processor_with_jsonl_output.rs index a92f762096..88369c3854 100644 --- a/codex-rs/exec/src/event_processor_with_jsonl_output.rs +++ b/codex-rs/exec/src/event_processor_with_jsonl_output.rs @@ -15,6 +15,7 @@ use codex_app_server_protocol::ThreadTokenUsage; use codex_app_server_protocol::TurnStatus; use codex_app_server_protocol::WebSearchAction as ApiWebSearchAction; use codex_core::config::Config; +use codex_exec_server::BoundOutput; use codex_protocol::models::WebSearchAction; use codex_protocol::protocol::SessionConfiguredEvent; use serde_json::json; @@ -630,12 +631,13 @@ impl EventProcessor for EventProcessorWithJsonOutput { collected.status } - fn print_final_output(&mut self) { + fn print_final_output(&mut self, output: Option) -> anyhow::Result<()> { if self.emit_final_message_on_shutdown - && let Some(path) = self.last_message_path.as_deref() + && let (Some(path), Some(file)) = (self.last_message_path.as_deref(), output) { - handle_last_message(self.final_message.as_deref(), path); + handle_last_message(self.final_message.as_deref(), path, file)?; } + Ok(()) } } diff --git a/codex-rs/exec/src/event_processor_with_jsonl_output_tests.rs b/codex-rs/exec/src/event_processor_with_jsonl_output_tests.rs index 7205c8ec47..aa6bbb378e 100644 --- a/codex-rs/exec/src/event_processor_with_jsonl_output_tests.rs +++ b/codex-rs/exec/src/event_processor_with_jsonl_output_tests.rs @@ -56,7 +56,8 @@ fn failed_turn_does_not_overwrite_output_last_message_file() { assert_eq!(status, CodexStatus::InitiateShutdown); assert_eq!(processor.final_message(), None); - EventProcessor::print_final_output(&mut processor); + let output = BoundOutput::open(&output_path).expect("bind output"); + EventProcessor::print_final_output(&mut processor, Some(output)).expect("print final output"); assert_eq!( std::fs::read_to_string(&output_path).expect("read output file"), diff --git a/codex-rs/exec/src/lib.rs b/codex-rs/exec/src/lib.rs index c1a445b01e..d841ebf65a 100644 --- a/codex-rs/exec/src/lib.rs +++ b/codex-rs/exec/src/lib.rs @@ -1025,6 +1025,16 @@ async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> { std::process::exit(1); } + // Bind before starting the agent. Sandbox activity cannot redirect the final write. + let bound_output = last_message_file + .as_deref() + .map(|path| { + codex_exec_server::BoundOutput::open(path).map_err(|error| { + anyhow::anyhow!("Failed to open last message file {path:?}: {error}") + }) + }) + .transpose()?; + let mut request_ids = RequestIdSequencer::new(); let mut client = InProcessAppServerClient::start(in_process_start_args) .await @@ -1212,7 +1222,7 @@ async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> { .shutdown() .await .map_err(|err| anyhow::anyhow!("in-process app-server shutdown failed: {err}"))?; - event_processor.print_final_output(); + event_processor.print_final_output(bound_output)?; return Ok(()); } InitialOperation::UserTurn { @@ -1412,7 +1422,7 @@ async fn run_exec_session(args: ExecRunArgs) -> anyhow::Result<()> { if let Err(err) = client.shutdown().await { warn!("in-process app-server shutdown failed: {err}"); } - event_processor.print_final_output(); + event_processor.print_final_output(bound_output)?; if error_seen { std::process::exit(1); } diff --git a/codex-rs/exec/tests/suite/mod.rs b/codex-rs/exec/tests/suite/mod.rs index baf2957b82..29ea858da5 100644 --- a/codex-rs/exec/tests/suite/mod.rs +++ b/codex-rs/exec/tests/suite/mod.rs @@ -13,6 +13,7 @@ mod ephemeral; mod hooks; mod mcp_required_exit; mod originator; +mod output_last_message; mod output_schema; mod prompt_stdin; mod resume; diff --git a/codex-rs/exec/tests/suite/output_last_message.rs b/codex-rs/exec/tests/suite/output_last_message.rs new file mode 100644 index 0000000000..419d1f56d3 --- /dev/null +++ b/codex-rs/exec/tests/suite/output_last_message.rs @@ -0,0 +1,110 @@ +#![cfg(unix)] + +//! Exercise the CLI output path used by automation with a synthetic model response. + +use core_test_support::responses; +use core_test_support::test_codex_exec::test_codex_exec; +use pretty_assertions::assert_eq; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn human_output_to_redirected_stdout_prints_one_answer() -> anyhow::Result<()> { + core_test_support::skip_if_no_network!(Ok(())); + let test = test_codex_exec(); + let server = responses::start_mock_server().await; + let command = test.cmd_with_server(&server); + let saved = test.cwd_path().join("stdout"); + for alias in ["/dev/stdout", "/dev/fd/1", "/proc/self/fd/1"] { + responses::mount_sse_once( + &server, + responses::sse(vec![ + responses::ev_response_created("r"), + responses::ev_assistant_message("m", "done"), + responses::ev_completed("r"), + ]), + ) + .await; + let output = std::process::Command::new(command.get_program()) + .args(command.get_args()) + .envs(command.get_envs().filter_map(|(k, v)| v.map(|v| (k, v)))) + .current_dir(test.cwd_path()) + .args(["--skip-git-repo-check", "-o", alias, "reply"]) + .stdout(std::fs::File::create(&saved)?) + .output()?; + assert!(output.status.success(), "{output:?}"); + assert_eq!(std::fs::read(&saved)?, b"done\n", "{alias}"); + } + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn sandbox_link_cannot_redirect_this_or_the_next_invocation() -> anyhow::Result<()> { + core_test_support::skip_if_no_network!(Ok(())); + core_test_support::skip_if_sandbox!(Ok(())); + #[cfg(target_os = "linux")] + if super::sandbox::linux_sandbox_test_env().await.is_none() { + return Ok(()); + } + for mode in [None, Some("--json")] { + let test = test_codex_exec(); + let outside = tempfile::tempdir()?; + let target = outside.path().join("victim.txt"); + let output = test.cwd_path().join("codex-output.md"); + std::fs::write(&target, "original victim")?; + std::fs::write(&output, "original output")?; + let mut original = std::fs::File::open(&output)?; + let quoted_target = format!("'{}'", target.display().to_string().replace('\'', "'\\''")); + let attack = format!( + "if (printf forbidden > {quoted_target}) 2>/dev/null; then exit 1; fi; ln -sfn -- {quoted_target} codex-output.md" + ); + let server = responses::start_mock_server().await; + let args = serde_json::json!({"cmd": attack, "shell": "/bin/sh", "login": false, "yield_time_ms": 10_000}); + responses::mount_function_call_agent_response( + &server, + "plant-output", + &args.to_string(), + "exec_command", + ) + .await; + let command = || { + let mut cmd = test.cmd_with_server(&server); + cmd.args(["--skip-git-repo-check", "--output-last-message"]) + .arg(&output); + cmd.args(mode); + cmd + }; + let mut cmd = command(); + cmd.args([ + "--sandbox", + "workspace-write", + "-c", + "approval_policy='never'", + "-c", + "sandbox_workspace_write.exclude_slash_tmp=true", + "-c", + "sandbox_workspace_write.exclude_tmpdir_env_var=true", + ]); + cmd.arg("write a response") + .assert() + .failure() + .stderr(predicates::str::contains( + "Failed to write last message file", + )); + let mut saved = String::new(); + std::io::Read::read_to_string(&mut original, &mut saved)?; + assert_eq!(saved, "original output"); + assert_eq!(std::fs::read_to_string(&target)?, "original victim"); + assert_eq!(std::fs::read_link(&output)?, target); + + // The link left by the first invocation must not gain authority on the next one. + command() + .arg("write another response") + .assert() + .failure() + .stderr(predicates::str::contains( + "Failed to open last message file", + )); + + assert_eq!(std::fs::read_to_string(&target)?, "original victim"); + } + Ok(()) +}