From c8f9db2e2425c490ffa34aa22d5c9fbcea0836e3 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 3 Nov 2025 11:50:05 +0100 Subject: [PATCH] remote: Fix more quoting issues with nushell (#41547) https://github.com/zed-industries/zed/pull/40084#issuecomment-3464159871 Closes https://github.com/zed-industries/zed/pull/41547 Release Notes: - Fixed remoting not working when the remote has nu set as its shell --- crates/languages/src/python.rs | 2 +- crates/project/src/terminals.rs | 17 ++-- crates/remote/src/transport/ssh.rs | 111 ++++++++++++++++++------ crates/remote/src/transport/wsl.rs | 64 ++++++-------- crates/util/src/shell.rs | 130 +++++++++++++++++++++++++++++ 5 files changed, 246 insertions(+), 78 deletions(-) diff --git a/crates/languages/src/python.rs b/crates/languages/src/python.rs index a87f17795f5b6a1d69368d826688a6ed48309d23..40054abeec298930033cd208c93492cf3354e346 100644 --- a/crates/languages/src/python.rs +++ b/crates/languages/src/python.rs @@ -1211,7 +1211,7 @@ impl ToolchainLister for PythonToolchainProvider { activation_script.extend(match shell { ShellKind::Fish => Some(format!("\"{pyenv}\" shell - fish {version}")), ShellKind::Posix => Some(format!("\"{pyenv}\" shell - sh {version}")), - ShellKind::Nushell => Some(format!("\"{pyenv}\" shell - nu {version}")), + ShellKind::Nushell => Some(format!("^\"{pyenv}\" shell - nu {version}")), ShellKind::PowerShell => None, ShellKind::Csh => None, ShellKind::Tcsh => None, diff --git a/crates/project/src/terminals.rs b/crates/project/src/terminals.rs index 5ea9824916520cfb53673f82f17c1d0e5d31ede3..17564b17dd4d6623d7ca72fadbd0aa8defd1f9cc 100644 --- a/crates/project/src/terminals.rs +++ b/crates/project/src/terminals.rs @@ -8,7 +8,6 @@ use remote::RemoteClient; use settings::{Settings, SettingsLocation}; use smol::channel::bounded; use std::{ - borrow::Cow, path::{Path, PathBuf}, sync::Arc, }; @@ -122,6 +121,7 @@ impl Project { let lang_registry = self.languages.clone(); cx.spawn(async move |project, cx| { let shell_kind = ShellKind::new(&shell, is_windows); + let activation_script = maybe!(async { for toolchain in toolchains { let Some(toolchain) = toolchain.await else { @@ -143,14 +143,8 @@ impl Project { .update(cx, move |_, cx| { let format_to_run = || { if let Some(command) = &spawn_task.command { - let mut command: Option> = shell_kind.try_quote(command); - if let Some(command) = &mut command - && command.starts_with('"') - && let Some(prefix) = shell_kind.command_prefix() - { - *command = Cow::Owned(format!("{prefix}{command}")); - } - + let command = shell_kind.prepend_command_prefix(command); + let command = shell_kind.try_quote_prefix_aware(&command); let args = spawn_task .args .iter() @@ -172,12 +166,13 @@ impl Project { let activation_script = activation_script.join(&format!("{separator} ")); let to_run = format_to_run(); + + let arg = format!("{activation_script}{separator} {to_run}"); + let args = shell_kind.args_for_shell(false, arg); let shell = remote_client .read(cx) .shell() .unwrap_or_else(get_default_system_shell); - let arg = format!("{activation_script}{separator} {to_run}"); - let args = shell_kind.args_for_shell(false, arg); create_remote_shell( Some((&shell, &args)), diff --git a/crates/remote/src/transport/ssh.rs b/crates/remote/src/transport/ssh.rs index 86d93ac2454a41a45d531dd8076066988634e5ce..18a4f64de28d1665deb4c788d7e4673e1e3b9ec5 100644 --- a/crates/remote/src/transport/ssh.rs +++ b/crates/remote/src/transport/ssh.rs @@ -39,6 +39,7 @@ pub(crate) struct SshRemoteConnection { ssh_platform: RemotePlatform, ssh_path_style: PathStyle, ssh_shell: String, + ssh_shell_kind: ShellKind, ssh_default_system_shell: String, _temp_dir: TempDir, } @@ -241,6 +242,7 @@ impl RemoteConnection for SshRemoteConnection { let Self { ssh_path_style, socket, + ssh_shell_kind, ssh_shell, .. } = self; @@ -254,6 +256,7 @@ impl RemoteConnection for SshRemoteConnection { env, *ssh_path_style, ssh_shell, + *ssh_shell_kind, socket.ssh_args(), ) } @@ -367,7 +370,7 @@ impl RemoteConnection for SshRemoteConnection { let ssh_proxy_process = match self .socket - .ssh_command("env", &proxy_args) + .ssh_command(self.ssh_shell_kind, "env", &proxy_args) // IMPORTANT: we kill this process when we drop the task that uses it. .kill_on_drop(true) .spawn() @@ -490,6 +493,13 @@ impl SshRemoteConnection { _ => PathStyle::Posix, }; let ssh_default_system_shell = String::from("/bin/sh"); + let ssh_shell_kind = ShellKind::new( + &ssh_shell, + match ssh_platform.os { + "windows" => true, + _ => false, + }, + ); let mut this = Self { socket, @@ -499,6 +509,7 @@ impl SshRemoteConnection { ssh_path_style, ssh_platform, ssh_shell, + ssh_shell_kind, ssh_default_system_shell, }; @@ -563,7 +574,11 @@ impl SshRemoteConnection { if self .socket - .run_command(&dst_path.display(self.path_style()), &["version"]) + .run_command( + self.ssh_shell_kind, + &dst_path.display(self.path_style()), + &["version"], + ) .await .is_ok() { @@ -632,7 +647,11 @@ impl SshRemoteConnection { ) -> Result<()> { if let Some(parent) = tmp_path_gz.parent() { self.socket - .run_command("mkdir", &["-p", parent.display(self.path_style()).as_ref()]) + .run_command( + self.ssh_shell_kind, + "mkdir", + &["-p", parent.display(self.path_style()).as_ref()], + ) .await?; } @@ -641,6 +660,7 @@ impl SshRemoteConnection { match self .socket .run_command( + self.ssh_shell_kind, "curl", &[ "-f", @@ -660,13 +680,19 @@ impl SshRemoteConnection { { Ok(_) => {} Err(e) => { - if self.socket.run_command("which", &["curl"]).await.is_ok() { + if self + .socket + .run_command(self.ssh_shell_kind, "which", &["curl"]) + .await + .is_ok() + { return Err(e); } match self .socket .run_command( + self.ssh_shell_kind, "wget", &[ "--header=Content-Type: application/json", @@ -681,7 +707,12 @@ impl SshRemoteConnection { { Ok(_) => {} Err(e) => { - if self.socket.run_command("which", &["wget"]).await.is_ok() { + if self + .socket + .run_command(self.ssh_shell_kind, "which", &["wget"]) + .await + .is_ok() + { return Err(e); } else { anyhow::bail!("Neither curl nor wget is available"); @@ -703,7 +734,11 @@ impl SshRemoteConnection { ) -> Result<()> { if let Some(parent) = tmp_path_gz.parent() { self.socket - .run_command("mkdir", &["-p", parent.display(self.path_style()).as_ref()]) + .run_command( + self.ssh_shell_kind, + "mkdir", + &["-p", parent.display(self.path_style()).as_ref()], + ) .await?; } @@ -750,7 +785,7 @@ impl SshRemoteConnection { format!("chmod {server_mode} {orig_tmp_path} && mv {orig_tmp_path} {dst_path}",) }; let args = shell_kind.args_for_shell(false, script.to_string()); - self.socket.run_command("sh", &args).await?; + self.socket.run_command(shell_kind, "sh", &args).await?; Ok(()) } @@ -894,11 +929,16 @@ impl SshSocket { // Furthermore, some setups (e.g. Coder) will change directory when SSH'ing // into a machine. You must use `cd` to get back to $HOME. // You need to do it like this: $ ssh host "cd; sh -c 'ls -l /tmp'" - fn ssh_command(&self, program: &str, args: &[impl AsRef]) -> process::Command { - let shell_kind = ShellKind::Posix; + fn ssh_command( + &self, + shell_kind: ShellKind, + program: &str, + args: &[impl AsRef], + ) -> process::Command { let mut command = util::command::new_smol_command("ssh"); + let program = shell_kind.prepend_command_prefix(program); let mut to_run = shell_kind - .try_quote(program) + .try_quote_prefix_aware(&program) .expect("shell quoting") .into_owned(); for arg in args { @@ -920,8 +960,13 @@ impl SshSocket { command } - async fn run_command(&self, program: &str, args: &[impl AsRef]) -> Result { - let output = self.ssh_command(program, args).output().await?; + async fn run_command( + &self, + shell_kind: ShellKind, + program: &str, + args: &[impl AsRef], + ) -> Result { + let output = self.ssh_command(shell_kind, program, args).output().await?; anyhow::ensure!( output.status.success(), "failed to run command: {}", @@ -994,12 +1039,7 @@ impl SshSocket { } async fn platform(&self, shell: ShellKind) -> Result { - let program = if shell == ShellKind::Nushell { - "^uname" - } else { - "uname" - }; - let uname = self.run_command(program, &["-sm"]).await?; + let uname = self.run_command(shell, "uname", &["-sm"]).await?; let Some((os, arch)) = uname.split_once(" ") else { anyhow::bail!("unknown uname: {uname:?}") }; @@ -1030,7 +1070,10 @@ impl SshSocket { } async fn shell(&self) -> String { - match self.run_command("sh", &["-c", "echo $SHELL"]).await { + match self + .run_command(ShellKind::Posix, "sh", &["-c", "echo $SHELL"]) + .await + { Ok(shell) => shell.trim().to_owned(), Err(e) => { log::error!("Failed to get shell: {e}"); @@ -1256,11 +1299,11 @@ fn build_command( ssh_env: HashMap, ssh_path_style: PathStyle, ssh_shell: &str, + ssh_shell_kind: ShellKind, ssh_args: Vec, ) -> Result { use std::fmt::Write as _; - let shell_kind = ShellKind::new(ssh_shell, false); let mut exec = String::new(); if let Some(working_dir) = working_dir { let working_dir = RemotePathBuf::new(working_dir, ssh_path_style).to_string(); @@ -1270,12 +1313,24 @@ fn build_command( const TILDE_PREFIX: &'static str = "~/"; if working_dir.starts_with(TILDE_PREFIX) { let working_dir = working_dir.trim_start_matches("~").trim_start_matches("/"); - write!(exec, "cd \"$HOME/{working_dir}\" && ",)?; + write!( + exec, + "cd \"$HOME/{working_dir}\" {} ", + ssh_shell_kind.sequential_and_commands_separator() + )?; } else { - write!(exec, "cd \"{working_dir}\" && ",)?; + write!( + exec, + "cd \"{working_dir}\" {} ", + ssh_shell_kind.sequential_and_commands_separator() + )?; } } else { - write!(exec, "cd && ")?; + write!( + exec, + "cd {} ", + ssh_shell_kind.sequential_and_commands_separator() + )?; }; write!(exec, "exec env ")?; @@ -1284,7 +1339,7 @@ fn build_command( exec, "{}={} ", k, - shell_kind.try_quote(v).context("shell quoting")? + ssh_shell_kind.try_quote(v).context("shell quoting")? )?; } @@ -1292,12 +1347,12 @@ fn build_command( write!( exec, "{}", - shell_kind - .try_quote(&input_program) + ssh_shell_kind + .try_quote_prefix_aware(&input_program) .context("shell quoting")? )?; for arg in input_args { - let arg = shell_kind.try_quote(&arg).context("shell quoting")?; + let arg = ssh_shell_kind.try_quote(&arg).context("shell quoting")?; write!(exec, " {}", &arg)?; } } else { @@ -1341,6 +1396,7 @@ mod tests { env.clone(), PathStyle::Posix, "/bin/fish", + ShellKind::Fish, vec!["-p".to_string(), "2222".to_string()], )?; @@ -1370,6 +1426,7 @@ mod tests { env.clone(), PathStyle::Posix, "/bin/fish", + ShellKind::Fish, vec!["-p".to_string(), "2222".to_string()], )?; diff --git a/crates/remote/src/transport/wsl.rs b/crates/remote/src/transport/wsl.rs index e6827347914cc35e266080dab7c83fd182e16a64..1bfa5e640d991f939456418750b633d87cbde3f6 100644 --- a/crates/remote/src/transport/wsl.rs +++ b/crates/remote/src/transport/wsl.rs @@ -44,6 +44,7 @@ pub(crate) struct WslRemoteConnection { remote_binary_path: Option>, platform: RemotePlatform, shell: String, + shell_kind: ShellKind, default_system_shell: String, connection_options: WslConnectionOptions, can_exec: bool, @@ -73,16 +74,17 @@ impl WslRemoteConnection { remote_binary_path: None, platform: RemotePlatform { os: "", arch: "" }, shell: String::new(), + shell_kind: ShellKind::Posix, default_system_shell: String::from("/bin/sh"), can_exec: true, }; delegate.set_status(Some("Detecting WSL environment"), cx); this.shell = this.detect_shell().await?; - let shell = ShellKind::new(&this.shell, false); - this.can_exec = this.detect_can_exec(shell).await?; - this.platform = this.detect_platform(shell).await?; + this.shell_kind = ShellKind::new(&this.shell, false); + this.can_exec = this.detect_can_exec().await?; + this.platform = this.detect_platform().await?; this.remote_binary_path = Some( - this.ensure_server_binary(&delegate, release_channel, version, commit, shell, cx) + this.ensure_server_binary(&delegate, release_channel, version, commit, cx) .await?, ); log::debug!("Detected WSL environment: {this:#?}"); @@ -90,20 +92,16 @@ impl WslRemoteConnection { Ok(this) } - async fn detect_can_exec(&self, shell: ShellKind) -> Result { + async fn detect_can_exec(&self) -> Result { let options = &self.connection_options; - let program = if shell == ShellKind::Nushell { - "^uname" - } else { - "uname" - }; + let program = self.shell_kind.prepend_command_prefix("uname"); let args = &["-m"]; - let output = wsl_command_impl(options, program, args, true) + let output = wsl_command_impl(options, &program, args, true) .output() .await?; if !output.status.success() { - let output = wsl_command_impl(options, program, args, false) + let output = wsl_command_impl(options, &program, args, false) .output() .await?; @@ -120,14 +118,9 @@ impl WslRemoteConnection { Ok(true) } } - async fn detect_platform(&self, shell: ShellKind) -> Result { - let arch_str = if shell == ShellKind::Nushell { - // https://github.com/nushell/nushell/issues/12570 - self.run_wsl_command("sh", &["-c", "uname -m"]) - } else { - self.run_wsl_command("uname", &["-m"]) - } - .await?; + async fn detect_platform(&self) -> Result { + let program = self.shell_kind.prepend_command_prefix("uname"); + let arch_str = self.run_wsl_command(&program, &["-m"]).await?; let arch_str = arch_str.trim().to_string(); let arch = match arch_str.as_str() { "x86_64" => "x86_64", @@ -163,7 +156,6 @@ impl WslRemoteConnection { release_channel: ReleaseChannel, version: SemanticVersion, commit: Option, - shell: ShellKind, cx: &mut AsyncApp, ) -> Result> { let version_str = match release_channel { @@ -186,12 +178,9 @@ impl WslRemoteConnection { if let Some(parent) = dst_path.parent() { let parent = parent.display(PathStyle::Posix); - if shell == ShellKind::Nushell { - self.run_wsl_command("mkdir", &[&parent]).await - } else { - self.run_wsl_command("mkdir", &["-p", &parent]).await - } - .map_err(|e| anyhow!("Failed to create directory: {}", e))?; + self.run_wsl_command("mkdir", &["-p", &parent]) + .await + .map_err(|e| anyhow!("Failed to create directory: {}", e))?; } #[cfg(debug_assertions)] @@ -206,7 +195,7 @@ impl WslRemoteConnection { )) .unwrap(), ); - self.upload_file(&remote_server_path, &tmp_path, delegate, &shell, cx) + self.upload_file(&remote_server_path, &tmp_path, delegate, cx) .await?; self.extract_and_install(&tmp_path, &dst_path, delegate, cx) .await?; @@ -239,8 +228,7 @@ impl WslRemoteConnection { ); let tmp_path = RelPath::unix(&tmp_path).unwrap(); - self.upload_file(&src_path, &tmp_path, delegate, &shell, cx) - .await?; + self.upload_file(&src_path, &tmp_path, delegate, cx).await?; self.extract_and_install(&tmp_path, &dst_path, delegate, cx) .await?; @@ -252,19 +240,15 @@ impl WslRemoteConnection { src_path: &Path, dst_path: &RelPath, delegate: &Arc, - shell: &ShellKind, cx: &mut AsyncApp, ) -> Result<()> { delegate.set_status(Some("Uploading remote server to WSL"), cx); if let Some(parent) = dst_path.parent() { let parent = parent.display(PathStyle::Posix); - if *shell == ShellKind::Nushell { - self.run_wsl_command("mkdir", &[&parent]).await - } else { - self.run_wsl_command("mkdir", &["-p", &parent]).await - } - .map_err(|e| anyhow!("Failed to create directory when uploading file: {}", e))?; + self.run_wsl_command("mkdir", &["-p", &parent]) + .await + .map_err(|e| anyhow!("Failed to create directory when uploading file: {}", e))?; } let t0 = Instant::now(); @@ -441,7 +425,7 @@ impl RemoteConnection for WslRemoteConnection { bail!("WSL shares the network interface with the host system"); } - let shell_kind = ShellKind::new(&self.shell, false); + let shell_kind = self.shell_kind; let working_dir = working_dir .map(|working_dir| RemotePathBuf::new(working_dir, PathStyle::Posix).to_string()) .unwrap_or("~".to_string()); @@ -461,7 +445,9 @@ impl RemoteConnection for WslRemoteConnection { write!( exec, "{}", - shell_kind.try_quote(&program).context("shell quoting")? + shell_kind + .try_quote_prefix_aware(&program) + .context("shell quoting")? )?; for arg in args { let arg = shell_kind.try_quote(&arg).context("shell quoting")?; diff --git a/crates/util/src/shell.rs b/crates/util/src/shell.rs index 7ab214d5105fb81c930954a1aaf9c4aa6fb865c5..e2da1c394b7d151a9ac4c7059c7d4f25e0d5fea5 100644 --- a/crates/util/src/shell.rs +++ b/crates/util/src/shell.rs @@ -408,6 +408,15 @@ impl ShellKind { } } + pub fn prepend_command_prefix<'a>(&self, command: &'a str) -> Cow<'a, str> { + match self.command_prefix() { + Some(prefix) if !command.starts_with(prefix) => { + Cow::Owned(format!("{prefix}{command}")) + } + _ => Cow::Borrowed(command), + } + } + pub const fn sequential_commands_separator(&self) -> char { match self { ShellKind::Cmd => '&', @@ -422,6 +431,20 @@ impl ShellKind { } } + pub const fn sequential_and_commands_separator(&self) -> &'static str { + match self { + ShellKind::Cmd + | ShellKind::Posix + | ShellKind::Csh + | ShellKind::Tcsh + | ShellKind::Rc + | ShellKind::Fish + | ShellKind::PowerShell + | ShellKind::Xonsh => "&&", + ShellKind::Nushell => ";", + } + } + pub fn try_quote<'a>(&self, arg: &'a str) -> Option> { shlex::try_quote(arg).ok().map(|arg| match self { // If we are running in PowerShell, we want to take extra care when escaping strings. @@ -438,6 +461,42 @@ impl ShellKind { }) } + /// Quotes the given argument if necessary, taking into account the command prefix. + /// + /// In other words, this will consider quoting arg without its command prefix to not break the command. + /// You should use this over `try_quote` when you want to quote a shell command. + pub fn try_quote_prefix_aware<'a>(&self, arg: &'a str) -> Option> { + if let Some(char) = self.command_prefix() { + if let Some(arg) = arg.strip_prefix(char) { + // we have a command that is prefixed + for quote in ['\'', '"'] { + if let Some(arg) = arg + .strip_prefix(quote) + .and_then(|arg| arg.strip_suffix(quote)) + { + // and the command itself is wrapped as a literal, that + // means the prefix exists to interpret a literal as a + // command. So strip the quotes, quote the command, and + // re-add the quotes if they are missing after requoting + let quoted = self.try_quote(arg)?; + return Some(if quoted.starts_with(['\'', '"']) { + Cow::Owned(self.prepend_command_prefix("ed).into_owned()) + } else { + Cow::Owned( + self.prepend_command_prefix(&format!("{quote}{quoted}{quote}")) + .into_owned(), + ) + }); + } + } + return self + .try_quote(arg) + .map(|quoted| Cow::Owned(self.prepend_command_prefix("ed).into_owned())); + } + } + self.try_quote(arg) + } + pub fn split(&self, input: &str) -> Option> { shlex::split(input) } @@ -525,4 +584,75 @@ mod tests { "\"C:\\Users\\johndoe\\dev\\python\\39007\\tests\\.venv\\Scripts\\python.exe -m pytest \\\"test_foo.py::test_foo\\\"\"".to_string() ); } + + #[test] + fn test_try_quote_nu_command() { + let shell_kind = ShellKind::Nushell; + assert_eq!( + shell_kind.try_quote("'uname'").unwrap().into_owned(), + "\"'uname'\"".to_string() + ); + assert_eq!( + shell_kind + .try_quote_prefix_aware("'uname'") + .unwrap() + .into_owned(), + "\"'uname'\"".to_string() + ); + assert_eq!( + shell_kind.try_quote("^uname").unwrap().into_owned(), + "'^uname'".to_string() + ); + assert_eq!( + shell_kind + .try_quote_prefix_aware("^uname") + .unwrap() + .into_owned(), + "^uname".to_string() + ); + assert_eq!( + shell_kind.try_quote("^'uname'").unwrap().into_owned(), + "'^'\"'uname\'\"".to_string() + ); + assert_eq!( + shell_kind + .try_quote_prefix_aware("^'uname'") + .unwrap() + .into_owned(), + "^'uname'".to_string() + ); + assert_eq!( + shell_kind.try_quote("'uname a'").unwrap().into_owned(), + "\"'uname a'\"".to_string() + ); + assert_eq!( + shell_kind + .try_quote_prefix_aware("'uname a'") + .unwrap() + .into_owned(), + "\"'uname a'\"".to_string() + ); + assert_eq!( + shell_kind.try_quote("^'uname a'").unwrap().into_owned(), + "'^'\"'uname a'\"".to_string() + ); + assert_eq!( + shell_kind + .try_quote_prefix_aware("^'uname a'") + .unwrap() + .into_owned(), + "^'uname a'".to_string() + ); + assert_eq!( + shell_kind.try_quote("uname").unwrap().into_owned(), + "uname".to_string() + ); + assert_eq!( + shell_kind + .try_quote_prefix_aware("uname") + .unwrap() + .into_owned(), + "uname".to_string() + ); + } }