Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

child_process: allow URL instances as paths to executables #49031

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

child_process: allow URL instances as paths to executables #49031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
29fc263
child_process: allow `URL` instances as paths to executables
LiviaMedeiros Aug 5, 2023
79ae91a
squash: add test
LiviaMedeiros Aug 6, 2023
d7aaae1
squash: strict validation, test for bad input
LiviaMedeiros Aug 6, 2023
71a2fef
squash: more tests
LiviaMedeiros Aug 7, 2023
d25509f
squash: skip non-empty host test on Windows
LiviaMedeiros Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions doc/api/child_process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,10 @@ controller.abort();
<!-- YAML
added: v0.1.91
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49031
description: The `file` parameter can be a WHATWG `URL` object using
`file:` protocol.
- version:
- v16.4.0
- v14.18.0
Expand All @@ -298,7 +302,7 @@ changes:
description: The `windowsHide` option is supported now.
-->

* `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.
Expand Down Expand Up @@ -394,6 +398,10 @@ controller.abort();
<!-- YAML
added: v0.5.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49031
description: The `execPath` option can be a WHATWG `URL` object using
`file:` protocol.
- version:
- v17.4.0
- v16.14.0
Expand Down Expand Up @@ -442,7 +450,7 @@ changes:
process. Specific behavior depends on the platform, see
[`options.detached`][]).
* `env` {Object} Environment key-value pairs. **Default:** `process.env`.
* `execPath` {string} Executable used to create the child process.
* `execPath` {string|URL} Executable used to create the child process.
* `execArgv` {string\[]} List of string arguments passed to the executable.
**Default:** `process.execArgv`.
* `gid` {number} Sets the group identity of the process (see setgid(2)).
Expand Down Expand Up @@ -522,6 +530,10 @@ if (process.argv[2] === 'child') {
<!-- YAML
added: v0.1.90
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49031
description: The `command` parameter can be a WHATWG `URL` object using
`file:` protocol.
- version:
- v16.4.0
- v14.18.0
Expand Down Expand Up @@ -559,7 +571,7 @@ changes:
description: The `shell` option is supported now.
-->

* `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.
Expand Down Expand Up @@ -897,6 +909,10 @@ configuration at startup.
<!-- YAML
added: v0.11.12
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49031
description: The `file` parameter can be a WHATWG `URL` object using
`file:` protocol.
- version:
- v16.4.0
- v14.18.0
Expand All @@ -920,7 +936,7 @@ changes:
description: The `encoding` option can now explicitly be set to `buffer`.
-->

* `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.
Expand Down Expand Up @@ -1040,6 +1056,10 @@ metacharacters may be used to trigger arbitrary command execution.**
<!-- YAML
added: v0.11.12
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49031
description: The `command` parameter can be a WHATWG `URL` object using
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The word object hints towards an actual javascript object with URL class properties, but there isn't any test to check towards it.

Suggested change
description: The `command` parameter can be a WHATWG `URL` object using
description: The `command` parameter can be a WHATWG `URL` instance using

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isURL() is duck-typed:

node/lib/internal/url.js

Lines 761 to 763 in d150316

function isURL(self) {
return Boolean(self?.href && self.protocol && self.auth === undefined && self.path === undefined);
}

so it probably makes sense? The same term is used in other entries here as well.

`file:` protocol.
- version:
- v16.4.0
- v14.18.0
Expand All @@ -1066,7 +1086,7 @@ changes:
description: The `shell` option is supported now.
-->

* `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.
Expand Down
10 changes: 5 additions & 5 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ let addAbortListener;
* cwd?: string | URL;
* detached?: boolean;
* env?: Record<string, string>;
* execPath?: string;
* execPath?: string | URL;
* execArgv?: string[];
* gid?: number;
* serialization?: string;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -545,8 +545,8 @@ function copyProcessEnvToEnv(env, name, optionEnv) {
}

function normalizeSpawnArguments(file, args, options) {
file = getValidatedPath(file, 'file');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validatePath inside getValidatedPath also accepts buffers. Can you add a test for it as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I guess it should be restricted to string or stringified URL, at least for now.

validateString(file, 'file');
validateArgumentNullCheck(file, 'file');

if (file.length === 0)
throw new ERR_INVALID_ARG_VALUE('file', file, 'cannot be empty');
Expand Down Expand Up @@ -730,7 +730,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;
Expand Down Expand Up @@ -800,7 +800,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;
Expand Down
Loading