From 18c2d715e36f26cc8dcae0c409b3d075bb07b433 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mike=20Repe=C4=87?= Date: Thu, 6 Aug 2026 13:44:14 +0200 Subject: [PATCH] perf(core): Resolve webhook description fields via declared native resolvers (#35404) Co-authored-by: Claude Fable 5 --- .../src/configs/expression-engine.config.ts | 8 + packages/@n8n/config/test/config.test.ts | 1 + .../webhooks/__tests__/live-webhooks.test.ts | 269 +++++++++++++++- .../webhook-execution-context.test.ts | 107 +++++++ .../__tests__/webhook.service.test.ts | 46 ++- packages/cli/src/webhooks/live-webhooks.ts | 48 ++- .../src/webhooks/webhook-execution-context.ts | 29 +- packages/cli/src/webhooks/webhook-helpers.ts | 72 ++--- packages/cli/src/webhooks/webhook.service.ts | 66 ++-- .../utils/webhook-helper-functions.ts | 40 ++- .../nodes-base/nodes/Webhook/description.ts | 24 +- .../nodes/Webhook/test/description.test.ts | 119 +++++++ .../testing/playwright/services/api-helper.ts | 3 + .../playwright/services/metrics-api-helper.ts | 21 ++ .../e2e/api/webhook-isolate-skip.spec.ts | 184 +++++++++++ .../webhook-isolate-skip-expression.json | 57 ++++ .../webhook-isolate-skip-static.json | 45 +++ packages/workflow/src/index.ts | 10 + packages/workflow/src/interfaces.ts | 23 ++ .../src/webhook-description-fields.ts | 153 +++++++++ .../test/webhook-description-fields.test.ts | 303 ++++++++++++++++++ 21 files changed, 1521 insertions(+), 107 deletions(-) create mode 100644 packages/cli/src/webhooks/__tests__/webhook-execution-context.test.ts create mode 100644 packages/nodes-base/nodes/Webhook/test/description.test.ts create mode 100644 packages/testing/playwright/services/metrics-api-helper.ts create mode 100644 packages/testing/playwright/tests/e2e/api/webhook-isolate-skip.spec.ts create mode 100644 packages/testing/playwright/workflows/webhook-isolate-skip-expression.json create mode 100644 packages/testing/playwright/workflows/webhook-isolate-skip-static.json create mode 100644 packages/workflow/src/webhook-description-fields.ts create mode 100644 packages/workflow/test/webhook-description-fields.test.ts diff --git a/packages/@n8n/config/src/configs/expression-engine.config.ts b/packages/@n8n/config/src/configs/expression-engine.config.ts index 6a92b5b6eff..4e893660d64 100644 --- a/packages/@n8n/config/src/configs/expression-engine.config.ts +++ b/packages/@n8n/config/src/configs/expression-engine.config.ts @@ -61,4 +61,12 @@ export class ExpressionEngineConfig { /** If set, scale the pool to 0 warm isolates after this many seconds with no acquire. */ @Env('N8N_EXPRESSION_ENGINE_IDLE_TIMEOUT') idleTimeout?: number; + + /** + * Whether a production webhook request may skip acquiring an isolate when its + * trigger provably evaluates no expression during the webhook phase. Off + * acquires one for every request. + */ + @Env('N8N_EXPRESSION_ENGINE_ALLOW_WEBHOOK_ISOLATE_SKIP') + allowWebhookIsolateSkip: boolean = true; } diff --git a/packages/@n8n/config/test/config.test.ts b/packages/@n8n/config/test/config.test.ts index 0fa937bc3a6..c90f26be830 100644 --- a/packages/@n8n/config/test/config.test.ts +++ b/packages/@n8n/config/test/config.test.ts @@ -636,6 +636,7 @@ describe('GlobalConfig', () => { tracesEnabled: true, slowEvaluationThresholdMs: 50, tracesSampleRate: 0.0, + allowWebhookIsolateSkip: true, }, instanceSettingsLoader: { ownerManagedByEnv: false, diff --git a/packages/cli/src/webhooks/__tests__/live-webhooks.test.ts b/packages/cli/src/webhooks/__tests__/live-webhooks.test.ts index dad972c924e..6e009bc73fb 100644 --- a/packages/cli/src/webhooks/__tests__/live-webhooks.test.ts +++ b/packages/cli/src/webhooks/__tests__/live-webhooks.test.ts @@ -1,18 +1,28 @@ import { mockLogger } from '@n8n/backend-test-utils'; -import type { WorkflowsConfig } from '@n8n/config'; +import type { ExpressionEngineConfig, WorkflowsConfig } from '@n8n/config'; import type { WebhookEntity, WorkflowEntity, WorkflowHistory, WorkflowRepository } from '@n8n/db'; import type { Response } from 'express'; import type { IConnections, IHttpRequestMethods, INode, + INodeParameters, + INodeProperties, INodeType, IWebhookData, + IWebhookDescription, IWorkflowBase, IWorkflowExecuteAdditionalData, Workflow, } from 'n8n-workflow'; -import type { Mock } from 'vitest'; +import { + fromFunction, + fromParameter, + WEBHOOK_NODE_TYPE, + webhookDescriptionFields, + WorkflowExpression, +} from 'n8n-workflow'; +import type { Mock, MockInstance } from 'vitest'; import { mock } from 'vitest-mock-extended'; import { WebhookNotFoundError } from '@/errors/response-errors/webhook-not-found.error'; @@ -39,11 +49,16 @@ describe('LiveWebhooks', () => { const workflowStaticDataService = mock(); const workflowsConfig = mock({ useWorkflowPublicationService: false }); const workflowPublishedDataService = mock(); + const expressionEngineConfig = mock({ + allowWebhookIsolateSkip: true, + }); let liveWebhooks: LiveWebhooks; beforeEach(() => { vi.clearAllMocks(); + // `clearAllMocks` resets call history, not properties set on a mock + expressionEngineConfig.allowWebhookIsolateSkip = true; liveWebhooks = new LiveWebhooks( mockLogger(), nodeTypes, @@ -52,6 +67,7 @@ describe('LiveWebhooks', () => { workflowStaticDataService, workflowsConfig, workflowPublishedDataService, + expressionEngineConfig, ); // Mock WorkflowExecuteAdditionalData.getBase to avoid DI issues @@ -603,4 +619,253 @@ describe('LiveWebhooks', () => { ).resolves.toBeDefined(); }); }); + + describe('webhook-phase isolate acquisition', () => { + const nodeName = 'Trigger'; + const webhookPath = 'my-path'; + const httpMethod: IHttpRequestMethods = 'GET'; + + const nodeProperties: INodeProperties[] = [ + { displayName: 'Path', name: 'path', type: 'string', default: '' }, + { displayName: 'Method', name: 'httpMethod', type: 'string', default: 'GET' }, + { displayName: 'Code', name: 'responseCode', type: 'number', default: 200 }, + { displayName: 'Auth', name: 'authentication', type: 'string', default: 'none' }, + { + displayName: 'Options', + name: 'options', + type: 'collection', + default: {}, + options: [{ displayName: 'Only Run If', name: 'onlyRunIf', type: 'string', default: '' }], + }, + ]; + + // Mirrors the real Webhook node's description: every templated field is + // declared once, so it carries a native resolver. + const nativelyResolvableWebhooks: IWebhookDescription[] = [ + { + name: 'default', + isFullPath: true, + ...webhookDescriptionFields({ + path: fromParameter('path'), + httpMethod: fromParameter('httpMethod', 'GET'), + responseCode: fromFunction((p: INodeParameters) => p.responseCode as number), + }), + }, + ]; + + const setupMocks = ({ + nodeType = WEBHOOK_NODE_TYPE, + typeVersion = 2.1, + parameters = { path: webhookPath, httpMethod }, + webhooks = nativelyResolvableWebhooks, + credentials, + }: { + nodeType?: string; + typeVersion?: number; + parameters?: INodeParameters; + webhooks?: IWebhookDescription[]; + credentials?: INode['credentials']; + } = {}) => { + const node: INode = { + id: 'trigger-node', + name: nodeName, + type: nodeType, + typeVersion, + position: [0, 0], + parameters, + credentials, + }; + + const activeVersion = mock({ + versionId: 'v1', + workflowId: WORKFLOW_ID, + nodes: [node], + connections: {}, + }); + + workflowRepository.findOne.mockResolvedValue( + mock({ + id: WORKFLOW_ID, + active: true, + activeVersionId: 'v1', + nodes: [node], + connections: {}, + staticData: {}, + activeVersion, + shared: [{ role: 'workflow:owner', project: { id: 'project-1', projectRelations: [] } }], + }), + ); + + webhookService.findWebhook.mockResolvedValue( + mock({ + workflowId: WORKFLOW_ID, + node: nodeName, + webhookPath, + method: httpMethod, + isDynamic: false, + }), + ); + webhookService.getWebhookMethods.mockResolvedValue([httpMethod]); + nodeTypes.getByNameAndVersion.mockReturnValue({ + description: { + displayName: 'Trigger', + name: nodeType, + group: ['trigger'], + version: 2.1, + description: '', + defaults: { name: 'Trigger' }, + inputs: [], + outputs: ['main'], + properties: nodeProperties, + webhooks, + }, + webhook: vi.fn(), + }); + webhookService.getNodeWebhooks.mockReturnValue([ + mock({ + httpMethod, + path: webhookPath, + node: nodeName, + webhookDescription: { nodeType: undefined } as never, + workflowId: WORKFLOW_ID, + }), + ]); + + // eslint-disable-next-line @typescript-eslint/require-await + (WebhookHelpers.executeWebhook as Mock).mockImplementation(async (...args: unknown[]) => { + const webhookCallback = args[args.length - 1] as ( + error: Error | null, + data: object, + ) => void; + void webhookCallback(null, {}); + }); + + return mock({ method: httpMethod, params: { path: webhookPath } }); + }; + + let acquireIsolate: MockInstance; + let releaseIsolate: MockInstance; + + beforeEach(() => { + acquireIsolate = vi + .spyOn(WorkflowExpression.prototype, 'acquireIsolate') + .mockResolvedValue(true); + releaseIsolate = vi + .spyOn(WorkflowExpression.prototype, 'releaseIsolate') + .mockResolvedValue(undefined); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('skips acquisition for a Webhook node with no expression parameters', async () => { + const request = setupMocks(); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).not.toHaveBeenCalled(); + // Still released: a no-op here, but correct if the phase acquired one. + expect(releaseIsolate).toHaveBeenCalled(); + }); + + it('acquires when a node parameter holds an expression', async () => { + const request = setupMocks({ + parameters: { + path: webhookPath, + httpMethod, + options: { onlyRunIf: '={{ $json.body.id === 1 }}' }, + }, + }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + + it('acquires for typeVersion 1, whose body parsing evaluates a template', async () => { + const request = setupMocks({ typeVersion: 1 }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + + it('still skips for the n8nOAuth2 authentication mode', async () => { + const request = setupMocks({ + parameters: { path: webhookPath, httpMethod, authentication: 'n8nOAuth2' }, + }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).not.toHaveBeenCalled(); + }); + + it('acquires for a trigger type that is not on the allowlist', async () => { + const request = setupMocks({ nodeType: 'n8n-nodes-base.formTrigger' }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + + it('acquires when a webhook description has a template it cannot resolve natively', async () => { + const request = setupMocks({ + webhooks: [ + { + name: 'default', + httpMethod: 'POST', + path: '={{$parameter["path"]}}', + responseCode: '={{(function (p) { return p.responseCode; })($parameter)}}', + }, + ], + }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + + it('acquires when a webhook description nests a template it cannot resolve natively', async () => { + const request = setupMocks({ + webhooks: [ + { + name: 'default', + httpMethod: 'POST', + path: '={{$parameter["path"]}}', + responseHeaders: { entries: [{ name: 'x-a', value: '={{ $json.body.a }}' }] }, + } as unknown as IWebhookDescription, + ], + }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + + it('acquires when the node type declares no webhooks', async () => { + const request = setupMocks({ webhooks: [] }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + + it('still skips for a node that uses credentials', async () => { + const request = setupMocks({ credentials: { httpHeaderAuth: { id: '1', name: 'auth' } } }); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).not.toHaveBeenCalled(); + }); + + it('acquires when the kill switch is off', async () => { + expressionEngineConfig.allowWebhookIsolateSkip = false; + const request = setupMocks(); + + await liveWebhooks.executeWebhook(request, mock()); + + expect(acquireIsolate).toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/webhooks/__tests__/webhook-execution-context.test.ts b/packages/cli/src/webhooks/__tests__/webhook-execution-context.test.ts new file mode 100644 index 00000000000..8e4145341a5 --- /dev/null +++ b/packages/cli/src/webhooks/__tests__/webhook-execution-context.test.ts @@ -0,0 +1,107 @@ +import type { + INode, + INodeParameters, + IWebhookData, + IWorkflowDataProxyAdditionalKeys, + Workflow, +} from 'n8n-workflow'; +import { fromFunction, fromParameter, webhookDescriptionFields } from 'n8n-workflow'; +import { mock } from 'vitest-mock-extended'; + +import { WebhookExecutionContext } from '../webhook-execution-context'; + +// Every description-field read goes through this class, so the native-resolver +// short-circuit has to hold on each one: a field that reaches the engine 500s +// on a request that skipped acquiring an isolate. +describe('WebhookExecutionContext', () => { + const workflow = mock({ + expression: mock(), + }); + + const webhookDescription = { + name: 'default' as const, + httpMethod: 'POST', + ...webhookDescriptionFields({ + path: fromParameter('path'), + responseData: fromFunction((p: INodeParameters) => + p.responseMode === 'lastNode' ? 'noData' : undefined, + ), + }), + }; + + const buildContext = (node: INode) => + new WebhookExecutionContext( + workflow, + node, + // A plain object, not `mock`: a deep mock auto-creates a + // resolver for every field, defeating the point of these tests. + { webhookDescription } as unknown as IWebhookData, + 'trigger', + mock(), + ); + + beforeEach(() => vi.clearAllMocks()); + + describe('with static node parameters', () => { + const context = buildContext( + mock({ name: 'Webhook', parameters: { path: 'my-path', responseMode: 'lastNode' } }), + ); + + it('resolves a simple value natively without touching the engine', () => { + expect(context.evaluateSimpleWebhookDescriptionExpression('path')).toBe('my-path'); + expect(workflow.expression.getSimpleParameterValue).not.toHaveBeenCalled(); + }); + + it('resolves a complex value natively without touching the engine', () => { + expect(context.evaluateComplexWebhookDescriptionExpression('responseData')).toBe('noData'); + expect(workflow.expression.getComplexParameterValue).not.toHaveBeenCalled(); + }); + + it('returns a natively resolved undefined as-is, not the default value', () => { + const undefinedContext = buildContext( + mock({ name: 'Webhook', parameters: { responseMode: 'onReceived' } }), + ); + + expect( + undefinedContext.evaluateComplexWebhookDescriptionExpression( + 'responseData', + undefined, + 'firstEntryJson', + ), + ).toBeUndefined(); + expect(workflow.expression.getComplexParameterValue).not.toHaveBeenCalled(); + }); + + it('falls back to the engine for a field without a resolver', () => { + context.evaluateSimpleWebhookDescriptionExpression('httpMethod'); + + expect(workflow.expression.getSimpleParameterValue).toHaveBeenCalledWith( + expect.anything(), + 'POST', + 'trigger', + expect.anything(), + undefined, + undefined, + ); + }); + }); + + describe('with an expression in the node parameters', () => { + const context = buildContext( + mock({ name: 'Webhook', parameters: { path: '={{ $json.path }}' } }), + ); + + it('falls back to the engine even for a field with a resolver', () => { + context.evaluateSimpleWebhookDescriptionExpression('path'); + + expect(workflow.expression.getSimpleParameterValue).toHaveBeenCalledWith( + expect.anything(), + '={{$parameter["path"]}}', + 'trigger', + expect.anything(), + undefined, + undefined, + ); + }); + }); +}); diff --git a/packages/cli/src/webhooks/__tests__/webhook.service.test.ts b/packages/cli/src/webhooks/__tests__/webhook.service.test.ts index 760c344c67d..62fa97113f3 100644 --- a/packages/cli/src/webhooks/__tests__/webhook.service.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook.service.test.ts @@ -7,7 +7,12 @@ import type { IWebhookData, IWorkflowExecuteAdditionalData, } from 'n8n-workflow'; -import { Workflow, WebhookPathTakenError } from 'n8n-workflow'; +import { + Workflow, + WebhookPathTakenError, + webhookDescriptionFields, + fromParameter, +} from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; import { mock } from 'vitest-mock-extended'; @@ -590,6 +595,45 @@ describe('WebhookService', () => { expect(webhooks[0].path).not.toMatch(/\s/); expect(webhooks[0].path).toMatch(/\/path$/); }); + + test('should resolve declared fields natively, without the expression engine', async () => { + const node = { + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + disabled: false, + parameters: { path: 'native-path', httpMethod: 'POST' }, + } as unknown as INode; + + const fields = webhookDescriptionFields({ + httpMethod: fromParameter('httpMethod', 'GET'), + path: fromParameter('path'), + }); + const nodeType = { + description: { + webhooks: [ + { + name: 'default', + ...fields, + isFullPath: false, + restartWebhook: false, + }, + ], + }, + } as INodeType; + + nodeTypes.getByNameAndVersion.mockReturnValue(nodeType); + const engineSpy = vi.spyOn(workflow.expression, 'getSimpleParameterValue'); + + const webhooks = webhookService.getNodeWebhooks(workflow, node, additionalData); + + expect(webhooks).toHaveLength(1); + expect(webhooks[0]).toMatchObject({ httpMethod: 'POST' }); + expect(webhooks[0].path).toMatch(/\/native-path$/); + // fields with declared resolvers must never engage the expression engine + const engineEvaluatedValues = engineSpy.mock.calls.map((call) => call[1]); + expect(engineEvaluatedValues).not.toContain(fields.path); + expect(engineEvaluatedValues).not.toContain(fields.httpMethod); + }); }); describe('createWebhookIfNotExists()', () => { diff --git a/packages/cli/src/webhooks/live-webhooks.ts b/packages/cli/src/webhooks/live-webhooks.ts index 4445bdc38e5..3191b1ef1cf 100644 --- a/packages/cli/src/webhooks/live-webhooks.ts +++ b/packages/cli/src/webhooks/live-webhooks.ts @@ -1,9 +1,15 @@ import { Logger } from '@n8n/backend-common'; -import { WorkflowsConfig } from '@n8n/config'; +import { ExpressionEngineConfig, WorkflowsConfig } from '@n8n/config'; import { WorkflowRepository, type WorkflowEntity, type WorkflowHistory } from '@n8n/db'; import { Service } from '@n8n/di'; import type { Response } from 'express'; -import { Workflow, CHAT_TRIGGER_NODE_TYPE } from 'n8n-workflow'; +import { + Workflow, + CHAT_TRIGGER_NODE_TYPE, + WEBHOOK_NODE_TYPE, + nodeParametersAreStatic, + webhookDescriptionIsNativelyResolvable, +} from 'n8n-workflow'; import type { INode, IWebhookData, IHttpRequestMethods, IWorkflowBase } from 'n8n-workflow'; import { NotFoundError } from '@/errors/response-errors/not-found.error'; @@ -41,6 +47,7 @@ export class LiveWebhooks implements IWebhookManager { private readonly workflowStaticDataService: WorkflowStaticDataService, private readonly workflowsConfig: WorkflowsConfig, private readonly workflowPublishedDataService: WorkflowPublishedDataService, + private readonly expressionEngineConfig: ExpressionEngineConfig, ) {} async getWebhookMethods(path: string) { @@ -131,10 +138,15 @@ export class LiveWebhooks implements IWebhookManager { projectId: ownerProjectId, }); - await workflow.expression.acquireIsolate(); + const startNode = workflow.getNode(webhook.node); + + if (this.webhookPhaseNeedsIsolate(startNode)) { + await workflow.expression.acquireIsolate(); + } + try { const webhookData = this.webhookService - .getNodeWebhooks(workflow, workflow.getNode(webhook.node) as INode, additionalData) + .getNodeWebhooks(workflow, startNode as INode, additionalData) .find((w) => w.httpMethod === httpMethod && w.path === webhook.webhookPath) as IWebhookData; if ( @@ -183,10 +195,38 @@ export class LiveWebhooks implements IWebhookManager { ).catch(reject); // ensure the Promise settles even if executeWebhook throws }); } finally { + // A no-op when the acquire was skipped. await workflow.expression.releaseIsolate(); } } + /** + * Expression Engine VM acquisition builds a V8 isolate per request, which is + * worth skipping when the webhook phase provably evaluates nothing: every + * description field of the trigger resolves natively (see + * `webhookDescriptionFields` in n8n-workflow) and the node's own parameters + * contain no expressions. Anything not proven below acquires eagerly. + */ + private webhookPhaseNeedsIsolate(startNode: INode | null): boolean { + if (!this.expressionEngineConfig.allowWebhookIsolateSkip) return true; + if (startNode === null) return true; + + // Extend only after reviewing the node type's webhook() for + // evaluateExpression() calls or other internal evaluations. + if (startNode.type !== WEBHOOK_NODE_TYPE) return true; + + // typeVersion 1 body parsing evaluates a hardcoded template. + if (startNode.typeVersion === 1) return true; + + if (!nodeParametersAreStatic(startNode)) return true; + + const webhooks = this.nodeTypes.getByNameAndVersion(startNode.type, startNode.typeVersion) + ?.description.webhooks; + if (!webhooks?.length) return true; + + return !webhooks.every(webhookDescriptionIsNativelyResolvable); + } + private async loadWebhookExecutionData( workflowId: string, ): Promise<{ workflow: WorkflowEntity; publishedVersion: WorkflowHistory }> { diff --git a/packages/cli/src/webhooks/webhook-execution-context.ts b/packages/cli/src/webhooks/webhook-execution-context.ts index 3470397bf95..25cf320359c 100644 --- a/packages/cli/src/webhooks/webhook-execution-context.ts +++ b/packages/cli/src/webhooks/webhook-execution-context.ts @@ -8,6 +8,10 @@ import type { IWebhookDescription, NodeParameterValueType, } from 'n8n-workflow'; +import { resolveWebhookDescriptionField } from 'n8n-workflow'; + +/** The description's evaluable fields — the symbol key holds the resolver map. */ +type WebhookDescriptionKey = Exclude; /** * A helper class that holds the context for the webhook execution. @@ -26,10 +30,13 @@ export class WebhookExecutionContext { * Evaluates a simple expression from the webhook description. */ evaluateSimpleWebhookDescriptionExpression( - propertyName: keyof IWebhookDescription, + propertyName: WebhookDescriptionKey, executeData?: IExecuteData, defaultValue?: T, ): T | undefined { + const native = this.resolveNatively(propertyName); + if (native.resolved) return native.value as T | undefined; + return this.workflow.expression.getSimpleParameterValue( this.workflowStartNode, this.webhookData.webhookDescription[propertyName], @@ -44,10 +51,13 @@ export class WebhookExecutionContext { * Evaluates a complex expression from the webhook description. */ evaluateComplexWebhookDescriptionExpression( - propertyName: keyof IWebhookDescription, + propertyName: WebhookDescriptionKey, executeData?: IExecuteData, defaultValue?: T, ): T | undefined { + const native = this.resolveNatively(propertyName); + if (native.resolved) return native.value as T | undefined; + return this.workflow.expression.getComplexParameterValue( this.workflowStartNode, this.webhookData.webhookDescription[propertyName], @@ -57,4 +67,19 @@ export class WebhookExecutionContext { defaultValue, ) as T | undefined; } + + /** + * Resolves a description field without the expression engine when the field + * declares a native resolver and the node's parameters are static (see + * `webhookDescriptionFields` in n8n-workflow). Like the engine path, the + * resolved value is returned as-is: `defaultValue` only stands in for a + * field the description does not define at all. + */ + private resolveNatively(propertyName: WebhookDescriptionKey) { + return resolveWebhookDescriptionField( + this.workflowStartNode, + this.webhookData.webhookDescription, + String(propertyName), + ); + } } diff --git a/packages/cli/src/webhooks/webhook-helpers.ts b/packages/cli/src/webhooks/webhook-helpers.ts index 205eaa3b526..f35fa497f25 100644 --- a/packages/cli/src/webhooks/webhook-helpers.ts +++ b/packages/cli/src/webhooks/webhook-helpers.ts @@ -543,14 +543,7 @@ export async function executeWebhook( responsePropertyName, responseContentType, responseBinaryPropertyName, - } = evaluateResponseOptions( - workflowStartNode, - workflow, - req, - webhookData, - executionMode, - additionalKeys, - ); + } = evaluateResponseOptions(context, req); if ( !['onReceived', 'lastNode', 'responseNode', 'formPage', 'streaming', 'hostedChat'].includes( @@ -1137,71 +1130,44 @@ export async function executeWebhook( /** * Evaluates the response mode, code and data for a webhook node */ -function evaluateResponseOptions( - workflowStartNode: INode, - workflow: Workflow, - req: WebhookRequest, - webhookData: IWebhookData, - executionMode: WorkflowExecuteMode, - additionalKeys: IWorkflowDataProxyAdditionalKeys, -) { +function evaluateResponseOptions(context: WebhookExecutionContext, req: WebhookRequest) { + const { workflow, workflowStartNode } = context; + //check if response mode should be set automatically, e.g. multipage form const responseMode = autoDetectResponseMode(workflowStartNode, workflow, req.method) ?? - (workflow.expression.getSimpleParameterValue( - workflowStartNode, - webhookData.webhookDescription.responseMode, - executionMode, - additionalKeys, + context.evaluateSimpleWebhookDescriptionExpression( + 'responseMode', undefined, 'onReceived', - ) as WebhookResponseMode); + )!; - const responseCode = workflow.expression.getSimpleParameterValue( - workflowStartNode, - webhookData.webhookDescription.responseCode as string, - executionMode, - additionalKeys, + const responseCode = context.evaluateSimpleWebhookDescriptionExpression( + 'responseCode', undefined, 200, - ) as number; + )!; // This parameter is used for two different purposes: // 1. as arbitrary string input defined in the workflow in the "respond immediately" mode, // 2. as well as WebhookResponseData config in all the other modes - const responseData = workflow.expression.getComplexParameterValue( - workflowStartNode, - webhookData.webhookDescription.responseData, - executionMode, - additionalKeys, - undefined, - 'firstEntryJson', - ) as WebhookResponseData | string | undefined; + const responseData = context.evaluateComplexWebhookDescriptionExpression< + WebhookResponseData | string + >('responseData', undefined, 'firstEntryJson'); // This is needed for backward compatibility, where only the first main output was checked for data. // We want to keep existing behavior for webhooks, but change for chat triggers, where checking all main outputs makes more sense. // We can unify the behavior in the next major release and get rid of this flag const checkAllMainOutputs = workflowStartNode.type === CHAT_TRIGGER_NODE_TYPE; - const responsePropertyName = workflow.expression.getSimpleParameterValue( - workflowStartNode, - webhookData.webhookDescription.responsePropertyName, - executionMode, - additionalKeys, - ) as string | undefined; + const responsePropertyName = + context.evaluateSimpleWebhookDescriptionExpression('responsePropertyName'); - const responseContentType = workflow.expression.getSimpleParameterValue( - workflowStartNode, - webhookData.webhookDescription.responseContentType, - executionMode, - additionalKeys, - ) as string | undefined; + const responseContentType = + context.evaluateSimpleWebhookDescriptionExpression('responseContentType'); - const responseBinaryPropertyName = workflow.expression.getSimpleParameterValue( - workflowStartNode, - webhookData.webhookDescription.responseBinaryPropertyName, - executionMode, - additionalKeys, + const responseBinaryPropertyName = context.evaluateSimpleWebhookDescriptionExpression( + 'responseBinaryPropertyName', undefined, 'data', ); diff --git a/packages/cli/src/webhooks/webhook.service.ts b/packages/cli/src/webhooks/webhook.service.ts index 65ec70fb2bb..03c35f16376 100644 --- a/packages/cli/src/webhooks/webhook.service.ts +++ b/packages/cli/src/webhooks/webhook.service.ts @@ -7,6 +7,7 @@ import { ensureError } from '@n8n/utils/errors/ensure-error'; import { isNodeClassInstance, NodeHelpers, + resolveWebhookDescriptionField, UnexpectedError, WebhookPathTakenError, } from 'n8n-workflow'; @@ -15,6 +16,7 @@ import type { INode, IRunExecutionData, IWebhookData, + IWebhookDescription, IWebhookResponseData, IWorkflowExecuteAdditionalData, WebhookSetupMethodNames, @@ -331,7 +333,6 @@ export class WebhookService { } const workflowId = workflow.id || '__UNSAVED__'; - const mode = 'internal'; const returnData: IWebhookData[] = []; for (const webhookDescription of nodeType.description.webhooks) { @@ -339,13 +340,13 @@ export class WebhookService { continue; } - let nodeWebhookPath = workflow.expression.getSimpleParameterValue( + let nodeWebhookPath = this.evaluateDescriptionProperty( + workflow, node, - webhookDescription.path, - mode, - {}, + webhookDescription, + 'path', ); - if (nodeWebhookPath === undefined) { + if (nodeWebhookPath === undefined || nodeWebhookPath === null) { this.logger.error( `No webhook path could be found for node "${node.name}" in workflow "${workflowId}".`, ); @@ -361,20 +362,18 @@ export class WebhookService { nodeWebhookPath = nodeWebhookPath.slice(0, -1); } - const isFullPath: boolean = workflow.expression.getSimpleParameterValue( + const isFullPath = this.evaluateDescriptionProperty( + workflow, node, - webhookDescription.isFullPath, - 'internal', - {}, - undefined, + webhookDescription, + 'isFullPath', false, ) as boolean; - const restartWebhook: boolean = workflow.expression.getSimpleParameterValue( + const restartWebhook = this.evaluateDescriptionProperty( + workflow, node, - webhookDescription.restartWebhook, - 'internal', - {}, - undefined, + webhookDescription, + 'restartWebhook', false, ) as boolean; const path = NodeHelpers.getNodeWebhookPath( @@ -385,12 +384,11 @@ export class WebhookService { restartWebhook, ); - const webhookMethods = workflow.expression.getSimpleParameterValue( + const webhookMethods = this.evaluateDescriptionProperty( + workflow, node, - webhookDescription.httpMethod, - mode, - {}, - undefined, + webhookDescription, + 'httpMethod', 'GET', ); @@ -426,6 +424,32 @@ export class WebhookService { return returnData; } + /** + * Evaluates a webhook-description property, preferring the field's native + * resolver (see `webhookDescriptionFields` in n8n-workflow) so static-parameter + * nodes never engage the expression engine. Falls back to the engine, which + * returns plain values as-is and only evaluates `=` templates. + */ + private evaluateDescriptionProperty( + workflow: Workflow, + node: INode, + webhookDescription: IWebhookDescription, + property: string, + defaultValue?: string | boolean, + ) { + const native = resolveWebhookDescriptionField(node, webhookDescription, property); + if (native.resolved) return native.value; + + return workflow.expression.getSimpleParameterValue( + node, + webhookDescription[property], + 'internal', + {}, + undefined, + defaultValue, + ); + } + private async _findWebhookConflicts( workflow: Workflow, checkEntries: Array<{ diff --git a/packages/core/src/execution-engine/node-execution-context/utils/webhook-helper-functions.ts b/packages/core/src/execution-engine/node-execution-context/utils/webhook-helper-functions.ts index d26fa55d490..46c0083a28e 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/webhook-helper-functions.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/webhook-helper-functions.ts @@ -7,7 +7,7 @@ import type { IWorkflowDataProxyAdditionalKeys, IWebhookDescription, } from 'n8n-workflow'; -import { NodeHelpers } from 'n8n-workflow'; +import { NodeHelpers, resolveWebhookDescriptionField } from 'n8n-workflow'; /** Returns the full webhook description of the webhook with the given name */ export function getWebhookDescription( @@ -51,21 +51,31 @@ export function getNodeWebhookUrl( baseUrl = isTest === true ? additionalData.webhookTestBaseUrl : additionalData.webhookBaseUrl; } - const path = workflow.expression.getSimpleParameterValue( - node, - webhookDescription.path, - mode, - additionalKeys, - ); - if (path === undefined) return; + // Prefer the field's native resolver (see `webhookDescriptionFields` in + // n8n-workflow) so static-parameter nodes never engage the expression engine. + const nativePath = resolveWebhookDescriptionField(node, webhookDescription, 'path'); + const path = nativePath.resolved + ? nativePath.value + : workflow.expression.getSimpleParameterValue( + node, + webhookDescription.path, + mode, + additionalKeys, + ); + if (path === undefined || path === null) return; - const isFullPath: boolean = workflow.expression.getSimpleParameterValue( - node, - webhookDescription.isFullPath, - mode, - additionalKeys, - undefined, - false, + const nativeIsFullPath = resolveWebhookDescriptionField(node, webhookDescription, 'isFullPath'); + const isFullPath = ( + nativeIsFullPath.resolved + ? nativeIsFullPath.value + : workflow.expression.getSimpleParameterValue( + node, + webhookDescription.isFullPath, + mode, + additionalKeys, + undefined, + false, + ) ) as boolean; return NodeHelpers.getNodeWebhookUrl(baseUrl, workflow.id, node, path.toString(), isFullPath); } diff --git a/packages/nodes-base/nodes/Webhook/description.ts b/packages/nodes-base/nodes/Webhook/description.ts index 607da941a54..4310fc10480 100644 --- a/packages/nodes-base/nodes/Webhook/description.ts +++ b/packages/nodes-base/nodes/Webhook/description.ts @@ -4,6 +4,7 @@ import type { INodeTypeDescription, IWebhookDescription, } from 'n8n-workflow'; +import { fromFunction, fromParameter, webhookDescriptionFields } from 'n8n-workflow'; import { getResponseCode, getResponseData } from './utils'; @@ -20,18 +21,23 @@ const n8nOAuth2AuthOption: INodePropertyOptions = { envFeatureFlag: 'WEBHOOK_PRIVATE_CREDENTIALS', }; +// Each field declares its expression template and native resolver in one place: +// the editor evaluates the generated template strings, while the backend reads +// parameters directly (no expression engine) whenever they are static. export const defaultWebhookDescription: IWebhookDescription = { name: 'default', - httpMethod: '={{$parameter["httpMethod"] || "GET"}}', isFullPath: true, - responseCode: `={{(${getResponseCode})($parameter)}}`, - responseMode: '={{$parameter["responseMode"]}}', - responseData: `={{(${getResponseData})($parameter)}}`, - responseBinaryPropertyName: '={{$parameter["responseBinaryPropertyName"]}}', - responseContentType: '={{$parameter["options"]["responseContentType"]}}', - responsePropertyName: '={{$parameter["options"]["responsePropertyName"]}}', - responseHeaders: '={{$parameter["options"]["responseHeaders"]}}', - path: '={{$parameter["path"]}}', + ...webhookDescriptionFields({ + httpMethod: fromParameter('httpMethod', 'GET'), + responseCode: fromFunction(getResponseCode), + responseMode: fromParameter('responseMode'), + responseData: fromFunction(getResponseData), + responseBinaryPropertyName: fromParameter('responseBinaryPropertyName'), + responseContentType: fromParameter(['options', 'responseContentType']), + responsePropertyName: fromParameter(['options', 'responsePropertyName']), + responseHeaders: fromParameter(['options', 'responseHeaders']), + path: fromParameter('path'), + }), }; export const credentialsProperty = ( diff --git a/packages/nodes-base/nodes/Webhook/test/description.test.ts b/packages/nodes-base/nodes/Webhook/test/description.test.ts new file mode 100644 index 00000000000..648fe53c1cb --- /dev/null +++ b/packages/nodes-base/nodes/Webhook/test/description.test.ts @@ -0,0 +1,119 @@ +import type { IDataObject, INodeParameters, INodeType, INodeTypes } from 'n8n-workflow'; +import { Workflow, WEBHOOK_RESOLVERS, webhookDescriptionIsNativelyResolvable } from 'n8n-workflow'; + +import { defaultWebhookDescription } from '../description'; +import { Webhook } from '../Webhook.node'; + +// Pins what lets `LiveWebhooks` resolve this description without the expression +// engine: every field declares a native resolver (or is a plain value), each +// resolver returns exactly what its template returns through the engine, and +// the generated template strings are the long-standing hand-written ones the +// editor keeps evaluating. + +const webhookNode = new Webhook(); + +const nodeTypes: INodeTypes = { + getByName: () => webhookNode as unknown as INodeType, + getByNameAndVersion: () => webhookNode as unknown as INodeType, + getKnownTypes: () => ({}) as IDataObject, +}; + +const nodeWithParameters = (parameters: INodeParameters) => { + const workflow = new Workflow({ + id: '1', + nodes: [ + { + name: 'Webhook', + type: 'n8n-nodes-base.webhook', + typeVersion: 2.1, + id: 'webhook-1', + position: [0, 0], + parameters, + }, + ], + connections: {}, + active: true, + nodeTypes, + }); + + return { workflow, node: workflow.getNode('Webhook')! }; +}; + +describe('defaultWebhookDescription', () => { + it('is fully resolvable without the expression engine', () => { + expect(webhookDescriptionIsNativelyResolvable(defaultWebhookDescription)).toBe(true); + }); + + it('generates the same template strings the description has always shipped', () => { + expect(defaultWebhookDescription).toMatchObject({ + httpMethod: '={{$parameter["httpMethod"] || "GET"}}', + responseMode: '={{$parameter["responseMode"]}}', + responseBinaryPropertyName: '={{$parameter["responseBinaryPropertyName"]}}', + responseContentType: '={{$parameter["options"]["responseContentType"]}}', + responsePropertyName: '={{$parameter["options"]["responsePropertyName"]}}', + responseHeaders: '={{$parameter["options"]["responseHeaders"]}}', + path: '={{$parameter["path"]}}', + }); + // The function-body templates inline the functions' source, as before. + expect(defaultWebhookDescription.responseCode).toMatch(/^=\{\{\(.+\)\(\$parameter\)\}\}$/s); + expect(defaultWebhookDescription.responseData).toMatch(/^=\{\{\(.+\)\(\$parameter\)\}\}$/s); + }); + + describe('resolvers match their templates', () => { + const parameterSets: Array<{ case: string; parameters: INodeParameters }> = [ + { case: 'defaults only', parameters: {} }, + { + case: 'respond immediately with a fixed body', + parameters: { responseMode: 'onReceived', options: {} }, + }, + { + case: 'last node, all entries', + parameters: { responseMode: 'lastNode', responseData: 'allEntries', options: {} }, + }, + { case: 'respond via node', parameters: { responseMode: 'responseNode', options: {} } }, + { + case: 'no response body', + parameters: { responseMode: 'onReceived', options: { noResponseBody: true } }, + }, + { + case: 'response data from options', + parameters: { responseMode: 'onReceived', options: { responseData: 'noData' } }, + }, + { + case: 'custom status code', + parameters: { options: { responseCode: { values: { responseCode: 201 } } } }, + }, + { + case: 'custom status code with a custom value', + parameters: { options: { responseCode: { values: { responseCode: 0, customCode: 418 } } } }, + }, + { + case: 'legacy typeVersion 1 status code', + parameters: { responseCode: 302, options: {} }, + }, + { + case: 'multiple methods', + parameters: { httpMethod: ['GET', 'POST'], multipleMethods: true, options: {} }, + }, + ]; + + const resolvers = defaultWebhookDescription[WEBHOOK_RESOLVERS]!; + + describe.each(parameterSets)('$case', ({ parameters }) => { + const { workflow, node } = nodeWithParameters(parameters); + + test.each(Object.keys(resolvers))('%s', (field) => { + const native = resolvers[field].resolve(node.parameters); + + const viaEngine = workflow.expression.getSimpleParameterValue( + node, + defaultWebhookDescription[field] as string, + 'internal', + {}, + ); + + expect(native).toEqual(viaEngine); + }); + }); + }); +}); diff --git a/packages/testing/playwright/services/api-helper.ts b/packages/testing/playwright/services/api-helper.ts index 2091d3c0bbe..d9c658a7b21 100644 --- a/packages/testing/playwright/services/api-helper.ts +++ b/packages/testing/playwright/services/api-helper.ts @@ -21,6 +21,7 @@ import { DynamicCredentialApiHelper } from './dynamic-credential-api-helper'; import { ExternalSecretsApiHelper } from './external-secrets-api-helper'; import { McpApiHelper } from './mcp-api-helper'; import { McpOAuthApiHelper } from './mcp-oauth-api-helper'; +import { MetricsApiHelper } from './metrics-api-helper'; import { ProjectApiHelper } from './project-api-helper'; import { PublicApiHelper } from './public-api-helper'; import { RoleApiHelper } from './role-api-helper'; @@ -69,6 +70,7 @@ export class ApiHelpers { request: APIRequestContext; workflows: WorkflowApiHelper; webhooks: WebhookApiHelper; + metrics: MetricsApiHelper; mcp: McpApiHelper; mcpOauth: McpOAuthApiHelper; projects: ProjectApiHelper; @@ -89,6 +91,7 @@ export class ApiHelpers { this.request = requestContext; this.workflows = new WorkflowApiHelper(this); this.webhooks = new WebhookApiHelper(this); + this.metrics = new MetricsApiHelper(this); this.mcp = new McpApiHelper(this); this.mcpOauth = new McpOAuthApiHelper(this); this.projects = new ProjectApiHelper(this); diff --git a/packages/testing/playwright/services/metrics-api-helper.ts b/packages/testing/playwright/services/metrics-api-helper.ts new file mode 100644 index 00000000000..89cbdb91f88 --- /dev/null +++ b/packages/testing/playwright/services/metrics-api-helper.ts @@ -0,0 +1,21 @@ +import { TestError } from '../Types'; +import type { ApiHelpers } from './api-helper'; + +/** Reads Prometheus counters from `/metrics` (requires `N8N_METRICS=true`). */ +export class MetricsApiHelper { + constructor(private readonly api: ApiHelpers) {} + + /** Returns the value of the first metric line starting with `name`, or 0 when absent. */ + async getCounter(name: string): Promise { + const response = await this.api.request.get('/metrics'); + if (!response.ok()) { + throw new TestError( + `Failed to fetch /metrics (is N8N_METRICS enabled?): ${response.status()}`, + ); + } + const metrics = await response.text(); + const line = metrics.split('\n').find((l) => l.startsWith(name)); + + return line ? Number(line.split(' ')[1]) : 0; + } +} diff --git a/packages/testing/playwright/tests/e2e/api/webhook-isolate-skip.spec.ts b/packages/testing/playwright/tests/e2e/api/webhook-isolate-skip.spec.ts new file mode 100644 index 00000000000..366e900c506 --- /dev/null +++ b/packages/testing/playwright/tests/e2e/api/webhook-isolate-skip.spec.ts @@ -0,0 +1,184 @@ +import { test, expect } from '../../../fixtures/base'; +import type { ApiHelpers } from '../../../services/api-helper'; + +/** + * A production webhook whose trigger provably evaluates nothing skips acquiring + * an expression isolate for the webhook phase (`LiveWebhooks`). That proof is a + * scan of declared values — it cannot see an `evaluateExpression()` call added + * to a node's `webhook()` or to a helper it calls. Such a call throws + * `IsolateError: No bridge acquired` under `N8N_EXPRESSION_ENGINE=vm`, which is + * what these tests guard: a new evaluation on the webhook phase turns the + * request into a 500. + */ +const ACQUIRED_METRIC = 'n8n_expression_pool_acquired_total'; + +// The acquire counter is instance-global, so the metric-delta tests must not +// run concurrently with this file's other webhook-firing tests. +test.describe.configure({ mode: 'default' }); + +async function isolateAcquires(api: ApiHelpers) { + return await api.metrics.getCounter(ACQUIRED_METRIC); +} + +/** Acquires attributable to one request, webhook phase and execution together. */ +async function acquiresForTrigger(api: ApiHelpers, workflowFile: string, expected: string) { + const { webhookPath } = await api.workflows.importWorkflowFromFile(workflowFile); + + const before = await isolateAcquires(api); + const response = await api.webhooks.trigger(`/webhook/${webhookPath}`, { method: 'POST' }); + + expect(response.ok()).toBe(true); + expect(await response.json()).toMatchObject({ result: expected }); + + return (await isolateAcquires(api)) - before; +} + +/** + * Imports the static workflow with a transform over the trigger's parameters + * (and optionally the node itself), then triggers it. Every case in the + * request-path matrix below runs with the webhook-phase isolate skipped, so a + * newly added evaluation anywhere on that path fails these as a 500. + */ +async function triggerTransformed( + api: ApiHelpers, + transformNode: (node: { + typeVersion: number; + parameters: Record; + }) => void, + requestOptions: Parameters[1] = {}, +) { + const { webhookPath } = await api.workflows.importWorkflowFromFile( + 'webhook-isolate-skip-static.json', + { + transform: (workflow) => { + transformNode(workflow.nodes![0] as never); + return workflow; + }, + }, + ); + + return await api.webhooks.trigger(`/webhook/${webhookPath}`, { + method: 'POST', + ...requestOptions, + }); +} + +test.describe( + 'Webhook isolate skip', + { annotation: [{ type: 'owner', description: 'Catalysts' }] }, + () => { + test('skips the webhook-phase isolate only when the trigger evaluates nothing', async ({ + api, + }) => { + // Identical workflows but for the trigger's parameters, so the difference + // in acquires is the webhook phase — the execution costs the same in both + const staticAcquires = await acquiresForTrigger( + api, + 'webhook-isolate-skip-static.json', + 'static-ok', + ); + const expressionAcquires = await acquiresForTrigger( + api, + 'webhook-isolate-skip-expression.json', + 'expression-ok', + ); + + expect(expressionAcquires).toBeGreaterThan(staticAcquires); + }); + + test('serves an immediate response with custom code, body and headers natively', async ({ + api, + }) => { + const response = await triggerTransformed(api, (node) => { + node.parameters.responseMode = 'onReceived'; + node.parameters.options = { + responseData: 'custom-body', + responseCode: { values: { responseCode: 202 } }, + responseHeaders: { entries: [{ name: 'x-isolate-skip', value: 'native' }] }, + }; + }); + + expect(response.status()).toBe(202); + expect(await response.text()).toBe('custom-body'); + expect(response.headers()['x-isolate-skip']).toBe('native'); + }); + + test('serves an empty-body response natively', async ({ api }) => { + const response = await triggerTransformed(api, (node) => { + node.parameters.responseMode = 'onReceived'; + node.parameters.options = { noResponseBody: true }; + }); + + expect(response.status()).toBe(200); + expect(await response.text()).toBe(''); + }); + + test('serves a last-node response with property selection and content type natively', async ({ + api, + }) => { + const response = await triggerTransformed(api, (node) => { + node.parameters.options = { + responsePropertyName: 'result', + responseContentType: 'text/plain', + }; + }); + + expect(response.ok()).toBe(true); + expect(response.headers()['content-type']).toContain('text/plain'); + expect(await response.text()).toContain('static-ok'); + }); + + test('parses a multipart body without the webhook-phase isolate', async ({ api }) => { + const boundary = '----isolateSkipBoundary'; + const response = await triggerTransformed(api, () => {}, { + headers: { 'content-type': `multipart/form-data; boundary=${boundary}` }, + data: [ + `--${boundary}`, + 'Content-Disposition: form-data; name="field1"', + '', + 'value1', + `--${boundary}--`, + '', + ].join('\r\n'), + }); + + expect(response.ok()).toBe(true); + expect(await response.json()).toMatchObject({ result: 'static-ok' }); + }); + + test('acquires for typeVersion 1, whose body parsing evaluates a template', async ({ api }) => { + // Same comparative shape as the headline test: v1 must fall back to the + // engine, so it costs strictly more acquires than the gated v2 request. + const staticAcquires = await acquiresForTrigger( + api, + 'webhook-isolate-skip-static.json', + 'static-ok', + ); + + const before = await isolateAcquires(api); + const response = await triggerTransformed(api, (node) => { + node.typeVersion = 1; + }); + expect(response.ok()).toBe(true); + const v1Acquires = (await isolateAcquires(api)) - before; + + expect(v1Acquires).toBeGreaterThan(staticAcquires); + }); + + test('rejects an unauthenticated n8n user-auth request without the webhook-phase isolate', async ({ + api, + }) => { + // The deepest skip-path case: `authentication: 'n8nOAuth2'` is a static + // parameter, so the skip applies, and the 401 is produced inside + // `webhook()` after resolving the webhook URL natively. Credential + // resolution never uses this workflow's bridge — a 500 here would mean + // it started to. + const response = await triggerTransformed(api, (node) => { + node.parameters.authentication = 'n8nOAuth2'; + }); + + expect(response.status()).toBe(401); + expect(response.headers()['www-authenticate']).toContain('n8n Webhook'); + }); + }, +); diff --git a/packages/testing/playwright/workflows/webhook-isolate-skip-expression.json b/packages/testing/playwright/workflows/webhook-isolate-skip-expression.json new file mode 100644 index 00000000000..a2d600086fc --- /dev/null +++ b/packages/testing/playwright/workflows/webhook-isolate-skip-expression.json @@ -0,0 +1,57 @@ +{ + "name": "Webhook Isolate Skip - Expression Parameter", + "active": true, + "nodes": [ + { + "parameters": { + "httpMethod": "POST", + "path": "isolate-skip-expression", + "responseMode": "lastNode", + "options": { + "onlyRunIf": "={{ true }}" + } + }, + "id": "b1e1c9b0-0000-4000-8000-000000000011", + "name": "Webhook", + "type": "n8n-nodes-base.webhook", + "typeVersion": 2, + "position": [0, 0], + "webhookId": "b1e1c9b0-0000-4000-8000-000000000011" + }, + { + "parameters": { + "assignments": { + "assignments": [ + { + "id": "result", + "name": "result", + "value": "expression-ok", + "type": "string" + } + ] + }, + "options": {} + }, + "id": "b1e1c9b0-0000-4000-8000-000000000012", + "name": "Set Result", + "type": "n8n-nodes-base.set", + "typeVersion": 3.3, + "position": [220, 0] + } + ], + "connections": { + "Webhook": { + "main": [ + [ + { + "node": "Set Result", + "type": "main", + "index": 0 + } + ] + ] + } + }, + "settings": {}, + "pinData": {} +} diff --git a/packages/testing/playwright/workflows/webhook-isolate-skip-static.json b/packages/testing/playwright/workflows/webhook-isolate-skip-static.json new file mode 100644 index 00000000000..dca8ee541ef --- /dev/null +++ b/packages/testing/playwright/workflows/webhook-isolate-skip-static.json @@ -0,0 +1,45 @@ +{ + "name": "Webhook Isolate Skip - Static Parameters", + "active": true, + "nodes": [ + { + "parameters": { + "httpMethod": "POST", + "path": "isolate-skip-static", + "responseMode": "lastNode", + "options": {} + }, + "id": "b1e1c9b0-0000-4000-8000-000000000001", + "name": "Webhook", + "type": "n8n-nodes-base.webhook", + "typeVersion": 2, + "position": [0, 0], + "webhookId": "b1e1c9b0-0000-4000-8000-000000000001" + }, + { + "parameters": { + "assignments": { + "assignments": [ + { + "id": "result", + "name": "result", + "value": "static-ok", + "type": "string" + } + ] + }, + "options": {} + }, + "id": "b1e1c9b0-0000-4000-8000-000000000002", + "name": "Set Result", + "type": "n8n-nodes-base.set", + "typeVersion": 3.3, + "position": [220, 0] + } + ], + "connections": { + "Webhook": { "main": [[{ "node": "Set Result", "type": "main", "index": 0 }]] } + }, + "settings": {}, + "pinData": {} +} diff --git a/packages/workflow/src/index.ts b/packages/workflow/src/index.ts index d4acf261abc..1233244644d 100644 --- a/packages/workflow/src/index.ts +++ b/packages/workflow/src/index.ts @@ -15,6 +15,16 @@ export * from './dynamic-credentials-helpers'; export * from './safe-regex'; export * from './global-state'; export * from './interfaces'; +export { + fromFunction, + fromParameter, + nodeParametersAreStatic, + resolveWebhookDescriptionField, + webhookDescriptionFields, + webhookDescriptionIsNativelyResolvable, + type NativeResolution, + type WebhookDescriptionField, +} from './webhook-description-fields'; export * from './sub-workflow-output'; export * from './run-execution-data-factory'; export * from './message-event-bus'; diff --git a/packages/workflow/src/interfaces.ts b/packages/workflow/src/interfaces.ts index eae2aafa33e..7fa7c12cb98 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -3004,8 +3004,31 @@ export interface IWebhookData { export type WebhookType = 'default' | 'setup'; +/** + * Key under which an {@link IWebhookDescription} holds native (engine-free) + * resolvers for its expression-template fields, keyed by field name. Populated + * by `webhookDescriptionFields()` and read via `resolveWebhookDescriptionField()`. + * Backend-only: not serialized with the description. + */ +export const WEBHOOK_RESOLVERS: unique symbol = Symbol.for('n8n.webhookDescriptionResolvers'); + +/** + * Native resolvers for a webhook description's fields, keyed by field name. + * Each entry pairs the expression template a field carries with a function + * computing the same value from the node's parameters, without the expression + * engine. Stored under {@link WEBHOOK_RESOLVERS}. + */ +export type NativeParameterResolvers = Record< + string, + { + template: string; + resolve: (parameters: INodeParameters) => NodeParameterValueType | undefined; + } +>; + export interface IWebhookDescription { [key: string]: IHttpRequestMethods | WebhookResponseMode | boolean | string | undefined; + [WEBHOOK_RESOLVERS]?: NativeParameterResolvers; httpMethod: IHttpRequestMethods | string; isFullPath?: boolean; name: WebhookType; diff --git a/packages/workflow/src/webhook-description-fields.ts b/packages/workflow/src/webhook-description-fields.ts new file mode 100644 index 00000000000..602b61df96f --- /dev/null +++ b/packages/workflow/src/webhook-description-fields.ts @@ -0,0 +1,153 @@ +/** + * Single-source declaration of webhook-description fields. + * + * A webhook description needs values derived from the node instance's + * parameters, historically encoded as expression templates such as + * `={{$parameter["path"]}}`. The editor still needs those strings (they are + * what survives the JSON serialization of node descriptions), but on the + * backend they force the expression engine (under `N8N_EXPRESSION_ENGINE=vm`, + * a V8 isolate) into every webhook request even when the user authored no + * expression. + * + * The helpers here generate the template string and a native resolver from one + * declaration, so the two cannot drift. Backend resolution sites call + * {@link resolveWebhookDescriptionField} first and only fall back to the engine + * when the user's parameters actually contain expressions. + */ + +import type { + INode, + INodeParameters, + IWebhookDescription, + NativeParameterResolvers, + NodeParameterValueType, +} from './interfaces'; +import { WEBHOOK_RESOLVERS } from './interfaces'; +import { isResourceLocatorValue } from './type-guards'; + +export type WebhookDescriptionField = NativeParameterResolvers[string]; + +/** + * A field that reads a node parameter, e.g. `fromParameter('httpMethod', 'GET')` + * → template `={{$parameter["httpMethod"] || "GET"}}` and a resolver applying + * the same truthiness fallback. Nested reads take a path array. Walking into a + * missing parent yields `undefined`, matching what the engine returns for the + * same reference. + */ +export function fromParameter(path: string | string[], fallback?: string): WebhookDescriptionField { + const segments = Array.isArray(path) ? path : [path]; + const accessor = segments.map((segment) => `["${segment}"]`).join(''); + const tail = fallback === undefined ? '' : ` || ${JSON.stringify(fallback)}`; + + return { + template: `={{$parameter${accessor}${tail}}}`, + resolve: (parameters) => { + let current: unknown = parameters; + for (const key of segments) { + if (current === null || current === undefined) return fallback; + current = (current as INodeParameters)[key]; + } + // `||` in the template is plain JS truthiness, not a nullish check. + if (fallback !== undefined && !current) return fallback; + return current as NodeParameterValueType | undefined; + }, + }; +} + +/** + * A field computed by a function of the parameters: the template inlines the + * function's source and the resolver is the function itself, so both + * representations run the same code. + * + * The function MUST be self-contained: closing over an outer identifier (an + * import, a module constant) produces a template the expression sandbox cannot + * evaluate. The parity test in the Webhook node's description.test.ts catches + * this; add one for any node that declares `fromFunction` fields. + */ +export function fromFunction

( + fn: (parameters: P) => NodeParameterValueType | undefined, +): WebhookDescriptionField { + return { + template: `={{(${String(fn)})($parameter)}}`, + resolve: fn as WebhookDescriptionField['resolve'], + }; +} + +/** + * Spreads into an {@link IWebhookDescription}: the template string per field, + * plus the resolver map under the backend-only {@link WEBHOOK_RESOLVERS} key. + */ +export function webhookDescriptionFields( + fields: Record, +): Record & { [WEBHOOK_RESOLVERS]: NativeParameterResolvers } { + const templates = {} as Record; + const resolvers: NativeParameterResolvers = {}; + for (const [field, entry] of Object.entries(fields)) { + templates[field as K] = entry.template; + resolvers[field] = entry; + } + return { ...templates, [WEBHOOK_RESOLVERS]: resolvers }; +} + +export type NativeResolution = + | { resolved: true; value: NodeParameterValueType | undefined } + | { resolved: false }; + +const NOT_RESOLVED: NativeResolution = { resolved: false }; + +/** + * Resolves a description field without the expression engine when the field + * declares a resolver, still carries the exact template that resolver was + * generated from (a description built by spreading another inherits resolvers + * for fields it overrides), and the node's parameters are static. + * + * Pass the workflow's own node (`workflow.nodes[name]`): its parameters + * include the defaults the `Workflow` constructor applies. + */ +export function resolveWebhookDescriptionField( + node: Pick, + description: IWebhookDescription, + field: string, +): NativeResolution { + const entry = description[WEBHOOK_RESOLVERS]?.[field]; + if (entry === undefined || description[field] !== entry.template) return NOT_RESOLVED; + if (!nodeParametersAreStatic(node)) return NOT_RESOLVED; + return { resolved: true, value: entry.resolve(node.parameters) }; +} + +/** + * Whether every field of the description is engine-free for a static-parameter + * node: either a resolver is declared for it, or the value contains no + * expression template at any depth. A template added later without a resolver + * makes this false, so callers skipping isolate acquisition on the strength of + * this stay correct automatically. + */ +export function webhookDescriptionIsNativelyResolvable(description: IWebhookDescription): boolean { + const resolvers = description[WEBHOOK_RESOLVERS]; + return Object.entries(description).every( + ([field, value]) => resolvers?.[field]?.template === value || !containsDynamicValue(value), + ); +} + +/** + * Whether a node's parameters can stand in for the `$parameter` proxy: beyond a + * plain property read, the proxy only resolves `=` values, unwraps + * resource-locator values and handles `&sibling` lookups — all of which require + * a dynamic value somewhere in the parameters. + */ +export function nodeParametersAreStatic(node: Pick): boolean { + return !containsDynamicValue(node.parameters); +} + +function containsDynamicValue(value: unknown): boolean { + if (typeof value === 'string') return value.startsWith('='); + + if (Array.isArray(value)) return value.some(containsDynamicValue); + + if (value !== null && typeof value === 'object') { + if (isResourceLocatorValue(value)) return true; + return Object.keys(value).some((key) => containsDynamicValue(Reflect.get(value, key))); + } + + return false; +} diff --git a/packages/workflow/test/webhook-description-fields.test.ts b/packages/workflow/test/webhook-description-fields.test.ts new file mode 100644 index 00000000000..c841c93f58b --- /dev/null +++ b/packages/workflow/test/webhook-description-fields.test.ts @@ -0,0 +1,303 @@ +import type { + IDataObject, + INodeParameters, + INodeType, + INodeTypes, + IWebhookDescription, +} from '../src/interfaces'; +import { WEBHOOK_RESOLVERS } from '../src/interfaces'; +import { + fromFunction, + fromParameter, + nodeParametersAreStatic, + resolveWebhookDescriptionField, + webhookDescriptionFields, + webhookDescriptionIsNativelyResolvable, +} from '../src/webhook-description-fields'; +import { Workflow } from '../src/workflow'; + +// Template and resolver are generated from one declaration, so this parity +// corpus is the entire sync guarantee: for every field shape, the resolver must +// return exactly what the generated template returns through the engine. + +// Declaring the parameters matters: the `Workflow` constructor runs +// `NodeHelpers.getNodeParameters`, which applies declared defaults — and the +// engine's `$parameter` proxy reads that same post-processed object, which is +// what resolvers read too. +const webhookNodeType: INodeType = { + description: { + displayName: 'Webhook', + name: 'webhook', + group: ['trigger'], + version: 1, + description: '', + defaults: { name: 'Webhook' }, + inputs: [], + outputs: ['main'], + properties: [ + { displayName: 'Method', name: 'httpMethod', type: 'string', default: 'GET' }, + { displayName: 'Path', name: 'path', type: 'string', default: '' }, + { displayName: 'Respond', name: 'responseMode', type: 'string', default: 'onReceived' }, + { + displayName: 'Options', + name: 'options', + type: 'collection', + default: {}, + options: [ + { displayName: 'Content-Type', name: 'responseContentType', type: 'string', default: '' }, + { displayName: 'No Body', name: 'noResponseBody', type: 'boolean', default: false }, + ], + }, + ], + }, +}; + +const nodeTypes: INodeTypes = { + getByName: () => webhookNodeType, + getByNameAndVersion: () => webhookNodeType, + getKnownTypes: () => ({}) as IDataObject, +}; + +const nodeWithParameters = (parameters: INodeParameters) => { + const workflow = new Workflow({ + id: '1', + nodes: [ + { + name: 'Webhook', + typeVersion: 1, + type: 'test.webhook', + id: 'webhook-1', + position: [0, 0], + parameters, + }, + ], + connections: {}, + active: false, + nodeTypes, + }); + + return { workflow, node: workflow.getNode('Webhook')! }; +}; + +// This file runs under both vitest projects (legacy and vm), so parity is +// pinned against both engines; the vm engine needs an acquired isolate. +const viaEngine = async (parameters: INodeParameters, template: string) => { + const { workflow, node } = nodeWithParameters(parameters); + await workflow.expression.acquireIsolate(); + try { + return workflow.expression.getSimpleParameterValue(node, template, 'internal', {}); + } finally { + await workflow.expression.releaseIsolate(); + } +}; + +const viaResolver = (parameters: INodeParameters, field: ReturnType) => { + const { node } = nodeWithParameters(parameters); + return field.resolve(node.parameters); +}; + +describe('fromParameter', () => { + it('generates the long-standing template shapes', () => { + expect(fromParameter('path').template).toBe('={{$parameter["path"]}}'); + expect(fromParameter('httpMethod', 'GET').template).toBe( + '={{$parameter["httpMethod"] || "GET"}}', + ); + expect(fromParameter(['options', 'responseContentType']).template).toBe( + '={{$parameter["options"]["responseContentType"]}}', + ); + }); + + describe('resolver matches the engine', () => { + const cases: Array<[string, ReturnType, INodeParameters]> = [ + ['single segment, present', fromParameter('path'), { path: 'my-path' }], + ['single segment, default applies', fromParameter('path'), {}], + ['fallback, value present', fromParameter('httpMethod', 'POST'), { httpMethod: 'DELETE' }], + ['fallback, value falsy', fromParameter('responseMode', 'onReceived'), { responseMode: '' }], + ['fallback, value missing', fromParameter('httpMethod', 'HEAD'), {}], + [ + 'nested, present', + fromParameter(['options', 'responseContentType']), + { options: { responseContentType: 'text/plain' } }, + ], + ['nested, missing leaf', fromParameter(['options', 'responseContentType']), { options: {} }], + [ + 'nested boolean, falsy present', + fromParameter(['options', 'noResponseBody']), + { options: { noResponseBody: false } }, + ], + ]; + + test.each(cases)('%s', async (_name, field, parameters) => { + expect(viaResolver(parameters, field)).toEqual(await viaEngine(parameters, field.template)); + }); + }); + + it('yields undefined when walking into a missing parent, like the engine', async () => { + // 'undeclared' is not in the node type's properties, so no default is + // applied and the parent is truly absent — pins that the engine also + // yields undefined (it does, under both vitest engine projects) rather + // than throwing on the nested access. + const field = fromParameter(['undeclared', 'x']); + expect(viaResolver({}, field)).toBeUndefined(); + expect(await viaEngine({}, field.template)).toBeUndefined(); + + expect(fromParameter(['options', 'responseContentType']).resolve({})).toBeUndefined(); + }); + + it('applies the fallback when walking into a missing parent', () => { + expect(fromParameter(['options', 'responseContentType'], 'text/html').resolve({})).toBe( + 'text/html', + ); + }); +}); + +describe('fromFunction', () => { + const getResponseData = (parameters: INodeParameters) => + parameters.responseMode === 'lastNode' ? 'noData' : undefined; + + it('inlines the function source into the template and reuses it as resolver', () => { + const field = fromFunction(getResponseData); + expect(field.template).toBe(`={{(${getResponseData})($parameter)}}`); + expect(field.resolve).toBe(getResponseData); + }); + + test.each<[string, INodeParameters]>([ + ['branch taken', { responseMode: 'lastNode' }], + ['branch not taken', { responseMode: 'onReceived' }], + ])('resolver matches the engine: %s', async (_name, parameters) => { + const field = fromFunction(getResponseData); + expect(viaResolver(parameters, field)).toEqual(await viaEngine(parameters, field.template)); + }); +}); + +describe('webhookDescriptionFields', () => { + const fields = webhookDescriptionFields({ + path: fromParameter('path'), + httpMethod: fromParameter('httpMethod', 'GET'), + }); + + it('spreads template strings and keeps resolvers under the symbol key', () => { + expect(fields.path).toBe('={{$parameter["path"]}}'); + expect(fields.httpMethod).toBe('={{$parameter["httpMethod"] || "GET"}}'); + expect(Object.keys(fields[WEBHOOK_RESOLVERS])).toEqual(['path', 'httpMethod']); + }); + + it('does not leak resolvers through JSON serialization', () => { + expect(JSON.parse(JSON.stringify(fields))).toEqual({ + path: '={{$parameter["path"]}}', + httpMethod: '={{$parameter["httpMethod"] || "GET"}}', + }); + }); +}); + +describe('resolveWebhookDescriptionField', () => { + const description: IWebhookDescription = { + name: 'default', + isFullPath: true, + ...webhookDescriptionFields({ + path: fromParameter('path'), + httpMethod: fromParameter('httpMethod', 'GET'), + }), + }; + + it('resolves a declared field for a static-parameter node', () => { + expect( + resolveWebhookDescriptionField({ parameters: { path: 'my-path' } }, description, 'path'), + ).toEqual({ resolved: true, value: 'my-path' }); + }); + + it('does not resolve when the node parameters contain an expression', () => { + expect( + resolveWebhookDescriptionField( + { parameters: { path: 'my-path', extra: '={{ $json.x }}' } }, + description, + 'path', + ), + ).toEqual({ resolved: false }); + }); + + it('does not resolve a field without a resolver', () => { + expect(resolveWebhookDescriptionField({ parameters: {} }, description, 'isFullPath')).toEqual({ + resolved: false, + }); + }); +}); + +describe('spread-and-override descriptions (Wait/GitHub pattern)', () => { + // Object spread copies the resolver map's symbol key, so a description built + // by spreading another inherits resolvers for fields it may override. The + // template-identity check must invalidate exactly those. + const base: IWebhookDescription = { + name: 'default', + httpMethod: 'GET', + ...webhookDescriptionFields({ + path: fromParameter('path'), + responseMode: fromParameter('responseMode'), + }), + }; + const overridden: IWebhookDescription = { + ...base, + path: '={{$parameter["options"]["webhookSuffix"] || ""}}', + }; + + it('does not apply an inherited resolver to an overridden template', () => { + expect( + resolveWebhookDescriptionField( + { parameters: { options: { webhookSuffix: 'abc' } } }, + overridden, + 'path', + ), + ).toEqual({ resolved: false }); + }); + + it('still applies inherited resolvers whose templates are untouched', () => { + expect( + resolveWebhookDescriptionField( + { parameters: { responseMode: 'lastNode' } }, + overridden, + 'responseMode', + ), + ).toEqual({ resolved: true, value: 'lastNode' }); + }); + + it('is not fully resolvable once a template is overridden without a resolver', () => { + expect(webhookDescriptionIsNativelyResolvable(overridden)).toBe(false); + }); +}); + +describe('webhookDescriptionIsNativelyResolvable', () => { + it('accepts a description whose templates all have resolvers', () => { + const description: IWebhookDescription = { + name: 'default', + isFullPath: true, + httpMethod: 'GET', + ...webhookDescriptionFields({ path: fromParameter('path') }), + }; + expect(webhookDescriptionIsNativelyResolvable(description)).toBe(true); + }); + + it('rejects a description with a template lacking a resolver', () => { + const description: IWebhookDescription = { + name: 'default', + path: '={{$parameter["path"]}}', + httpMethod: 'GET', + }; + expect(webhookDescriptionIsNativelyResolvable(description)).toBe(false); + }); +}); + +describe('nodeParametersAreStatic', () => { + test.each<[string, INodeParameters, boolean]>([ + ['plain values', { path: 'x', options: { a: 1 } }, true], + ['expression string', { path: '={{ $json.x }}' }, false], + ['nested expression string', { options: { a: '={{ 1 }}' } }, false], + ['expression in array', { list: ['a', '={{ 1 }}'] }, false], + [ + 'resource locator', + { target: { __rl: true, mode: 'id', value: 'x' } as unknown as INodeParameters }, + false, + ], + ])('%s', (_name, parameters, expected) => { + expect(nodeParametersAreStatic({ parameters })).toBe(expected); + }); +});