fix: introduce ShellQuoted to avoid shell quoting issues (#56)

There was a shell quoting issue here:
https://github.com/pnpm/pn/blob/9c9654ac4e6501386e6f0782f6ee24d7aa8dad95/src/main.rs#L112

Where `pn echo ';ls'` would end up running `ls`.

This PR introduces `ShellQuoted` that takes care of quoting, and changes
functions that end up calling `sh -c ...` to use that as parameter, to
reduce the chance of reintroducing this kind of issue.

Another option is to avoid calling `sh -c ...` and instead use
https://github.com/denoland/deno_task_shell which will also have the
benefit of being more consistent across operating systems.

---------

Co-authored-by: khai96_ <[email protected]>

src/error.rs

@@ -1,3 +1,4 @@
+use crate::shell_quoted::ShellQuoted;
 use derive_more::{Display, From};
 use std::{env::JoinPathsError, io, num::NonZeroI32, path::PathBuf};
 
@@ -13,8 +14,8 @@ pub enum PnError {
     ScriptError { name: String, status: NonZeroI32 },
 
     /// Subprocess finishes but without a status code.
-    #[display(fmt = "Command {command:?} has ended unexpectedly")]
-    UnexpectedTermination { command: String },
+    #[display(fmt = "Command ended unexpectedly: {command}")]
+    UnexpectedTermination { command: ShellQuoted },
 
     /// Fail to spawn a subprocess.
     #[display(fmt = "Failed to spawn process: {_0}")]

src/main.rs

@@ -1,14 +1,11 @@
 use clap::Parser;
 use cli::Cli;
 use error::{MainError, PnError};
-use format_buf::format as format_buf;
 use indexmap::IndexMap;
-use itertools::Itertools;
-use os_display::Quoted;
 use pipe_trait::Pipe;
 use serde::{Deserialize, Serialize};
+use shell_quoted::ShellQuoted;
 use std::{
-    borrow::Cow,
     env,
     ffi::OsString,
     fs::File,
@@ -22,6 +19,7 @@ use yansi::Color::{Black, Red};
 mod cli;
 mod error;
 mod passed_through;
+mod shell_quoted;
 mod workspace;
 
 /// Structure of `package.json`.
@@ -64,25 +62,26 @@ fn run() -> Result<(), MainError> {
         let manifest = read_package_manifest(&manifest_path)?;
         Ok((cwd, manifest))
     };
-    let print_and_run_script = |manifest: &NodeManifest, name: &str, command: &str, cwd: &Path| {
-        eprintln!(
-            "\n> {name}@{version} {cwd}",
-            name = &manifest.name,
-            version = &manifest.version,
-            cwd = dunce::canonicalize(cwd)
-                .unwrap_or_else(|_| cwd.to_path_buf())
-                .display(),
-        );
-        eprintln!("> {command}\n");
-        run_script(name, command, cwd)
-    };
+    let print_and_run_script =
+        |manifest: &NodeManifest, name: &str, command: ShellQuoted, cwd: &Path| {
+            eprintln!(
+                "\n> {name}@{version} {cwd}",
+                name = &manifest.name,
+                version = &manifest.version,
+                cwd = dunce::canonicalize(cwd)
+                    .unwrap_or_else(|_| cwd.to_path_buf())
+                    .display(),
+            );
+            eprintln!("> {command}\n");
+            run_script(name, command, cwd)
+        };
     match cli.command {
         cli::Command::Run(args) => {
             let (cwd, manifest) = cwd_and_manifest()?;
             if let Some(name) = args.script {
                 if let Some(command) = manifest.scripts.get(&name) {
-                    let command = append_args(command, &args.args);
-                    print_and_run_script(&manifest, &name, &command, &cwd)
+                    let command = ShellQuoted::from_command_and_args(command.into(), &args.args);
+                    print_and_run_script(&manifest, &name, command, &cwd)
                 } else {
                     PnError::MissingScript { name }
                         .pipe(MainError::Pn)
@@ -105,22 +104,22 @@ fn run() -> Result<(), MainError> {
                     return pass_to_pnpm(&args); // args already contain name, no need to prepend
                 }
                 if let Some(command) = manifest.scripts.get(name) {
-                    let command = append_args(command, &args[1..]);
-                    return print_and_run_script(&manifest, name, &command, &cwd);
+                    let command = ShellQuoted::from_command_and_args(command.into(), &args[1..]);
+                    return print_and_run_script(&manifest, name, command, &cwd);
                 }
             }
-            pass_to_sub(args.join(" "))
+            pass_to_sub(ShellQuoted::from_args(args))
         }
     }
 }
 
-fn run_script(name: &str, command: &str, cwd: &Path) -> Result<(), MainError> {
+fn run_script(name: &str, command: ShellQuoted, cwd: &Path) -> Result<(), MainError> {
     let path_env = create_path_env()?;
     let status = Command::new("sh")
         .current_dir(cwd)
         .env("PATH", path_env)
         .arg("-c")
-        .arg(command)
+        .arg(&command)
         .stdin(Stdio::inherit())
         .stdout(Stdio::inherit())
         .stderr(Stdio::inherit())
@@ -136,26 +135,12 @@ fn run_script(name: &str, command: &str, cwd: &Path) -> Result<(), MainError> {
             name: name.to_string(),
             status,
         },
-        None => PnError::UnexpectedTermination {
-            command: command.to_string(),
-        },
+        None => PnError::UnexpectedTermination { command },
     }
     .pipe(MainError::Pn)
     .pipe(Err)
 }
 
-fn append_args<'a>(command: &'a str, args: &[String]) -> Cow<'a, str> {
-    if args.is_empty() {
-        return Cow::Borrowed(command);
-    }
-    let mut command = command.to_string();
-    for arg in args {
-        let quoted = Quoted::unix(arg); // because pn uses `sh -c` even on Windows
-        format_buf!(command, " {quoted}");
-    }
-    Cow::Owned(command)
-}
-
 fn list_scripts(
     mut stdout: impl Write,
     script_map: impl IntoIterator<Item = (String, String)>,
@@ -184,12 +169,12 @@ fn pass_to_pnpm(args: &[String]) -> Result<(), MainError> {
         Some(None) => return Ok(()),
         Some(Some(status)) => MainError::Sub(status),
         None => MainError::Pn(PnError::UnexpectedTermination {
-            command: format!("pnpm {}", args.iter().join(" ")),
+            command: ShellQuoted::from_command_and_args("pnpm".into(), args),
         }),
     })
 }
 
