From 29fc263902146a02189c126314ab43afb49ba845 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Sun, 6 Aug 2023 06:10:13 +0800 Subject: [PATCH 1/5] child_process: allow `URL` instances as paths to executables --- doc/api/child_process.md | 30 +++++++++++++++++++++++++----- lib/child_process.js | 11 +++++------ test/common/index.js | 4 ++-- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/doc/api/child_process.md b/doc/api/child_process.md index a32917997d37f1..b5f398663556ec 100644 --- a/doc/api/child_process.md +++ b/doc/api/child_process.md @@ -282,6 +282,10 @@ controller.abort(); -* `file` {string} The name or path of the executable file to run. +* `file` {string|URL} The name or path of the executable file to run. * `args` {string\[]} List of string arguments. * `options` {Object} * `cwd` {string|URL} Current working directory of the child process. @@ -394,6 +398,10 @@ controller.abort(); -* `command` {string} The command to run. +* `command` {string|URL} The command to run. * `args` {string\[]} List of string arguments. * `options` {Object} * `cwd` {string|URL} Current working directory of the child process. @@ -897,6 +909,10 @@ configuration at startup. -* `file` {string} The name or path of the executable file to run. +* `file` {string|URL} The name or path of the executable file to run. * `args` {string\[]} List of string arguments. * `options` {Object} * `cwd` {string|URL} Current working directory of the child process. @@ -1040,6 +1056,10 @@ metacharacters may be used to trigger arbitrary command execution.** -* `command` {string} The command to run. +* `command` {string|URL} The command to run. * `args` {string\[]} List of string arguments. * `options` {Object} * `cwd` {string|URL} Current working directory of the child process. diff --git a/lib/child_process.js b/lib/child_process.js index 449013906e93e5..27bec285f8bbe0 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -106,7 +106,7 @@ let addAbortListener; * cwd?: string | URL; * detached?: boolean; * env?: Record; - * execPath?: string; + * execPath?: string | URL; * execArgv?: string[]; * gid?: number; * serialization?: string; @@ -302,7 +302,7 @@ function normalizeExecFileArgs(file, args, options, callback) { /** * Spawns the specified file as a shell. - * @param {string} file + * @param {string | URL} file * @param {string[]} [args] * @param {{ * cwd?: string | URL; @@ -545,8 +545,7 @@ function copyProcessEnvToEnv(env, name, optionEnv) { } function normalizeSpawnArguments(file, args, options) { - validateString(file, 'file'); - validateArgumentNullCheck(file, 'file'); + file = getValidatedPath(file, 'file'); if (file.length === 0) throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty'); @@ -730,7 +729,7 @@ function abortChildProcess(child, killSignal, reason) { /** * Spawns a new process using the given `file`. - * @param {string} file + * @param {string | URL} file * @param {string[]} [args] * @param {{ * cwd?: string | URL; @@ -800,7 +799,7 @@ function spawn(file, args, options) { /** * Spawns a new process synchronously using the given `file`. - * @param {string} file + * @param {string | URL} file * @param {string[]} [args] * @param {{ * cwd?: string | URL; diff --git a/test/common/index.js b/test/common/index.js index c10dea59319264..1175ea3153867f 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -263,8 +263,8 @@ function createZeroFilledFile(filename) { const pwdCommand = isWindows ? - ['cmd.exe', ['/d', '/c', 'cd']] : - ['pwd', []]; + [new URL('file:///C:/Windows/system32/cmd.exe'), ['/d', '/c', 'cd']] : + [new URL('file:///bin/pwd'), []]; function platformTimeout(ms) { From 79ae91adba4647861a0d82354d9b6cbd1fa8d132 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Sun, 6 Aug 2023 14:57:10 +0800 Subject: [PATCH 2/5] squash: add test --- test/common/index.js | 4 +- test/parallel/test-child-process-urlfile.mjs | 42 ++++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-child-process-urlfile.mjs diff --git a/test/common/index.js b/test/common/index.js index 1175ea3153867f..c10dea59319264 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -263,8 +263,8 @@ function createZeroFilledFile(filename) { const pwdCommand = isWindows ? - [new URL('file:///C:/Windows/system32/cmd.exe'), ['/d', '/c', 'cd']] : - [new URL('file:///bin/pwd'), []]; + ['cmd.exe', ['/d', '/c', 'cd']] : + ['pwd', []]; function platformTimeout(ms) { diff --git a/test/parallel/test-child-process-urlfile.mjs b/test/parallel/test-child-process-urlfile.mjs new file mode 100644 index 00000000000000..f0dee47f4a3630 --- /dev/null +++ b/test/parallel/test-child-process-urlfile.mjs @@ -0,0 +1,42 @@ +import { isWindows, mustCall, mustSucceed, mustNotCall } from '../common/index.mjs'; +import { strictEqual } from 'node:assert'; +import { pathToFileURL } from 'node:url'; +import cp from 'node:child_process'; +import tmpdir from '../common/tmpdir.js'; + +tmpdir.refresh(); + +const cwd = tmpdir.path; +const whichCommand = isWindows ? 'where.exe cmd.exe' : 'which pwd'; +const pwdFullPath = `${cp.execSync(whichCommand)}`.trim(); +const pwdUrl = pathToFileURL(pwdFullPath); +const pwdCommandAndOptions = isWindows ? + [pwdUrl, ['/d', '/c', 'cd'], { cwd: pathToFileURL(cwd) }] : + [pwdUrl, [], { cwd: pathToFileURL(cwd) }]; + +{ + cp.execFile(...pwdCommandAndOptions, mustSucceed((stdout) => { + strictEqual(`${stdout}`.trim(), cwd); + })); +} + +{ + const stdout = cp.execFileSync(...pwdCommandAndOptions); + strictEqual(`${stdout}`.trim(), cwd); +} + +{ + const pwd = cp.spawn(...pwdCommandAndOptions); + pwd.stdout.on('data', mustCall((stdout) => { + strictEqual(`${stdout}`.trim(), cwd); + })); + pwd.stderr.on('data', mustNotCall()); + pwd.on('close', mustCall((code) => { + strictEqual(code, 0); + })); +} + +{ + const stdout = cp.spawnSync(...pwdCommandAndOptions).stdout; + strictEqual(`${stdout}`.trim(), cwd); +} From d7aaae1c1129d8e6deeb40dd2d1a17ae783fde50 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Mon, 7 Aug 2023 02:27:14 +0800 Subject: [PATCH 3/5] squash: strict validation, test for bad input --- lib/child_process.js | 1 + test/parallel/test-child-process-urlfile.mjs | 105 ++++++++++++++----- 2 files changed, 81 insertions(+), 25 deletions(-) diff --git a/lib/child_process.js b/lib/child_process.js index 27bec285f8bbe0..10bcaacc3a9f06 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -546,6 +546,7 @@ function copyProcessEnvToEnv(env, name, optionEnv) { function normalizeSpawnArguments(file, args, options) { file = getValidatedPath(file, 'file'); + validateString(file, 'file'); if (file.length === 0) throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty'); diff --git a/test/parallel/test-child-process-urlfile.mjs b/test/parallel/test-child-process-urlfile.mjs index f0dee47f4a3630..4bd56b397a0e8f 100644 --- a/test/parallel/test-child-process-urlfile.mjs +++ b/test/parallel/test-child-process-urlfile.mjs @@ -1,42 +1,97 @@ import { isWindows, mustCall, mustSucceed, mustNotCall } from '../common/index.mjs'; -import { strictEqual } from 'node:assert'; -import { pathToFileURL } from 'node:url'; +import { strictEqual, throws } from 'node:assert'; import cp from 'node:child_process'; +import url from 'node:url'; import tmpdir from '../common/tmpdir.js'; tmpdir.refresh(); const cwd = tmpdir.path; +const cwdUrl = url.pathToFileURL(cwd); const whichCommand = isWindows ? 'where.exe cmd.exe' : 'which pwd'; const pwdFullPath = `${cp.execSync(whichCommand)}`.trim(); -const pwdUrl = pathToFileURL(pwdFullPath); -const pwdCommandAndOptions = isWindows ? - [pwdUrl, ['/d', '/c', 'cd'], { cwd: pathToFileURL(cwd) }] : - [pwdUrl, [], { cwd: pathToFileURL(cwd) }]; +const pwdWHATWGUrl = url.pathToFileURL(pwdFullPath); -{ - cp.execFile(...pwdCommandAndOptions, mustSucceed((stdout) => { +// Test for WHATWG URL instance, legacy URL, and URL-like object +for (const pwdUrl of [ + pwdWHATWGUrl, + new url.URL(pwdWHATWGUrl), + { + href: pwdWHATWGUrl.href, + origin: pwdWHATWGUrl.origin, + protocol: pwdWHATWGUrl.protocol, + username: pwdWHATWGUrl.username, + password: pwdWHATWGUrl.password, + host: pwdWHATWGUrl.host, + hostname: pwdWHATWGUrl.hostname, + port: pwdWHATWGUrl.port, + pathname: pwdWHATWGUrl.pathname, + search: pwdWHATWGUrl.search, + searchParams: new URLSearchParams(pwdWHATWGUrl.searchParams), + hash: pwdWHATWGUrl.hash, + }, +]) { + const pwdCommandAndOptions = isWindows ? + [pwdUrl, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : + [pwdUrl, [], { cwd: cwdUrl }]; + + { + cp.execFile(...pwdCommandAndOptions, mustSucceed((stdout) => { + strictEqual(`${stdout}`.trim(), cwd); + })); + } + + { + const stdout = cp.execFileSync(...pwdCommandAndOptions); strictEqual(`${stdout}`.trim(), cwd); - })); -} + } -{ - const stdout = cp.execFileSync(...pwdCommandAndOptions); - strictEqual(`${stdout}`.trim(), cwd); -} + { + const pwd = cp.spawn(...pwdCommandAndOptions); + pwd.stdout.on('data', mustCall((stdout) => { + strictEqual(`${stdout}`.trim(), cwd); + })); + pwd.stderr.on('data', mustNotCall()); + pwd.on('close', mustCall((code) => { + strictEqual(code, 0); + })); + } -{ - const pwd = cp.spawn(...pwdCommandAndOptions); - pwd.stdout.on('data', mustCall((stdout) => { + { + const stdout = cp.spawnSync(...pwdCommandAndOptions).stdout; strictEqual(`${stdout}`.trim(), cwd); - })); - pwd.stderr.on('data', mustNotCall()); - pwd.on('close', mustCall((code) => { - strictEqual(code, 0); - })); + } + } -{ - const stdout = cp.spawnSync(...pwdCommandAndOptions).stdout; - strictEqual(`${stdout}`.trim(), cwd); +// Test for non-URL objects +for (const badFile of [ + Buffer.from(pwdFullPath), + {}, + { href: pwdWHATWGUrl.href }, + { pathname: pwdWHATWGUrl.pathname }, +]) { + const pwdCommandAndOptions = isWindows ? + [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : + [badFile, [], { cwd: cwdUrl }]; + + throws( + () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), + { code: 'ERR_INVALID_ARG_TYPE' }, + ); + + throws( + () => cp.execFileSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_ARG_TYPE' }, + ); + + throws( + () => cp.spawn(...pwdCommandAndOptions), + { code: 'ERR_INVALID_ARG_TYPE' }, + ); + + throws( + () => cp.spawnSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_ARG_TYPE' }, + ); } From 71a2fef858cf3bf79553d2faa3a09b3c72c1bebc Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Mon, 7 Aug 2023 15:25:36 +0800 Subject: [PATCH 4/5] squash: more tests --- test/parallel/test-child-process-urlfile.mjs | 195 +++++++++++++++++++ 1 file changed, 195 insertions(+) diff --git a/test/parallel/test-child-process-urlfile.mjs b/test/parallel/test-child-process-urlfile.mjs index 4bd56b397a0e8f..68f2f3960bef89 100644 --- a/test/parallel/test-child-process-urlfile.mjs +++ b/test/parallel/test-child-process-urlfile.mjs @@ -75,6 +75,9 @@ for (const badFile of [ [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : [badFile, [], { cwd: cwdUrl }]; + // Passing an object that doesn't have shape of WHATWG URL object + // results in TypeError + throws( () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), { code: 'ERR_INVALID_ARG_TYPE' }, @@ -95,3 +98,195 @@ for (const badFile of [ { code: 'ERR_INVALID_ARG_TYPE' }, ); } + +// Test for non-file: URL objects +for (const badFile of [ + new URL('https://nodejs.org/file:///'), + new url.URL('https://nodejs.org/file:///'), + { + href: 'https://nodejs.org/file:///', + origin: 'https://nodejs.org', + protocol: 'https:', + username: '', + password: '', + host: 'nodejs.org', + hostname: 'nodejs.org', + port: '', + pathname: '/file:///', + search: '', + searchParams: new URLSearchParams(), + hash: '' + }, +]) { + const pwdCommandAndOptions = isWindows ? + [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : + [badFile, [], { cwd: cwdUrl }]; + + // Passing an URL object with protocol other than `file:` + // results in TypeError + + throws( + () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), + { code: 'ERR_INVALID_URL_SCHEME' }, + ); + + throws( + () => cp.execFileSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_URL_SCHEME' }, + ); + + throws( + () => cp.spawn(...pwdCommandAndOptions), + { code: 'ERR_INVALID_URL_SCHEME' }, + ); + + throws( + () => cp.spawnSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_URL_SCHEME' }, + ); +} + +// Test for malformed file URL objects +for (const badFile of [ + new URL('file://nodejs.org/file:///'), + new url.URL('file://nodejs.org/file:///'), + { + href: 'file://nodejs.org/file:///', + origin: 'null', + protocol: 'file:', + username: '', + password: '', + host: 'nodejs.org', + hostname: 'nodejs.org', + port: '', + pathname: '/file:///', + search: '', + searchParams: new URLSearchParams(), + hash: '' + }, +]) { + const pwdCommandAndOptions = isWindows ? + [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : + [badFile, [], { cwd: cwdUrl }]; + + // Passing an URL object with non-empty host + // results in TypeError + + throws( + () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + + throws( + () => cp.execFileSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + + throws( + () => cp.spawn(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + + throws( + () => cp.spawnSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); +} + +// Test for file URL objects with %2F in path +const urlWithSlash = new URL(pwdWHATWGUrl); +urlWithSlash.pathname += '%2F'; +for (const badFile of [ + urlWithSlash, + new url.URL(urlWithSlash), + { + href: urlWithSlash.href, + origin: urlWithSlash.origin, + protocol: urlWithSlash.protocol, + username: urlWithSlash.username, + password: urlWithSlash.password, + host: urlWithSlash.host, + hostname: urlWithSlash.hostname, + port: urlWithSlash.port, + pathname: urlWithSlash.pathname, + search: urlWithSlash.search, + searchParams: new URLSearchParams(urlWithSlash.searchParams), + hash: urlWithSlash.hash, + }, +]) { + const pwdCommandAndOptions = isWindows ? + [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : + [badFile, [], { cwd: cwdUrl }]; + + // Passing an URL object with percent-encoded '/' + // results in TypeError + + throws( + () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), + { code: 'ERR_INVALID_FILE_URL_PATH' }, + ); + + throws( + () => cp.execFileSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_PATH' }, + ); + + throws( + () => cp.spawn(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_PATH' }, + ); + + throws( + () => cp.spawnSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_PATH' }, + ); +} + +// Test for file URL objects with %00 in path +const urlWithNul = new URL(pwdWHATWGUrl); +urlWithNul.pathname += '%00'; +for (const badFile of [ + urlWithNul, + new url.URL(urlWithNul), + { + href: urlWithNul.href, + origin: urlWithNul.origin, + protocol: urlWithNul.protocol, + username: urlWithNul.username, + password: urlWithNul.password, + host: urlWithNul.host, + hostname: urlWithNul.hostname, + port: urlWithNul.port, + pathname: urlWithNul.pathname, + search: urlWithNul.search, + searchParams: new URLSearchParams(urlWithNul.searchParams), + hash: urlWithNul.hash, + }, +]) { + const pwdCommandAndOptions = isWindows ? + [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : + [badFile, [], { cwd: cwdUrl }]; + + // Passing an URL object with percent-encoded '\0' + // results in TypeError + + throws( + () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), + { code: 'ERR_INVALID_ARG_VALUE' }, + ); + + throws( + () => cp.execFileSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_ARG_VALUE' }, + ); + + throws( + () => cp.spawn(...pwdCommandAndOptions), + { code: 'ERR_INVALID_ARG_VALUE' }, + ); + + throws( + () => cp.spawnSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_ARG_VALUE' }, + ); +} From d25509f6770a4f2599f8b8f891f76e0f9157f630 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Thu, 17 Aug 2023 18:25:58 +0800 Subject: [PATCH 5/5] squash: skip non-empty host test on Windows --- test/parallel/test-child-process-urlfile.mjs | 89 ++++++++++---------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/test/parallel/test-child-process-urlfile.mjs b/test/parallel/test-child-process-urlfile.mjs index 68f2f3960bef89..381f2c2c7f6ef9 100644 --- a/test/parallel/test-child-process-urlfile.mjs +++ b/test/parallel/test-child-process-urlfile.mjs @@ -147,50 +147,51 @@ for (const badFile of [ } // Test for malformed file URL objects -for (const badFile of [ - new URL('file://nodejs.org/file:///'), - new url.URL('file://nodejs.org/file:///'), - { - href: 'file://nodejs.org/file:///', - origin: 'null', - protocol: 'file:', - username: '', - password: '', - host: 'nodejs.org', - hostname: 'nodejs.org', - port: '', - pathname: '/file:///', - search: '', - searchParams: new URLSearchParams(), - hash: '' - }, -]) { - const pwdCommandAndOptions = isWindows ? - [badFile, ['/d', '/c', 'cd'], { cwd: cwdUrl }] : - [badFile, [], { cwd: cwdUrl }]; - - // Passing an URL object with non-empty host - // results in TypeError - - throws( - () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), - { code: 'ERR_INVALID_FILE_URL_HOST' }, - ); - - throws( - () => cp.execFileSync(...pwdCommandAndOptions), - { code: 'ERR_INVALID_FILE_URL_HOST' }, - ); - - throws( - () => cp.spawn(...pwdCommandAndOptions), - { code: 'ERR_INVALID_FILE_URL_HOST' }, - ); - - throws( - () => cp.spawnSync(...pwdCommandAndOptions), - { code: 'ERR_INVALID_FILE_URL_HOST' }, - ); +// On Windows, non-empty host is allowed +if (!isWindows) { + for (const badFile of [ + new URL('file://nodejs.org/file:///'), + new url.URL('file://nodejs.org/file:///'), + { + href: 'file://nodejs.org/file:///', + origin: 'null', + protocol: 'file:', + username: '', + password: '', + host: 'nodejs.org', + hostname: 'nodejs.org', + port: '', + pathname: '/file:///', + search: '', + searchParams: new URLSearchParams(), + hash: '' + }, + ]) { + const pwdCommandAndOptions = [badFile, [], { cwd: cwdUrl }]; + + // Passing an URL object with non-empty host + // results in TypeError + + throws( + () => cp.execFile(...pwdCommandAndOptions, mustNotCall()), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + + throws( + () => cp.execFileSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + + throws( + () => cp.spawn(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + + throws( + () => cp.spawnSync(...pwdCommandAndOptions), + { code: 'ERR_INVALID_FILE_URL_HOST' }, + ); + } } // Test for file URL objects with %2F in path