From a0d0e06f50e5e4c762e8c00acadb0bc07da69006 Mon Sep 17 00:00:00 2001
From: omrilotan <omri@hamadgera.com>
Date: Sun, 24 Jan 2021 20:35:07 +0200
Subject: [PATCH] Use spawn with git to avoid shell script vulnerabilities

---
 helpers/spawn/index.js  | 38 ++++++++++++++++++++++++++++++
 index.js                | 27 +++++++++++-----------
 lib/index.js            |  1 -
 lib/list/index.js       | 15 ------------
 lib/list/spec.js        | 51 -----------------------------------------
 lib/modified/index.js   |  4 ++--
 lib/remote/index.js     |  4 ++--
 lib/remote/spec.js      |  6 ++---
 lib/reset/index.js      | 12 +++++++---
 lib/reset/spec.js       | 18 +++++++--------
 lib/tag/index.js        | 10 ++++----
 lib/tag/spec.js         | 10 ++++----
 lib/unadded/index.js    |  4 ++--
 lib/unadded/spec.js     |  2 +-
 package.json            |  7 +++---
 spec.js                 | 21 +----------------
 vulnerabilities/spec.js |  8 +++++++
 17 files changed, 102 insertions(+), 136 deletions(-)
 create mode 100644 helpers/spawn/index.js
 delete mode 100644 lib/list/index.js
 delete mode 100644 lib/list/spec.js

