child_process: escape args[] for shell

[Artoria2e5]

Closes #29532

(NO LONGER) BREAKING CHANGE: This changes the behavior of args[] in shell: true to
escape globs and other metacharacters. If you want to keep your scripts,
do it the proper way and stuff them in "command."

Checklist

• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Artoria2e5]

Going to wait for the tests to weed out some extra tests I need to change. The removal of cmd → vwa might mess up a few.

[Artoria2e5]

The documentation does feel a little repetitive at times with multiple segments used to describe the exact same type of spawn normalisation. I am thinking about collapsing them down into the same place and linking the others in, but should that be moved to a different PR?

CC @bnoordhuis

[Artoria2e5]

Windows is a fustercluck for those familiar with Unix cmdline handling. The bash tests should pass, but I am gonna see how they fail on my own copy of Windows and lower the bars on cmd accordingly...

The vcbuild thing looks painful. But it does look like I can go around the native require with a bit of:

const current_cp = require('child_process')

function spawnSync(file, args, options) {
  options = normalizeSpawnArguments(file, args, options);
  console.log(options.args);
  return current_cp.spawnSync(options.file, options.args.slice(1), options);
}

// normalizeSpawnArguments:
return { /* ... */ shell: false }

// ...
const _forkChild = current_cp._forkChild
module.exports =  { _forkChild, ...current_cp, spawnSync }

const cp = require('../../../patch_cp.js');

[Artoria2e5]

The test is almost passing on Windows. What fails include:

• That powershell test. The builtin Write-Line works but external commands like bash still fail with some characters (leading space, newline) being swallowed as well as quote alternations. Gotta dig into the PS source code to see what it is doing to call that command.

  + `Lorem Ipsu\\"m dolor sit on $'\\\\a\\a' ringing chair. `
  - `$'  Lorem  Ipsu""m\\ndolor\\tsit' $'on \\\\a\\a ringing chair.' `

  • So PS does it via System.Management.Automation.NativeCommandProcessor, which gets the stringified arguments for System.Diagnostics.ProcessStartInfo from NativeCommandParameterBinder.BindParameters. That part, however, isn't something I can read at midnight...
  • I should probably first port it to the same node-based check cmd uses to get a more representative sample...

---

Ho boy how did the one infalliable thing fail

[aduh95]

What if options.shellEscape is truthy but not a function (E.G. 1)? I think we should throw a ERR_INVALID_CALLBACK error in that case.

Suggested change

	if (!options.shellEscape)
	quote = (s) => s;
	else if (typeof options.shellEscape == 'function')
	quote = options.shellEscape;
	if (options.shellEscape === false || options.shellEscape === undefined) {
	quote = (s) => s;
	} else if (options.shellEscape !== true) {
	validateCallback(options.shellEscape);
	quote = options.shellEscape;
	}

[Artoria2e5]

I thought the ERR_INVALID_ARG_TYPE throw earliser would've caught that. When was validateCallback even introduced?

[aduh95]

I thought the ERR_INVALID_ARG_TYPE throw earliser would've caught that.

Yes you're probably right. Could add a test case for this scenario please?

[bnb]

@Artoria2e5 is this roughly ready? Looks like there's a conflict and one remaining piece of feedback. Would love to get this merged (if possible!). If not, no worries - we can close it out 👍🏻

[cjihrig]

The last comment on this PR was over a year ago, and suggested we could close this... so I'm closing. Feel free to revisit though.