From e7806cb7a7269b377a6e7cab89250494495d29b8 Mon Sep 17 00:00:00 2001 From: Justin Schneck Date: Thu, 13 Aug 2026 09:25:32 -0400 Subject: [PATCH 1/3] sdk run: quote argv elements instead of joining them raw `sdk run` spliced the user's argv into a shell script inside the container with a bare `cmd.join(" ")`, so the container shell re-split it. This: avocado sdk run -- bash -lc 'U=/opt/x; ls $U' reached the container as: bash -lc U=/opt/x; ls $U which the shell reads as two commands: a throwaway `bash -lc U=/opt/x`, then `ls $U` with `$U` expanded by the *outer* shell to nothing -- i.e. `ls`. It silently listed the wrong directory instead of failing, which makes it an actively misleading debugging tool. Quote each element with the existing utils::runs_on::shell_escape (now pub(crate)). Extract join_argv so the behavior is unit-testable without a container. --- src/commands/sdk/run.rs | 40 ++++++++++++++++++++++++++++++++++++++-- src/utils/runs_on.rs | 2 +- 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/src/commands/sdk/run.rs b/src/commands/sdk/run.rs index ce55e8ab..6ac5d80e 100644 --- a/src/commands/sdk/run.rs +++ b/src/commands/sdk/run.rs @@ -14,6 +14,17 @@ use crate::utils::{ target::validate_and_log_target, }; +/// Join a user-supplied argv into one shell-safe command string. +/// +/// The result is spliced into a shell script inside the container, so each +/// element must be quoted or the container shell re-splits it. +fn join_argv(argv: &[String]) -> String { + argv.iter() + .map(|a| crate::utils::runs_on::shell_escape(a)) + .collect::>() + .join(" ") +} + /// Implementation of the 'sdk run' command. pub struct SdkRunCommand { /// Path to configuration file @@ -310,9 +321,16 @@ impl SdkRunCommand { println!("Container name: {name}"); } - // Build the command to execute + // Build the command to execute. + // + // The argv vector is spliced into a shell script inside the container, so + // each element has to be quoted or the container shell re-splits it. A bare + // join(" ") turns `-- bash -lc 'X=1; echo $X'` into + // `bash -lc X=1; echo $X`, which the shell reads as two commands and + // expands `$X` in the *outer* shell (to nothing). That silently produces + // wrong output instead of an error, which is worse than failing. let command = if let Some(ref cmd) = self.command { - let user_command = cmd.join(" "); + let user_command = join_argv(cmd); if self.env { format!(". avocado-env && {user_command}") } else { @@ -442,6 +460,24 @@ mod tests { assert_eq!(cmd.command, Some(vec!["ls".to_string(), "-la".to_string()])); } + #[test] + fn test_join_argv_preserves_argv_boundaries() { + // A bare join(" ") would emit `bash -lc X=1; echo $X`, which the + // container shell splits into two commands and expands `$X` itself. + let argv = vec![ + "bash".to_string(), + "-lc".to_string(), + "X=1; echo $X".to_string(), + ]; + assert_eq!(join_argv(&argv), "'bash' '-lc' 'X=1; echo $X'"); + } + + #[test] + fn test_join_argv_escapes_embedded_single_quotes() { + let argv = vec!["echo".to_string(), "it's".to_string()]; + assert_eq!(join_argv(&argv), "'echo' 'it'\\''s'"); + } + #[tokio::test] async fn test_invalid_arguments() { let cmd = SdkRunCommand::new( diff --git a/src/utils/runs_on.rs b/src/utils/runs_on.rs index 51f60a68..9cd5d149 100644 --- a/src/utils/runs_on.rs +++ b/src/utils/runs_on.rs @@ -693,7 +693,7 @@ impl RunsOnContext { } /// Shell escape a string for safe use in a shell command -fn shell_escape(s: &str) -> String { +pub(crate) fn shell_escape(s: &str) -> String { format!("'{}'", s.replace('\'', "'\\''")) } From c785a6939f53cfd6fb639fff29342e4cbd6559c4 Mon Sep 17 00:00:00 2001 From: Justin Schneck Date: Thu, 13 Aug 2026 21:21:21 -0400 Subject: [PATCH 2/3] review: changelog the expansion loss, fix stale flag text, rehome shell_escape - CHANGELOG entry under Fixed. The people this affects are the ones who were relying on the container shell to expand a $VAR or a glob out of a single argument, and that loss is invisible in the diff, so it says so explicitly and names the `bash -lc` form to move to. - The "You must either provide a --command (-c)" error names a flag that does not exist: the command is a trailing positional, and -c is unassigned (-C is --config). Reworded to "a command to run", and the test asserting on that string follows it. - shell_escape moves from utils::runs_on to a new utils::shell. A generic quoting helper living in the remote-execution module meant a local, non-remote path had to reach into runs_on for it. Its four tests move with it, and the import is a `use` rather than an inline path now. --- CHANGELOG.md | 17 +++++++++++++++++ src/commands/sdk/run.rs | 7 ++++--- src/utils/mod.rs | 1 + src/utils/runs_on.rs | 34 +--------------------------------- src/utils/shell.rs | 38 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 61 insertions(+), 36 deletions(-) create mode 100644 src/utils/shell.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 57007d78..097be8db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 that collapse is what let an unparseable version fall through as a pass. ### Fixed +- **`sdk run` no longer lets the container shell re-split its argv.** The + arguments after `--` were spliced into the container's script with a bare + `join(" ")`, so the shell inside re-parsed them. `avocado sdk run -- bash + -lc 'U=/opt/x; ls $U'` arrived as `bash -lc U=/opt/x; ls $U`, which the shell + read as two commands and whose `$U` the *outer* shell expanded to nothing — + printing plausible output for a different directory rather than failing. + Each element is quoted now, so argv is argv, matching `docker run` and + `kubectl exec`. + + **This removes an expansion that some invocations relied on.** Anything + passing a `$VAR`, a glob, or a `&&` chain as a single argument and counting + on the container shell to interpret it now gets that string through + literally: `avocado sdk run -- 'ls /foo && ls /bar'` becomes one command + word, and `avocado sdk run -- echo '$HOME'` prints `$HOME`. Ask for a shell + explicitly instead — `avocado sdk run -- bash -lc 'ls /foo && ls /bar'` — + which is the form the flags already implied and which only works correctly + after this change. - **Rootfs and initramfs no longer reinstall on every run.** `avocado sdk install` wiped and rebuilt both sysroots from scratch on every invocation, even with nothing changed. Removal detection compared the lockfile against diff --git a/src/commands/sdk/run.rs b/src/commands/sdk/run.rs index 6ac5d80e..8022f11e 100644 --- a/src/commands/sdk/run.rs +++ b/src/commands/sdk/run.rs @@ -11,6 +11,7 @@ use crate::utils::{ config::{ComposedConfig, Config}, container::{RunConfig, SdkContainer}, output::{print_info, print_success, OutputLevel}, + shell::shell_escape, target::validate_and_log_target, }; @@ -20,7 +21,7 @@ use crate::utils::{ /// element must be quoted or the container shell re-splits it. fn join_argv(argv: &[String]) -> String { argv.iter() - .map(|a| crate::utils::runs_on::shell_escape(a)) + .map(|a| shell_escape(a)) .collect::>() .join(" ") } @@ -272,7 +273,7 @@ impl SdkRunCommand { // Require either a command or --interactive flag if !self.interactive && self.command.is_none() { return Err(anyhow::anyhow!( - "You must either provide a --command (-c) or use --interactive (-i)." + "You must either provide a command to run or use --interactive (-i)." )); } @@ -529,7 +530,7 @@ mod tests { assert!(result .unwrap_err() .to_string() - .contains("You must either provide a --command")); + .contains("You must either provide a command to run")); } #[test] diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 7d62bae5..d61e17b9 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -28,6 +28,7 @@ pub mod runs_on; pub mod runtime; pub mod runtime_extension; pub mod scheduler; +pub mod shell; pub mod signing_keys; #[cfg(unix)] pub mod signing_service; diff --git a/src/utils/runs_on.rs b/src/utils/runs_on.rs index 9cd5d149..c382b807 100644 --- a/src/utils/runs_on.rs +++ b/src/utils/runs_on.rs @@ -18,6 +18,7 @@ use crate::utils::remote::{ get_local_ip_for_remote, version_check_notice, RemoteHost, RemoteVolumeManager, SshClient, SshControlMaster, VersionNotice, }; +use crate::utils::shell::shell_escape; #[cfg(unix)] use crate::utils::remote::SshTunnel; @@ -691,36 +692,3 @@ impl RunsOnContext { Ok(()) } } - -/// Shell escape a string for safe use in a shell command -pub(crate) fn shell_escape(s: &str) -> String { - format!("'{}'", s.replace('\'', "'\\''")) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_shell_escape_simple() { - assert_eq!(shell_escape("hello"), "'hello'"); - } - - #[test] - fn test_shell_escape_with_spaces() { - assert_eq!(shell_escape("hello world"), "'hello world'"); - } - - #[test] - fn test_shell_escape_with_quotes() { - assert_eq!(shell_escape("it's"), "'it'\\''s'"); - } - - #[test] - fn test_shell_escape_complex() { - assert_eq!( - shell_escape("echo 'hello' && rm -rf /"), - "'echo '\\''hello'\\'' && rm -rf /'" - ); - } -} diff --git a/src/utils/shell.rs b/src/utils/shell.rs new file mode 100644 index 00000000..2c9ae85b --- /dev/null +++ b/src/utils/shell.rs @@ -0,0 +1,38 @@ +//! Shell quoting helpers. +//! +//! Several commands splice values into a shell script that runs inside the SDK +//! container. Anything user-supplied has to be quoted on the way in or the +//! container shell re-splits and expands it. + +/// Shell escape a string for safe use in a shell command +pub(crate) fn shell_escape(s: &str) -> String { + format!("'{}'", s.replace('\'', "'\\''")) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_shell_escape_simple() { + assert_eq!(shell_escape("hello"), "'hello'"); + } + + #[test] + fn test_shell_escape_with_spaces() { + assert_eq!(shell_escape("hello world"), "'hello world'"); + } + + #[test] + fn test_shell_escape_with_quotes() { + assert_eq!(shell_escape("it's"), "'it'\\''s'"); + } + + #[test] + fn test_shell_escape_complex() { + assert_eq!( + shell_escape("echo 'hello' && rm -rf /"), + "'echo '\\''hello'\\'' && rm -rf /'" + ); + } +} From 4d54dfa356b74b3685d26ca13425fa883922626a Mon Sep 17 00:00:00 2001 From: Justin Schneck Date: Tue, 18 Aug 2026 07:59:15 -0700 Subject: [PATCH 3/3] changelog: file the sdk run argv change under Breaking The entry described a user-visible CLI break that fails silently at runtime, but sat under Fixed, while Breaking held only a change scoped to consumers of the lib target. A maintainer bumping a pinned CLI in CI and reading Breaking for upgrade impact would have concluded lib-only and shipped. Moves the sdk run entry into Breaking, ahead of the lib-target one so the section reads worst-blast-radius first, and leads on the expansion loss rather than the bug, with the fix as the second half. Adds the bit that was missing either way: this break produces no error and no warning, so CI will not catch it. --- CHANGELOG.md | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 097be8db..7022b1cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 is skipped with nothing to say rather than asserting a version it never read. ### Breaking +- **`sdk run` passes its argv through literally; the container shell no longer + re-splits or expands it.** Anything passing a `$VAR`, a glob, or a `&&` chain + as a single argument and counting on the container shell to interpret it now + gets that string through as one command word: `avocado sdk run -- 'ls /foo && + ls /bar'` no longer runs two commands, and `avocado sdk run -- echo '$HOME'` + prints a literal `$HOME`. Ask for a shell explicitly instead — `avocado sdk + run -- bash -lc 'ls /foo && ls /bar'` — which is the form the flags already + implied and which only works correctly after this change. + + This one breaks silently at runtime: there is no error and no warning, so a + pinned-version bump in CI will not fail the build, it will just run a + different command. Audit any job that shells through `sdk run` before + upgrading. + + **What it fixes.** The arguments after `--` were spliced into the container's + script with a bare `join(" ")`, so the shell inside re-parsed them. `avocado + sdk run -- bash -lc 'U=/opt/x; ls $U'` arrived as `bash -lc U=/opt/x; ls $U`, + which the shell read as two commands and whose `$U` the *outer* shell expanded + to nothing — printing plausible output for a different directory rather than + failing. Each element is quoted now, so argv is argv, matching `docker run` + and `kubectl exec`. - **`is_version_compatible` returns `Option` instead of `bool`.** Only affects consumers of the `avocado_cli` lib target. `None` means the remote version could not be read at all, which the old `bool` could not express - @@ -49,23 +70,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 that collapse is what let an unparseable version fall through as a pass. ### Fixed -- **`sdk run` no longer lets the container shell re-split its argv.** The - arguments after `--` were spliced into the container's script with a bare - `join(" ")`, so the shell inside re-parsed them. `avocado sdk run -- bash - -lc 'U=/opt/x; ls $U'` arrived as `bash -lc U=/opt/x; ls $U`, which the shell - read as two commands and whose `$U` the *outer* shell expanded to nothing — - printing plausible output for a different directory rather than failing. - Each element is quoted now, so argv is argv, matching `docker run` and - `kubectl exec`. - - **This removes an expansion that some invocations relied on.** Anything - passing a `$VAR`, a glob, or a `&&` chain as a single argument and counting - on the container shell to interpret it now gets that string through - literally: `avocado sdk run -- 'ls /foo && ls /bar'` becomes one command - word, and `avocado sdk run -- echo '$HOME'` prints `$HOME`. Ask for a shell - explicitly instead — `avocado sdk run -- bash -lc 'ls /foo && ls /bar'` — - which is the form the flags already implied and which only works correctly - after this change. - **Rootfs and initramfs no longer reinstall on every run.** `avocado sdk install` wiped and rebuilt both sysroots from scratch on every invocation, even with nothing changed. Removal detection compared the lockfile against