Conversation
| // override exec for it to become silent by default and filter deprecation warnings | ||
| global.exec = function (cmd, done) { | ||
| return exec(cmd, { silent: true }, done); | ||
| return exec(cmd, { silent: true }, function (code, stdout, stderr) { |
Check warning
Code scanning / CodeQL
Shell command built from environment values Medium test
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
In general, the problem is that shelljs.exec is called with a single string cmd that was built by interpolating a path. The shell then parses cmd, so spaces or metacharacters in the path can alter the command. The standard fix is to avoid the shell for argument parsing: either (a) change callers to pass arguments separately and use an API that does not invoke a shell, or (b) at least escape or quote the interpolated values. Since we cannot touch the callers other than the shown test file, the best approach here is to (1) refactor the tests to use child_process.spawn/execFile directly with an argument array instead of global.exec, and (2) make global.exec in npm/test-cli.js reject multi-word shell commands and only allow a single binary path, effectively preventing unsafe templates from being used.
Concretely:
-
In
test/cli/working-directory.test.js, stop callingglobal.execwith a template literal. Instead, require Node’schild_processand usespawn(orexecFile) with:- command:
'node' - args:
['./bin/newman.js', 'run', 'test/fixtures/run/single-file-....json', '--working-dir', workingDir, ...]
This passesworkingDiras a separate argument and bypasses shell parsing entirely.
- command:
-
In
npm/test-cli.js, harden the overriddenglobal.execso it only accepts a command that is a simple binary path with no spaces (or is already an array), and throws if the caller passes something that looks like a shell command line. This addresses the CodeQL sink at line 35 by ensuring we never pass an arbitrary space-containing string toshelljs.exec. For compatibility with existing tests that legitimately use a string binary path, we allow a single word (no whitespace) or, optionally, an object/array form; for now we’ll implement the strict single-token check. This keeps existing functionality for legitimate usages while blocking unsafe patterns like those inworking-directory.test.jswhich we are refactoring away.
No new external packages are needed: child_process is built-in. The required changes are limited to the shown snippets in npm/test-cli.js and test/cli/working-directory.test.js.
| @@ -32,6 +32,14 @@ | ||
|
|
||
| // override exec for it to become silent by default and filter deprecation warnings | ||
| global.exec = function (cmd, done) { | ||
| // For safety, only allow simple commands without whitespace so that shell parsing | ||
| // cannot be influenced by untrusted or environment-derived values embedded in `cmd`. | ||
| // More complex invocations should use child_process.spawn/execFile with an args array. | ||
| if (typeof cmd === 'string' && /\s/.test(cmd)) { | ||
| throw new Error('Unsafe use of global.exec with a shell command string containing whitespace. ' + | ||
| 'Use child_process.spawn/execFile with argument arrays instead.'); | ||
| } | ||
|
|
||
| return exec(cmd, { silent: true }, function (code, stdout, stderr) { | ||
| // Filter out Node.js deprecation warnings from stderr (e.g., DEP0040 punycode warning in Node 22) | ||
| // This prevents test failures when Node emits deprecation warnings |
There was a problem hiding this comment.
this was already here, but added check for allowed prefixes to make it happier
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (52.12%) is below the target coverage (65.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #3347 +/- ##
===========================================
+ Coverage 81.85% 82.16% +0.30%
===========================================
Files 21 21
Lines 1152 1155 +3
Branches 352 354 +2
===========================================
+ Hits 943 949 +6
+ Misses 114 111 -3
Partials 95 95
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ad38fd8 to
641d45a
Compare
641d45a to
5ed273b
Compare
55d7a0b to
e250918
Compare
Removes remaining major production vulnerabilities (currently addressed via "overrides") primarily by upgrading postman-runtime 7.39.1 → 7.51.1. and postman-collection 4.4.0 -> 5.2.0, which also necessitates dropping Node 16 support.
Related issue: #3336
The current version of this PR also re-adds HTTP/2 support.
changes
- includes uvm 4.0.1 which re-adds vm, fixing issue seen on Node 22 / 6.2.0 release: newman 6.2.0 exception - node:internal/event_target:1090 process.nextTick(() => { throw err; }); #3267
tests
Note: The major upgrade of postman-collections from v4-> v5 includes a breaking change w/r/t PropertyBase.parent: https://github.com/postmanlabs/postman-collection/blob/develop/CHANGELOG.yaml#L40 This seems safe since tests for nested collection items are coming clean for related functionality in Newman eg
lib/util.js:getFullName()