From 3d9d5bf9d58f3c49830d42a140d6c8c6b59952dc Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Thu, 23 Jan 2025 10:18:00 +0200 Subject: [PATCH] fix(core): Fix usage of external libs in task runner (#12788) Co-authored-by: Cornelius Suermann --- .../__tests__/require-resolver.test.ts | 78 +++++++++++++++++++ .../src/js-task-runner/js-task-runner.ts | 26 +++++-- .../src/js-task-runner/require-resolver.ts | 12 +-- 3 files changed, 105 insertions(+), 11 deletions(-) create mode 100644 packages/@n8n/task-runner/src/js-task-runner/__tests__/require-resolver.test.ts diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/require-resolver.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/require-resolver.test.ts new file mode 100644 index 0000000000..16e0f99e7c --- /dev/null +++ b/packages/@n8n/task-runner/src/js-task-runner/__tests__/require-resolver.test.ts @@ -0,0 +1,78 @@ +import { ApplicationError } from 'n8n-workflow'; + +import { ExecutionError } from '@/js-task-runner/errors/execution-error'; + +import { createRequireResolver, type RequireResolverOpts } from '../require-resolver'; + +describe('require resolver', () => { + let defaultOpts: RequireResolverOpts; + + beforeEach(() => { + defaultOpts = { + allowedBuiltInModules: new Set(['path', 'fs']), + allowedExternalModules: new Set(['lodash']), + }; + }); + + describe('built-in modules', () => { + it('should allow requiring whitelisted built-in modules', () => { + const resolver = createRequireResolver(defaultOpts); + expect(() => resolver('path')).not.toThrow(); + expect(() => resolver('fs')).not.toThrow(); + }); + + it('should throw when requiring non-whitelisted built-in modules', () => { + const resolver = createRequireResolver(defaultOpts); + expect(() => resolver('crypto')).toThrow(ExecutionError); + }); + + it('should allow all built-in modules when allowedBuiltInModules is "*"', () => { + const resolver = createRequireResolver({ + ...defaultOpts, + allowedBuiltInModules: '*', + }); + + expect(() => resolver('path')).not.toThrow(); + expect(() => resolver('crypto')).not.toThrow(); + expect(() => resolver('fs')).not.toThrow(); + }); + }); + + describe('external modules', () => { + it('should allow requiring whitelisted external modules', () => { + const resolver = createRequireResolver(defaultOpts); + expect(() => resolver('lodash')).not.toThrow(); + }); + + it('should throw when requiring non-whitelisted external modules', () => { + const resolver = createRequireResolver(defaultOpts); + expect(() => resolver('express')).toThrow( + new ExecutionError(new ApplicationError("Cannot find module 'express'")), + ); + }); + + it('should allow all external modules when allowedExternalModules is "*"', () => { + const resolver = createRequireResolver({ + ...defaultOpts, + allowedExternalModules: '*', + }); + + expect(() => resolver('lodash')).not.toThrow(); + expect(() => resolver('express')).not.toThrow(); + }); + }); + + describe('error handling', () => { + it('should wrap ApplicationError in ExecutionError', () => { + const resolver = createRequireResolver(defaultOpts); + expect(() => resolver('non-existent-module')).toThrow(ExecutionError); + }); + + it('should include the module name in the error message', () => { + const resolver = createRequireResolver(defaultOpts); + expect(() => resolver('non-existent-module')).toThrow( + "Cannot find module 'non-existent-module'", + ); + }); + }); +}); 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 44745a850c..36639a3a0e 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 @@ -98,17 +98,33 @@ export class JsTaskRunner extends TaskRunner { const { jsRunnerConfig } = config; const parseModuleAllowList = (moduleList: string) => - moduleList === '*' ? null : new Set(moduleList.split(',').map((x) => x.trim())); + moduleList === '*' ? '*' : new Set(moduleList.split(',').map((x) => x.trim())); + + const allowedBuiltInModules = parseModuleAllowList(jsRunnerConfig.allowedBuiltInModules ?? ''); + const allowedExternalModules = parseModuleAllowList( + jsRunnerConfig.allowedExternalModules ?? '', + ); this.requireResolver = createRequireResolver({ - allowedBuiltInModules: parseModuleAllowList(jsRunnerConfig.allowedBuiltInModules ?? ''), - allowedExternalModules: parseModuleAllowList(jsRunnerConfig.allowedExternalModules ?? ''), + allowedBuiltInModules, + allowedExternalModules, }); - this.preventPrototypePollution(); + this.preventPrototypePollution(allowedExternalModules); } - private preventPrototypePollution() { + private preventPrototypePollution(allowedExternalModules: Set | '*') { + if (allowedExternalModules instanceof Set) { + // This is a workaround to enable the allowed external libraries to mutate + // prototypes directly. For example momentjs overrides .toString() directly + // on the Moment.prototype, which doesn't work if Object.prototype has been + // frozen. This works as long as the overrides are done when the library is + // imported. + for (const module of allowedExternalModules) { + require(module); + } + } + // Freeze globals, except for Jest if (process.env.NODE_ENV !== 'test') { Object.getOwnPropertyNames(globalThis) 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 index ffa00c0441..4facbd365c 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/require-resolver.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/require-resolver.ts @@ -6,15 +6,15 @@ 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. + * execution sandbox. `"*"` means all are allowed. */ - allowedBuiltInModules: Set | null; + allowedBuiltInModules: Set | '*'; /** * List of external modules that are allowed to be required in the - * execution sandbox. `null` means all are allowed. + * execution sandbox. `"*"` means all are allowed. */ - allowedExternalModules: Set | null; + allowedExternalModules: Set | '*'; }; export type RequireResolver = (request: string) => unknown; @@ -24,8 +24,8 @@ export function createRequireResolver({ allowedExternalModules, }: RequireResolverOpts) { return (request: string) => { - const checkIsAllowed = (allowList: Set | null, moduleName: string) => { - return allowList ? allowList.has(moduleName) : true; + const checkIsAllowed = (allowList: Set | '*', moduleName: string) => { + return allowList === '*' || allowList.has(moduleName); }; const isAllowed = isBuiltin(request)