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]