From 718f1d99ba5fe48cc96edef5cc6901d065031bd3 Mon Sep 17 00:00:00 2001 From: Bernardo Uriarte Date: Thu, 26 Oct 2023 17:33:00 +0200 Subject: [PATCH 01/10] introduce ShellQuoted to avoid shell injection when calling `sh` --- src/main.rs | 87 ++++++++++++++------------------------------- src/shell_quoted.rs | 73 +++++++++++++++++++++++++++++++++++++ tests/test_main.rs | 10 ++++++ 3 files changed, 110 insertions(+), 60 deletions(-) create mode 100644 src/shell_quoted.rs diff --git a/src/main.rs b/src/main.rs index bba81ec..0abdc89 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,14 +1,12 @@ 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 +20,7 @@ use yansi::Color::{Black, Red}; mod cli; mod error; mod passed_through; +mod shell_quoted; mod workspace; /// Structure of `package.json`. @@ -64,25 +63,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 +105,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()) @@ -137,25 +137,13 @@ fn run_script(name: &str, command: &str, cwd: &Path) -> Result<(), MainError> { status, }, None => PnError::UnexpectedTermination { - command: command.to_string(), + command: command.into(), }, } .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, @@ -189,7 +177,7 @@ fn pass_to_pnpm(args: &[String]) -> Result<(), MainError> { }) } -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) @@ -207,7 +195,9 @@ fn pass_to_sub(command: String) -> Result<(), MainError> { Err(match status { Some(None) => return Ok(()), Some(Some(status)) => MainError::Sub(status), - None => MainError::Pn(PnError::UnexpectedTermination { command }), + None => MainError::Pn(PnError::UnexpectedTermination { + command: command.into(), + }), }) } @@ -250,29 +240,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 = [ diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs new file mode 100644 index 0000000..931adda --- /dev/null +++ b/src/shell_quoted.rs @@ -0,0 +1,73 @@ +use os_display::Quoted; +use std::{ + ffi::OsStr, + fmt::{self, Write as _}, +}; + +#[derive(Debug, Clone, Default)] +pub struct ShellQuoted(String); + +impl fmt::Display for ShellQuoted { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(&self.0) + } +} + +impl AsRef for ShellQuoted { + fn as_ref(&self) -> &OsStr { + self.0.as_ref() + } +} + +impl From for String { + fn from(value: ShellQuoted) -> Self { + value.0 + } +} + +impl ShellQuoted { + /// `command` will not be quoted + pub fn from_command(command: String) -> Self { + Self(command) + } + + pub fn push_arg>(&mut self, arg: S) { + let delim = if self.0.is_empty() { "" } else { " " }; + let quoted = Quoted::unix(arg.as_ref()); // because pn uses `sh -c` even on Windows + write!(&mut self.0, "{delim}{quoted}").expect("String write doesn't panic"); + } + + // convenience methods based on usage + + pub fn from_command_and_args, I: IntoIterator>( + command: String, + args: I, + ) -> Self { + let mut cmd = Self::from_command(command); + for arg in args { + cmd.push_arg(arg); + } + cmd + } + + pub fn from_args, I: IntoIterator>(args: I) -> Self { + Self::from_command_and_args(String::default(), args) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[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' '"' "'""# + ); + } +} diff --git a/tests/test_main.rs b/tests/test_main.rs index 34bf2d4..82ce111 100644 --- a/tests/test_main.rs +++ b/tests/test_main.rs @@ -58,6 +58,16 @@ fn run_script() { "\n> test@1.0.0 {}\n> echo hello world '\"me\"'\n\n", temp_dir.path().pipe(dunce::canonicalize).unwrap().display(), )); + + // other command not passthrough + Command::cargo_bin("pn") + .unwrap() + .current_dir(&temp_dir) + .args(["echo", ";ls"]) + .assert() + .success() + .stdout(";ls\n") + .stderr(""); } #[test] From 35c41e7d75f0ce7fb45cc078c2c277c50dcce8ad Mon Sep 17 00:00:00 2001 From: Bernardo Uriarte Date: Thu, 26 Oct 2023 17:36:14 +0200 Subject: [PATCH 02/10] use `ShellQuoted` in error message so that it can be safely copied to a shell --- src/error.rs | 8 ++++++-- src/main.rs | 11 +++-------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/error.rs b/src/error.rs index 1aa5785..242d70e 100644 --- a/src/error.rs +++ b/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,11 @@ 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 { + // using ShellQuoted here so that output can be copy-pasted into shell + command: ShellQuoted, + }, /// Fail to spawn a subprocess. #[display(fmt = "Failed to spawn process: {_0}")] diff --git a/src/main.rs b/src/main.rs index 0abdc89..bc9639e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,6 @@ use clap::Parser; use cli::Cli; use error::{MainError, PnError}; use indexmap::IndexMap; -use itertools::Itertools; use pipe_trait::Pipe; use serde::{Deserialize, Serialize}; use shell_quoted::ShellQuoted; @@ -136,9 +135,7 @@ fn run_script(name: &str, command: ShellQuoted, cwd: &Path) -> Result<(), MainEr name: name.to_string(), status, }, - None => PnError::UnexpectedTermination { - command: command.into(), - }, + None => PnError::UnexpectedTermination { command }, } .pipe(MainError::Pn) .pipe(Err) @@ -172,7 +169,7 @@ 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), }), }) } @@ -195,9 +192,7 @@ fn pass_to_sub(command: ShellQuoted) -> Result<(), MainError> { Err(match status { Some(None) => return Ok(()), Some(Some(status)) => MainError::Sub(status), - None => MainError::Pn(PnError::UnexpectedTermination { - command: command.into(), - }), + None => MainError::Pn(PnError::UnexpectedTermination { command }), }) } From d159cd61b9897f1b2debc6fedd959ccfb15390d6 Mon Sep 17 00:00:00 2001 From: Bernardo Uriarte Date: Wed, 29 Nov 2023 20:57:11 +0100 Subject: [PATCH 03/10] use derive_more --- src/shell_quoted.rs | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index 931adda..aa95a59 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -1,30 +1,16 @@ +use derive_more::{Display, Into}; use os_display::Quoted; -use std::{ - ffi::OsStr, - fmt::{self, Write as _}, -}; +use std::{ffi::OsStr, fmt::Write as _}; -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone, Default, Display, Into)] pub struct ShellQuoted(String); -impl fmt::Display for ShellQuoted { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.0) - } -} - impl AsRef for ShellQuoted { fn as_ref(&self) -> &OsStr { self.0.as_ref() } } -impl From for String { - fn from(value: ShellQuoted) -> Self { - value.0 - } -} - impl ShellQuoted { /// `command` will not be quoted pub fn from_command(command: String) -> Self { From ef2eff528164e9122c2eaf6596c2d39203d18bd6 Mon Sep 17 00:00:00 2001 From: Bernardo Uriarte Date: Wed, 29 Nov 2023 20:58:55 +0100 Subject: [PATCH 04/10] remove comments --- src/error.rs | 5 +---- src/shell_quoted.rs | 2 -- tests/test_main.rs | 1 - 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/error.rs b/src/error.rs index 242d70e..44e2595 100644 --- a/src/error.rs +++ b/src/error.rs @@ -15,10 +15,7 @@ pub enum PnError { /// Subprocess finishes but without a status code. #[display(fmt = "Command ended unexpectedly: {command}")] - UnexpectedTermination { - // using ShellQuoted here so that output can be copy-pasted into shell - command: ShellQuoted, - }, + UnexpectedTermination { command: ShellQuoted }, /// Fail to spawn a subprocess. #[display(fmt = "Failed to spawn process: {_0}")] diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index aa95a59..248bce5 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -23,8 +23,6 @@ impl ShellQuoted { write!(&mut self.0, "{delim}{quoted}").expect("String write doesn't panic"); } - // convenience methods based on usage - pub fn from_command_and_args, I: IntoIterator>( command: String, args: I, diff --git a/tests/test_main.rs b/tests/test_main.rs index 82ce111..a2f2cc4 100644 --- a/tests/test_main.rs +++ b/tests/test_main.rs @@ -59,7 +59,6 @@ fn run_script() { temp_dir.path().pipe(dunce::canonicalize).unwrap().display(), )); - // other command not passthrough Command::cargo_bin("pn") .unwrap() .current_dir(&temp_dir) From 5a0243f10dd9e168954046b4cdab7e310347750a Mon Sep 17 00:00:00 2001 From: Bernardo Uriarte Date: Wed, 29 Nov 2023 21:01:27 +0100 Subject: [PATCH 05/10] code style --- src/shell_quoted.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index 248bce5..6241b2a 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -23,10 +23,11 @@ impl ShellQuoted { write!(&mut self.0, "{delim}{quoted}").expect("String write doesn't panic"); } - pub fn from_command_and_args, I: IntoIterator>( - command: String, - args: I, - ) -> Self { + pub fn from_command_and_args(command: String, args: Args) -> Self + where + Args: IntoIterator, + Args::Item: AsRef, + { let mut cmd = Self::from_command(command); for arg in args { cmd.push_arg(arg); @@ -34,7 +35,11 @@ impl ShellQuoted { cmd } - pub fn from_args, I: IntoIterator>(args: I) -> Self { + pub fn from_args(args: Args) -> Self + where + Args: IntoIterator, + Args::Item: AsRef, + { Self::from_command_and_args(String::default(), args) } } From 160bd0c80f77444f237950eeb7cceac383c796a3 Mon Sep 17 00:00:00 2001 From: Bernardo Uriarte Date: Thu, 30 Nov 2023 19:29:20 +0100 Subject: [PATCH 06/10] remove unused derives --- src/shell_quoted.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index 6241b2a..5eaa2e5 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -2,7 +2,7 @@ use derive_more::{Display, Into}; use os_display::Quoted; use std::{ffi::OsStr, fmt::Write as _}; -#[derive(Debug, Clone, Default, Display, Into)] +#[derive(Debug, Display, Into)] pub struct ShellQuoted(String); impl AsRef for ShellQuoted { From aa09d306078a1c30eeb4ce897bf62d78a0aeeb4e Mon Sep 17 00:00:00 2001 From: khai96_ Date: Fri, 1 Dec 2023 01:40:44 +0700 Subject: [PATCH 07/10] perf: micro optimization --- src/shell_quoted.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index 5eaa2e5..a155a4c 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -18,9 +18,11 @@ impl ShellQuoted { } pub fn push_arg>(&mut self, arg: S) { - let delim = if self.0.is_empty() { "" } else { " " }; + if !self.0.is_empty() { + self.0.push(' '); + } let quoted = Quoted::unix(arg.as_ref()); // because pn uses `sh -c` even on Windows - write!(&mut self.0, "{delim}{quoted}").expect("String write doesn't panic"); + write!(self.0, "{quoted}").expect("string write doesn't panic"); } pub fn from_command_and_args(command: String, args: Args) -> Self From 5fd7e09c2ab6b51282595361fa41cdb0a479822c Mon Sep 17 00:00:00 2001 From: khai96_ Date: Fri, 1 Dec 2023 01:41:19 +0700 Subject: [PATCH 08/10] style: remove underscore --- src/shell_quoted.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index a155a4c..35add46 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -1,6 +1,6 @@ use derive_more::{Display, Into}; use os_display::Quoted; -use std::{ffi::OsStr, fmt::Write as _}; +use std::{ffi::OsStr, fmt::Write}; #[derive(Debug, Display, Into)] pub struct ShellQuoted(String); From 0e60649549233db00b3d142dfd45ca4db33feb9f Mon Sep 17 00:00:00 2001 From: khai96_ Date: Fri, 1 Dec 2023 01:44:36 +0700 Subject: [PATCH 09/10] style: local import --- src/shell_quoted.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index 35add46..4a8ee31 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -1,6 +1,6 @@ use derive_more::{Display, Into}; use os_display::Quoted; -use std::{ffi::OsStr, fmt::Write}; +use std::ffi::OsStr; #[derive(Debug, Display, Into)] pub struct ShellQuoted(String); @@ -18,6 +18,7 @@ impl ShellQuoted { } pub fn push_arg>(&mut self, arg: S) { + use std::fmt::Write; if !self.0.is_empty() { self.0.push(' '); } From 9d90414a033e6e5d5f3591acb11eb83fd8808838 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Fri, 1 Dec 2023 01:45:44 +0700 Subject: [PATCH 10/10] test: use pretty_assertions --- src/shell_quoted.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/shell_quoted.rs b/src/shell_quoted.rs index 4a8ee31..a5ad57e 100644 --- a/src/shell_quoted.rs +++ b/src/shell_quoted.rs @@ -50,6 +50,7 @@ impl ShellQuoted { #[cfg(test)] mod tests { use super::*; + use pretty_assertions::assert_eq; #[test] fn test_from_command_and_args() {