-fn pass_to_sub(command: String) -> Result<(), MainError> {
+fn pass_to_sub(command: ShellQuoted) -> Result<(), MainError> {
     let path_env = create_path_env()?;
     let status = Command::new("sh")
         .env("PATH", path_env)
@@ -250,29 +235,6 @@ mod tests {
     use pretty_assertions::assert_eq;
     use serde_json::json;
 
-    #[test]
-    fn test_append_args_empty() {
-        let command = append_args("echo hello world", &[]);
-        dbg!(&command);
-        assert!(matches!(&command, Cow::Borrowed(_)));
-    }
-
-    #[test]
-    fn test_append_args_non_empty() {
-        let command = append_args(
-            "echo hello world",
-            &[
-                "abc".to_string(),
-                "def".to_string(),
-                "ghi jkl".to_string(),
-                "\"".to_string(),
-            ],
-        );
-        dbg!(&command);
-        assert!(matches!(&command, Cow::Owned(_)));
-        assert_eq!(command, r#"echo hello world 'abc' 'def' 'ghi jkl' '"'"#);
-    }
-
     #[test]
     fn test_list_scripts() {
         let script_map = [

src/shell_quoted.rs

@@ -0,0 +1,66 @@
+use derive_more::{Display, Into};
+use os_display::Quoted;
+use std::ffi::OsStr;
+
+#[derive(Debug, Display, Into)]
+pub struct ShellQuoted(String);
+
+impl AsRef<OsStr> for ShellQuoted {
+    fn as_ref(&self) -> &OsStr {
+        self.0.as_ref()
+    }
+}
+
+impl ShellQuoted {
+    /// `command` will not be quoted
+    pub fn from_command(command: String) -> Self {
+        Self(command)
+    }
+
+    pub fn push_arg<S: AsRef<str>>(&mut self, arg: S) {
+        use std::fmt::Write;
+        if !self.0.is_empty() {
+            self.0.push(' ');
+        }
+        let quoted = Quoted::unix(arg.as_ref()); // because pn uses `sh -c` even on Windows
+        write!(self.0, "{quoted}").expect("string write doesn't panic");
+    }
+
+    pub fn from_command_and_args<Args>(command: String, args: Args) -> Self
+    where
+        Args: IntoIterator,
+        Args::Item: AsRef<str>,
+    {
+        let mut cmd = Self::from_command(command);
+        for arg in args {
+            cmd.push_arg(arg);
+        }
+        cmd
+    }
+
+    pub fn from_args<Args>(args: Args) -> Self
+    where
+        Args: IntoIterator,
+        Args::Item: AsRef<str>,
+    {
+        Self::from_command_and_args(String::default(), args)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use pretty_assertions::assert_eq;
+
+    #[test]
+    fn test_from_command_and_args() {
+        let command = ShellQuoted::from_command_and_args(
+            "echo hello world".into(),
+            ["abc", ";ls /etc", "ghi jkl", "\"", "'"],
+        );
+        assert_eq!(
+            command.to_string(),
+            r#"echo hello world 'abc' ';ls /etc' 'ghi jkl' '"' "'""#
+        );
+    }
+}

tests/test_main.rs

@@ -58,6 +58,15 @@ fn run_script() {
             "\n> [email protected] {}\n> echo hello world '\"me\"'\n\n",
             temp_dir.path().pipe(dunce::canonicalize).unwrap().display(),
         ));
+
+    Command::cargo_bin("pn")
+        .unwrap()
+        .current_dir(&temp_dir)
+        .args(["echo", ";ls"])
+        .assert()
+        .success()
+        .stdout(";ls\n")
+        .stderr("");
 }
 
 #[test]