Repository navigation
cli: harden clean and Nix provenance execution #102
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,7 +173,10 @@ pub struct DemoDriftArgs { | |
| #[arg(long, default_value_t = 1)] | ||
| pub threads: usize, | ||
|
|
||
| #[arg(long)] | ||
| #[arg( | ||
| long, | ||
| help = "Clean output dir first; refuses dangerous paths (. , root, home, .git, symlinks)" | ||
| )] | ||
| pub clean: bool, | ||
| } | ||
|
|
||
|
|
@@ -199,7 +202,10 @@ pub struct RunArgs { | |
| #[arg(long, default_value = "out")] | ||
| pub out_dir: PathBuf, | ||
|
|
||
| #[arg(long)] | ||
| #[arg( | ||
| long, | ||
| help = "Clean output dir first; refuses dangerous paths (. , root, home, .git, symlinks)" | ||
| )] | ||
| pub clean: bool, | ||
|
|
||
| #[arg(long)] | ||
|
|
@@ -314,7 +320,7 @@ pub struct RunArgs { | |
| long, | ||
| default_value = "auto", | ||
| value_enum, | ||
| help = "Capture Nix provenance (auto|on|off)" | ||
| help = "Capture Nix provenance (auto|on|off). auto/on may run bounded local nix commands for diagnostic provenance only" | ||
| )] | ||
| pub nix_provenance: nix_provenance::NixProvenanceMode, | ||
| } | ||
|
|
@@ -385,7 +391,7 @@ fn run(args: RunArgs) -> Result<()> { | |
| let (summary, pass_count, fail_count) = eval::summarize_with_counts(&output.results); | ||
|
|
||
| if args.clean && args.out_dir.exists() { | ||
| fs::remove_dir_all(&args.out_dir).with_context(|| "failed to clean output dir")?; | ||
| safe_clean_output_dir(&args.out_dir, "output dir")?; | ||
| } | ||
|
|
||
| fs::create_dir_all(&args.out_dir).with_context(|| "failed to create output dir")?; | ||
|
|
@@ -527,7 +533,7 @@ fn demo_cmd(args: DemoArgs) -> Result<()> { | |
|
|
||
| fn demo_drift(args: DemoDriftArgs) -> Result<()> { | ||
| if args.clean && args.out_dir.exists() { | ||
| fs::remove_dir_all(&args.out_dir).with_context(|| "failed to clean demo output dir")?; | ||
| safe_clean_output_dir(&args.out_dir, "demo output dir")?; | ||
| } | ||
| fs::create_dir_all(&args.out_dir).with_context(|| "failed to create demo output dir")?; | ||
|
|
||
|
|
@@ -780,7 +786,7 @@ fn run_agent(args: RunArgs) -> Result<()> { | |
|
|
||
| thread_pool.install(|| -> Result<()> { | ||
| if args.clean && args.out_dir.exists() { | ||
| fs::remove_dir_all(&args.out_dir).with_context(|| "failed to clean output dir")?; | ||
| safe_clean_output_dir(&args.out_dir, "output dir")?; | ||
| } | ||
| fs::create_dir_all(&args.out_dir).with_context(|| "failed to create output dir")?; | ||
|
|
||
|
|
@@ -1616,15 +1622,78 @@ fn command_version(command: &str) -> Option<String> { | |
| Some(String::from_utf8_lossy(&output.stdout).trim().to_string()) | ||
| } | ||
|
|
||
| fn safe_clean_output_dir(path: &Path, label: &str) -> Result<()> { | ||
| validate_clean_output_dir(path, Path::new("."))?; | ||
| fs::remove_dir_all(path).with_context(|| format!("failed to clean {}", label))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_clean_output_dir(path: &Path, repo_root: &Path) -> Result<()> { | ||
| if path.as_os_str().is_empty() { | ||
| anyhow::bail!("refusing to clean an empty output path"); | ||
| } | ||
| if path == Path::new(".") { | ||
| anyhow::bail!("refusing to clean current directory '.'"); | ||
| } | ||
|
|
||
| let candidate = path.canonicalize().unwrap_or_else(|_| path.to_path_buf()); | ||
| if candidate.parent().is_none() { | ||
| anyhow::bail!("refusing to clean filesystem root"); | ||
| } | ||
|
|
||
| if let Ok(repo_canon) = repo_root.canonicalize() { | ||
| if candidate == repo_canon { | ||
| anyhow::bail!("refusing to clean repository root"); | ||
| } | ||
| } | ||
|
|
||
| if let Some(home) = user_home_dir() { | ||
| if candidate == home { | ||
| anyhow::bail!("refusing to clean user home directory"); | ||
| } | ||
| } | ||
|
|
||
| if let Some(name) = candidate | ||
| .file_name() | ||
| .or_else(|| path.file_name()) | ||
| .map(|name| name.to_string_lossy().to_ascii_lowercase()) | ||
| { | ||
| if name == ".git" { | ||
| anyhow::bail!("refusing to clean git metadata directory"); | ||
| } | ||
| } | ||
|
|
||
| if let Ok(metadata) = fs::symlink_metadata(path) { | ||
| if metadata.file_type().is_symlink() { | ||
| anyhow::bail!("refusing to clean symlink output directory"); | ||
|
Comment on lines
+1666
to
+1668
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.
The symlink check uses Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn user_home_dir() -> Option<PathBuf> { | ||
| #[cfg(windows)] | ||
| { | ||
| std::env::var_os("USERPROFILE").map(PathBuf::from) | ||
| } | ||
| #[cfg(not(windows))] | ||
| { | ||
| std::env::var_os("HOME").map(PathBuf::from) | ||
| } | ||
| } | ||
|
|
||
| fn read_trimmed(path: &Path) -> Result<String> { | ||
| let content = fs::read_to_string(path).with_context(|| "failed to read witness file")?; | ||
| Ok(content.trim().to_string()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::{resolve_ordeal_regress, resolve_replay_artifact_path, Cli}; | ||
| use super::{ | ||
| resolve_ordeal_regress, resolve_replay_artifact_path, validate_clean_output_dir, Cli, | ||
| }; | ||
| use clap::Parser; | ||
| use std::path::Path; | ||
| use std::sync::{Mutex, OnceLock}; | ||
| use tempfile::tempdir; | ||
|
|
||
|
|
@@ -1725,4 +1794,35 @@ mod tests { | |
| .expect("resolve inside bundle"); | ||
| assert!(resolved.ends_with("agent_trace.json")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn clean_guard_rejects_dangerous_paths() { | ||
| assert!(validate_clean_output_dir(Path::new("."), Path::new(".")).is_err()); | ||
| assert!(validate_clean_output_dir(Path::new("/"), Path::new(".")).is_err()); | ||
| assert!(validate_clean_output_dir(Path::new(".git"), Path::new(".")).is_err()); | ||
| assert!(validate_clean_output_dir(Path::new(""), Path::new(".")).is_err()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn clean_guard_allows_normal_output_paths() { | ||
| let tmp = tempdir().expect("tempdir"); | ||
| let repo_root = tmp.path().join("repo"); | ||
| let out_dir = repo_root.join("out"); | ||
| std::fs::create_dir_all(&out_dir).expect("create out dir"); | ||
| validate_clean_output_dir(&out_dir, &repo_root).expect("safe output dir"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn clean_guard_rejects_symlink_dirs() { | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::symlink; | ||
| let tmp = tempdir().expect("tempdir"); | ||
| let target = tmp.path().join("target"); | ||
| let link = tmp.path().join("link"); | ||
| std::fs::create_dir_all(&target).expect("create target"); | ||
| symlink(&target, &link).expect("create symlink"); | ||
| assert!(validate_clean_output_dir(&link, Path::new(".")).is_err()); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,14 @@ use anyhow::{Context, Result}; | |
| use clap::ValueEnum; | ||
| use serde_json::Value; | ||
| use std::path::Path; | ||
| use std::process::Command; | ||
| use std::process::{Command, Stdio}; | ||
| use std::thread; | ||
| use std::time::{Duration, Instant}; | ||
|
|
||
| use crate::model::NixProvenance; | ||
|
|
||
| const MAX_OUTPUT_BYTES: usize = 128 * 1024; | ||
| const COMMAND_TIMEOUT: Duration = Duration::from_secs(3); | ||
|
|
||
| #[derive(ValueEnum, Clone, Debug)] | ||
| pub enum NixProvenanceMode { | ||
|
|
@@ -99,7 +102,7 @@ fn current_system_info() -> Option<Value> { | |
| } | ||
|
|
||
| fn command_output(command: &str, args: &[&str]) -> Option<String> { | ||
| let output = Command::new(command).args(args).output().ok()?; | ||
| let output = run_bounded_command(command, args, None).ok()?; | ||
| if !output.status.success() { | ||
| return None; | ||
| } | ||
|
|
@@ -115,12 +118,7 @@ fn command_output(command: &str, args: &[&str]) -> Option<String> { | |
| } | ||
|
|
||
| fn command_json(command: &str, args: &[&str], cwd: Option<&Path>) -> Option<Value> { | ||
| let mut cmd = Command::new(command); | ||
| cmd.args(args); | ||
| if let Some(dir) = cwd { | ||
| cmd.current_dir(dir); | ||
| } | ||
| let output = cmd.output().ok()?; | ||
| let output = run_bounded_command(command, args, cwd).ok()?; | ||
| if !output.status.success() || output.stdout.len() > MAX_OUTPUT_BYTES { | ||
| return None; | ||
| } | ||
|
|
@@ -129,6 +127,66 @@ fn command_json(command: &str, args: &[&str], cwd: Option<&Path>) -> Option<Valu | |
| .map(canonicalize_json) | ||
| } | ||
|
|
||
| fn run_bounded_command( | ||
| command: &str, | ||
| args: &[&str], | ||
| cwd: Option<&Path>, | ||
| ) -> std::io::Result<std::process::Output> { | ||
| let mut cmd = Command::new(command); | ||
| cmd.args(args) | ||
| .stdin(Stdio::null()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()); | ||
| if let Some(dir) = cwd { | ||
| cmd.current_dir(dir); | ||
| } | ||
|
|
||
| for key in [ | ||
| "HTTP_PROXY", | ||
| "HTTPS_PROXY", | ||
| "ALL_PROXY", | ||
| "NO_PROXY", | ||
| "http_proxy", | ||
| "https_proxy", | ||
| "all_proxy", | ||
| "no_proxy", | ||
| "AWS_SECRET_ACCESS_KEY", | ||
| "AWS_SESSION_TOKEN", | ||
| "GITHUB_TOKEN", | ||
| "NIX_CONFIG", | ||
| ] { | ||
| cmd.env_remove(key); | ||
| } | ||
|
|
||
| let mut child = cmd.spawn()?; | ||
| let deadline = Instant::now() + COMMAND_TIMEOUT; | ||
| loop { | ||
| if child.try_wait()?.is_some() { | ||
| let mut output = child.wait_with_output()?; | ||
| truncate_output(&mut output.stdout); | ||
|
Comment on lines
+163
to
+166
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.
Useful? React with 👍 / 👎. |
||
| truncate_output(&mut output.stderr); | ||
| return Ok(output); | ||
| } | ||
|
|
||
| if Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::TimedOut, | ||
| "nix provenance command timed out", | ||
| )); | ||
| } | ||
|
|
||
| thread::sleep(Duration::from_millis(25)); | ||
| } | ||
| } | ||
|
|
||
| fn truncate_output(bytes: &mut Vec<u8>) { | ||
| if bytes.len() > MAX_OUTPUT_BYTES { | ||
| bytes.truncate(MAX_OUTPUT_BYTES); | ||
| } | ||
| } | ||
|
|
||
| fn canonicalize_json(value: Value) -> Value { | ||
| match value { | ||
| Value::Object(map) => { | ||
|
|
@@ -150,3 +208,15 @@ pub fn write_nix_provenance(path: &Path, provenance: &NixProvenance) -> Result<( | |
| .with_context(|| "failed to write nix_provenance.json")?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::run_bounded_command; | ||
|
|
||
| #[test] | ||
| #[cfg(unix)] | ||
| fn bounded_command_times_out() { | ||
| let err = run_bounded_command("sh", &["-c", "sleep 10"], None).expect_err("must timeout"); | ||
| assert_eq!(err.kind(), std::io::ErrorKind::TimedOut); | ||
| } | ||
| } | ||
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.
safe_clean_output_dirpassesPath::new(".")as the repository root sentinel, so the guard only protects the current working directory, not the actual Git top-level directory. If the CLI is invoked from a subdirectory,--out-dir ..(or another path resolving to the repo root) can pass validation and be recursively removed, which defeats the intended repository-root protection.Useful? React with 👍 / 👎.