From 569ad497b7bd2cbb61b5fc256da3dae2976fddff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 9 Apr 2026 11:14:57 +0200 Subject: [PATCH] fix(core): Align VM expression engine error handler with legacy engine (#28166) Co-authored-by: Danny Martini Co-authored-by: Claude Opus 4.6 (1M context) --- .../src/__tests__/integration.test.ts | 101 +++++++++-------- .../src/bridge/isolated-vm-bridge.ts | 35 ++++-- packages/workflow/test/expression.test.ts | 104 ++++++++++++++++++ 3 files changed, 177 insertions(+), 63 deletions(-) diff --git a/packages/@n8n/expression-runtime/src/__tests__/integration.test.ts b/packages/@n8n/expression-runtime/src/__tests__/integration.test.ts index 8e4285a8728..57e7c54f476 100644 --- a/packages/@n8n/expression-runtime/src/__tests__/integration.test.ts +++ b/packages/@n8n/expression-runtime/src/__tests__/integration.test.ts @@ -377,58 +377,34 @@ describe('Integration: ExpressionEvaluator + IsolatedVmBridge', () => { it('should handle throw null without crashing', () => { const data = { $json: {} }; - let error: Error | undefined; - try { - evaluator.evaluate('{{ (() => { throw null })() }}', data, caller); - } catch (e) { - error = e as Error; - } - expect(error).toBeDefined(); - expect(error?.message).not.toContain('Cannot read properties'); + expect(evaluator.evaluate('{{ (() => { throw null })() }}', data, caller)).toBeUndefined(); }); it('should handle throw undefined without crashing', () => { const data = { $json: {} }; - let error: Error | undefined; - try { - evaluator.evaluate('{{ (() => { throw undefined })() }}', data, caller); - } catch (e) { - error = e as Error; - } - expect(error).toBeDefined(); - expect(error?.message).not.toContain('Cannot read properties'); + expect(evaluator.evaluate('{{ (() => { throw undefined })() }}', data, caller)).toBeUndefined(); }); it('should handle throw of null-prototype object with properties without crashing', () => { const data = { $json: {} }; - let error: Error | undefined; - try { + expect( evaluator.evaluate( '{{ (() => { var e = Object.create(null); e.foo = "bar"; throw e; })() }}', data, caller, - ); - } catch (e) { - error = e as Error; - } - expect(error).toBeDefined(); - expect(error?.message).not.toContain('hasOwnProperty is not a function'); + ), + ).toBeUndefined(); }); it('should handle throw of object with hasOwnProperty shadowed by null without crashing', () => { const data = { $json: {} }; - let error: Error | undefined; - try { + expect( evaluator.evaluate( '{{ (() => { throw { hasOwnProperty: null, foo: "bar" }; })() }}', data, caller, - ); - } catch (e) { - error = e as Error; - } - expect(error).toBeDefined(); - expect(error?.message).not.toContain('hasOwnProperty is not a function'); + ), + ).toBeUndefined(); }); it('should swallow TypeError and return undefined', () => { @@ -445,19 +421,48 @@ describe('Integration: ExpressionEvaluator + IsolatedVmBridge', () => { expect(result).toBeUndefined(); }); - it('should propagate errors thrown when reading a property across the isolate boundary', () => { + it('should re-throw ExpressionError from host-side callbacks', () => { + const json = { + get brokenProp() { + const err = new Error('paired item failed'); + err.name = 'ExpressionError'; + throw err; + }, + }; + + expect(() => evaluator.evaluate('{{ $json.brokenProp }}', { $json: json }, caller)).toThrow( + expect.objectContaining({ name: 'ExpressionError', message: 'paired item failed' }), + ); + }); + + it('should re-throw ExpressionExtensionError from host-side callbacks', () => { + const json = { + get brokenProp() { + const err = new Error('extension failed'); + err.name = 'ExpressionExtensionError'; + throw err; + }, + }; + + expect(() => evaluator.evaluate('{{ $json.brokenProp }}', { $json: json }, caller)).toThrow( + expect.objectContaining({ + name: 'ExpressionExtensionError', + message: 'extension failed', + }), + ); + }); + + it('should swallow generic errors thrown when reading a property across the isolate boundary', () => { const json = { get brokenProp() { throw new Error('property access failed'); }, }; - expect(() => evaluator.evaluate('{{ $json.brokenProp }}', { $json: json }, caller)).toThrow( - 'property access failed', - ); + expect(evaluator.evaluate('{{ $json.brokenProp }}', { $json: json }, caller)).toBeUndefined(); }); - it('should propagate errors thrown by functions accessed via the lazy proxy', () => { + it('should swallow generic errors thrown by functions accessed via the lazy proxy', () => { const data = { $json: { myFn() { @@ -466,22 +471,20 @@ describe('Integration: ExpressionEvaluator + IsolatedVmBridge', () => { }, }; - expect(() => evaluator.evaluate('{{ $json.myFn() }}', data, caller)).toThrow('function threw'); + expect(evaluator.evaluate('{{ $json.myFn() }}', data, caller)).toBeUndefined(); }); - it('should propagate errors from $items() when result properties are accessed', () => { + it('should swallow generic errors from $items() when result properties are accessed', () => { const data = { $items() { throw new Error('items failed'); }, }; - // Without throwIfErrorSentinel in the $items wrapper, the sentinel is - // returned as a value and .length reads undefined on it — silently swallowed - expect(() => evaluator.evaluate('{{ $items().length }}', data, caller)).toThrow('items failed'); + expect(evaluator.evaluate('{{ $items().length }}', data, caller)).toBeUndefined(); }); - it('should propagate errors thrown during array element access across the isolate boundary', () => { + it('should swallow generic errors thrown during array element access across the isolate boundary', () => { const items = [1, 2, 3]; Object.defineProperty(items, '0', { get() { @@ -493,12 +496,10 @@ describe('Integration: ExpressionEvaluator + IsolatedVmBridge', () => { const data = { $json: { items } }; - expect(() => evaluator.evaluate('{{ $json.items[0] }}', data, caller)).toThrow( - 'element access failed', - ); + expect(evaluator.evaluate('{{ $json.items[0] }}', data, caller)).toBeUndefined(); }); - it('should propagate errors thrown during an "in" operator check across the isolate boundary', () => { + it('should swallow generic errors thrown during an "in" operator check across the isolate boundary', () => { const json = { get brokenProp() { throw new Error('in-check access failed'); @@ -507,11 +508,9 @@ describe('Integration: ExpressionEvaluator + IsolatedVmBridge', () => { // The 'in' operator triggers the has trap on $json proxy. // The bridge calls __getValueAtPath(['$json', 'brokenProp']) which throws. - // Without throwIfErrorSentinel in the has trap, the sentinel is returned - // as a non-undefined value so 'brokenProp' in $json incorrectly returns true. - expect(() => + expect( evaluator.evaluate('{{ "brokenProp" in $json }}', { $json: json }, caller), - ).toThrow('in-check access failed'); + ).toBeUndefined(); }); }); diff --git a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts index bc7c5eeec6b..5da8718371f 100644 --- a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts +++ b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts @@ -252,11 +252,22 @@ export class IsolatedVmBridge implements RuntimeBridge { /** * Inject the E() error handler into the isolate context. * - * Tournament wraps expressions with try-catch that calls E(error, this). - * This handler: - * - Re-throws security violations from __sanitize - * - Swallows TypeErrors (failed attack attempts return undefined) - * - Re-throws all other errors + * There are two exception-handling layers inside the isolate: + * + * 1. **Inner layer (this handler, `E()`)** — Tournament wraps each + * expression with try-catch that calls `E(error, this)`. This handler + * must match the legacy engine's behavior (set in expression.ts via + * setErrorHandler): + * - Re-throw ExpressionError / ExpressionExtensionError + * - Swallow everything else (TypeErrors, generic Errors, etc.) + * + * 2. **Outer layer (`wrappedCode` try-catch in `execute()`)** — Catches + * anything that escaped `E()` (e.g. re-thrown ExpressionErrors) and + * serializes it into a sentinel object so the host can reconstruct it. + * + * Inside the isolate, errors from host callbacks arrive as sentinel + * objects ({ __isError, name, message, ... }) rather than class instances, + * so we match by name instead of instanceof. * * @private * @throws {Error} If context not initialized @@ -269,15 +280,15 @@ export class IsolatedVmBridge implements RuntimeBridge { await this.context.eval(` if (typeof E === 'undefined') { globalThis.E = function(error, _context) { - // Re-throw security violations from __sanitize - if (error && error.message && error.message.includes('due to security concerns')) { + // Re-throw ExpressionError / ExpressionExtensionError to match + // the legacy handler in expression.ts. Errors from host callbacks + // arrive as sentinels (not class instances), so check by name. + const name = error?.name; + if (name === 'ExpressionError' || name === 'ExpressionExtensionError') { throw error; } - // Swallow TypeErrors (failed attack attempts return undefined) - if (error instanceof TypeError) { - return undefined; - } - throw error; + // Swallow everything else (TypeErrors, generic Errors, etc.) + return undefined; }; } `); diff --git a/packages/workflow/test/expression.test.ts b/packages/workflow/test/expression.test.ts index 0de79300b5f..095b1b6ee34 100644 --- a/packages/workflow/test/expression.test.ts +++ b/packages/workflow/test/expression.test.ts @@ -10,6 +10,7 @@ import { ExpressionReservedVariableError } from '../src/errors/expression-reserv import { ExpressionError } from '../src/errors/expression.error'; import { Expression } from '../src/expression'; import { extendSyntax } from '../src/extensions/expression-extension'; +import { createRunExecutionData } from '../src'; import type { INodeExecutionData } from '../src/interfaces'; import { Workflow } from '../src/workflow'; import { WorkflowDataProxy } from '../src/workflow-data-proxy'; @@ -924,6 +925,109 @@ describe('Expression', () => { }); }); + describe('$() node reference through expression engine', () => { + const nodeTypes = Helpers.NodeTypes(); + + function createTestWorkflow(connected: boolean) { + return new Workflow({ + id: 'test-dollar-ref', + name: 'Test', + nodes: [ + { + id: 'source-id', + name: 'source', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + { + id: 'consumer-id', + name: 'consumer', + type: 'n8n-nodes-base.set', + typeVersion: 1, + position: [200, 0], + parameters: {}, + }, + ], + connections: connected + ? { source: { main: [[{ node: 'consumer', type: 'main', index: 0 }]] } } + : {}, + active: false, + nodeTypes, + }); + } + + const runExecutionData = createRunExecutionData({ + resultData: { + runData: { + source: [ + { + startTime: 1, + executionTime: 1, + executionIndex: 0, + source: [], + data: { + main: [[{ json: { city: 'Prague' }, pairedItem: { item: 0 } }]], + }, + }, + ], + }, + }, + }); + + it("should resolve $('source').item.json.city", async () => { + const testWorkflow = createTestWorkflow(true); + + await testWorkflow.expression.acquireIsolate(); + try { + const result = testWorkflow.expression.getParameterValue( + "={{ $('source').item.json.city }}", + runExecutionData, + 0, + 0, + 'consumer', + [{ json: { city: 'Prague' }, pairedItem: { item: 0 } }], + 'manual', + {}, + { + node: testWorkflow.getNode('consumer')!, + data: {}, + source: { + main: [{ previousNode: 'source', previousNodeOutput: 0, previousNodeRun: 0 }], + }, + }, + ); + + expect(result).toBe('Prague'); + } finally { + await testWorkflow.expression.releaseIsolate(); + } + }); + + it('should throw ExpressionError when nodes are not connected', async () => { + const testWorkflow = createTestWorkflow(false); + + await testWorkflow.expression.acquireIsolate(); + try { + expect(() => + testWorkflow.expression.getParameterValue( + "={{ $('source').item.json.city }}", + runExecutionData, + 0, + 0, + 'consumer', + [{ json: {} }], + 'manual', + {}, + ), + ).toThrow(ExpressionError); + } finally { + await testWorkflow.expression.releaseIsolate(); + } + }); + }); + describe('getParameterValue with IWorkflowDataProxyData', () => { it('should evaluate simple expression with provided IWorkflowDataProxyData', async () => { const nodeTypes = Helpers.NodeTypes();