fix: do not use a shell for git commands

[nlf]

this is an alternative to #27

rather than attempting to escape strings to be friendly with the shell, let's not use a shell at all

References

[ferdinando-ferreira]

let's not use a shell at all

@wraithgar , @nlf: All tests pass with flying colors in my win32 cmd shell system.

c:\tests\git>npm run test

> @npmcli/git@2.0.8 test
> tap

 PASS  test/find.js 4 OK 222.861ms
 PASS  test/lines-to-revs.js 22 OK 88.644ms
 PASS  test/opts.js 5 OK 131.684ms
 PASS  test/index.js 1 OK 13.646ms
 PASS  test/is.js 5 OK 192.227ms
 PASS  test/is-clean.js 6 OK 921.413ms
 PASS  test/revs.js 20 OK 3s
 PASS  test/should-retry.js 3 OK 11.476ms
 PASS  test/which.js 5 OK 2s
 PASS  test/spawn.js 6 OK 8s
 PASS  test/clone.js 119 OK 45s


  🌈 SUMMARY RESULTS 🌈


Suites:   11 passed, 11 of 11 completed
Asserts:  196 passed, of 196
Time:     54s
------------------|----------|----------|----------|----------|-------------------|
File              |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------|----------|----------|----------|----------|-------------------|
All files         |      100 |      100 |      100 |      100 |                   |
 clone.js         |      100 |      100 |      100 |      100 |                   |
 find.js          |      100 |      100 |      100 |      100 |                   |
 index.js         |      100 |      100 |      100 |      100 |                   |
 is-clean.js      |      100 |      100 |      100 |      100 |                   |
 is.js            |      100 |      100 |      100 |      100 |                   |
 lines-to-revs.js |      100 |      100 |      100 |      100 |                   |
 opts.js          |      100 |      100 |      100 |      100 |                   |
 revs.js          |      100 |      100 |      100 |      100 |                   |
 should-retry.js  |      100 |      100 |      100 |      100 |                   |
 spawn.js         |      100 |      100 |      100 |      100 |                   |
 which.js         |      100 |      100 |      100 |      100 |                   |
------------------|----------|----------|----------|----------|-------------------|

> @npmcli/git@2.0.8 posttest
> npm run lint


> @npmcli/git@2.0.8 lint
> standard

Just to be extra sure, when the shell: false is removed from opts.js and the test is run again clone fails as in the original issue, which proves it is that one line that fixes it. A much better solution!

[wraithgar]

@ferdinando-ferreira That's awesome to hear! Pretty ironic that this solution came only a short while after we figured out the core problem w/ detecting cmd. Hopefully though you can use that conversation with your attempts to clean up nodejs itself with the same issues.

[ferdinando-ferreira]

Hopefully though you can use that conversation with your attempts to clean up nodejs itself with the same issues.

@wraithgar : The hardest part was getting node to compile on my Win system, after that I already got the solution on lib/child_process figured out from the beggining.

Here is a rough draft.

