From 0679179efec73b9fd58e28086305cce048fe1bcd Mon Sep 17 00:00:00 2001 From: "zed-zippy[bot]" <234243425+zed-zippy[bot]@users.noreply.github.com> Date: Tue, 3 Mar 2026 15:42:22 -0700 Subject: [PATCH] Fix a few cases where we weren't escaping shell vars correctly (#50562) (cherry-pick to preview) (#50563) Cherry-pick of #50562 to preview ---- Release Notes: - N/A Co-authored-by: Conrad Irwin --- Cargo.lock | 1 + crates/gpui_linux/src/linux/platform.rs | 15 ++-- crates/remote/Cargo.toml | 1 + crates/remote/src/transport/docker.rs | 2 +- crates/remote/src/transport/ssh.rs | 49 +++++++++--- crates/remote/src/transport/wsl.rs | 11 +-- crates/repl/Cargo.toml | 1 + crates/repl/src/kernels/wsl_kernel.rs | 99 +++++++++++++------------ 8 files changed, 108 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8eb88fa552d7f4d1adc13320d8f8dad17e62b3c1..c81fdd60c7035f80e6ed68b7168747943c9c334f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -14057,6 +14057,7 @@ dependencies = [ "serde", "serde_json", "settings", + "shlex", "smol", "telemetry", "terminal", diff --git a/crates/gpui_linux/src/linux/platform.rs b/crates/gpui_linux/src/linux/platform.rs index 3be017b9409d4bc4b04f3065e32fff3ce7388d94..5db5785853659dbeb4f668d9a1b7b3e9765ed368 100644 --- a/crates/gpui_linux/src/linux/platform.rs +++ b/crates/gpui_linux/src/linux/platform.rs @@ -227,17 +227,14 @@ impl Platform for LinuxPlatform

