From 871201ff8de6091d4b9b2ce9bf8611a1fc2f7836 Mon Sep 17 00:00:00 2001 From: Joe Bottigliero Date: Sat, 11 Jul 2020 22:32:33 -0500 Subject: [PATCH] Merge pull request from GHSA-7xcx-6wjh-7xp2 - Uses `execFile` with arguments instead of `exec` where possible. - See GHSL-2020-111 Co-authored-by: Benjamin E. Coe --- lib/lifecycles/commit.js | 29 ++++++++++++++++++++++------- lib/lifecycles/tag.js | 10 +++++----- lib/run-execFile.js | 20 ++++++++++++++++++++ test.js | 23 ++++++++++++++++++++--- 4 files changed, 67 insertions(+), 15 deletions(-) create mode 100644 lib/run-execFile.js diff --git a/lib/lifecycles/commit.js b/lib/lifecycles/commit.js index e8fe17c1f..a4bdb7909 100644 --- a/lib/lifecycles/commit.js +++ b/lib/lifecycles/commit.js @@ -2,7 +2,7 @@ const bump = require('../lifecycles/bump') const checkpoint = require('../checkpoint') const formatCommitMessage = require('../format-commit-message') const path = require('path') -const runExec = require('../run-exec') +const runExecFile = require('../run-execFile') const runLifecycleScript = require('../run-lifecycle-script') module.exports = function (args, newVersion) { @@ -20,20 +20,21 @@ module.exports = function (args, newVersion) { function execCommit (args, newVersion) { let msg = 'committing %s' let paths = [] - const verify = args.verify === false || args.n ? '--no-verify ' : '' - let toAdd = '' + const verify = args.verify === false || args.n ? ['--no-verify'] : [] + const sign = args.sign ? ['-S'] : [] + const toAdd = [] // only start with a pre-populated paths list when CHANGELOG processing is not skipped if (!args.skip.changelog) { paths = [args.infile] - toAdd += ' ' + args.infile + toAdd.push(args.infile) } // commit any of the config files that we've updated // the version # for. Object.keys(bump.getUpdatedConfigs()).forEach(function (p) { paths.unshift(p) - toAdd += ' ' + path.relative(process.cwd(), p) + toAdd.push(path.relative(process.cwd(), p)) // account for multiple files in the output message if (paths.length > 1) { @@ -53,8 +54,22 @@ function execCommit (args, newVersion) { return Promise.resolve() } - return runExec(args, 'git add' + toAdd) + return runExecFile(args, 'git', ['add'].concat(toAdd)) .then(() => { - return runExec(args, 'git commit ' + verify + (args.sign ? '-S ' : '') + (args.commitAll ? '' : (toAdd)) + ' -m "' + formatCommitMessage(args.releaseCommitMessageFormat, newVersion) + '"') + return runExecFile( + args, + 'git', + [ + 'commit' + ].concat( + verify, + sign, + args.commitAll ? [] : toAdd, + [ + '-m', + `"${formatCommitMessage(args.releaseCommitMessageFormat, newVersion)}"` + ] + ) + ) }) } diff --git a/lib/lifecycles/tag.js b/lib/lifecycles/tag.js index f6a210829..214e69c92 100644 --- a/lib/lifecycles/tag.js +++ b/lib/lifecycles/tag.js @@ -3,7 +3,7 @@ const chalk = require('chalk') const checkpoint = require('../checkpoint') const figures = require('figures') const formatCommitMessage = require('../format-commit-message') -const runExec = require('../run-exec') +const runExecFile = require('../run-execFile') const runLifecycleScript = require('../run-lifecycle-script') module.exports = function (newVersion, pkgPrivate, args) { @@ -20,13 +20,13 @@ module.exports = function (newVersion, pkgPrivate, args) { function execTag (newVersion, pkgPrivate, args) { let tagOption if (args.sign) { - tagOption = '-s ' + tagOption = '-s' } else { - tagOption = '-a ' + tagOption = '-a' } checkpoint(args, 'tagging release %s%s', [args.tagPrefix, newVersion]) - return runExec(args, 'git tag ' + tagOption + args.tagPrefix + newVersion + ' -m "' + formatCommitMessage(args.releaseCommitMessageFormat, newVersion) + '"') - .then(() => runExec('', 'git rev-parse --abbrev-ref HEAD')) + return runExecFile(args, 'git', ['tag', tagOption, args.tagPrefix + newVersion, '-m', `"${formatCommitMessage(args.releaseCommitMessageFormat, newVersion)}"`]) + .then(() => runExecFile('', 'git', ['rev-parse', '--abbrev-ref', 'HEAD'])) .then((currentBranch) => { let message = 'git push --follow-tags origin ' + currentBranch.trim() if (pkgPrivate !== true && bump.getUpdatedConfigs()['package.json']) { diff --git a/lib/run-execFile.js b/lib/run-execFile.js new file mode 100644 index 000000000..076f95134 --- /dev/null +++ b/lib/run-execFile.js @@ -0,0 +1,20 @@ +const { execFile } = require('child_process') +const printError = require('./print-error') + +module.exports = function (args, cmd, cmdArgs) { + if (args.dryRun) return Promise.resolve() + return new Promise((resolve, reject) => { + // Exec given cmd and handle possible errors + execFile(cmd, cmdArgs, function (err, stdout, stderr) { + // If exec returns content in stderr, but no error, print it as a warning + // If exec returns an error, print it and exit with return code 1 + if (err) { + printError(args, stderr || err.message) + return reject(err) + } else if (stderr) { + printError(args, stderr, { level: 'warn', color: 'yellow' }) + } + return resolve(stdout) + }) + }) +} diff --git a/test.js b/test.js index b170513c9..0a60ab82e 100644 --- a/test.js +++ b/test.js @@ -258,9 +258,10 @@ describe('cli', function () { const captured = shell.cat('gitcapture.log').stdout.split('\n').map(function (line) { return line ? JSON.parse(line) : line }) - captured[captured.length - 4].should.deep.equal(['commit', '-S', 'CHANGELOG.md', 'package.json', '-m', 'chore(release): 1.0.1']) - captured[captured.length - 3].should.deep.equal(['tag', '-s', 'v1.0.1', '-m', 'chore(release): 1.0.1']) - + /* eslint-disable no-useless-escape */ + captured[captured.length - 4].should.deep.equal(['commit', '-S', 'CHANGELOG.md', 'package.json', '-m', '\"chore(release): 1.0.1\"']) + captured[captured.length - 3].should.deep.equal(['tag', '-s', 'v1.0.1', '-m', '\"chore(release): 1.0.1\"']) + /* eslint-enable no-useless-escape */ unmock() }) }) @@ -1314,3 +1315,19 @@ describe('standard-version', function () { }) }) }) + +describe('GHSL-2020-111', function () { + beforeEach(initInTempFolder) + afterEach(finishTemp) + it('does not allow command injection via basic configuration', function () { + return standardVersion({ + silent: true, + noVerify: true, + infile: 'foo.txt', + releaseCommitMessageFormat: 'bla `touch exploit`' + }).then(function () { + const stat = shell.test('-f', './exploit') + stat.should.equal(false) + }) + }) +})