refactor(core): Normalize webhook helper errors (no-changelog) (#39374)

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'));
 	});
 });

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)),
 			});
 }

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'));
 		}
 	});
 });

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.

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!`;
 }