From ad3cde25b29b2d09dd83309be91dba0438eba9ab Mon Sep 17 00:00:00 2001 From: Narek Hovhannisyan Date: Thu, 5 Dec 2024 12:56:54 +0400 Subject: [PATCH 1/6] fix(util): tune npm to escape injection on shell commands --- src/if-check/util/npm.ts | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/if-check/util/npm.ts b/src/if-check/util/npm.ts index 9717819f..18a4dcac 100644 --- a/src/if-check/util/npm.ts +++ b/src/if-check/util/npm.ts @@ -56,18 +56,25 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { sanitizedManifest, ]; - const fullCommand = [ - ...ifEnvCommand, - '&&', - ...ifRunCommand, - '&&', - ...ttyCommand, - '|', - ...ifDiffCommand, - ].join(' '); + // Execute ifEnvCommand + await execPromise(ifEnvCommand.join(' '), { + cwd: process.env.CURRENT_DIR || process.cwd(), + }); + + // Execute ifRunCommand + await execPromise(ifRunCommand.join(' '), { + cwd: process.env.CURRENT_DIR || process.cwd(), + }); + + // Execute ttyCommand and capture its output + const ttyResult = await execPromise(ttyCommand.join(' '), { + cwd: process.env.CURRENT_DIR || process.cwd(), + }); - // Execute the full command - await execPromise(fullCommand, { + // Pipe ttyResult into ifDiffCommand + const diffCommand = ifDiffCommand.join(' '); + const tty = ttyResult && ttyResult.stdout.trim(); + await execPromise(`${tty ? `${tty} | ` : ''}${diffCommand}`, { cwd: process.env.CURRENT_DIR || process.cwd(), }); From 1aadc4e59c793b7134253c76d241d7e19bfa869a Mon Sep 17 00:00:00 2001 From: Narek Hovhannisyan Date: Thu, 5 Dec 2024 12:57:47 +0400 Subject: [PATCH 2/6] test(util): tune command assertions in npm --- src/__tests__/if-check/util/npm.test.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/__tests__/if-check/util/npm.test.ts b/src/__tests__/if-check/util/npm.test.ts index cad8b9bd..d0109e4d 100644 --- a/src/__tests__/if-check/util/npm.test.ts +++ b/src/__tests__/if-check/util/npm.test.ts @@ -24,9 +24,14 @@ jest.mock('../../../common/util/helpers', () => { expect(param).toEqual('npm init -y'); break; case 'if-check': - expect(param).toEqual( - "npm run if-env -- -m ./src/__mocks__/mock-manifest.yaml && npm run if-run -- -m ./src/__mocks__/mock-manifest.yaml -o src/__mocks__/re-mock-manifest && node -p 'Boolean(process.stdout.isTTY)' | npm run if-diff -- -s src/__mocks__/re-mock-manifest.yaml -t ./src/__mocks__/mock-manifest.yaml" - ); + expect( + [ + 'npm run if-env -- -m ./src/__mocks__/mock-manifest.yaml', + 'npm run if-run -- -m ./src/__mocks__/mock-manifest.yaml -o src/__mocks__/re-mock-manifest', + "node -p 'Boolean(process.stdout.isTTY)'", + 'npm run if-diff -- -s src/__mocks__/re-mock-manifest.yaml -t ./src/__mocks__/mock-manifest.yaml', + ].includes(param) + ).toBeTruthy(); break; } return; @@ -45,7 +50,7 @@ describe('if-check/util/npm: ', () => { await executeCommands(manifest, false); - expect.assertions(2); + expect.assertions(5); expect(logSpy).toHaveBeenCalledWith( '✔ if-check successfully verified mock-manifest.yaml\n' ); From a60966670ac1468993b2dba4b4dfe041f341f126 Mon Sep 17 00:00:00 2001 From: Narek Hovhannisyan Date: Thu, 5 Dec 2024 18:42:07 +0400 Subject: [PATCH 3/6] fix(util): tune npm to escape injection on shell commands --- src/if-check/util/npm.ts | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/src/if-check/util/npm.ts b/src/if-check/util/npm.ts index 18a4dcac..62bb9c45 100644 --- a/src/if-check/util/npm.ts +++ b/src/if-check/util/npm.ts @@ -1,5 +1,5 @@ import * as path from 'node:path'; -import {execPromise} from '../../common/util/helpers'; +import {execFileSync} from 'child_process'; import {getFileName, removeFileIfExists} from '../../common/util/fs'; import {STRINGS} from '../config'; @@ -30,7 +30,7 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { const ifEnvCommand = [ isGlobal ? 'if-env' : 'npm run if-env', '--', - ...(prefixFlag === '' ? [] : prefixFlag), + ...(prefixFlag === '' ? [] : [prefixFlag]), '-m', sanitizedManifest, ]; @@ -38,46 +38,49 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { const ifRunCommand = [ isGlobal ? 'if-run' : 'npm run if-run', '--', - ...(prefixFlag === '' ? [] : prefixFlag), + ...(prefixFlag === '' ? [] : [prefixFlag]), '-m', sanitizedManifest, '-o', sanitizedExecutedManifest, ]; - const ttyCommand = ['node', '-p', "'Boolean(process.stdout.isTTY)'"]; + const ttyCommand = ['node', '-p', 'Boolean(process.stdout.isTTY)']; const ifDiffCommand = [ isGlobal ? 'if-diff' : 'npm run if-diff', '--', - ...(prefixFlag === '' ? [] : prefixFlag), + ...(prefixFlag === '' ? [] : [prefixFlag]), '-s', `${sanitizedExecutedManifest}.yaml`, '-t', sanitizedManifest, ]; - // Execute ifEnvCommand - await execPromise(ifEnvCommand.join(' '), { + execFileSync(ifEnvCommand[0], ifEnvCommand.slice(1), { cwd: process.env.CURRENT_DIR || process.cwd(), }); - // Execute ifRunCommand - await execPromise(ifRunCommand.join(' '), { + execFileSync(ifRunCommand[0], ifRunCommand.slice(1), { cwd: process.env.CURRENT_DIR || process.cwd(), }); - // Execute ttyCommand and capture its output - const ttyResult = await execPromise(ttyCommand.join(' '), { + execFileSync(ttyCommand[0], ttyCommand.slice(1), { cwd: process.env.CURRENT_DIR || process.cwd(), }); - // Pipe ttyResult into ifDiffCommand - const diffCommand = ifDiffCommand.join(' '); - const tty = ttyResult && ttyResult.stdout.trim(); - await execPromise(`${tty ? `${tty} | ` : ''}${diffCommand}`, { + const ttyResult = execFileSync(ttyCommand[0], ttyCommand.slice(1), { cwd: process.env.CURRENT_DIR || process.cwd(), }); + const tty = ttyResult && ttyResult.toString().trim(); + execFileSync( + `${tty ? `${tty} | ` : ''}${ifDiffCommand[0]}`, + ifDiffCommand.slice(1), + { + cwd: process.env.CURRENT_DIR || process.cwd(), + } + ); + if (!cwd) { await removeFileIfExists(`${manifestDirPath}/package.json`); } From 5ff51afe4fc00a944016c333763e8028c5f0d474 Mon Sep 17 00:00:00 2001 From: Narek Hovhannisyan Date: Thu, 5 Dec 2024 18:42:43 +0400 Subject: [PATCH 4/6] test(util): tune command assertions in npm --- src/__tests__/if-check/util/npm.test.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/__tests__/if-check/util/npm.test.ts b/src/__tests__/if-check/util/npm.test.ts index d0109e4d..b418cb0d 100644 --- a/src/__tests__/if-check/util/npm.test.ts +++ b/src/__tests__/if-check/util/npm.test.ts @@ -11,26 +11,26 @@ jest.mock('../../../common/util/logger', () => ({ }, })); -jest.mock('../../../common/util/helpers', () => { - const originalModule = jest.requireActual('../../../common/util/helpers'); +jest.mock('child_process', () => { + const originalModule = jest.requireActual('child_process'); return { ...originalModule, - execPromise: async (param: any) => { + execFileSync: (file: any, args: any) => { switch (process.env.NPM_INSTALL) { case 'true': - expect(param).toEqual('npm install @grnsft/if@0.3.3-beta.0'); + expect(file).toEqual('npm install @grnsft/if@0.3.3-beta.0'); break; case 'npm init -y': - expect(param).toEqual('npm init -y'); + expect(file).toEqual('npm init -y'); break; case 'if-check': expect( [ 'npm run if-env -- -m ./src/__mocks__/mock-manifest.yaml', 'npm run if-run -- -m ./src/__mocks__/mock-manifest.yaml -o src/__mocks__/re-mock-manifest', - "node -p 'Boolean(process.stdout.isTTY)'", + 'node -p Boolean(process.stdout.isTTY)', 'npm run if-diff -- -s src/__mocks__/re-mock-manifest.yaml -t ./src/__mocks__/mock-manifest.yaml', - ].includes(param) + ].includes(`${file} ${args.join(' ')}`) ).toBeTruthy(); break; } @@ -50,7 +50,7 @@ describe('if-check/util/npm: ', () => { await executeCommands(manifest, false); - expect.assertions(5); + expect.assertions(6); expect(logSpy).toHaveBeenCalledWith( '✔ if-check successfully verified mock-manifest.yaml\n' ); From 56c8ed38faed327378ec838b03b500e4c1dffd26 Mon Sep 17 00:00:00 2001 From: manushak Date: Fri, 6 Dec 2024 13:03:08 +0400 Subject: [PATCH 5/6] fix(util): fix if-check commands --- src/if-check/util/npm.ts | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/if-check/util/npm.ts b/src/if-check/util/npm.ts index 62bb9c45..7688e839 100644 --- a/src/if-check/util/npm.ts +++ b/src/if-check/util/npm.ts @@ -28,7 +28,8 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { const sanitizedExecutedManifest = escapeShellArg(executedManifest); const ifEnvCommand = [ - isGlobal ? 'if-env' : 'npm run if-env', + isGlobal ? 'if-env' : 'npm', + ...(isGlobal ? [] : ['run', 'if-env']), '--', ...(prefixFlag === '' ? [] : [prefixFlag]), '-m', @@ -36,7 +37,8 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { ]; const ifRunCommand = [ - isGlobal ? 'if-run' : 'npm run if-run', + isGlobal ? 'if-run' : 'npm', + ...(isGlobal ? [] : ['run', 'if-run']), '--', ...(prefixFlag === '' ? [] : [prefixFlag]), '-m', @@ -47,7 +49,8 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { const ttyCommand = ['node', '-p', 'Boolean(process.stdout.isTTY)']; const ifDiffCommand = [ - isGlobal ? 'if-diff' : 'npm run if-diff', + isGlobal ? 'if-diff' : 'npm', + ...(isGlobal ? [] : ['run', 'if-diff']), '--', ...(prefixFlag === '' ? [] : [prefixFlag]), '-s', @@ -58,6 +61,7 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { execFileSync(ifEnvCommand[0], ifEnvCommand.slice(1), { cwd: process.env.CURRENT_DIR || process.cwd(), + shell: true, }); execFileSync(ifRunCommand[0], ifRunCommand.slice(1), { @@ -73,13 +77,15 @@ export const executeCommands = async (manifest: string, cwd: boolean) => { }); const tty = ttyResult && ttyResult.toString().trim(); - execFileSync( - `${tty ? `${tty} | ` : ''}${ifDiffCommand[0]}`, - ifDiffCommand.slice(1), - { - cwd: process.env.CURRENT_DIR || process.cwd(), - } - ); + const fullCommand = `${tty === 'true' ? 'tty |' : ''} ${ifDiffCommand.join( + ' ' + )}`; + + execFileSync(fullCommand, { + cwd: process.env.CURRENT_DIR || process.cwd(), + stdio: 'inherit', + shell: true, + }); if (!cwd) { await removeFileIfExists(`${manifestDirPath}/package.json`); From 44479552bc5d6f017b495b09b1a4f9d6f579da5b Mon Sep 17 00:00:00 2001 From: manushak Date: Fri, 6 Dec 2024 13:03:47 +0400 Subject: [PATCH 6/6] test(util): fix if-check unit test --- src/__tests__/if-check/util/npm.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/__tests__/if-check/util/npm.test.ts b/src/__tests__/if-check/util/npm.test.ts index b418cb0d..d7b4d798 100644 --- a/src/__tests__/if-check/util/npm.test.ts +++ b/src/__tests__/if-check/util/npm.test.ts @@ -30,7 +30,9 @@ jest.mock('child_process', () => { 'npm run if-run -- -m ./src/__mocks__/mock-manifest.yaml -o src/__mocks__/re-mock-manifest', 'node -p Boolean(process.stdout.isTTY)', 'npm run if-diff -- -s src/__mocks__/re-mock-manifest.yaml -t ./src/__mocks__/mock-manifest.yaml', - ].includes(`${file} ${args.join(' ')}`) + ].includes( + Array.isArray(args) ? `${file} ${args.join(' ')}` : file.trim() + ) ).toBeTruthy(); break; }