Isolate external editor buffers from sandbox-writable paths (#38830)

## Why

External editor buffers can contain the current composer text and should not be
placed in directories exposed as writable by a restricted filesystem policy.

## What changed

- Create editor buffer files under a protected `editor` directory, trying the
  configured Codex home, the default Codex home, and a workspace fallback.
- Reject candidates that overlap writable roots or resolve through symbolic
  links, while preserving external editor support for full-disk-write policies.
- Surface an editor error when no protected directory is available.

## Testing

Add coverage for writable roots, aliases, symbolic links, fallback selection,
temporary-directory grants, full-disk-write policies, and editor invocation.

GitOrigin-RevId: df1029d81b327678991ab84ffd1077f0e5855631
This commit is contained in:
viyatb-oai
2026-08-16 03:09:15 +00:00
committed by copyberry
parent c7a95f84b3
commit 73abda8bfe
5 changed files with 572 additions and 4 deletions

View File

@@ -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);

View File

@@ -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<std::ffi::OsString>);
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;

View File

@@ -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<Vec<String>, Edito
Ok(parts)
}
pub(super) fn editor_directory(
candidate_homes: &[&Path],
file_system_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> Result<PathBuf> {
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::<Vec<_>>()
} 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<String> {
pub(crate) async fn run_editor(
seed: &str,
editor_cmd: &[String],
codex_home: &Path,
file_system_policy: &FileSystemSandboxPolicy,
cwd: &Path,
) -> Result<String> {
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;

View File

@@ -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::<Vec<_>>();
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());
}

View File

@@ -0,0 +1,5 @@
---
source: tui/src/app/tests.rs
expression: rendered
---
■ Failed to open editor: editor directory must not be writable