test: add escapePOSIXShell util

PR-URL: #55125 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
nodejs · Sep 29, 2024 · 99e0d0d · 99e0d0d
1 parent 858bce5
commit 99e0d0d
Show file tree

Hide file tree

Showing 46 changed files with 274 additions and 344 deletions.
diff --git a/test/abort/test-abort-fatal-error.js b/test/abort/test-abort-fatal-error.js
@@ -27,16 +27,17 @@ if (common.isWindows)
 const assert = require('assert');
 const exec = require('child_process').exec;
 
-let cmdline = `ulimit -c 0; ${process.execPath}`;
-cmdline += ' --max-old-space-size=16 --max-semi-space-size=4';
-cmdline += ' -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"';
+const cmdline =
+  common.escapePOSIXShell`ulimit -c 0; "${
+    process.execPath
+  }" --max-old-space-size=16 --max-semi-space-size=4 -e "a = []; for (i = 0; i < 1e9; i++) { a.push({}) }"`;
 
-exec(cmdline, function(err, stdout, stderr) {
+exec(...cmdline, common.mustCall((err, stdout, stderr) => {
   if (!err) {
     console.log(stdout);
     console.log(stderr);
     assert(false, 'this test should fail');
   }
 
   assert(common.nodeProcessAborted(err.code, err.signal));
-});
+}));
diff --git a/test/async-hooks/test-callback-error.js b/test/async-hooks/test-callback-error.js
@@ -62,12 +62,14 @@ assert.ok(!arg);
   let program = process.execPath;
   let args = [
     '--abort-on-uncaught-exception', __filename, 'test_callback_abort' ];
-  const options = { encoding: 'utf8' };
+  let options = {};
   if (!common.isWindows) {
-    program = `ulimit -c 0 && exec ${program} ${args.join(' ')}`;
+    [program, options] = common.escapePOSIXShell`ulimit -c 0 && exec "${program}" ${args[0]} "${args[1]}" ${args[2]}`;
     args = [];
     options.shell = true;
   }
