From e5208484aa2fabd7f0cb657b2331eb3d569b2189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 2 Apr 2026 11:22:14 +0200 Subject: [PATCH] fix(core): Fix missing isolate acquisition and VM globals in expression engine (#27895) --- .../src/bridge/isolated-vm-bridge.ts | 22 +- .../expression-runtime/src/runtime/index.ts | 9 + .../expression-runtime/src/runtime/reset.ts | 53 +++-- packages/cli/src/active-workflow-manager.ts | 58 +++--- .../webhooks/__tests__/test-webhooks.test.ts | 14 +- packages/cli/src/webhooks/test-webhooks.ts | 193 +++++++++--------- .../cli/src/workflows/workflow.service.ts | 7 +- 7 files changed, 214 insertions(+), 142 deletions(-) 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 7ba755baf18..6c55ef09898 100644 --- a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts +++ b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts @@ -362,6 +362,12 @@ export class IsolatedVmBridge implements RuntimeBridge { value = (itemFn as (i: number) => unknown)(itemIndex); startIndex = 2; } + } else { + const dollarFn = (data as Record).$; + if (path.length >= 2 && path[0] === '$' && typeof dollarFn === 'function') { + value = (dollarFn as (name: string) => unknown)(path[1]); + startIndex = 2; + } } for (let i = startIndex; i < path.length; i++) { value = (value as Record)?.[path[i]]; @@ -419,6 +425,12 @@ export class IsolatedVmBridge implements RuntimeBridge { arr = (itemFn as (i: number) => unknown)(itemIndex); startIndex = 2; } + } else { + const dollarFn = (data as Record).$; + if (path.length >= 2 && path[0] === '$' && typeof dollarFn === 'function') { + arr = (dollarFn as (name: string) => unknown)(path[1]); + startIndex = 2; + } } for (let i = startIndex; i < path.length; i++) { arr = (arr as Record)?.[path[i]]; @@ -462,9 +474,15 @@ export class IsolatedVmBridge implements RuntimeBridge { // Navigate to function, tracking parent to preserve `this` context let fn: unknown = data; let parent: unknown = undefined; - for (const key of path) { + 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)?.[key]; + fn = (fn as Record)?.[path[i]]; } if (typeof fn !== 'function') { diff --git a/packages/@n8n/expression-runtime/src/runtime/index.ts b/packages/@n8n/expression-runtime/src/runtime/index.ts index c38e12e9218..ef332cd5396 100644 --- a/packages/@n8n/expression-runtime/src/runtime/index.ts +++ b/packages/@n8n/expression-runtime/src/runtime/index.ts @@ -51,6 +51,15 @@ declare global { var $now: import('luxon').DateTime; var $today: import('luxon').DateTime; var $items: unknown; + var $execution: unknown; + var $vars: unknown; + var $secrets: unknown; + var $executionId: string | undefined; + var $resumeWebhookUrl: string | undefined; + var $webhookId: string | undefined; + var $nodeId: string | undefined; + var $nodeVersion: number | undefined; + var $: (nodeName: string) => unknown; } } diff --git a/packages/@n8n/expression-runtime/src/runtime/reset.ts b/packages/@n8n/expression-runtime/src/runtime/reset.ts index f793e5f274b..b107bc98640 100644 --- a/packages/@n8n/expression-runtime/src/runtime/reset.ts +++ b/packages/@n8n/expression-runtime/src/runtime/reset.ts @@ -18,6 +18,17 @@ const SafeURIError = createSafeErrorSubclass(URIError); // Reset Function for Data Proxies // ============================================================================ +function fetchPrimitive(key: string): unknown { + try { + return globalThis.__getValueAtPath.applySync(null, [[key]], { + arguments: { copy: true }, + result: { copy: true }, + }); + } catch { + return undefined; + } +} + /** * Reset workflow data proxies before each evaluation. * @@ -73,6 +84,9 @@ export function resetDataProxies(timezone?: string): void { globalThis.__data.$data = createDeepLazyProxy(['$data']); globalThis.__data.$env = createDeepLazyProxy(['$env']); globalThis.__data.process = createDeepLazyProxy(['process']); + globalThis.__data.$execution = createDeepLazyProxy(['$execution']); + globalThis.__data.$vars = createDeepLazyProxy(['$vars']); + globalThis.__data.$secrets = createDeepLazyProxy(['$secrets']); // ------------------------------------------------------------------------- // Create DateTime values inside the isolate (not lazy-loaded from host, @@ -88,25 +102,13 @@ export function resetDataProxies(timezone?: string): void { // Fetch primitives directly (no lazy loading needed for simple values) // ------------------------------------------------------------------------- - try { - globalThis.__data.$runIndex = globalThis.__getValueAtPath.applySync(null, [['$runIndex']], { - arguments: { copy: true }, - result: { copy: true }, - }); - } catch (error) { - // Property doesn't exist - set to undefined - globalThis.__data.$runIndex = undefined; - } - - try { - globalThis.__data.$itemIndex = globalThis.__getValueAtPath.applySync(null, [['$itemIndex']], { - arguments: { copy: true }, - result: { copy: true }, - }); - } catch (error) { - // Property doesn't exist - set to undefined - globalThis.__data.$itemIndex = undefined; - } + globalThis.__data.$runIndex = fetchPrimitive('$runIndex'); + globalThis.__data.$itemIndex = fetchPrimitive('$itemIndex'); + globalThis.__data.$executionId = fetchPrimitive('$executionId'); + globalThis.__data.$resumeWebhookUrl = fetchPrimitive('$resumeWebhookUrl'); + globalThis.__data.$webhookId = fetchPrimitive('$webhookId'); + globalThis.__data.$nodeId = fetchPrimitive('$nodeId'); + globalThis.__data.$nodeVersion = fetchPrimitive('$nodeVersion'); // ------------------------------------------------------------------------- // Expose workflow data to globalThis for expression access @@ -125,6 +127,14 @@ export function resetDataProxies(timezone?: string): void { globalThis.$env = globalThis.__data.$env; globalThis.$now = globalThis.__data.$now as DateTime; globalThis.$today = globalThis.__data.$today as DateTime; + globalThis.$execution = globalThis.__data.$execution; + globalThis.$vars = globalThis.__data.$vars; + globalThis.$secrets = globalThis.__data.$secrets; + globalThis.$executionId = globalThis.__data.$executionId as string | undefined; + globalThis.$resumeWebhookUrl = globalThis.__data.$resumeWebhookUrl as string | undefined; + globalThis.$webhookId = globalThis.__data.$webhookId as string | undefined; + globalThis.$nodeId = globalThis.__data.$nodeId as string | undefined; + globalThis.$nodeVersion = globalThis.__data.$nodeVersion as number | undefined; // Expose standalone functions (min, max, average, numberList, zip, $ifEmpty, etc.) Object.assign(globalThis.__data, extendedFunctions); @@ -189,6 +199,11 @@ export function resetDataProxies(timezone?: string): void { $binary: createDeepLazyProxy(['$item', indexStr, '$binary']), }; }; + + globalThis.$ = function (nodeName: string) { + return createDeepLazyProxy(['$', nodeName]); + }; + globalThis.__data.$ = globalThis.$; } // Matches initializeGlobalContext() lines 262-318 in packages/workflow/src/expression.ts diff --git a/packages/cli/src/active-workflow-manager.ts b/packages/cli/src/active-workflow-manager.ts index a7ba6a02c47..c60fde28919 100644 --- a/packages/cli/src/active-workflow-manager.ts +++ b/packages/cli/src/active-workflow-manager.ts @@ -273,10 +273,20 @@ export class ActiveWorkflowManager { workflowSettings: workflowData.settings, }); - const webhooks = WebhookHelpers.getWorkflowWebhooks(workflow, additionalData, undefined, true); + await workflow.expression.acquireIsolate(); + try { + const webhooks = WebhookHelpers.getWorkflowWebhooks( + workflow, + additionalData, + undefined, + true, + ); - for (const webhookData of webhooks) { - await this.webhookService.deleteWebhook(workflow, webhookData, mode, 'update'); + for (const webhookData of webhooks) { + await this.webhookService.deleteWebhook(workflow, webhookData, mode, 'update'); + } + } finally { + await workflow.expression.releaseIsolate(); } await this.workflowStaticDataService.saveStaticData(workflow); @@ -671,21 +681,29 @@ export class ActiveWorkflowManager { workflowSettings: dbWorkflow.settings, }); - if (shouldAddWebhooks) { - added.webhooks = await this.addWebhooks( - workflow, - additionalData, - 'trigger', - activationMode, - ); - } + let triggerCount = 0; + await workflow.expression.acquireIsolate(); + try { + if (shouldAddWebhooks) { + added.webhooks = await this.addWebhooks( + workflow, + additionalData, + 'trigger', + activationMode, + ); + } - if (shouldAddTriggersAndPollers) { - added.triggersAndPollers = await this.addTriggersAndPollers(dbWorkflow, workflow, { - activationMode, - executionMode: 'trigger', - additionalData, - }); + if (shouldAddTriggersAndPollers) { + added.triggersAndPollers = await this.addTriggersAndPollers(dbWorkflow, workflow, { + activationMode, + executionMode: 'trigger', + additionalData, + }); + } + + triggerCount = this.countTriggers(workflow, additionalData); + } finally { + await workflow.expression.releaseIsolate(); } // Workflow got now successfully activated so make sure nothing is left in the queue @@ -693,7 +711,6 @@ export class ActiveWorkflowManager { await this.activationErrorsService.deregister(workflowId); - const triggerCount = this.countTriggers(workflow, additionalData); await this.workflowRepository.updateWorkflowTriggerCount(workflow.id, triggerCount); } catch (e) { const error = e instanceof Error ? e : new Error(`${e}`); @@ -776,11 +793,6 @@ export class ActiveWorkflowManager { /** * Count all triggers in the workflow, excluding Manual Trigger and other n8n-internal triggers. - * - * TODO: This method calls getWorkflowWebhooks, which evaluates webhook description expressions - * (path, httpMethod, etc.) that may reference user-authored expressions via $parameter. It - * should acquire an isolate before calling getWorkflowWebhooks, but countTriggers is sync. - * addWebhooks and removeWorkflow are async and can be fixed straightforwardly. */ private countTriggers(workflow: Workflow, additionalData: IWorkflowExecuteAdditionalData) { const triggerFilter = (nodeType: INodeType) => diff --git a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts index 32c41afe1a8..1d99dadf98d 100644 --- a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts +++ b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts @@ -9,6 +9,7 @@ import type { IWorkflowExecuteAdditionalData, Workflow, IHttpRequestMethods, + WorkflowExpression, } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; @@ -71,7 +72,7 @@ describe('TestWebhooks', () => { }; test('if webhook is needed, should register then create webhook and return true', async () => { - const workflow = mock(); + const workflow = mock({ expression: mock() }); jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValueOnce(workflow); jest.spyOn(WebhookHelpers, 'getWorkflowWebhooks').mockReturnValue([webhook]); @@ -107,7 +108,7 @@ describe('TestWebhooks', () => { }); test('returns false if a triggerToStartFrom with triggerData is given', async () => { - const workflow = mock(); + const workflow = mock({ expression: mock() }); jest.spyOn(testWebhooks, 'toWorkflow').mockReturnValueOnce(workflow); jest.spyOn(WebhookHelpers, 'getWorkflowWebhooks').mockReturnValue([webhook]); @@ -124,7 +125,7 @@ describe('TestWebhooks', () => { test('returns true, registers and then creates webhook if triggerToStartFrom is given with no triggerData', async () => { // ARRANGE - const workflow = mock(); + const workflow = mock({ expression: mock() }); const webhook2 = mock({ node: 'trigger', httpMethod, @@ -161,6 +162,7 @@ describe('TestWebhooks', () => { name: 'chatTriggerNode', }, }, + expression: mock(), }); const chatSessionId = 'test-session-123'; const chatWebhook = mock({ @@ -200,6 +202,7 @@ describe('TestWebhooks', () => { name: 'chatTriggerNode', }, }, + expression: mock(), }); const chatWebhook = mock({ node: 'chatTriggerNode', @@ -230,6 +233,7 @@ describe('TestWebhooks', () => { name: 'webhookNode', }, }, + expression: mock(), }); const chatSessionId = 'test-session-123'; const regularWebhook = mock({ @@ -256,7 +260,7 @@ describe('TestWebhooks', () => { test('should handle destinationNode parameter correctly', async () => { // ARRANGE - const workflow = mock(); + const workflow = mock({ expression: mock() }); const destinationNodeObj = { nodeName: 'DestinationNode', mode: 'inclusive' as const }; webhook.webhookDescription = { restartWebhook: false, @@ -291,7 +295,7 @@ describe('TestWebhooks', () => { }>)( 'handles single webhook trigger when workflowIsActive=%s', async ({ published: workflowIsActive, withSingleWebhookTrigger, shouldThrow }) => { - const workflow = mock(); + const workflow = mock({ expression: mock() }); const regularWebhook = mock({ node: 'Webhook', httpMethod, diff --git a/packages/cli/src/webhooks/test-webhooks.ts b/packages/cli/src/webhooks/test-webhooks.ts index 38ad466e33a..9ecc38f4575 100644 --- a/packages/cli/src/webhooks/test-webhooks.ts +++ b/packages/cli/src/webhooks/test-webhooks.ts @@ -309,119 +309,128 @@ export class TestWebhooks implements IWebhookManager { const workflow = this.toWorkflow(workflowEntity); - let webhooks = WebhookHelpers.getWorkflowWebhooks( - workflow, - additionalData, - destinationNode, - true, - ); - - // If we have a preferred trigger with data, we don't have to listen for a - // webhook. - if (triggerToStartFrom?.data) { - return false; - } - - // If we have a preferred trigger without data we only want to listen for - // that trigger, not the other ones. - if (triggerToStartFrom) { - webhooks = webhooks.filter((w) => w.node === triggerToStartFrom.name); - } - - if (!webhooks.some((w) => w.webhookDescription.restartWebhook !== true)) { - return false; // no webhooks found to start a workflow - } - - const timeoutDuration = TEST_WEBHOOK_TIMEOUT; - - // Check if any webhook is a single webhook trigger and workflow is active - if (workflowIsActive) { - const singleWebhookTrigger = webhooks.find((w) => - SINGLE_WEBHOOK_TRIGGERS.includes(workflow.getNode(w.node)?.type ?? ''), + await workflow.expression.acquireIsolate(); + let webhooks: IWebhookData[]; + try { + webhooks = WebhookHelpers.getWorkflowWebhooks( + workflow, + additionalData, + destinationNode, + true, ); - if (singleWebhookTrigger) { - throw new SingleWebhookTriggerError( - workflow.getNode(singleWebhookTrigger.node)?.name ?? '', - ); - } - } - const timeout = setTimeout(async () => await this.cancelWebhook(workflow.id), timeoutDuration); - - for (const webhook of webhooks) { - webhook.path = removeTrailingSlash(webhook.path); - - // Use sessionId-based path for ChatTrigger nodes when sessionId is provided - // IMPORTANT: This must happen BEFORE key generation - if ( - chatSessionId && - webhook.node && - workflow.nodes[webhook.node]?.type === '@n8n/n8n-nodes-langchain.chatTrigger' - ) { - // Generate predictable path using workflowId and sessionId (without leading slash to match lookup format) - webhook.path = `${workflow.id}/${chatSessionId}`; - } - - const key = this.registrations.toKey(webhook); - const registrationByKey = await this.registrations.get(key); - - if (runData && webhook.node in runData) { + // If we have a preferred trigger with data, we don't have to listen for a + // webhook. + if (triggerToStartFrom?.data) { return false; } - // if registration already exists and is not a test webhook created by this user in this workflow throw an error - if ( - registrationByKey && - !webhook.webhookId && - !registrationByKey.webhook.isTest && - registrationByKey.webhook.userId !== userId && - registrationByKey.webhook.workflowId !== workflow.id - ) { - throw new WebhookPathTakenError(webhook.node); + // If we have a preferred trigger without data we only want to listen for + // that trigger, not the other ones. + if (triggerToStartFrom) { + webhooks = webhooks.filter((w) => w.node === triggerToStartFrom.name); } - webhook.isTest = true; + if (!webhooks.some((w) => w.webhookDescription.restartWebhook !== true)) { + return false; // no webhooks found to start a workflow + } - /** - * Additional data cannot be cached because of circular refs. - * Hence store the `userId` and recreate additional data when needed. - */ - const { workflowExecuteAdditionalData: _, ...cacheableWebhook } = webhook; + const timeoutDuration = TEST_WEBHOOK_TIMEOUT; - cacheableWebhook.userId = userId; + // Check if any webhook is a single webhook trigger and workflow is active + if (workflowIsActive) { + const singleWebhookTrigger = webhooks.find((w) => + SINGLE_WEBHOOK_TRIGGERS.includes(workflow.getNode(w.node)?.type ?? ''), + ); + if (singleWebhookTrigger) { + throw new SingleWebhookTriggerError( + workflow.getNode(singleWebhookTrigger.node)?.name ?? '', + ); + } + } - const registration: TestWebhookRegistration = { - version: 1, - pushRef, - workflowEntity, - destinationNode, - webhook: cacheableWebhook as IWebhookData, - }; + const timeout = setTimeout( + async () => await this.cancelWebhook(workflow.id), + timeoutDuration, + ); + + for (const webhook of webhooks) { + webhook.path = removeTrailingSlash(webhook.path); + + // Use sessionId-based path for ChatTrigger nodes when sessionId is provided + // IMPORTANT: This must happen BEFORE key generation + if ( + chatSessionId && + webhook.node && + workflow.nodes[webhook.node]?.type === '@n8n/n8n-nodes-langchain.chatTrigger' + ) { + // Generate predictable path using workflowId and sessionId (without leading slash to match lookup format) + webhook.path = `${workflow.id}/${chatSessionId}`; + } + + const key = this.registrations.toKey(webhook); + const registrationByKey = await this.registrations.get(key); + + if (runData && webhook.node in runData) { + return false; + } + + // if registration already exists and is not a test webhook created by this user in this workflow throw an error + if ( + registrationByKey && + !webhook.webhookId && + !registrationByKey.webhook.isTest && + registrationByKey.webhook.userId !== userId && + registrationByKey.webhook.workflowId !== workflow.id + ) { + throw new WebhookPathTakenError(webhook.node); + } + + webhook.isTest = true; - try { /** - * Register the test webhook _before_ creation at third-party service - * in case service sends a confirmation request immediately on creation. + * Additional data cannot be cached because of circular refs. + * Hence store the `userId` and recreate additional data when needed. */ - await this.registrations.register(registration); + const { workflowExecuteAdditionalData: _, ...cacheableWebhook } = webhook; - await this.webhookService.createWebhookIfNotExists(workflow, webhook, 'manual', 'manual'); + cacheableWebhook.userId = userId; - cacheableWebhook.staticData = workflow.staticData; + const registration: TestWebhookRegistration = { + version: 1, + pushRef, + workflowEntity, + destinationNode, + webhook: cacheableWebhook as IWebhookData, + }; - await this.registrations.register(registration); + try { + /** + * Register the test webhook _before_ creation at third-party service + * in case service sends a confirmation request immediately on creation. + */ + await this.registrations.register(registration); - this.timeouts[key] = timeout; - } catch (error) { - await this.deactivateWebhooks(workflow); + await this.webhookService.createWebhookIfNotExists(workflow, webhook, 'manual', 'manual'); - delete this.timeouts[key]; + cacheableWebhook.staticData = workflow.staticData; - throw error; + await this.registrations.register(registration); + + this.timeouts[key] = timeout; + } catch (error) { + await this.deactivateWebhooks(workflow); + + delete this.timeouts[key]; + + throw error; + } } - } - return true; + return true; + } finally { + await workflow.expression.releaseIsolate(); + } } async cancelWebhook(workflowId: string) { diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index a59901a101e..ffbcfb2a1d8 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -592,7 +592,12 @@ export class WorkflowService { workflowId: workflow.id, }); - return await this.webhookService.findWebhookConflicts(workflow, additionalData); + await workflow.expression.acquireIsolate(); + try { + return await this.webhookService.findWebhookConflicts(workflow, additionalData); + } finally { + await workflow.expression.releaseIsolate(); + } } private async _detectWebhookConflicts(