-
Notifications
You must be signed in to change notification settings - Fork 3.3k
fix(tui): allow worktree git metadata writes in sandbox #3356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
+93
−0
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| //! tightly controlled workspace-only write access. | ||
|
|
||
| use serde::{Deserialize, Serialize}; | ||
| use std::fs; | ||
| use std::io; | ||
| use std::path::{Path, PathBuf}; | ||
|
|
||
|
|
@@ -171,6 +172,14 @@ impl SandboxPolicy { | |
| roots.push(cwd.to_path_buf()); | ||
| } | ||
|
|
||
| // Git worktrees keep mutable metadata outside the worktree | ||
| // directory. Allow only the gitdir and commondir derived from | ||
| // a workspace `.git` pointer, preserving the workspace boundary | ||
| // for all other external paths. | ||
| for root in roots.clone() { | ||
| roots.extend(resolve_git_worktree_writable_roots(&root)); | ||
| } | ||
|
|
||
| // Add /tmp unless excluded | ||
| if !exclude_slash_tmp && let Ok(tmp) = Path::new("/tmp").canonicalize() { | ||
| roots.push(tmp); | ||
|
|
@@ -211,6 +220,57 @@ impl SandboxPolicy { | |
| } | ||
| } | ||
|
|
||
| fn resolve_git_worktree_writable_roots(root: &Path) -> Vec<PathBuf> { | ||
| let Some(git_dir) = resolve_gitdir_pointer(root) else { | ||
| return Vec::new(); | ||
| }; | ||
| let Some(common_dir) = resolve_git_common_dir(&git_dir) else { | ||
| return Vec::new(); | ||
| }; | ||
| if !git_dir.starts_with(common_dir.join("worktrees")) { | ||
| return Vec::new(); | ||
| } | ||
|
|
||
| vec![git_dir, common_dir] | ||
| } | ||
|
|
||
| fn resolve_gitdir_pointer(root: &Path) -> Option<PathBuf> { | ||
| let contents = fs::read_to_string(root.join(".git")).ok()?; | ||
| let value = contents | ||
| .lines() | ||
| .find_map(|line| line.strip_prefix("gitdir:"))? | ||
| .trim(); | ||
| if value.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let path = PathBuf::from(value); | ||
| let resolved = if path.is_absolute() { | ||
| path | ||
| } else { | ||
| root.join(path) | ||
| }; | ||
|
|
||
| resolved.canonicalize().ok() | ||
| } | ||
|
|
||
| fn resolve_git_common_dir(git_dir: &Path) -> Option<PathBuf> { | ||
| let contents = fs::read_to_string(git_dir.join("commondir")).ok()?; | ||
| let value = contents.lines().next()?.trim(); | ||
| if value.is_empty() { | ||
| return None; | ||
| } | ||
|
|
||
| let path = PathBuf::from(value); | ||
| let resolved = if path.is_absolute() { | ||
| path | ||
| } else { | ||
| git_dir.join(path) | ||
| }; | ||
|
|
||
| resolved.canonicalize().ok() | ||
| } | ||
|
|
||
| /// A directory tree where writes are allowed, with optional read-only subpaths. | ||
| /// | ||
| /// This allows fine-grained control like "allow writes to /project but not /project/.deepseek". | ||
|
|
@@ -346,6 +406,39 @@ mod tests { | |
| assert!(policy.should_sandbox()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn workspace_write_includes_git_worktree_metadata_roots() { | ||
| let tmp = tempfile::tempdir().expect("tempdir"); | ||
| let common_git_dir = tmp.path().join("main-repo").join(".git"); | ||
| let worktree_git_dir = common_git_dir.join("worktrees").join("feature"); | ||
| let worktree = tmp.path().join("feature-worktree"); | ||
| std::fs::create_dir_all(&worktree_git_dir).expect("mkdir gitdir"); | ||
| std::fs::create_dir_all(&worktree).expect("mkdir worktree"); | ||
| std::fs::write( | ||
| worktree.join(".git"), | ||
| format!("gitdir: {}\n", worktree_git_dir.display()), | ||
| ) | ||
| .expect("write git pointer"); | ||
| std::fs::write(worktree_git_dir.join("commondir"), "../..").expect("write commondir"); | ||
|
|
||
| let policy = SandboxPolicy::WorkspaceWrite { | ||
| writable_roots: vec![worktree.clone()], | ||
| network_access: true, | ||
| exclude_tmpdir: true, | ||
| exclude_slash_tmp: true, | ||
| }; | ||
|
|
||
| let root_paths: Vec<PathBuf> = policy | ||
| .get_writable_roots(&worktree) | ||
| .into_iter() | ||
| .map(|root| root.root) | ||
| .collect(); | ||
|
|
||
| assert!(root_paths.contains(&worktree.canonicalize().expect("canonical worktree"))); | ||
| assert!(root_paths.contains(&worktree_git_dir.canonicalize().expect("canonical gitdir"))); | ||
| assert!(root_paths.contains(&common_git_dir.canonicalize().expect("canonical common git"))); | ||
| } | ||
|
Comment on lines
+410
to
+440
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's update the test to also verify that worktree metadata roots are correctly resolved when executing from a subdirectory of the worktree. #[test]
fn workspace_write_includes_git_worktree_metadata_roots() {
let tmp = tempfile::tempdir().expect("tempdir");
let common_git_dir = tmp.path().join("main-repo").join(".git");
let worktree_git_dir = common_git_dir.join("worktrees").join("feature");
let worktree = tmp.path().join("feature-worktree");
std::fs::create_dir_all(&worktree_git_dir).expect("mkdir gitdir");
std::fs::create_dir_all(&worktree).expect("mkdir worktree");
std::fs::write(
worktree.join(".git"),
format!("gitdir: {}\n", worktree_git_dir.display()),
)
.expect("write git pointer");
std::fs::write(worktree_git_dir.join("commondir"), "../..").expect("write commondir");
let policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![worktree.clone()],
network_access: true,
exclude_tmpdir: true,
exclude_slash_tmp: true,
};
// Test resolving from the worktree root
let root_paths: Vec<PathBuf> = policy
.get_writable_roots(&worktree)
.into_iter()
.map(|root| root.root)
.collect();
assert!(root_paths.contains(&worktree.canonicalize().expect("canonical worktree")));
assert!(root_paths.contains(&worktree_git_dir.canonicalize().expect("canonical gitdir")));
assert!(root_paths.contains(&common_git_dir.canonicalize().expect("canonical common git")));
// Test resolving from a subdirectory of the worktree
let subdirectory = worktree.join("src");
std::fs::create_dir_all(&subdirectory).expect("mkdir subdirectory");
let root_paths_sub: Vec<PathBuf> = policy
.get_writable_roots(&subdirectory)
.into_iter()
.map(|root| root.root)
.collect();
assert!(root_paths_sub.contains(&worktree.canonicalize().expect("canonical worktree")));
assert!(root_paths_sub.contains(&worktree_git_dir.canonicalize().expect("canonical gitdir")));
assert!(root_paths_sub.contains(&common_git_dir.canonicalize().expect("canonical common git")));
} |
||
|
|
||
| #[test] | ||
| fn test_writable_root_basic() { | ||
| let root = WritableRoot::new(PathBuf::from("/project")); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a command is executed from a subdirectory of a git worktree (which is extremely common),
root.join(".git")will not exist because the.gitfile is only located at the root of the worktree. As a result, the sandbox will fail to detect the worktree metadata roots, and git commands will be blocked by the sandbox.To fix this, we should traverse upwards using
root.ancestors()to find the.gitfile (or directory, in which case we can stop early as it's a normal repository).