diff --git a/packages/@n8n/expression-runtime/docs/deep-lazy-proxy.md b/packages/@n8n/expression-runtime/docs/deep-lazy-proxy.md index 114c4e6a73b..23b4146a8b4 100644 --- a/packages/@n8n/expression-runtime/docs/deep-lazy-proxy.md +++ b/packages/@n8n/expression-runtime/docs/deep-lazy-proxy.md @@ -29,9 +29,12 @@ Key functions exposed on `globalThis` inside the isolate: Host-side callbacks registered by `IsolatedVmBridge` as `ivm.Reference` objects (synchronous cross-isolate calls): -- `__getValueAtPath(path[])` — returns a primitive, array metadata, or object metadata +- `__getValueAtPath(path[])` — returns a primitive, array metadata, or object metadata. + Function-typed values are returned as `undefined`; callable bindings route through `callHost`. - `__getArrayElement(path[], index)` — returns a single array element (or its metadata) -- `__callFunctionAtPath(path[], ...args)` — invokes a host-side function and returns the result +- `callHost(envelope)` — typed-RPC dispatcher; the isolate sends a schema-validated + envelope (e.g. `{ type: 'getItems', nodeName, ... }`) and the host dispatches + to the matching handler ## Usage @@ -50,7 +53,7 @@ From the expression's perspective it just sees normal objects: // Inside an expression (runs in isolate): $json.user.email // triggers getValueAtPath(['$json','user','email']) $json.items[150].id // triggers getArrayElement(['$json','items'], 150) -$items() // triggers callFunctionAtPath(['$items']) +$items() // triggers callHost({ type: 'getItems' }) ``` ### Array metadata diff --git a/packages/@n8n/expression-runtime/src/__tests__/host-fn-shadowing.test.ts b/packages/@n8n/expression-runtime/src/__tests__/host-fn-shadowing.test.ts index ce15d44c2a4..ea82fc0ab7a 100644 --- a/packages/@n8n/expression-runtime/src/__tests__/host-fn-shadowing.test.ts +++ b/packages/@n8n/expression-runtime/src/__tests__/host-fn-shadowing.test.ts @@ -7,12 +7,12 @@ * `target.` (set in `runtime/context.ts`) and returns the in-isolate * copy before any host lookup happens. * - * Why this matters under the threat model: every host function reachable - * from `data` becomes a callable target via the bridge's `callFunctionAtPath`. - * Confirming that helpers resolve in-isolate lets us safely strip them from - * the VM-path `data` object (see `expression.ts` shouldUseVm guard) without - * losing functionality. Keeping the helpers shadowed is a load-bearing - * structural invariant of the security model. + * Why this matters under the threat model: function-typed bindings on + * `data` are now structurally unreachable — `getValueAtPath` returns + * `undefined` for any function-typed value and there is no `__isFunction` + * wrapper path. Confirming that helpers resolve in-isolate also lets us + * safely strip them from the VM-path `data` object (see `expression.ts` + * `shouldUseVm` guard) without losing functionality. * * If this test ever fails, one of the following changed: * - Tournament's polyfill no longer rewrites bare identifiers to context-first diff --git a/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts b/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts index 02b400b0a08..2b0799bf3c7 100644 --- a/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts +++ b/packages/@n8n/expression-runtime/src/__tests__/typed-rpc.test.ts @@ -2,12 +2,12 @@ * Regression suite for the typed-RPC routing pattern. * * The pattern: `$('Foo').first()` is routed through the dedicated - * `getNodeFirst` typed RPC rather than the generic `callFunctionAtPath` - * channel. The in-isolate runtime exposes a synthetic proxy on - * `target.$(...)` that intercepts `.first` and sends a typed envelope via - * `callHost`; the bridge handler reads the literal string `"first"` - * from the host-side proxy. The property name is compile-time fixed and - * cannot be influenced by expression input. + * `getNodeFirst` typed RPC over the `callHost` dispatcher. The in-isolate + * runtime exposes a synthetic proxy on `target.$(...)` that intercepts + * `.first` and sends a typed envelope via `callHost`; the bridge handler + * reads the literal string `"first"` from the host-side proxy. The + * property name is compile-time fixed and cannot be influenced by + * expression input. * * Each typed RPC should land with a test in this file * confirming: 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 99e9ec087a8..25a663c76a8 100644 --- a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts +++ b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts @@ -280,7 +280,13 @@ export class IsolatedVmBridge implements RuntimeBridge { * Create an ivm.Reference callback for getting value/metadata at a path. * * Used by createDeepLazyProxy when accessing properties. Returns metadata - * markers for functions, arrays, and objects, or the primitive value directly. + * markers for arrays and objects, or the primitive value directly. + * + * Function-typed values are returned as `undefined` — every callable on + * the host data surface (`$('Foo').first()`, `$items()`, `$fromAI()`, + * `$evaluateExpression()`, `$getPairedItem()`) is wired in-isolate via + * the typed-RPC dispatcher (`callHost`). No expression form should + * reach a function through this path. * * @param data - Current workflow data to use for callback responses * @private @@ -314,14 +320,13 @@ export class IsolatedVmBridge implements RuntimeBridge { } } - // Handle functions - return metadata marker + // Functions are not reachable via the lazy-proxy data path — + // every callable on the host data surface routes through the + // typed-RPC dispatcher. Return undefined so any residual + // access surfaces as missing rather than as a stale metadata + // marker the runtime no longer knows how to interpret. if (typeof value === 'function') { - const fnString = value.toString(); - // Block native functions for security - if (fnString.includes('[native code]')) { - return undefined; - } - return { __isFunction: true, __name: path[path.length - 1] }; + return undefined; } // Handle arrays - always lazy, only transfer length @@ -414,48 +419,6 @@ export class IsolatedVmBridge implements RuntimeBridge { }); } - /** - * Create an ivm.Reference callback for calling functions at a path. - * - * Used when expressions invoke functions from workflow data. - * - * @param data - Current workflow data to use for callback responses - * @private - */ - private createCallFunctionAtPathRef(data: WorkflowData): ivm.Reference { - return new (getIvm().Reference)((path: string[], ...args: unknown[]) => { - try { - // Navigate to function, tracking parent to preserve `this` context - let fn: unknown = data; - let parent: unknown = undefined; - let startIndex = 0; - const dollarFn = (data as Record).$; - if (path.length >= 2 && path[0] === '$' && typeof dollarFn === 'function') { - fn = (dollarFn as (name: string) => unknown)(path[1]); - startIndex = 2; - } - for (let i = startIndex; i < path.length; i++) { - parent = fn; - fn = (fn as Record)?.[path[i]]; - } - - if (typeof fn !== 'function') { - throw new Error(`${path.join('.')} is not a function`); - } - - // Block native functions for security (same check as getValueAtPath) - if (fn.toString().includes('[native code]')) { - throw new Error(`${path.join('.')} is a native function and cannot be called`); - } - - // Execute function with parent as `this` to preserve method context - return (fn as (...fnArgs: unknown[]) => unknown).call(parent, ...args); - } catch (err) { - return serializeError(err); - } - }); - } - /** * Create the single typed-RPC dispatcher. * @@ -707,9 +670,9 @@ export class IsolatedVmBridge implements RuntimeBridge { * Execute JavaScript code in the isolated context. * * Flow: - * 1. Create four ivm.Reference callbacks scoped to the current data: - * `getValueAtPath`, `getArrayElement`, `callFunctionAtPath`, `callHost`. - * 2. Use evalClosureSync to run the code in a closure where `$0`/`$1`/`$2`/`$3` + * 1. Create three ivm.Reference callbacks scoped to the current data: + * `getValueAtPath`, `getArrayElement`, `callHost`. + * 2. Use evalClosureSync to run the code in a closure where `$0`/`$1`/`$2` * are the callback references — no global mutable state. * 3. buildContext() inside the isolate creates a fresh evaluation context * from the closure-scoped references. @@ -729,7 +692,6 @@ export class IsolatedVmBridge implements RuntimeBridge { const getValueAtPath = this.createGetValueAtPathRef(data); const getArrayElement = this.createGetArrayElementRef(data); - const callFunctionAtPath = this.createCallFunctionAtPathRef(data); const callHost = this.createCallHostRef(data); try { @@ -750,8 +712,7 @@ export class IsolatedVmBridge implements RuntimeBridge { var __ctx = buildContext({ getValueAtPath: $0, getArrayElement: $1, - callFunctionAtPath: $2, - callHost: $3, + callHost: $2, }, ${timezone}); try { var __result = (function() { @@ -776,7 +737,7 @@ try { const result = this.context.evalClosureSync( wrappedCode, - [getValueAtPath, getArrayElement, callFunctionAtPath, callHost], + [getValueAtPath, getArrayElement, callHost], { result: { copy: true }, timeout: this.config.timeout }, ); @@ -813,7 +774,6 @@ try { } finally { getValueAtPath.release(); getArrayElement.release(); - callFunctionAtPath.release(); callHost.release(); } } diff --git a/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts b/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts index 57475c735fb..465ba4f4dd5 100644 --- a/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts +++ b/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts @@ -24,7 +24,6 @@ describe('buildContext proxy', () => { const ctx = buildContext({ getValueAtPath, getArrayElement: makeRef(() => undefined), - callFunctionAtPath: makeRef(() => undefined), callHost: makeRef(() => undefined), }); @@ -37,7 +36,6 @@ describe('buildContext proxy', () => { const ctx = buildContext({ getValueAtPath, getArrayElement: makeRef(() => undefined), - callFunctionAtPath: makeRef(() => undefined), callHost: makeRef(() => undefined), }); @@ -54,7 +52,6 @@ describe('buildContext proxy', () => { const ctx = buildContext({ getValueAtPath, getArrayElement: makeRef(() => undefined), - callFunctionAtPath: makeRef(() => undefined), callHost: makeRef(() => undefined), }); diff --git a/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts b/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts index 7cb3481117c..82573d917ab 100644 --- a/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts +++ b/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts @@ -15,21 +15,18 @@ function mockApplySync(returnValue: unknown = undefined) { function createMockCallbacks( overrides: { getValueAtPath?: ReturnType; - callFunctionAtPath?: ReturnType; getArrayElement?: ReturnType; } = {}, ) { const getValueAtPath = overrides.getValueAtPath ?? mockApplySync(); - const callFunctionAtPath = overrides.callFunctionAtPath ?? mockApplySync(); const getArrayElement = overrides.getArrayElement ?? mockApplySync(); const callbacks = { getValueAtPath: { applySync: getValueAtPath }, - callFunctionAtPath: { applySync: callFunctionAtPath }, getArrayElement: { applySync: getArrayElement }, }; - return { getValueAtPath, callFunctionAtPath, getArrayElement, callbacks }; + return { getValueAtPath, getArrayElement, callbacks }; } // --------------------------------------------------------------------------- @@ -161,36 +158,7 @@ describe('createDeepLazyProxy', () => { }); // ----------------------------------------------------------------------- - // 4. Function metadata - // ----------------------------------------------------------------------- - - describe('function metadata', () => { - it('creates a callable wrapper for function metadata', () => { - mocks.getValueAtPath.mockReturnValue({ __isFunction: true, __name: 'myFn' }); - const p = proxy(); - expect(typeof p.myFn).toBe('function'); - }); - - it('invokes __callFunctionAtPath with correct args when called', () => { - mocks.getValueAtPath.mockReturnValue({ __isFunction: true, __name: 'myFn' }); - mocks.callFunctionAtPath.mockReturnValue('result'); - const p = proxy(); - p.myFn('a', 1); - expect(mocks.callFunctionAtPath).toHaveBeenCalledWith(null, [['myFn'], 'a', 1], ivmCallOpts); - }); - - it('caches the function wrapper', () => { - mocks.getValueAtPath.mockReturnValue({ __isFunction: true, __name: 'myFn' }); - const p = proxy(); - const first = p.myFn; - const second = p.myFn; - expect(first).toBe(second); - expect(mocks.getValueAtPath).toHaveBeenCalledTimes(1); - }); - }); - - // ----------------------------------------------------------------------- - // 5. Array metadata (always lazy-loaded via array proxy) + // 4. Array metadata (always lazy-loaded via array proxy) // ----------------------------------------------------------------------- describe('array metadata', () => { @@ -428,16 +396,6 @@ describe('createDeepLazyProxy', () => { // ----------------------------------------------------------------------- describe('edge cases', () => { - it('plain object with __isFunction=false is treated as primitive', () => { - mocks.getValueAtPath.mockReturnValue({ __isFunction: false, other: 1 }); - const p = proxy(); - const val = p.prop; - // Not a function — falls through to "primitive" caching - expect(typeof val).toBe('object'); - expect(val.__isFunction).toBe(false); - expect(val.other).toBe(1); - }); - it('plain object with __isArray=false is treated as primitive', () => { mocks.getValueAtPath.mockReturnValue({ __isArray: false, data: 'x' }); const p = proxy(); @@ -452,14 +410,6 @@ describe('createDeepLazyProxy', () => { p.arr[-1]; expect(mocks.getArrayElement).not.toHaveBeenCalled(); }); - - it('function wrapper on a basePath proxy passes full path', () => { - mocks.getValueAtPath.mockReturnValue({ __isFunction: true, __name: '$items' }); - mocks.callFunctionAtPath.mockReturnValue([]); - const p = proxy(['$root']); - p.fn(); - expect(mocks.callFunctionAtPath).toHaveBeenCalledWith(null, [['$root', 'fn']], ivmCallOpts); - }); }); // ----------------------------------------------------------------------- diff --git a/packages/@n8n/expression-runtime/src/runtime/context.ts b/packages/@n8n/expression-runtime/src/runtime/context.ts index abc15b0530d..e33ae0acacb 100644 --- a/packages/@n8n/expression-runtime/src/runtime/context.ts +++ b/packages/@n8n/expression-runtime/src/runtime/context.ts @@ -72,8 +72,6 @@ interface BridgeCallback { * * - `getValueAtPath`, `getArrayElement`: data-access primitives used by the * lazy-proxy system. Hot path; one ivm.Reference each for minimum overhead. - * - `callFunctionAtPath`: legacy generic dispatch; to be removed once - * every consumer has migrated to typed messages. * - `callHost`: typed-RPC dispatcher. The in-isolate runtime constructs * an envelope (e.g. `{ type: 'getNodeFirst', nodeName, ... }`) and the * host-side dispatcher validates it with zod before routing to a handler. @@ -82,14 +80,13 @@ interface BridgeCallback { * dispatcher switch. The name reflects what this is: a synchronous * host RPC, not a postMessage-style async send. * - * The bridge wires all four callbacks unconditionally before invoking + * The bridge wires all three callbacks unconditionally before invoking * `buildContext`, so the runtime treats them as present — no defensive * null/undefined checks at each call site. */ export interface BridgeCallbacks { getValueAtPath: BridgeCallback; getArrayElement: BridgeCallback; - callFunctionAtPath: BridgeCallback; callHost: BridgeCallback; } @@ -405,19 +402,6 @@ export function buildContext( throwIfErrorSentinel(value); - // Function metadata — create a callable wrapper - if (value && typeof value === 'object' && (value as any).__isFunction) { - target[key] = function (...args: unknown[]) { - const result = callbacks.callFunctionAtPath.applySync(null, [[key], ...args], { - arguments: { copy: true }, - result: { copy: true }, - }); - throwIfErrorSentinel(result); - return result; - }; - return true; - } - // Object / array metadata — create a shape-matched lazy proxy for deep access if (isArrayMetadata(value)) { target[key] = createDeepLazyProxy( diff --git a/packages/@n8n/expression-runtime/src/runtime/jmespath.ts b/packages/@n8n/expression-runtime/src/runtime/jmespath.ts index 83a0f64b499..0c726f4206b 100644 --- a/packages/@n8n/expression-runtime/src/runtime/jmespath.ts +++ b/packages/@n8n/expression-runtime/src/runtime/jmespath.ts @@ -38,9 +38,11 @@ const unsafeJmespathPropertyPattern = new RegExp( * * Mirrors the host-side wrapper in `packages/workflow/src/workflow-data-proxy.ts` * so that expressions resolve `$jmespath` from the in-isolate runtime via - * Tournament's polyfill, instead of falling through to the bridge's - * `callFunctionAtPath` channel. This shrinks the host-callable surface - * exposed by the data object. + * Tournament's polyfill. `$jmespath` is a pure utility — it takes its + * data as an argument and runs a query on it, with no need for host + * context. Keeping it in-isolate means it does not appear as a + * host-callable on the bridge, which shrinks the host-callable surface + * the isolate can reach. * * Behavioural parity with the host wrapper: * - Throws `ExpressionError` (same name) when args are wrong. diff --git a/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts b/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts index d25b4896030..1f5d9ab5bca 100644 --- a/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts +++ b/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts @@ -96,10 +96,13 @@ export type ProxyMeta = { kind: 'object'; keys?: string[] } | { kind: 'array'; l * * Pattern: * 1. When property accessed: Call getValueAtPath([path]) to get metadata - * 2. Metadata indicates type: primitive, object, array, or function + * 2. Metadata indicates type: primitive, object, or array * 3. For objects/arrays: Create nested proxy (shape-matched) for lazy loading - * 4. For functions: Create wrapper that calls callFunctionAtPath - * 5. Cache all fetched values in target to avoid repeated callbacks + * 4. Cache all fetched values in target to avoid repeated callbacks + * + * Callable host bindings (`$('Foo').first()`, `$items()`, etc.) do not flow + * through this proxy — they are wired as in-isolate stubs on `target` that + * route through the typed-RPC dispatcher (`callHost`). * * @param basePath - Current path in object tree (e.g., ['$json', 'user']) * @param meta - Optional shape descriptor (object/array + known keys/length) @@ -112,13 +115,12 @@ export function createDeepLazyProxy( callbacks?: { getValueAtPath: any; getArrayElement: any; - callFunctionAtPath: any; }, ): Record { if (!callbacks) { throw new Error('createDeepLazyProxy requires callbacks parameter'); } - const { getValueAtPath, getArrayElement, callFunctionAtPath } = callbacks; + const { getValueAtPath, getArrayElement } = callbacks; const isArray = meta?.kind === 'array'; const arrayLength = isArray ? meta.length : 0; @@ -153,19 +155,8 @@ export function createDeepLazyProxy( function materializeChild(basePath: string[], propOrIdx: string, value: unknown): unknown { if (value === undefined || value === null) return value; throwIfErrorSentinel(value); - // `path = [...basePath, propOrIdx]` is built only inside the three branches + // `path = [...basePath, propOrIdx]` is built only inside the two branches // that actually use it, so non-metadata objects don't pay for the spread. - if (value && typeof value === 'object' && (value as { __isFunction?: unknown }).__isFunction) { - const path = [...basePath, propOrIdx]; - return function (...args: any[]) { - const result = callFunctionAtPath.applySync(null, [path, ...args], { - arguments: { copy: true }, - result: { copy: true }, - }); - throwIfErrorSentinel(result); - return result; - }; - } if (isArrayMetadata(value)) { const path = [...basePath, propOrIdx]; return createDeepLazyProxy(path, { kind: 'array', length: value.__length }, callbacks); diff --git a/packages/workflow/src/expression.ts b/packages/workflow/src/expression.ts index 57e823b9eb3..6782c2e5bb8 100644 --- a/packages/workflow/src/expression.ts +++ b/packages/workflow/src/expression.ts @@ -551,14 +551,14 @@ export class Expression { // Expression extensions — only attached for the legacy engine. // - // In the VM engine, every host function reachable from `data` becomes - // a callable target the isolate can reach via `callFunctionAtPath`. - // To minimise that surface we keep the VM-path data object as small as - // possible and let the in-isolate runtime resolve helpers itself - // (see packages/@n8n/expression-runtime/src/runtime/context.ts, where - // Tournament's polyfill rewrites bare `extend(...)` calls to the - // in-isolate copy on `target.extend`). Setting them on `data` in VM - // mode would be dead code AND an unnecessary host-callable. + // In the VM engine, function-typed bindings on `data` are + // structurally unreachable: the bridge's `getValueAtPath` returns + // `undefined` for any function-typed value, and the in-isolate + // runtime resolves helpers itself via Tournament's polyfill + // (see packages/@n8n/expression-runtime/src/runtime/context.ts, + // where bare `extend(...)` calls bind to the in-isolate copy on + // `target.extend`). Setting them on `data` in VM mode would be + // dead code. if (!usingVm) { data.extend = extend; data.extendOptional = extendOptional; diff --git a/packages/workflow/test/ExpressionExtensions/expression-extension.test.ts b/packages/workflow/test/ExpressionExtensions/expression-extension.test.ts index 96a5c87a815..e44e8df256c 100644 --- a/packages/workflow/test/ExpressionExtensions/expression-extension.test.ts +++ b/packages/workflow/test/ExpressionExtensions/expression-extension.test.ts @@ -234,16 +234,18 @@ describe('Expression Parser', () => { expect(evaluate('={{ $if("a"==="b", 1, 2) }}')).toEqual(2); expect(evaluate('={{ $if("a"==="a", 1) }}')).toEqual(1); expect(evaluate('={{ $if("a"==="b", 1) }}')).toEqual(false); + expect(evaluate('={{ $if("a"==="a", 1, 2) }}')).toEqual(1); + expect(evaluate('={{ $if("a"==="b", 1, 2) }}')).toEqual(2); - // This will likely break when sandboxing is implemented but it works for now. - // If you're implementing sandboxing maybe provide a way to add functions to - // sandbox we can check instead? - const mockCallback = vi.fn(() => false); - evaluate('={{ $if("a"==="a", true, $data.cb()) }}', [{ cb: mockCallback }]); - expect(mockCallback.mock.calls.length).toEqual(0); - - evaluate('={{ $if("a"==="b", true, $data.cb()) }}', [{ cb: mockCallback }]); - expect(mockCallback.mock.calls.length).toEqual(1); + // Short-circuit: $if is AST-rewritten to a JS ternary, so the + // unselected branch is not evaluated. Probe with a throwing IIFE + // in the else position — the throw is swallowed by the + // evaluator's E() handler, but a non-undefined result for the + // true case proves the IIFE didn't run, and the undefined + // result for the false case proves it did. + const throwingExpr = "(function(){ throw new Error('side effect'); })()"; + expect(evaluate(`={{ $if(true, 1, ${throwingExpr}) }}`)).toEqual(1); + expect(evaluate(`={{ $if(false, 1, ${throwingExpr}) }}`)).toBeUndefined(); }); test('$not', () => {