From eb2d1ca3570c835b941f92b9f215b15b9f10d630 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Thu, 10 Oct 2024 12:44:02 +0300 Subject: [PATCH] feat: Add configurable require support to js task runner (no-changelog) (#11184) --- .../__tests__/js-task-runner.test.ts | 268 +++++++++++++++++- .../src/js-task-runner/js-task-runner.ts | 50 +++- .../src/js-task-runner/require-resolver.ts | 43 +++ packages/@n8n/task-runner/src/start.ts | 8 +- .../__tests__/task-runner-process.test.ts | 48 ++++ .../cli/src/runners/task-runner-process.ts | 7 +- 6 files changed, 404 insertions(+), 20 deletions(-) create mode 100644 packages/@n8n/task-runner/src/js-task-runner/require-resolver.ts create mode 100644 packages/cli/src/runners/__tests__/task-runner-process.test.ts diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts index fec14acc8c..c940966fbd 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts @@ -1,7 +1,10 @@ import { DateTime } from 'luxon'; import type { CodeExecutionMode, IDataObject } from 'n8n-workflow'; +import fs from 'node:fs'; +import { builtinModules } from 'node:module'; import { ValidationError } from '@/js-task-runner/errors/validation-error'; +import type { JsTaskRunnerOpts } from '@/js-task-runner/js-task-runner'; import { JsTaskRunner, type AllCodeTaskData, @@ -14,17 +17,27 @@ import { newAllCodeTaskData, newTaskWithSettings, withPairedItem, wrapIntoJson } jest.mock('ws'); describe('JsTaskRunner', () => { - const jsTaskRunner = new JsTaskRunner('taskType', 'ws://localhost', 'grantToken', 1); + const createRunnerWithOpts = (opts: Partial = {}) => + new JsTaskRunner({ + wsUrl: 'ws://localhost', + grantToken: 'grantToken', + maxConcurrency: 1, + ...opts, + }); + + const defaultTaskRunner = createRunnerWithOpts(); const execTaskWithParams = async ({ task, taskData, + runner = defaultTaskRunner, }: { task: Task; taskData: AllCodeTaskData; + runner?: JsTaskRunner; }) => { - jest.spyOn(jsTaskRunner, 'requestData').mockResolvedValue(taskData); - return await jsTaskRunner.executeTask(task); + jest.spyOn(runner, 'requestData').mockResolvedValue(taskData); + return await runner.executeTask(task); }; afterEach(() => { @@ -35,7 +48,13 @@ describe('JsTaskRunner', () => { code, inputItems, settings, - }: { code: string; inputItems: IDataObject[]; settings?: Partial }) => { + runner, + }: { + code: string; + inputItems: IDataObject[]; + settings?: Partial; + runner?: JsTaskRunner; + }) => { return await execTaskWithParams({ task: newTaskWithSettings({ code, @@ -43,6 +62,7 @@ describe('JsTaskRunner', () => { ...settings, }), taskData: newAllCodeTaskData(inputItems.map(wrapIntoJson)), + runner, }); }; @@ -50,7 +70,14 @@ describe('JsTaskRunner', () => { code, inputItems, settings, - }: { code: string; inputItems: IDataObject[]; settings?: Partial }) => { + runner, + }: { + code: string; + inputItems: IDataObject[]; + settings?: Partial; + + runner?: JsTaskRunner; + }) => { return await execTaskWithParams({ task: newTaskWithSettings({ code, @@ -58,6 +85,7 @@ describe('JsTaskRunner', () => { ...settings, }), taskData: newAllCodeTaskData(inputItems.map(wrapIntoJson)), + runner, }); }; @@ -65,7 +93,7 @@ describe('JsTaskRunner', () => { test.each<[CodeExecutionMode]>([['runOnceForAllItems'], ['runOnceForEachItem']])( 'should make an rpc call for console log in %s mode', async (nodeMode) => { - jest.spyOn(jsTaskRunner, 'makeRpcCall').mockResolvedValue(undefined); + jest.spyOn(defaultTaskRunner, 'makeRpcCall').mockResolvedValue(undefined); const task = newTaskWithSettings({ code: "console.log('Hello', 'world!'); return {}", nodeMode, @@ -76,7 +104,7 @@ describe('JsTaskRunner', () => { taskData: newAllCodeTaskData([wrapIntoJson({})]), }); - expect(jsTaskRunner.makeRpcCall).toHaveBeenCalledWith(task.taskId, 'logNodeOutput', [ + expect(defaultTaskRunner.makeRpcCall).toHaveBeenCalledWith(task.taskId, 'logNodeOutput', [ 'Hello world!', ]); }, @@ -452,4 +480,230 @@ describe('JsTaskRunner', () => { }, ); }); + + describe('require', () => { + const inputItems = [{ a: 1 }]; + const packageJson = JSON.parse(fs.readFileSync('package.json', 'utf8')); + + describe('blocked by default', () => { + const testCases = [...builtinModules, ...Object.keys(packageJson.dependencies)]; + + test.each(testCases)( + 'should throw an error when requiring %s in runOnceForAllItems mode', + async (module) => { + await expect( + executeForAllItems({ + code: `return require('${module}')`, + inputItems, + }), + ).rejects.toThrow(`Cannot find module '${module}'`); + }, + ); + + test.each(testCases)( + 'should throw an error when requiring %s in runOnceForEachItem mode', + async (module) => { + await expect( + executeForEachItem({ + code: `return require('${module}')`, + inputItems, + }), + ).rejects.toThrow(`Cannot find module '${module}'`); + }, + ); + }); + + describe('all built-ins allowed with *', () => { + const testCases = builtinModules; + const runner = createRunnerWithOpts({ + allowedBuiltInModules: '*', + }); + + test.each(testCases)( + 'should be able to require %s in runOnceForAllItems mode', + async (module) => { + await expect( + executeForAllItems({ + code: `return { val: require('${module}') }`, + inputItems, + runner, + }), + ).resolves.toBeDefined(); + }, + ); + + test.each(testCases)( + 'should be able to require %s in runOnceForEachItem mode', + async (module) => { + await expect( + executeForEachItem({ + code: `return { val: require('${module}') }`, + inputItems, + runner, + }), + ).resolves.toBeDefined(); + }, + ); + }); + + describe('all external modules allowed with *', () => { + const testCases = Object.keys(packageJson.dependencies); + const runner = createRunnerWithOpts({ + allowedExternalModules: '*', + }); + + test.each(testCases)( + 'should be able to require %s in runOnceForAllItems mode', + async (module) => { + await expect( + executeForAllItems({ + code: `return { val: require('${module}') }`, + inputItems, + runner, + }), + ).resolves.toBeDefined(); + }, + ); + + test.each(testCases)( + 'should be able to require %s in runOnceForEachItem mode', + async (module) => { + await expect( + executeForEachItem({ + code: `return { val: require('${module}') }`, + inputItems, + runner, + }), + ).resolves.toBeDefined(); + }, + ); + }); + + describe('specifically allowed built-in modules', () => { + const runner = createRunnerWithOpts({ + allowedBuiltInModules: 'crypto,path', + }); + + const allowedCases = [ + ['crypto', 'require("crypto").randomBytes(16).toString("hex")', expect.any(String)], + ['path', 'require("path").normalize("/root/./dir")', '/root/dir'], + ]; + + const blockedCases = [['http'], ['process']]; + + test.each(allowedCases)( + 'should allow requiring %s in runOnceForAllItems mode', + async (_moduleName, expression, expected) => { + const outcome = await executeForAllItems({ + code: `return { val: ${expression} }`, + inputItems, + runner, + }); + + expect(outcome.result).toEqual([wrapIntoJson({ val: expected })]); + }, + ); + + test.each(allowedCases)( + 'should allow requiring %s in runOnceForEachItem mode', + async (_moduleName, expression, expected) => { + const outcome = await executeForEachItem({ + code: `return { val: ${expression} }`, + inputItems, + runner, + }); + + expect(outcome.result).toEqual([withPairedItem(0, wrapIntoJson({ val: expected }))]); + }, + ); + + test.each(blockedCases)( + 'should throw when trying to require %s in runOnceForAllItems mode', + async (moduleName) => { + await expect( + executeForAllItems({ + code: `require("${moduleName}")`, + inputItems, + runner, + }), + ).rejects.toThrow(`Cannot find module '${moduleName}'`); + }, + ); + + test.each(blockedCases)( + 'should throw when trying to require %s in runOnceForEachItem mode', + async (moduleName) => { + await expect( + executeForEachItem({ + code: `require("${moduleName}")`, + inputItems, + runner, + }), + ).rejects.toThrow(`Cannot find module '${moduleName}'`); + }, + ); + }); + + describe('specifically allowed external modules', () => { + const runner = createRunnerWithOpts({ + allowedExternalModules: 'nanoid', + }); + + const allowedCases = [['nanoid', 'require("nanoid").nanoid()', expect.any(String)]]; + + const blockedCases = [['n8n-core']]; + + test.each(allowedCases)( + 'should allow requiring %s in runOnceForAllItems mode', + async (_moduleName, expression, expected) => { + const outcome = await executeForAllItems({ + code: `return { val: ${expression} }`, + inputItems, + runner, + }); + + expect(outcome.result).toEqual([wrapIntoJson({ val: expected })]); + }, + ); + + test.each(allowedCases)( + 'should allow requiring %s in runOnceForEachItem mode', + async (_moduleName, expression, expected) => { + const outcome = await executeForEachItem({ + code: `return { val: ${expression} }`, + inputItems, + runner, + }); + + expect(outcome.result).toEqual([withPairedItem(0, wrapIntoJson({ val: expected }))]); + }, + ); + + test.each(blockedCases)( + 'should throw when trying to require %s in runOnceForAllItems mode', + async (moduleName) => { + await expect( + executeForAllItems({ + code: `require("${moduleName}")`, + inputItems, + runner, + }), + ).rejects.toThrow(`Cannot find module '${moduleName}'`); + }, + ); + + test.each(blockedCases)( + 'should throw when trying to require %s in runOnceForEachItem mode', + async (moduleName) => { + await expect( + executeForEachItem({ + code: `require("${moduleName}")`, + inputItems, + runner, + }), + ).rejects.toThrow(`Cannot find module '${moduleName}'`); + }, + ); + }); + }); }); diff --git a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts index 3547e35b6c..4b861fd3cc 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts @@ -25,6 +25,8 @@ import { runInNewContext, type Context } from 'node:vm'; import type { TaskResultData } from '@/runner-types'; import { type Task, TaskRunner } from '@/task-runner'; +import type { RequireResolver } from './require-resolver'; +import { createRequireResolver } from './require-resolver'; import { validateRunForAllItemsOutput, validateRunForEachItemOutput } from './result-validation'; export interface JSExecSettings { @@ -72,19 +74,47 @@ export interface AllCodeTaskData { additionalData: PartialAdditionalData; } +export interface JsTaskRunnerOpts { + wsUrl: string; + grantToken: string; + maxConcurrency: number; + name?: string; + /** + * List of built-in nodejs modules that are allowed to be required in the + * execution sandbox. Asterisk (*) can be used to allow all. + */ + allowedBuiltInModules?: string; + /** + * List of npm modules that are allowed to be required in the execution + * sandbox. Asterisk (*) can be used to allow all. + */ + allowedExternalModules?: string; +} + type CustomConsole = { log: (...args: unknown[]) => void; }; export class JsTaskRunner extends TaskRunner { - constructor( - taskType: string, - wsUrl: string, - grantToken: string, - maxConcurrency: number, - name?: string, - ) { - super(taskType, wsUrl, grantToken, maxConcurrency, name ?? 'JS Task Runner'); + private readonly requireResolver: RequireResolver; + + constructor({ + grantToken, + maxConcurrency, + wsUrl, + name = 'JS Task Runner', + allowedBuiltInModules, + allowedExternalModules, + }: JsTaskRunnerOpts) { + super('javascript', wsUrl, grantToken, maxConcurrency, name); + + const parseModuleAllowList = (moduleList: string) => + moduleList === '*' ? null : new Set(moduleList.split(',').map((x) => x.trim())); + + this.requireResolver = createRequireResolver({ + allowedBuiltInModules: parseModuleAllowList(allowedBuiltInModules ?? ''), + allowedExternalModules: parseModuleAllowList(allowedExternalModules ?? ''), + }); } async executeTask(task: Task): Promise { @@ -145,7 +175,7 @@ export class JsTaskRunner extends TaskRunner { const inputItems = allData.connectionInputData; const context: Context = { - require, + require: this.requireResolver, module: {}, console: customConsole, @@ -192,7 +222,7 @@ export class JsTaskRunner extends TaskRunner { const item = inputItems[index]; const dataProxy = this.createDataProxy(allData, workflow, index); const context: Context = { - require, + require: this.requireResolver, module: {}, console: customConsole, item, diff --git a/packages/@n8n/task-runner/src/js-task-runner/require-resolver.ts b/packages/@n8n/task-runner/src/js-task-runner/require-resolver.ts new file mode 100644 index 0000000000..ffa00c0441 --- /dev/null +++ b/packages/@n8n/task-runner/src/js-task-runner/require-resolver.ts @@ -0,0 +1,43 @@ +import { ApplicationError } from 'n8n-workflow'; +import { isBuiltin } from 'node:module'; + +import { ExecutionError } from './errors/execution-error'; + +export type RequireResolverOpts = { + /** + * List of built-in nodejs modules that are allowed to be required in the + * execution sandbox. `null` means all are allowed. + */ + allowedBuiltInModules: Set | null; + + /** + * List of external modules that are allowed to be required in the + * execution sandbox. `null` means all are allowed. + */ + allowedExternalModules: Set | null; +}; + +export type RequireResolver = (request: string) => unknown; + +export function createRequireResolver({ + allowedBuiltInModules, + allowedExternalModules, +}: RequireResolverOpts) { + return (request: string) => { + const checkIsAllowed = (allowList: Set | null, moduleName: string) => { + return allowList ? allowList.has(moduleName) : true; + }; + + const isAllowed = isBuiltin(request) + ? checkIsAllowed(allowedBuiltInModules, request) + : checkIsAllowed(allowedExternalModules, request); + + if (!isAllowed) { + const error = new ApplicationError(`Cannot find module '${request}'`); + throw new ExecutionError(error); + } + + // eslint-disable-next-line @typescript-eslint/no-var-requires + return require(request) as unknown; + }; +} diff --git a/packages/@n8n/task-runner/src/start.ts b/packages/@n8n/task-runner/src/start.ts index 7570a11780..5f856140d9 100644 --- a/packages/@n8n/task-runner/src/start.ts +++ b/packages/@n8n/task-runner/src/start.ts @@ -66,7 +66,13 @@ void (async function start() { } const wsUrl = `ws://${config.n8nUri}/runners/_ws`; - runner = new JsTaskRunner('javascript', wsUrl, grantToken, 5); + runner = new JsTaskRunner({ + wsUrl, + grantToken, + maxConcurrency: 5, + allowedBuiltInModules: process.env.NODE_FUNCTION_ALLOW_BUILTIN, + allowedExternalModules: process.env.NODE_FUNCTION_ALLOW_EXTERNAL, + }); process.on('SIGINT', createSignalHandler('SIGINT')); process.on('SIGTERM', createSignalHandler('SIGTERM')); diff --git a/packages/cli/src/runners/__tests__/task-runner-process.test.ts b/packages/cli/src/runners/__tests__/task-runner-process.test.ts new file mode 100644 index 0000000000..b2ad678ee1 --- /dev/null +++ b/packages/cli/src/runners/__tests__/task-runner-process.test.ts @@ -0,0 +1,48 @@ +import { GlobalConfig } from '@n8n/config'; +import { mock } from 'jest-mock-extended'; +import type { ChildProcess, SpawnOptions } from 'node:child_process'; + +import { mockInstance } from '../../../test/shared/mocking'; +import type { TaskRunnerAuthService } from '../auth/task-runner-auth.service'; +import { TaskRunnerProcess } from '../task-runner-process'; + +const spawnMock = jest.fn(() => + mock({ + stdout: { + pipe: jest.fn(), + }, + stderr: { + pipe: jest.fn(), + }, + }), +); +require('child_process').spawn = spawnMock; + +describe('TaskRunnerProcess', () => { + const globalConfig = mockInstance(GlobalConfig); + const authService = mock(); + const taskRunnerProcess = new TaskRunnerProcess(globalConfig, authService); + + afterEach(async () => { + spawnMock.mockClear(); + }); + + describe('start', () => { + it('should propagate NODE_FUNCTION_ALLOW_BUILTIN and NODE_FUNCTION_ALLOW_EXTERNAL from env', async () => { + jest.spyOn(authService, 'createGrantToken').mockResolvedValue('grantToken'); + process.env.NODE_FUNCTION_ALLOW_BUILTIN = '*'; + process.env.NODE_FUNCTION_ALLOW_EXTERNAL = '*'; + + await taskRunnerProcess.start(); + + // @ts-expect-error The type is not correct + const options = spawnMock.mock.calls[0][2] as SpawnOptions; + expect(options.env).toEqual( + expect.objectContaining({ + NODE_FUNCTION_ALLOW_BUILTIN: '*', + NODE_FUNCTION_ALLOW_EXTERNAL: '*', + }), + ); + }); + }); +}); diff --git a/packages/cli/src/runners/task-runner-process.ts b/packages/cli/src/runners/task-runner-process.ts index 5f420ab568..9f570fcb38 100644 --- a/packages/cli/src/runners/task-runner-process.ts +++ b/packages/cli/src/runners/task-runner-process.ts @@ -1,6 +1,7 @@ import { GlobalConfig } from '@n8n/config'; import * as a from 'node:assert/strict'; import { spawn } from 'node:child_process'; +import * as process from 'node:process'; import { Service } from 'typedi'; import { TaskRunnerAuthService } from './auth/task-runner-auth.service'; @@ -45,6 +46,8 @@ export class TaskRunnerProcess { PATH: process.env.PATH, N8N_RUNNERS_GRANT_TOKEN: grantToken, N8N_RUNNERS_N8N_URI: `127.0.0.1:${this.globalConfig.taskRunners.port}`, + NODE_FUNCTION_ALLOW_BUILTIN: process.env.NODE_FUNCTION_ALLOW_BUILTIN, + NODE_FUNCTION_ALLOW_EXTERNAL: process.env.NODE_FUNCTION_ALLOW_EXTERNAL, }, }); @@ -69,9 +72,9 @@ export class TaskRunnerProcess { this.isShuttingDown = false; } - private monitorProcess(process: ChildProcess) { + private monitorProcess(taskRunnerProcess: ChildProcess) { this.runPromise = new Promise((resolve) => { - process.on('exit', (code) => { + taskRunnerProcess.on('exit', (code) => { this.onProcessExit(code, resolve); }); });