From 99a9ea497a3d21739f911da5c88c076f60471bed Mon Sep 17 00:00:00 2001 From: Michael Kret <88898367+michael-radency@users.noreply.github.com> Date: Wed, 22 Nov 2023 18:49:56 +0200 Subject: [PATCH] feat(core): Add Support for custom CORS origins for webhooks (#7455) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit node-850 https://community.n8n.io/t/add-ability-to-set-cors-allow-list-in-n8n-webhooks/7610 https://community.n8n.io/t/configure-cors-pre-flight-request-option-method-in-the-roadmap/32189 --------- Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- packages/cli/src/ActiveWorkflowRunner.ts | 56 ++++--- packages/cli/src/Interfaces.ts | 12 ++ packages/cli/src/TestWebhooks.ts | 26 +++- packages/cli/src/WebhookHelpers.ts | 23 ++- packages/cli/test/unit/WebhookHelpers.test.ts | 140 ++++++++++++++++++ packages/cli/test/unit/webhooks.test.ts | 10 +- .../nodes-base/nodes/Webhook/Webhook.node.ts | 1 + packages/workflow/src/Interfaces.ts | 3 +- packages/workflow/src/NodeHelpers.ts | 22 ++- 9 files changed, 264 insertions(+), 29 deletions(-) create mode 100644 packages/cli/test/unit/WebhookHelpers.test.ts diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index a32ce7aa3d..de684c9d2c 100644 --- 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, @@ -39,6 +40,7 @@ import type { IWebhookManager, IWorkflowDb, IWorkflowExecutionDataProcess, + WebhookAccessControlOptions, WebhookRequest, } from '@/Interfaces'; import * as ResponseHelper from '@/ResponseHelper'; @@ -137,26 +139,14 @@ export class ActiveWorkflowRunner implements IWebhookManager { response: express.Response, ): Promise { 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, - ); - } + const webhook = await this.findWebhook(path, httpMethod); if (webhook.isDynamic) { const pathElements = path.split('/').slice(1); @@ -235,13 +225,45 @@ export class ActiveWorkflowRunner implements IWebhookManager { }); } - /** - * Gets all request methods associated with a single webhook - */ async getWebhookMethods(path: string) { return this.webhookService.getWebhookMethods(path); } + async findAccessControlOptions(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; + const webhookNode = nodes?.find( + ({ type, parameters, typeVersion }) => + parameters?.path === path && + (parameters?.httpMethod ?? 'GET') === httpMethod && + 'webhook' in this.nodeTypes.getByNameAndVersion(type, typeVersion), + ); + return webhookNode?.parameters?.options as WebhookAccessControlOptions; + } + + 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, + ); + } + + 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 index b47b1a91c7..1caadbee57 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -267,8 +267,20 @@ export type WaitingWebhookRequest = WebhookRequest & { params: WebhookRequest['path'] & { suffix?: string }; }; +export interface WebhookAccessControlOptions { + allowedOrigins?: string; +} + export interface IWebhookManager { + /** Gets all request methods associated with a webhook path*/ getWebhookMethods?: (path: string) => Promise; + + /** Find the CORS options matching a path and method */ + findAccessControlOptions?: ( + path: string, + httpMethod: IHttpRequestMethods, + ) => Promise; + executeWebhook(req: WebhookRequest, res: Response): Promise; } diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts index 4cd5d99809..a31e37ea6b 100644 --- a/packages/cli/src/TestWebhooks.ts +++ b/packages/cli/src/TestWebhooks.ts @@ -15,9 +15,11 @@ import type { IResponseCallbackData, IWebhookManager, IWorkflowDb, + WebhookAccessControlOptions, WebhookRequest, } from '@/Interfaces'; import { Push } from '@/push'; +import { NodeTypes } from '@/NodeTypes'; import * as ResponseHelper from '@/ResponseHelper'; import * as WebhookHelpers from '@/WebhookHelpers'; import { webhookNotFoundErrorMessage } from './utils'; @@ -38,8 +40,9 @@ export class TestWebhooks implements IWebhookManager { } = {}; constructor( - private activeWebhooks: ActiveWebhooks, - private push: Push, + private readonly activeWebhooks: ActiveWebhooks, + private readonly push: Push, + private readonly nodeTypes: NodeTypes, ) { activeWebhooks.testWebhooks = true; } @@ -161,9 +164,6 @@ export class TestWebhooks implements IWebhookManager { }); } - /** - * Gets all request methods associated with a single test webhook - */ async getWebhookMethods(path: string): Promise { const webhookMethods = this.activeWebhooks.getWebhookMethods(path); if (!webhookMethods.length) { @@ -177,6 +177,22 @@ export class TestWebhooks implements IWebhookManager { return webhookMethods; } + async findAccessControlOptions(path: string, httpMethod: IHttpRequestMethods) { + const webhookKey = Object.keys(this.testWebhookData).find( + (key) => key.includes(path) && key.startsWith(httpMethod), + ); + if (!webhookKey) return; + + const { workflow } = this.testWebhookData[webhookKey]; + const webhookNode = Object.values(workflow.nodes).find( + ({ type, parameters, typeVersion }) => + parameters?.path === path && + (parameters?.httpMethod ?? 'GET') === httpMethod && + 'webhook' in this.nodeTypes.getByNameAndVersion(type, typeVersion), + ); + return webhookNode?.parameters?.options as WebhookAccessControlOptions; + } + /** * Checks if it has to wait for webhook data to execute the workflow. * If yes it waits for it and resolves with the result of the workflow if not it simply resolves with undefined diff --git a/packages/cli/src/WebhookHelpers.ts b/packages/cli/src/WebhookHelpers.ts index 6a200e694f..ae87e130e9 100644 --- a/packages/cli/src/WebhookHelpers.ts +++ b/packages/cli/src/WebhookHelpers.ts @@ -98,7 +98,28 @@ export const webhookRequestHandler = return ResponseHelper.sendErrorResponse(res, error as Error); } } - res.header('Access-Control-Allow-Origin', req.headers.origin); + + 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 ?? {}; + + res.header( + 'Access-Control-Allow-Origin', + !allowedOrigins || allowedOrigins === '*' ? req.headers.origin : allowedOrigins, + ); + + 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') { diff --git a/packages/cli/test/unit/WebhookHelpers.test.ts b/packages/cli/test/unit/WebhookHelpers.test.ts new file mode 100644 index 0000000000..fb455b098a --- /dev/null +++ b/packages/cli/test/unit/WebhookHelpers.test.ts @@ -0,0 +1,140 @@ +import { type Response } from 'express'; +import { mock } from 'jest-mock-extended'; +import type { IHttpRequestMethods } from 'n8n-workflow'; +import type { IWebhookManager, WebhookCORSRequest, WebhookRequest } from '@/Interfaces'; +import { webhookRequestHandler } from '@/WebhookHelpers'; + +describe('WebhookHelpers', () => { + describe('webhookRequestHandler', () => { + const webhookManager = mock>(); + const handler = webhookRequestHandler(webhookManager); + + beforeEach(() => { + jest.resetAllMocks(); + }); + + it('should throw for unsupported methods', async () => { + const req = mock({ + method: 'CONNECT' as IHttpRequestMethods, + }); + const res = mock(); + 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({ + method: 'OPTIONS', + headers: { + origin: 'https://example.com', + 'access-control-request-method': undefined, + }, + params: { path: 'test' }, + }); + const res = mock(); + 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({ + method: 'OPTIONS', + headers: { + origin: 'https://example.com', + 'access-control-request-method': 'GET', + }, + params: { path: 'test' }, + }); + const res = mock(); + 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 = (Math.random() * 10e6).toString(16); + const req = mock({ + method: 'OPTIONS', + headers: { + origin: randomOrigin, + 'access-control-request-method': 'GET', + }, + params: { path: 'test' }, + }); + const res = mock(); + 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({ + method: 'OPTIONS', + headers: { + origin: 'https://example.com', + 'access-control-request-method': 'GET', + }, + params: { path: 'test' }, + }); + const res = mock(); + 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'); + }); + }); + }); +}); diff --git a/packages/cli/test/unit/webhooks.test.ts b/packages/cli/test/unit/webhooks.test.ts index f6478806b4..8d315f02d5 100644 --- a/packages/cli/test/unit/webhooks.test.ts +++ b/packages/cli/test/unit/webhooks.test.ts @@ -46,7 +46,10 @@ describe('WebhookServer', () => { const pathPrefix = config.getEnv(`endpoints.${key}`); manager.getWebhookMethods.mockResolvedValueOnce(['GET']); - const response = await agent.options(`/${pathPrefix}/abcd`).set('origin', corsOrigin); + const response = await agent + .options(`/${pathPrefix}/abcd`) + .set('origin', corsOrigin) + .set('access-control-request-method', 'GET'); expect(response.statusCode).toEqual(204); expect(response.body).toEqual({}); expect(response.headers['access-control-allow-origin']).toEqual(corsOrigin); @@ -60,7 +63,10 @@ describe('WebhookServer', () => { mockResponse({ test: true }, { key: 'value ' }), ); - const response = await agent.get(`/${pathPrefix}/abcd`).set('origin', corsOrigin); + const response = await agent + .get(`/${pathPrefix}/abcd`) + .set('origin', corsOrigin) + .set('access-control-request-method', 'GET'); expect(response.statusCode).toEqual(200); expect(response.body).toEqual({ test: true }); expect(response.headers['access-control-allow-origin']).toEqual(corsOrigin); diff --git a/packages/nodes-base/nodes/Webhook/Webhook.node.ts b/packages/nodes-base/nodes/Webhook/Webhook.node.ts index 191d2af7f5..677ae1b7c4 100644 --- a/packages/nodes-base/nodes/Webhook/Webhook.node.ts +++ b/packages/nodes-base/nodes/Webhook/Webhook.node.ts @@ -45,6 +45,7 @@ export class Webhook extends Node { defaults: { name: 'Webhook', }, + supportsCORS: true, triggerPanel: { header: '', executionsHelp: { diff --git a/packages/workflow/src/Interfaces.ts b/packages/workflow/src/Interfaces.ts index 4b35633ec8..06b6340834 100644 --- a/packages/workflow/src/Interfaces.ts +++ b/packages/workflow/src/Interfaces.ts @@ -1594,7 +1594,8 @@ export interface INodeTypeDescription extends INodeTypeBaseDescription { properties: INodeProperties[]; credentials?: INodeCredentialDescription[]; maxNodes?: number; // How many nodes of that type can be created in a workflow - polling?: boolean; + polling?: true | undefined; + supportsCORS?: true | undefined; requestDefaults?: DeclarativeRestApiSettings.HttpRequestOptions; requestOperations?: IN8nRequestOperations; hooks?: { diff --git a/packages/workflow/src/NodeHelpers.ts b/packages/workflow/src/NodeHelpers.ts index 23350559d1..ae46dc2fdb 100644 --- a/packages/workflow/src/NodeHelpers.ts +++ b/packages/workflow/src/NodeHelpers.ts @@ -236,7 +236,7 @@ export const cronNodeOptions: INodePropertyCollection[] = [ }, ]; -const specialNodeParameters: INodeProperties[] = [ +const commonPollingParameters: INodeProperties[] = [ { displayName: 'Poll Times', name: 'pollTimes', @@ -252,12 +252,28 @@ const specialNodeParameters: INodeProperties[] = [ }, ]; +const commonCORSParameters: INodeProperties[] = [ + { + displayName: 'Allowed Origins (CORS)', + name: 'allowedOrigins', + type: 'string', + default: '*', + description: 'The origin(s) to allow cross-origin non-preflight requests from in a browser', + }, +]; + /** * Apply special parameters which should be added to nodeTypes depending on their type or configuration */ export function applySpecialNodeParameters(nodeType: INodeType): void { - if (nodeType.description.polling === true) { - nodeType.description.properties.unshift(...specialNodeParameters); + const { properties, polling, supportsCORS } = nodeType.description; + if (polling) { + properties.unshift(...commonPollingParameters); + } + if (nodeType.webhook && supportsCORS) { + const optionsProperty = properties.find(({ name }) => name === 'options'); + if (optionsProperty) optionsProperty.options!.push(...commonCORSParameters); + else properties.push(...commonCORSParameters); } }