From 32f81417e21837ec15ebad2142f20ddd53f300e5 Mon Sep 17 00:00:00 2001 From: Derek Moore Date: Sat, 1 Aug 2026 21:34:47 -0500 Subject: [PATCH 1/2] Drop the custom-binary character allowlist Removes the regex validation in customBinaryPlugin that restricted the git binary/prefix to /^([a-z]:)?([a-z0-9/._~-]+)$/i. Why it is safe to remove: - The binary is always spawned directly by the OS with shell: false (the spawn.options plugin only forwards uid/gid, so shell cannot be enabled via user options). Shell metacharacters (; | & $ ( )) are therefore never interpreted by a shell - they are inert, and cannot be used for command injection. The allowlist provided no protection that shell: false did not already provide. - The allowlist was actively harmful: its character set was too narrow and rejected legitimate paths such as C:\Program Files\Git\bin\git.exe (the space in the path is outside the allowed charset). What changes: - toBinaryConfig now only validates shape (1-2 non-empty string elements); the per-character regex check is gone. Previously-rejected values pass through to spawn as-is. - unsafe.allowUnsafeCustomBinary previously bypassed the allowlist; with the allowlist removed it is retained for backwards compatibility but is now a no-op that emits a deprecation warning when supplied (validation always throws on an empty element regardless of the flag). The type is marked @deprecated. - Docs updated: the Caveats/Security section no longer claims a character allowlist, and documents the option as deprecated/no-op. Tests updated: syntax the old allowlist rejected (spaces, quotes, shell metacharacters, Windows drive-colon paths) is now asserted to pass through to spawn. Full unit suite: 634 passed. Build green. --- docs/PLUGIN-CUSTOM-BINARY.md | 18 +++++++++---- .../src/lib/plugins/custom-binary.plugin.ts | 26 +++++++++++-------- simple-git/src/lib/types/index.ts | 5 ++-- .../test/unit/plugins/plugin.binary.spec.ts | 21 +++++++++------ 4 files changed, 44 insertions(+), 26 deletions(-) diff --git a/docs/PLUGIN-CUSTOM-BINARY.md b/docs/PLUGIN-CUSTOM-BINARY.md index 9285eaca..4d390c1a 100644 --- a/docs/PLUGIN-CUSTOM-BINARY.md +++ b/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 diff --git a/simple-git/src/lib/plugins/custom-binary.plugin.ts b/simple-git/src/lib/plugins/custom-binary.plugin.ts index 18119090..7bf104cb 100644 --- a/simple-git/src/lib/plugins/custom-binary.plugin.ts +++ b/simple-git/src/lib/plugins/custom-binary.plugin.ts @@ -5,15 +5,14 @@ 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); @@ -21,11 +20,7 @@ function toBinaryConfig( 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', () => { diff --git a/simple-git/src/lib/types/index.ts b/simple-git/src/lib/types/index.ts index f4f36cdd..ee923d05 100644 --- a/simple-git/src/lib/types/index.ts +++ b/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; } diff --git a/simple-git/test/unit/plugins/plugin.binary.spec.ts b/simple-git/test/unit/plugins/plugin.binary.spec.ts index d1969a15..9a25c1c0 100644 --- a/simple-git/test/unit/plugins/plugin.binary.spec.ts +++ b/simple-git/test/unit/plugins/plugin.binary.spec.ts @@ -26,6 +26,10 @@ 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', @@ -33,11 +37,9 @@ describe('binaryPlugin', () => { "'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']); From 4fa5b24cad88fa18ad0c5fe93f1b1add0cdedde3 Mon Sep 17 00:00:00 2001 From: Derek Moore Date: Sat, 1 Aug 2026 22:37:08 -0500 Subject: [PATCH 2/2] Add changeset for custom-binary allowlist removal --- .changeset/custom-binary-allowlist-removed.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changeset/custom-binary-allowlist-removed.md diff --git a/.changeset/custom-binary-allowlist-removed.md b/.changeset/custom-binary-allowlist-removed.md new file mode 100644 index 00000000..93773f3c --- /dev/null +++ b/.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.