feat: implement argument escaping when the shell option is set

BREAKING CHANGE: when the `shell` option is set provided arguments will automatically be escaped

README.md

@@ -54,4 +54,9 @@ spawned process.
 - `cwd` String, default `process.cwd()`.  Current working directory for
   running the script.  Also the argument to `infer-owner` to determine
   effective uid/gid when run as root on Unix systems.
+- `shell` Boolean or String. If false, no shell is used during spawn. If true,
+  the system default shell is used. If a String, that specific shell is used.
+  When a shell is used, the given command runs from within that shell by
+  concatenating the command and its escaped arguments and running the result.
+  This option is _not_ passed through to `child_process.spawn`.
 - Any other options for `child_process.spawn` can be passed as well.

lib/escape.js

@@ -0,0 +1,68 @@
+'use strict'
+
+// eslint-disable-next-line max-len
+// this code adapted from: https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
+const cmd = (input, doubleEscape) => {
+  if (!input.length) {
+    return '""'
+  }
+
+  let result
+  if (!/[ \t\n\v"]/.test(input)) {
+    result = input
+  } else {
+    result = '"'
+    for (let i = 0; i <= input.length; ++i) {
+      let slashCount = 0
+      while (input[i] === '\\') {
+        ++i
+        ++slashCount
+      }
+
+      if (i === input.length) {
+        result += '\\'.repeat(slashCount * 2)
+        break
+      }
+
+      if (input[i] === '"') {
+        result += '\\'.repeat(slashCount * 2 + 1)
+        result += input[i]
+      } else {
+        result += '\\'.repeat(slashCount)
+        result += input[i]
+      }
+    }
+    result += '"'
+  }
+
+  // and finally, prefix shell meta chars with a ^
+  result = result.replace(/[ !%^&()<>|"]/g, '^$&')
+  if (doubleEscape) {
+    result = result.replace(/[ !%^&()<>|"]/g, '^$&')
+  }
+
+  return result
+}
+
+const sh = (input) => {
+  if (!input.length) {
+    return `''`
+  }
+
+  if (!/[\t\n\r "#$&'()*;<>?\\`|~]/.test(input)) {
+    return input
+  }
+
+  // replace single quotes with '\'' and wrap the whole result in a fresh set of quotes
+  const result = `'${input.replace(/'/g, `'\\''`)}'`
+    // if the input string already had single quotes around it, clean those up
+    .replace(/^(?:'')+(?!$)/, '')
+    .replace(/\\'''/g, `\\'`)
+
+  return result
+}
+
+module.exports = {
+  cmd,
+  sh,
+}

lib/index.js

@@ -1,32 +1,43 @@
+'use strict'
+
 const { spawn } = require('child_process')
+const which = require('which')
 
-const isPipe = (stdio = 'pipe', fd) =>
-  stdio === 'pipe' || stdio === null ? true
-  : Array.isArray(stdio) ? isPipe(stdio[fd], fd)
-  : false
+const escape = require('./escape.js')
 
 // 'extra' object is for decorating the error a bit more
 const promiseSpawn = (cmd, args, opts = {}, extra = {}) => {
+  if (opts.shell) {
+    return spawnWithShell(cmd, args, opts, extra)
+  }
+
   let proc
+
   const p = new Promise((res, rej) => {
     proc = spawn(cmd, args, opts)
+
     const stdout = []
     const stderr = []
+
     const reject = er => rej(Object.assign(er, {
       cmd,
       args,
       ...stdioResult(stdout, stderr, opts),
       ...extra,
     }))
+
     proc.on('error', reject)
+
     if (proc.stdout) {
       proc.stdout.on('data', c => stdout.push(c)).on('error', reject)
       proc.stdout.on('error', er => reject(er))
     }
+
     if (proc.stderr) {
       proc.stderr.on('data', c => stderr.push(c)).on('error', reject)
       proc.stderr.on('error', er => reject(er))
     }
+
     proc.on('close', (code, signal) => {
       const result = {
         cmd,
@@ -36,6 +47,7 @@ const promiseSpawn = (cmd, args, opts = {}, extra = {}) => {
         ...stdioResult(stdout, stderr, opts),
         ...extra,
       }
+
       if (code || signal) {
         rej(Object.assign(new Error('command failed'), result))
       } else {
@@ -49,14 +61,101 @@ const promiseSpawn = (cmd, args, opts = {}, extra = {}) => {
   return p
 }
 
-const stdioResult = (stdout, stderr, { stdioString, stdio }) =>
-  stdioString ? {
-    stdout: isPipe(stdio, 1) ? Buffer.concat(stdout).toString().trim() : null,
-    stderr: isPipe(stdio, 2) ? Buffer.concat(stderr).toString().trim() : null,
+const spawnWithShell = (cmd, args, opts, extra) => {
+  let command = opts.shell
+  // if shell is set to true, we use a platform default. we can't let the core
+  // spawn method decide this for us because we need to know what shell is in use
+  // ahead of time so that we can escape arguments properly. we don't need coverage here.
+  if (command === true) {
+    // istanbul ignore next
+    command = process.platform === 'win32' ? process.env.ComSpec : 'sh'
   }
-  : {
-    stdout: isPipe(stdio, 1) ? Buffer.concat(stdout) : null,
-    stderr: isPipe(stdio, 2) ? Buffer.concat(stderr) : null,
+
+  const options = { ...opts, shell: false }
+  const realArgs = []
+  let script = cmd
+
+  // first, determine if we're in windows because if we are we need to know if we're
+  // running an .exe or a .cmd/.bat since the latter requires extra escaping
+  const isCmd = /(?:^|\\)cmd(?:\.exe)?$/i.test(command)
+  if (isCmd) {
+    let doubleEscape = false
+
+    // find the actual command we're running
+    let initialCmd = ''
+    let insideQuotes = false
+    for (let i = 0; i < cmd.length; ++i) {
+      const char = cmd.charAt(i)
+      if (char === ' ' && !insideQuotes) {
+        break
+      }
+
+      initialCmd += char
+      if (char === '"' || char === "'") {
+        insideQuotes = !insideQuotes
+      }
+    }
+
+    let pathToInitial
+    try {
+      pathToInitial = which.sync(initialCmd, {
+        path: (options.env && options.env.PATH) || process.env.PATH,
+        pathext: (options.env && options.env.PATHEXT) || process.env.PATHEXT,
+      }).toLowerCase()
+    } catch (err) {
+      pathToInitial = initialCmd.toLowerCase()
+    }
+
+    doubleEscape = pathToInitial.endsWith('.cmd') || pathToInitial.endsWith('.bat')
+    for (const arg of args) {
+      script += ` ${escape.cmd(arg, doubleEscape)}`
+    }
+    realArgs.push('/d', '/s', '/c', script)
+    options.windowsVerbatimArguments = true
+  } else {
+    for (const arg of args) {
+      script += ` ${escape.sh(arg)}`
+    }
+    realArgs.push('-c', script)
+  }
+
+  return promiseSpawn(command, realArgs, options, extra)
+}
+
+const isPipe = (stdio = 'pipe', fd) => {
+  if (stdio === 'pipe' || stdio === null) {
+    return true
+  }
+
+  if (Array.isArray(stdio)) {
+    return isPipe(stdio[fd], fd)
+  }
+
+  return false
+}
+
+const stdioResult = (stdout, stderr, { stdioString, stdio }) => {
+  const result = {
+    stdout: null,
+    stderr: null,
+  }
+
+  // stdio is [stdin, stdout, stderr]
+  if (isPipe(stdio, 1)) {
+    result.stdout = Buffer.concat(stdout)
+    if (stdioString) {
+      result.stdout = result.stdout.toString().trim()
+    }
   }
 
+  if (isPipe(stdio, 2)) {
+    result.stderr = Buffer.concat(stderr)
+    if (stdioString) {
+      result.stderr = result.stderr.toString().trim()
+    }
+  }
+
+  return result
+}
+
 module.exports = promiseSpawn

package.json

@@ -43,5 +43,8 @@
   "templateOSS": {
     "//@npmcli/template-oss": "This file is partially managed by @npmcli/template-oss. Edits may be overwritten.",
     "version": "4.8.0"
+  },
+  "dependencies": {
+    "which": "^2.0.2"
   }
 }

test/escape.js

@@ -0,0 +1,119 @@
+'use strict'
+
+const { writeFileSync: writeFile } = require('fs')
+const { join } = require('path')
+const t = require('tap')
+const promiseSpawn = require('../lib/index.js')
+
+const escape = require('../lib/escape.js')
+const isWindows = process.platform === 'win32'
+
+t.test('sh', (t) => {
+  const expectations = [
+    ['', `''`],
+    ['test', 'test'],
+    ['test words', `'test words'`],
+    ['$1', `'$1'`],
+    ['"$1"', `'"$1"'`],
+    [`'$1'`, `\\''$1'\\'`],
+    ['\\$1', `'\\$1'`],
+    ['--arg="$1"', `'--arg="$1"'`],
+    ['--arg=npm exec -c "$1"', `'--arg=npm exec -c "$1"'`],
+    [`--arg=npm exec -c '$1'`, `'--arg=npm exec -c '\\''$1'\\'`],
+    [`'--arg=npm exec -c "$1"'`, `\\''--arg=npm exec -c "$1"'\\'`],
+  ]
+
+  for (const [input, expectation] of expectations) {
+    t.equal(escape.sh(input), expectation,
+      `expected to escape \`${input}\` to \`${expectation}\``)
+  }
+
+  t.test('integration', { skip: isWindows && 'posix only' }, async (t) => {
+    for (const [input] of expectations) {
+      const p = await promiseSpawn('node', ['-p', 'process.argv[1]', '--', input],
+        { shell: true, stdioString: true })
+      const stdout = p.stdout.trim()
+      t.equal(stdout, input, `expected \`${stdout}\` to equal \`${input}\``)
+    }
+
+    t.end()
+  })
+
+  t.end()
+})
+
+t.test('cmd', (t) => {
+  const expectations = [
+    ['', '""'],
+    ['test', 'test'],
+    ['%PATH%', '^%PATH^%'],
+    ['%PATH%', '^^^%PATH^^^%', true],
+    ['"%PATH%"', '^"\\^"^%PATH^%\\^"^"'],
+    ['"%PATH%"', '^^^"\\^^^"^^^%PATH^^^%\\^^^"^^^"', true],
+    [`'%PATH%'`, `'^%PATH^%'`],
+    [`'%PATH%'`, `'^^^%PATH^^^%'`, true],
+    ['\\%PATH%', '\\^%PATH^%'],
+    ['\\%PATH%', '\\^^^%PATH^^^%', true],
+    ['--arg="%PATH%"', '^"--arg=\\^"^%PATH^%\\^"^"'],
+    ['--arg="%PATH%"', '^^^"--arg=\\^^^"^^^%PATH^^^%\\^^^"^^^"', true],
+    ['--arg=npm exec -c "%PATH%"', '^"--arg=npm^ exec^ -c^ \\^"^%PATH^%\\^"^"'],
+    ['--arg=npm exec -c "%PATH%"',
+      '^^^"--arg=npm^^^ exec^^^ -c^^^ \\^^^"^^^%PATH^^^%\\^^^"^^^"', true],
+    [`--arg=npm exec -c '%PATH%'`, `^"--arg=npm^ exec^ -c^ '^%PATH^%'^"`],
+    [`--arg=npm exec -c '%PATH%'`, `^^^"--arg=npm^^^ exec^^^ -c^^^ '^^^%PATH^^^%'^^^"`, true],
+    [`'--arg=npm exec -c "%PATH%"'`, `^"'--arg=npm^ exec^ -c^ \\^"^%PATH^%\\^"'^"`],
+    [`'--arg=npm exec -c "%PATH%"'`,
+      `^^^"'--arg=npm^^^ exec^^^ -c^^^ \\^^^"^^^%PATH^^^%\\^^^"'^^^"`, true],
+    ['"C:\\Program Files\\test.bat"', '^"\\^"C:\\Program^ Files\\test.bat\\^"^"'],
+    ['"C:\\Program Files\\test.bat"', '^^^"\\^^^"C:\\Program^^^ Files\\test.bat\\^^^"^^^"', true],
+    ['"C:\\Program Files\\test%.bat"', '^"\\^"C:\\Program^ Files\\test^%.bat\\^"^"'],
+    ['"C:\\Program Files\\test%.bat"',
+      '^^^"\\^^^"C:\\Program^^^ Files\\test^^^%.bat\\^^^"^^^"', true],
+    ['% % %', '^"^%^ ^%^ ^%^"'],
+    ['% % %', '^^^"^^^%^^^ ^^^%^^^ ^^^%^^^"', true],
+    ['hello^^^^^^', 'hello^^^^^^^^^^^^'],
+    ['hello^^^^^^', 'hello^^^^^^^^^^^^^^^^^^^^^^^^', true],
+    ['hello world', '^"hello^ world^"'],
+    ['hello world', '^^^"hello^^^ world^^^"', true],
+    ['hello"world', '^"hello\\^"world^"'],
+    ['hello"world', '^^^"hello\\^^^"world^^^"', true],
+    ['hello""world', '^"hello\\^"\\^"world^"'],
+    ['hello""world', '^^^"hello\\^^^"\\^^^"world^^^"', true],
+    ['hello\\world', 'hello\\world'],
+    ['hello\\world', 'hello\\world', true],
+    ['hello\\\\world', 'hello\\\\world'],
+    ['hello\\\\world', 'hello\\\\world', true],
+    ['hello\\"world', '^"hello\\\\\\^"world^"'],
+    ['hello\\"world', '^^^"hello\\\\\\^^^"world^^^"', true],
+    ['hello\\\\"world', '^"hello\\\\\\\\\\^"world^"'],
+    ['hello\\\\"world', '^^^"hello\\\\\\\\\\^^^"world^^^"', true],
+    ['hello world\\', '^"hello^ world\\\\^"'],
+    ['hello world\\', '^^^"hello^^^ world\\\\^^^"', true],
+    ['hello %PATH%', '^"hello^ ^%PATH^%^"'],
+    ['hello %PATH%', '^^^"hello^^^ ^^^%PATH^^^%^^^"', true],
+  ]
+
+  for (const [input, expectation, double] of expectations) {
+    const msg = `expected to${double ? ' double' : ''} escape \`${input}\` to \`${expectation}\``
+    t.equal(escape.cmd(input, double), expectation, msg)
+  }
+
+  t.test('integration', { skip: !isWindows && 'Windows only' }, async (t) => {
+    const dir = t.testdir()
+    const shimFile = join(dir, 'shim.cmd')
+    const shim = `@echo off\nnode -p process.argv[1] -- %*`
+    writeFile(shimFile, shim)
+
+    const spawnOpts = { shell: true, stdioString: true }
+    for (const [input,, double] of expectations) {
+      const p = double
+        ? await promiseSpawn(shimFile, [input], spawnOpts)
+        : await promiseSpawn('node', ['-p', 'process.argv[1]', '--', input], spawnOpts)
+      t.equal(p.stdout, input, `expected \`${p.stdout}\` to equal \`${input}\``)
+    }
+
+    t.end()
+  })
+
+  t.end()
+})