Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions crates/nono-cli/src/tool-sandbox/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,34 @@ pub(crate) fn lexically_normalize(path: &std::path::Path) -> std::path::PathBuf
normalized
}

/// Restore owner-write on every directory in `dir`'s tree so a following
/// `remove_dir_all` can unlink entries inside sealed subdirectories.
///
/// The shim directory is sealed to `0o500` while the sandbox runs so its
/// private shim copies stay immutable. Unlinking a file needs write on its
/// *parent* directory and `remove_dir_all` does not chmod as it descends, so
/// without this the per-invocation runtime directory leaks on every exit.
/// Best-effort and never follows or modifies symlinks; a real failure still
/// surfaces from the subsequent `remove_dir_all`.
#[cfg(any(target_os = "linux", target_os = "macos"))]
pub(crate) fn restore_dir_tree_writable(dir: &std::path::Path) {
use std::os::unix::fs::PermissionsExt;
let Ok(meta) = std::fs::symlink_metadata(dir) else {
return;
};
if !meta.is_dir() || meta.file_type().is_symlink() {
return;
}
// Grant owner rwx before descending so we can traverse and unlink children.
let _ = std::fs::set_permissions(dir, std::fs::Permissions::from_mode(0o700));
let Ok(entries) = std::fs::read_dir(dir) else {
return;
};
for entry in entries.flatten() {
restore_dir_tree_writable(&entry.path());
}
}

/// Whether the agent's own filesystem grants admit *writing* `path` directly.
///
/// True when `path` is (or is under) the agent's own `--workdir`
Expand Down
40 changes: 40 additions & 0 deletions crates/nono-cli/src/tool-sandbox/platform/linux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5112,6 +5112,9 @@ fn guarded_remove_runtime_dir(path: &Path) -> Result<()> {
path.display()
)));
}
// The `shims` subdir is sealed to 0o500; re-grant owner-write across the
// tree so `remove_dir_all` can unlink the sealed shim copies inside it.
crate::tool_sandbox::restore_dir_tree_writable(path);
fs::remove_dir_all(path).map_err(|source| NonoError::ConfigWrite {
path: path.to_path_buf(),
source,
Expand Down Expand Up @@ -5545,6 +5548,43 @@ mod tests {
})
}

#[test]
fn guarded_remove_deletes_runtime_dir_with_sealed_shims() -> Result<()> {
let tmp = test_tempdir()?;
let runtime = tmp.path().join("nono-tool-sandbox-test");
std::fs::create_dir(&runtime).map_err(|source| NonoError::ConfigWrite {
path: runtime.clone(),
source,
})?;
std::fs::set_permissions(&runtime, std::fs::Permissions::from_mode(0o700)).map_err(
|source| NonoError::ConfigWrite {
path: runtime.clone(),
source,
},
)?;
let shim_dir = create_shim_dir(&runtime)?;
let shim = shim_dir.join("git");
std::fs::write(&shim, b"shim").map_err(|source| NonoError::ConfigWrite {
path: shim.clone(),
source,
})?;
std::fs::set_permissions(&shim, std::fs::Permissions::from_mode(0o500)).map_err(
|source| NonoError::ConfigWrite {
path: shim.clone(),
source,
},
)?;
// Seal the shim dir to 0o500 exactly as the live runtime does.
seal_shim_dir(&shim_dir)?;

// Regression: with the shim dir sealed, a naive remove_dir_all cannot
// unlink its contents, so cleanup must first re-grant owner-write.
guarded_remove_runtime_dir(&runtime)?;

assert!(!runtime.exists(), "sealed runtime dir was not removed");
Ok(())
}

fn create_dir(path: &Path) -> Result<()> {
fs::create_dir(path).map_err(|source| NonoError::ConfigWrite {
path: path.to_path_buf(),
Expand Down
46 changes: 40 additions & 6 deletions crates/nono-cli/src/tool-sandbox/platform/macos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4737,12 +4737,9 @@ fn guarded_remove_runtime_dir(dir: &Path) -> Result<()> {
dir.display()
)));
}
fs::set_permissions(dir, fs::Permissions::from_mode(0o700)).map_err(|e| {
NonoError::ConfigWrite {
path: dir.to_path_buf(),
source: e,
}
})?;
// The `shims` subdir is sealed to 0o500; re-grant owner-write across the
// tree so `remove_dir_all` can unlink the sealed shim copies inside it.
crate::tool_sandbox::restore_dir_tree_writable(dir);
fs::remove_dir_all(dir).map_err(|e| NonoError::ConfigWrite {
path: dir.to_path_buf(),
source: e,
Expand Down Expand Up @@ -6110,6 +6107,43 @@ mod tests {
Ok(())
}

#[test]
fn guarded_remove_deletes_runtime_dir_with_sealed_shims() -> Result<()> {
let tmp = test_tempdir()?;
let runtime = tmp.path().join("nono-tool-sandbox-test");
fs::create_dir(&runtime).map_err(|source| NonoError::ConfigWrite {
path: runtime.clone(),
source,
})?;
fs::set_permissions(&runtime, fs::Permissions::from_mode(0o700)).map_err(|source| {
NonoError::ConfigWrite {
path: runtime.clone(),
source,
}
})?;
let shim_dir = create_shim_dir(&runtime)?;
let shim = shim_dir.join("git");
fs::write(&shim, b"shim").map_err(|source| NonoError::ConfigWrite {
path: shim.clone(),
source,
})?;
fs::set_permissions(&shim, fs::Permissions::from_mode(0o500)).map_err(|source| {
NonoError::ConfigWrite {
path: shim.clone(),
source,
}
})?;
// Seal the shim dir to 0o500 exactly as the live runtime does.
seal_shim_dir(&shim_dir)?;

// Regression: with the shim dir sealed, a naive remove_dir_all cannot
// unlink its contents, so cleanup must first re-grant owner-write.
guarded_remove_runtime_dir(&runtime)?;

assert!(!runtime.exists(), "sealed runtime dir was not removed");
Ok(())
}

#[test]
fn selected_stdio_mode_uses_supervisor_direct_fds() {
let request = request_with_env(Vec::new());
Expand Down