From 378f256a528a7be07c9fa29b69f97e9cc526d1a5 Mon Sep 17 00:00:00 2001 From: Timothy Jones Date: Mon, 28 Oct 2019 16:01:25 +1100 Subject: [PATCH] fix(spawn): Now binaries are spawned directly with array arguments, rather than quoted strings. Should fix #118 --- package-lock.json | 106 ++++++++++++++++++++++++++++++++---- package.json | 2 + src/can-deploy.ts | 10 +++- src/message.ts | 2 +- src/publisher.ts | 5 +- src/server.ts | 39 +++++++------ src/service.ts | 46 +++++++++------- src/spawn/arguments.spec.ts | 22 ++++---- src/spawn/arguments.ts | 13 ++++- src/spawn/spawn.ts | 25 ++------- src/verifier.ts | 7 ++- 11 files changed, 190 insertions(+), 87 deletions(-) diff --git a/package-lock.json b/package-lock.json index d6fc2ae4..7b511857 100644 --- a/package-lock.json +++ b/package-lock.json @@ -290,6 +290,15 @@ "@types/express": "*" } }, + "@types/cross-spawn": { + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/@types/cross-spawn/-/cross-spawn-6.0.1.tgz", + "integrity": "sha512-MtN1pDYdI6D6QFDzy39Q+6c9rl2o/xN7aWGe6oZuzqq5N6+YuwFsWiEAv3dNzvzN9YzU+itpN8lBzFpphQKLAw==", + "dev": true, + "requires": { + "@types/node": "*" + } + }, "@types/debug": { "version": "4.1.5", "resolved": "https://registry.npmjs.org/@types/debug/-/debug-4.1.5.tgz", @@ -1934,16 +1943,41 @@ } }, "cross-spawn": { - "version": "6.0.5", - "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-6.0.5.tgz", - "integrity": "sha512-eTVLrBSt7fjbDygz805pMnstIs2VTBNkRm0qxZd+M7A5XDdxVRWO5MxGBXZhjY4cqLYLdtrGqRf8mBPmzwSpWQ==", - "dev": true, + "version": "7.0.1", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-7.0.1.tgz", + "integrity": "sha512-u7v4o84SwFpD32Z8IIcPZ6z1/ie24O6RU3RbtL5Y316l3KuHVPx9ItBgWQ6VlfAFnRnTtMUrsQ9MUUTuEZjogg==", "requires": { - "nice-try": "^1.0.4", - "path-key": "^2.0.1", - "semver": "^5.5.0", - "shebang-command": "^1.2.0", - "which": "^1.2.9" + "path-key": "^3.1.0", + "shebang-command": "^2.0.0", + "which": "^2.0.1" + }, + "dependencies": { + "path-key": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/path-key/-/path-key-3.1.0.tgz", + "integrity": "sha512-8cChqz0RP6SHJkMt48FW0A7+qUOn+OsnOsVtzI59tZ8m+5bCSk7hzwET0pulwOM2YMn9J1efb07KB9l9f30SGg==" + }, + "shebang-command": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/shebang-command/-/shebang-command-2.0.0.tgz", + "integrity": "sha512-kHxr2zZpYtdmrN1qDjrrX/Z1rR1kG8Dx+gkpK1G4eXmvXswmcE1hTWBWYUzlraYw1/yZp6YuDY77YtvbN0dmDA==", + "requires": { + "shebang-regex": "^3.0.0" + } + }, + "shebang-regex": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/shebang-regex/-/shebang-regex-3.0.0.tgz", + "integrity": "sha512-7++dFhtcx3353uBaq8DDR4NuxBetBzC7ZQOhmTQInHEd6bSrXdiEyzCvG07Z44UYdLShWUyXt5M/yhz8ekcb1A==" + }, + "which": { + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/which/-/which-2.0.1.tgz", + "integrity": "sha512-N7GBZOTswtB9lkQBZA4+zAXrjEIWAUOB93AvzUiudRzRxhUdLURQ7D/gAIMY1gatT/LTbmbcv8SiYazy3eYB7w==", + "requires": { + "isexe": "^2.0.0" + } + } } }, "crypto-random-string": { @@ -2579,6 +2613,27 @@ "integrity": "sha512-1apePfXM1UOSqw0o9IiFAovVz9M5S1Dg+4TrDwfMewQ6p/rmMueb7tWZjQ1rx4Loy1ArBggoqGpfqqdI4rondg==", "dev": true }, + "cross-spawn": { + "version": "6.0.5", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-6.0.5.tgz", + "integrity": "sha512-eTVLrBSt7fjbDygz805pMnstIs2VTBNkRm0qxZd+M7A5XDdxVRWO5MxGBXZhjY4cqLYLdtrGqRf8mBPmzwSpWQ==", + "dev": true, + "requires": { + "nice-try": "^1.0.4", + "path-key": "^2.0.1", + "semver": "^5.5.0", + "shebang-command": "^1.2.0", + "which": "^1.2.9" + }, + "dependencies": { + "semver": { + "version": "5.7.1", + "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.1.tgz", + "integrity": "sha512-sauaDf/PZdVgrLTNYHRtpXa1iRiKcaebiKQ1BJdpQlWH2lCvexQdX55snPFyK7QzpudqbCI0qXFfOasHdyNDGQ==", + "dev": true + } + } + }, "debug": { "version": "4.1.1", "resolved": "https://registry.npmjs.org/debug/-/debug-4.1.1.tgz", @@ -5099,8 +5154,7 @@ "isexe": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/isexe/-/isexe-2.0.0.tgz", - "integrity": "sha1-6PvzdNxVb/iUehDcsFctYz8s+hA=", - "dev": true + "integrity": "sha1-6PvzdNxVb/iUehDcsFctYz8s+hA=" }, "isobject": { "version": "3.0.1", @@ -6317,6 +6371,21 @@ "p-finally": "^1.0.0", "signal-exit": "^3.0.0", "strip-eof": "^1.0.0" + }, + "dependencies": { + "cross-spawn": { + "version": "6.0.5", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-6.0.5.tgz", + "integrity": "sha512-eTVLrBSt7fjbDygz805pMnstIs2VTBNkRm0qxZd+M7A5XDdxVRWO5MxGBXZhjY4cqLYLdtrGqRf8mBPmzwSpWQ==", + "dev": true, + "requires": { + "nice-try": "^1.0.4", + "path-key": "^2.0.1", + "semver": "^5.5.0", + "shebang-command": "^1.2.0", + "which": "^1.2.9" + } + } } }, "get-stream": { @@ -9606,6 +9675,21 @@ "p-finally": "^1.0.0", "signal-exit": "^3.0.0", "strip-eof": "^1.0.0" + }, + "dependencies": { + "cross-spawn": { + "version": "6.0.5", + "resolved": "https://registry.npmjs.org/cross-spawn/-/cross-spawn-6.0.5.tgz", + "integrity": "sha512-eTVLrBSt7fjbDygz805pMnstIs2VTBNkRm0qxZd+M7A5XDdxVRWO5MxGBXZhjY4cqLYLdtrGqRf8mBPmzwSpWQ==", + "dev": true, + "requires": { + "nice-try": "^1.0.4", + "path-key": "^2.0.1", + "semver": "^5.5.0", + "shebang-command": "^1.2.0", + "which": "^1.2.9" + } + } } }, "get-stream": { diff --git a/package.json b/package.json index 61561759..e256979a 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "bunyan-prettystream": "0.1.3", "chalk": "2.3.1", "check-types": "7.3.0", + "cross-spawn": "^7.0.1", "decompress": "4.2.0", "mkdirp": "0.5.1", "q": "1.5.1", @@ -74,6 +75,7 @@ "@types/chai-as-promised": "7.1.0", "@types/check-types": "^7.3.1", "@types/cors": "^2.8.6", + "@types/cross-spawn": "^6.0.1", "@types/decompress": "^4.2.3", "@types/express": "4.11.1", "@types/mkdirp": "^0.5.2", diff --git a/src/can-deploy.ts b/src/can-deploy.ts index 846f463f..b967d4bd 100644 --- a/src/can-deploy.ts +++ b/src/can-deploy.ts @@ -2,7 +2,7 @@ import q = require('q'); import logger from './logger'; import spawn from './spawn'; import pactStandalone from './pact-standalone'; -import { PACT_NODE_NO_VALUE } from './spawn'; +import { PACT_NODE_NO_VALUE, DEFAULT_ARG } from './spawn'; import * as _ from 'underscore'; // eslint-disable-next-line @typescript-eslint/no-var-requires @@ -38,6 +38,7 @@ export class CanDeploy { public readonly options: CanDeployOptions; private readonly __argMapping = { + cliVerb: DEFAULT_ARG, name: '--pacticipant', version: '--version', latest: '--latest', @@ -95,8 +96,11 @@ export class CanDeploy { ); const deferred = q.defer(); const instance = spawn.spawnBinary( - `${pactStandalone.brokerPath} can-i-deploy`, - CanDeploy.convertForSpawnBinary(this.options), + pactStandalone.brokerPath, + [ + { cliVerb: 'can-i-deploy' }, + ...CanDeploy.convertForSpawnBinary(this.options), + ], this.__argMapping, ); const output: Array = []; diff --git a/src/message.ts b/src/message.ts index 89d5da2f..2d29f2f4 100644 --- a/src/message.ts +++ b/src/message.ts @@ -88,7 +88,7 @@ export class Message { logger.info(`Creating message pact`); const deferred = q.defer(); const instance = spawn.spawnBinary( - `${pactStandalone.messagePath}`, + pactStandalone.messagePath, this.options, this.__argMapping, ); diff --git a/src/publisher.ts b/src/publisher.ts index 8fe5fc41..687432d3 100644 --- a/src/publisher.ts +++ b/src/publisher.ts @@ -18,6 +18,7 @@ export class Publisher { public readonly options: PublisherOptions; private readonly __argMapping = { pactFilesOrDirs: DEFAULT_ARG, + cliVerb: DEFAULT_ARG, pactBroker: '--broker-base-url', pactBrokerUsername: '--broker-username', pactBrokerPassword: '--broker-password', @@ -93,8 +94,8 @@ export class Publisher { logger.info(`Publishing pacts to broker at: ${this.options.pactBroker}`); const deferred = q.defer(); const instance = spawn.spawnBinary( - `${pactStandalone.brokerPath} publish`, - this.options, + pactStandalone.brokerPath, + [{ cliVerb: 'publish' }, this.options], this.__argMapping, ); const output: Array = []; diff --git a/src/server.ts b/src/server.ts index 3e937a6c..d5106268 100644 --- a/src/server.ts +++ b/src/server.ts @@ -2,6 +2,7 @@ import { AbstractService } from './service'; import { deprecate } from 'util'; import pact from './pact-standalone'; +import { DEFAULT_ARG } from './spawn'; import path = require('path'); import fs = require('fs'); import mkdirp = require('mkdirp'); @@ -89,22 +90,28 @@ export class Server extends AbstractService { opts.logLevel = options.logLevel.toUpperCase() as LogLevel; } - super(`${pact.mockServicePath} service`, opts, { - port: '--port', - host: '--host', - log: '--log', - ssl: '--ssl', - sslcert: '--sslcert', - sslkey: '--sslkey', - cors: '--cors', - dir: '--pact_dir', - spec: '--pact_specification_version', - pactFileWriteMode: '--pact-file-write-mode', - consumer: '--consumer', - provider: '--provider', - monkeypatch: '--monkeypatch', - logLevel: '--log-level', - }); + super( + pact.mockServicePath, + opts, + { + cliVerb: DEFAULT_ARG, + port: '--port', + host: '--host', + log: '--log', + ssl: '--ssl', + sslcert: '--sslcert', + sslkey: '--sslkey', + cors: '--cors', + dir: '--pact_dir', + spec: '--pact_specification_version', + pactFileWriteMode: '--pact-file-write-mode', + consumer: '--consumer', + provider: '--provider', + monkeypatch: '--monkeypatch', + logLevel: '--log-level', + }, + { cliVerb: 'service' }, + ); } } diff --git a/src/service.ts b/src/service.ts index a4abc045..5177b919 100644 --- a/src/service.ts +++ b/src/service.ts @@ -6,7 +6,7 @@ import events = require('events'); import http = require('request'); import q = require('q'); import logger from './logger'; -import spawn from './spawn'; +import spawn, { CliVerbOptions } from './spawn'; import { ChildProcess } from 'child_process'; import mkdirp = require('mkdirp'); // eslint-disable-next-line @typescript-eslint/no-var-requires @@ -20,9 +20,9 @@ const RETRY_AMOUNT = 60; const PROCESS_TIMEOUT = 30000; interface AbstractServiceEventInterface { - START_EVENT: string; - STOP_EVENT: string; - DELETE_EVENT: string; + START_EVENT: string; + STOP_EVENT: string; + DELETE_EVENT: string; } export abstract class AbstractService extends events.EventEmitter { @@ -35,14 +35,21 @@ export abstract class AbstractService extends events.EventEmitter { } public readonly options: ServiceOptions; + // eslint-disable-next-line @typescript-eslint/no-explicit-any protected __argMapping: any; protected __running: boolean; protected __instance: ChildProcess; + protected __cliVerb?: CliVerbOptions; protected __serviceCommand: string; // eslint-disable-next-line @typescript-eslint/no-explicit-any - protected constructor(command: string, options: ServiceOptions, argMapping: any) { + protected constructor( + command: string, + options: ServiceOptions, + argMapping: any, + cliVerb?: CliVerbOptions, + ) { super(); // defaults @@ -122,6 +129,7 @@ export abstract class AbstractService extends events.EventEmitter { this.options = options; this.__running = false; + this.__cliVerb = cliVerb; this.__serviceCommand = command; this.__argMapping = argMapping; } @@ -152,7 +160,7 @@ export abstract class AbstractService extends events.EventEmitter { this.__instance.stdout.on('data', catchPort); } - this.__instance.stderr.on('data', (data) => + this.__instance.stderr.on('data', data => logger.error(`Pact Binary Error: ${data}`), ); @@ -193,7 +201,7 @@ export abstract class AbstractService extends events.EventEmitter { protected spawnBinary(): ChildProcess { return spawn.spawnBinary( this.__serviceCommand, - this.options, + this.__cliVerb ? [this.__cliVerb, this.options] : [this.options], this.__argMapping, ); } @@ -269,7 +277,7 @@ export abstract class AbstractService extends events.EventEmitter { headers: { 'X-Pact-Mock-Service': true, 'Content-Type': 'application/json', - } + }, }; if (options.ssl) { @@ -301,15 +309,15 @@ export interface ServiceOptions { } export interface HTTPConfig { - uri: string; - method: string; - headers: { - 'X-Pact-Mock-Service': boolean; - 'Content-Type': string; - }; - agentOptions?: { - rejectUnauthorized?: boolean; - requestCert?: boolean; - agent?: boolean; - }; + uri: string; + method: string; + headers: { + 'X-Pact-Mock-Service': boolean; + 'Content-Type': string; + }; + agentOptions?: { + rejectUnauthorized?: boolean; + requestCert?: boolean; + agent?: boolean; + }; } diff --git a/src/spawn/arguments.spec.ts b/src/spawn/arguments.spec.ts index 9a5e9784..387cc3b7 100644 --- a/src/spawn/arguments.spec.ts +++ b/src/spawn/arguments.spec.ts @@ -32,9 +32,9 @@ describe('Pact Util Spec', () => { }, ); expect(result).to.include('--provider-base-url'); - expect(result).to.include("'http://localhost'"); + expect(result).to.include('http://localhost'); expect(result).to.include('--pact-urls'); - expect(result).to.include("'http://idontexist'"); + expect(result).to.include('http://idontexist'); }); }); describe('when called with an array', () => { @@ -52,7 +52,7 @@ describe('Pact Util Spec', () => { expect(result.length).to.be.equal(2); }); - it('should wrap its argument values in quotes', () => { + it('should produce correct arguments array', () => { const result = argsHelper.toArgumentsArray( [ { @@ -66,13 +66,13 @@ describe('Pact Util Spec', () => { }, ); expect(result).to.include('--provider-base-url'); - expect(result).to.include("'http://localhost'"); + expect(result).to.include('http://localhost'); expect(result).to.include('--pact-urls'); - expect(result).to.include("'http://idontexist'"); + expect(result).to.include('http://idontexist'); }); }); describe('with multiple elements', () => { - it('should wrap its argument values in quotes', () => { + it('should produce correct arguments array', () => { const result = argsHelper.toArgumentsArray( [ { participant: 'one' }, @@ -86,13 +86,13 @@ describe('Pact Util Spec', () => { expect(result).to.be.an('array'); expect(result).to.eql([ '--participant', - "'one'", + 'one', '--version', - "'v1'", + 'v1', '--participant', - "'two'", + 'two', '--version', - "'v2'", + 'v2', ]); }); }); @@ -110,7 +110,7 @@ describe('Pact Util Spec', () => { }, ); expect(result.length).to.be.equal(3); - expect(result[0]).to.be.equal("'http://idontexist'"); + expect(result[0]).to.be.equal('http://idontexist'); }); }); }); diff --git a/src/spawn/arguments.ts b/src/spawn/arguments.ts index 867ce936..721c1d13 100644 --- a/src/spawn/arguments.ts +++ b/src/spawn/arguments.ts @@ -9,19 +9,26 @@ import { PublisherOptions } from '../publisher'; import { ServiceOptions } from '../service'; import { VerifierOptions } from '../verifier'; -export type SpawnArguments = - | CanDeployOptions[] +export type CliVerbOptions = { + cliVerb: string; +}; + +export type SpawnArgument = + | CanDeployOptions | MessageOptions | PublisherOptions | ServiceOptions | VerifierOptions + | CliVerbOptions | {}; // Empty object is allowed to make tests less noisy. We should change this in the future +export type SpawnArguments = Array | SpawnArgument; + export const DEFAULT_ARG = 'DEFAULT'; export const PACT_NODE_NO_VALUE = 'PACT_NODE_NO_VALUE'; const valFor = (v: string): Array => - v !== PACT_NODE_NO_VALUE ? [`'${v}'`] : []; + v !== PACT_NODE_NO_VALUE ? [v] : []; const mapFor = (mapping: string, v: string): Array => mapping === DEFAULT_ARG ? valFor(v) : [mapping].concat(valFor(v)); diff --git a/src/spawn/spawn.ts b/src/spawn/spawn.ts index 998b3da9..0d4662c2 100644 --- a/src/spawn/spawn.ts +++ b/src/spawn/spawn.ts @@ -1,4 +1,5 @@ // tslint:disable:no-string-literal +import spawn = require('cross-spawn'); import cp = require('child_process'); import { ChildProcess, SpawnOptions } from 'child_process'; import * as path from 'path'; @@ -25,36 +26,22 @@ export class Spawn { // https://github.com/pact-foundation/pact-mock-service-npm/issues/16 delete envVars['RUBYGEMS_GEMDEPS']; - let file: string; - let opts: SpawnOptions = { + const opts: SpawnOptions = { cwd: pactEnvironment.cwd, detached: !pactEnvironment.isWindows(), env: envVars, }; - let cmd: string = [command] - .concat(argsHelper.toArgumentsArray(args, argMapping)) - .join(' '); - - let spawnArgs: string[]; - if (pactEnvironment.isWindows()) { - file = 'cmd.exe'; - spawnArgs = ['/s', '/c', cmd]; - (opts).windowsVerbatimArguments = true; - } else { - cmd = `./${cmd}`; - file = '/bin/sh'; - spawnArgs = ['-c', cmd]; - } + const spawnArgs: string[] = argsHelper.toArgumentsArray(args, argMapping); logger.debug( `Starting pact binary with '${_.flatten([ - file, + command, spawnArgs, JSON.stringify(opts), ])}'`, ); - const instance = cp.spawn(file, spawnArgs, opts); + const instance = spawn(command, spawnArgs, opts); instance.stdout.setEncoding('utf8'); instance.stderr.setEncoding('utf8'); @@ -67,7 +54,7 @@ export class Spawn { } }); - logger.debug(`Created '${cmd}' process with PID: ${instance.pid}`); + logger.debug(`Created '${command}' process with PID: ${instance.pid}`); return instance; } diff --git a/src/verifier.ts b/src/verifier.ts index 9bbe39dc..3939972d 100644 --- a/src/verifier.ts +++ b/src/verifier.ts @@ -81,7 +81,10 @@ export class Verifier { checkTypes.assert.nonEmptyString(options.providerBaseUrl); - if (checkTypes.emptyArray(options.pactUrls as string[]) && !options.pactBrokerUrl) { + if ( + checkTypes.emptyArray(options.pactUrls as string[]) && + !options.pactBrokerUrl + ) { throw new Error( 'Must provide the pactUrls argument if no pactBrokerUrl provided', ); @@ -180,7 +183,7 @@ export class Verifier { this.options, this.__argMapping, ); - const output: Array = []; + const output: Array = []; instance.stdout.on('data', l => output.push(l)); instance.stderr.on('data', l => output.push(l)); instance.once('close', code => {