diff --git a/CHANGELOG.md b/CHANGELOG.md index 31a0597c..5c51bd09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,6 +52,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 - diff --git a/src/commands/sdk/run.rs b/src/commands/sdk/run.rs index ce55e8ab..8022f11e 100644 --- a/src/commands/sdk/run.rs +++ b/src/commands/sdk/run.rs @@ -11,9 +11,21 @@ use crate::utils::{ config::{ComposedConfig, Config}, container::{RunConfig, SdkContainer}, output::{print_info, print_success, OutputLevel}, + shell::shell_escape, 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| shell_escape(a)) + .collect::>() + .join(" ") +} + /// Implementation of the 'sdk run' command. pub struct SdkRunCommand { /// Path to configuration file @@ -261,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)." )); } @@ -310,9 +322,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 +461,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( @@ -493,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 afd54eff..fcb23e9a 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -30,6 +30,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 51f60a68..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 -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 /'" + ); + } +}