Escape arguments by default

Cargo.toml

@@ -19,7 +19,7 @@ maintenance = { status = "experimental" }
 
 [dependencies]
 tempfile = "3.1.0"
-shellwords = "1"
+shell-escape = "0.1"
 tokio = { version = "0.2", features = [ "process", "io-util" ] }
 
 [dev-dependencies]

src/command.rs

@@ -1,5 +1,6 @@
 use super::RemoteChild;
 use super::{Error, Session};
+use std::borrow::Cow;
 use std::ffi::OsStr;
 use std::io;
 use std::process::Stdio;
@@ -60,11 +61,8 @@ impl<'s> Command<'s> {
 impl<'s> Command<'s> {
     /// Adds an argument to pass to the remote program.
     ///
-    /// The argument is passed as written to `ssh`, which will pass it again as an argument to the
-    /// remote shell. However, since the remote shell may do argument parsing, characters such as
-    /// spaces and `*` may be interpreted by the remote shell. Since we do not know what shell the
-    /// remote host is running, we cannot prevent this, so consider escaping your arguments with
-    /// something like [`shellwords`] as necessary.
+    /// Before it is passed to the remote host, `arg` is escaped so that special characters aren't
+    /// evaluated by the remote shell. If you do not want this behavior, use [`raw_arg`].
     ///
     /// Only one argument can be passed per use. So instead of:
     ///
@@ -84,25 +82,52 @@ impl<'s> Command<'s> {
     /// ```
     ///
     /// To pass multiple arguments see [`args`](Command::args).
+    pub fn arg<S: AsRef<str>>(&mut self, arg: S) -> &mut Self {
+        self.raw_arg(&*shell_escape::unix::escape(Cow::Borrowed(arg.as_ref())));
+        self
+    }
+
+    /// Adds an argument to pass to the remote program.
     ///
-    ///   [`shellwords`]: https://crates.io/crates/shellwords
-    pub fn arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Self {
+    /// Unlike [`arg`], this method does not shell-escape `arg`. The argument is passed as written
+    /// to `ssh`, which will pass it again as an argument to the remote shell. Since the remote
+    /// shell may do argument parsing, characters such as spaces and `*` may be interpreted by the
+    /// remote shell.
+    ///
+    /// To pass multiple unescaped arguments see [`raw_args`](Command::raw_args).
+    pub fn raw_arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Self {
         self.builder.arg(arg);
         self
     }
 
     /// Adds multiple arguments to pass to the remote program.
     ///
-    /// The arguments are passed as written to `ssh`, which will pass them again as arguments to
-    /// the remote shell. However, since the remote shell may do argument parsing, characters such
-    /// as spaces and `*` may be interpreted by the remote shell. Since we do not know what shell
-    /// the remote host is running, we cannot prevent this, so consider escaping your arguments
-    /// with something like [`shellwords`] as necessary.
+    /// Before they are passed to the remote host, each argument in `args` is escaped so that
+    /// special characters aren't evaluated by the remote shell. If you do not want this behavior,
+    /// use [`raw_args`].
     ///
     /// To pass a single argument see [`arg`](Command::arg).
-    ///
-    ///   [`shellwords`]: https://crates.io/crates/shellwords
     pub fn args<I, S>(&mut self, args: I) -> &mut Self
+    where
+        I: IntoIterator<Item = S>,
+        S: AsRef<str>,
+    {
+        for arg in args {
+            self.builder
+                .arg(&*shell_escape::unix::escape(Cow::Borrowed(arg.as_ref())));
+        }
+        self
+    }
+
+    /// Adds multiple arguments to pass to the remote program.
+    ///
+    /// Unlike [`args`], this method does not shell-escape `args`. The arguments are passed as
+    /// written to `ssh`, which will pass them again as arguments to the remote shell. However,
+    /// since the remote shell may do argument parsing, characters such as spaces and `*` may be
+    /// interpreted by the remote shell.
+    ///
+    /// To pass a single argument see [`raw_arg`](Command::raw_arg).
+    pub fn raw_args<I, S>(&mut self, args: I) -> &mut Self
     where
         I: IntoIterator<Item = S>,
         S: AsRef<OsStr>,

src/lib.rs

@@ -30,10 +30,7 @@
 //! handle to the _local_ `ssh` instance corresponding to the spawned remote command. The behavior
 //! of the methods of [`RemoteChild`] therefore match the behavior of `ssh`, rather than that of
 //! the remote command directly. Usually, these are the same, though not always, as highlighted in
-//! the documetantation the individual methods. Of particular note is the fact that arguments are
-//! passed to the remote shell, which **may interpret them**. Strings with `$` in them are
-//! particularly subject to this behavior, and you may want to consider using a library like
-//! [`shellwords`] on arguments and paths.
+//! the documetantation the individual methods. See also the section below on Remote Shells.
 //!
 //! And finally, our commands never default to inheriting stdin/stdout/stderr, since we expect you
 //! are using this to automate things. Instead, unless otherwise noted, all I/O ports default to
@@ -65,6 +62,22 @@
 //! master connection is still operational, and _may_ provide you with more information than you
 //! got from the failing command (that is, just [`Error::Disconnected`]) if it is not.
 //!
+//! # Remote Shells
+//!
+//! When you invoke a remote command through ssh, the remote command is executed by a shell on the
+//! remote end. That shell _interprets_ anything passed to it — it might evalute words starting
+//! with `$` as variables, split arguments by whitespace, and other things a shell is wont to do.
+//! Since that is _usually_ not what you expect to happen, `.arg("a b")` should pass a _single_
+//! argument with the value `a b`, `openssh` _escapes_ every argument (and the command itself) by
+//! default using [`shell-escape`]. This works well in most cases, but might run into issues when
+//! the remote shell (generally the remote user's login shell) has a different syntax than the
+//! shell `shell-escape` targets (bash). For example, Windows shells have different escaping syntax
+//! than bash does.
+//!
+//! If this applies to you, you can use [`raw_arg`](Command::raw_arg),
+//! [`raw_args`](Command::raw_args), and [`raw_command`](Session::raw_command) to bypass the
+//! escaping that `openssh` normally does for you.
+//!
 //! # Examples
 //!
 //! ```rust,no_run
@@ -84,7 +97,7 @@
 //! ```
 //!
 //!   [`ControlMaster`]: https://en.wikibooks.org/wiki/OpenSSH/Cookbook/Multiplexing
-//!   [`shellwords`]: https://crates.io/crates/shellwords
+//!   [`shell-escape`]: https://crates.io/crates/shell-escape
 
 #![warn(
     missing_docs,
@@ -94,6 +107,7 @@
     unreachable_pub
 )]
 
+use std::borrow::Cow;
 use std::ffi::OsStr;
 use std::io;
 use tokio::io::AsyncReadExt;
@@ -187,6 +201,29 @@ impl Session {
     /// Constructs a new [`Command`] for launching the program at path `program` on the remote
     /// host.
     ///
+    /// Before it is passed to the remote host, `program` is escaped so that special characters
+    /// aren't evaluated by the remote shell. If you do not want this behavior, use
+    /// [`raw_command`].
+    ///
+    /// The returned `Command` is a builder, with the following default configuration:
+    ///
+    /// * No arguments to the program
+    /// * Empty stdin and dsicard stdout/stderr for `spawn` or `status`, but create output pipes for `output`
+    ///
+    /// Builder methods are provided to change these defaults and otherwise configure the process.
+    ///
+    /// If `program` is not an absolute path, the `PATH` will be searched in an OS-defined way on
+    /// the host.
+    pub fn command<'a, S: Into<Cow<'a, str>>>(&self, program: S) -> Command<'_> {
+        self.raw_command(&*shell_escape::unix::escape(program.into()))
+    }
+
+    /// Constructs a new [`Command`] for launching the program at path `program` on the remote
+    /// host.
+    ///
+    /// Unlike [`command`], this method does not shell-escape `program`, so it may be evaluated in
+    /// unforeseen ways by the remote shell.
+    ///
     /// The returned `Command` is a builder, with the following default configuration:
     ///
     /// * No arguments to the program
@@ -196,7 +233,7 @@ impl Session {
     ///
     /// If `program` is not an absolute path, the `PATH` will be searched in an OS-defined way on
     /// the host.
-    pub fn command<S: AsRef<OsStr>>(&self, program: S) -> Command<'_> {
+    pub fn raw_command<S: AsRef<OsStr>>(&self, program: S) -> Command<'_> {
         // XXX: Should we do a self.check() here first?
 
         // NOTE: we pass -p 9 nine here (the "discard" port) to ensure that ssh does not
@@ -242,23 +279,23 @@ impl Session {
     ///
     /// To counter this, this method assumes that the remote shell (the one launched by `sshd`) is
     /// [POSIX compliant]. This is more or less equivalent to "supports `bash` syntax" if you don't
-    /// look too closely. It uses [`shellwords`] to escape `command` before sending it to the
+    /// look too closely. It uses [`shell-escape`] to escape `command` before sending it to the
     /// remote shell, with the expectation that the remote shell will only end up undoing that one
     /// "level" of escaping, thus producing the original `command` as an argument to `sh`. This
     /// works _most of the time_.
     ///
-    /// With sufficiently complex or weird commands, the escaping of `shellwords` may not fully
+    /// With sufficiently complex or weird commands, the escaping of `shell-escape` may not fully
     /// match the "un-escaping" of the remote shell. This will manifest as escape characters
     /// appearing in the `sh` command that you did not intend to be there. If this happens, try
     /// changing the remote shell if you can, or fall back to [`command`] and do the escaping
     /// manually instead.
     ///
     ///   [POSIX compliant]: https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xcu_chap02.html
     ///   [this article]: https://mywiki.wooledge.org/Arguments
-    ///   [`shellwords`]: https://crates.io/crates/shellwords
+    ///   [`shell-escape`]: https://crates.io/crates/shell-escape
     pub fn shell<S: AsRef<str>>(&self, command: S) -> Command<'_> {
         let mut cmd = self.command("sh");
-        cmd.arg("-c").arg(shellwords::escape(command.as_ref()));
+        cmd.arg("-c").arg(command);
         cmd
     }
 

src/sftp.rs

@@ -178,18 +178,21 @@ impl<'s> Sftp<'s> {
             )));
         };
 