diff --git "a/lib/child_process.js" "b/lib/child_process.js"
index 26e1bb33d0..a01c2dd8a6 100644
--- "a/lib/child_process.js"
+++ "b/lib/child_process.js"
@@ -502,27 +502,39 @@ function normalizeSpawnArguments(file, args, options) {
   }
 
   if (options.shell) {
-    const command = ArrayPrototypeJoin([file, ...args], ' ');
+    let shell;
+    let useWin32Cmd = false;
     // Set the shell, switches, and commands.
     if (process.platform === 'win32') {
       if (typeof options.shell === 'string')
-        file = options.shell;
+        shell = options.shell;
       else
-        file = process.env.comspec || 'cmd.exe';
-      // '/d /s /c' is used only for cmd.exe.
-      if (RegExpPrototypeTest(/^(?:.*\\)?cmd(?:\.exe)?$/i, file)) {
-        args = ['/d', '/s', '/c', `"${command}"`];
-        windowsVerbatimArguments = true;
-      } else {
-        args = ['-c', command];
+        shell = process.env.comspec || 'cmd.exe';
+      if (RegExpPrototypeTest(/^(?:.*\\)?cmd(?:\.exe)?$/i, shell)) {
+        useWin32Cmd = true;
       }
     } else {
       if (typeof options.shell === 'string')
-        file = options.shell;
+        shell = options.shell;
       else if (process.platform === 'android')
-        file = '/system/bin/sh';
+        shell = '/system/bin/sh';
       else
-        file = '/bin/sh';
+        shell = '/bin/sh';
+    }
+    // escaping is used only for cmd.exe.
+    const needsEscaping = (arg) => useWin32Cmd && !(/^"/.test(String(arg)));
+    // Unless the argument was explicitly made an ArgFilePath with the 
+    // use of escapeFilePath, processArgs is a noop
+    const processArgs = (arg) => (!(arg instanceof ArgFilePath))
+      ? arg 
+      : (needsEscaping(arg) ? '"' + arg + '"' : String(arg));
+    const command = ArrayPrototypeJoin([file, ...args.map(processArgs)], ' ');
+    file = shell;
+    // '/d /s /c' is used only for cmd.exe.
+    if (useWin32Cmd) {
+      args = ['/d', '/s', '/c', `"${command}"`];
+      windowsVerbatimArguments = true;
+    } else {
       args = ['-c', command];
     }
   }
@@ -776,6 +788,24 @@ function sanitizeKillSignal(killSignal) {
   }
 }
 
+// A wrapper for a filePath whose  purpose is to signal that an argument is
+// a filePath to be used by spawn other functions that make use of 
+// normalizeSpawnArguments
+class ArgFilePath {
+  constructor(filePath) {
+    this.filePath = filePath;
+  }
+
+  toString() {
+    return `${this.filePath}`;
+  }
+}
+
+function escapeFilePath(filePath) {
+  validateString(filePath, 'filePath');
+  return new ArgFilePath(filePath);
+}
+
 module.exports = {
   _forkChild,
   ChildProcess,
@@ -785,5 +815,6 @@ module.exports = {
   execSync,
   fork,
   spawn,
-  spawnSync
+  spawnSync,
+  escapeFilePath
 };

Here is the test case (that would only be applicable for WIndows):

const { spawnSync, escapeFilePath } = require('child_process');
let dirName = escapeFilePath('c:\\Program Files\\');
console.log('\n****************************************');
console.log('Testing with escapeFilePath');
console.log('****************************************\n');
spawnSync('dir', [dirName], {shell: 'cmd', stdio: 'inherit'});
console.log('\n****************************************');
console.log('Testing without escapeFilePath');
dirName = 'c:\\Program Files\\';
console.log('****************************************\n');
spawnSync('dir', [dirName], {shell: 'cmd', stdio: 'inherit'});

Executing that using the changed node with the patch above, prints

****************************************
Testing with escapeFilePath
****************************************

 Volume in drive C has no label.
 Volume Serial Number is <redacted>

 Directory of c:\Program Files

2021-04-15  16:11    <DIR>          .
2021-04-15  16:11    <DIR>          ..
2018-12-10  12:29    <DIR>          <assorted files populating the folder ... >

****************************************
Testing without escapeFilePath
****************************************

The system cannot find the file specified.

Technically that's a sound solution:

• it is "opt in" and needs explicit action by the downstream developers with the default behaviour being "no change"
• it doesn't add any breaking change or performance hit
• the scope is very limited and the change easy to understand
• it doesn't depend on any "internal" behaviour being exposed, considering escapeFilePath merely wraps the string into a class to ensure it can be identified internally.

I'll try to polish it a little to ensure no friction and will post a bug report plus a PR on nodejs.

With that said, in the great majority of the projects that can hit this bug the solution is to force shell: false in the opts and not use the shell at all in the first place (given that there is already a function on child_process to spawn the shell and call a command, that being exec).

But I am digressing and taking your folks valuable brain cycles, I'll present this case in the proper venue and see if they accept the proposed changes.