Cargo.lock 🔗
@@ -14240,6 +14240,7 @@ dependencies = [
"serde",
"serde_json",
"settings",
+ "shlex",
"smol",
"telemetry",
"terminal",
Fix a few cases where we weren't escaping shell vars correctly (#50562)
Conrad Irwin created
Release Notes:
- N/AChange summary
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(-)
Detailed changes
crates/gpui_linux/src/linux/platform.rs 🔗
@@ -229,17 +229,14 @@ impl<P: LinuxClient + 'static> Platform for LinuxPlatform<P> {
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,
@@ -249,6 +246,8 @@ impl<P: LinuxClient + 'static> Platform for LinuxPlatform<P> {
.arg("bash")
.arg("-c")
.arg(script)
+ .arg(&app_pid)
+ .arg(&app_path)
.process_group(0)
.spawn();
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"] }
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}"));
}
}
crates/remote/src/transport/ssh.rs 🔗
@@ -463,7 +463,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());
@@ -1666,12 +1666,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 {
@@ -1882,7 +1881,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);
@@ -1918,7 +1917,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);
@@ -1926,6 +1925,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 {
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 {
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
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<String> {
+ 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<S: KernelSession + 'static>(
kernel_specification: WslKernelSpecification,
@@ -129,9 +139,8 @@ impl WslRunningKernel {
// `wsl -d <distro> --exec <argv0> <argv1> ...`
// 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<String> = 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<String> = 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<String> = 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<String> = 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::<Vec<_>>()
- .join(" ");
+ let rest_args: Vec<String> = 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::<Vec<_>>()
- .join(" ")
+ quote_posix_shell_arguments(&kernel_args)?
};
cmd.arg("bash")