child_process: validate options.shell and correctly enforce shell invocation in exec/execSync

[Renegade334]

Previously opened as #54623, and the PR description there is still valid. Unfortunately this was originally submitted while CI was blocked by the macos test platform issues, and never made it back to request-ci.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.21%. Comparing base (bade7a1) to head (975a59e).
Report is 1011 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56761      +/-   ##
==========================================
- Coverage   89.21%   89.21%   -0.01%     
==========================================
  Files         662      662              
  Lines      192124   192129       +5     
  Branches    36980    36976       -4     
==========================================
+ Hits       171404   171407       +3     
- Misses      13552    13558       +6     
+ Partials     7168     7164       -4     

Files with missing lines	Coverage Δ
lib/child_process.js	97.74% <100.00%> (+0.01%)	⬆️

... and 21 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aduh95]

I don't understand why would we want to accept the empty string a valid value

Suggested change

	}
	options.shell ||= true;
	} else {
	options.shell = true;
	}

[Renegade334]

This suggested change would maintain the existing edge case behaviour when { shell: '' } is passed to exec.

• It is not null, and validates as a string, so is passed through unchanged.
• When this reaches spawn() downstream, because it is not a truthy value, a shell is not invoked.

exec() must always invoke a shell, and therefore anything that passes this validation needs to be a truthy value.

[aduh95]

It would make more sense to throw on empty string IMO

[Renegade334]

I'd agree, but this would then diverge from the behaviour of the other spawning child_process functions, unless we change them all? (Everywhere else considers { shell: '' } equivalent to { shell: undefined }.)

[aduh95]

I guess we could start by deprecating that, sure

[Renegade334]

Separate PR?

[aduh95]

Yes, two separate PRs eve, one for doc-only deprecation, and one that adds a runtime warning. Would you like to take up that effort?

[Renegade334]

While deprecated, should the behaviour of exec(..., { shell: '' }) be

1. its current behaviour (attempt to spawn the target directly, equivalent to execFile(..., { shell: false })), or
2. this proposal's behaviour (spawn a default shell, the same as exec(..., { shell: undefined })?

My vote would be for the latter – the whole point of exec is that it always invokes a shell, so I can't see that anyone would be relying on 1 as intended behaviour.

[aduh95]

IMO it should be the current behavior until we move forward with the deprecation at which point it will throw. I don't see the point of introducing an intermediary breaking change only to land another breaking change