node
node copied to clipboard
nodejs
child_process: validate strings in exec and spawn
I went through the exec, execFile, spawn, execSync, execFileSync and spawnSync functions in child_process and edited all the functions to properly validate their string parameters
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 90.16%. Comparing base (
3c2da4b) to head (e67638f). Report is 1531 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #56148 +/- ##
==========================================
+ Coverage 87.99% 90.16% +2.17%
==========================================
Files 656 630 -26
Lines 188999 186467 -2532
Branches 35981 36616 +635
==========================================
+ Hits 166301 168131 +1830
+ Misses 15865 11125 -4740
- Partials 6833 7211 +378
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/child_process.js | 97.74% <100.00%> (+0.30%) |
:arrow_up: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
It looks like the added test pass on
main, meaning either the change inlib/is only a refactor, or we are not adding sufficient coverage to avoid regression.
it was mainly refactoring because, for example, when calling execFile, the validation was not done immediately but down the road when, at the very end, the method itself was calling the spawn function.
Tests are passing in main because the validation in spawn is already present; my change is mainly about doing it as soon as possible to reject as soon as possible in order to save some cycles and having a better stack trace
in order to save some cycles and having a better stack trace
I don't think it's the right approach, we should optimize for the happy path, where there are no error thrown. IIUC, with this change we would be checking twice if the arguments are valid, so in order to save some cycles we should not land this.
in order to save some cycles and having a better stack trace
I don't think it's the right approach, we should optimize for the happy path, where there are no error thrown. IIUC, with this change we would be checking twice if the arguments are valid, so in order to save some cycles we should not land this.
gotcha. I went in that direction because I noticed that was the case already. With the latest push all the validation is done down the line and only once
The added tests are passing on latest main. If we want to move forward with this, we should split this into two PRs/commits, one that increases the coverage, one that refactors the implementation
The commit message of https://github.com/nodejs/node/pull/56148/commits/80fcad1c2e36e27809642087c6fda99e32442b18 should say it's a refactor, e.g. child_process: refactor string validation in exec and spawn.
The commit message of https://github.com/nodejs/node/pull/56148/commits/2f434320482e72d85d848c58ea482158b7a25c26 should be using test: subsystem, not child_process:, e.g. test: increase coverage of argument validation of cp methods.
IMO the order of commits should be reversed, tests should land first.