Clone API use pathspec (#1132)

* Use a `pathspec` wrapper for remote and local paths when using `git.clone` and `git.mirror`

Extract `clone` methods to `SimpleGitApi` base class.

* Fix block-unsafe test now handled with pathspec

* Restore ability to use `-b non-default-branch` in `git.clone`

Closes #1137

* Add tests for unsafe `-u` switch detection

* Add documentation for the use of pathspecs

Author: steveukx

.changeset/cold-moments-yell.md

@@ -0,0 +1,6 @@
+---
+"simple-git": minor
+---
+
+Use `pathspec` wrappers for remote and local paths when running either `git.clone` or `git.mirror` to
+avoid leaving them less open for unexpected outcomes when passing unsanitised data into these tasks. 

simple-git/readme.md

@@ -250,6 +250,8 @@ in v2 (deprecation notices were logged to `stdout` as `console.warn` in v2).
 
 -  `mirror(repoPath, [localPath, [options]])` behaves the same as the `.clone` interface with the [`--mirror` flag](https://git-scm.com/docs/git-clone#Documentation/git-clone.txt---mirror) enabled.
 
+Note: as of version 3.33.0, `repoPath` and `localPath` are passed to `git` as "pathspec" arguments so are less open to unexpected unsafe behaviour when passing unsanitised data into the command.
+
 ## git config
 
 -  `.addConfig(key, value, append = false, scope = 'local')` add a local configuration property, when `append` is set to

simple-git/src/git.js

@@ -23,7 +23,6 @@ const {
 } = require('./lib/tasks/branch');
 const { checkIgnoreTask } = require('./lib/tasks/check-ignore');
 const { checkIsRepoTask } = require('./lib/tasks/check-is-repo');
-const { cloneTask, cloneMirrorTask } = require('./lib/tasks/clone');
 const { cleanWithOptionsTask, isCleanOptionsArray } = require('./lib/tasks/clean');
 const { diffSummaryTask } = require('./lib/tasks/diff');
 const { fetchTask } = require('./lib/tasks/fetch');
@@ -101,34 +100,6 @@ Git.prototype.stashList = function (options) {
    );
 };
 
-function createCloneTask(api, task, repoPath, localPath) {
-   if (typeof repoPath !== 'string') {
-      return configurationErrorTask(`git.${api}() requires a string 'repoPath'`);
-   }
-
-   return task(repoPath, filterType(localPath, filterString), getTrailingOptions(arguments));
-}
-
-/**
- * Clone a git repo
- */
-Git.prototype.clone = function () {
-   return this._runTask(
-      createCloneTask('clone', cloneTask, ...arguments),
-      trailingFunctionArgument(arguments)
-   );
-};
-
-/**
- * Mirror a git repo
- */
-Git.prototype.mirror = function () {
-   return this._runTask(
-      createCloneTask('mirror', cloneMirrorTask, ...arguments),
-      trailingFunctionArgument(arguments)
-   );
-};
-
 /**
  * Moves one or more files to a new destination.
  *

simple-git/src/lib/git-factory.ts

@@ -59,12 +59,12 @@ export function gitInstanceFactory(
    }
 
    plugins.add(blockUnsafeOperationsPlugin(config.unsafe));
-   plugins.add(suffixPathsPlugin());
    plugins.add(completionDetectionPlugin(config.completion));
    config.abort && plugins.add(abortPlugin(config.abort));
    config.progress && plugins.add(progressMonitorPlugin(config.progress));
    config.timeout && plugins.add(timeoutPlugin(config.timeout));
    config.spawnOptions && plugins.add(spawnOptionsPlugin(config.spawnOptions));
+   plugins.add(suffixPathsPlugin());
 
    plugins.add(errorDetectionPlugin(errorDetectionHandler(true)));
    config.errors && plugins.add(errorDetectionPlugin(config.errors));

simple-git/src/lib/plugins/block-unsafe-operations-plugin.ts

@@ -7,13 +7,13 @@ function isConfigSwitch(arg: string | unknown) {
    return typeof arg === 'string' && arg.trim().toLowerCase() === '-c';
 }
 
-function isCloneSwitch(char: string, arg: string | unknown) {
+export function isCloneUploadPackSwitch(char: string, arg: string | unknown) {
    if (typeof arg !== 'string' || !arg.includes(char)) {
       return false;
    }
 
-   const token = arg.replace(/\0g/, '').replace(/^(--no)?-{1,2}/, '');
-   return /^[\dlsqvnobucj]+\b/.test(token);
+   const cleaned = arg.trim().replace(/\0/g, '');
+   return /^(--no)?-{1,2}[\dlsqvnobucj]+(\s|$)/.test(cleaned);
 }
 
 function preventConfigBuilder(
@@ -59,7 +59,7 @@ function preventUploadPack(arg: string, method: string) {
       );
    }
 
-   if (method === 'clone' && isCloneSwitch('u', arg)) {
+   if (method === 'clone' && isCloneUploadPackSwitch('u', arg)) {
       throw new GitPluginError(
          undefined,
          'unsafe',

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

@@ -1,5 +1,5 @@
 export * from './abort-plugin';
-export * from './block-unsafe-operations-plugin';
+export { blockUnsafeOperationsPlugin } from './block-unsafe-operations-plugin';
 export * from './command-config-prefixing-plugin';
 export * from './completion-detection.plugin';
 export * from './custom-binary.plugin';

simple-git/src/lib/simple-git-api.ts

@@ -24,6 +24,7 @@ import {
    getTrailingOptions,
    trailingFunctionArgument,
 } from './utils';
+import clone from './tasks/clone';
 
 export class SimpleGitApi implements SimpleGitBase {
    constructor(private _executor: SimpleGitExecutor) {}
@@ -144,6 +145,7 @@ export class SimpleGitApi implements SimpleGitBase {
 Object.assign(
    SimpleGitApi.prototype,
    checkout(),
+   clone(),
    commit(),
    config(),
    countObjects(),

simple-git/src/lib/tasks/clone.ts

@@ -1,6 +1,15 @@
 import { configurationErrorTask, EmptyTask, straightThroughStringTask } from './task';
 import { OptionFlags, Options, StringTask } from '../types';
-import { append, filterString } from '../utils';
+import {
+   append,
+   filterString,
+   filterType,
+   getTrailingOptions,
+   trailingFunctionArgument,
+} from '../utils';
+import { pathspec } from '../args/pathspec';
+import { SimpleGit } from '../../../typings';
+import { SimpleGitApi } from '../simple-git-api';
 
 export type CloneOptions = Options &
    OptionFlags<
@@ -29,34 +38,53 @@ export type CloneOptions = Options &
       string
    >;
 
-function disallowedCommand(command: string) {
-   return /^--upload-pack(=|$)/.test(command);
-}
-
-export function cloneTask(
+type CloneTaskBuilder = (
    repo: string | undefined,
    directory: string | undefined,
    customArgs: string[]
-): StringTask<string> | EmptyTask {
-   const commands = ['clone', ...customArgs];
+) => StringTask<string> | EmptyTask;
 
-   filterString(repo) && commands.push(repo);
-   filterString(directory) && commands.push(directory);
+export const cloneTask: CloneTaskBuilder = (repo, directory, customArgs) => {
+   const commands = ['clone', ...customArgs];
 
-   const banned = commands.find(disallowedCommand);
-   if (banned) {
-      return configurationErrorTask(`git.fetch: potential exploit argument blocked.`);
-   }
+   filterString(repo) && commands.push(pathspec(repo));
+   filterString(directory) && commands.push(pathspec(directory));
 
    return straightThroughStringTask(commands);
-}
+};
 
-export function cloneMirrorTask(
-   repo: string | undefined,
-   directory: string | undefined,
-   customArgs: string[]
-) {
+export const cloneMirrorTask: CloneTaskBuilder = (repo, directory, customArgs) => {
    append(customArgs, '--mirror');
 
    return cloneTask(repo, directory, customArgs);
+};
+
+function createCloneTask(
+   api: 'clone' | 'mirror',
+   task: CloneTaskBuilder,
+   repoPath: string | undefined,
+   ...args: unknown[]
+) {
+   if (!filterString(repoPath)) {
+      return configurationErrorTask(`git.${api}() requires a string 'repoPath'`);
+   }
+
+   return task(repoPath, filterType(args[0], filterString), getTrailingOptions(arguments));
+}
+
+export default function (): Pick<SimpleGit, 'clone' | 'mirror'> {
+   return {
+      clone(this: SimpleGitApi, repo: string | unknown, ...rest: unknown[]) {
+         return this._runTask(
+            createCloneTask('clone', cloneTask, filterType(repo, filterString), ...rest),
+            trailingFunctionArgument(arguments)
+         );
+      },
+      mirror(this: SimpleGitApi, repo: string | unknown, ...rest: unknown[]) {
+         return this._runTask(
+            createCloneTask('mirror', cloneMirrorTask, filterType(repo, filterString), ...rest),
+            trailingFunctionArgument(arguments)
+         );
+      },
+   };
 }

simple-git/src/lib/utils/argument-filters.ts

@@ -39,7 +39,7 @@ export const filterNumber: ArgumentFilterPredicate<number> = (input: unknown): i
 };
 
 export const filterString: ArgumentFilterPredicate<string> = (input: unknown): input is string => {
-   return typeof input === 'string';
+   return typeof input === 'string' || isPathSpec(input);
 };
 
 export const filterStringOrStringArray: ArgumentFilterPredicate<string | string[]> = (

simple-git/test/integration/plugin.unsafe.spec.ts

@@ -57,7 +57,7 @@ describe('plugin.unsafe', () => {
       });
 
       it('command injection report', async () => {
-         for (const i of [45, 54, 52, 45, 118, 115, 113, 110, 108]) {
+         for (const i of [45, 54, 52, 118, 115, 113, 110, 108]) {
             expectError(
                await promiseResult(
                   newSimpleGit({ baseDir: context.root })