diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index a32ce7aa3d85e3..6a89804a29ae98 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 }) => + type === 'n8n-nodes-base.webhook' && + parameters?.path === path && + (parameters?.httpMethod ?? 'GET') === httpMethod, + ); + 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 4e943a767be2d2..551e32189c6019 100644 --- a/packages/cli/src/Interfaces.ts +++ b/packages/cli/src/Interfaces.ts @@ -267,12 +267,21 @@ 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; - getAccessControlOptions?: ( + + /** Find the CORS options matching a path and method */ + findAccessControlOptions?: ( path: string, httpMethod: IHttpRequestMethods, - ) => Promise; + ) => Promise; + executeWebhook(req: WebhookRequest, res: Response): Promise; } diff --git a/packages/cli/src/TestWebhooks.ts b/packages/cli/src/TestWebhooks.ts index 53b277b94fdb56..d050915ce9626d 100644 --- 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'; @@ -16,6 +15,7 @@ import type { IResponseCallbackData, IWebhookManager, IWorkflowDb, + WebhookAccessControlOptions, WebhookRequest, } from '@/Interfaces'; import { Push } from '@/push'; @@ -162,9 +162,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) { @@ -178,31 +175,19 @@ export class TestWebhooks implements IWebhookManager { return webhookMethods; } - /** - * Gets all request methods associated with a single test webhook - */ - async getAccessControlOptions(path: string, httpMethod: string): Promise { - const webhookWorkflow = Object.keys(this.testWebhookData).find( + async findAccessControlOptions(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 : {}; + const webhookNode = Object.values(nodes).find( + ({ type, parameters }) => + type === 'n8n-nodes-base.webhook' && + parameters?.path === path && + (parameters?.httpMethod ?? 'GET') === httpMethod, + ); + return webhookNode?.parameters?.options as WebhookAccessControlOptions; } /** diff --git a/packages/cli/src/WebhookHelpers.ts b/packages/cli/src/WebhookHelpers.ts index 2dce404729666d..cde83636356855 100644 --- a/packages/cli/src/WebhookHelpers.ts +++ b/packages/cli/src/WebhookHelpers.ts @@ -98,29 +98,29 @@ 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.findAccessControlOptions && requestedMethod) { + const options = await webhookManager.findAccessControlOptions(path, requestedMethod); + const { allowedOrigins, preflightMaxAge } = options ?? {}; - 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/cli/test/unit/WebhookHelpers.test.ts b/packages/cli/test/unit/WebhookHelpers.test.ts new file mode 100644 index 00000000000000..ff2d45353332d3 --- /dev/null +++ b/packages/cli/test/unit/WebhookHelpers.test.ts @@ -0,0 +1,142 @@ +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', '60000'); + }); + + 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 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']); + webhookManager.findAccessControlOptions.mockResolvedValue({ + allowedOrigins: 'https://test.com', + preflightMaxAge: 360, + }); + + 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'); + expect(res.header).toHaveBeenCalledWith('Access-Control-Max-Age', '360000'); + }); + }); + }); +}); diff --git a/packages/cli/test/unit/webhooks.test.ts b/packages/cli/test/unit/webhooks.test.ts index f6478806b4fc20..8d315f02d5213c 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 b6eec3ee361fad..191d2af7f5a598 100644 --- 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 index f475cad0fb9355..064637ceb39e6a 100644 --- 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)', }, ], };