From 296684e67c10e40d1f70dcdb1ebc07d2d9b7e18a Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Tue, 18 Nov 2025 10:06:18 +0100 Subject: [PATCH] fix(core): Use execFile instead of exec to prevent command injection (#21952) --- .../community-packages.integration.test.ts | 2 +- .../community-packages.service.test.ts | 79 +++++++++++-------- .../community-node.command.ts | 2 +- .../community-packages.controller.ts | 5 +- .../community-packages.service.ts | 31 ++++---- 5 files changed, 65 insertions(+), 54 deletions(-) diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts index 436e5cbe0b9..c83afea7ea1 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts @@ -122,7 +122,7 @@ describe('GET /community-packages', () => { await authAgent.get('/community-packages').expect(200); - const args = ['npm outdated --json', { doNotHandleError: true }]; + const args = [['outdated', '--json'], { doNotHandleError: true }]; expect(communityPackagesService.executeNpmCommand).toHaveBeenCalledWith(...args); }); diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts index 81075fa8390..b752f896408 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts @@ -6,7 +6,7 @@ import { mocked } from 'jest-mock'; import { mock } from 'jest-mock-extended'; import type { InstanceSettings, PackageDirectoryLoader } from 'n8n-core'; import type { PublicInstalledPackage } from 'n8n-workflow'; -import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { access, constants, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; import path, { join } from 'node:path'; @@ -35,13 +35,15 @@ jest.mock('node:fs/promises'); jest.mock('node:child_process'); jest.mock('axios'); -type ExecOptions = NonNullable[1]>; -type ExecCallback = NonNullable[2]>; +type ExecFileOptions = NonNullable[2]>; +type ExecFileCallback = NonNullable[3]>; -const execMock = ((...args) => { - const cb = args[args.length - 1] as ExecCallback; - cb(null, 'Done', ''); -}) as typeof exec; +const execMock: typeof execFile = ((...args) => { + const currentCallback = args[args.length - 1] as ExecFileCallback; + currentCallback(null, 'Done', ''); +}) as typeof execFile; + +mocked(execFile).mockImplementation(execMock); describe('CommunityPackagesService', () => { const license = mock(); @@ -145,48 +147,49 @@ describe('CommunityPackagesService', () => { describe('executeCommand()', () => { beforeEach(() => { - mocked(exec).mockImplementation(execMock); + mocked(execFile).mockImplementation(execMock); }); test('should call command with valid options', async () => { const execMock = ((...args) => { - const arg = args[1] as ExecOptions; + const arg = args[2] as ExecFileOptions; expect(arg.cwd).toBeDefined(); expect(arg.env).toBeDefined(); // PATH or NODE_PATH may be undefined depending on environment so we don't check for these keys. - const cb = args[args.length - 1] as ExecCallback; + const cb = args[args.length - 1] as ExecFileCallback; cb(null, 'Done', ''); - }) as typeof exec; + }) as typeof execFile; - mocked(exec).mockImplementation(execMock); + mocked(execFile).mockImplementation(execMock); - await communityPackagesService.executeNpmCommand('ls'); + await communityPackagesService.executeNpmCommand(['ls']); - expect(exec).toHaveBeenCalled(); + expect(execFile).toHaveBeenCalled(); }); test('should make sure folder exists', async () => { - mocked(exec).mockImplementation(execMock); + mocked(execFile).mockImplementation(execMock); - await communityPackagesService.executeNpmCommand('ls'); + await communityPackagesService.executeNpmCommand(['ls']); - expect(exec).toHaveBeenCalled(); + expect(execFile).toHaveBeenCalled(); }); test('should throw especial error when package is not found', async () => { const erroringExecMock = ((...args) => { - const cb = args[args.length - 1] as ExecCallback; + const cb = args[args.length - 1] as ExecFileCallback; const msg = `Something went wrong - ${NPM_COMMAND_TOKENS.NPM_PACKAGE_NOT_FOUND_ERROR}. Aborting.`; cb(new Error(msg), '', ''); - }) as typeof exec; + return undefined as any; + }) as typeof execFile; - mocked(exec).mockImplementation(erroringExecMock); + mocked(execFile).mockImplementation(erroringExecMock); - const call = async () => await communityPackagesService.executeNpmCommand('ls'); + const call = async () => await communityPackagesService.executeNpmCommand(['ls']); await expect(call).rejects.toThrowError(RESPONSE_ERROR_MESSAGES.PACKAGE_NOT_FOUND); - expect(exec).toHaveBeenCalled(); + expect(execFile).toHaveBeenCalled(); }); }); @@ -396,19 +399,22 @@ describe('CommunityPackagesService', () => { `--registry=${testBlockRegistry}`, ].join(' '); - const execMockForThisBlock = (command: string, optionsOrCallback: any, callback?: any) => { - const actualCallback = typeof optionsOrCallback === 'function' ? optionsOrCallback : callback; - if (command.startsWith('npm pack') && command.includes(PACKAGE_NAME)) { - actualCallback(null, { stdout: testBlockTarballName, stderr: '' }); + const execMockForThisBlock = ((...args: Parameters) => { + const command = args[0]; + const cmdArgs = args[1]; + const actualCallback = args[args.length - 1] as ExecFileCallback; + + if (command === 'npm' && cmdArgs?.[0] === 'pack') { + actualCallback(null, { stdout: testBlockTarballName } as never, ''); } else { actualCallback(null, 'Done', ''); } - }; + }) as typeof execFile; beforeEach(() => { jest.clearAllMocks(); - mocked(exec).mockImplementation(execMockForThisBlock as typeof exec); + mocked(execFile).mockImplementation(execMockForThisBlock); mocked(readFile).mockResolvedValue( JSON.stringify({ @@ -452,24 +458,27 @@ describe('CommunityPackagesService', () => { path.join(nodesDownloadDir, 'n8n-nodes-test-latest.tgz'), ); - expect(exec).toHaveBeenCalledTimes(3); - expect(exec).toHaveBeenNthCalledWith( + expect(execFile).toHaveBeenCalledTimes(3); + expect(execFile).toHaveBeenNthCalledWith( 1, - `npm pack ${PACKAGE_NAME}@latest --registry=${testBlockRegistry} --quiet`, + 'npm', + ['pack', `${PACKAGE_NAME}@latest`, `--registry=${testBlockRegistry}`, '--quiet'], { cwd: testBlockDownloadDir }, expect.any(Function), ); - expect(exec).toHaveBeenNthCalledWith( + expect(execFile).toHaveBeenNthCalledWith( 2, - `tar -xzf ${testBlockTarballName} -C ${testBlockPackageDir} --strip-components=1`, + 'tar', + ['-xzf', testBlockTarballName, '-C', testBlockPackageDir, '--strip-components=1'], { cwd: testBlockDownloadDir }, expect.any(Function), ); - expect(exec).toHaveBeenNthCalledWith( + expect(execFile).toHaveBeenNthCalledWith( 3, - `npm install ${testBlockNpmInstallArgs}`, + 'npm', + ['install', ...testBlockNpmInstallArgs.split(' ')], { cwd: testBlockPackageDir }, expect.any(Function), ); diff --git a/packages/cli/src/modules/community-packages/community-node.command.ts b/packages/cli/src/modules/community-packages/community-node.command.ts index 2868da31e6a..cb2080576d6 100644 --- a/packages/cli/src/modules/community-packages/community-node.command.ts +++ b/packages/cli/src/modules/community-packages/community-node.command.ts @@ -137,7 +137,7 @@ export class CommunityNode extends BaseCommand> { } async pruneDependencies() { - await Container.get(CommunityPackagesService).executeNpmCommand('npm prune'); + await Container.get(CommunityPackagesService).executeNpmCommand(['prune']); } async deleteCommunityNode(node: InstalledNodes) { diff --git a/packages/cli/src/modules/community-packages/community-packages.controller.ts b/packages/cli/src/modules/community-packages/community-packages.controller.ts index 39923da2d90..d3dac501bb9 100644 --- a/packages/cli/src/modules/community-packages/community-packages.controller.ts +++ b/packages/cli/src/modules/community-packages/community-packages.controller.ts @@ -169,8 +169,9 @@ export class CommunityPackagesController { let pendingUpdates: CommunityPackages.AvailableUpdates | undefined; try { - const command = ['npm', 'outdated', '--json'].join(' '); - await this.communityPackagesService.executeNpmCommand(command, { doNotHandleError: true }); + await this.communityPackagesService.executeNpmCommand(['outdated', '--json'], { + doNotHandleError: true, + }); } catch (error) { // when there are updates, npm exits with code 1 // when there are no updates, command succeeds diff --git a/packages/cli/src/modules/community-packages/community-packages.service.ts b/packages/cli/src/modules/community-packages/community-packages.service.ts index e994afd434e..f25d3b16a30 100644 --- a/packages/cli/src/modules/community-packages/community-packages.service.ts +++ b/packages/cli/src/modules/community-packages/community-packages.service.ts @@ -6,7 +6,7 @@ import axios from 'axios'; import type { PackageDirectoryLoader } from 'n8n-core'; import { InstanceSettings } from 'n8n-core'; import { jsonParse, UnexpectedError, UserError, type PublicInstalledPackage } from 'n8n-workflow'; -import { exec } from 'node:child_process'; +import { execFile } from 'node:child_process'; import { access, constants, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; import { promisify } from 'node:util'; @@ -56,7 +56,7 @@ const { NPM_PACKAGE_VERSION_NOT_FOUND_ERROR, } = NPM_COMMAND_TOKENS; -const asyncExec = promisify(exec); +const asyncExecFile = promisify(execFile); const INVALID_OR_SUSPICIOUS_PACKAGE_NAME = /[^0-9a-z@\-._/]/; @@ -156,7 +156,7 @@ export class CommunityPackagesService { } /** @deprecated */ - async executeNpmCommand(command: string, options?: { doNotHandleError?: boolean }) { + async executeNpmCommand(args: string[], options?: { doNotHandleError?: boolean }) { const execOptions = { cwd: this.downloadFolder, env: { @@ -168,8 +168,7 @@ export class CommunityPackagesService { }; try { - const commandResult = await asyncExec(command, execOptions); - + const commandResult = await asyncExecFile('npm', args, execOptions); return commandResult.stdout; } catch (error) { if (options?.doNotHandleError) throw error; @@ -376,9 +375,7 @@ export class CommunityPackagesService { } private getNpmInstallArgs() { - return [...NPM_COMMON_ARGS, ...NPM_INSTALL_ARGS, `--registry=${this.getNpmRegistry()}`].join( - ' ', - ); + return [...NPM_COMMON_ARGS, ...NPM_INSTALL_ARGS, `--registry=${this.getNpmRegistry()}`]; } private checkInstallPermissions(checksumProvided: boolean) { @@ -497,18 +494,20 @@ export class CommunityPackagesService { // TODO: make sure that this works for scoped packages as well // if (packageName.startsWith('@') && packageName.includes('/')) {} - - const { stdout: tarOutput } = await asyncExec( - `npm pack ${packageName}@${packageVersion} --registry=${registry} --quiet`, + const { stdout: tarOutput } = await asyncExecFile( + 'npm', + ['pack', `${packageName}@${packageVersion}`, `--registry=${registry}`, '--quiet'], { cwd: this.downloadFolder }, ); const tarballName = tarOutput?.trim(); try { - await asyncExec(`tar -xzf ${tarballName} -C ${packageDirectory} --strip-components=1`, { - cwd: this.downloadFolder, - }); + await asyncExecFile( + 'tar', + ['-xzf', tarballName, '-C', packageDirectory, '--strip-components=1'], + { cwd: this.downloadFolder }, + ); // Strip dev, optional, and peer dependencies before running `npm install` const packageJsonPath = `${packageDirectory}/package.json`; @@ -527,7 +526,9 @@ export class CommunityPackagesService { } = JSON.parse(packageJsonContent); await writeFile(packageJsonPath, JSON.stringify(packageJson, null, 2), 'utf-8'); - await asyncExec(`npm install ${this.getNpmInstallArgs()}`, { cwd: packageDirectory }); + await asyncExecFile('npm', ['install', ...this.getNpmInstallArgs()], { + cwd: packageDirectory, + }); await this.updatePackageJsonDependency(packageName, packageJson.version); } finally { await rm(join(this.downloadFolder, tarballName));