diff --git a/codex-rs/tui/src/app/input.rs b/codex-rs/tui/src/app/input.rs index e386834d3c..526905ac1f 100644 --- a/codex-rs/tui/src/app/input.rs +++ b/codex-rs/tui/src/app/input.rs @@ -107,8 +107,19 @@ impl App { }; let seed = self.chat_widget.composer_text_with_pending(); + let config = self.chat_widget.config_ref(); + let file_system_policy = config.permissions.file_system_sandbox_policy(); let editor_result = tui - .with_restored(|| async { external_editor::run_editor(&seed, &editor_cmd).await }) + .with_restored(|| async { + external_editor::run_editor( + &seed, + &editor_cmd, + config.codex_home.as_path(), + &file_system_policy, + config.cwd.as_path(), + ) + .await + }) .await; self.reset_external_editor_state(tui); diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index 95fbd2f4b1..04322ea582 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -318,6 +318,53 @@ async fn cyber_model_auto_review_notice_snapshot() -> Result<()> { Ok(()) } +#[tokio::test] +#[serial_test::serial] +async fn external_editor_writable_directory_rejected_snapshot() -> Result<()> { + struct RestoreVisual(Option); + + impl Drop for RestoreVisual { + fn drop(&mut self) { + match self.0.take() { + Some(value) => unsafe { std::env::set_var("VISUAL", value) }, + None => unsafe { std::env::remove_var("VISUAL") }, + } + } + } + + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + let codex_home = app.chat_widget.config_ref().codex_home.clone(); + let fallback_home = dirs::home_dir() + .expect("home directory") + .join(".codex") + .abs(); + let workspace_codex_home = app.chat_widget.config_ref().cwd.join(".codex"); + let permission_profile = PermissionProfile::workspace_write_with( + &[codex_home, fallback_home, workspace_codex_home], + codex_protocol::permissions::NetworkSandboxPolicy::Restricted, + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ); + app.chat_widget + .set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy( + permission_profile, + ))?; + let mut tui = crate::tui::test_support::make_test_tui()?; + let restore_visual = RestoreVisual(std::env::var_os("VISUAL")); + unsafe { std::env::set_var("VISUAL", "editor") }; + + app.launch_external_editor(&mut tui).await; + drop(restore_visual); + + let cell = match app_event_rx.try_recv() { + Ok(AppEvent::InsertHistoryCell(cell)) => cell, + other => panic!("expected InsertHistoryCell event, got {other:?}"), + }; + let rendered = lines_to_single_string(&cell.display_lines(/*width*/ 80)); + assert_app_snapshot!("external_editor_writable_directory_rejected", rendered); + Ok(()) +} + #[tokio::test] async fn enqueue_primary_thread_session_replays_buffered_approval_after_attach() -> Result<()> { let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; diff --git a/codex-rs/tui/src/external_editor.rs b/codex-rs/tui/src/external_editor.rs index 2818503d03..8639e5d15e 100644 --- a/codex-rs/tui/src/external_editor.rs +++ b/codex-rs/tui/src/external_editor.rs @@ -1,7 +1,14 @@ use std::env; use std::fs; +use std::path::Path; +use std::path::PathBuf; use std::process::Stdio; +#[cfg(windows)] +use codex_protocol::permissions::FileSystemPath; +use codex_protocol::permissions::FileSystemSandboxPolicy; +#[cfg(windows)] +use codex_protocol::permissions::FileSystemSpecialPath; use color_eyre::eyre::Report; use color_eyre::eyre::Result; use tempfile::Builder; @@ -50,14 +57,145 @@ pub(crate) fn resolve_editor_command() -> std::result::Result, Edito Ok(parts) } +pub(super) fn editor_directory( + candidate_homes: &[&Path], + file_system_policy: &FileSystemSandboxPolicy, + cwd: &Path, +) -> Result { + let writable_roots = file_system_policy.get_writable_roots_with_cwd(cwd); + #[cfg(windows)] + let windows_temporary_roots = if !file_system_policy.has_full_disk_write_access() + && file_system_policy.entries.iter().any(|entry| { + matches!( + &entry.path, + FileSystemPath::Special { + value: FileSystemSpecialPath::Tmpdir + } + ) && entry.access.can_write() + }) { + ["TEMP", "TMP"] + .into_iter() + .filter_map(env::var_os) + .map(PathBuf::from) + .filter(|path| path.is_absolute()) + .flat_map(|path| { + let canonical_path = dunce::canonicalize(&path).ok(); + std::iter::once(path).chain(canonical_path) + }) + .collect::>() + } else { + Vec::new() + }; + let mut error = Report::msg("editor directory must not be writable"); + let mut rejected_writable = false; + + for candidate_home in candidate_homes { + let canonical_home = match dunce::canonicalize(candidate_home) { + Ok(path) => path, + Err(canonicalize_error) + if canonicalize_error.kind() == std::io::ErrorKind::NotFound => + { + let Some(parent) = candidate_home.parent() else { + error = Report::msg("editor directory has no parent"); + continue; + }; + let Some(name) = candidate_home.file_name() else { + error = Report::msg("editor directory has no parent"); + continue; + }; + match dunce::canonicalize(parent) { + Ok(parent) => parent.join(name), + Err(canonicalize_error) => { + error = canonicalize_error.into(); + continue; + } + } + } + Err(canonicalize_error) => { + error = canonicalize_error.into(); + continue; + } + }; + let editor_directory = canonical_home.join("editor"); + let logical_editor_directory = candidate_home.join("editor"); + + if !file_system_policy.has_full_disk_write_access() + && [&logical_editor_directory, &editor_directory] + .into_iter() + .any(|directory| { + let Some(parent) = directory.parent() else { + return true; + }; + let is_writable = file_system_policy.can_write_path_with_cwd(directory, cwd) + || file_system_policy.can_write_path_with_cwd(parent, cwd) + || writable_roots.iter().any(|root| { + root.is_path_writable(directory) + || root.root.as_path().starts_with(directory) + }); + #[cfg(windows)] + let is_writable = is_writable + || windows_temporary_roots.iter().any(|root| { + directory.starts_with(root) + || parent.starts_with(root) + || root.starts_with(directory) + }); + is_writable + }) + { + error = Report::msg("editor directory must not be writable"); + rejected_writable = true; + continue; + } + + if let Err(create_error) = fs::create_dir_all(&editor_directory) { + error = create_error.into(); + continue; + } + match dunce::canonicalize(&editor_directory) { + Ok(path) if path == editor_directory => return Ok(editor_directory), + Ok(_) => { + error = Report::msg("editor directory must not contain symbolic links"); + } + Err(canonicalize_error) => { + error = canonicalize_error.into(); + } + } + } + + if rejected_writable { + Err(Report::msg("editor directory must not be writable")) + } else { + Err(error) + } +} + /// Write `seed` to a temp file, launch the editor command, and return the updated content. -pub(crate) async fn run_editor(seed: &str, editor_cmd: &[String]) -> Result { +pub(crate) async fn run_editor( + seed: &str, + editor_cmd: &[String], + codex_home: &Path, + file_system_policy: &FileSystemSandboxPolicy, + cwd: &Path, +) -> Result { if editor_cmd.is_empty() { return Err(Report::msg("editor command is empty")); } + let default_codex_home = dirs::home_dir().map(|home| home.join(".codex")); + #[cfg(any(target_os = "macos", target_os = "linux"))] + let project_codex_home = cwd.join(".codex"); + let mut candidate_homes = vec![codex_home]; + if let Some(default_codex_home) = default_codex_home.as_deref() { + candidate_homes.push(default_codex_home); + } + #[cfg(any(target_os = "macos", target_os = "linux"))] + candidate_homes.push(&project_codex_home); + let editor_directory = editor_directory(&candidate_homes, file_system_policy, cwd)?; // Convert to TempPath immediately so no file handle stays open on Windows. - let temp_path = Builder::new().suffix(".md").tempfile()?.into_temp_path(); + let temp_path = Builder::new() + .suffix(".md") + .tempfile_in(editor_directory)? + .into_temp_path(); fs::write(&temp_path, seed)?; let mut cmd = { @@ -165,7 +303,14 @@ mod tests { fs::set_permissions(&script_path, perms).unwrap(); let cmd = vec![script_path.to_string_lossy().to_string()]; - let result = run_editor("seed", &cmd).await.unwrap(); + let policy = FileSystemSandboxPolicy::read_only(); + let result = run_editor("seed", &cmd, dir.path(), &policy, dir.path()) + .await + .unwrap(); assert_eq!(result, "edited".to_string()); } } + +#[cfg(test)] +#[path = "external_editor_tests.rs"] +mod buffer_tests; diff --git a/codex-rs/tui/src/external_editor_tests.rs b/codex-rs/tui/src/external_editor_tests.rs new file mode 100644 index 0000000000..575221dcf2 --- /dev/null +++ b/codex-rs/tui/src/external_editor_tests.rs @@ -0,0 +1,360 @@ +use super::editor_directory; +#[cfg(unix)] +use super::run_editor; +use codex_protocol::permissions::FileSystemAccessMode; +use codex_protocol::permissions::FileSystemSandboxEntry; +use codex_protocol::permissions::FileSystemSandboxPolicy; +use codex_utils_absolute_path::AbsolutePathBuf; +use pretty_assertions::assert_eq; +use std::fs; +use std::path::Path; +use std::path::PathBuf; +use tempfile::TempDir; + +struct EditorPaths { + _root: TempDir, + codex_home: PathBuf, + cwd: PathBuf, +} + +impl EditorPaths { + fn new() -> Self { + let root = tempfile::tempdir().expect("create editor test root"); + let canonical_root = + dunce::canonicalize(root.path()).expect("canonicalize editor test root"); + let codex_home = canonical_root.join("codex-home"); + let cwd = canonical_root.join("workspace"); + fs::create_dir(&codex_home).expect("create Codex home"); + fs::create_dir(&cwd).expect("create workspace"); + + Self { + _root: root, + codex_home, + cwd, + } + } +} + +fn workspace_write_policy(writable_roots: &[&Path]) -> FileSystemSandboxPolicy { + let writable_roots = writable_roots + .iter() + .map(|root| AbsolutePathBuf::from_absolute_path(root).expect("absolute writable root")) + .collect::>(); + + FileSystemSandboxPolicy::workspace_write( + &writable_roots, + /*exclude_tmpdir_env_var*/ true, + /*exclude_slash_tmp*/ true, + ) +} + +#[test] +fn editor_directory_is_inside_isolated_codex_home() { + let paths = EditorPaths::new(); + let policy = workspace_write_policy(&[]); + + let directory = editor_directory(&[&paths.codex_home], &policy, &paths.cwd) + .expect("create isolated editor directory"); + + assert_eq!(directory, paths.codex_home.join("editor")); + assert!(directory.is_dir()); +} + +#[test] +fn editor_directory_rejects_writable_home_editor_and_parent() { + let paths = EditorPaths::new(); + let editor = paths.codex_home.join("editor"); + fs::create_dir(&editor).expect("create editor directory"); + let parent = paths.codex_home.parent().expect("Codex home parent"); + + for writable_root in [paths.codex_home.as_path(), editor.as_path(), parent] { + let policy = workspace_write_policy(&[writable_root]); + + assert!( + editor_directory(&[&paths.codex_home], &policy, &paths.cwd).is_err(), + "writable root {} must not expose editor buffers", + writable_root.display() + ); + } +} + +#[test] +fn editor_directory_rejects_writable_descendant() { + let paths = EditorPaths::new(); + let writable_descendant = paths.codex_home.join("editor").join("nested"); + fs::create_dir_all(&writable_descendant).expect("create writable editor descendant"); + let policy = workspace_write_policy(&[&writable_descendant]); + + assert!(editor_directory(&[&paths.codex_home], &policy, &paths.cwd).is_err()); +} + +#[test] +fn editor_directory_rejects_read_only_carveout_with_writable_parent() { + let paths = EditorPaths::new(); + let editor = paths.codex_home.join("editor"); + fs::create_dir(&editor).expect("create editor directory"); + let policy = FileSystemSandboxPolicy::restricted(vec![ + FileSystemSandboxEntry::new( + AbsolutePathBuf::from_absolute_path(&paths.codex_home) + .expect("absolute Codex home") + .into(), + FileSystemAccessMode::Write, + ), + FileSystemSandboxEntry::new( + AbsolutePathBuf::from_absolute_path(&editor) + .expect("absolute editor directory") + .into(), + FileSystemAccessMode::Read, + ), + ]); + + assert!(editor_directory(&[&paths.codex_home], &policy, &paths.cwd).is_err()); +} + +#[test] +#[cfg(unix)] +fn editor_directory_rejects_preexisting_symlink() { + use std::os::unix::fs::symlink; + + let paths = EditorPaths::new(); + let outside = paths.cwd.join("outside"); + fs::create_dir(&outside).expect("create editor symlink target"); + symlink(&outside, paths.codex_home.join("editor")).expect("create editor directory symlink"); + let policy = FileSystemSandboxPolicy::read_only(); + + assert!(editor_directory(&[&paths.codex_home], &policy, &paths.cwd).is_err()); +} + +#[test] +#[cfg(unix)] +fn editor_directory_rejects_writable_codex_home_alias() { + use std::os::unix::fs::symlink; + + let paths = EditorPaths::new(); + let aliased_home = paths.cwd.join("codex-home-link"); + symlink(&paths.codex_home, &aliased_home).expect("create Codex home symlink"); + let policy = workspace_write_policy(&[]); + + assert!(policy.can_write_path_with_cwd(&aliased_home.join("editor"), &paths.cwd)); + assert!(!policy.can_write_path_with_cwd(&paths.codex_home.join("editor"), &paths.cwd)); + assert!(editor_directory(&[&aliased_home], &policy, &paths.cwd).is_err()); +} + +#[test] +#[cfg(unix)] +fn editor_directory_rejects_writable_codex_home_alias_target() { + use std::os::unix::fs::symlink; + + let paths = EditorPaths::new(); + let alias_parent = paths + .codex_home + .parent() + .expect("Codex home parent") + .join("aliases"); + fs::create_dir(&alias_parent).expect("create protected alias parent"); + let aliased_home = alias_parent.join("codex-home-link"); + symlink(&paths.codex_home, &aliased_home).expect("create Codex home symlink"); + let policy = workspace_write_policy(&[&paths.codex_home]); + + assert!(!policy.can_write_path_with_cwd(&aliased_home.join("editor"), &paths.cwd)); + assert!(policy.can_write_path_with_cwd(&paths.codex_home.join("editor"), &paths.cwd)); + assert!(editor_directory(&[&aliased_home], &policy, &paths.cwd).is_err()); +} + +#[test] +#[cfg(unix)] +fn editor_directory_uses_protected_workspace_fallback_with_default_temporary_grants() { + let root = tempfile::tempdir().expect("create editor test root"); + let codex_home = root.path().join("codex-home"); + let cwd = root.path().join("workspace"); + fs::create_dir(&codex_home).expect("create Codex home"); + fs::create_dir(&cwd).expect("create workspace"); + let workspace_codex_home = cwd.join(".codex"); + let policy = FileSystemSandboxPolicy::workspace_write( + &[], + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + + assert!(!workspace_codex_home.exists()); + assert!(policy.can_write_path_with_cwd(&codex_home, &cwd)); + assert!(!policy.can_write_path_with_cwd(&workspace_codex_home, &cwd)); + assert!(!policy.can_write_path_with_cwd(&workspace_codex_home.join("editor"), &cwd)); + + let directory = editor_directory(&[&codex_home, &workspace_codex_home], &policy, &cwd) + .expect("use protected workspace metadata directory"); + + assert_eq!( + directory, + dunce::canonicalize(&workspace_codex_home) + .expect("canonicalize workspace metadata directory") + .join("editor") + ); + assert!(directory.is_dir()); +} + +#[test] +#[cfg(unix)] +fn editor_directory_rejects_explicitly_writable_workspace_fallback() { + let root = tempfile::tempdir().expect("create editor test root"); + let codex_home = root.path().join("codex-home"); + let cwd = root.path().join("workspace"); + fs::create_dir(&codex_home).expect("create Codex home"); + fs::create_dir(&cwd).expect("create workspace"); + let workspace_codex_home = cwd.join(".codex"); + let writable_workspace_codex_home = AbsolutePathBuf::from_absolute_path(&workspace_codex_home) + .expect("absolute workspace metadata directory"); + let policy = FileSystemSandboxPolicy::workspace_write( + &[writable_workspace_codex_home], + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + + assert!(policy.can_write_path_with_cwd(&codex_home, &cwd)); + assert!(policy.can_write_path_with_cwd(&workspace_codex_home, &cwd)); + assert!( + editor_directory(&[&codex_home, &workspace_codex_home], &policy, &cwd).is_err(), + "explicitly writable metadata must not be used for editor buffers" + ); + assert!(!workspace_codex_home.exists()); +} + +#[test] +#[cfg(unix)] +fn editor_directory_rejects_workspace_fallback_symlink_to_writable_target() { + use std::os::unix::fs::symlink; + + let paths = EditorPaths::new(); + let workspace_codex_home = paths.cwd.join(".codex"); + symlink(&paths.codex_home, &workspace_codex_home).expect("create workspace metadata symlink"); + let policy = workspace_write_policy(&[&paths.codex_home]); + + assert!(!policy.can_write_path_with_cwd(&workspace_codex_home, &paths.cwd)); + assert!(policy.can_write_path_with_cwd(&paths.codex_home, &paths.cwd)); + assert!( + editor_directory( + &[&paths.codex_home, &workspace_codex_home], + &policy, + &paths.cwd, + ) + .is_err() + ); +} + +#[test] +fn editor_directory_uses_next_protected_candidate_after_creation_error() { + let paths = EditorPaths::new(); + let unavailable_home = paths + .codex_home + .parent() + .expect("Codex home parent") + .join("unavailable-home"); + fs::write(&unavailable_home, "not a directory").expect("create unavailable Codex home"); + let policy = workspace_write_policy(&[]); + + let directory = editor_directory(&[&unavailable_home, &paths.codex_home], &policy, &paths.cwd) + .expect("use next protected candidate after directory creation fails"); + + assert_eq!(directory, paths.codex_home.join("editor")); +} + +#[test] +#[cfg(windows)] +fn editor_directory_rejects_windows_temporary_directory_outside_tmpdir_policy_root() { + let paths = EditorPaths::new(); + let policy = FileSystemSandboxPolicy::workspace_write( + &[], + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ true, + ); + + assert!( + editor_directory(&[&paths.codex_home], &policy, &paths.cwd).is_err(), + "effective Windows temporary directories must not contain editor buffers" + ); + assert!(!paths.codex_home.join("editor").exists()); +} + +#[test] +fn editor_directory_allows_full_disk_write_policies() { + let paths = EditorPaths::new(); + + for policy in [ + FileSystemSandboxPolicy::unrestricted(), + FileSystemSandboxPolicy::external_sandbox(), + ] { + let directory = editor_directory(&[&paths.codex_home], &policy, &paths.cwd) + .expect("full-disk-write policies should preserve external editor support"); + + assert_eq!(directory, paths.codex_home.join("editor")); + } +} + +#[tokio::test] +#[cfg(unix)] +async fn editor_process_receives_buffer_in_isolated_codex_home() { + let paths = EditorPaths::new(); + let policy = workspace_write_policy(&[]); + let editor_directory = paths.codex_home.join("editor"); + let editor_command = vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "case \"$2\" in \"$1\"/*) printf edited > \"$2\" ;; *) exit 88 ;; esac".to_string(), + "editor".to_string(), + editor_directory.to_string_lossy().into_owned(), + ]; + + let content = run_editor( + "seed", + &editor_command, + &paths.codex_home, + &policy, + &paths.cwd, + ) + .await + .expect("run editor with isolated buffer"); + + assert_eq!(content, "edited"); +} + +#[tokio::test] +#[cfg(any(target_os = "macos", target_os = "linux"))] +async fn editor_process_uses_protected_workspace_fallback_with_default_temporary_grants() { + let root = tempfile::tempdir().expect("create editor test root"); + let codex_home = root.path().join("codex-home"); + let cwd = root.path().join("workspace"); + fs::create_dir(&codex_home).expect("create Codex home"); + fs::create_dir(&cwd).expect("create workspace"); + let default_codex_home = dirs::home_dir().expect("home directory").join(".codex"); + let writable_default_codex_home = AbsolutePathBuf::from_absolute_path(&default_codex_home) + .expect("absolute default Codex home"); + let policy = FileSystemSandboxPolicy::workspace_write( + &[writable_default_codex_home], + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + let workspace_codex_home = cwd.join(".codex"); + let editor_directory = dunce::canonicalize(&cwd) + .expect("canonicalize workspace") + .join(".codex") + .join("editor"); + let editor_command = vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "case \"$2\" in \"$1\"/*) printf edited > \"$2\" ;; *) exit 88 ;; esac".to_string(), + "editor".to_string(), + editor_directory.to_string_lossy().into_owned(), + ]; + + assert!(!workspace_codex_home.exists()); + assert!(policy.can_write_path_with_cwd(&codex_home, &cwd)); + assert!(policy.can_write_path_with_cwd(&default_codex_home, &cwd)); + assert!(!policy.can_write_path_with_cwd(&workspace_codex_home, &cwd)); + + let content = run_editor("seed", &editor_command, &codex_home, &policy, &cwd) + .await + .expect("run editor with protected workspace fallback"); + + assert_eq!(content, "edited"); + assert!(editor_directory.is_dir()); +} diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__external_editor_writable_directory_rejected.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__external_editor_writable_directory_rejected.snap new file mode 100644 index 0000000000..ac46c12a6c --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__external_editor_writable_directory_rejected.snap @@ -0,0 +1,5 @@ +--- +source: tui/src/app/tests.rs +expression: rendered +--- +■ Failed to open editor: editor directory must not be writable