Relax workspace git metadata carveouts

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<AbsolutePathBuf>, 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<FileSystemSandboxEntry>,
     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() {

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<AbsolutePathBuf>, 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"))
 }

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<PathBuf> {
+    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<PathBuf> = [dunce::canonicalize(&command_cwd).unwrap()]
             .into_iter()
             .collect();
-        let expected_deny: HashSet<PathBuf> = [dunce::canonicalize(&git_dir).unwrap()]
-            .into_iter()
-            .collect();
+        let expected_deny: HashSet<PathBuf> = [
+            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<PathBuf> = [dunce::canonicalize(&command_cwd).unwrap()]
             .into_iter()
             .collect();
-        let expected_deny: HashSet<PathBuf> = [dunce::canonicalize(&git_file).unwrap()]
-            .into_iter()
-            .collect();
+        let expected_deny: HashSet<PathBuf> = [
+            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"
+        );
     }
 }