refactor, and get access control working for production webhooks as well

n8n-io · Nov 20, 2023 · 1991d6d · 1991d6d
1 parent 659523f
commit 1991d6d
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 130 deletions.
diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts
@@ -23,6 +23,7 @@ import type {
 	WorkflowExecuteMode,
 	INodeType,
 	IWebhookData,
+	IHttpRequestMethods,
 } from 'n8n-workflow';
 import {
 	NodeHelpers,
@@ -137,40 +138,14 @@ export class ActiveWorkflowRunner implements IWebhookManager {
 		response: express.Response,
 	): Promise<IResponseCallbackData> {
 		const httpMethod = request.method;
-		let path = request.params.path;
+		const path = request.params.path;
 
 		this.logger.debug(`Received webhook "${httpMethod}" for path "${path}"`);
 
 		// Reset request parameters
 		request.params = {} as WebhookRequest['params'];
 
-		// Remove trailing slash
-		if (path.endsWith('/')) {
-			path = path.slice(0, -1);
-		}
-
-		const webhook = await this.webhookService.findWebhook(httpMethod, path);
-
-		if (webhook === null) {
-			throw new ResponseHelper.NotFoundError(
-				webhookNotFoundErrorMessage(path, httpMethod),
-				WEBHOOK_PROD_UNREGISTERED_HINT,
-			);
-		}
-
-		if (webhook.isDynamic) {
-			const pathElements = path.split('/').slice(1);
-
-			// extracting params from path
-			// @ts-ignore
-			webhook.webhookPath.split('/').forEach((ele, index) => {
-				if (ele.startsWith(':')) {
-					// write params to req.params
-					// @ts-ignore
-					request.params[ele.slice(1)] = pathElements[index];
-				}
-			});
-		}
+		const webhook = await this.findWebhook(path, httpMethod);
 
 		const workflowData = await this.workflowRepository.findOne({
 			where: { id: webhook.workflowId },
@@ -235,13 +210,58 @@ export class ActiveWorkflowRunner implements IWebhookManager {
 		});
 	}
 
-	/**
-	 * Gets all request methods associated with a single webhook
-	 */
 	async getWebhookMethods(path: string) {
 		return this.webhookService.getWebhookMethods(path);
 	}
 
+	async findWebhookNode(path: string, httpMethod: IHttpRequestMethods) {
+		const webhook = await this.findWebhook(path, httpMethod);
+
+		const workflowData = await this.workflowRepository.findOne({
+			where: { id: webhook.workflowId },
+			select: ['nodes'],
+		});
+
+		const nodes = workflowData?.nodes;
+		return nodes?.find(
+			({ type, parameters }) =>
+				type === 'n8n-nodes-base.webhook' &&
+				parameters?.path === path &&
+				(parameters?.httpMethod ?? 'GET') === httpMethod,
+		);
+	}
+
+	private async findWebhook(path: string, httpMethod: IHttpRequestMethods) {
+		// Remove trailing slash
+		if (path.endsWith('/')) {
+			path = path.slice(0, -1);
+		}
+
+		const webhook = await this.webhookService.findWebhook(httpMethod, path);
+		if (webhook === null) {
+			throw new ResponseHelper.NotFoundError(
+				webhookNotFoundErrorMessage(path, httpMethod),
+				WEBHOOK_PROD_UNREGISTERED_HINT,
+			);
+		}
+
+		if (webhook.isDynamic) {
+			const pathElements = path.split('/').slice(1);
+
+			// extracting params from path
+			// @ts-ignore
+			webhook.webhookPath.split('/').forEach((ele, index) => {
+				if (ele.startsWith(':')) {
+					// write params to req.params
+					// @ts-ignore
+					request.params[ele.slice(1)] = pathElements[index];
+				}
+			});
+		}
+
+		return webhook;
+	}
+
 	/**
 	 * Returns the ids of the currently active workflows from memory.
 	 */

diff --git a/packages/cli/src/Interfaces.ts b/packages/cli/src/Interfaces.ts
@@ -23,6 +23,7 @@ import type {
 	INodeProperties,
 	IUserSettings,
 	IHttpRequestMethods,
+	INode,
 } from 'n8n-workflow';
 
 import type { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner';
@@ -267,12 +268,18 @@ export type WaitingWebhookRequest = WebhookRequest & {
 	params: WebhookRequest['path'] & { suffix?: string };
 };
 
+export interface WebhookAccessControlOptions {
+	allowedOrigins?: string;
+	preflightMaxAge?: number;
+}
+
 export interface IWebhookManager {
+	/** Gets all request methods associated with a webhook path*/
 	getWebhookMethods?: (path: string) => Promise<IHttpRequestMethods[]>;
-	getAccessControlOptions?: (
-		path: string,
-		httpMethod: IHttpRequestMethods,
-	) => Promise<IDataObject | null>;
+
+	/** Find the webhook node matching a path and method */
+	findWebhookNode?: (path: string, httpMethod: IHttpRequestMethods) => Promise<INode | undefined>;
+
 	executeWebhook(req: WebhookRequest, res: Response): Promise<IResponseCallbackData>;
 }
 

diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts
@@ -8,7 +8,6 @@ import type {
 	Workflow,
 	WorkflowActivateMode,
 	WorkflowExecuteMode,
-	IDataObject,
 } from 'n8n-workflow';
 
 import { ActiveWebhooks } from '@/ActiveWebhooks';
@@ -162,9 +161,6 @@ export class TestWebhooks implements IWebhookManager {
 		});
 	}
 
-	/**
-	 * Gets all request methods associated with a single test webhook
-	 */
 	async getWebhookMethods(path: string): Promise<IHttpRequestMethods[]> {
 		const webhookMethods = this.activeWebhooks.getWebhookMethods(path);
 		if (!webhookMethods.length) {
@@ -178,31 +174,18 @@ export class TestWebhooks implements IWebhookManager {
 		return webhookMethods;
 	}
 
-	/**
-	 * Gets all request methods associated with a single test webhook
-	 */
-	async getAccessControlOptions(path: string, httpMethod: string): Promise<IDataObject | null> {
-		const webhookWorkflow = Object.keys(this.testWebhookData).find(
+	async findWebhookNode(path: string, httpMethod: IHttpRequestMethods) {
+		const webhookKey = Object.keys(this.testWebhookData).find(
 			(key) => key.includes(path) && key.startsWith(httpMethod),
 		);
 
-		const nodes = webhookWorkflow ? this.testWebhookData[webhookWorkflow].workflow.nodes : {};
-
-		if (!Object.keys(nodes).length) {
-			return null;
-		}
-
-		const result = Object.values(nodes).find((node) => {
-			return (
-				node.type === 'n8n-nodes-base.webhook' &&
-				node.parameters?.path === path &&
-				node.parameters?.httpMethod === httpMethod
-			);
-		});
-
-		const { accessControl } = (result?.parameters?.options as IDataObject) || {};
-
-		return accessControl ? ((accessControl as IDataObject).values as IDataObject) : null;
+		const nodes = webhookKey ? this.testWebhookData[webhookKey].workflow.nodes : {};
+		return Object.values(nodes).find(
+			({ type, parameters }) =>
+				type === 'n8n-nodes-base.webhook' &&
+				parameters?.path === path &&
+				(parameters?.httpMethod ?? 'GET') === httpMethod,
+		);
 	}
 
 	/**

diff --git a/packages/cli/src/WebhookHelpers.ts b/packages/cli/src/WebhookHelpers.ts
@@ -47,6 +47,7 @@ import type {
 	IWebhookManager,
 	IWorkflowDb,
 	IWorkflowExecutionDataProcess,
+	WebhookAccessControlOptions,
 	WebhookCORSRequest,
 	WebhookRequest,
 } from '@/Interfaces';
@@ -98,29 +99,30 @@ export const webhookRequestHandler =
 					return ResponseHelper.sendErrorResponse(res, error as Error);
 				}
 			}
-			res.header('Access-Control-Allow-Origin', req.headers.origin);
-		}
 
-		if (method === 'OPTIONS') {
-			const controlRequestMethod = req.headers['access-control-request-method'];
+			const requestedMethod =
+				method === 'OPTIONS'
+					? (req.headers['access-control-request-method'] as IHttpRequestMethods)
+					: method;
+			if (webhookManager.findWebhookNode && requestedMethod) {
+				const webhookNode = await webhookManager.findWebhookNode(path, requestedMethod);
+				const { allowedOrigins, preflightMaxAge } =
+					(webhookNode?.parameters?.options as WebhookAccessControlOptions) ?? {};
 
-			if (webhookManager.getAccessControlOptions && controlRequestMethod) {
-				try {
-					const accessControlOptions = await webhookManager.getAccessControlOptions(
-						path,
-						controlRequestMethod as IHttpRequestMethods,
-					);
+				res.header(
+					'Access-Control-Allow-Origin',
+					!allowedOrigins || allowedOrigins === '*' ? req.headers.origin : allowedOrigins,
+				);
 
-					if (accessControlOptions) {
-						const { allowMethods, allowOrigin, maxAge } = accessControlOptions;
+				if (method === 'OPTIONS') {
+					res.header('Access-Control-Max-Age', String((preflightMaxAge ?? 60) * 1000));
 
-						res.header('Access-Control-Allow-Methods', (allowMethods as string) || '*');
-						res.header('Access-Control-Allow-Origin', (allowOrigin as string) || '*');
-						res.header('Access-Control-Max-Age', String((maxAge as number) * 1000) || '60000');
-					}
-				} catch (error) {}
+					res.header('Access-Control-Allow-Headers', req.headers['access-control-request-headers']);
+				}
 			}
+		}
 
+		if (method === 'OPTIONS') {
 			return ResponseHelper.sendSuccessResponse(res, {}, true, 204);
 		}
 

diff --git a/packages/nodes-base/nodes/Webhook/Webhook.node.ts b/packages/nodes-base/nodes/Webhook/Webhook.node.ts
@@ -99,7 +99,6 @@ export class Webhook extends Node {
 			ignoreBots: boolean;
 			rawBody: Buffer;
 			responseData?: string;
-			accessControl: IDataObject;
 		};
 		const req = context.getRequestObject();
 		const resp = context.getResponseObject();

diff --git a/packages/nodes-base/nodes/Webhook/description.ts b/packages/nodes-base/nodes/Webhook/description.ts
@@ -336,58 +336,21 @@ export const optionsProperty: INodeProperties = {
 			description: 'Name of the property to return the data of instead of the whole JSON',
 		},
 		{
-			displayName: 'Access Control',
-			name: 'accessControl',
-			placeholder: 'Add Options Header',
-			description: 'Add headers to the preflight response',
-			type: 'fixedCollection',
-			default: {
-				values: {
-					allowOrigin: '*',
-					allowMethods: '*',
-					// allowHeaders: '',
-					maxAge: 60,
-				},
+			displayName: 'Allowed Origin(s)',
+			name: 'allowedOrigins',
+			type: 'string',
+			default: '*',
+			description: 'The origin(s) to allow non-preflight requests from',
+		},
+		{
+			displayName: 'Pre-Flight Cache Max Age',
+			name: 'preflightMaxAge',
+			type: 'number',
+			typeOptions: {
+				minValue: 0,
 			},
-			options: [
-				{
-					name: 'values',
-					displayName: 'Values',
-					values: [
-						{
-							displayName: 'Origin',
-							name: 'allowOrigin',
-							type: 'string',
-							default: '',
-							description: 'The origin(s) to allow requests from',
-						},
-						{
-							displayName: 'Methods',
-							name: 'allowMethods',
-							type: 'string',
-							default: '',
-							description: 'The methods to allow',
-						},
-						// {
-						// 	displayName: 'Headers',
-						// 	name: 'allowHeaders',
-						// 	type: 'string',
-						// 	default: '',
-						// 	description: 'The headers to allow',
-						// },
-						{
-							displayName: 'Max Age',
-							name: 'maxAge',
-							type: 'number',
-							typeOptions: {
-								minValue: 0,
-							},
-							default: 0,
-							description: 'The max age to allow',
-						},
-					],
-				},
-			],
+			default: 60,
+			description: 'How long the results of a preflight request are cached (in seconds)',
 		},
 	],
 };