child_process: allow `URL` instances as paths to executables #49031

LiviaMedeiros · 2023-08-05T22:27:05Z

Allow URL instances to be passed as file parameters and execPath options. For example:

import { execFile } from 'node:child_process';

execFile(
  new URL('../../bin/executable', import.meta.url),
  ['argv1'],
  { cwd: new URL('../dir/subdir', import.meta.url) },
  () => { ... }
);

mscdex · 2023-08-05T22:45:01Z

test/common/index.js

+  [new URL('file:///C:/Windows/system32/cmd.exe'), ['/d', '/c', 'cd']] :
+  [new URL('file:///bin/pwd'), []];


Why these changes? Are we sure these absolute paths are safe assumptions?

Not 100% sure and I'd like to see if it's not the case on any platform covered by CI first.

Then it will most likely be moved to separate test-child-process-urlfile.mjs test.

If there is a concise&robust way to get absolute url or absolute path to pwd or any other convenient good-for-test executable, or if we can create one in fixtures, I'd like to know.

I would guess the "where"/"which" commands should generally be available, even outside of CI since tests should work locally too.

github-actions · 2023-08-06T04:46:44Z

Failed to start CI

- Validating Jenkins credentials
✘  Jenkins credentials invalid

https://github.com/nodejs/node/actions/runs/5774680936

anonrig · 2023-08-06T15:55:25Z

doc/api/child_process.md

@@ -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


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

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.

anonrig · 2023-08-06T15:59:11Z

test/parallel/test-child-process-urlfile.mjs

+}
+
+{
+  const stdout = cp.spawnSync(...pwdCommandAndOptions).stdout;


Can you also add a test where the legacy url class is used? We need to make sure it does exactly what we want it to be.

anonrig · 2023-08-06T16:00:08Z

test/parallel/test-child-process-urlfile.mjs

+const pwdFullPath = `${cp.execSync(whichCommand)}`.trim();
+const pwdUrl = pathToFileURL(pwdFullPath);
+const pwdCommandAndOptions = isWindows ?
+  [pwdUrl, ['/d', '/c', 'cd'], { cwd: pathToFileURL(cwd) }] :


Can you also add a test where javascript object representation of the URL is used?

anonrig · 2023-08-06T16:01:56Z

lib/child_process.js

@@ -545,8 +545,7 @@ function copyProcessEnvToEnv(env, name, optionEnv) {
 }

 function normalizeSpawnArguments(file, args, options) {
-  validateString(file, 'file');
-  validateArgumentNullCheck(file, 'file');
+  file = getValidatedPath(file, 'file');


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

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

anonrig · 2023-08-06T16:04:38Z

cc @nodejs/url

anonrig · 2023-08-06T21:37:40Z

test/parallel/test-child-process-urlfile.mjs

+
+  throws(
+    () => cp.execFileSync(...pwdCommandAndOptions),
+    { code: 'ERR_INVALID_ARG_TYPE' },


Can you be specific, or at least add comment about what this line is testing? It is really cryptic atm.

Added tests for specific errors, added brief comments.

nodejs-github-bot · 2023-08-10T17:32:17Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53137/

nodejs-github-bot · 2023-09-07T12:44:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53781/

nodejs-github-bot · 2023-09-07T18:42:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53792/

nodejs-github-bot · 2023-09-08T02:10:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53799/

nodejs-github-bot · 2023-09-08T04:47:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/53803/

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Aug 5, 2023

mscdex reviewed Aug 5, 2023

View reviewed changes

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 6, 2023

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 6, 2023

anonrig reviewed Aug 6, 2023

View reviewed changes

aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Aug 8, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 10, 2023

This comment was marked as outdated.

Sign in to view

LiviaMedeiros added 4 commits August 11, 2023 01:30

child_process: allow URL instances as paths to executables

29fc263

squash: add test

79ae91a

squash: strict validation, test for bad input

d7aaae1

squash: more tests

71a2fef

LiviaMedeiros force-pushed the childprocess-file-url branch from 7ca5ecf to 71a2fef Compare August 10, 2023 17:31

squash: skip non-empty host test on Windows

d25509f

LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 7, 2023

github-actions bot mentioned this pull request Sep 8, 2023

CI Reliability 2023-09-08 nodejs/reliability#659

Open

26 tasks

LiviaMedeiros added the review wanted PRs that need reviews. label Sep 8, 2023

github-actions bot mentioned this pull request Sep 9, 2023

CI Reliability 2023-09-09 nodejs/reliability#660

Open

24 tasks

This was referenced Sep 10, 2023

CI Reliability 2023-09-10 nodejs/reliability#661

Open

CI Reliability 2023-09-11 nodejs/reliability#662

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

child_process: allow `URL` instances as paths to executables #49031

child_process: allow `URL` instances as paths to executables #49031

LiviaMedeiros commented Aug 5, 2023

mscdex Aug 5, 2023

LiviaMedeiros Aug 5, 2023

mscdex Aug 5, 2023

github-actions bot commented Aug 6, 2023

anonrig Aug 6, 2023

LiviaMedeiros Aug 6, 2023

anonrig Aug 6, 2023

anonrig Aug 6, 2023

anonrig Aug 6, 2023

LiviaMedeiros Aug 6, 2023

anonrig commented Aug 6, 2023

anonrig Aug 6, 2023

LiviaMedeiros Aug 7, 2023

This comment was marked as outdated.

nodejs-github-bot commented Aug 10, 2023

nodejs-github-bot commented Sep 7, 2023

nodejs-github-bot commented Sep 7, 2023

nodejs-github-bot commented Sep 8, 2023

nodejs-github-bot commented Sep 8, 2023

		[new URL('file:///C:/Windows/system32/cmd.exe'), ['/d', '/c', 'cd']] :
		[new URL('file:///bin/pwd'), []];

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

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

child_process: allow URL instances as paths to executables #49031

Are you sure you want to change the base?

child_process: allow URL instances as paths to executables #49031

Conversation

LiviaMedeiros commented Aug 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anonrig commented Aug 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

nodejs-github-bot commented Aug 10, 2023

nodejs-github-bot commented Sep 7, 2023

nodejs-github-bot commented Sep 7, 2023

nodejs-github-bot commented Sep 8, 2023

nodejs-github-bot commented Sep 8, 2023

child_process: allow `URL` instances as paths to executables #49031

child_process: allow `URL` instances as paths to executables #49031