diff --git a/lib/child_process.js b/lib/child_process.js index e2b7a3292069d1..dad7faa43347b1 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -135,8 +135,10 @@ function normalizeExecArgs(command, options, callback) { options = undefined; } - // Make a shallow copy so we don't clobber the user's options object. - options = Object.assign({}, options); + // Make a shallow copy so we don't clobber the user's options object + // and we strictly make use of Object.create(null) so that only self + // properties are checked. + options = Object.assign(Object.create(null), options); options.shell = typeof options.shell === 'string' ? options.shell : true; return { @@ -416,6 +418,11 @@ function normalizeSpawnArguments(file, args, options) { else if (options === null || typeof options !== 'object') throw new ERR_INVALID_ARG_TYPE('options', 'object', options); + // Make a shallow copy so we don't clobber the user's options object + // and we strictly make use of Object.create(null) so that only self + // properties are checked. + options = Object.assign(Object.create(null), options); + // Validate the cwd, if present. if (options.cwd != null && typeof options.cwd !== 'string') { @@ -468,9 +475,6 @@ function normalizeSpawnArguments(file, args, options) { options.windowsVerbatimArguments); } - // Make a shallow copy so we don't clobber the user's options object. - options = Object.assign({}, options); - if (options.shell) { const command = [file].concat(args).join(' '); // Set the shell, switches, and commands. diff --git a/test/parallel/test-child-process-execfile.js b/test/parallel/test-child-process-execfile.js index af69d685789d62..80c2da1d13ca4b 100644 --- a/test/parallel/test-child-process-execfile.js +++ b/test/parallel/test-child-process-execfile.js @@ -2,7 +2,7 @@ const common = require('../common'); const assert = require('assert'); -const execFile = require('child_process').execFile; +const { execFile, execFileSync } = require('child_process'); const { getSystemErrorName } = require('util'); const fixtures = require('../common/fixtures'); @@ -47,3 +47,39 @@ const execOpts = { encoding: 'utf8', shell: true }; assert.ifError(err); })); } + +{ + // Verify the shell option is only passed explicitly + // in the options object and not inherited from the + // object prototype + Object.prototype.shell = true; + + execFile( + 'date', + ['; echo baz'], + common.mustCall((e, stdout) => { + assert.notStrictEqual(e, null); + assert.strictEqual(stdout.trim().indexOf('baz'), -1); + }) + ); + + Reflect.deleteProperty(Object.prototype, 'shell'); +} + +{ + // Verify the shell option is only passed explicitly + // in the options object and not inherited from the + // object prototype + Object.prototype.shell = true; + + assert.throws(function() { + execFileSync( + 'date', + ['; echo baz'] + ); + }, { + status: 1 + }, 'No safe-guard detected against value present on the prototype chain'); + + Reflect.deleteProperty(Object.prototype, 'shell'); +} diff --git a/test/parallel/test-child-process-spawn-shell.js b/test/parallel/test-child-process-spawn-shell.js index e9a753e91a4e48..dab8480023c435 100644 --- a/test/parallel/test-child-process-spawn-shell.js +++ b/test/parallel/test-child-process-spawn-shell.js @@ -2,63 +2,92 @@ const common = require('../common'); const assert = require('assert'); const cp = require('child_process'); +{ + // Verify that a shell is, in fact, executed + const doesNotExist = cp.spawn('does-not-exist', { shell: true }); -// Verify that a shell is, in fact, executed -const doesNotExist = cp.spawn('does-not-exist', { shell: true }); + assert.notStrictEqual(doesNotExist.spawnfile, 'does-not-exist'); + doesNotExist.on('error', common.mustNotCall()); + doesNotExist.on('exit', common.mustCall((code, signal) => { + assert.strictEqual(signal, null); -assert.notStrictEqual(doesNotExist.spawnfile, 'does-not-exist'); -doesNotExist.on('error', common.mustNotCall()); -doesNotExist.on('exit', common.mustCall((code, signal) => { - assert.strictEqual(signal, null); + if (common.isWindows) + assert.strictEqual(code, 1); // Exit code of cmd.exe + else + assert.strictEqual(code, 127); // Exit code of /bin/sh + })); +} - if (common.isWindows) - assert.strictEqual(code, 1); // Exit code of cmd.exe - else - assert.strictEqual(code, 127); // Exit code of /bin/sh -})); +{ + // Verify that passing arguments works + const echo = cp.spawn('echo', ['foo'], { + encoding: 'utf8', + shell: true + }); + let echoOutput = ''; -// Verify that passing arguments works -const echo = cp.spawn('echo', ['foo'], { - encoding: 'utf8', - shell: true -}); -let echoOutput = ''; + assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''), + 'echo foo'); + echo.stdout.on('data', (data) => { + echoOutput += data; + }); + echo.on('close', common.mustCall((code, signal) => { + assert.strictEqual(echoOutput.trim(), 'foo'); + })); +} -assert.strictEqual(echo.spawnargs[echo.spawnargs.length - 1].replace(/"/g, ''), - 'echo foo'); -echo.stdout.on('data', (data) => { - echoOutput += data; -}); -echo.on('close', common.mustCall((code, signal) => { - assert.strictEqual(echoOutput.trim(), 'foo'); -})); +{ + // Verify that shell features can be used + const cmd = 'echo bar | cat'; + const command = cp.spawn(cmd, { + encoding: 'utf8', + shell: true + }); + let commandOutput = ''; -// Verify that shell features can be used -const cmd = 'echo bar | cat'; -const command = cp.spawn(cmd, { - encoding: 'utf8', - shell: true -}); -let commandOutput = ''; + command.stdout.on('data', (data) => { + commandOutput += data; + }); + command.on('close', common.mustCall((code, signal) => { + assert.strictEqual(commandOutput.trim(), 'bar'); + })); +} -command.stdout.on('data', (data) => { - commandOutput += data; -}); -command.on('close', common.mustCall((code, signal) => { - assert.strictEqual(commandOutput.trim(), 'bar'); -})); +{ + // Verify that shell features can not be passed through + // object prototype manipulation, and must be strictly + // passed through the options object + const spawnCmd = 'date'; + const spawnCmdArgs = ['; echo baz']; + Object.prototype.shell = true; -// Verify that the environment is properly inherited -const env = cp.spawn(`"${process.execPath}" -pe process.env.BAZ`, { - env: Object.assign({}, process.env, { BAZ: 'buzz' }), - encoding: 'utf8', - shell: true -}); -let envOutput = ''; + const spawnHandler = cp.spawn(spawnCmd, spawnCmdArgs, { + encoding: 'utf8' + }); -env.stdout.on('data', (data) => { - envOutput += data; -}); -env.on('close', common.mustCall((code, signal) => { - assert.strictEqual(envOutput.trim(), 'buzz'); -})); + // assertion is expected to fail due to `spawnCmdArgs` not + // being a valid argument to the date command + const SUCCESS_CODE = 0; + spawnHandler.on('close', common.mustCall((code) => { + assert.notStrictEqual(code, SUCCESS_CODE); + })); + + Reflect.deleteProperty(Object.prototype, 'shell'); +} + +{ + // Verify that the environment is properly inherited + const env = cp.spawn(`"${process.execPath}" -pe process.env.BAZ`, { + env: Object.assign({}, process.env, { BAZ: 'buzz' }), + encoding: 'utf8', + shell: true + }); + let envOutput = ''; + + env.stdout.on('data', (data) => { + envOutput += data; + }); + env.on('close', common.mustCall((code, signal) => { + assert.strictEqual(envOutput.trim(), 'buzz'); + })); +}