From e3144221151448dd8bd1d10feb12a9c3cff2231d Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 20:03:49 -0600 Subject: [PATCH 01/27] Add OverSsh trait to allow conversion of std::process::Command into a openssh::Session. Signed-off-by: Aalekh Patel --- src/command.rs | 35 +++++++++++++++++++++++++++++++++++ src/error.rs | 5 +++++ src/lib.rs | 1 + 3 files changed, 41 insertions(+) diff --git a/src/command.rs b/src/command.rs index c8651c30b..d3ed9eb5b 100644 --- a/src/command.rs +++ b/src/command.rs @@ -49,6 +49,41 @@ macro_rules! delegate { }}; } +/// If a command is `OverSsh` then it can be executed over an SSH session. +/// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`. +pub trait OverSsh { + /// Given a session, return a command that can be executed over that session. + fn with_session<'session>(&self, session: &'session Session) -> Result, Error>; +} + +impl OverSsh for std::process::Command { + /// Given a session, convert a std::process::Command into an `openssh::Command` + /// that can be executed over that session. + fn with_session<'session>(&self, session: &'session Session) -> Result, Error> { + let program = + self + .get_program() + .to_str() + .ok_or_else(|| Error::InvalidUtf8String(self.get_program().to_os_string()))?; + + let args = + self. + get_args() + .into_iter() + .map( + |s| + s + .to_str() + .ok_or_else(|| Error::InvalidUtf8String(s.to_os_string())) + ) + .collect::, Error>>()?; + + let mut command = session.command(program); + command.args(args); + Ok(command) + } +} + /// A remote process builder, providing fine-grained control over how a new remote process should /// be spawned. /// diff --git a/src/error.rs b/src/error.rs index 037826af5..28857f036 100644 --- a/src/error.rs +++ b/src/error.rs @@ -34,6 +34,11 @@ pub enum Error { #[error("the remote command could not be executed")] Remote(#[source] io::Error), + /// Invalid UTF-8 string when trying to + /// convert a `std::ffi::OsString` to a `String`. + #[error("invalid string")] + InvalidUtf8String(std::ffi::OsString), + /// The connection to the remote host was severed. /// /// Note that for the process impl, this is a best-effort error, and it _may_ instead diff --git a/src/lib.rs b/src/lib.rs index 6f86ba8dd..ff5225488 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -168,6 +168,7 @@ pub use builder::{KnownHosts, SessionBuilder}; mod command; pub use command::Command; +pub use command::OverSsh; mod child; pub use child::RemoteChild; From d9ba8d305dfb8f27e89341c86276f350b39a88e5 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 20:16:40 -0600 Subject: [PATCH 02/27] Add a doc comment and example usage. Signed-off-by: Aalekh Patel --- src/command.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/command.rs b/src/command.rs index d3ed9eb5b..1e6cdc273 100644 --- a/src/command.rs +++ b/src/command.rs @@ -59,6 +59,23 @@ pub trait OverSsh { impl OverSsh for std::process::Command { /// Given a session, convert a std::process::Command into an `openssh::Command` /// that can be executed over that session. + /// ```no_run + /// use std::process::Command; + /// use openssh::{Session, KnownHosts, OverSsh}; + + /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; + /// let ls = + /// Command::new("ls") + /// .arg("-l") + /// .arg("-a") + /// .arg("-h") + /// .with_session(&session) + /// .output() + /// .await?; + /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); + /// + /// + /// ``` fn with_session<'session>(&self, session: &'session Session) -> Result, Error> { let program = self From dd646336ff9d0a9bbc89365e82944f397f0060a5 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 20:30:17 -0600 Subject: [PATCH 03/27] Add tests for OverSsh's with_session Signed-off-by: Aalekh Patel --- tests/openssh.rs | 65 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/tests/openssh.rs b/tests/openssh.rs index df980658d..9efc0ad2a 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -6,7 +6,7 @@ use std::{ net::IpAddr, path::PathBuf, process, - time::Duration, + time::Duration, ffi::OsStr, os::unix::prelude::OsStrExt, }; use tempfile::tempdir; use tokio::{ @@ -292,6 +292,69 @@ async fn stdout() { } } + +#[tokio::test] +#[cfg_attr(not(ci), ignore)] +async fn with_session() { + for session in connects().await { + let mut command = + std::process::Command::new("echo") + .arg("foo") + .with_session(&session) + .expect("Expected valid utf-8 command."); + + let child = command.output().await.unwrap(); + assert_eq!(child.stdout, b"foo\n"); + + let child = session + .command("echo") + .arg("foo") + .raw_arg(">") + .arg("/dev/stderr") + .output() + .await + .unwrap(); + assert!(child.stdout.is_empty()); + + session.close().await.unwrap(); + } +} + + +#[tokio::test] +#[cfg_attr(not(ci), ignore)] +async fn with_session_err() { + + for session in connects().await { + let bad_string = OsStr::from_bytes(b"foo\xFF"); + let command = + std::process::Command::new("echo") + .arg(bad_string) + .with_session(&session); + + let expected_error = openssh::Error::InvalidUtf8String(bad_string.to_owned()); + assert!(command.is_err()); + let err = command.unwrap_err(); + assert_eq!(err.to_string(), expected_error.to_string()); + + // let child = command.output().await.unwrap(); + // assert_eq!(child.stdout, b"foo\n"); + + // let child = session + // .command("echo") + // .arg("foo") + // .raw_arg(">") + // .arg("/dev/stderr") + // .output() + // .await + // .unwrap(); + // assert!(child.stdout.is_empty()); + + session.close().await.unwrap(); + } +} + + #[tokio::test] #[cfg_attr(not(ci), ignore)] async fn shell() { From 9743a5e0efaf1ba19d5d87de239ed7eafb23cbd7 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 21:02:38 -0600 Subject: [PATCH 04/27] Use raw_command and raw_args. Refactor the name to 'over_ssh'. Signed-off-by: Aalekh Patel --- src/command.rs | 30 +++++++----------------------- src/error.rs | 5 ----- tests/openssh.rs | 37 +------------------------------------ 3 files changed, 8 insertions(+), 64 deletions(-) diff --git a/src/command.rs b/src/command.rs index 1e6cdc273..86ffbe3fb 100644 --- a/src/command.rs +++ b/src/command.rs @@ -53,7 +53,7 @@ macro_rules! delegate { /// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`. pub trait OverSsh { /// Given a session, return a command that can be executed over that session. - fn with_session<'session>(&self, session: &'session Session) -> Result, Error>; + fn over_session<'session>(&self, session: &'session Session) -> crate::Command<'session>; } impl OverSsh for std::process::Command { @@ -69,35 +69,19 @@ impl OverSsh for std::process::Command { /// .arg("-l") /// .arg("-a") /// .arg("-h") - /// .with_session(&session) + /// .over_session(&session) /// .output() /// .await?; + /// /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); /// /// /// ``` - fn with_session<'session>(&self, session: &'session Session) -> Result, Error> { - let program = - self - .get_program() - .to_str() - .ok_or_else(|| Error::InvalidUtf8String(self.get_program().to_os_string()))?; - - let args = - self. - get_args() - .into_iter() - .map( - |s| - s - .to_str() - .ok_or_else(|| Error::InvalidUtf8String(s.to_os_string())) - ) - .collect::, Error>>()?; + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - let mut command = session.command(program); - command.args(args); - Ok(command) + let mut command = session.raw_command(self.get_program()); + command.raw_args(self.get_args()); + command } } diff --git a/src/error.rs b/src/error.rs index 28857f036..037826af5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -34,11 +34,6 @@ pub enum Error { #[error("the remote command could not be executed")] Remote(#[source] io::Error), - /// Invalid UTF-8 string when trying to - /// convert a `std::ffi::OsString` to a `String`. - #[error("invalid string")] - InvalidUtf8String(std::ffi::OsString), - /// The connection to the remote host was severed. /// /// Note that for the process impl, this is a best-effort error, and it _may_ instead diff --git a/tests/openssh.rs b/tests/openssh.rs index 9efc0ad2a..5c5991a98 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -300,8 +300,7 @@ async fn with_session() { let mut command = std::process::Command::new("echo") .arg("foo") - .with_session(&session) - .expect("Expected valid utf-8 command."); + .over_session(&session); let child = command.output().await.unwrap(); assert_eq!(child.stdout, b"foo\n"); @@ -321,40 +320,6 @@ async fn with_session() { } -#[tokio::test] -#[cfg_attr(not(ci), ignore)] -async fn with_session_err() { - - for session in connects().await { - let bad_string = OsStr::from_bytes(b"foo\xFF"); - let command = - std::process::Command::new("echo") - .arg(bad_string) - .with_session(&session); - - let expected_error = openssh::Error::InvalidUtf8String(bad_string.to_owned()); - assert!(command.is_err()); - let err = command.unwrap_err(); - assert_eq!(err.to_string(), expected_error.to_string()); - - // let child = command.output().await.unwrap(); - // assert_eq!(child.stdout, b"foo\n"); - - // let child = session - // .command("echo") - // .arg("foo") - // .raw_arg(">") - // .arg("/dev/stderr") - // .output() - // .await - // .unwrap(); - // assert!(child.stdout.is_empty()); - - session.close().await.unwrap(); - } -} - - #[tokio::test] #[cfg_attr(not(ci), ignore)] async fn shell() { From 0a69ed11d9a705236f7901cdbc052f1b87f7d786 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 21:05:08 -0600 Subject: [PATCH 05/27] Remove a dangling test import. Use better name for test. Signed-off-by: Aalekh Patel --- tests/openssh.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openssh.rs b/tests/openssh.rs index 5c5991a98..d6a267e42 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -6,7 +6,7 @@ use std::{ net::IpAddr, path::PathBuf, process, - time::Duration, ffi::OsStr, os::unix::prelude::OsStrExt, + time::Duration, }; use tempfile::tempdir; use tokio::{ @@ -295,7 +295,7 @@ async fn stdout() { #[tokio::test] #[cfg_attr(not(ci), ignore)] -async fn with_session() { +async fn over_session() { for session in connects().await { let mut command = std::process::Command::new("echo") From 974cd52da76969a932c3703e39fb532eae6e0847 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 21:06:39 -0600 Subject: [PATCH 06/27] Clean up doc comment. Signed-off-by: Aalekh Patel --- src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/command.rs b/src/command.rs index 86ffbe3fb..36eff4411 100644 --- a/src/command.rs +++ b/src/command.rs @@ -57,7 +57,7 @@ pub trait OverSsh { } impl OverSsh for std::process::Command { - /// Given a session, convert a std::process::Command into an `openssh::Command` + /// Given a session, convert a `std::process::Command` into an `openssh::Command` /// that can be executed over that session. /// ```no_run /// use std::process::Command; From cca22b1ebd3b0bf4b923804a373a25986b2d9bdf Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 22:37:03 -0600 Subject: [PATCH 07/27] Apply suggestions of code review. Signed-off-by: Aalekh Patel --- src/command.rs | 121 ++++++++++++++++++++++++++++++++++++++++++------- src/lib.rs | 3 +- 2 files changed, 106 insertions(+), 18 deletions(-) diff --git a/src/command.rs b/src/command.rs index 36eff4411..54bef0ec9 100644 --- a/src/command.rs +++ b/src/command.rs @@ -53,38 +53,127 @@ macro_rules! delegate { /// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`. pub trait OverSsh { /// Given a session, return a command that can be executed over that session. + /// + /// **Note**: The command to be executed on the remote machine does not include + /// any environment variables or the current directory. They must be + /// set explicitly, if desired. fn over_session<'session>(&self, session: &'session Session) -> crate::Command<'session>; } impl OverSsh for std::process::Command { /// Given a session, convert a `std::process::Command` into an `openssh::Command` /// that can be executed over that session. + /// **Note**: The command to be executed on the remote machine does not include + /// any environment variables or the current directory. They must be + /// set explicitly, if desired. /// ```no_run - /// use std::process::Command; - /// use openssh::{Session, KnownHosts, OverSsh}; - - /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; - /// let ls = - /// Command::new("ls") - /// .arg("-l") - /// .arg("-a") - /// .arg("-h") - /// .over_session(&session) - /// .output() - /// .await?; - /// - /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); + /// # #[tokio::main(flavor = "current_thread")] + /// async fn main() -> Result<(), Box> { + /// use std::process::Command; + /// use openssh::{Session, KnownHosts, OverSsh}; + + /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; + /// let ls = + /// Command::new("ls") + /// .arg("-l") + /// .arg("-a") + /// .arg("-h") + /// .over_session(&session) + /// .output() + /// .await?; /// + /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); + /// # Ok(()) + /// } /// /// ``` fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - - let mut command = session.raw_command(self.get_program()); + let mut command = session.command(self.get_program().to_string_lossy()); command.raw_args(self.get_args()); command } } + +impl OverSsh for tokio::process::Command { + /// Given a session, convert a `tokio::process::Command` into an `openssh::Command` + /// that can be executed over that session. + /// + /// **Note**: The command to be executed on the remote machine does not include + /// any environment variables or the current directory. They must be + /// set explicitly, if desired. + /// ```no_run + /// # #[tokio::main(flavor = "current_thread")] + /// # async fn main() -> Result<(), Box> { + /// use tokio::process::Command; + /// use openssh::{Session, KnownHosts, OverSsh}; + + /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; + /// let ls = + /// Command::new("ls") + /// .arg("-l") + /// .arg("-a") + /// .arg("-h") + /// .over_session(&session) + /// .output() + /// .await?; + /// + /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); + /// # Ok(()) + /// # } + /// + /// ``` + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + self.as_std().over_session(session) + } +} + + +impl OverSsh for &S +where + S: OverSsh +{ + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + ::over_session(self, session) + } +} + +impl OverSsh for &mut S +where + S: OverSsh +{ + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + ::over_session(self, session) + } +} + +impl OverSsh for Box +where + S: OverSsh +{ + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + ::over_session(self, session) + } +} + +impl OverSsh for std::rc::Rc +where + S: OverSsh +{ + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + ::over_session(self, session) + } +} + +impl OverSsh for std::sync::Arc +where + S: OverSsh +{ + fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + ::over_session(self, session) + } +} + /// A remote process builder, providing fine-grained control over how a new remote process should /// be spawned. /// diff --git a/src/lib.rs b/src/lib.rs index ff5225488..8857a09cf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -167,8 +167,7 @@ mod builder; pub use builder::{KnownHosts, SessionBuilder}; mod command; -pub use command::Command; -pub use command::OverSsh; +pub use command::{Command, OverSsh}; mod child; pub use child::RemoteChild; From 66cecbabffdafa86eabb9714a4384a6f86d9285b Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Mon, 30 Jan 2023 23:16:42 -0600 Subject: [PATCH 08/27] Update src/command.rs Co-authored-by: Jiahao XU --- src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/command.rs b/src/command.rs index 54bef0ec9..b8a4cb708 100644 --- a/src/command.rs +++ b/src/command.rs @@ -71,7 +71,7 @@ impl OverSsh for std::process::Command { /// async fn main() -> Result<(), Box> { /// use std::process::Command; /// use openssh::{Session, KnownHosts, OverSsh}; - + /// /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; /// let ls = /// Command::new("ls") From 511c62c7193feb50d937f67b31019cc2bbb1df79 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 00:10:18 -0600 Subject: [PATCH 09/27] Apply more suggestions from the code review. Signed-off-by: Aalekh Patel --- src/command.rs | 67 +++++++++++++---------------------------- src/escape.rs | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ src/lib.rs | 3 ++ 3 files changed, 104 insertions(+), 47 deletions(-) create mode 100644 src/escape.rs diff --git a/src/command.rs b/src/command.rs index b8a4cb708..787702640 100644 --- a/src/command.rs +++ b/src/command.rs @@ -1,3 +1,5 @@ +use crate::escape; + use super::stdio::TryFromChildIo; use super::RemoteChild; use super::Stdio; @@ -5,6 +7,7 @@ use super::{Error, Session}; use std::borrow::Cow; use std::ffi::OsStr; +use std::os::unix::prelude::OsStrExt; use std::process; #[derive(Debug)] @@ -57,15 +60,10 @@ pub trait OverSsh { /// **Note**: The command to be executed on the remote machine does not include /// any environment variables or the current directory. They must be /// set explicitly, if desired. - fn over_session<'session>(&self, session: &'session Session) -> crate::Command<'session>; -} - -impl OverSsh for std::process::Command { - /// Given a session, convert a `std::process::Command` into an `openssh::Command` - /// that can be executed over that session. - /// **Note**: The command to be executed on the remote machine does not include - /// any environment variables or the current directory. They must be - /// set explicitly, if desired. + /// + /// # Example + /// Consider the implementation of `OverSsh` for `std::process::Command`: + /// /// ```no_run /// # #[tokio::main(flavor = "current_thread")] /// async fn main() -> Result<(), Box> { @@ -87,42 +85,26 @@ impl OverSsh for std::process::Command { /// } /// /// ``` + fn over_session<'session>(&self, session: &'session Session) -> crate::Command<'session>; +} + +impl OverSsh for std::process::Command { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - let mut command = session.command(self.get_program().to_string_lossy()); - command.raw_args(self.get_args()); + let program_escaped = escape(self.get_program().as_bytes()); + let mut command = session.command(Cow::Borrowed(program_escaped.as_str())); + + self + .get_args() + .for_each(|arg| { + let arg_escaped = escape(arg.as_bytes()); + command.arg(Cow::Borrowed(arg_escaped.as_str())); + }); command } } impl OverSsh for tokio::process::Command { - /// Given a session, convert a `tokio::process::Command` into an `openssh::Command` - /// that can be executed over that session. - /// - /// **Note**: The command to be executed on the remote machine does not include - /// any environment variables or the current directory. They must be - /// set explicitly, if desired. - /// ```no_run - /// # #[tokio::main(flavor = "current_thread")] - /// # async fn main() -> Result<(), Box> { - /// use tokio::process::Command; - /// use openssh::{Session, KnownHosts, OverSsh}; - - /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; - /// let ls = - /// Command::new("ls") - /// .arg("-l") - /// .arg("-a") - /// .arg("-h") - /// .over_session(&session) - /// .output() - /// .await?; - /// - /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); - /// # Ok(()) - /// # } - /// - /// ``` fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { self.as_std().over_session(session) } @@ -138,15 +120,6 @@ where } } -impl OverSsh for &mut S -where - S: OverSsh -{ - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - ::over_session(self, session) - } -} - impl OverSsh for Box where S: OverSsh diff --git a/src/escape.rs b/src/escape.rs new file mode 100644 index 000000000..455a4fa6d --- /dev/null +++ b/src/escape.rs @@ -0,0 +1,81 @@ +//! Escape characters that may have special meaning in a shell, including spaces. +//! This is a modified version of the [`shell-escape::unix`] module of [`shell-escape`] crate. +//! +//! [`shell-escape`]: https://crates.io/crates/shell-escape +//! [`shell-escape::unix`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 + + +fn whitelisted(ch: char) -> bool { + match ch { + 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_' | '=' | '/' | ',' | '.' | '+' => true, + _ => false, + } +} + +/// Escape characters that may have special meaning in a shell, including spaces. +/// +/// **Note**: This function is an adaptation of [`shell-escape::unix::escape`]. +/// This function exists only for type compatibility and the implementation is +/// almost exactly the same as [`shell-escape::unix::escape`]. +/// +/// [`shell-escape::unix::escape`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 +/// +pub fn escape(s: &[u8]) -> String { + let all_whitelisted = s.iter().all(|x| whitelisted(*x as char)); + + if !s.is_empty() && all_whitelisted { + // All bytes are whitelisted and valid single-byte UTF-8, + // so we can build the original string and return as is. + return String::from_utf8(s.to_vec()).unwrap(); + } + + let mut escaped = String::with_capacity(s.len() + 2); + escaped.push('\''); + + for &b in s { + match b { + b'\'' | b'!' => { + escaped.push_str("'\\"); + escaped.push(b as char); + escaped.push('\''); + } + _ => escaped.push(b as char), + } + } + escaped.push('\''); + escaped +} + + +#[cfg(test)] +mod tests { + use super::*; + + // These tests are courtesy of the `shell-escape` crate. + #[test] + fn test_escape() { + assert_eq!( + escape(b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+"), + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+" + ); + assert_eq!( + escape(b"--aaa=bbb-ccc"), + "--aaa=bbb-ccc" + ); + assert_eq!( + escape(b"linker=gcc -L/foo -Wl,bar"), + r#"'linker=gcc -L/foo -Wl,bar'"# + ); + assert_eq!( + escape(br#"--features="default""#), + r#"'--features="default"'"# + ); + assert_eq!( + escape(br#"'!\$`\\\n "#), + r#"''\'''\!'\$`\\\n '"# + ); + assert_eq!(escape(b""), r#"''"#); + assert_eq!(escape(b" "), r#"' '"#); + assert_eq!(escape(b"\xC4b"), r#"'Äb'"#); + } +} \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index 8857a09cf..354ed4da8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -169,6 +169,9 @@ pub use builder::{KnownHosts, SessionBuilder}; mod command; pub use command::{Command, OverSsh}; +mod escape; +pub use escape::escape; + mod child; pub use child::RemoteChild; From d14a4c2f7b0be281761f7e33f0183c3834b9fa54 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 01:10:51 -0600 Subject: [PATCH 10/27] Apply more suggestions from the code review. Signed-off-by: Aalekh Patel --- src/command.rs | 17 +++++------ src/escape.rs | 77 +++++++++++++++++++++++++++++++------------------- src/lib.rs | 3 +- 3 files changed, 56 insertions(+), 41 deletions(-) diff --git a/src/command.rs b/src/command.rs index 787702640..03b34705a 100644 --- a/src/command.rs +++ b/src/command.rs @@ -1,4 +1,4 @@ -use crate::escape; +use crate::escape::escape; use super::stdio::TryFromChildIo; use super::RemoteChild; @@ -7,7 +7,6 @@ use super::{Error, Session}; use std::borrow::Cow; use std::ffi::OsStr; -use std::os::unix::prelude::OsStrExt; use std::process; #[derive(Debug)] @@ -90,15 +89,13 @@ pub trait OverSsh { impl OverSsh for std::process::Command { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - let program_escaped = escape(self.get_program().as_bytes()); - let mut command = session.command(Cow::Borrowed(program_escaped.as_str())); + let program_escaped: Cow<'_, OsStr> = escape(self.get_program()); + let mut command = session.raw_command(program_escaped); - self - .get_args() - .for_each(|arg| { - let arg_escaped = escape(arg.as_bytes()); - command.arg(Cow::Borrowed(arg_escaped.as_str())); - }); + let args = self + .get_args() + .map(|arg| escape(arg)); + command.raw_args(args); command } } diff --git a/src/escape.rs b/src/escape.rs index 455a4fa6d..d22d18719 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -4,10 +4,17 @@ //! [`shell-escape`]: https://crates.io/crates/shell-escape //! [`shell-escape::unix`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 +use std::{ + borrow::Cow, + ffi::{OsStr, OsString}, + os::unix::ffi::OsStrExt, + os::unix::ffi::OsStringExt, +}; -fn whitelisted(ch: char) -> bool { - match ch { - 'a'..='z' | 'A'..='Z' | '0'..='9' | '-' | '_' | '=' | '/' | ',' | '.' | '+' => true, + +fn whitelisted(byte: u8) -> bool { + match byte { + b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'=' | b'/' | b',' | b'.' | b'+' => true, _ => false, } } @@ -20,30 +27,30 @@ fn whitelisted(ch: char) -> bool { /// /// [`shell-escape::unix::escape`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 /// -pub fn escape(s: &[u8]) -> String { - let all_whitelisted = s.iter().all(|x| whitelisted(*x as char)); +pub fn escape(s: &OsStr) -> Cow<'_, OsStr> { + let s = s.as_bytes(); + let all_whitelisted = s.iter().all(|x| whitelisted(*x)); if !s.is_empty() && all_whitelisted { - // All bytes are whitelisted and valid single-byte UTF-8, - // so we can build the original string and return as is. - return String::from_utf8(s.to_vec()).unwrap(); + return OsString::from_vec(s.to_vec()).into(); } - let mut escaped = String::with_capacity(s.len() + 2); - escaped.push('\''); + let mut escaped = Vec::with_capacity(s.len() + 2); + escaped.push(b'\''); for &b in s { match b { b'\'' | b'!' => { - escaped.push_str("'\\"); - escaped.push(b as char); - escaped.push('\''); + escaped.push(b'\''); + escaped.push(b'\\'); + escaped.push(b); + escaped.push(b'\''); } - _ => escaped.push(b as char), + _ => escaped.push(b), } } - escaped.push('\''); - escaped + escaped.push(b'\''); + OsString::from_vec(escaped).into() } @@ -51,31 +58,43 @@ pub fn escape(s: &[u8]) -> String { mod tests { use super::*; + fn test_escape_case(input: &str, expected: &str) { + let input_os_str = OsStr::from_bytes(input.as_bytes()); + let observed_os_str = escape(input_os_str); + let expected_os_str = OsStr::from_bytes(expected.as_bytes()); + assert_eq!(observed_os_str, expected_os_str); + } + // These tests are courtesy of the `shell-escape` crate. #[test] fn test_escape() { - assert_eq!( - escape(b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+"), + test_escape_case( + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+", "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+" ); - assert_eq!( - escape(b"--aaa=bbb-ccc"), + test_escape_case( + "--aaa=bbb-ccc", "--aaa=bbb-ccc" ); - assert_eq!( - escape(b"linker=gcc -L/foo -Wl,bar"), + test_escape_case( + "linker=gcc -L/foo -Wl,bar", r#"'linker=gcc -L/foo -Wl,bar'"# ); - assert_eq!( - escape(br#"--features="default""#), + test_escape_case( + r#"--features="default""#, r#"'--features="default"'"# ); - assert_eq!( - escape(br#"'!\$`\\\n "#), + test_escape_case( + r#"'!\$`\\\n "#, r#"''\'''\!'\$`\\\n '"# ); - assert_eq!(escape(b""), r#"''"#); - assert_eq!(escape(b" "), r#"' '"#); - assert_eq!(escape(b"\xC4b"), r#"'Äb'"#); + test_escape_case( + "", + r#"''"# + ); + test_escape_case( + " ", + r#"' '"# + ); } } \ No newline at end of file diff --git a/src/lib.rs b/src/lib.rs index 354ed4da8..4ac7eba40 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -169,8 +169,7 @@ pub use builder::{KnownHosts, SessionBuilder}; mod command; pub use command::{Command, OverSsh}; -mod escape; -pub use escape::escape; +pub(crate) mod escape; mod child; pub use child::RemoteChild; From 32a8960aecca8a4011ceb5335991dccda3871e9c Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:03:36 -0600 Subject: [PATCH 11/27] Update src/command.rs Co-authored-by: Jiahao XU --- src/command.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/command.rs b/src/command.rs index 03b34705a..63c89c0e5 100644 --- a/src/command.rs +++ b/src/command.rs @@ -94,7 +94,7 @@ impl OverSsh for std::process::Command { let args = self .get_args() - .map(|arg| escape(arg)); + .map(escape); command.raw_args(args); command } From 08b77a52246638ff59abc94bee777c1dcc9f5ef5 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:03:49 -0600 Subject: [PATCH 12/27] Update src/lib.rs Co-authored-by: Jiahao XU --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 4ac7eba40..7444fced0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -169,7 +169,7 @@ pub use builder::{KnownHosts, SessionBuilder}; mod command; pub use command::{Command, OverSsh}; -pub(crate) mod escape; +mod escape; mod child; pub use child::RemoteChild; From 5d9a112ee92023a369b2f36e6facf11e660da105 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:04:35 -0600 Subject: [PATCH 13/27] Update src/escape.rs Co-authored-by: Jiahao XU --- src/escape.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/escape.rs b/src/escape.rs index d22d18719..acaa840b1 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -29,7 +29,7 @@ fn whitelisted(byte: u8) -> bool { /// pub fn escape(s: &OsStr) -> Cow<'_, OsStr> { let s = s.as_bytes(); - let all_whitelisted = s.iter().all(|x| whitelisted(*x)); + let all_whitelisted = s.iter().copied().all(whitelisted); if !s.is_empty() && all_whitelisted { return OsString::from_vec(s.to_vec()).into(); From 7d84871ab54466d4e7bd9aae2444e237f8057560 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:41:04 -0600 Subject: [PATCH 14/27] Apply more suggestions. Add a test for non-utf8 chars for escape. --- src/escape.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/escape.rs b/src/escape.rs index acaa840b1..eacb822e0 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -32,10 +32,11 @@ pub fn escape(s: &OsStr) -> Cow<'_, OsStr> { let all_whitelisted = s.iter().copied().all(whitelisted); if !s.is_empty() && all_whitelisted { - return OsString::from_vec(s.to_vec()).into(); + return Cow::Borrowed(s); } let mut escaped = Vec::with_capacity(s.len() + 2); + escaped.reserve(4); escaped.push(b'\''); for &b in s { @@ -65,6 +66,13 @@ mod tests { assert_eq!(observed_os_str, expected_os_str); } + fn test_escape_from_bytes(input: &[u8], expected: &[u8]) { + let input_os_str = OsStr::from_bytes(input); + let observed_os_str = escape(input_os_str); + let expected_os_str = OsStr::from_bytes(expected); + assert_eq!(observed_os_str, expected_os_str); + } + // These tests are courtesy of the `shell-escape` crate. #[test] fn test_escape() { @@ -96,5 +104,10 @@ mod tests { " ", r#"' '"# ); + + test_escape_from_bytes( + &[0x66, 0x6f, 0x80, 0x6f], + &[b'\'', 0x66, 0x6f, 0x80, 0x6f, b'\''] + ); } } \ No newline at end of file From c4baafd9f1ce9cced3af5f98008eceb8fe3fac5f Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:47:51 -0600 Subject: [PATCH 15/27] Slight refactor of escape. Signed-off-by: Aalekh Patel --- src/escape.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/escape.rs b/src/escape.rs index eacb822e0..7a319f4e9 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -28,18 +28,18 @@ fn whitelisted(byte: u8) -> bool { /// [`shell-escape::unix::escape`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 /// pub fn escape(s: &OsStr) -> Cow<'_, OsStr> { - let s = s.as_bytes(); - let all_whitelisted = s.iter().copied().all(whitelisted); + let as_bytes = s.as_bytes(); + let all_whitelisted = as_bytes.iter().copied().all(whitelisted); - if !s.is_empty() && all_whitelisted { + if !as_bytes.is_empty() && all_whitelisted { return Cow::Borrowed(s); } - let mut escaped = Vec::with_capacity(s.len() + 2); + let mut escaped = Vec::with_capacity(as_bytes.len() + 2); escaped.reserve(4); escaped.push(b'\''); - for &b in s { + for &b in as_bytes { match b { b'\'' | b'!' => { escaped.push(b'\''); @@ -110,4 +110,4 @@ mod tests { &[b'\'', 0x66, 0x6f, 0x80, 0x6f, b'\''] ); } -} \ No newline at end of file +} From 7e84bebfa56f6237f56f5b54d98520e8e17a6c9e Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:49:54 -0600 Subject: [PATCH 16/27] Run cargo clippy --fix and persist all modifications to escape.rs Signed-off-by: Aalekh Patel --- src/escape.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/escape.rs b/src/escape.rs index 7a319f4e9..99939851a 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -13,10 +13,7 @@ use std::{ fn whitelisted(byte: u8) -> bool { - match byte { - b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'=' | b'/' | b',' | b'.' | b'+' => true, - _ => false, - } + matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'=' | b'/' | b',' | b'.' | b'+') } /// Escape characters that may have special meaning in a shell, including spaces. @@ -27,7 +24,7 @@ fn whitelisted(byte: u8) -> bool { /// /// [`shell-escape::unix::escape`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 /// -pub fn escape(s: &OsStr) -> Cow<'_, OsStr> { +pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> { let as_bytes = s.as_bytes(); let all_whitelisted = as_bytes.iter().copied().all(whitelisted); From 7fcfaf2df293230d23c933f582c455c664ac7449 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 08:51:16 -0600 Subject: [PATCH 17/27] Apply rustfmt to command.rs and escape.rs Signed-off-by: Aalekh Patel --- src/command.rs | 30 +++++++++++++----------------- src/escape.rs | 43 +++++++++++++------------------------------ 2 files changed, 26 insertions(+), 47 deletions(-) diff --git a/src/command.rs b/src/command.rs index 63c89c0e5..df52a5a5f 100644 --- a/src/command.rs +++ b/src/command.rs @@ -55,14 +55,14 @@ macro_rules! delegate { /// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`. pub trait OverSsh { /// Given a session, return a command that can be executed over that session. - /// - /// **Note**: The command to be executed on the remote machine does not include + /// + /// **Note**: The command to be executed on the remote machine does not include /// any environment variables or the current directory. They must be /// set explicitly, if desired. - /// + /// /// # Example /// Consider the implementation of `OverSsh` for `std::process::Command`: - /// + /// /// ```no_run /// # #[tokio::main(flavor = "current_thread")] /// async fn main() -> Result<(), Box> { @@ -70,7 +70,7 @@ pub trait OverSsh { /// use openssh::{Session, KnownHosts, OverSsh}; /// /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; - /// let ls = + /// let ls = /// Command::new("ls") /// .arg("-l") /// .arg("-a") @@ -78,11 +78,11 @@ pub trait OverSsh { /// .over_session(&session) /// .output() /// .await?; - /// + /// /// assert!(String::from_utf8(ls.stdout).unwrap().contains("total")); /// # Ok(()) /// } - /// + /// /// ``` fn over_session<'session>(&self, session: &'session Session) -> crate::Command<'session>; } @@ -91,26 +91,22 @@ impl OverSsh for std::process::Command { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { let program_escaped: Cow<'_, OsStr> = escape(self.get_program()); let mut command = session.raw_command(program_escaped); - - let args = self - .get_args() - .map(escape); + + let args = self.get_args().map(escape); command.raw_args(args); command } } - impl OverSsh for tokio::process::Command { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { self.as_std().over_session(session) } } - impl OverSsh for &S where - S: OverSsh + S: OverSsh, { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { ::over_session(self, session) @@ -119,7 +115,7 @@ where impl OverSsh for Box where - S: OverSsh + S: OverSsh, { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { ::over_session(self, session) @@ -128,7 +124,7 @@ where impl OverSsh for std::rc::Rc where - S: OverSsh + S: OverSsh, { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { ::over_session(self, session) @@ -137,7 +133,7 @@ where impl OverSsh for std::sync::Arc where - S: OverSsh + S: OverSsh, { fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { ::over_session(self, session) diff --git a/src/escape.rs b/src/escape.rs index 99939851a..9c7603dfe 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -1,29 +1,28 @@ //! Escape characters that may have special meaning in a shell, including spaces. //! This is a modified version of the [`shell-escape::unix`] module of [`shell-escape`] crate. -//! +//! //! [`shell-escape`]: https://crates.io/crates/shell-escape //! [`shell-escape::unix`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 use std::{ - borrow::Cow, + borrow::Cow, ffi::{OsStr, OsString}, os::unix::ffi::OsStrExt, os::unix::ffi::OsStringExt, }; - fn whitelisted(byte: u8) -> bool { matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'=' | b'/' | b',' | b'.' | b'+') } /// Escape characters that may have special meaning in a shell, including spaces. -/// +/// /// **Note**: This function is an adaptation of [`shell-escape::unix::escape`]. /// This function exists only for type compatibility and the implementation is /// almost exactly the same as [`shell-escape::unix::escape`]. -/// +/// /// [`shell-escape::unix::escape`]: https://docs.rs/shell-escape/latest/src/shell_escape/lib.rs.html#101 -/// +/// pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> { let as_bytes = s.as_bytes(); let all_whitelisted = as_bytes.iter().copied().all(whitelisted); @@ -51,7 +50,6 @@ pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> { OsString::from_vec(escaped).into() } - #[cfg(test)] mod tests { use super::*; @@ -75,36 +73,21 @@ mod tests { fn test_escape() { test_escape_case( "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+", - "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+" - ); - test_escape_case( - "--aaa=bbb-ccc", - "--aaa=bbb-ccc" + "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_=/,.+", ); + test_escape_case("--aaa=bbb-ccc", "--aaa=bbb-ccc"); test_escape_case( "linker=gcc -L/foo -Wl,bar", - r#"'linker=gcc -L/foo -Wl,bar'"# - ); - test_escape_case( - r#"--features="default""#, - r#"'--features="default"'"# - ); - test_escape_case( - r#"'!\$`\\\n "#, - r#"''\'''\!'\$`\\\n '"# - ); - test_escape_case( - "", - r#"''"# - ); - test_escape_case( - " ", - r#"' '"# + r#"'linker=gcc -L/foo -Wl,bar'"#, ); + test_escape_case(r#"--features="default""#, r#"'--features="default"'"#); + test_escape_case(r#"'!\$`\\\n "#, r#"''\'''\!'\$`\\\n '"#); + test_escape_case("", r#"''"#); + test_escape_case(" ", r#"' '"#); test_escape_from_bytes( &[0x66, 0x6f, 0x80, 0x6f], - &[b'\'', 0x66, 0x6f, 0x80, 0x6f, b'\''] + &[b'\'', 0x66, 0x6f, 0x80, 0x6f, b'\''], ); } } From 13d865a7c057cc0e469d76e0553bcaec1166ddec Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 09:34:21 -0600 Subject: [PATCH 18/27] Lint over_session test Signed-off-by: Aalekh Patel --- tests/openssh.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/openssh.rs b/tests/openssh.rs index d6a267e42..92ea776c6 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -292,13 +292,11 @@ async fn stdout() { } } - #[tokio::test] #[cfg_attr(not(ci), ignore)] async fn over_session() { for session in connects().await { - let mut command = - std::process::Command::new("echo") + let mut command = std::process::Command::new("echo") .arg("foo") .over_session(&session); @@ -319,7 +317,6 @@ async fn over_session() { } } - #[tokio::test] #[cfg_attr(not(ci), ignore)] async fn shell() { From c4ded38673b7de94461ed392752ab34a21f25f44 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 18:47:11 -0600 Subject: [PATCH 19/27] Update src/escape.rs Co-authored-by: Jiahao XU --- src/escape.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/escape.rs b/src/escape.rs index 9c7603dfe..fb6f28066 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -38,6 +38,7 @@ pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> { for &b in as_bytes { match b { b'\'' | b'!' => { + escaped.reserve(4); escaped.push(b'\''); escaped.push(b'\\'); escaped.push(b); From 67d08854f2f29299b5d8af798f54f470104bf185 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 18:47:19 -0600 Subject: [PATCH 20/27] Update src/escape.rs Co-authored-by: Jiahao XU --- src/escape.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/escape.rs b/src/escape.rs index fb6f28066..5bc015796 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -32,7 +32,6 @@ pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> { } let mut escaped = Vec::with_capacity(as_bytes.len() + 2); - escaped.reserve(4); escaped.push(b'\''); for &b in as_bytes { From 69707531ef44cc121111764825d0be6994cd052c Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Tue, 31 Jan 2023 18:47:26 -0600 Subject: [PATCH 21/27] Update src/escape.rs Co-authored-by: Jiahao XU --- src/escape.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/escape.rs b/src/escape.rs index 5bc015796..2c88cbeac 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -55,10 +55,7 @@ mod tests { use super::*; fn test_escape_case(input: &str, expected: &str) { - let input_os_str = OsStr::from_bytes(input.as_bytes()); - let observed_os_str = escape(input_os_str); - let expected_os_str = OsStr::from_bytes(expected.as_bytes()); - assert_eq!(observed_os_str, expected_os_str); + test_escape_from_bytes(input.as_bytes(), expected.as_bytes()) } fn test_escape_from_bytes(input: &[u8], expected: &[u8]) { From 8207907ea5024f84a1dd5aba806839f1da234254 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Sun, 5 Feb 2023 21:25:17 -0600 Subject: [PATCH 22/27] Apply more suggestions. Signed-off-by: Aalekh Patel --- src/command.rs | 91 +++++++++++++++++++++++++++++------------------- src/error.rs | 10 ++++++ src/escape.rs | 7 ++-- tests/openssh.rs | 30 ++++++++++++++-- 4 files changed, 98 insertions(+), 40 deletions(-) diff --git a/src/command.rs b/src/command.rs index df52a5a5f..07fa83d20 100644 --- a/src/command.rs +++ b/src/command.rs @@ -52,16 +52,23 @@ macro_rules! delegate { } /// If a command is `OverSsh` then it can be executed over an SSH session. +/// /// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`. pub trait OverSsh { - /// Given a session, return a command that can be executed over that session. + /// Given an ssh session, return a command that can be executed over that ssh session. /// - /// **Note**: The command to be executed on the remote machine does not include - /// any environment variables or the current directory. They must be - /// set explicitly, if desired. - /// - /// # Example - /// Consider the implementation of `OverSsh` for `std::process::Command`: + /// ### Notes + /// + /// The command to be executed on the remote machine should not explicitly + /// set environment variables or the current working directory. It errors if the source command + /// has environment variables or a current working directory set, since `openssh` doesn't (yet) have + /// a method to set environment variables and `ssh` doesn't support setting a current working directory + /// outside of `bash/dash/zsh` (which is not always available). + /// + /// ### Examples + /// + /// 1. Consider the implementation of `OverSsh` for `std::process::Command`. Let's build a + /// `ls -l -a -h` command and execute it over an SSH session. /// /// ```no_run /// # #[tokio::main(flavor = "current_thread")] @@ -75,7 +82,7 @@ pub trait OverSsh { /// .arg("-l") /// .arg("-a") /// .arg("-h") - /// .over_session(&session) + /// .over_ssh(&session)? /// .output() /// .await?; /// @@ -84,23 +91,54 @@ pub trait OverSsh { /// } /// /// ``` - fn over_session<'session>(&self, session: &'session Session) -> crate::Command<'session>; + /// 2. Building a command with environment variables or a current working directory set will + /// results in an error. + /// + /// ```no_run + /// # #[tokio::main(flavor = "current_thread")] + /// async fn main() -> Result<(), Box> { + /// use std::process::Command; + /// use openssh::{Session, KnownHosts, OverSsh}; + /// + /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; + /// let echo = + /// Command::new("echo") + /// .arg("$MY_ENV_VAR") + /// .over_ssh(&session); + /// assert_matches!(echo, Err(openssh::Error::CommandHasEnv)); + /// + /// # Ok(()) + /// } + /// + /// ``` + fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error>; } impl OverSsh for std::process::Command { - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { + fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + + // I'd really like `!self.get_envs().is_empty()` here, but that's + // behind a `exact_size_is_empty` feature flag. + if self.get_envs().len() > 0 { + return Err(crate::Error::CommandHasEnv); + } + + if self.get_current_dir().is_some() { + return Err(crate::Error::CommandHasCwd); + } + let program_escaped: Cow<'_, OsStr> = escape(self.get_program()); let mut command = session.raw_command(program_escaped); let args = self.get_args().map(escape); command.raw_args(args); - command + Ok(command) } } impl OverSsh for tokio::process::Command { - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - self.as_std().over_session(session) + fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + self.as_std().over_ssh(session) } } @@ -108,37 +146,20 @@ impl OverSsh for &S where S: OverSsh, { - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - ::over_session(self, session) - } -} - -impl OverSsh for Box -where - S: OverSsh, -{ - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - ::over_session(self, session) + fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + ::over_ssh(self, session) } } -impl OverSsh for std::rc::Rc +impl OverSsh for &mut S where S: OverSsh, { - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - ::over_session(self, session) + fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + ::over_ssh(self, session) } } -impl OverSsh for std::sync::Arc -where - S: OverSsh, -{ - fn over_session<'session>(&self, session: &'session Session) -> Command<'session> { - ::over_session(self, session) - } -} /// A remote process builder, providing fine-grained control over how a new remote process should /// be spawned. diff --git a/src/error.rs b/src/error.rs index 037826af5..943021464 100644 --- a/src/error.rs +++ b/src/error.rs @@ -67,6 +67,16 @@ pub enum Error { /// IO Error when creating/reading/writing from ChildStdin, ChildStdout, ChildStderr. #[error("failure while accessing standard i/o of remote process")] ChildIo(#[source] io::Error), + + /// The command has some env variables that it expects to carry over ssh. + /// However, OverSsh does not support passing env variables over ssh. + #[error("rejected runing a command over ssh that expects env variables to be carried over to remote.")] + CommandHasEnv, + + /// The command expects to be in a specific working directory in remote. + /// However, OverSsh does not support setting a working directory for commands to be executed over ssh. + #[error("rejected runing a command over ssh that expects a specific working directory to be carried over to remote.")] + CommandHasCwd, } #[cfg(feature = "native-mux")] diff --git a/src/escape.rs b/src/escape.rs index 2c88cbeac..d0a545f7d 100644 --- a/src/escape.rs +++ b/src/escape.rs @@ -11,7 +11,7 @@ use std::{ os::unix::ffi::OsStringExt, }; -fn whitelisted(byte: u8) -> bool { +fn allowed(byte: u8) -> bool { matches!(byte, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'=' | b'/' | b',' | b'.' | b'+') } @@ -25,9 +25,9 @@ fn whitelisted(byte: u8) -> bool { /// pub(crate) fn escape(s: &OsStr) -> Cow<'_, OsStr> { let as_bytes = s.as_bytes(); - let all_whitelisted = as_bytes.iter().copied().all(whitelisted); + let all_allowed = as_bytes.iter().copied().all(allowed); - if !as_bytes.is_empty() && all_whitelisted { + if !as_bytes.is_empty() && all_allowed { return Cow::Borrowed(s); } @@ -81,6 +81,7 @@ mod tests { test_escape_case(r#"'!\$`\\\n "#, r#"''\'''\!'\$`\\\n '"#); test_escape_case("", r#"''"#); test_escape_case(" ", r#"' '"#); + test_escape_case("*", r#"'*'"#); test_escape_from_bytes( &[0x66, 0x6f, 0x80, 0x6f], diff --git a/tests/openssh.rs b/tests/openssh.rs index 92ea776c6..5bcceaded 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -294,11 +294,11 @@ async fn stdout() { #[tokio::test] #[cfg_attr(not(ci), ignore)] -async fn over_session() { +async fn over_session_ok() { for session in connects().await { let mut command = std::process::Command::new("echo") .arg("foo") - .over_session(&session); + .over_ssh(&session).expect("No env vars or current working dir is set."); let child = command.output().await.unwrap(); assert_eq!(child.stdout, b"foo\n"); @@ -317,6 +317,32 @@ async fn over_session() { } } +/// Test that `over_ssh` errors if the source command has env vars specified. +#[tokio::test] +#[cfg_attr(not(ci), ignore)] +async fn over_session_err_because_env_var() { + for session in connects().await { + let command_with_env = std::process::Command::new("printenv") + .arg("MY_ENV_VAR") + .env("MY_ENV_VAR", "foo") + .over_ssh(&session); + assert!(matches!(command_with_env, Err(openssh::Error::CommandHasEnv))); + } +} + +/// Test that `over_ssh` errors if the source command has a `current_dir` specified. +#[tokio::test] +#[cfg_attr(not(ci), ignore)] +async fn over_session_err_because_cwd() { + for session in connects().await { + let command_with_current_dir = std::process::Command::new("echo") + .arg("foo") + .current_dir("/tmp") + .over_ssh(&session); + assert!(matches!(command_with_current_dir, Err(openssh::Error::CommandHasCwd))); + } +} + #[tokio::test] #[cfg_attr(not(ci), ignore)] async fn shell() { From 10687fb4fc34b6e65c729b05d16d98ca4fda5280 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Sun, 5 Feb 2023 21:27:46 -0600 Subject: [PATCH 23/27] Rustfmt fix. Signed-off-by: Aalekh Patel --- src/command.rs | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/command.rs b/src/command.rs index 07fa83d20..de27b1d51 100644 --- a/src/command.rs +++ b/src/command.rs @@ -52,21 +52,21 @@ macro_rules! delegate { } /// If a command is `OverSsh` then it can be executed over an SSH session. -/// +/// /// Primarily a way to allow `std::process::Command` to be turned directly into an `openssh::Command`. pub trait OverSsh { /// Given an ssh session, return a command that can be executed over that ssh session. /// /// ### Notes - /// + /// /// The command to be executed on the remote machine should not explicitly - /// set environment variables or the current working directory. It errors if the source command + /// set environment variables or the current working directory. It errors if the source command /// has environment variables or a current working directory set, since `openssh` doesn't (yet) have /// a method to set environment variables and `ssh` doesn't support setting a current working directory /// outside of `bash/dash/zsh` (which is not always available). - /// + /// /// ### Examples - /// + /// /// 1. Consider the implementation of `OverSsh` for `std::process::Command`. Let's build a /// `ls -l -a -h` command and execute it over an SSH session. /// @@ -93,7 +93,7 @@ pub trait OverSsh { /// ``` /// 2. Building a command with environment variables or a current working directory set will /// results in an error. - /// + /// /// ```no_run /// # #[tokio::main(flavor = "current_thread")] /// async fn main() -> Result<(), Box> { @@ -111,12 +111,17 @@ pub trait OverSsh { /// } /// /// ``` - fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error>; + fn over_ssh<'session>( + &self, + session: &'session Session, + ) -> Result, crate::Error>; } impl OverSsh for std::process::Command { - fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { - + fn over_ssh<'session>( + &self, + session: &'session Session, + ) -> Result, crate::Error> { // I'd really like `!self.get_envs().is_empty()` here, but that's // behind a `exact_size_is_empty` feature flag. if self.get_envs().len() > 0 { @@ -137,7 +142,10 @@ impl OverSsh for std::process::Command { } impl OverSsh for tokio::process::Command { - fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + fn over_ssh<'session>( + &self, + session: &'session Session, + ) -> Result, crate::Error> { self.as_std().over_ssh(session) } } @@ -146,7 +154,10 @@ impl OverSsh for &S where S: OverSsh, { - fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + fn over_ssh<'session>( + &self, + session: &'session Session, + ) -> Result, crate::Error> { ::over_ssh(self, session) } } @@ -155,12 +166,14 @@ impl OverSsh for &mut S where S: OverSsh, { - fn over_ssh<'session>(&self, session: &'session Session) -> Result, crate::Error> { + fn over_ssh<'session>( + &self, + session: &'session Session, + ) -> Result, crate::Error> { ::over_ssh(self, session) } } - /// A remote process builder, providing fine-grained control over how a new remote process should /// be spawned. /// From 020366ae7f6e148c2a96a70e97e24d0376e0bec4 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Sat, 11 Feb 2023 15:19:57 -0600 Subject: [PATCH 24/27] Fix a doctest for overssh. Signed-off-by: Aalekh Patel --- src/command.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/command.rs b/src/command.rs index de27b1d51..bc821a74f 100644 --- a/src/command.rs +++ b/src/command.rs @@ -103,9 +103,10 @@ pub trait OverSsh { /// let session = Session::connect_mux("me@ssh.example.com", KnownHosts::Strict).await?; /// let echo = /// Command::new("echo") + /// .env("MY_ENV_VAR", "foo") /// .arg("$MY_ENV_VAR") /// .over_ssh(&session); - /// assert_matches!(echo, Err(openssh::Error::CommandHasEnv)); + /// assert!(matches!(echo, Err(openssh::Error::CommandHasEnv))); /// /// # Ok(()) /// } From 6842e5a128f4d6bb527b797ed5247754f7d3cdf1 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Sat, 11 Feb 2023 15:25:32 -0600 Subject: [PATCH 25/27] Add a test for overssh that requires escaping the argument passed to the command. Signed-off-by: Aalekh Patel --- tests/openssh.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/openssh.rs b/tests/openssh.rs index 5bcceaded..708dc2201 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -317,6 +317,32 @@ async fn over_session_ok() { } } + +#[tokio::test] +#[cfg_attr(not(ci), ignore)] +async fn over_session_ok_require_escaping_arguments() { + for session in connects().await { + let mut command = std::process::Command::new("echo") + .arg("\"\'\'foo\'\'\"") + .over_ssh(&session).expect("No env vars or current working dir is set."); + + let child = command.output().await.unwrap(); + assert_eq!(child.stdout, b"\"\'\'foo\'\'\"\n"); + + let child = session + .command("echo") + .arg("foo") + .raw_arg(">") + .arg("/dev/stderr") + .output() + .await + .unwrap(); + assert!(child.stdout.is_empty()); + + session.close().await.unwrap(); + } +} + /// Test that `over_ssh` errors if the source command has env vars specified. #[tokio::test] #[cfg_attr(not(ci), ignore)] From e09d2fde3072ea50987d76ccbe2b943c02b35c85 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Sat, 11 Feb 2023 15:45:41 -0600 Subject: [PATCH 26/27] Update overssh require-escaping-arguments test to contain a space. Signed-off-by: Aalekh Patel --- tests/openssh.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/openssh.rs b/tests/openssh.rs index 708dc2201..51a2e895d 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -323,11 +323,11 @@ async fn over_session_ok() { async fn over_session_ok_require_escaping_arguments() { for session in connects().await { let mut command = std::process::Command::new("echo") - .arg("\"\'\'foo\'\'\"") + .arg("\"\'\' foo \'\'\"") .over_ssh(&session).expect("No env vars or current working dir is set."); let child = command.output().await.unwrap(); - assert_eq!(child.stdout, b"\"\'\'foo\'\'\"\n"); + assert_eq!(child.stdout, b"\"\'\' foo \'\'\"\n"); let child = session .command("echo") From f30d34eff611f05cedd8e74837124c922a0fea94 Mon Sep 17 00:00:00 2001 From: Aalekh Patel Date: Sat, 11 Feb 2023 15:54:47 -0600 Subject: [PATCH 27/27] Apply rustfmt to tests/openssh.rs Signed-off-by: Aalekh Patel --- tests/openssh.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/openssh.rs b/tests/openssh.rs index 51a2e895d..3d3549c5f 100644 --- a/tests/openssh.rs +++ b/tests/openssh.rs @@ -298,7 +298,8 @@ async fn over_session_ok() { for session in connects().await { let mut command = std::process::Command::new("echo") .arg("foo") - .over_ssh(&session).expect("No env vars or current working dir is set."); + .over_ssh(&session) + .expect("No env vars or current working dir is set."); let child = command.output().await.unwrap(); assert_eq!(child.stdout, b"foo\n"); @@ -317,14 +318,14 @@ async fn over_session_ok() { } } - #[tokio::test] #[cfg_attr(not(ci), ignore)] async fn over_session_ok_require_escaping_arguments() { for session in connects().await { let mut command = std::process::Command::new("echo") .arg("\"\'\' foo \'\'\"") - .over_ssh(&session).expect("No env vars or current working dir is set."); + .over_ssh(&session) + .expect("No env vars or current working dir is set."); let child = command.output().await.unwrap(); assert_eq!(child.stdout, b"\"\'\' foo \'\'\"\n"); @@ -352,7 +353,10 @@ async fn over_session_err_because_env_var() { .arg("MY_ENV_VAR") .env("MY_ENV_VAR", "foo") .over_ssh(&session); - assert!(matches!(command_with_env, Err(openssh::Error::CommandHasEnv))); + assert!(matches!( + command_with_env, + Err(openssh::Error::CommandHasEnv) + )); } } @@ -365,7 +369,10 @@ async fn over_session_err_because_cwd() { .arg("foo") .current_dir("/tmp") .over_ssh(&session); - assert!(matches!(command_with_current_dir, Err(openssh::Error::CommandHasCwd))); + assert!(matches!( + command_with_current_dir, + Err(openssh::Error::CommandHasCwd) + )); } }