refactor(core): Extract webhook request handler to own file (#10301)

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

packages/cli/src/AbstractServer.ts

@@ -16,7 +16,7 @@ import { rawBodyReader, bodyParser, corsMiddleware } from '@/middlewares';
 import { WaitingForms } from '@/WaitingForms';
 import { TestWebhooks } from '@/webhooks/TestWebhooks';
 import { WaitingWebhooks } from '@/webhooks/WaitingWebhooks';
-import { webhookRequestHandler } from '@/webhooks/WebhookHelpers';
+import { createWebhookHandlerFor } from '@/webhooks/WebhookRequestHandler';
 import { ActiveWebhooks } from '@/webhooks/ActiveWebhooks';
 import { generateHostInstanceId } from './databases/utils/generators';
 import { Logger } from '@/Logger';
@@ -181,33 +181,32 @@ export abstract class AbstractServer {
 
 		// Setup webhook handlers before bodyParser, to let the Webhook node handle binary data in requests
 		if (this.webhooksEnabled) {
-			const activeWebhooks = Container.get(ActiveWebhooks);
+			const activeWebhooksRequestHandler = createWebhookHandlerFor(Container.get(ActiveWebhooks));
-
 			// Register a handler for active forms
-			this.app.all(`/${this.endpointForm}/:path(*)`, webhookRequestHandler(activeWebhooks));
+			this.app.all(`/${this.endpointForm}/:path(*)`, activeWebhooksRequestHandler);
 
 			// Register a handler for active webhooks
-			this.app.all(`/${this.endpointWebhook}/:path(*)`, webhookRequestHandler(activeWebhooks));
+			this.app.all(`/${this.endpointWebhook}/:path(*)`, activeWebhooksRequestHandler);
 
 			// Register a handler for waiting forms
 			this.app.all(
 				`/${this.endpointFormWaiting}/:path/:suffix?`,
-				webhookRequestHandler(Container.get(WaitingForms)),
+				createWebhookHandlerFor(Container.get(WaitingForms)),
 			);
 
 			// Register a handler for waiting webhooks
 			this.app.all(
 				`/${this.endpointWebhookWaiting}/:path/:suffix?`,
-				webhookRequestHandler(Container.get(WaitingWebhooks)),
+				createWebhookHandlerFor(Container.get(WaitingWebhooks)),
 			);
 		}
 
 		if (this.testWebhooksEnabled) {
-			const testWebhooks = Container.get(TestWebhooks);
+			const testWebhooksRequestHandler = createWebhookHandlerFor(Container.get(TestWebhooks));
 
 			// Register a handler
-			this.app.all(`/${this.endpointFormTest}/:path(*)`, webhookRequestHandler(testWebhooks));
+			this.app.all(`/${this.endpointFormTest}/:path(*)`, testWebhooksRequestHandler);
-			this.app.all(`/${this.endpointWebhookTest}/:path(*)`, webhookRequestHandler(testWebhooks));
+			this.app.all(`/${this.endpointWebhookTest}/:path(*)`, testWebhooksRequestHandler);
 		}
 
 		// Block bots from scanning the application

packages/cli/src/webhooks/WebhookHelpers.ts

@@ -20,7 +20,6 @@ import type {
 	IDataObject,
 	IDeferredPromise,
 	IExecuteData,
-	IHttpRequestMethods,
 	IN8nHttpFullResponse,
 	INode,
 	IPinData,
@@ -41,13 +40,7 @@ import {
 	NodeHelpers,
 } from 'n8n-workflow';
 
-import type {
+import type { IWebhookResponseCallbackData, WebhookRequest } from './webhook.types';
-	IWebhookResponseCallbackData,
-	IWebhookManager,
-	WebhookCORSRequest,
-	WebhookRequest,
-} from './webhook.types';
-import * as ResponseHelper from '@/ResponseHelper';
 import * as WorkflowHelpers from '@/WorkflowHelpers';
 import { WorkflowRunner } from '@/WorkflowRunner';
 import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData';
@@ -62,97 +55,6 @@ import { UnprocessableRequestError } from '@/errors/response-errors/unprocessabl
 import type { Project } from '@/databases/entities/Project';
 import type { IExecutionDb, IWorkflowDb, IWorkflowExecutionDataProcess } from '@/Interfaces';
 
-export const WEBHOOK_METHODS: IHttpRequestMethods[] = [
-	'DELETE',
-	'GET',
-	'HEAD',
-	'PATCH',
-	'POST',
-	'PUT',
-];
-
-export const webhookRequestHandler =
-	(webhookManager: IWebhookManager) =>
-	async (req: WebhookRequest | WebhookCORSRequest, res: express.Response) => {
-		const { path } = req.params;
-		const method = req.method;
-
-		if (method !== 'OPTIONS' && !WEBHOOK_METHODS.includes(method)) {
-			return ResponseHelper.sendErrorResponse(
-				res,
-				new Error(`The method ${method} is not supported.`),
-			);
-		}
-
-		// Setup CORS headers only if the incoming request has an `origin` header
-		if ('origin' in req.headers) {
-			if (webhookManager.getWebhookMethods) {
-				try {
-					const allowedMethods = await webhookManager.getWebhookMethods(path);
-					res.header('Access-Control-Allow-Methods', ['OPTIONS', ...allowedMethods].join(', '));
-				} catch (error) {
-					return ResponseHelper.sendErrorResponse(res, error as Error);
-				}
-			}
-
-			const requestedMethod =
-				method === 'OPTIONS'
-					? (req.headers['access-control-request-method'] as IHttpRequestMethods)
-					: method;
-			if (webhookManager.findAccessControlOptions && requestedMethod) {
-				const options = await webhookManager.findAccessControlOptions(path, requestedMethod);
-				const { allowedOrigins } = options ?? {};
-
-				if (allowedOrigins && allowedOrigins !== '*' && allowedOrigins !== req.headers.origin) {
-					const originsList = allowedOrigins.split(',');
-					const defaultOrigin = originsList[0];
-
-					if (originsList.length === 1) {
-						res.header('Access-Control-Allow-Origin', defaultOrigin);
-					}
-
-					if (originsList.includes(req.headers.origin as string)) {
-						res.header('Access-Control-Allow-Origin', req.headers.origin);
-					} else {
-						res.header('Access-Control-Allow-Origin', defaultOrigin);
-					}
-				} else {
-					res.header('Access-Control-Allow-Origin', req.headers.origin);
-				}
-
-				if (method === 'OPTIONS') {
-					res.header('Access-Control-Max-Age', '300');
-					const requestedHeaders = req.headers['access-control-request-headers'];
-					if (requestedHeaders?.length) {
-						res.header('Access-Control-Allow-Headers', requestedHeaders);
-					}
-				}
-			}
-		}
-
-		if (method === 'OPTIONS') {
-			return ResponseHelper.sendSuccessResponse(res, {}, true, 204);
-		}
-
-		let response: IWebhookResponseCallbackData;
-		try {
-			response = await webhookManager.executeWebhook(req, res);
-		} catch (error) {
-			return ResponseHelper.sendErrorResponse(res, error as Error);
-		}
-
-		// Don't respond, if already responded
-		if (response.noWebhookResponse !== true) {
-			ResponseHelper.sendSuccessResponse(
-				res,
-				response.data,
-				true,
-				response.responseCode,
-				response.headers,
-			);
-		}
-	};
-
 /**
  * Returns all the webhooks which should be created for the given workflow
  */

packages/cli/src/webhooks/WebhookRequestHandler.ts

@@ -0,0 +1,119 @@
+import type express from 'express';
+import type { IHttpRequestMethods } from 'n8n-workflow';
+import type {
+	IWebhookManager,
+	WebhookOptionsRequest,
+	WebhookRequest,
+} from '@/webhooks/webhook.types';
+import * as ResponseHelper from '@/ResponseHelper';
+
+const WEBHOOK_METHODS: IHttpRequestMethods[] = ['DELETE', 'GET', 'HEAD', 'PATCH', 'POST', 'PUT'];
+
+class WebhookRequestHandler {
+	constructor(private readonly webhookManager: IWebhookManager) {}
+
+	/**
+	 * Handles an incoming webhook request. Handles CORS and delegates the
+	 * request to the webhook manager to execute the webhook.
+	 */
+	async handleRequest(req: WebhookRequest | WebhookOptionsRequest, res: express.Response) {
+		const method = req.method;
+
+		if (method !== 'OPTIONS' && !WEBHOOK_METHODS.includes(method)) {
+			return ResponseHelper.sendErrorResponse(
+				res,
+				new Error(`The method ${method} is not supported.`),
+			);
+		}
+
+		// Setup CORS headers only if the incoming request has an `origin` header
+		if ('origin' in req.headers) {
+			const corsSetupError = await this.setupCorsHeaders(req, res);
+			if (corsSetupError) {
+				return ResponseHelper.sendErrorResponse(res, corsSetupError);
+			}
+		}
+
+		if (method === 'OPTIONS') {
+			return ResponseHelper.sendSuccessResponse(res, {}, true, 204);
+		}
+
+		try {
+			const response = await this.webhookManager.executeWebhook(req, res);
+
+			// Don't respond, if already responded
+			if (response.noWebhookResponse !== true) {
+				ResponseHelper.sendSuccessResponse(
+					res,
+					response.data,
+					true,
+					response.responseCode,
+					response.headers,
+				);
+			}
+		} catch (error) {
+			return ResponseHelper.sendErrorResponse(res, error as Error);
+		}
+	}
+
+	private async setupCorsHeaders(
+		req: WebhookRequest | WebhookOptionsRequest,
+		res: express.Response,
+	): Promise<Error | null> {
+		const method = req.method;
+		const { path } = req.params;
+
+		if (this.webhookManager.getWebhookMethods) {
+			try {
+				const allowedMethods = await this.webhookManager.getWebhookMethods(path);
+				res.header('Access-Control-Allow-Methods', ['OPTIONS', ...allowedMethods].join(', '));
+			} catch (error) {
+				return error as Error;
+			}
+		}
+
+		const requestedMethod =
+			method === 'OPTIONS'
+				? (req.headers['access-control-request-method'] as IHttpRequestMethods)
+				: method;
+		if (this.webhookManager.findAccessControlOptions && requestedMethod) {
+			const options = await this.webhookManager.findAccessControlOptions(path, requestedMethod);
+			const { allowedOrigins } = options ?? {};
+
+			if (allowedOrigins && allowedOrigins !== '*' && allowedOrigins !== req.headers.origin) {
+				const originsList = allowedOrigins.split(',');
+				const defaultOrigin = originsList[0];
+
+				if (originsList.length === 1) {
+					res.header('Access-Control-Allow-Origin', defaultOrigin);
+				}
+
+				if (originsList.includes(req.headers.origin as string)) {
+					res.header('Access-Control-Allow-Origin', req.headers.origin);
+				} else {
+					res.header('Access-Control-Allow-Origin', defaultOrigin);
+				}
+			} else {
+				res.header('Access-Control-Allow-Origin', req.headers.origin);
+			}
+
+			if (method === 'OPTIONS') {
+				res.header('Access-Control-Max-Age', '300');
+				const requestedHeaders = req.headers['access-control-request-headers'];
+				if (requestedHeaders?.length) {
+					res.header('Access-Control-Allow-Headers', requestedHeaders);
+				}
+			}
+		}
+
+		return null;
+	}
+}
+
+export function createWebhookHandlerFor(webhookManager: IWebhookManager) {
+	const handler = new WebhookRequestHandler(webhookManager);
+
+	return async (req: WebhookRequest | WebhookOptionsRequest, res: express.Response) => {
+		await handler.handleRequest(req, res);
+	};
+}

packages/cli/src/webhooks/__tests__/WebhookHelpers.test.ts

@@ -1,142 +0,0 @@
-import { type Response } from 'express';
-import { mock } from 'jest-mock-extended';
-import { randomString } from 'n8n-workflow';
-import type { IHttpRequestMethods } from 'n8n-workflow';
-
-import type { IWebhookManager, WebhookCORSRequest, WebhookRequest } from '@/webhooks/webhook.types';
-import { webhookRequestHandler } from '@/webhooks/WebhookHelpers';
-
-describe('WebhookHelpers', () => {
-	describe('webhookRequestHandler', () => {
-		const webhookManager = mock<Required<IWebhookManager>>();
-		const handler = webhookRequestHandler(webhookManager);
-
-		beforeEach(() => {
-			jest.resetAllMocks();
-		});
-
-		it('should throw for unsupported methods', async () => {
-			const req = mock<WebhookRequest | WebhookCORSRequest>({
-				method: 'CONNECT' as IHttpRequestMethods,
-			});
-			const res = mock<Response>();
-			res.status.mockReturnValue(res);
-
-			await handler(req, res);
-
-			expect(res.status).toHaveBeenCalledWith(500);
-			expect(res.json).toHaveBeenCalledWith({
-				code: 0,
-				message: 'The method CONNECT is not supported.',
-			});
-		});
-
-		describe('preflight requests', () => {
-			it('should handle missing header for requested method', async () => {
-				const req = mock<WebhookRequest | WebhookCORSRequest>({
-					method: 'OPTIONS',
-					headers: {
-						origin: 'https://example.com',
-						'access-control-request-method': undefined,
-					},
-					params: { path: 'test' },
-				});
-				const res = mock<Response>();
-				res.status.mockReturnValue(res);
-
-				webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
-
-				await handler(req, res);
-
-				expect(res.status).toHaveBeenCalledWith(204);
-				expect(res.header).toHaveBeenCalledWith(
-					'Access-Control-Allow-Methods',
-					'OPTIONS, GET, PATCH',
-				);
-			});
-
-			it('should handle default origin and max-age', async () => {
-				const req = mock<WebhookRequest | WebhookCORSRequest>({
-					method: 'OPTIONS',
-					headers: {
-						origin: 'https://example.com',
-						'access-control-request-method': 'GET',
-					},
-					params: { path: 'test' },
-				});
-				const res = mock<Response>();
-				res.status.mockReturnValue(res);
-
-				webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
-
-				await handler(req, res);
-
-				expect(res.status).toHaveBeenCalledWith(204);
-				expect(res.header).toHaveBeenCalledWith(
-					'Access-Control-Allow-Methods',
-					'OPTIONS, GET, PATCH',
-				);
-				expect(res.header).toHaveBeenCalledWith(
-					'Access-Control-Allow-Origin',
-					'https://example.com',
-				);
-				expect(res.header).toHaveBeenCalledWith('Access-Control-Max-Age', '300');
-			});
-
-			it('should handle wildcard origin', async () => {
-				const randomOrigin = randomString(10);
-				const req = mock<WebhookRequest | WebhookCORSRequest>({
-					method: 'OPTIONS',
-					headers: {
-						origin: randomOrigin,
-						'access-control-request-method': 'GET',
-					},
-					params: { path: 'test' },
-				});
-				const res = mock<Response>();
-				res.status.mockReturnValue(res);
-
-				webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
-				webhookManager.findAccessControlOptions.mockResolvedValue({
-					allowedOrigins: '*',
-				});
-
-				await handler(req, res);
-
-				expect(res.status).toHaveBeenCalledWith(204);
-				expect(res.header).toHaveBeenCalledWith(
-					'Access-Control-Allow-Methods',
-					'OPTIONS, GET, PATCH',
-				);
-				expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Origin', randomOrigin);
-			});
-
-			it('should handle custom origin', async () => {
-				const req = mock<WebhookRequest | WebhookCORSRequest>({
-					method: 'OPTIONS',
-					headers: {
-						origin: 'https://example.com',
-						'access-control-request-method': 'GET',
-					},
-					params: { path: 'test' },
-				});
-				const res = mock<Response>();
-				res.status.mockReturnValue(res);
-
-				webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
-				webhookManager.findAccessControlOptions.mockResolvedValue({
-					allowedOrigins: 'https://test.com',
-				});
-
-				await handler(req, res);
-
-				expect(res.status).toHaveBeenCalledWith(204);
-				expect(res.header).toHaveBeenCalledWith(
-					'Access-Control-Allow-Methods',
-					'OPTIONS, GET, PATCH',
-				);
-				expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Origin', 'https://test.com');
-			});
-		});
-	});
-});

packages/cli/src/webhooks/__tests__/WebhookRequestHandler.test.ts

@@ -0,0 +1,220 @@
+import { type Response } from 'express';
+import { mock } from 'jest-mock-extended';
+import { randomString } from 'n8n-workflow';
+import type { IHttpRequestMethods } from 'n8n-workflow';
+
+import type {
+	IWebhookManager,
+	IWebhookResponseCallbackData,
+	WebhookOptionsRequest,
+	WebhookRequest,
+} from '@/webhooks/webhook.types';
+import { createWebhookHandlerFor } from '@/webhooks/WebhookRequestHandler';
+import { ResponseError } from '@/errors/response-errors/abstract/response.error';
+
+describe('WebhookRequestHandler', () => {
+	const webhookManager = mock<Required<IWebhookManager>>();
+	const handler = createWebhookHandlerFor(webhookManager);
+
+	beforeEach(() => {
+		jest.resetAllMocks();
+	});
+
+	it('should throw for unsupported methods', async () => {
+		const req = mock<WebhookRequest | WebhookOptionsRequest>({
+			method: 'CONNECT' as IHttpRequestMethods,
+		});
+		const res = mock<Response>();
+		res.status.mockReturnValue(res);
+
+		await handler(req, res);
+
+		expect(res.status).toHaveBeenCalledWith(500);
+		expect(res.json).toHaveBeenCalledWith({
+			code: 0,
+			message: 'The method CONNECT is not supported.',
+		});
+	});
+
+	describe('preflight requests', () => {
+		it('should handle missing header for requested method', async () => {
+			const req = mock<WebhookRequest | WebhookOptionsRequest>({
+				method: 'OPTIONS',
+				headers: {
+					origin: 'https://example.com',
+					'access-control-request-method': undefined,
+				},
+				params: { path: 'test' },
+			});
+			const res = mock<Response>();
+			res.status.mockReturnValue(res);
+
+			webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
+
+			await handler(req, res);
+
+			expect(res.status).toHaveBeenCalledWith(204);
+			expect(res.header).toHaveBeenCalledWith(
+				'Access-Control-Allow-Methods',
+				'OPTIONS, GET, PATCH',
+			);
+		});
+
+		it('should handle default origin and max-age', async () => {
+			const req = mock<WebhookRequest | WebhookOptionsRequest>({
+				method: 'OPTIONS',
+				headers: {
+					origin: 'https://example.com',
+					'access-control-request-method': 'GET',
+				},
+				params: { path: 'test' },
+			});
+			const res = mock<Response>();
+			res.status.mockReturnValue(res);
+
+			webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
+
+			await handler(req, res);
+
+			expect(res.status).toHaveBeenCalledWith(204);
+			expect(res.header).toHaveBeenCalledWith(
+				'Access-Control-Allow-Methods',
+				'OPTIONS, GET, PATCH',
+			);
+			expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Origin', 'https://example.com');
+			expect(res.header).toHaveBeenCalledWith('Access-Control-Max-Age', '300');
+		});
+
+		it('should handle wildcard origin', async () => {
+			const randomOrigin = randomString(10);
+			const req = mock<WebhookRequest | WebhookOptionsRequest>({
+				method: 'OPTIONS',
+				headers: {
+					origin: randomOrigin,
+					'access-control-request-method': 'GET',
+				},
+				params: { path: 'test' },
+			});
+			const res = mock<Response>();
+			res.status.mockReturnValue(res);
+
+			webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
+			webhookManager.findAccessControlOptions.mockResolvedValue({
+				allowedOrigins: '*',
+			});
+
+			await handler(req, res);
+
+			expect(res.status).toHaveBeenCalledWith(204);
+			expect(res.header).toHaveBeenCalledWith(
+				'Access-Control-Allow-Methods',
+				'OPTIONS, GET, PATCH',
+			);
+			expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Origin', randomOrigin);
+		});
+
+		it('should handle custom origin', async () => {
+			const req = mock<WebhookRequest | WebhookOptionsRequest>({
+				method: 'OPTIONS',
+				headers: {
+					origin: 'https://example.com',
+					'access-control-request-method': 'GET',
+				},
+				params: { path: 'test' },
+			});
+			const res = mock<Response>();
+			res.status.mockReturnValue(res);
+
+			webhookManager.getWebhookMethods.mockResolvedValue(['GET', 'PATCH']);
+			webhookManager.findAccessControlOptions.mockResolvedValue({
+				allowedOrigins: 'https://test.com',
+			});
+
+			await handler(req, res);
+
+			expect(res.status).toHaveBeenCalledWith(204);
+			expect(res.header).toHaveBeenCalledWith(
+				'Access-Control-Allow-Methods',
+				'OPTIONS, GET, PATCH',
+			);
+			expect(res.header).toHaveBeenCalledWith('Access-Control-Allow-Origin', 'https://test.com');
+		});
+	});
+
+	describe('webhook requests', () => {
+		it('should delegate the request to the webhook manager and send the response', async () => {
+			const req = mock<WebhookRequest>({
+				method: 'GET',
+				params: { path: 'test' },
+			});
+
+			const res = mock<Response>();
+
+			const executeWebhookResponse: IWebhookResponseCallbackData = {
+				responseCode: 200,
+				data: {},
+				headers: {
+					'x-custom-header': 'test',
+				},
+			};
+			webhookManager.executeWebhook.mockResolvedValueOnce(executeWebhookResponse);
+
+			await handler(req, res);
+
+			expect(webhookManager.executeWebhook).toHaveBeenCalledWith(req, res);
+			expect(res.status).toHaveBeenCalledWith(200);
+			expect(res.header).toHaveBeenCalledWith({
+				'x-custom-header': 'test',
+			});
+			expect(res.json).toHaveBeenCalledWith(executeWebhookResponse.data);
+		});
+
+		it('should send an error response if webhook execution throws', async () => {
+			class TestError extends ResponseError {}
+			const req = mock<WebhookRequest>({
+				method: 'GET',
+				params: { path: 'test' },
+			});
+
+			const res = mock<Response>();
+			res.status.mockReturnValue(res);
+
+			webhookManager.executeWebhook.mockRejectedValueOnce(
+				new TestError('Test error', 500, 100, 'Test hint'),
+			);
+
+			await handler(req, res);
+
+			expect(webhookManager.executeWebhook).toHaveBeenCalledWith(req, res);
+			expect(res.status).toHaveBeenCalledWith(500);
+			expect(res.json).toHaveBeenCalledWith({
+				code: 100,
+				message: 'Test error',
+				hint: 'Test hint',
+			});
+		});
+
+		test.each<IHttpRequestMethods>(['DELETE', 'GET', 'HEAD', 'PATCH', 'POST', 'PUT'])(
+			"should handle '%s' method",
+			async (method) => {
+				const req = mock<WebhookRequest>({
+					method,
+					params: { path: 'test' },
+				});
+
+				const res = mock<Response>();
+
+				const executeWebhookResponse: IWebhookResponseCallbackData = {
+					responseCode: 200,
+				};
+				webhookManager.executeWebhook.mockResolvedValueOnce(executeWebhookResponse);
+
+				await handler(req, res);
+
+				expect(webhookManager.executeWebhook).toHaveBeenCalledWith(req, res);
+				expect(res.status).toHaveBeenCalledWith(200);
+				expect(res.json).toHaveBeenCalledWith(executeWebhookResponse.data);
+			},
+		);
+	});
+});

packages/cli/src/webhooks/webhook.types.ts

@@ -1,7 +1,7 @@
 import type { Request, Response } from 'express';
 import type { IDataObject, IHttpRequestMethods } from 'n8n-workflow';
 
-export type WebhookCORSRequest = Request & { method: 'OPTIONS' };
+export type WebhookOptionsRequest = Request & { method: 'OPTIONS' };
 
 export type WebhookRequest = Request<{ path: string }> & {
 	method: IHttpRequestMethods;