Skip to content

child_process: validate exec's options.shell as string #59185

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.

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Renegade334
Copy link
Contributor

@Renegade334 Renegade334 commented Jul 23, 2025

exec() is documented to only take a string for the shell option, but this is not validated; passing something like { shell: false } (or any other invalid value) is currently silently ignored. This adds explicit validation.

Replaces #58525.

@nodejs-github-bot 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 Jul 23, 2025
Copy link

codecov bot commented Jul 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.03%. Comparing base (6bb08f7) to head (a355ebf).
Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59185      +/-   ##
==========================================
- Coverage   90.04%   90.03%   -0.01%     
==========================================
  Files         648      648              
  Lines      190978   191031      +53     
  Branches    37434    37451      +17     
==========================================
+ Hits       171964   171994      +30     
- Misses      11641    11650       +9     
- Partials     7373     7387      +14     
Files with missing lines Coverage Δ
lib/child_process.js 95.57% <100.00%> (+0.02%) ⬆️

... and 33 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.

@nodejs-github-bot
Copy link
Collaborator

@@ -33,7 +33,7 @@ const testCopy = (shellName, shellPath) => {
const system32 = `${process.env.SystemRoot}\\System32`;

// Test CMD
test(true);
Copy link
Member

Choose a reason for hiding this comment

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

shell: true is a widely used input. many downstream devs (and me) are actually using it.

https://github.com/search?q=child_process+shell%3A+true+language%3AJavaScript&type=code&l=JavaScript

So this is a breaking change. I personally unvote this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On exec() or its sister functions?

This is only adding validation of documented behaviour, which by precedent is not a breaking change – not that it's for me to say.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, it is string on document. But I think it's too loose before on runtime, now it's hard to make it back

@himself65 himself65 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants