opensource/n8n
3
0
Fork 0
mirror of https://github.com/n8n-io/n8n.git synced 2026-09-24 23:22:38 +08:00
Code Issues Packages Projects Releases Wiki Activity

fix(core): Use execFile instead of exec to prevent command injection (#21952)

Browse Source
This commit is contained in:
Elias Meire
2025-11-18 11:06:18 +02:00
committed by GitHub
parent 40b1fc9a4f
commit 296684e67c
5 changed files with 65 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts
+1 -1
View File
@@ -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);
});
packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts
+44 -35
View File
@@ -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<Parameters<typeof exec>[1]>;
type ExecCallback = NonNullable<Parameters<typeof exec>[2]>;
type ExecFileOptions = NonNullable<Parameters<typeof execFile>[2]>;
type ExecFileCallback = NonNullable<Parameters<typeof execFile>[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<License>();
@@ -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<typeof execFile>) => {
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),
);
packages/cli/src/modules/community-packages/community-node.command.ts
+1 -1
View File
@@ -137,7 +137,7 @@ export class CommunityNode extends BaseCommand<z.infer<typeof flagsSchema>> {
}
async pruneDependencies() {
await Container.get(CommunityPackagesService).executeNpmCommand('npm prune');
await Container.get(CommunityPackagesService).executeNpmCommand(['prune']);
}
async deleteCommunityNode(node: InstalledNodes) {
packages/cli/src/modules/community-packages/community-packages.controller.ts
+3 -2
View File
@@ -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
packages/cli/src/modules/community-packages/community-packages.service.ts
+16 -15
View File
@@ -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));