Drop the custom-binary character allowlist

.changeset/custom-binary-allowlist-removed.md

@@ -0,0 +1,7 @@
+---
+"simple-git": minor
+---
+
+Removed the character allowlist that restricted the `binary` / `customBinary` value to a narrow set of characters. The git binary is spawned directly with `shell: false`, so shell metacharacters and unusual path characters are never interpreted by a shell and cannot be used for command injection; the allowlist provided no protection and only served to reject legitimate binaries (e.g. paths containing a space such as `C:\Program Files\Git\bin\git.exe`).
+
+The `unsafe.allowUnsafeCustomBinary` option is now deprecated and is a no-op (it previously bypassed the removed allowlist); passing it emits a deprecation warning.

docs/PLUGIN-CUSTOM-BINARY.md

@@ -55,14 +55,22 @@ git init
 
 ### Caveats / Security
 
-To prevent accidentally merging arbitrary code into the spawned child processes, the strings supplied
-in the `binary` config are limited to alphanumeric, slashes, dot, hyphen and underscore. Colon is also
-permitted when part of a valid windows path (ie: after one letter at the start of the string).
+The `binary` value is never executed through a shell - it is spawned directly with Node's
+default `shell: false` (the `spawnOptions` config only forwards `uid`/`gid`, so `shell` cannot
+be set via user options). Any value you supply is treated as a literal command or argument, so
+shell metacharacters (`; | & $ ( )`) are inert and cannot be used for command injection; a bad
+path simply fails at spawn if it is wrong. There is no character allowlist to bypass, so the
+previously documented restriction on the `binary` string no longer applies.
 
-This protection can be overridden by passing an additional unsafe configuration setting:
+The `unsafe.allowUnsafeCustomBinary` option is **deprecated and is now a no-op**. It used to
+bypass the old character allowlist, which has been removed; passing it emits a deprecation
+warning and otherwise has no effect.
 
 ```typescript
-// this would normally throw because of the invalid value for `binary` 
+// `!` is now accepted as-is (previously required the deprecated override below)
+simpleGit({ binary: '!' });
+
+// still works, but is deprecated - it only emits a warning and does not change behaviour
 simpleGit({
    unsafe: {
       allowUnsafeCustomBinary: true

simple-git/src/lib/plugins/custom-binary.plugin.ts

@@ -5,27 +5,22 @@ import { asArray } from '../utils';
 import { PluginStore } from './plugin-store';
 
 const WRONG_NUMBER_ERR = `Invalid value supplied for custom binary, requires a single string or an array containing either one or two strings`;
-const WRONG_CHARS_ERR = `Invalid value supplied for custom binary, restricted characters must be removed or supply the unsafe.allowUnsafeCustomBinary option`;
+const EMPTY_ARG_ERR = `Invalid value supplied for custom binary, each element must be a non-empty string`;
 
 function isBadArgument(arg: string) {
-   return !arg || !/^([a-z]:)?([a-z0-9/.\\_~-]+)$/i.test(arg);
+   return !arg;
 }
 
 function toBinaryConfig(
-   input: string[],
-   allowUnsafe: boolean
+   input: string[]
 ): { binary: string; prefix?: string } {
    if (input.length < 1 || input.length > 2) {
       throw new GitPluginError(undefined, 'binary', WRONG_NUMBER_ERR);
    }
 
    const isBad = input.some(isBadArgument);
    if (isBad) {
-      if (allowUnsafe) {
-         console.warn(WRONG_CHARS_ERR);
-      } else {
-         throw new GitPluginError(undefined, 'binary', WRONG_CHARS_ERR);
-      }
+      throw new GitPluginError(undefined, 'binary', EMPTY_ARG_ERR);
    }
 
    const [binary, prefix] = input;
@@ -40,10 +35,19 @@ export function customBinaryPlugin(
    input: SimpleGitOptions['binary'] = ['git'],
    allowUnsafe = false
 ) {
-   let config = toBinaryConfig(asArray(input), allowUnsafe);
+   if (allowUnsafe) {
+      // Retained for backwards compatibility: `unsafe.allowUnsafeCustomBinary` used
+      // to bypass the character allowlist, which no longer exists. The option now
+      // has no effect, so warn to nudge users off it.
+      console.warn(
+         `simple-git: the unsafe.allowUnsafeCustomBinary option is deprecated and no longer has any effect - the custom binary is spawned directly with shell: false, so there is no character allowlist to bypass.`
+      );
+   }
+
+   let config = toBinaryConfig(asArray(input));
 
    plugins.on('binary', (input) => {
-      config = toBinaryConfig(asArray(input), allowUnsafe);
+      config = toBinaryConfig(asArray(input));
    });
 
    plugins.append('spawn.binary', () => {

simple-git/src/lib/types/index.ts

@@ -133,8 +133,9 @@ export interface SimpleGitPluginConfig {
    unsafe: Partial<
       VulnerabilityCategoryFlags & {
          /**
-          * Allows potentially unsafe values to be supplied in the `binary` configuration option and
-          * `git.customBinary()` method call.
+          * @deprecated No longer has any effect. The custom binary is spawned
+          * directly with `shell: false`, so the character allowlist this option
+          * used to bypass no longer exists.
           */
          allowUnsafeCustomBinary: boolean;
       }

simple-git/test/unit/plugins/plugin.binary.spec.ts

@@ -26,18 +26,20 @@ describe('binaryPlugin', () => {
       expect(await expected()).toEqual([binary, 'hello']);
    });
 
+   // The binary is spawned directly with `shell: false`, so shell metacharacters,
+   // spaces, quotes and Windows drive-colon paths are inert - they cannot be used
+   // for command injection and are passed through to spawn as-is (a bad path simply
+   // fails at spawn). The previous character allowlist wrongly rejected these.
    each(
       'long:\\path\\git.exe',
       'space fail',
       '"dquote fail"',
       "'squote fail'",
       '$',
       '!'
-   )('rejects invalid syntax "%s"', async (binary) => {
-      assertGitError(
-         await promiseError((async () => newSimpleGit({ binary }).raw('hello'))()),
-         'Invalid value supplied for custom binary'
-      );
+   )('allows previously-rejected syntax "%s"', async (binary) => {
+      newSimpleGit({ binary }).raw('hello');
+      expect(await expected()).toEqual([binary, 'hello']);
    });
 
    it('works with config plugin', async () => {
@@ -56,17 +58,20 @@ describe('binaryPlugin', () => {
       expect(await expected()).toEqual(['abc', 'def', 'g']);
    });
 
-   it('rejects reconfiguring to an invalid binary', async () => {
+   it('rejects reconfiguring to an empty binary', async () => {
       const git = newSimpleGit().raw('a');
       expect(await expected()).toEqual(['git', 'a']);
 
       assertGitError(
-         await promiseError((async () => git.customBinary('not valid'))()),
+         await promiseError((async () => git.customBinary(''))()),
          'Invalid value supplied for custom binary'
       );
    });
 
-   it('allows configuring to bad values when overridden', async () => {
+   // `unsafe.allowUnsafeCustomBinary` is deprecated and a no-op: it used to bypass
+   // the character allowlist, which has been removed. The values below are allowed
+   // regardless, and passing the flag just emits a deprecation warning.
+   it('allows shell metacharacters (allowUnsafeCustomBinary is now a no-op)', async () => {
       const git = newSimpleGit({ unsafe: { allowUnsafeCustomBinary: true }, binary: '$' }).raw('a');
       expect(await expected()).toEqual(['$', 'a']);