{ log::info!("Restarting process, using app path: {:?}", app_path); // Script to wait for the current process to exit and then restart the app. - let script = format!( - r#" - while kill -0 {pid} 2>/dev/null; do + // Pass dynamic values as positional parameters to avoid shell interpolation issues. + let script = r#" + while kill -0 "$0" 2>/dev/null; do sleep 0.1 done - {app_path} - "#, - pid = app_pid, - app_path = app_path.display() - ); + "$1" + "#; #[allow( clippy::disallowed_methods, @@ -247,6 +244,8 @@ impl Platform for LinuxPlatform

{ .arg("bash") .arg("-c") .arg(script) + .arg(&app_pid) + .arg(&app_path) .process_group(0) .spawn(); diff --git a/crates/remote/Cargo.toml b/crates/remote/Cargo.toml index 50026904a8f1ae9bf1954b8c41383487f59a001b..c08561954ebc0ba47a7bf1ab58092275161679a0 100644 --- a/crates/remote/Cargo.toml +++ b/crates/remote/Cargo.toml @@ -48,3 +48,4 @@ which.workspace = true [dev-dependencies] gpui = { workspace = true, features = ["test-support"] } fs = { workspace = true, features = ["test-support"] } +util = { workspace = true, features = ["test-support"] } diff --git a/crates/remote/src/transport/docker.rs b/crates/remote/src/transport/docker.rs index 1bcf80880ab17ddea63bd56fb54acfddc48db2dd..74076b58e35bd1ea7759927bad255925e7f7d9b9 100644 --- a/crates/remote/src/transport/docker.rs +++ b/crates/remote/src/transport/docker.rs @@ -635,7 +635,7 @@ impl RemoteConnection for DockerExecConnection { for env_var in ["RUST_LOG", "RUST_BACKTRACE", "ZED_GENERATE_MINIDUMPS"] { if let Some(value) = std::env::var(env_var).ok() { docker_args.push("-e".to_string()); - docker_args.push(format!("{}='{}'", env_var, value)); + docker_args.push(format!("{env_var}={value}")); } } diff --git a/crates/remote/src/transport/ssh.rs b/crates/remote/src/transport/ssh.rs index 83733306e7a1c96209f12158263f719c22abf54c..63dab3cc9bdc736d156e5222c24968480a460a10 100644 --- a/crates/remote/src/transport/ssh.rs +++ b/crates/remote/src/transport/ssh.rs @@ -450,7 +450,7 @@ impl RemoteConnection for SshRemoteConnection { let mut proxy_args = vec![]; for env_var in VARS { if let Some(value) = std::env::var(env_var).ok() { - proxy_args.push(format!("{}='{}'", env_var, value)); + proxy_args.push(format!("{env_var}={value}")); } } proxy_args.push(remote_binary_path.display(self.path_style()).into_owned()); @@ -1612,12 +1612,11 @@ fn build_command_posix( write!(exec, "exec env ")?; for (k, v) in input_env.iter() { - write!( - exec, - "{}={} ", - k, - ssh_shell_kind.try_quote(v).context("shell quoting")? - )?; + let assignment = format!("{k}={v}"); + let assignment = ssh_shell_kind + .try_quote(&assignment) + .context("shell quoting")?; + write!(exec, "{assignment} ")?; } if let Some(input_program) = input_program { @@ -1818,7 +1817,7 @@ mod tests { "-q", "-t", "user@host", - "cd \"$HOME/work\" && exec env INPUT_VA=val remote_program arg1 arg2" + "cd \"$HOME/work\" && exec env 'INPUT_VA=val' remote_program arg1 arg2" ] ); assert_eq!(command.env, env); @@ -1854,7 +1853,7 @@ mod tests { "-q", "-t", "user@host", - "cd && exec env INPUT_VA=val /bin/fish -l" + "cd && exec env 'INPUT_VA=val' /bin/fish -l" ] ); assert_eq!(command.env, env); @@ -1862,6 +1861,38 @@ mod tests { Ok(()) } + #[test] + fn test_build_command_quotes_env_assignment() -> Result<()> { + let mut input_env = HashMap::default(); + input_env.insert("ZED$(echo foo)".to_string(), "value".to_string()); + + let command = build_command_posix( + Some("remote_program".to_string()), + &[], + &input_env, + None, + None, + HashMap::default(), + PathStyle::Posix, + "/bin/bash", + ShellKind::Posix, + vec![], + "user@host", + Interactive::No, + )?; + + let remote_command = command + .args + .last() + .context("missing remote command argument")?; + assert!( + remote_command.contains("exec env 'ZED$(echo foo)=value' remote_program"), + "expected env assignment to be quoted, got: {remote_command}" + ); + + Ok(()) + } + #[test] fn scp_args_exclude_port_forward_flags() { let options = SshConnectionOptions { diff --git a/crates/remote/src/transport/wsl.rs b/crates/remote/src/transport/wsl.rs index 2eb2aea59abdbe24a3dae168d4399aaa59a9c6e3..5a37e1c65bfe11221b60499779c57f0ce7dca364 100644 --- a/crates/remote/src/transport/wsl.rs +++ b/crates/remote/src/transport/wsl.rs @@ -450,13 +450,10 @@ impl RemoteConnection for WslRemoteConnection { let mut exec = String::from("exec env "); - for (k, v) in env.iter() { - write!( - exec, - "{}={} ", - k, - shell_kind.try_quote(v).context("shell quoting")? - )?; + for (key, value) in env.iter() { + let assignment = format!("{key}={value}"); + let assignment = shell_kind.try_quote(&assignment).context("shell quoting")?; + write!(exec, "{assignment} ")?; } if let Some(program) = program { diff --git a/crates/repl/Cargo.toml b/crates/repl/Cargo.toml index 7bf63657bdea126d7a3f77681e587521356f9eb1..c2d6f745d9272651bd90bcdfdc689263958b8b09 100644 --- a/crates/repl/Cargo.toml +++ b/crates/repl/Cargo.toml @@ -47,6 +47,7 @@ runtimelib.workspace = true serde.workspace = true serde_json.workspace = true settings.workspace = true +shlex.workspace = true smol.workspace = true telemetry.workspace = true terminal.workspace = true diff --git a/crates/repl/src/kernels/wsl_kernel.rs b/crates/repl/src/kernels/wsl_kernel.rs index 1cdb774008d6a40e57b0abeeec73e294896c221a..34340c74feeb76cc4822a6ca5d669693cc448334 100644 --- a/crates/repl/src/kernels/wsl_kernel.rs +++ b/crates/repl/src/kernels/wsl_kernel.rs @@ -21,6 +21,7 @@ use std::{ path::PathBuf, sync::Arc, }; + use uuid::Uuid; // Find a set of open ports. This creates a listener with port set to 0. The listener will be closed at the end when it goes out of scope. @@ -56,6 +57,15 @@ impl Debug for WslRunningKernel { } } +fn quote_posix_shell_arguments(arguments: &[String]) -> Result { + let mut quoted_arguments = Vec::with_capacity(arguments.len()); + for argument in arguments { + let quoted = shlex::try_quote(argument).map(|quoted| quoted.into_owned())?; + quoted_arguments.push(quoted); + } + Ok(quoted_arguments.join(" ")) +} + impl WslRunningKernel { pub fn new( kernel_specification: WslKernelSpecification, @@ -129,9 +139,8 @@ impl WslRunningKernel { // `wsl -d --exec ...` // But we need to replace {connection_file} with wsl_connection_path. - let argv = kernel_specification.kernelspec.argv; anyhow::ensure!( - !argv.is_empty(), + !kernel_specification.kernelspec.argv.is_empty(), "Empty argv in kernelspec {}", kernel_specification.name ); @@ -182,50 +191,57 @@ impl WslRunningKernel { // We use bash -lc to run in a login shell for proper environment setup let mut kernel_args: Vec = Vec::new(); - if let Some(env) = &kernel_specification.kernelspec.env { - if !env.is_empty() { - kernel_args.push("env".to_string()); - for (k, v) in env { - kernel_args.push(format!("{}={}", k, v)); + let resolved_argv: Vec = kernel_specification + .kernelspec + .argv + .iter() + .map(|arg| { + if arg == "{connection_file}" { + wsl_connection_path.clone() + } else { + arg.clone() } + }) + .collect(); + + let executable = resolved_argv.first().map(String::as_str); + let needs_python_resolution = executable.map_or(false, |executable| { + executable == "python" || executable == "python3" || !executable.starts_with('/') + }); + + let mut env_assignments: Vec = Vec::new(); + if let Some(env) = &kernel_specification.kernelspec.env { + env_assignments.reserve(env.len()); + for (key, value) in env { + let assignment = format!("{key}={value}"); + let assignment = shlex::try_quote(&assignment) + .map(|quoted| quoted.into_owned())?; + env_assignments.push(assignment); } - } - for arg in argv { - if arg == "{connection_file}" { - kernel_args.push(wsl_connection_path.clone()); - } else { - kernel_args.push(arg.clone()); + if !env_assignments.is_empty() { + kernel_args.push("env".to_string()); + kernel_args.extend(env_assignments.iter().cloned()); } } - // because first command is python/python3 we need make sure it's present in the env - let first_cmd = kernel_args.first().map(|arg| { - arg.split_whitespace().next().unwrap_or(arg) - }); - - let needs_python_resolution = first_cmd.map_or(false, |cmd| { - cmd == "python" || cmd == "python3" || !cmd.starts_with('/') - }); + kernel_args.extend(resolved_argv.iter().cloned()); let shell_command = if needs_python_resolution { // 1. Check for .venv/bin/python or .venv/bin/python3 in working directory // 2. Fall back to system python3 or python - let rest_args: Vec = kernel_args.iter().skip(1).cloned().collect(); - let rest_string = rest_args - .iter() - .map(|arg| { - if arg.contains(' ') || arg.contains('\'') || arg.contains('"') { - format!("'{}'", arg.replace('\'', "'\\''")) - } else { - arg.clone() - } - }) - .collect::>() - .join(" "); + let rest_args: Vec = resolved_argv.iter().skip(1).cloned().collect(); + let arg_string = quote_posix_shell_arguments(&rest_args)?; + let set_env_command = if env_assignments.is_empty() { + String::new() + } else { + format!("export {}; ", env_assignments.join(" ")) + }; let cd_command = if let Some(wd) = wsl_working_directory.as_ref() { - format!("cd '{}' && ", wd.replace('\'', "'\\''")) + let quoted_wd = shlex::try_quote(wd) + .map(|quoted| quoted.into_owned())?; + format!("cd {quoted_wd} && ") } else { String::new() }; @@ -233,6 +249,7 @@ impl WslRunningKernel { format!( "set -e; \ + {} \ {} \ echo \"Working directory: $(pwd)\" >&2; \ if [ -x .venv/bin/python ]; then \ @@ -254,20 +271,10 @@ impl WslRunningKernel { echo 'PATH:' \"$PATH\" >&2; \ exit 127; \ fi", - cd_command, rest_string, rest_string, rest_string, rest_string + cd_command, set_env_command, arg_string, arg_string, arg_string, arg_string ) } else { - kernel_args - .iter() - .map(|arg| { - if arg.contains(' ') || arg.contains('\'') || arg.contains('"') { - format!("'{}'", arg.replace('\'', "'\\''")) - } else { - arg.clone() - } - }) - .collect::>() - .join(" ") + quote_posix_shell_arguments(&kernel_args)? }; cmd.arg("bash")