+
+  options.encoding = 'utf8';
   const child = spawnSync(program, args, options);
   if (common.isWindows) {
     assert.strictEqual(child.status, 134);

diff --git a/test/common/README.md b/test/common/README.md
@@ -112,6 +112,33 @@ Creates a 10 MiB file of all null characters.
 
 Indicates if there is more than 1gb of total memory.
 
+### ``escapePOSIXShell`shell command` ``
+
+Escapes values in a string template literal to pass them as env variable. On Windows, this function
+does not escape anything (which is fine for most paths, as `"` is not a valid
+char in a path on Windows), so for tests that must pass on Windows, you should
+use it only to escape paths, inside double quotes.
+This function is meant to be used for tagged template strings.
+
+```js
+const { escapePOSIXShell } = require('../common');
+const fixtures = require('../common/fixtures');
+const { execSync } = require('node:child_process');
+const origin = fixtures.path('origin');
+const destination = fixtures.path('destination');
+
+execSync(...escapePOSIXShell`cp "${origin}" "${destination}"`);
+
+// When you need to specify specific options, and/or additional env variables:
+const [cmd, opts] = escapePOSIXShell`cp "${origin}" "${destination}"`;
+console.log(typeof cmd === 'string'); // true
+console.log(opts === undefined || typeof opts.env === 'object'); // true
+execSync(cmd, { ...opts, stdio: 'ignore' });
+execSync(cmd, { stdio: 'ignore', env: { ...opts?.env, KEY: 'value' } });
+```
+
+When possible, avoid using a shell; that way, there's no need to escape values.
+
 ### `expectsError(validator[, exact])`
 
 * `validator` [\<Object>][<Object>] | [\<RegExp>][<RegExp>] |

diff --git a/test/common/index.js b/test/common/index.js
@@ -249,15 +249,13 @@ const PIPE = (() => {
 // `$node --abort-on-uncaught-exception $file child`
 // the process aborts.
 function childShouldThrowAndAbort() {
-  let testCmd = '';
+  const escapedArgs = escapePOSIXShell`"${process.argv[0]}" --abort-on-uncaught-exception "${process.argv[1]}" child`;
   if (!isWindows) {
     // Do not create core files, as it can take a lot of disk space on
     // continuous testing and developers' machines
-    testCmd += 'ulimit -c 0 && ';
+    escapedArgs[0] = 'ulimit -c 0 && ' + escapedArgs[0];
   }
-  testCmd += `"${process.argv[0]}" --abort-on-uncaught-exception `;
-  testCmd += `"${process.argv[1]}" child`;
-  const child = exec(testCmd);
+  const child = exec(...escapedArgs);
   child.on('exit', function onExit(exitCode, signal) {
     const errMsg = 'Test should have aborted ' +
                    `but instead exited with exit code ${exitCode}` +
@@ -888,6 +886,32 @@ function spawnPromisified(...args) {
   });
 }
 
+/**
+ * Escape values in a string template literal. On Windows, this function
+ * does not escape anything (which is fine for paths, as `"` is not a valid char
+ * in a path on Windows), so you should use it only to escape paths – or other
+ * values on tests which are skipped on Windows.
+ * This function is meant to be used for tagged template strings.
+ * @returns {[string, object | undefined]} An array that can be passed as
+ *                                         arguments to `exec` or `execSync`.
+ */
+function escapePOSIXShell(cmdParts, ...args) {
+  if (common.isWindows) {
+    // On Windows, paths cannot contain `"`, so we can return the string unchanged.
+    return [String.raw({ raw: cmdParts }, ...args)];
+  }
+  // On POSIX shells, we can pass values via the env, as there's a standard way for referencing a variable.
+  const env = { ...process.env };
+  let cmd = cmdParts[0];
+  for (let i = 0; i < args.length; i++) {
+    const envVarName = `ESCAPED_${i}`;
+    env[envVarName] = args[i];
+    cmd += '${' + envVarName + '}' + cmdParts[i + 1];
+  }
+
+  return [cmd, { env }];
+};
+
 function getPrintedStackTrace(stderr) {
   const lines = stderr.split('\n');
 
@@ -951,6 +975,7 @@ const common = {
   childShouldThrowAndAbort,
   createZeroFilledFile,
   defaultAutoSelectFamilyAttemptTimeout,
+  escapePOSIXShell,
   expectsError,
   expectRequiredModule,
   expectWarning,

diff --git a/test/common/index.mjs b/test/common/index.mjs
@@ -11,6 +11,7 @@ const {
   childShouldThrowAndAbort,
   createZeroFilledFile,
   enoughTestMem,
+  escapePOSIXShell,
   expectsError,
   expectWarning,
   getArrayBufferViews,
@@ -64,6 +65,7 @@ export {
   createRequire,
   createZeroFilledFile,
   enoughTestMem,
+  escapePOSIXShell,
   expectsError,
   expectWarning,
   getArrayBufferViews,

diff --git a/test/parallel/test-child-process-bad-stdio.js b/test/parallel/test-child-process-bad-stdio.js
@@ -27,7 +27,8 @@ ChildProcess.prototype.spawn = function() {
 };
 
 function createChild(options, callback) {
-  const cmd = `"${process.execPath}" "${__filename}" child`;
+  const [cmd, opts] = common.escapePOSIXShell`"${process.execPath}" "${__filename}" child`;
+  options = { ...options, env: { ...opts?.env, ...options.env } };
 
   return cp.exec(cmd, options, common.mustCall(callback));
 }

diff --git a/test/parallel/test-child-process-exec-encoding.js b/test/parallel/test-child-process-exec-encoding.js
@@ -13,7 +13,8 @@ if (process.argv[2] === 'child') {
   const expectedStdout = `${stdoutData}\n`;
   const expectedStderr = `${stderrData}\n`;
   function run(options, callback) {
-    const cmd = `"${process.execPath}" "${__filename}" child`;
+    const [cmd, opts] = common.escapePOSIXShell`"${process.execPath}" "${__filename}" child`;
+    options = { ...options, env: { ...opts?.env, ...options.env } };
 
     cp.exec(cmd, options, common.mustSucceed((stdout, stderr) => {
       callback(stdout, stderr);

diff --git a/test/parallel/test-child-process-exec-maxbuf.js b/test/parallel/test-child-process-exec-maxbuf.js
@@ -14,14 +14,15 @@ function runChecks(err, stdio, streamName, expected) {
 // On non-Windows, we can pass the path via the env; `"` is not a valid char on
 // Windows, so we can simply pass the path.
 const execNode = (args, optionsOrCallback, callback) => {
+  const [cmd, opts] = common.escapePOSIXShell`"${process.execPath}" `;
   let options = optionsOrCallback;
   if (typeof optionsOrCallback === 'function') {
     options = undefined;
     callback = optionsOrCallback;
   }
   return cp.exec(
-    `"${common.isWindows ? process.execPath : '$NODE'}" ${args}`,
-    common.isWindows ? options : { ...options, env: { ...process.env, NODE: process.execPath } },
+    cmd + args,
+    { ...opts, ...options },
     callback,
   );
 };

diff --git a/test/parallel/test-child-process-exec-std-encoding.js b/test/parallel/test-child-process-exec-std-encoding.js
@@ -12,8 +12,7 @@ if (process.argv[2] === 'child') {
   console.log(stdoutData);
   console.error(stderrData);
 } else {
-  const cmd = `"${process.execPath}" "${__filename}" child`;
-  const child = cp.exec(cmd, common.mustSucceed((stdout, stderr) => {
+  const child = cp.exec(...common.escapePOSIXShell`"${process.execPath}" "${__filename}" child`, common.mustSucceed((stdout, stderr) => {
     assert.strictEqual(stdout, expectedStdout);
     assert.strictEqual(stderr, expectedStderr);
   }));

diff --git a/test/parallel/test-child-process-exec-timeout-expire.js b/test/parallel/test-child-process-exec-timeout-expire.js
@@ -18,9 +18,10 @@ if (process.argv[2] === 'child') {
   return;
 }
 
-const cmd = `"${process.execPath}" "${__filename}" child`;
+const [cmd, opts] = common.escapePOSIXShell`"${process.execPath}" "${__filename}" child`;
 
 cp.exec(cmd, {
+  ...opts,
   timeout: kExpiringParentTimer,
 }, common.mustCall((err, stdout, stderr) => {
   console.log('[stdout]', stdout.trim());

diff --git a/test/parallel/test-child-process-exec-timeout-kill.js b/test/parallel/test-child-process-exec-timeout-kill.js
@@ -18,10 +18,11 @@ if (process.argv[2] === 'child') {
   return;
 }
 
-const cmd = `"${process.execPath}" "${__filename}" child`;
+const [cmd, opts] = common.escapePOSIXShell`"${process.execPath}" "${__filename}" child`;
 
 // Test with a different kill signal.
 cp.exec(cmd, {
+  ...opts,
   timeout: kExpiringParentTimer,
   killSignal: 'SIGKILL'
 }, common.mustCall((err, stdout, stderr) => {

diff --git a/test/parallel/test-child-process-exec-timeout-not-expired.js b/test/parallel/test-child-process-exec-timeout-not-expired.js
@@ -22,10 +22,11 @@ if (process.argv[2] === 'child') {
   return;
 }
 
-const cmd = `"${process.execPath}" "${__filename}" child`;
+const [cmd, opts] = common.escapePOSIXShell`"${process.execPath}" "${__filename}" child`;
 
 cp.exec(cmd, {
-  timeout: kTimeoutNotSupposedToExpire
+  ...opts,
+  timeout: kTimeoutNotSupposedToExpire,
 }, common.mustSucceed((stdout, stderr) => {
   assert.strictEqual(stdout.trim(), 'child stdout');
   assert.strictEqual(stderr.trim(), 'child stderr');

diff --git a/test/parallel/test-child-process-execsync-maxbuf.js b/test/parallel/test-child-process-execsync-maxbuf.js
@@ -1,5 +1,5 @@
 'use strict';
-require('../common');
+const { escapePOSIXShell } = require('../common');
 
 // This test checks that the maxBuffer option for child_process.spawnSync()
 // works as expected.
@@ -10,15 +10,12 @@ const { execSync } = require('child_process');
 const msgOut = 'this is stdout';
 const msgOutBuf = Buffer.from(`${msgOut}\n`);
 
-const args = [
-  '-e',
-  `"console.log('${msgOut}')";`,
-];
+const [cmd, opts] = escapePOSIXShell`"${process.execPath}" -e "${`console.log('${msgOut}')`}"`;
 
 // Verify that an error is returned if maxBuffer is surpassed.
 {
   assert.throws(() => {
-    execSync(`"${process.execPath}" ${args.join(' ')}`, { maxBuffer: 1 });
+    execSync(cmd, { ...opts, maxBuffer: 1 });
   }, (e) => {
     assert.ok(e, 'maxBuffer should error');
     assert.strictEqual(e.code, 'ENOBUFS');
@@ -33,8 +30,8 @@ const args = [
 // Verify that a maxBuffer size of Infinity works.
 {
   const ret = execSync(
-    `"${process.execPath}" ${args.join(' ')}`,
-    { maxBuffer: Infinity }
+    cmd,
+    { ...opts, maxBuffer: Infinity },
   );
 
   assert.deepStrictEqual(ret, msgOutBuf);
@@ -43,9 +40,7 @@ const args = [
 // Default maxBuffer size is 1024 * 1024.
 {
   assert.throws(() => {
-    execSync(
-      `"${process.execPath}" -e "console.log('a'.repeat(1024 * 1024))"`
-    );
+    execSync(...escapePOSIXShell`"${process.execPath}" -e "console.log('a'.repeat(1024 * 1024))"`);
   }, (e) => {
     assert.ok(e, 'maxBuffer should error');
     assert.strictEqual(e.code, 'ENOBUFS');
@@ -56,9 +51,7 @@ const args = [
 
 // Default maxBuffer size is 1024 * 1024.
 {
-  const ret = execSync(
-    `"${process.execPath}" -e "console.log('a'.repeat(1024 * 1024 - 1))"`
-  );
+  const ret = execSync(...escapePOSIXShell`"${process.execPath}" -e "console.log('a'.repeat(1024 * 1024 - 1))"`);
 
   assert.deepStrictEqual(
     ret.toString().trim(),

diff --git a/test/parallel/test-child-process-promisified.js b/test/parallel/test-child-process-promisified.js
@@ -7,16 +7,8 @@ const { promisify } = require('util');
 const exec = promisify(child_process.exec);
 const execFile = promisify(child_process.execFile);
 
-// The execPath might contain chars that should be escaped in a shell context.
-// On non-Windows, we can pass the path via the env; `"` is not a valid char on
-// Windows, so we can simply pass the path.
-const execNode = (args) => exec(
-  `"${common.isWindows ? process.execPath : '$NODE'}" ${args}`,
-  common.isWindows ? undefined : { env: { ...process.env, NODE: process.execPath } },
-);
-
 {
-  const promise = execNode('-p 42');
+  const promise = exec(...common.escapePOSIXShell`"${process.execPath}" -p 42`);
 
   assert(promise.child instanceof child_process.ChildProcess);
   promise.then(common.mustCall((obj) => {
@@ -53,7 +45,7 @@ const execNode = (args) => exec(
 const failingCodeWithStdoutErr =
   'console.log(42);console.error(43);process.exit(1)';
 {
-  execNode(`-e "${failingCodeWithStdoutErr}"`)
+  exec(...common.escapePOSIXShell`"${process.execPath}" -e "${failingCodeWithStdoutErr}"`)
     .catch(common.mustCall((err) => {
       assert.strictEqual(err.code, 1);
       assert.strictEqual(err.stdout, '42\n');