diff --git a/helpers/spawn/index.js b/helpers/spawn/index.js
new file mode 100644
index 0000000..e587cad
--- /dev/null
+++ b/helpers/spawn/index.js
@@ -0,0 +1,38 @@
+const { spawn } = require('child_process');
+
+/**
+ * Executes the script and resolves with the output message
+ * @param  {String}  script
+ * @return {Promise}
+ */
+module.exports = rest => new Promise(
+	(resolve, reject) => {
+		const git = spawn(
+			'git',
+			rest.split(' '),
+		);
+
+		const outputs = { stdout: [], stderr: [] };
+
+		git.stdout.on(
+			'data',
+			data => outputs.stdout.push(data.toString()),
+		);
+
+		git.stderr.on(
+			'data',
+			data => outputs.stderr.push(data.toString()),
+		);
+
+		git.on('exit', code => {
+			switch (code) {
+				case 0:
+					resolve(outputs.stdout.join('').trim());
+					break;
+				default:
+					reject(outputs.stderr.join('').trim());
+					break;
+			}
+		});
+	},
+);
diff --git a/index.js b/index.js
index 5249db0..fb77471 100644
--- a/index.js
+++ b/index.js
@@ -1,7 +1,6 @@
-const exec = require('async-execute');
+const spawn = require('./helpers/spawn');
 
 const {
-	list,
 	modified,
 	remote,
 	reset,
@@ -30,8 +29,8 @@ const formats = [
  * @type {Array[]}
  */
 const outputs = [
-	[ 'branch', 'git rev-parse --abbrev-ref HEAD' ],
-	[ 'origin', 'git remote get-url origin' ],
+	[ 'branch', 'rev-parse --abbrev-ref HEAD' ],
+	[ 'origin', 'remote get-url origin' ],
 ];
 
 /**
@@ -39,11 +38,11 @@ const outputs = [
  * @type {Array[]}
  */
 const lists = [
-	[ 'changed', 'git diff-tree --no-commit-id --name-only -r HEAD' ],
-	[ 'staged', 'git diff --name-only --cached' ],
-	[ 'tags', 'git tag' ],
-	[ 'unstaged', 'git diff --name-only' ],
-	[ 'untracked', 'git ls-files -o --exclude-standard' ],
+	[ 'changed', 'diff-tree --no-commit-id --name-only -r HEAD' ],
+	[ 'staged', 'diff --name-only --cached' ],
+	[ 'tags', 'tag' ],
+	[ 'unstaged', 'diff --name-only' ],
+	[ 'untracked', 'ls-files -o --exclude-standard' ],
 ];
 
 /**
@@ -51,20 +50,20 @@ const lists = [
  */
 const getters = Object.assign(
 	{
-		date: async () => new Date(parseInt(await exec('git show -s --format=%at')) * 1000),
+		date: async () => new Date(parseInt(await spawn('show -s --format=%at')) * 1000),
 		name: async () => (await remote()).name,
 		owner: async () => (await remote()).owner,
 		unadded,
-		version: async () => (await exec('git version')).split(' ').pop(),
+		version: async () => (await spawn('version')).split(' ').pop(),
 	},
 	...outputs.map(
-		([ key, value ]) => ({ [key]: exec.bind(null, value) }),
+		([ key, value ]) => ({ [key]: spawn.bind(null, value) }),
 	),
 	...lists.map(
-		([ key, value ]) => ({ [key]: list.bind(null, value) }),
+		([ key, value ]) => ({ [key]: async () => (await spawn(value)).split('\n') }),
 	),
 	...formats.map(
-		([ key, value ]) => ({ [key]: exec.bind(null, `git show -s --format=%${value}`) }),
+		([ key, value ]) => ({ [key]: spawn.bind(null, `show -s --format=%${value}`) }),
 	),
 );
 
diff --git a/lib/index.js b/lib/index.js
index 73d8d0a..3de9933 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -1,5 +1,4 @@
 module.exports = {
-	list: require('./list'),
 	modified: require('./modified'),
 	remote: require('./remote'),
 	reset: require('./reset'),
diff --git a/lib/list/index.js b/lib/list/index.js
deleted file mode 100644
index 83a42fa..0000000
--- a/lib/list/index.js
+++ /dev/null
@@ -1,15 +0,0 @@
-const exec = require('async-execute');
-
-/**
- * List the output lines from command
- * @return {string[]}
- */
-module.exports = async function list(command) {
-	const output = await exec(command);
-
-	return output
-		.split('\n')
-		.map(item => item.trim())
-		.filter(Boolean)
-	;
-};
diff --git a/lib/list/spec.js b/lib/list/spec.js
deleted file mode 100644
index 19ff095..0000000
--- a/lib/list/spec.js
+++ /dev/null
@@ -1,51 +0,0 @@
-const { clean, override } = abuser(__filename);
-
-const diff = `
-		line 1
-		line 2
-line 3
-
-line 5
-`;
-
-describe('lib/list', async() => {
-	const exec = stub();
-	let list;
-
-	before(() => {
-		clean('.');
-		override('async-execute', exec);
-		exec.returns(diff);
-		list = require('.').bind(null, 'some command');
-	});
-	afterEach(() => exec.resetHistory());
-	after(() => clean('.'));
-
-	it('Should call exec with passed command', async() => {
-		await list();
-		expect(exec).to.have.been.calledWith('some command');
-	});
-	it('Should hard reset to a given sha', async() => {
-		const result = await list();
-		expect(result).to.deep.equal([
-			'line 1',
-			'line 2',
-			'line 3',
-			'line 5',
-		]);
-	});
-	it('Should return an empty array for empty output', async() => {
-		exec.returns('');
-		const result = await list();
-		expect(result).to.be.an('array');
-		expect(result).to.have.lengthOf(0);
-	});
-	it('Should filter out empty lines', async() => {
-		exec.returns(`
-
-		`);
-		const result = await list();
-		expect(result).to.be.an('array');
-		expect(result).to.have.lengthOf(0);
-	});
-});
diff --git a/lib/modified/index.js b/lib/modified/index.js
index d70f34a..d1aa07f 100644
--- a/lib/modified/index.js
+++ b/lib/modified/index.js
@@ -1,6 +1,6 @@
 const { resolve } = require('path');
-const exec = require('async-execute');
 const exist = require('@does/exist');
+const spawn = require('../../helpers/spawn');
 
 /**
  * Create and push a git tag with the last commit message
@@ -18,7 +18,7 @@ module.exports = async function(path) {
 		throw new Error(`Could not find file at path "${absolute}"`);
 	}
 
-	const ts = await exec(`git log -1 --format="%at" -- ${path}`);
+	const ts = await spawn(`log -1 --format="%at" -- ${path}`);
 
 	return new Date(Number(ts) * 1000);
 };
diff --git a/lib/remote/index.js b/lib/remote/index.js
index 07218a0..302e8a3 100644
--- a/lib/remote/index.js
+++ b/lib/remote/index.js
@@ -1,11 +1,11 @@
-const exec = require('async-execute');
+const spawn = require('../../helpers/spawn');
 
 /**
  * Get remote repo name and owner
  * @return {string{}}
  */
 module.exports = async function remote() {
-	const origin = await exec('git remote get-url origin');
+	const origin = await spawn('remote get-url origin');
 	const nosuffix = origin.replace(/\.git$/, '');
 	const [ match ] = nosuffix.match(/[\w-]*\/[\w-]+$/) || [ nosuffix ];
 
diff --git a/lib/remote/spec.js b/lib/remote/spec.js
index 9a8ee2d..8a05e30 100644
--- a/lib/remote/spec.js
+++ b/lib/remote/spec.js
@@ -2,7 +2,7 @@ const { clean, override } = abuser(__filename);
 
 function cleanup() {
 	clean('.');
-	clean('async-execute');
+	clean('../../helpers/spawn');
 }
 
 describe('lib/remote', () => {
@@ -23,7 +23,7 @@ describe('lib/remote', () => {
 		'ssh://[username@]repo_owner1/repo-name',
 	].forEach(
 		format => it(`should find owner and repo name in ${format}`, async () => {
-			override('async-execute', () => format);
+			override('../../helpers/spawn', () => format);
 
 			const remote = require('.');
 			const { owner, name } = await remote();
@@ -40,7 +40,7 @@ describe('lib/remote', () => {
 		format => it(
 			`Still returns reponame when no owner is found (${format})`,
 			async () => {
-				override('async-execute', async () => format);
+				override('../../helpers/spawn', async () => format);
 
 				const remote = require('.');
 				const { owner, name } = await remote();
diff --git a/lib/reset/index.js b/lib/reset/index.js
index dd1250a..cc78817 100644
--- a/lib/reset/index.js
+++ b/lib/reset/index.js
@@ -1,4 +1,4 @@
-const exec = require('async-execute');
+const spawn = require('../../helpers/spawn');
 
 /**
  * Reset current HEAD to the specified state
@@ -8,11 +8,17 @@ const exec = require('async-execute');
  */
 module.exports = async function(destination, { hard = true } = {}) {
 	if (destination && typeof destination === 'string') {
-		return await exec(`git reset ${JSON.stringify(destination)} ${hard ? '--hard' : ''}`);
+		try {
+			await spawn(`check-ref-format ${destination}`);
+		} catch (error) {
+			throw new RangeError('can not reset to illegal ref "${destination}"');
+		}
+
+		return await spawn(`reset ${destination} ${hard ? '--hard' : ''}`);
 	}
 
 	if (destination && typeof destination === 'number') {
-		return await exec(`git reset HEAD~${Math.abs(destination)} ${hard ? '--hard' : ''}`);
+		return await spawn(`reset HEAD~${Math.abs(destination)} ${hard ? '--hard' : ''}`);
 	}
 
 	throw new TypeError(`No case for handling destination ${destination} (${typeof destination})`);
diff --git a/lib/reset/spec.js b/lib/reset/spec.js
index f6232f6..09a12ed 100644
--- a/lib/reset/spec.js
+++ b/lib/reset/spec.js
@@ -6,7 +6,7 @@ describe('lib/reset', async() => {
 
 	before(() => {
 		clean('.');
-		override('async-execute', exec);
+		override('../../helpers/spawn', exec);
 		reset = require('.');
 	});
 	afterEach(() => exec.resetHistory());
@@ -31,22 +31,22 @@ describe('lib/reset', async() => {
 	});
 
 	it('Should hard reset to a given sha', async() => {
-		reset('shaid');
-		expect(exec.getCall(0).args[0]).to.equal('git reset "shaid" --hard');
+		await reset('shaid');
+		expect(exec.getCall(1).args[0]).to.equal('reset shaid --hard');
 	});
 
 	it('Should hard reset to n commits back', async() => {
-		reset(1);
-		expect(exec.getCall(0).args[0]).to.equal('git reset HEAD~1 --hard');
+		await reset(1);
+		expect(exec.getCall(0).args[0]).to.equal('reset HEAD~1 --hard');
 	});
 
 	it('Should hard reset to n commits back with negative value as well', async() => {
-		reset(-3);
-		expect(exec.getCall(0).args[0]).to.equal('git reset HEAD~3 --hard');
+		await reset(-3);
+		expect(exec.getCall(0).args[0]).to.equal('reset HEAD~3 --hard');
 	});
 
 	it('Should reset w/o hard argument', async() => {
-		reset('shaid', { hard: false });
-		expect(exec.getCall(0).args[0].trim()).to.equal('git reset "shaid"');
+		await reset('shaid', { hard: false });
+		expect(exec.getCall(1).args[0].trim()).to.equal('reset shaid');
 	});
 });
diff --git a/lib/tag/index.js b/lib/tag/index.js
index 4d1e784..a4e6e37 100644
--- a/lib/tag/index.js
+++ b/lib/tag/index.js
@@ -1,4 +1,4 @@
-const exec = require('async-execute');
+const spawn = require('../../helpers/spawn');
 
 /**
  * Create and push a git tag with the last commit message
@@ -13,9 +13,9 @@ module.exports = async function(tag) {
 	const { message, author, email } = this;
 
 	await Promise.all([
-		exec(`git config user.name "${await author}"`),
-		exec(`git config user.email "${await email}"`),
+		spawn(`config user.name "${await author}"`),
+		spawn(`config user.email "${await email}"`),
 	]);
-	await exec(`git tag -a ${JSON.stringify(tag)} -m "${await message}"`);
-	await exec(`git push origin ${JSON.stringify(`refs/tags/${tag}`)}`);
+	await spawn(`tag -a ${tag} -m "${await message}"`);
+	await spawn(`push origin refs/tags/${tag}`);
 };
diff --git a/lib/tag/spec.js b/lib/tag/spec.js
index 528bfc1..24d7bf0 100644
--- a/lib/tag/spec.js
+++ b/lib/tag/spec.js
@@ -6,7 +6,7 @@ describe('lib/tag', async() => {
 
 	before(() => {
 		clean('.');
-		override('async-execute', (...args) => dummy.stub(...args));
+		override('../../helpers/spawn', (...args) => dummy.stub(...args));
 
 		gitTag = require('.').bind({
 			message: 'this is a message',
@@ -22,8 +22,8 @@ describe('lib/tag', async() => {
 		dummy.stub = command => lines.push(command);
 
 		await gitTag('1.1.1');
-		expect(lines).to.include('git config user.name "boaty mcboatface"');
-		expect(lines).to.include('git config user.email "boaty@boat.face"');
+		expect(lines).to.include('config user.name "boaty mcboatface"');
+		expect(lines).to.include('config user.email "boaty@boat.face"');
 	});
 
 	it('Should create a git tag with given message and push it', async() => {
@@ -31,7 +31,7 @@ describe('lib/tag', async() => {
 		dummy.stub = command => lines.push(command);
 
 		await gitTag('1.1.1');
-		expect(lines).to.include('git tag -a "1.1.1" -m "this is a message"');
-		expect(lines).to.include('git push origin "refs/tags/1.1.1"');
+		expect(lines).to.include('tag -a 1.1.1 -m "this is a message"');
+		expect(lines).to.include('push origin refs/tags/1.1.1');
 	});
 });
diff --git a/lib/unadded/index.js b/lib/unadded/index.js
index 1ebeff9..2414e55 100644
--- a/lib/unadded/index.js
+++ b/lib/unadded/index.js
@@ -1,11 +1,11 @@
-const exec = require('async-execute');
+const spawn = require('../../helpers/spawn');
 
 /**
  * List all files that would be added by "add ."
  * @return {string[]}
  */
 module.exports = async function unadded() {
-	const output = await exec('git add --dry-run .');
+	const output = await spawn('add --dry-run .');
 
 	return output
 		.split('\n')
diff --git a/lib/unadded/spec.js b/lib/unadded/spec.js
index 54c442a..740cbaa 100644
--- a/lib/unadded/spec.js
+++ b/lib/unadded/spec.js
@@ -5,7 +5,7 @@ describe('lib/unadded', () => {
 
 	before(() => {
 		clean('.');
-		override('async-execute', () => `remove '.gitattributes'
+		override('../../helpers/spawn', () => `remove '.gitattributes'
 add 'apps/post/index.js'
 add 'new file.txt'
 `);
diff --git a/package.json b/package.json
index 14c62da..d9518df 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "async-git",
-  "version": "1.13.1",
+  "version": "1.13.2",
   "description": "👾 Retrieve data from current git repository",
   "keywords": [
     "git",
@@ -25,17 +25,18 @@
   "homepage": "https://omrilotan.com/async-git/",
   "main": "index.js",
   "scripts": {
+    "pretest": "echo \"content\" > unadded.txt",
     "test": "mocha '**/spec.js' --require .mocha.js --recursive --exclude 'node_modules'",
     "lint": "eslint . --ext .js"
   },
   "dependencies": {
-    "@does/exist": "^1.1.0",
-    "async-execute": "^1.1.0"
+    "@does/exist": "^1.1.0"
   },
   "devDependencies": {
     "@lets/wait": "^2.0.2",
     "@omrilotan/eslint-config": "^1.1.0",
     "abuser": "^2.0.2",
+    "async-execute": "^1.2.0",
     "chai": "^4.2.0",
     "chai-as-promised": "^7.1.1",
     "deep-equal-in-any-order": "^1.0.21",
diff --git a/spec.js b/spec.js
index d7b58fc..c5c64ac 100644
--- a/spec.js
+++ b/spec.js
@@ -1,7 +1,6 @@
 const exec = require('async-execute');
 
-const { clean, override } = abuser(__filename);
-const list = stub();
+const { clean } = abuser(__filename);
 let git;
 
 const is = Object.defineProperty(
@@ -18,9 +17,6 @@ describe('async-git', async() => {
 	before(async() => {
 		clean('.');
 		clean('./lib');
-		override('./lib/list', list);
-		override('./lib/unadded', list);
-		list.returns([ 'a', 'b', 'c' ]);
 		git = require('.');
 
 		if (is.ci) { return; }
@@ -32,8 +28,6 @@ describe('async-git', async() => {
 		await exec('echo "content" > unadded.txt');
 	});
 
-	afterEach(() => list.resetHistory());
-
 	after(async() => {
 		clean('.');
 
@@ -99,18 +93,10 @@ describe('async-git', async() => {
 		);
 	});
 
-	// Lists
-
-	it('Should call list with relevant git command', async() => {
-		await git.changed;
-		expect(list).to.have.been.calledWith('git diff-tree --no-commit-id --name-only -r HEAD');
-	});
-
 	[
 		'changed',
 		'staged',
 		'tags',
-		'unadded',
 		'untracked',
 	].forEach(member => it(`${member} should retrieve an array`, async () => {
 		const value = await git[member];
@@ -118,11 +104,6 @@ describe('async-git', async() => {
 		expect(value).to.have.lengthOf.at.least(1);
 	}));
 
-	it('changed should call list with relevant git command', async() => {
-		await git.changed;
-		expect(list).to.have.been.calledWith('git diff-tree --no-commit-id --name-only -r HEAD');
-	});
-
 	it('changed should retrieve an array of strings', async () => {
 		const value = await git.changed;
 		expect(value).to.be.an('array');
diff --git a/vulnerabilities/spec.js b/vulnerabilities/spec.js
index fc312e4..4d5609c 100644
--- a/vulnerabilities/spec.js
+++ b/vulnerabilities/spec.js
@@ -31,6 +31,10 @@ async function softly(fn, ...args) {
 }
 
 describe('vulnerabilities', async() => {
+	before(async() => {
+		await wait(100);
+		await softly(unlink, 'HACKED');
+	});
 	afterEach(async() => {
 		await wait(100);
 		await softly(unlink, 'HACKED');
@@ -39,6 +43,10 @@ describe('vulnerabilities', async() => {
 		await softly(git.reset, '; touch HACKED #');
 		expect(await exists('HACKED')).to.be.false;
 	});
+	it('shell injection in reset', async() => {
+		await softly(git.reset, 'a`touch HACKED`b');
+		expect(await exists('HACKED')).to.be.false;
+	});
 	it('shell injection in tag', async() => {
 		await softly(git.tag, '; touch HACKED #');
 		expect(await exists('HACKED')).to.be.false;