From a5c6e2fecfd2ee64c28298a915bfc83ad4e35331 Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Wed, 20 Aug 2025 14:20:53 +0200 Subject: [PATCH] fix(core): Fix getting webhook methods from path only when dynamic webhook path (#17803) --- .../webhooks/__tests__/test-webhooks.test.ts | 58 ++++++++++++++++- .../__tests__/webhook.service.test.ts | 15 +++++ .../test-webhook-registrations.service.ts | 4 ++ packages/cli/src/webhooks/test-webhooks.ts | 63 ++++++++++++------- packages/cli/src/webhooks/webhook.service.ts | 19 ++++-- 5 files changed, 131 insertions(+), 28 deletions(-) diff --git a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts index 13c97ce55d..44d5e47675 100644 --- a/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts +++ b/packages/cli/src/webhooks/__tests__/test-webhooks.test.ts @@ -1,3 +1,4 @@ +import type { WorkflowEntity } from '@n8n/db'; import { generateNanoId } from '@n8n/db'; import type * as express from 'express'; import { mock } from 'jest-mock-extended'; @@ -7,6 +8,7 @@ import type { IWebhookData, IWorkflowExecuteAdditionalData, Workflow, + IHttpRequestMethods, } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; @@ -195,11 +197,40 @@ describe('TestWebhooks', () => { }); describe('getWebhookMethods()', () => { + beforeEach(() => { + registrations.toKey.mockImplementation( + (webhook: Pick) => { + const { webhookId, httpMethod, path: webhookPath } = webhook; + if (!webhookId) return [httpMethod, webhookPath].join('|'); + + let path = webhookPath; + if (path.startsWith(webhookId)) { + const cutFromIndex = path.indexOf('/') + 1; + + path = path.slice(cutFromIndex); + } + return [httpMethod, webhookId, path.split('/').length].join('|'); + }, + ); + }); + test('should normalize trailing slash', async () => { const METHOD = 'POST'; const PATH_WITH_SLASH = 'register/'; const PATH_WITHOUT_SLASH = 'register'; - registrations.getAllKeys.mockResolvedValue([`${METHOD}|${PATH_WITHOUT_SLASH}`]); + const webhookData = { + httpMethod: METHOD as IHttpRequestMethods, + path: PATH_WITHOUT_SLASH, + } as IWebhookData; + + registrations.getRegistrationsHash.mockImplementation(async () => { + return { + [registrations.toKey(webhookData)]: { + workflowEntity: mock(), + webhook: webhookData, + }, + }; + }); const resultWithSlash = await testWebhooks.getWebhookMethods(PATH_WITH_SLASH); const resultWithoutSlash = await testWebhooks.getWebhookMethods(PATH_WITHOUT_SLASH); @@ -207,5 +238,30 @@ describe('TestWebhooks', () => { expect(resultWithSlash).toEqual([METHOD]); expect(resultWithoutSlash).toEqual([METHOD]); }); + + test('should return methods for webhooks with dynamic paths', async () => { + const METHOD = 'POST'; + const PATH = '12345/register/:id'; + + const webhookData = { + webhookId: '12345', + httpMethod: METHOD as IHttpRequestMethods, + // Path for dynamic webhook does not contain webhookId + path: 'register/:id', + }; + + registrations.getRegistrationsHash.mockImplementation(async () => { + return { + [registrations.toKey(webhookData)]: { + workflowEntity: mock(), + webhook: webhookData as IWebhookData, + }, + }; + }); + + const result = await testWebhooks.getWebhookMethods(PATH); + + expect(result).toEqual([METHOD]); + }); }); }); diff --git a/packages/cli/src/webhooks/__tests__/webhook.service.test.ts b/packages/cli/src/webhooks/__tests__/webhook.service.test.ts index f4c1c7ac60..d06951fb92 100644 --- a/packages/cli/src/webhooks/__tests__/webhook.service.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook.service.test.ts @@ -154,6 +154,21 @@ describe('WebhookService', () => { expect(returnedMethods).toEqual([]); }); + + test('should return dynamic webhook method when static search returns nothing', async () => { + const webhookId = uuid(); + const dynamicPath = `${webhookId}/user/1`; + const mockDynamicWebhook = createWebhook('POST', 'user/:id', webhookId, 2); + + // Mock static webhook search to return empty + webhookRepository.find.mockResolvedValue([]); + // Mock dynamic webhook search to return a webhook + webhookRepository.findBy.mockResolvedValue([mockDynamicWebhook]); + + const returnedMethods = await webhookService.getWebhookMethods(dynamicPath); + + expect(returnedMethods).toEqual(['POST']); + }); }); describe('deleteWorkflowWebhooks()', () => { diff --git a/packages/cli/src/webhooks/test-webhook-registrations.service.ts b/packages/cli/src/webhooks/test-webhook-registrations.service.ts index 0f86be48d6..454acad599 100644 --- a/packages/cli/src/webhooks/test-webhook-registrations.service.ts +++ b/packages/cli/src/webhooks/test-webhook-registrations.service.ts @@ -71,6 +71,10 @@ export class TestWebhookRegistrationsService { return Object.values(hash); } + async getRegistrationsHash() { + return await this.cacheService.getHash(this.cacheKey); + } + async deregisterAll() { await this.cacheService.delete(this.cacheKey); } diff --git a/packages/cli/src/webhooks/test-webhooks.ts b/packages/cli/src/webhooks/test-webhooks.ts index 1af0a130a1..6e9f711caa 100644 --- a/packages/cli/src/webhooks/test-webhooks.ts +++ b/packages/cli/src/webhooks/test-webhooks.ts @@ -200,13 +200,34 @@ export class TestWebhooks implements IWebhookManager { if (timeout) clearTimeout(timeout); } + async getWebhooksFromPath(rawPath: string) { + const path = removeTrailingSlash(rawPath); + const webhooks: IWebhookData[] = []; + const registrations = await this.registrations.getRegistrationsHash(); + + for (const httpMethod of ['GET', 'POST', 'PUT', 'DELETE', 'PATCH'] as IHttpRequestMethods[]) { + const key = this.registrations.toKey({ httpMethod, path }); + let webhook = registrations?.[key]?.webhook; + if (!webhook) { + // check for dynamic webhooks + const [webhookId, ...segments] = path.split('/'); + const key = this.registrations.toKey({ httpMethod, path, webhookId }); + if (registrations?.[key]) { + webhook = this.getActiveWebhookFromRegistration(segments.join('/'), registrations?.[key]); + } + } + if (webhook) { + webhooks.push(webhook); + } + } + return webhooks; + } + async getWebhookMethods(rawPath: string) { const path = removeTrailingSlash(rawPath); - const allKeys = await this.registrations.getAllKeys(); + const webhooks = await this.getWebhooksFromPath(path); - const webhookMethods = allKeys - .filter((key) => key.includes(path)) - .map((key) => key.split('|')[0] as IHttpRequestMethods); + const webhookMethods = webhooks.map((webhook) => webhook.httpMethod); if (!webhookMethods.length) throw new WebhookNotFoundError({ path }); @@ -393,33 +414,31 @@ export class TestWebhooks implements IWebhookManager { return foundWebhook; } - async getActiveWebhook(httpMethod: IHttpRequestMethods, path: string, webhookId?: string) { - const key = this.registrations.toKey({ httpMethod, path, webhookId }); - - let webhook: IWebhookData | undefined; - let maxMatches = 0; + getActiveWebhookFromRegistration( + path: string, + registration: TestWebhookRegistration, + ): IWebhookData | undefined { const pathElementsSet = new Set(path.split('/')); - // check if static elements match in path - // if more results have been returned choose the one with the most static-route matches - const registration = await this.registrations.get(key); - - if (!registration) return; const { webhook: dynamicWebhook } = registration; const staticElements = dynamicWebhook.path.split('/').filter((ele) => !ele.startsWith(':')); const allStaticExist = staticElements.every((staticEle) => pathElementsSet.has(staticEle)); - if (allStaticExist && staticElements.length > maxMatches) { - maxMatches = staticElements.length; - webhook = dynamicWebhook; - } - // handle routes with no static elements - else if (staticElements.length === 0 && !webhook) { - webhook = dynamicWebhook; + // webhook matches if all static elements exist or if there are no static elements + if ((allStaticExist && staticElements.length > 0) || staticElements.length === 0) { + return dynamicWebhook; } + return undefined; + } - return webhook; + async getActiveWebhook(httpMethod: IHttpRequestMethods, path: string, webhookId?: string) { + const key = this.registrations.toKey({ httpMethod, path, webhookId }); + const registration = await this.registrations.get(key); + + if (!registration) return; + + return this.getActiveWebhookFromRegistration(path, registration); } /** diff --git a/packages/cli/src/webhooks/webhook.service.ts b/packages/cli/src/webhooks/webhook.service.ts index e9571a67a5..d594c0b81a 100644 --- a/packages/cli/src/webhooks/webhook.service.ts +++ b/packages/cli/src/webhooks/webhook.service.ts @@ -57,7 +57,7 @@ export class WebhookService { return dbStaticWebhook; } - return await this.findDynamicWebhook(method, path); + return await this.findDynamicWebhook(path, method); } /** @@ -71,7 +71,7 @@ export class WebhookService { * Find a matching webhook with one or more dynamic path segments, e.g. `/user/:id/posts`. * It is mandatory for dynamic webhooks to have `/` at the base. */ - private async findDynamicWebhook(method: Method, path: string) { + private async findDynamicWebhook(path: string, method?: Method) { const [uuidSegment, ...otherSegments] = path.split('/'); const dynamicWebhooks = await this.webhookRepository.findBy({ @@ -133,10 +133,19 @@ export class WebhookService { return await this.webhookRepository.remove(webhooks); } - async getWebhookMethods(path: string) { - return await this.webhookRepository - .find({ select: ['method'], where: { webhookPath: path } }) + async getWebhookMethods(rawPath: string) { + // Try to find static webhooks first + const staticMethods = await this.webhookRepository + .find({ select: ['method'], where: { webhookPath: rawPath } }) .then((rows) => rows.map((r) => r.method)); + + if (staticMethods.length > 0) { + return staticMethods; + } + + // Otherwise, try to find dynamic webhooks based on path only + const dynamicWebhooks = await this.findDynamicWebhook(rawPath); + return dynamicWebhooks ? [dynamicWebhooks.method] : []; } private isDynamicPath(rawPath: string) {