Skip to content

Commit

Permalink
refactor, and get access control working for production webhooks as well
Browse files Browse the repository at this point in the history
  • Loading branch information
netroy committed Nov 22, 2023
1 parent c0187a9 commit 18188f4
Show file tree
Hide file tree
Showing 8 changed files with 242 additions and 116 deletions.
56 changes: 39 additions & 17 deletions packages/cli/src/ActiveWorkflowRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import type {
WorkflowExecuteMode,
INodeType,
IWebhookData,
IHttpRequestMethods,
} from 'n8n-workflow';
import {
NodeHelpers,
Expand All @@ -39,6 +40,7 @@ import type {
IWebhookManager,
IWorkflowDb,
IWorkflowExecutionDataProcess,
WebhookAccessControlOptions,
WebhookRequest,
} from '@/Interfaces';
import * as ResponseHelper from '@/ResponseHelper';
Expand Down Expand Up @@ -137,26 +139,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,
);
}
const webhook = await this.findWebhook(path, httpMethod);

if (webhook.isDynamic) {
const pathElements = path.split('/').slice(1);
Expand Down Expand Up @@ -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.
*/
Expand Down
13 changes: 11 additions & 2 deletions packages/cli/src/Interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<IHttpRequestMethods[]>;
getAccessControlOptions?: (

/** Find the CORS options matching a path and method */
findAccessControlOptions?: (
path: string,
httpMethod: IHttpRequestMethods,
) => Promise<IDataObject | null>;
) => Promise<WebhookAccessControlOptions | undefined>;

executeWebhook(req: WebhookRequest, res: Response): Promise<IResponseCallbackData>;
}

Expand Down
37 changes: 11 additions & 26 deletions packages/cli/src/TestWebhooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ import type {
Workflow,
WorkflowActivateMode,
WorkflowExecuteMode,
IDataObject,
} from 'n8n-workflow';

import { ActiveWebhooks } from '@/ActiveWebhooks';
import type {
IResponseCallbackData,
IWebhookManager,
IWorkflowDb,
WebhookAccessControlOptions,
WebhookRequest,
} from '@/Interfaces';
import { Push } from '@/push';
Expand Down Expand Up @@ -162,9 +162,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) {
Expand All @@ -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<IDataObject | null> {
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;
}

/**
Expand Down
34 changes: 17 additions & 17 deletions packages/cli/src/WebhookHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
142 changes: 142 additions & 0 deletions packages/cli/test/unit/WebhookHelpers.test.ts
Original file line number Diff line number Diff line change
@@ -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<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', '60000');
});

it('should handle wildcard origin', async () => {
const randomOrigin = (Math.random() * 10e6).toString(16);
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 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']);
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');
});
});
});
});
10 changes: 8 additions & 2 deletions packages/cli/test/unit/webhooks.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Loading

0 comments on commit 18188f4

Please sign in to comment.