Apply more suggestions.

Signed-off-by: Aalekh Patel <[email protected]>

Author: aalekhpatel07

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,61 +91,75 @@ 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<dyn std::error::Error>> {
+    ///     use std::process::Command;
+    ///     use openssh::{Session, KnownHosts, OverSsh};
+    ///
+    ///     let session = Session::connect_mux("[email protected]", 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::Command<'session>, 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<Command<'session>, 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<Command<'session>, crate::Error> {
+        self.as_std().over_ssh(session)
     }
 }
 
 impl<S> OverSsh for &S
 where
     S: OverSsh,
 {
-    fn over_session<'session>(&self, session: &'session Session) -> Command<'session> {
-        <S as OverSsh>::over_session(self, session)
-    }
-}
-
-impl<S> OverSsh for Box<S>
-where
-    S: OverSsh,
-{
-    fn over_session<'session>(&self, session: &'session Session) -> Command<'session> {
-        <S as OverSsh>::over_session(self, session)
+    fn over_ssh<'session>(&self, session: &'session Session) -> Result<Command<'session>, crate::Error> {
+        <S as OverSsh>::over_ssh(self, session)
     }
 }
 
-impl<S> OverSsh for std::rc::Rc<S>
+impl<S> OverSsh for &mut S
 where
     S: OverSsh,
 {
-    fn over_session<'session>(&self, session: &'session Session) -> Command<'session> {
-        <S as OverSsh>::over_session(self, session)
+    fn over_ssh<'session>(&self, session: &'session Session) -> Result<Command<'session>, crate::Error> {
+        <S as OverSsh>::over_ssh(self, session)
     }
 }
 
-impl<S> OverSsh for std::sync::Arc<S>
-where
-    S: OverSsh,
-{
-    fn over_session<'session>(&self, session: &'session Session) -> Command<'session> {
-        <S as OverSsh>::over_session(self, session)
-    }
-}
 
 /// A remote process builder, providing fine-grained control over how a new remote process should
 /// be spawned.

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")]

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],

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() {