From 053fcd8fbfd9daa9dbf128521443ac145f2e081d Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Sun, 26 Apr 2026 21:29:57 +0100 Subject: [PATCH 1/5] Remove old dependencies --- components/framework/index.js | 2 +- package.json | 3 - src/configuration/read.js | 2 +- src/utils/spawn.js | 60 ++++++++++++++++++++ test/unit/components/framework/index.test.js | 2 +- 5 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 src/utils/spawn.js diff --git a/components/framework/index.js b/components/framework/index.js index 4942d55..c032ad3 100644 --- a/components/framework/index.js +++ b/components/framework/index.js @@ -5,7 +5,7 @@ const YAML = require('js-yaml'); const hasha = require('hasha'); const glob = require('../../src/utils/glob'); const path = require('path'); -const spawnExt = require('child-process-ext/spawn'); +const spawnExt = require('../../src/utils/spawn'); const semver = require('semver'); const { configSchema } = require('./configuration'); const ServerlessError = require('../../src/serverless-error'); diff --git a/package.json b/package.json index 24fd6a9..31cd46a 100644 --- a/package.json +++ b/package.json @@ -34,8 +34,6 @@ "@aws-sdk/property-provider": "^3.366.0", "@dagrejs/graphlib": "^3.0.4", "ajv": "^8.11.0", - "child-process-ext": "^2.1.1", - "ci-info": "^3.3.2", "cli-cursor": "^3", "cli-progress-footer": "^2.3.2", "cross-spawn": "^7.0.3", @@ -52,7 +50,6 @@ "log-node": "^8.0.3", "memoizee": "^0.4.15", "minimist": "^1.2.6", - "path2": "^0.1.0", "ramda": "^0.28.0", "semver": "^7.3.7", "signal-exit": "^3.0.7", diff --git a/src/configuration/read.js b/src/configuration/read.js index c4acaac..74b973a 100644 --- a/src/configuration/read.js +++ b/src/configuration/read.js @@ -5,7 +5,7 @@ const { createRequire } = require('module'); const path = require('path'); const fsp = require('fs').promises; const yaml = require('js-yaml'); -const spawn = require('child-process-ext/spawn'); +const spawn = require('../utils/spawn'); const ServerlessError = require('../serverless-error'); // Logic for TS resolution is kept as similar as possible to the Serverless Framework codebase diff --git a/src/utils/spawn.js b/src/utils/spawn.js new file mode 100644 index 0000000..8558ffe --- /dev/null +++ b/src/utils/spawn.js @@ -0,0 +1,60 @@ +'use strict'; + +const { spawn } = require('child_process'); + +module.exports = (command, args = [], options = {}) => { + const child = spawn(command, args, options); + const stdoutChunks = []; + const stderrChunks = []; + const stdChunks = []; + + if (child.stdout) { + child.stdout.on('data', (chunk) => { + stdoutChunks.push(chunk); + stdChunks.push(chunk); + }); + } + + if (child.stderr) { + child.stderr.on('data', (chunk) => { + stderrChunks.push(chunk); + stdChunks.push(chunk); + }); + } + + const buildResult = () => ({ + child, + stdout: child.stdout, + stderr: child.stderr, + stdoutBuffer: Buffer.concat(stdoutChunks), + stderrBuffer: Buffer.concat(stderrChunks), + stdBuffer: Buffer.concat(stdChunks), + }); + + const promise = new Promise((resolve, reject) => { + child.on('error', (error) => { + Object.assign(error, buildResult()); + reject(error); + }); + + child.on('close', (code, signal) => { + const result = buildResult(); + if (code === 0) { + resolve(result); + return; + } + + const error = new Error(`Command failed: ${command} ${args.join(' ')}`); + error.code = code; + error.signal = signal; + Object.assign(error, result); + reject(error); + }); + }); + + promise.child = child; + promise.stdout = child.stdout; + promise.stderr = child.stderr; + + return promise; +}; diff --git a/test/unit/components/framework/index.test.js b/test/unit/components/framework/index.test.js index 1be3798..677d684 100644 --- a/test/unit/components/framework/index.test.js +++ b/test/unit/components/framework/index.test.js @@ -632,7 +632,7 @@ describe('test/unit/components/framework/index.test.js', () => { }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'child-process-ext/spawn': spawnExtStub, + '../../src/utils/spawn': spawnExtStub, }); const context = await getContext(); From 5010d2abadaa5dc7daa15622808e0d787e64425b Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Sun, 26 Apr 2026 22:08:59 +0100 Subject: [PATCH 2/5] Apply feedback from code review --- components/framework/index.js | 12 +- package.json | 2 +- src/utils/spawn.js | 138 +++++++++++++++---- test/unit/components/framework/index.test.js | 36 ++--- test/unit/src/utils/spawn.test.js | 123 +++++++++++++++++ 5 files changed, 263 insertions(+), 48 deletions(-) create mode 100644 test/unit/src/utils/spawn.test.js diff --git a/components/framework/index.js b/components/framework/index.js index c032ad3..66dc8bc 100644 --- a/components/framework/index.js +++ b/components/framework/index.js @@ -1,11 +1,10 @@ 'use strict'; -const spawn = require('cross-spawn'); const YAML = require('js-yaml'); const hasha = require('hasha'); const glob = require('../../src/utils/glob'); const path = require('path'); -const spawnExt = require('../../src/utils/spawn'); +const spawn = require('../../src/utils/spawn'); const semver = require('semver'); const { configSchema } = require('./configuration'); const ServerlessError = require('../../src/serverless-error'); @@ -217,7 +216,7 @@ class ServerlessFramework { ) { let stdoutResult; try { - const { stdoutBuffer } = await spawnExt('serverless', ['--version']); + const { stdoutBuffer } = await spawn('serverless', ['--version']); stdoutResult = stdoutBuffer.toString(); } catch (e) { throw new Error( @@ -271,11 +270,16 @@ class ServerlessFramework { this.context.logVerbose(`Running "${command} ${args.join(' ')}"`); return new Promise((resolve, reject) => { - const child = spawn(command, args, { + const subprocess = spawn(command, args, { cwd: this.inputs.path, stdio: streamStdout ? 'inherit' : undefined, env: { ...process.env, SLS_DISABLE_AUTO_UPDATE: '1', SLS_COMPOSE: '1' }, }); + const child = subprocess.child || subprocess; + + if (typeof subprocess.catch === 'function') { + subprocess.catch(() => {}); + } // Make sure that when our process is killed, we terminate the subprocess too const processExitCallback = () => { diff --git a/package.json b/package.json index 31cd46a..dfac392 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,7 @@ "ajv": "^8.11.0", "cli-cursor": "^3", "cli-progress-footer": "^2.3.2", - "cross-spawn": "^7.0.3", + "cross-spawn": "^7.0.6", "d": "^1.0.1", "event-emitter": "^0.3.5", "ext": "^1.7.0", diff --git a/src/utils/spawn.js b/src/utils/spawn.js index 8558ffe..fcf4cbf 100644 --- a/src/utils/spawn.js +++ b/src/utils/spawn.js @@ -1,60 +1,148 @@ 'use strict'; -const { spawn } = require('child_process'); +const spawn = require('cross-spawn'); +const { PassThrough } = require('stream'); + +const sensitiveOptionNamePattern = + /(?:^|[-_])(?:auth|authorization|credential|password|passwd|pwd|secret|token|api[-_]?key|access[-_]?key)(?:$|[-_])/i; + +const toBuffer = (chunk) => (Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); + +const redactArgs = (args) => { + const redactedArgs = []; + let redactNext = false; + + for (const arg of args) { + const value = String(arg); + + if (redactNext) { + redactedArgs.push(''); + redactNext = false; + continue; + } + + const equalsIndex = value.indexOf('='); + const optionName = value.replace(/^-+/, '').split('=')[0]; + + if (equalsIndex !== -1 && sensitiveOptionNamePattern.test(optionName)) { + redactedArgs.push(`${value.slice(0, equalsIndex + 1)}`); + continue; + } + + if (value.startsWith('-') && sensitiveOptionNamePattern.test(optionName)) { + redactedArgs.push(value); + redactNext = true; + continue; + } + + redactedArgs.push(value); + } + + return redactedArgs; +}; module.exports = (command, args = [], options = {}) => { - const child = spawn(command, args, options); + const normalizedCommand = String(command); + const normalizedArgs = args == null ? [] : Array.from(args, String); + const { shouldCloseStdin, input, ...spawnOptions } = options || {}; + + const child = spawn(normalizedCommand, normalizedArgs, spawnOptions); + const result = { + child, + stdout: child.stdout || null, + stderr: child.stderr || null, + std: child.stdout || child.stderr ? new PassThrough() : null, + stdoutBuffer: Buffer.alloc(0), + stderrBuffer: Buffer.alloc(0), + stdBuffer: Buffer.alloc(0), + code: undefined, + signal: undefined, + }; const stdoutChunks = []; const stderrChunks = []; const stdChunks = []; + const append = (bufferName, chunks, chunk) => { + chunks.push(toBuffer(chunk)); + result[bufferName] = Buffer.concat(chunks); + }; + + const snapshot = () => ({ ...result }); + + const endStd = () => { + if (result.std && !result.std.destroyed && !result.std.writableEnded) { + result.std.end(); + } + }; + if (child.stdout) { child.stdout.on('data', (chunk) => { - stdoutChunks.push(chunk); - stdChunks.push(chunk); + append('stdoutBuffer', stdoutChunks, chunk); + append('stdBuffer', stdChunks, chunk); + result.std.write(chunk); }); } if (child.stderr) { child.stderr.on('data', (chunk) => { - stderrChunks.push(chunk); - stdChunks.push(chunk); + append('stderrBuffer', stderrChunks, chunk); + append('stdBuffer', stdChunks, chunk); + result.std.write(chunk); }); } - const buildResult = () => ({ - child, - stdout: child.stdout, - stderr: child.stderr, - stdoutBuffer: Buffer.concat(stdoutChunks), - stderrBuffer: Buffer.concat(stderrChunks), - stdBuffer: Buffer.concat(stdChunks), - }); - const promise = new Promise((resolve, reject) => { + let settled = false; + child.on('error', (error) => { - Object.assign(error, buildResult()); + if (settled) return; + settled = true; + endStd(); + const metadata = snapshot(); + if (metadata.code === undefined) delete metadata.code; + if (metadata.signal === undefined) delete metadata.signal; + Object.assign(error, metadata); reject(error); }); child.on('close', (code, signal) => { - const result = buildResult(); + if (settled) return; + settled = true; + result.code = code; + result.signal = signal; + endStd(); + if (code === 0) { - resolve(result); + resolve(snapshot()); return; } - const error = new Error(`Command failed: ${command} ${args.join(' ')}`); + const reason = signal ? `signal ${signal}` : `code ${code}`; + const error = new Error( + `\`${[normalizedCommand, ...redactArgs(normalizedArgs)].join(' ')}\` Exited with ${reason}` + ); error.code = code; error.signal = signal; - Object.assign(error, result); + Object.assign(error, snapshot()); reject(error); }); - }); - promise.child = child; - promise.stdout = child.stdout; - promise.stderr = child.stderr; + if (input != null && child.stdin) { + child.stdin.end(input); + } else if (shouldCloseStdin && child.stdin) { + child.stdin.end(); + } + }); - return promise; + return Object.defineProperties(promise, { + child: { enumerable: true, get: () => result.child }, + stdout: { enumerable: true, get: () => result.stdout }, + stderr: { enumerable: true, get: () => result.stderr }, + std: { enumerable: true, get: () => result.std }, + stdoutBuffer: { enumerable: true, get: () => result.stdoutBuffer }, + stderrBuffer: { enumerable: true, get: () => result.stderrBuffer }, + stdBuffer: { enumerable: true, get: () => result.stdBuffer }, + code: { enumerable: true, get: () => result.code }, + signal: { enumerable: true, get: () => result.signal }, + }); }; diff --git a/test/unit/components/framework/index.test.js b/test/unit/components/framework/index.test.js index 677d684..48b9664 100644 --- a/test/unit/components/framework/index.test.js +++ b/test/unit/components/framework/index.test.js @@ -46,7 +46,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -79,7 +79,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -109,7 +109,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -139,7 +139,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -179,7 +179,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -219,7 +219,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -253,7 +253,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -294,7 +294,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -319,7 +319,7 @@ describe('test/unit/components/framework/index.test.js', () => { }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -351,7 +351,7 @@ describe('test/unit/components/framework/index.test.js', () => { }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -444,7 +444,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -470,7 +470,7 @@ describe('test/unit/components/framework/index.test.js', () => { }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -553,7 +553,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -580,7 +580,7 @@ describe('test/unit/components/framework/index.test.js', () => { }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -607,7 +607,7 @@ describe('test/unit/components/framework/index.test.js', () => { }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -657,7 +657,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -700,7 +700,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); @@ -727,7 +727,7 @@ describe('test/unit/components/framework/index.test.js', () => { kill: () => {}, }); const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { - 'cross-spawn': spawnStub, + '../../src/utils/spawn': spawnStub, }); const context = await getContext(); diff --git a/test/unit/src/utils/spawn.test.js b/test/unit/src/utils/spawn.test.js new file mode 100644 index 0000000..36d7035 --- /dev/null +++ b/test/unit/src/utils/spawn.test.js @@ -0,0 +1,123 @@ +'use strict'; + +const { expect } = require('chai'); +const spawn = require('../../../../src/utils/spawn'); + +const expectRejected = async (promise) => { + try { + await promise; + } catch (error) { + return error; + } + throw new Error('Expected promise to reject'); +}; + +describe('spawn', () => { + it('executes package manager shims through cross-platform resolution', async () => { + const result = await spawn('npm', ['--version']); + + expect(String(result.stdoutBuffer).trim()).to.match(/^\d+\.\d+\.\d+/); + expect(result.code).to.equal(0); + expect(result.signal).to.equal(null); + }); + + it('closes stdin when shouldCloseStdin is enabled', async () => { + const result = await spawn( + process.execPath, + [ + '-e', + 'process.stdin.resume(); process.stdin.on("end", () => process.stdout.write("closed"));', + ], + { shouldCloseStdin: true } + ); + + expect(String(result.stdoutBuffer)).to.equal('closed'); + }); + + it('exposes child, streams, std stream, buffers, code, and signal on success', async () => { + const execution = spawn(process.execPath, [ + '-e', + 'setTimeout(() => { process.stdout.write("out"); process.stderr.write("err"); }, 25);', + ]); + const stdChunks = []; + + expect(execution.child).to.exist; + expect(execution.stdout).to.exist; + expect(execution.stderr).to.exist; + expect(execution.std).to.exist; + + execution.std.on('data', (chunk) => stdChunks.push(chunk)); + + const result = await execution; + + expect(result.child).to.equal(execution.child); + expect(String(result.stdoutBuffer)).to.equal('out'); + expect(String(result.stderrBuffer)).to.equal('err'); + expect(String(result.stdBuffer)).to.include('out'); + expect(String(result.stdBuffer)).to.include('err'); + expect(String(Buffer.concat(stdChunks))).to.include('out'); + expect(String(Buffer.concat(stdChunks))).to.include('err'); + expect(result.code).to.equal(0); + expect(result.signal).to.equal(null); + }); + + it('rejects nonzero exits with buffers and redacted command arguments', async () => { + const error = await expectRejected( + spawn(process.execPath, [ + '-e', + 'process.stdout.write("out"); process.stderr.write("err"); process.exit(7);', + '--', + '--password', + 'super-secret', + '--token=secret-token', + ]) + ); + + expect(error.code).to.equal(7); + expect(error.signal).to.equal(null); + expect(String(error.stdoutBuffer)).to.equal('out'); + expect(String(error.stderrBuffer)).to.equal('err'); + expect(error.message).to.include('Exited with code 7'); + expect(error.message).to.not.include('super-secret'); + expect(error.message).to.not.include('secret-token'); + expect(error.message).to.include('--password '); + expect(error.message).to.include('--token='); + }); + + it('preserves spawn error codes such as ENOENT', async () => { + const error = await expectRejected(spawn(`missing-command-${process.pid}`, [])); + + expect(error.code).to.equal('ENOENT'); + expect(error.stdoutBuffer).to.deep.equal(Buffer.alloc(0)); + expect(error.stderrBuffer).to.deep.equal(Buffer.alloc(0)); + expect(error.stdBuffer).to.deep.equal(Buffer.alloc(0)); + }); + + it('handles stdio inherit with null streams on success', async () => { + const result = await spawn(process.execPath, ['-e', 'process.exit(0);'], { + stdio: 'inherit', + }); + + expect(result.stdout).to.equal(null); + expect(result.stderr).to.equal(null); + expect(result.std).to.equal(null); + expect(result.stdoutBuffer).to.deep.equal(Buffer.alloc(0)); + expect(result.stderrBuffer).to.deep.equal(Buffer.alloc(0)); + expect(result.stdBuffer).to.deep.equal(Buffer.alloc(0)); + expect(result.code).to.equal(0); + }); + + it('handles stdio inherit with null streams on nonzero exit', async () => { + const error = await expectRejected( + spawn(process.execPath, ['-e', 'process.exit(3);'], { stdio: 'inherit' }) + ); + + expect(error.code).to.equal(3); + expect(error.stdout).to.equal(null); + expect(error.stderr).to.equal(null); + expect(error.std).to.equal(null); + expect(error.stdoutBuffer).to.deep.equal(Buffer.alloc(0)); + expect(error.stderrBuffer).to.deep.equal(Buffer.alloc(0)); + expect(error.stdBuffer).to.deep.equal(Buffer.alloc(0)); + }); +}); From 2d7fb0df688d2304838ea4a1ff7f519f739aacfb Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Sun, 26 Apr 2026 22:55:23 +0100 Subject: [PATCH 3/5] Added more tests --- test/unit/components/framework/index.test.js | 59 ++++++++++++++++++++ test/unit/src/utils/spawn.test.js | 48 ++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/test/unit/components/framework/index.test.js b/test/unit/components/framework/index.test.js index 48b9664..7ad9a5c 100644 --- a/test/unit/components/framework/index.test.js +++ b/test/unit/components/framework/index.test.js @@ -14,6 +14,40 @@ const ServerlessFramework = require('../../../../components/framework'); const expect = chai.expect; +const createSpawnExecution = ({ code = 0, stdout = '', stderr = '' } = {}) => { + const child = { + stdout: { + on: (event, callback) => { + if (event === 'data' && stdout) callback(Buffer.from(stdout)); + }, + }, + stderr: { + on: (event, callback) => { + if (event === 'data' && stderr) callback(Buffer.from(stderr)); + }, + }, + on: (event, callback) => { + if (event === 'close') process.nextTick(() => callback(code)); + }, + kill: sinon.stub(), + }; + const execution = Promise.resolve({ + child, + stdoutBuffer: Buffer.from(stdout), + stderrBuffer: Buffer.from(stderr), + stdBuffer: Buffer.from(`${stdout}${stderr}`), + code, + signal: null, + }); + + execution.child = child; + execution.stdout = child.stdout; + execution.stderr = child.stderr; + execution.std = null; + + return execution; +}; + /** * @returns {Promise} */ @@ -65,6 +99,31 @@ describe('test/unit/components/framework/index.test.js', () => { expect(context.outputs).to.deep.equal({ Key: 'Output' }); }); + it('supports the shared spawn helper promise shape when executing framework commands', async () => { + const spawnStub = sinon.stub(); + spawnStub.onFirstCall().returns( + createSpawnExecution({ + stdout: 'region: us-east-1\n\nStack Outputs:\n Key: Output', + }) + ); + spawnStub.onSecondCall().returns( + createSpawnExecution({ + stdout: 'region: us-east-1\n\nStack Outputs:\n Key: Output', + }) + ); + const FrameworkComponent = proxyquire('../../../../components/framework/index.js', { + '../../src/utils/spawn': spawnStub, + }); + + const context = await getContext(); + const component = new FrameworkComponent('some-id', context, { path: 'path' }); + context.state.detectedFrameworkVersion = '9.9.9'; + await component.deploy(); + + expect(spawnStub).to.be.calledTwice; + expect(context.outputs).to.deep.equal({ Key: 'Output' }); + }); + it('correctly handles package', async () => { const spawnStub = sinon.stub().returns({ on: (arg, cb) => { diff --git a/test/unit/src/utils/spawn.test.js b/test/unit/src/utils/spawn.test.js index 36d7035..881a227 100644 --- a/test/unit/src/utils/spawn.test.js +++ b/test/unit/src/utils/spawn.test.js @@ -12,6 +12,20 @@ const expectRejected = async (promise) => { throw new Error('Expected promise to reject'); }; +const assertRedaction = async ({ args, redacted, visible = [] }) => { + const error = await expectRejected( + spawn(process.execPath, ['-e', 'process.exit(7);', '--', ...args]) + ); + + for (const value of redacted) { + expect(error.message).to.not.include(value); + } + + for (const value of visible) { + expect(error.message).to.include(value); + } +}; + describe('spawn', () => { it('executes package manager shims through cross-platform resolution', async () => { const result = await spawn('npm', ['--version']); @@ -84,6 +98,40 @@ describe('spawn', () => { expect(error.message).to.include('--token='); }); + it('redacts sensitive option values in generated error messages', async () => { + await assertRedaction({ + args: [ + '--authorization', + 'Bearer abc', + '--credential', + 'profile', + '--api-key', + 'key-value', + '--access_key', + 'access-value', + '--pwd', + 'pwd-value', + '--SECRET', + 'secret-value', + '--secret=inline-secret', + '--monkey', + 'visible-value', + '--tokenizer', + 'visible-tokenizer', + ], + redacted: [ + 'Bearer abc', + 'profile', + 'key-value', + 'access-value', + 'pwd-value', + 'secret-value', + 'inline-secret', + ], + visible: ['--monkey', 'visible-value', '--tokenizer', 'visible-tokenizer'], + }); + }); + it('preserves spawn error codes such as ENOENT', async () => { const error = await expectRejected(spawn(`missing-command-${process.pid}`, [])); From b5c86756d3dded4a35d6d5a401a5736741d02fe0 Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Sun, 26 Apr 2026 23:00:40 +0100 Subject: [PATCH 4/5] Update spawn.test.js --- test/unit/src/utils/spawn.test.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/unit/src/utils/spawn.test.js b/test/unit/src/utils/spawn.test.js index 881a227..595d185 100644 --- a/test/unit/src/utils/spawn.test.js +++ b/test/unit/src/utils/spawn.test.js @@ -137,8 +137,14 @@ describe('spawn', () => { expect(error.code).to.equal('ENOENT'); expect(error.stdoutBuffer).to.deep.equal(Buffer.alloc(0)); - expect(error.stderrBuffer).to.deep.equal(Buffer.alloc(0)); - expect(error.stdBuffer).to.deep.equal(Buffer.alloc(0)); + expect(Buffer.isBuffer(error.stderrBuffer)).to.equal(true); + expect(Buffer.isBuffer(error.stdBuffer)).to.equal(true); + + if (error.stderrBuffer.length) { + expect(error.stdBuffer.equals(error.stderrBuffer)).to.equal(true); + } else { + expect(error.stdBuffer).to.deep.equal(Buffer.alloc(0)); + } }); it('handles stdio inherit with null streams on success', async () => { From a04ddd882cd98e209e2ee2d5ac1249503f815fe7 Mon Sep 17 00:00:00 2001 From: Graham Campbell Date: Mon, 27 Apr 2026 00:11:41 +0100 Subject: [PATCH 5/5] Applied feedback from additional round of review --- src/utils/spawn.js | 122 ++++++++++++++++---- test/unit/src/utils/spawn.test.js | 177 +++++++++++++++++++++++++++++- 2 files changed, 273 insertions(+), 26 deletions(-) diff --git a/src/utils/spawn.js b/src/utils/spawn.js index fcf4cbf..9fe5870 100644 --- a/src/utils/spawn.js +++ b/src/utils/spawn.js @@ -8,6 +8,32 @@ const sensitiveOptionNamePattern = const toBuffer = (chunk) => (Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk)); +const createBufferState = () => ({ + buffer: Buffer.alloc(0), + chunks: [], + dirty: false, + length: 0, +}); + +const appendBuffer = (state, chunk) => { + const buffer = toBuffer(chunk); + + state.chunks.push(buffer); + state.length += buffer.length; + state.dirty = true; + + return buffer; +}; + +const getBuffer = (state) => { + if (state.dirty) { + state.buffer = Buffer.concat(state.chunks, state.length); + state.dirty = false; + } + + return state.buffer; +}; + const redactArgs = (args) => { const redactedArgs = []; let redactNext = false; @@ -52,48 +78,102 @@ module.exports = (command, args = [], options = {}) => { stdout: child.stdout || null, stderr: child.stderr || null, std: child.stdout || child.stderr ? new PassThrough() : null, - stdoutBuffer: Buffer.alloc(0), - stderrBuffer: Buffer.alloc(0), - stdBuffer: Buffer.alloc(0), code: undefined, signal: undefined, }; - const stdoutChunks = []; - const stderrChunks = []; - const stdChunks = []; + if (result.std) result.std.resume(); + + const stdoutState = createBufferState(); + const stderrState = createBufferState(); + const stdState = createBufferState(); + const outputStreams = [result.stdout, result.stderr].filter(Boolean); + const discardStdData = () => {}; + let settled = false; + let waitingForStdDrain = false; + const pausedForStd = new Set(); + + const resumeStdPausedStreams = () => { + waitingForStdDrain = false; + + for (const stream of pausedForStd) { + stream.resume(); + } + + pausedForStd.clear(); + }; + + const hasActiveStdConsumer = () => + result.std && + (result.std.listenerCount('data') > 1 || result.std.listenerCount('readable') > 0); + + const pauseForStdBackpressure = () => { + for (const stream of outputStreams) { + if (!stream.isPaused || stream.isPaused()) continue; + stream.pause(); + pausedForStd.add(stream); + } + + if (!waitingForStdDrain) { + waitingForStdDrain = true; + result.std.once('drain', resumeStdPausedStreams); + } + }; + + const writeStd = (chunk) => { + if (!result.std || result.std.destroyed || result.std.writableEnded) return; - const append = (bufferName, chunks, chunk) => { - chunks.push(toBuffer(chunk)); - result[bufferName] = Buffer.concat(chunks); + if (result.std.write(chunk) === false) { + if (hasActiveStdConsumer()) { + pauseForStdBackpressure(); + } else { + result.std.resume(); + } + } }; - const snapshot = () => ({ ...result }); + const snapshot = () => ({ + child: result.child, + stdout: result.stdout, + stderr: result.stderr, + std: result.std, + stdoutBuffer: getBuffer(stdoutState), + stderrBuffer: getBuffer(stderrState), + stdBuffer: getBuffer(stdState), + code: result.code, + signal: result.signal, + }); const endStd = () => { if (result.std && !result.std.destroyed && !result.std.writableEnded) { result.std.end(); } + + resumeStdPausedStreams(); }; + if (result.std) { + result.std.on('data', discardStdData); + result.std.once('close', resumeStdPausedStreams); + result.std.once('error', resumeStdPausedStreams); + } + if (child.stdout) { child.stdout.on('data', (chunk) => { - append('stdoutBuffer', stdoutChunks, chunk); - append('stdBuffer', stdChunks, chunk); - result.std.write(chunk); + const buffer = appendBuffer(stdoutState, chunk); + appendBuffer(stdState, buffer); + writeStd(buffer); }); } if (child.stderr) { child.stderr.on('data', (chunk) => { - append('stderrBuffer', stderrChunks, chunk); - append('stdBuffer', stdChunks, chunk); - result.std.write(chunk); + const buffer = appendBuffer(stderrState, chunk); + appendBuffer(stdState, buffer); + writeStd(buffer); }); } const promise = new Promise((resolve, reject) => { - let settled = false; - child.on('error', (error) => { if (settled) return; settled = true; @@ -139,9 +219,9 @@ module.exports = (command, args = [], options = {}) => { stdout: { enumerable: true, get: () => result.stdout }, stderr: { enumerable: true, get: () => result.stderr }, std: { enumerable: true, get: () => result.std }, - stdoutBuffer: { enumerable: true, get: () => result.stdoutBuffer }, - stderrBuffer: { enumerable: true, get: () => result.stderrBuffer }, - stdBuffer: { enumerable: true, get: () => result.stdBuffer }, + stdoutBuffer: { enumerable: true, get: () => getBuffer(stdoutState) }, + stderrBuffer: { enumerable: true, get: () => getBuffer(stderrState) }, + stdBuffer: { enumerable: true, get: () => getBuffer(stdState) }, code: { enumerable: true, get: () => result.code }, signal: { enumerable: true, get: () => result.signal }, }); diff --git a/test/unit/src/utils/spawn.test.js b/test/unit/src/utils/spawn.test.js index 595d185..046fd36 100644 --- a/test/unit/src/utils/spawn.test.js +++ b/test/unit/src/utils/spawn.test.js @@ -1,8 +1,54 @@ 'use strict'; +const fs = require('fs').promises; +const os = require('os'); +const path = require('path'); +const { EventEmitter } = require('events'); +const nodeStream = require('stream'); const { expect } = require('chai'); +const proxyquire = require('proxyquire'); +const sinon = require('sinon'); const spawn = require('../../../../src/utils/spawn'); +const loadSpawnWithStubs = (stubs) => + proxyquire.noCallThru().load('../../../../src/utils/spawn', stubs); + +const waitFor = async (condition) => { + for (let attempt = 0; attempt < 50; attempt += 1) { + if (condition()) return; + await new Promise((resolve) => setTimeout(resolve, 10)); + } + + throw new Error('Timed out waiting for condition'); +}; + +const createFakeOutputStream = () => { + const outputStream = new EventEmitter(); + let paused = false; + + outputStream.pause = sinon.spy(() => { + paused = true; + return outputStream; + }); + outputStream.resume = sinon.spy(() => { + paused = false; + return outputStream; + }); + outputStream.isPaused = () => paused; + + return outputStream; +}; + +const createFakeChild = () => { + const child = new EventEmitter(); + + child.stdout = createFakeOutputStream(); + child.stderr = createFakeOutputStream(); + child.stdin = { end: sinon.spy() }; + + return child; +}; + const expectRejected = async (promise) => { try { await promise; @@ -27,12 +73,43 @@ const assertRedaction = async ({ args, redacted, visible = [] }) => { }; describe('spawn', () => { - it('executes package manager shims through cross-platform resolution', async () => { - const result = await spawn('npm', ['--version']); + it('executes PATH shims through cross-platform resolution', async () => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'spawn-shim-')); + const commandName = `spawn-shim-${process.pid}-${Date.now()}`; + const commandPath = path.join( + tempDir, + process.platform === 'win32' ? `${commandName}.cmd` : commandName + ); + const pathKey = Object.keys(process.env).find((key) => key.toLowerCase() === 'path') || 'PATH'; + const originalPath = process.env[pathKey] || process.env.PATH || ''; - expect(String(result.stdoutBuffer).trim()).to.match(/^\d+\.\d+\.\d+/); - expect(result.code).to.equal(0); - expect(result.signal).to.equal(null); + try { + if (process.platform === 'win32') { + await fs.writeFile(commandPath, '@echo off\r\necho shim-ok\r\n'); + } else { + await fs.writeFile(commandPath, '#!/bin/sh\nprintf "shim-ok\\n"\n', { + mode: 0o755, + }); + await fs.chmod(commandPath, 0o755); + } + + const env = { + ...process.env, + [pathKey]: `${tempDir}${path.delimiter}${originalPath}`, + }; + + if (process.platform === 'win32') { + env.PATHEXT = [process.env.PATHEXT, '.CMD'].filter(Boolean).join(';'); + } + + const result = await spawn(commandName, [], { env }); + + expect(String(result.stdoutBuffer).trim()).to.equal('shim-ok'); + expect(result.code).to.equal(0); + expect(result.signal).to.equal(null); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } }); it('closes stdin when shouldCloseStdin is enabled', async () => { @@ -75,6 +152,96 @@ describe('spawn', () => { expect(result.signal).to.equal(null); }); + it('buffers output without concatenating on each chunk or retaining unread std', async () => { + const child = createFakeChild(); + const spawnWithStubs = loadSpawnWithStubs({ + 'cross-spawn': () => child, + }); + const concatSpy = sinon.spy(Buffer, 'concat'); + + try { + const execution = spawnWithStubs('fake-command'); + + child.stdout.emit('data', Buffer.from('out-')); + child.stdout.emit('data', Buffer.from('more')); + child.stderr.emit('data', Buffer.from('-err')); + + expect(concatSpy.called).to.equal(false); + expect(execution.std.readableLength).to.equal(0); + + child.emit('close', 0, null); + const result = await execution; + + expect(String(result.stdoutBuffer)).to.equal('out-more'); + expect(String(result.stderrBuffer)).to.equal('-err'); + expect(String(result.stdBuffer)).to.equal('out-more-err'); + expect(result.std).to.exist; + expect(result.std.readableLength).to.equal(0); + expect(concatSpy.callCount).to.be.at.most(3); + } finally { + concatSpy.restore(); + } + }); + + it('treats std as live while stdBuffer preserves history', async () => { + const execution = spawn(process.execPath, [ + '-e', + 'process.stdout.write("early"); setTimeout(() => process.stdout.write("late"), 25);', + ]); + + await waitFor(() => String(execution.stdoutBuffer).includes('early')); + + const stdChunks = []; + execution.std.on('data', (chunk) => stdChunks.push(Buffer.from(chunk))); + + const result = await execution; + + expect(String(result.stdBuffer)).to.equal('earlylate'); + expect(String(Buffer.concat(stdChunks))).to.equal('late'); + }); + + it('pauses child output while consumed std applies backpressure', async () => { + const child = createFakeChild(); + let stdStream; + + class BackpressurePassThrough extends nodeStream.PassThrough { + constructor(...args) { + super(...args); + stdStream = this; + } + + write(chunk) { + this.emit('data', Buffer.from(chunk)); + return false; + } + } + + const spawnWithStubs = loadSpawnWithStubs({ + 'cross-spawn': () => child, + 'stream': { PassThrough: BackpressurePassThrough }, + }); + const execution = spawnWithStubs('fake-command'); + const stdChunks = []; + + execution.std.on('data', (chunk) => stdChunks.push(Buffer.from(chunk))); + child.stdout.emit('data', Buffer.from('out')); + + expect(child.stdout.pause.calledOnce).to.equal(true); + expect(child.stderr.pause.calledOnce).to.equal(true); + + stdStream.emit('drain'); + + expect(child.stdout.resume.calledOnce).to.equal(true); + expect(child.stderr.resume.calledOnce).to.equal(true); + + child.stderr.emit('data', Buffer.from('err')); + child.emit('close', 0, null); + const result = await execution; + + expect(String(result.stdBuffer)).to.equal('outerr'); + expect(String(Buffer.concat(stdChunks))).to.equal('outerr'); + }); + it('rejects nonzero exits with buffers and redacted command arguments', async () => { const error = await expectRejected( spawn(process.execPath, [