xtask: Add local-rust-deps command for auto-detecting path dependencies #1942
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new "cargo xtask local-rust-deps" command to automatically detect and configure bind mounts for local path dependencies, significantly improving the local development workflow. However, the current implementation is vulnerable to command and argument injection due to unsafe interpolation in the Justfile and improper escaping of file paths in the xtask tool. Additionally, there are suggestions to improve debuggability and code clarity, such as avoiding error message swallowing in the Justfile and refactoring podman argument generation in the Rust code.
| if [[ -z "{{no_auto_local_deps}}" ]]; then | ||
| local_deps_args=$(cargo xtask local-rust-deps 2>/dev/null || true) | ||
| fi | ||
| podman build {{base_buildargs}} --build-arg=SOURCE_DATE_EPOCH=${SOURCE_DATE_EPOCH} --build-arg=pkgversion=${VERSION} -t localhost/bootc-pkg --target=build $local_deps_args . |
There was a problem hiding this comment.
The shell variable $local_deps_args is used unquoted in a podman build command. This variable contains the output of cargo xtask local-rust-deps, which includes file paths to local dependencies. If any of these paths contain spaces or shell metacharacters (like ;, &, or |), it will lead to argument injection or arbitrary command execution. Since these paths are derived from Cargo.toml files that could be located outside the workspace, this poses a significant security risk in environments where dependency paths might be untrusted or maliciously crafted.
There was a problem hiding this comment.
Man, the Gemini review rule for shell injection is one of those things where 20% of the time it's probably absolutely critical, and 50% it's pointless noise.
Rust crates I don't think can even have shell metacharacters.
| args_out.push("--security-opt=label=disable".to_string()); | ||
| } | ||
| if !args_out.is_empty() { | ||
| println!("{}", args_out.join(" ")); |
There was a problem hiding this comment.
The local_rust_deps function constructs a string of space-separated arguments for podman without performing any escaping or quoting on the paths. This is a command injection vulnerability, as paths with spaces or shell metacharacters could be incorrectly parsed by the shell in the Justfile. Paths should be properly escaped for shell consumption, for example, by using the shlex crate to quote each argument. Additionally, consider refactoring the --security-opt=label=disable argument to be specified only once for all volumes for cleaner command generation.
Justfile
Outdated
| # Auto-detect local Rust path dependencies (e.g., from [patch] sections) | ||
| local_deps_args="" | ||
| if [[ -z "{{no_auto_local_deps}}" ]]; then | ||
| local_deps_args=$(cargo xtask local-rust-deps 2>/dev/null || true) |
There was a problem hiding this comment.
Suppressing stderr with 2>/dev/null can hide important error messages if cargo xtask local-rust-deps fails for an unexpected reason (e.g., a malformed Cargo.toml or issues running cargo metadata). This can make debugging difficult. Since you're already using || true to prevent the script from failing, it would be better to let stderr be printed so the user is aware of any issues with local dependency detection.
local_deps_args=$(cargo xtask local-rust-deps || true)
cgwalters
force-pushed
the
local-rust-deps
branch
from
ec33e5a to
952588e
Compare
Add `cargo xtask local-rust-deps` which uses `cargo metadata` to find local path dependencies outside the workspace (e.g., from [patch] sections) and outputs podman bind mount arguments. This enables a cleaner workflow for local development against modified dependencies like composefs-rs: 1. Add a [patch] section to Cargo.toml with real local paths 2. Run `just build` - the Justfile auto-detects and bind-mounts them Benefits over the previous BOOTC_extra_src approach: - No manual env var needed - Paths work for both local `cargo build` and container builds - No /run/extra-src indirection or Cargo.toml path munging required - Auto-detection means it Just Works™ The Justfile's build target now calls `cargo xtask local-rust-deps` to get bind mount args, falling back gracefully if there are no external deps. The old BOOTC_extra_src mechanism is still supported for backwards compat. Assisted-by: OpenCode (Opus 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
Tests with equal numbers were getting unstable sorting, causing the generated file to flap. Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
local-rust-deps
branch
from
952588e to
7205ffe
Compare
1 participant
Add
cargo xtask local-rust-depswhich usescargo metadatato find local path dependencies outside the workspace (e.g., from [patch] sections) and outputs podman bind mount arguments.This enables a cleaner workflow for local development against modified dependencies like composefs-rs:
just build- the Justfile auto-detects and bind-mounts themBenefits over the previous BOOTC_extra_src approach:
cargo buildand container buildsThe Justfile's build target now calls
cargo xtask local-rust-depsto get bind mount args, falling back gracefully if there are no external deps. The old BOOTC_extra_src mechanism is still supported for backwards compat.Assisted-by: Claude Sonnet 4 (via OpenCode)