diff --git a/codex-rs/protocol/src/permissions.rs b/codex-rs/protocol/src/permissions.rs index ed56567454..5afde44932 100644 --- a/codex-rs/protocol/src/permissions.rs +++ b/codex-rs/protocol/src/permissions.rs @@ -1317,13 +1317,18 @@ fn default_read_only_subpaths_for_writable_root( let top_level_git_is_file = top_level_git.as_path().is_file(); let top_level_git_is_dir = top_level_git.as_path().is_dir(); if top_level_git_is_dir || top_level_git_is_file { - if top_level_git_is_file - && is_git_pointer_file(&top_level_git) - && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git) - { - subpaths.push(gitdir); + if top_level_git_is_file { + // Worktree/submodule pointer files remain read-only because rewriting + // them can redirect the sandbox at attacker-controlled git metadata. + subpaths.push(top_level_git.clone()); + if is_git_pointer_file(&top_level_git) + && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git) + { + extend_gitdir_protected_subpaths(&mut subpaths, &gitdir); + } + } else { + extend_gitdir_protected_subpaths(&mut subpaths, &top_level_git); } - subpaths.push(top_level_git); } let top_level_agents = writable_root.join(".agents"); @@ -1343,6 +1348,18 @@ fn default_read_only_subpaths_for_writable_root( dedup_absolute_paths(subpaths, /*normalize_effective_paths*/ false) } +fn extend_gitdir_protected_subpaths(subpaths: &mut Vec, gitdir: &AbsolutePathBuf) { + let config = gitdir.join("config"); + if config.as_path().exists() { + subpaths.push(config); + } + + let hooks = gitdir.join("hooks"); + if hooks.as_path().exists() { + subpaths.push(hooks); + } +} + fn append_path_entry_if_missing( entries: &mut Vec, path: AbsolutePathBuf, @@ -1791,6 +1808,101 @@ mod tests { ); } + #[cfg(unix)] + #[test] + fn writable_roots_only_protect_git_config_and_hooks_for_git_dirs() { + let cwd = TempDir::new().expect("tempdir"); + let root = cwd.path().join("root"); + let git_dir = root.join(".git"); + let git_config = git_dir.join("config"); + let git_hooks = git_dir.join("hooks"); + fs::create_dir_all(&git_hooks).expect("create hooks"); + fs::write(&git_config, "[core]\n").expect("write config"); + + let root = AbsolutePathBuf::from_absolute_path(&root).expect("absolute root"); + let expected_git_config = + AbsolutePathBuf::from_absolute_path(&git_config).expect("absolute git config"); + let expected_git_hooks = + AbsolutePathBuf::from_absolute_path(&git_hooks).expect("absolute git hooks"); + let unexpected_git_dir = + AbsolutePathBuf::from_absolute_path(&git_dir).expect("absolute git dir"); + + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { path: root.clone() }, + access: FileSystemAccessMode::Write, + }]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_git_config) + ); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_git_hooks) + ); + assert!( + !writable_roots[0] + .read_only_subpaths + .contains(&unexpected_git_dir) + ); + } + + #[cfg(unix)] + #[test] + fn writable_roots_keep_git_pointer_file_read_only_but_relax_gitdir_metadata() { + let cwd = TempDir::new().expect("tempdir"); + let root = cwd.path().join("root"); + let gitdir = root.join("actual-gitdir"); + let gitdir_config = gitdir.join("config"); + let gitdir_hooks = gitdir.join("hooks"); + let dot_git = root.join(".git"); + fs::create_dir_all(&gitdir_hooks).expect("create gitdir hooks"); + fs::write(&gitdir_config, "[core]\n").expect("write gitdir config"); + fs::write(&dot_git, format!("gitdir: {}\n", gitdir.display())).expect("write .git"); + + let root = AbsolutePathBuf::from_absolute_path(&root).expect("absolute root"); + let expected_dot_git = + AbsolutePathBuf::from_absolute_path(&dot_git).expect("absolute .git"); + let expected_gitdir_config = + AbsolutePathBuf::from_absolute_path(&gitdir_config).expect("absolute gitdir config"); + let expected_gitdir_hooks = + AbsolutePathBuf::from_absolute_path(&gitdir_hooks).expect("absolute gitdir hooks"); + let unexpected_gitdir = + AbsolutePathBuf::from_absolute_path(&gitdir).expect("absolute gitdir"); + + let policy = FileSystemSandboxPolicy::restricted(vec![FileSystemSandboxEntry { + path: FileSystemPath::Path { path: root.clone() }, + access: FileSystemAccessMode::Write, + }]); + + let writable_roots = policy.get_writable_roots_with_cwd(cwd.path()); + assert_eq!(writable_roots.len(), 1); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_dot_git) + ); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_gitdir_config) + ); + assert!( + writable_roots[0] + .read_only_subpaths + .contains(&expected_gitdir_hooks) + ); + assert!( + !writable_roots[0] + .read_only_subpaths + .contains(&unexpected_gitdir) + ); + } + #[cfg(unix)] #[test] fn writable_roots_preserve_explicit_symlinked_carveouts_under_symlinked_roots() { diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 6e0a62fc41..5937a0df7a 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -1415,13 +1415,18 @@ fn default_read_only_subpaths_for_writable_root( let top_level_git_is_file = top_level_git.as_path().is_file(); let top_level_git_is_dir = top_level_git.as_path().is_dir(); if top_level_git_is_dir || top_level_git_is_file { - if top_level_git_is_file - && is_git_pointer_file(&top_level_git) - && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git) - { - subpaths.push(gitdir); + if top_level_git_is_file { + // Worktree/submodule pointer files remain read-only because rewriting + // them can redirect the sandbox at attacker-controlled git metadata. + subpaths.push(top_level_git.clone()); + if is_git_pointer_file(&top_level_git) + && let Some(gitdir) = resolve_gitdir_from_file(&top_level_git) + { + extend_gitdir_protected_subpaths(&mut subpaths, &gitdir); + } + } else { + extend_gitdir_protected_subpaths(&mut subpaths, &top_level_git); } - subpaths.push(top_level_git); } let top_level_agents = writable_root.join(".agents"); @@ -1448,6 +1453,18 @@ fn default_read_only_subpaths_for_writable_root( deduped } +fn extend_gitdir_protected_subpaths(subpaths: &mut Vec, gitdir: &AbsolutePathBuf) { + let config = gitdir.join("config"); + if config.as_path().exists() { + subpaths.push(config); + } + + let hooks = gitdir.join("hooks"); + if hooks.as_path().exists() { + subpaths.push(hooks); + } +} + fn is_git_pointer_file(path: &AbsolutePathBuf) -> bool { path.as_path().is_file() && path.as_path().file_name() == Some(OsStr::new(".git")) } diff --git a/codex-rs/windows-sandbox-rs/src/allow.rs b/codex-rs/windows-sandbox-rs/src/allow.rs index b40532cda8..46aeb812e5 100644 --- a/codex-rs/windows-sandbox-rs/src/allow.rs +++ b/codex-rs/windows-sandbox-rs/src/allow.rs @@ -2,6 +2,7 @@ use crate::policy::SandboxPolicy; use dunce::canonicalize; use std::collections::HashMap; use std::collections::HashSet; +use std::ffi::OsStr; use std::path::Path; use std::path::PathBuf; @@ -52,7 +53,19 @@ pub fn compute_allow_paths( let canonical = canonicalize(&candidate).unwrap_or(candidate); add_allow(canonical.clone()); - for protected_subdir in [".git", ".codex", ".agents"] { + let git_entry = canonical.join(".git"); + if git_entry.is_file() { + add_deny(git_entry.clone()); + if is_git_pointer_file(&git_entry) + && let Some(gitdir) = resolve_gitdir_from_file(&git_entry) + { + add_existing_gitdir_protected_paths(&gitdir, add_deny); + } + } else if git_entry.is_dir() { + add_existing_gitdir_protected_paths(&git_entry, add_deny); + } + + for protected_subdir in [".codex", ".agents"] { let protected_entry = canonical.join(protected_subdir); if protected_entry.exists() { add_deny(protected_entry); @@ -92,6 +105,36 @@ pub fn compute_allow_paths( AllowDenyPaths { allow, deny } } +fn add_existing_gitdir_protected_paths(gitdir: &Path, add_deny: &mut dyn FnMut(PathBuf)) { + for protected_name in ["config", "hooks"] { + let protected_entry = gitdir.join(protected_name); + if protected_entry.exists() { + add_deny(canonicalize(&protected_entry).unwrap_or(protected_entry)); + } + } +} + +fn is_git_pointer_file(path: &Path) -> bool { + path.is_file() && path.file_name() == Some(OsStr::new(".git")) +} + +fn resolve_gitdir_from_file(dot_git: &Path) -> Option { + let contents = std::fs::read_to_string(dot_git).ok()?; + let (_, gitdir_raw) = contents.trim().split_once(':')?; + let gitdir_raw = gitdir_raw.trim(); + if gitdir_raw.is_empty() { + return None; + } + + let path = Path::new(gitdir_raw); + let resolved = if path.is_absolute() { + path.to_path_buf() + } else { + dot_git.parent()?.join(path) + }; + Some(canonicalize(&resolved).unwrap_or(resolved)) +} + #[cfg(test)] mod tests { use super::*; @@ -118,12 +161,16 @@ mod tests { let paths = compute_allow_paths(&policy, &command_cwd, &command_cwd, &HashMap::new()); - assert!(paths - .allow - .contains(&dunce::canonicalize(&command_cwd).unwrap())); - assert!(paths - .allow - .contains(&dunce::canonicalize(&extra_root).unwrap())); + assert!( + paths + .allow + .contains(&dunce::canonicalize(&command_cwd).unwrap()) + ); + assert!( + paths + .allow + .contains(&dunce::canonicalize(&extra_root).unwrap()) + ); assert!(paths.deny.is_empty(), "no deny paths expected"); } @@ -147,21 +194,28 @@ mod tests { let paths = compute_allow_paths(&policy, &command_cwd, &command_cwd, &env_map); - assert!(paths - .allow - .contains(&dunce::canonicalize(&command_cwd).unwrap())); - assert!(!paths - .allow - .contains(&dunce::canonicalize(&temp_dir).unwrap())); + assert!( + paths + .allow + .contains(&dunce::canonicalize(&command_cwd).unwrap()) + ); + assert!( + !paths + .allow + .contains(&dunce::canonicalize(&temp_dir).unwrap()) + ); assert!(paths.deny.is_empty(), "no deny paths expected"); } #[test] - fn denies_git_dir_inside_writable_root() { + fn denies_git_config_and_hooks_inside_writable_root() { let tmp = TempDir::new().expect("tempdir"); let command_cwd = tmp.path().join("workspace"); let git_dir = command_cwd.join(".git"); - let _ = fs::create_dir_all(&git_dir); + let git_config = git_dir.join("config"); + let git_hooks = git_dir.join("hooks"); + let _ = fs::create_dir_all(&git_hooks); + let _ = fs::write(&git_config, "[core]\n"); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], @@ -175,9 +229,12 @@ mod tests { let expected_allow: HashSet = [dunce::canonicalize(&command_cwd).unwrap()] .into_iter() .collect(); - let expected_deny: HashSet = [dunce::canonicalize(&git_dir).unwrap()] - .into_iter() - .collect(); + let expected_deny: HashSet = [ + dunce::canonicalize(&git_config).unwrap(), + dunce::canonicalize(&git_hooks).unwrap(), + ] + .into_iter() + .collect(); assert_eq!(expected_allow, paths.allow); assert_eq!(expected_deny, paths.deny); @@ -188,8 +245,13 @@ mod tests { let tmp = TempDir::new().expect("tempdir"); let command_cwd = tmp.path().join("workspace"); let git_file = command_cwd.join(".git"); + let gitdir = command_cwd.join("actual-gitdir"); + let gitdir_config = gitdir.join("config"); + let gitdir_hooks = gitdir.join("hooks"); + let _ = fs::create_dir_all(&gitdir_hooks); + let _ = fs::write(&gitdir_config, "[core]\n"); let _ = fs::create_dir_all(&command_cwd); - let _ = fs::write(&git_file, "gitdir: .git/worktrees/example"); + let _ = fs::write(&git_file, format!("gitdir: {}\n", gitdir.display())); let policy = SandboxPolicy::WorkspaceWrite { writable_roots: vec![], @@ -203,9 +265,13 @@ mod tests { let expected_allow: HashSet = [dunce::canonicalize(&command_cwd).unwrap()] .into_iter() .collect(); - let expected_deny: HashSet = [dunce::canonicalize(&git_file).unwrap()] - .into_iter() - .collect(); + let expected_deny: HashSet = [ + dunce::canonicalize(&git_file).unwrap(), + dunce::canonicalize(&gitdir_config).unwrap(), + dunce::canonicalize(&gitdir_hooks).unwrap(), + ] + .into_iter() + .collect(); assert_eq!(expected_allow, paths.allow); assert_eq!(expected_deny, paths.deny); @@ -259,6 +325,9 @@ mod tests { let paths = compute_allow_paths(&policy, &command_cwd, &command_cwd, &HashMap::new()); assert_eq!(paths.allow.len(), 1); - assert!(paths.deny.is_empty(), "no deny when protected dirs are absent"); + assert!( + paths.deny.is_empty(), + "no deny when protected dirs are absent" + ); } }