Repository navigation
cli: harden clean and Nix provenance execution - #102
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dc7c61784
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loop { | ||
| if child.try_wait()?.is_some() { | ||
| let mut output = child.wait_with_output()?; | ||
| truncate_output(&mut output.stdout); |
There was a problem hiding this comment.
run_bounded_command polls try_wait() and only reads stdout/stderr via wait_with_output() after the child has exited. Since both streams are set to Stdio::piped(), a command that writes more than the OS pipe buffer before exiting can block on write, which means it never reaches an exited state and this loop times out after 3s. In practice this can make valid nix ... --json provenance calls fail spuriously on larger outputs (e.g., large flake metadata). The timeout path needs concurrent pipe draining (or non-piped output) to avoid this regression.
Useful? React with 👍 / 👎.
| if let Ok(metadata) = fs::symlink_metadata(path) { | ||
| if metadata.file_type().is_symlink() { | ||
| anyhow::bail!("refusing to clean symlink output directory"); |
There was a problem hiding this comment.
The symlink check uses symlink_metadata(path) directly, which can be bypassed with a trailing separator (--out-dir link/): on Unix, symlink_metadata("link/") reports the target directory rather than the symlink, so this guard does not fire. safe_clean_output_dir then calls remove_dir_all on that path, which can delete contents of the symlink target before erroring, reintroducing the destructive --clean footgun for symlinked dirs.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| fn safe_clean_output_dir(path: &Path, label: &str) -> Result<()> { | ||
| validate_clean_output_dir(path, Path::new("."))?; |
There was a problem hiding this comment.
safe_clean_output_dir passes Path::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 👍 / 👎.
1 participant
Motivation
--cleancould recursively delete an important path (root, home, repo,.git, symlinks) and make Nix provenance collection safe from unbounded/hanging external commands.Description
safe_clean_output_dir(path: &Path, label: &str) -> Result<()>andvalidate_clean_output_dir(path: &Path, repo_root: &Path) -> Result<()>, and replaced directfs::remove_dir_allcall sites for all--cleanpaths with the helper insrc/main.rs.validate_clean_output_dirrejects empty paths, filesystem root,.(current dir), repository root when detectable, user home (fromHOME/USERPROFILE), final-component.git, and symlinked output directories; normal output dirs likeout/demo_outand nested project dirs remain allowed.src/nix_provenance.rsby introducingrun_bounded_command(...)which uses closed stdin (Stdio::null()), a timeout (COMMAND_TIMEOUT = 3s), bounded stdout/stderr capture with truncation toMAX_OUTPUT_BYTES = 128 * 1024, and environment scrubbing viaenv_removefor selected proxy/credential-like variables (e.g.,HTTP_PROXY/HTTPS_PROXYvariants,ALL_PROXY,NO_PROXY,AWS_SECRET_ACCESS_KEY,AWS_SESSION_TOKEN,GITHUB_TOKEN,NIX_CONFIG).--nix-provenance=autobehavior while documenting thatauto|onmay run bounded local Nix commands for diagnostic provenance only.--cleansafety guard and provenance execution note.src/main.rs,src/nix_provenance.rs,README.md.Testing
cargo fmt --check(ok),cargo clippy --locked --all-targets -- -D warnings(ok), andcargo test --locked(ok) which included the new tests for clean-guard and bounded command behavior.cargo run --locked -- ordeal check --golden goldens/ordeal_witness_root.txt(ok),cargo run --locked -- demo drift --seed 1 --threads 1 --fault-profile ci --out-dir demo_out --clean(ok),cargo run --locked -- verify --witness demo_out/drift/baseline_faults(ok),cargo run --locked -- verify --witness demo_out/drift/baseline_faults --recompute-witness-root(ok), andbash scripts/check_action_pins.sh(ok).validate_clean_output_dirrejects dangerous paths and permits normal output dirs, symlink rejection on Unix, and bounded provenance command timeout behavior; all passed.Non-goals (explicit): no changes to witness-root algorithm, schema constants, canonicalization, verifier semantics, release workflows, or production/provider integrations.
Codex Task