From 77b2d1a54542f3758e752a5051df32d80a3d880a Mon Sep 17 00:00:00 2001 From: nlf Date: Tue, 21 Jun 2022 16:26:14 -0700 Subject: [PATCH 1/2] fix: correctly double escape when script runs a known .cmd file --- README.md | 4 ++ lib/escape.js | 8 ++- lib/make-spawn-args.js | 58 ++++++++++++---- test/escape.js | 121 ++++++++++++++++++--------------- test/make-spawn-args.js | 144 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 268 insertions(+), 67 deletions(-) diff --git a/README.md b/README.md index 7ac7e2d..c4f9133 100644 --- a/README.md +++ b/README.md @@ -157,4 +157,8 @@ escaped to ensure they are processed as literal strings. We then instruct the shell to execute the script file, and when the process exits we remove the temporary file. +In Windows, when the shell is cmd, and when the initial command in the script +is a known batch file (i.e. `something.cmd`) we double escape additional +arguments so that the shim scripts npm installs work correctly. + The actual implementation of the escaping is in `lib/escape.js`. diff --git a/lib/escape.js b/lib/escape.js index 29d24a8..9a1a745 100644 --- a/lib/escape.js +++ b/lib/escape.js @@ -2,7 +2,7 @@ // 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) => { +const cmd = (input, doubleEscape) => { if (!input.length) { return '""' } @@ -37,7 +37,11 @@ const cmd = (input) => { // and finally, prefix shell meta chars with a ^ result = result.replace(/[!^&()<>|"]/g, '^$&') - // except for % which is escaped with another % + if (doubleEscape) { + result = result.replace(/[!^&()<>|"]/g, '^$&') + } + + // except for % which is escaped with another %, and only once result = result.replace(/%/g, '%%') return result diff --git a/lib/make-spawn-args.js b/lib/make-spawn-args.js index 6f3aa4c..ef27e91 100644 --- a/lib/make-spawn-args.js +++ b/lib/make-spawn-args.js @@ -3,7 +3,7 @@ const isWindows = require('./is-windows.js') const setPATH = require('./set-path.js') const { chmodSync: chmod, unlinkSync: unlink, writeFileSync: writeFile } = require('fs') const { tmpdir } = require('os') -const { resolve } = require('path') +const { isAbsolute, resolve } = require('path') const which = require('which') const npm_config_node_gyp = require.resolve('node-gyp/bin/node-gyp.js') const escape = require('./escape.js') @@ -20,19 +20,59 @@ const makeSpawnArgs = options => { stdioString = false, } = options + const spawnEnv = setPATH(path, { + // we need to at least save the PATH environment var + ...process.env, + ...env, + npm_package_json: resolve(path, 'package.json'), + npm_lifecycle_event: event, + npm_lifecycle_script: cmd, + npm_config_node_gyp, + }) + let scriptFile let script = '' + const isCmd = /(?:^|\\)cmd(?:\.exe)?$/i.test(scriptShell) if (isCmd) { + 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: spawnEnv.path, + pathext: spawnEnv.pathext, + }).toLowerCase() + } catch (err) { + pathToInitial = initialCmd.toLowerCase() + } + + const doubleEscape = pathToInitial.endsWith('.cmd') || pathToInitial.endsWith('.bat') + scriptFile = resolve(tmpdir(), `${event}-${Date.now()}.cmd`) script += '@echo off\n' - script += `${cmd} ${args.map((arg) => escape.cmd(arg)).join(' ')}` + script += `${cmd} ${args.map((arg) => escape.cmd(arg, doubleEscape)).join(' ')}` } else { - const shellPath = which.sync(scriptShell) + const shebang = isAbsolute(scriptShell) + ? `#!${scriptShell}` + : `#!/usr/bin/env ${scriptShell}` scriptFile = resolve(tmpdir(), `${event}-${Date.now()}.sh`) - script += `#!${shellPath}\n` + script += `${shebang}\n` script += `${cmd} ${args.map((arg) => escape.sh(arg)).join(' ')}` } + writeFile(scriptFile, script) if (!isCmd) { chmod(scriptFile, '0775') @@ -40,15 +80,7 @@ const makeSpawnArgs = options => { const spawnArgs = isCmd ? ['/d', '/s', '/c', scriptFile] : ['-c', scriptFile] const spawnOpts = { - env: setPATH(path, { - // we need to at least save the PATH environment var - ...process.env, - ...env, - npm_package_json: resolve(path, 'package.json'), - npm_lifecycle_event: event, - npm_lifecycle_script: cmd, - npm_config_node_gyp, - }), + env: spawnEnv, stdioString, stdio, cwd: path, diff --git a/test/escape.js b/test/escape.js index 35a419c..f232e0c 100644 --- a/test/escape.js +++ b/test/escape.js @@ -3,61 +3,78 @@ const t = require('tap') const escape = require('../lib/escape.js') t.test('sh', (t) => { - t.test('returns empty quotes when input is empty', async (t) => { - const input = '' - const output = escape.sh(input) - t.equal(output, `''`, 'returned empty single quotes') - }) + 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"'\\'`], + ] - t.test('returns plain string if quotes are not necessary', async (t) => { - const input = 'test' - const output = escape.sh(input) - t.equal(output, input, 'returned plain string') - }) - - t.test('wraps in single quotes if special character is present', async (t) => { - const input = 'test words' - const output = escape.sh(input) - t.equal(output, `'test words'`, 'wrapped in single quotes') - }) - t.end() + t.plan(expectations.length) + for (const [input, expectation] of expectations) { + t.equal(escape.sh(input), expectation, + `expected to escape \`${input}\` to \`${expectation}\``) + } }) t.test('cmd', (t) => { - t.test('returns empty quotes when input is empty', async (t) => { - const input = '' - const output = escape.cmd(input) - t.equal(output, '""', 'returned empty double quotes') - }) - - t.test('returns plain string if quotes are not necessary', async (t) => { - const input = 'test' - const output = escape.cmd(input) - t.equal(output, input, 'returned plain string') - }) - - t.test('wraps in double quotes when necessary', async (t) => { - const input = 'test words' - const output = escape.cmd(input) - t.equal(output, '^"test words^"', 'wrapped in double quotes') - }) - - t.test('doubles up backslashes at end of input', async (t) => { - const input = 'one \\ two \\' - const output = escape.cmd(input) - t.equal(output, '^"one \\ two \\\\^"', 'doubles backslash at end of string') - }) - - t.test('doubles up backslashes immediately before a double quote', async (t) => { - const input = 'one \\"' - const output = escape.cmd(input) - t.equal(output, '^"one \\\\\\^"^"', 'doubles backslash before double quote') - }) + 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], + ] - t.test('backslash escapes double quotes', async (t) => { - const input = '"test"' - const output = escape.cmd(input) - t.equal(output, '^"\\^"test\\^"^"', 'escaped double quotes') - }) - t.end() + t.plan(expectations.length) + 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) + } }) diff --git a/test/make-spawn-args.js b/test/make-spawn-args.js index aacd79e..640dced 100644 --- a/test/make-spawn-args.js +++ b/test/make-spawn-args.js @@ -81,6 +81,9 @@ if (isWindows) { windowsVerbatimArguments: true, }, 'got expected options') + const contents = fs.readFileSync(args[args.length - 1], { encoding: 'utf8' }) + // the contents will have a trailing space if no args are passed + t.equal(contents, `@echo off\nscript "quoted parameter"; second command `) t.ok(fs.existsSync(args[args.length - 1]), 'script file was written') cleanup() t.not(fs.existsSync(args[args.length - 1]), 'cleanup removes script file') @@ -93,6 +96,7 @@ if (isWindows) { whichPaths.set('blrorp', '/bin/blrorp') t.teardown(() => { whichPaths.delete('blrorp') + delete process.env.ComSpec }) const [shell, args, opts, cleanup] = makeSpawnArgs({ event: 'event', @@ -147,6 +151,114 @@ if (isWindows) { t.end() }) + t.test('single escapes when initial command is not a batch file', (t) => { + whichPaths.set('script', '/path/script.exe') + t.teardown(() => whichPaths.delete('script')) + + const [shell, args, opts, cleanup] = makeSpawnArgs({ + event: 'event', + path: 'path', + cmd: 'script', + args: ['"quoted parameter";', 'second command'], + }) + t.equal(shell, 'cmd', 'default shell applies') + t.match(args, ['/d', '/s', '/c', /\.cmd$/], 'got expected args') + t.match(opts, { + env: { + npm_package_json: /package\.json$/, + npm_lifecycle_event: 'event', + npm_lifecycle_script: 'script', + npm_config_node_gyp: require.resolve('node-gyp/bin/node-gyp.js'), + }, + stdio: undefined, + cwd: 'path', + windowsVerbatimArguments: true, + }, 'got expected options') + + const contents = fs.readFileSync(args[args.length - 1], { encoding: 'utf8' }) + t.equal(contents, `@echo off\nscript ^"\\^"quoted parameter\\^";^" ^"second command^"`) + t.ok(fs.existsSync(args[args.length - 1]), 'script file was written') + cleanup() + t.not(fs.existsSync(args[args.length - 1]), 'cleanup removes script file') + + t.end() + }) + + t.test('double escapes when initial command is a batch file', (t) => { + whichPaths.set('script', '/path/script.cmd') + t.teardown(() => whichPaths.delete('script')) + + const [shell, args, opts, cleanup] = makeSpawnArgs({ + event: 'event', + path: 'path', + cmd: 'script', + args: ['"quoted parameter";', 'second command'], + }) + t.equal(shell, 'cmd', 'default shell applies') + t.match(args, ['/d', '/s', '/c', /\.cmd$/], 'got expected args') + t.match(opts, { + env: { + npm_package_json: /package\.json$/, + npm_lifecycle_event: 'event', + npm_lifecycle_script: 'script', + npm_config_node_gyp: require.resolve('node-gyp/bin/node-gyp.js'), + }, + stdio: undefined, + cwd: 'path', + windowsVerbatimArguments: true, + }, 'got expected options') + + const contents = fs.readFileSync(args[args.length - 1], { encoding: 'utf8' }) + t.equal(contents, [ + '@echo off', + `script ^^^"\\^^^"quoted parameter\\^^^";^^^" ^^^"second command^^^"`, + ].join('\n')) + t.ok(fs.existsSync(args[args.length - 1]), 'script file was written') + cleanup() + t.not(fs.existsSync(args[args.length - 1]), 'cleanup removes script file') + + t.end() + }) + + t.test('correctly identifies initial cmd with spaces', (t) => { + // we do blind lookups in our test fixture here, however node-which + // will remove surrounding quotes + whichPaths.set('"my script"', '/path/script.cmd') + t.teardown(() => whichPaths.delete('my script')) + + const [shell, args, opts, cleanup] = makeSpawnArgs({ + event: 'event', + path: 'path', + cmd: '"my script"', + args: ['"quoted parameter";', 'second command'], + }) + t.equal(shell, 'cmd', 'default shell applies') + t.match(args, ['/d', '/s', '/c', /\.cmd$/], 'got expected args') + t.match(opts, { + env: { + npm_package_json: /package\.json$/, + npm_lifecycle_event: 'event', + npm_lifecycle_script: 'script', + npm_config_node_gyp: require.resolve('node-gyp/bin/node-gyp.js'), + }, + stdio: undefined, + cwd: 'path', + windowsVerbatimArguments: true, + }, 'got expected options') + + const contents = fs.readFileSync(args[args.length - 1], { encoding: 'utf8' }) + t.equal(contents, [ + '@echo off', + // eslint-disable-next-line max-len + `"my script" ^^^"\\^^^"quoted parameter\\^^^";^^^" ^^^"second command^^^"`, + ].join('\n')) + t.ok(fs.existsSync(args[args.length - 1]), 'script file was written') + cleanup() + t.not(fs.existsSync(args[args.length - 1]), 'cleanup removes script file') + + t.end() + }) + t.end() }) } else { @@ -176,6 +288,38 @@ if (isWindows) { windowsVerbatimArguments: undefined, }, 'got expected options') + const contents = fs.readFileSync(args[args.length - 1], { encoding: 'utf8' }) + t.equal(contents, `#!/usr/bin/env sh\nscript '"quoted parameter";' 'second command'`) + t.ok(fs.existsSync(args[args.length - 1]), 'script file was written') + cleanup() + t.not(fs.existsSync(args[args.length - 1]), 'cleanup removes script file') + + t.end() + }) + + t.test('skips /usr/bin/env if scriptShell is absolute', (t) => { + const [shell, args, opts, cleanup] = makeSpawnArgs({ + event: 'event', + path: 'path', + cmd: 'script', + args: ['"quoted parameter";', 'second command'], + scriptShell: '/bin/sh', + }) + t.equal(shell, '/bin/sh', 'kept provided setting') + t.match(args, ['-c', /\.sh$/], 'got expected args') + t.match(opts, { + env: { + npm_package_json: /package\.json$/, + npm_lifecycle_event: 'event', + npm_lifecycle_script: 'script', + }, + stdio: undefined, + cwd: 'path', + windowsVerbatimArguments: undefined, + }, 'got expected options') + + const contents = fs.readFileSync(args[args.length - 1], { encoding: 'utf8' }) + t.equal(contents, `#!/bin/sh\nscript '"quoted parameter";' 'second command'`) t.ok(fs.existsSync(args[args.length - 1]), 'script file was written') cleanup() t.not(fs.existsSync(args[args.length - 1]), 'cleanup removes script file') From 8c62010d4e49e4a568078c5418962da290a3bdb5 Mon Sep 17 00:00:00 2001 From: nlf Date: Wed, 22 Jun 2022 12:01:41 -0700 Subject: [PATCH 2/2] chore(tests): add some integration tests --- test/escape.js | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 51 insertions(+), 2 deletions(-) diff --git a/test/escape.js b/test/escape.js index f232e0c..2e5fd01 100644 --- a/test/escape.js +++ b/test/escape.js @@ -1,6 +1,12 @@ +'use strict' + +const { writeFileSync: writeFile, unlinkSync: unlink, chmodSync: chmod } = require('fs') +const { join } = require('path') const t = require('tap') +const promiseSpawn = require('@npmcli/promise-spawn') const escape = require('../lib/escape.js') +const isWindows = process.platform === 'win32' t.test('sh', (t) => { const expectations = [ @@ -17,11 +23,29 @@ t.test('sh', (t) => { [`'--arg=npm exec -c "$1"'`, `\\''--arg=npm exec -c "$1"'\\'`], ] - t.plan(expectations.length) 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) => { + const dir = t.testdir() + + for (const [input] of expectations) { + const filename = join(dir, 'posix.sh') + const script = `#!/usr/bin/env sh\nnode -p process.argv[1] -- ${escape.sh(input)}` + writeFile(filename, script) + chmod(filename, '0755') + const p = await promiseSpawn('sh', ['-c', filename], { stdioString: true }) + const stdout = p.stdout.trim() + t.equal(input, stdout, 'actual output matches input') + unlink(filename) + } + + t.end() + }) + + t.end() }) t.test('cmd', (t) => { @@ -72,9 +96,34 @@ t.test('cmd', (t) => { ['hello %PATH%', '^^^"hello %%PATH%%^^^"', true], ] - t.plan(expectations.length) 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() + + for (const [input,, double] of expectations) { + const filename = join(dir, 'win.cmd') + if (double) { + const shimFile = join(dir, 'shim.cmd') + const shim = `@echo off\nnode -p process.argv[1] -- %*` + writeFile(shimFile, shim) + const script = `@echo off\n"${shimFile}" ${escape.cmd(input, double)}` + writeFile(filename, script) + } else { + const script = `@echo off\nnode -p process.argv[1] -- ${escape.cmd(input)}` + writeFile(filename, script) + } + const p = await promiseSpawn('cmd', ['/d', '/s', '/c', filename], { stdioString: true }) + const stdout = p.stdout.trim() + t.equal(input, stdout, 'actual output matches input') + unlink(filename) + } + + t.end() + }) + + t.end() })