+        // https://github.com/sfackler/shell-escape/issues/5
+        let dir = dir.to_string_lossy();
+
         let test = self
             .session
             .command("test")
-            .arg("'('")
+            .arg("(")
             .arg("-d")
-            .arg(dir)
-            .arg("')'")
+            .arg(&dir)
+            .arg(")")
             .arg("-a")
-            .arg("'('")
+            .arg("(")
             .arg("-w")
-            .arg(dir)
-            .arg("')'")
+            .arg(&dir)
+            .arg(")")
             .output()
             .await?;
 
@@ -199,7 +202,7 @@ impl<'s> Sftp<'s> {
             .session
             .command("test")
             .arg("-e")
-            .arg(dir)
+            .arg(&dir)
             .status()
             .await?
             .success()
@@ -226,14 +229,17 @@ impl<'s> Sftp<'s> {
         path: impl AsRef<std::path::Path>,
     ) -> Result<(), Error> {
         if mode.is_write() {
+            // https://github.com/sfackler/shell-escape/issues/5
+            let path = path.as_ref().to_string_lossy();
+
             // for writes, we want a stronger (and cheaper) test than can()
             // we can't actually use `touch`, since it also works for dirs
             // note that this works b/c stdin is Stdio::null()
             let touch = self
                 .session
                 .command("cat")
-                .arg(">>")
-                .arg(path.as_ref())
+                .raw_arg(">>")
+                .arg(&path)
                 .stdin(Stdio::null())
                 .output()
                 .await?;
@@ -298,6 +304,9 @@ impl<'s> Sftp<'s> {
 
         self.init_op(Mode::Write, path).await?;
 
+        // https://github.com/sfackler/shell-escape/issues/5
+        let path = path.to_string_lossy();
+
         let cat = self
             .session
             .command("tee")
@@ -351,6 +360,9 @@ impl<'s> Sftp<'s> {
 
         self.init_op(Mode::Append, path).await?;
 
+        // https://github.com/sfackler/shell-escape/issues/5
+        let path = path.to_string_lossy();
+
         let cat = self
             .session
             .command("tee")
@@ -404,6 +416,9 @@ impl<'s> Sftp<'s> {
 
         self.init_op(Mode::Read, path).await?;
 
+        // https://github.com/sfackler/shell-escape/issues/5
+        let path = path.to_string_lossy();
+
         let cat = self
             .session
             .command("cat")

tests/openssh.rs

@@ -35,7 +35,7 @@ async fn stdout() {
     let child = session
         .command("echo")
         .arg("foo")
-        .arg(">")
+        .raw_arg(">")
         .arg("/dev/stderr")
         .output()
         .await
@@ -91,7 +91,7 @@ async fn stderr() {
     let child = session
         .command("echo")
         .arg("foo")
-        .arg(">")
+        .raw_arg(">")
         .arg("/dev/stderr")
         .output()
         .await
@@ -366,6 +366,66 @@ async fn spawn_and_wait() {
     assert!(t.elapsed() > Duration::from_secs(1));
     sleeping2.wait_with_output().await.unwrap();
     assert!(t.elapsed() > Duration::from_secs(2));
+
+    session.close().await.unwrap();
+}
+
+#[tokio::test]
+#[cfg_attr(not(ci), ignore)]
+async fn escaping() {
+    let session = Session::connect(&addr(), KnownHosts::Accept).await.unwrap();
+
+    let status = dbg!(session
+        .command("printf")
+        .arg("%d %d")
+        .arg("1")
+        .arg("2")
+        .output()
+        .await
+        .unwrap())
+    .status;
+    assert!(status.success());
+
+    let status = dbg!(session
+        .command("printf")
+        .args(vec!["%d %d", "1", "2"])
+        .output()
+        .await
+        .unwrap())
+    .status;
+    assert!(status.success());
+
+    let status = dbg!(session
+        .command("printf")
+        .arg("%d %d")
+        .raw_arg("1 2")
+        .output()
+        .await
+        .unwrap())
+    .status;
+    assert!(status.success());
+
+    let status = dbg!(session
+        .command("printf")
+        .arg("%d %d")
+        .raw_args(std::iter::once("1 2"))
+        .output()
+        .await
+        .unwrap())
+    .status;
+    assert!(status.success());
+
+    let status = dbg!(session
+        .raw_command("printf '%d %d'")
+        .arg("1")
+        .arg("2")
+        .output()
+        .await
+        .unwrap())
+    .status;
+    assert!(status.success());
+
+    session.close().await.unwrap();
 }
 
 #[tokio::test]
@@ -411,7 +471,12 @@ async fn broken_connection() {
     let sleeping = session.command("sleep").arg("1000").spawn().unwrap();
 
     // get ID of remote ssh process
-    let ppid = session.command("echo").arg("$PPID").output().await.unwrap();
+    let ppid = session
+        .command("echo")
+        .raw_arg("$PPID")
+        .output()
+        .await
+        .unwrap();
     eprintln!("ppid: {:?}", ppid);
     let ppid: u32 = String::from_utf8(ppid.stdout)
         .unwrap()