From e0ee51f44244716507d069b495558fe43b6961ba Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:39:10 +0000 Subject: [PATCH] refactor(core): Normalize webhook helper errors (no-changelog) (#39374) --- .../utils/src/errors/ensure-error.test.ts | 12 +-- .../@n8n/utils/src/errors/ensure-error.ts | 3 +- packages/@n8n/utils/src/result.test.ts | 2 +- .../__tests__/webhook-helpers.test.ts | 27 +++++++ packages/cli/src/webhooks/webhook-helpers.ts | 73 ++++++++++--------- 5 files changed, 73 insertions(+), 44 deletions(-) diff --git a/packages/@n8n/utils/src/errors/ensure-error.test.ts b/packages/@n8n/utils/src/errors/ensure-error.test.ts index 0bfa4348f34..b40d942d28c 100644 --- a/packages/@n8n/utils/src/errors/ensure-error.test.ts +++ b/packages/@n8n/utils/src/errors/ensure-error.test.ts @@ -6,22 +6,22 @@ describe('ensureError', () => { expect(ensureError(original)).toBe(original); }); - it('wraps a string in a new Error with it as the cause', () => { + it('wraps a string in a new Error cause', () => { const result = ensureError('something went wrong'); expect(result).toBeInstanceOf(Error); - expect(result.cause).toBe('something went wrong'); + expect(result.cause).toEqual(new Error('something went wrong')); }); - it('wraps an object in a new Error with it as the cause', () => { + it('wraps an object in a new Error cause', () => { const obj = { code: 42 }; const result = ensureError(obj); expect(result).toBeInstanceOf(Error); - expect(result.cause).toBe(obj); + expect(result.cause).toEqual(new Error('[object Object]')); }); - it('wraps undefined in a new Error with it as the cause', () => { + it('wraps undefined in a new Error cause', () => { const result = ensureError(undefined); expect(result).toBeInstanceOf(Error); - expect(result.cause).toBeUndefined(); + expect(result.cause).toEqual(new Error('undefined')); }); }); diff --git a/packages/@n8n/utils/src/errors/ensure-error.ts b/packages/@n8n/utils/src/errors/ensure-error.ts index 331692d6e84..49d12714cc2 100644 --- a/packages/@n8n/utils/src/errors/ensure-error.ts +++ b/packages/@n8n/utils/src/errors/ensure-error.ts @@ -4,6 +4,7 @@ export function ensureError(error: unknown): Error { ? error : new Error('Error that was not an instance of Error was thrown', { // We should never throw anything except something that derives from Error - cause: error, + // Wrap this into an error, so error-reporter can report it + cause: new Error(String(error)), }); } diff --git a/packages/@n8n/utils/src/result.test.ts b/packages/@n8n/utils/src/result.test.ts index cb10b0258ca..e07418acf22 100644 --- a/packages/@n8n/utils/src/result.test.ts +++ b/packages/@n8n/utils/src/result.test.ts @@ -35,7 +35,7 @@ describe('toResult', () => { expect(result.ok).toBe(false); if (!result.ok) { expect(result.error).toBeInstanceOf(Error); - expect(result.error.cause).toBe('string error'); + expect(result.error.cause).toEqual(new Error('string error')); } }); }); diff --git a/packages/cli/src/webhooks/__tests__/webhook-helpers.test.ts b/packages/cli/src/webhooks/__tests__/webhook-helpers.test.ts index 33fd1836e8d..a9440d4e27f 100644 --- a/packages/cli/src/webhooks/__tests__/webhook-helpers.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook-helpers.test.ts @@ -555,6 +555,33 @@ describe('setupResponseNodePromise', () => { expect(responseCallback).toHaveBeenCalledWith(error, {}); }); + test('should normalize non-Error rejections', async () => { + setupResponseNodePromise( + responsePromise, + res, + responseCallback, + workflowStartNode, + executionId, + workflow, + ); + + const rejection = 'Test rejection'; + responsePromise.reject(rejection as unknown as Error); + await new Promise(process.nextTick); + + const error = errorReporter.error.mock.calls[0][0]; + expect(error).toBeInstanceOf(Error); + expect(error).toMatchObject({ + message: 'Error that was not an instance of Error was thrown', + cause: new Error(rejection), + }); + expect(logger.error).toHaveBeenCalledWith( + `Error with Webhook-Response for execution "${executionId}": "Error that was not an instance of Error was thrown"`, + { executionId, workflowId }, + ); + expect(responseCallback).toHaveBeenCalledWith(error, {}); + }); + // When an execution ends without the Respond to Webhook node having run, // `ActiveExecutions.resolveExecutionResponsePromise` settles this promise with a // sentinel. The post-execute handler answers in that case, so this one must not. diff --git a/packages/cli/src/webhooks/webhook-helpers.ts b/packages/cli/src/webhooks/webhook-helpers.ts index 63ff41e52fe..d4b1d00719b 100644 --- a/packages/cli/src/webhooks/webhook-helpers.ts +++ b/packages/cli/src/webhooks/webhook-helpers.ts @@ -1,14 +1,9 @@ -/* eslint-disable @typescript-eslint/no-unsafe-argument */ - -/* eslint-disable @typescript-eslint/no-unsafe-assignment */ -/* eslint-disable id-denylist */ -/* eslint-disable @typescript-eslint/no-unsafe-member-access */ - import { Logger } from '@n8n/backend-common'; import { ExecutionsConfig, GlobalConfig } from '@n8n/config'; import type { Project } from '@n8n/db'; import { UserRepository } from '@n8n/db'; import { Container } from '@n8n/di'; +import { ensureError } from '@n8n/utils/errors/ensure-error'; import { createDeferredPromise, type IDeferredPromise } from '@n8n/utils/promise/deferred-promise'; import type express from 'express'; import merge from 'lodash/merge'; @@ -478,7 +473,8 @@ export function setupResponseNodePromise( process.nextTick(() => res.end()); }) - .catch(async (error) => { + .catch(async (e: unknown) => { + const error = ensureError(e); Container.get(ErrorReporter).error(error); Container.get(Logger).error( `Error with Webhook-Response for execution "${executionId}": "${error.message}"`, @@ -674,7 +670,7 @@ export async function executeWebhook( let project: Project; try { project = await Container.get(OwnershipService).getWorkflowProjectCached(workflowData.id); - } catch (error) { + } catch { throw new NotFoundError('Cannot find workflow'); } @@ -914,14 +910,15 @@ export async function executeWebhook( workflowId: workflow.id, node: workflowStartNode, }); - } catch (err) { + } catch (e: unknown) { + const error = ensureError(e); // Send error response to webhook caller const webhookType = ['formTrigger', 'form'].includes(nodeType.description.name) ? 'Form' : 'Webhook'; - const errorMessage = _privateGetWebhookErrorMessage(err, webhookType); + const errorMessage = _privateGetWebhookErrorMessage(error, webhookType); - Container.get(ErrorReporter).error(err, { + Container.get(ErrorReporter).error(error, { extra: { nodeName: workflowStartNode.name, nodeType: workflowStartNode.type, @@ -939,9 +936,9 @@ export async function executeWebhook( runData: {}, lastNodeExecuted: workflowStartNode.name, error: { - ...err, - message: err.message, - stack: err.stack, + ...error, + message: error.message, + stack: error.stack, }, }, }; @@ -982,7 +979,7 @@ export async function executeWebhook( // Data to respond with is given if (!didSendResponse) { responseCallback(null, { - data: webhookResultData.webhookResponse, + data: webhookResultData.webhookResponse as IDataObject | IDataObject[], responseCode, }); didSendResponse = true; @@ -1358,41 +1355,45 @@ export async function executeWebhook( didSendResponse = true; return runData; }) - .catch((e) => { - Container.get(ErrorReporter).error(e, { executionId }); + .catch((e: unknown) => { + const error = ensureError(e); + Container.get(ErrorReporter).error(error, { executionId }); if (!didSendResponse) { responseCallback( new OperationalError('There was a problem executing the workflow', { - cause: e, + cause: error, }), {}, ); } - const internalServerError = new InternalServerError(e.message, e); - if (e instanceof ExecutionCancelledError) internalServerError.level = 'warning'; + const internalServerError = new InternalServerError(error.message, error); + if (error instanceof ExecutionCancelledError) internalServerError.level = 'warning'; throw internalServerError; }); } return executionId; - } catch (e) { + } catch (e: unknown) { // Nothing will ever answer this one, so stop waiting for it. pendingEngineV2Response?.release(); - let error: Error; - if (e instanceof ResponseError && e.httpStatusCode < 500) { - error = e; - } else if (routesToEngineV2 && e instanceof UserError) { + const error = ensureError(e); + let responseError: Error; + if (error instanceof ResponseError && error.httpStatusCode < 500) { + responseError = error; + } else if (routesToEngineV2 && error instanceof UserError) { // The v2 path never falls back to v1, so its reason is the answer. The // branch below would replace it with a generic 500 and report it as a bug. - error = new BadRequestError(e.message); + responseError = new BadRequestError(error.message); } else { - Container.get(ErrorReporter).error(e, { executionId }); - error = new OperationalError('There was a problem executing the workflow', { cause: e }); + Container.get(ErrorReporter).error(error, { executionId }); + responseError = new OperationalError('There was a problem executing the workflow', { + cause: error, + }); } - if (didSendResponse) throw error; - responseCallback(error, {}); + if (didSendResponse) throw responseError; + responseCallback(responseError, {}); return; } finally { await cleanupMultipartFiles?.(); @@ -1550,19 +1551,19 @@ function evaluateResponseHeaders(context: WebhookExecutionContext): WebhookRespo * * ONLY EXPORTED FOR TESTING. * - * @param err the error being handled + * @param error the error being handled */ export function _privateGetWebhookErrorMessage( - err: unknown, + error: unknown, webhookType: 'Form' | 'Webhook', ): string { // if workflow started manually, show an actual error message - if (err instanceof NodeOperationError && err.type === 'manual-form-test') { - return err.message; + if (error instanceof NodeOperationError && error.type === 'manual-form-test') { + return error.message; } // if the error relates to a configuration error on the workflow, surface it - if (err instanceof WorkflowConfigurationError) { - return err.message; + if (error instanceof WorkflowConfigurationError) { + return error.message; } return `Workflow ${webhookType} Error: Workflow could not be started!`; }