diff --git a/packages/nodes-base/nodes/Github/GithubTrigger.node.ts b/packages/nodes-base/nodes/Github/GithubTrigger.node.ts index 607498a628e..3a4709ab2e4 100644 --- a/packages/nodes-base/nodes/Github/GithubTrigger.node.ts +++ b/packages/nodes-base/nodes/Github/GithubTrigger.node.ts @@ -1,3 +1,4 @@ +import { randomBytes } from 'crypto'; import type { IHookFunctions, IWebhookFunctions, @@ -10,6 +11,7 @@ import type { import { NodeConnectionTypes, NodeApiError, NodeOperationError } from 'n8n-workflow'; import { githubApiRequest } from './GenericFunctions'; +import { verifySignature } from './GithubTriggerHelpers'; import { getRepositories, getUsers } from './SearchFunctions'; export class GithubTrigger implements INodeType { @@ -507,12 +509,16 @@ export class GithubTrigger implements INodeType { const endpoint = `/repos/${owner}/${repository}/hooks`; const options = this.getNodeParameter('options') as { insecureSSL: boolean }; + // Generate a secure random secret for webhook signature verification + const webhookSecret = randomBytes(32).toString('hex'); + const body = { name: 'web', config: { url: webhookUrl, content_type: 'json', insecure_ssl: options.insecureSSL ? '1' : '0', + secret: webhookSecret, }, events, active: true, @@ -538,6 +544,8 @@ export class GithubTrigger implements INodeType { // create it again simply save the webhook-id webhookData.webhookId = webhook.id as string; webhookData.webhookEvents = webhook.events as string[]; + // Legacy webhook without secret on GitHub's side - not setting webhookData.webhookSecret + // so signature verification is skipped. To enable it, deactivate and reactivate the workflow. return true; } } @@ -570,6 +578,7 @@ export class GithubTrigger implements INodeType { webhookData.webhookId = responseData.id as string; webhookData.webhookEvents = responseData.events as string[]; + webhookData.webhookSecret = webhookSecret; return true; }, @@ -594,6 +603,7 @@ export class GithubTrigger implements INodeType { // that no webhooks are registered anymore delete webhookData.webhookId; delete webhookData.webhookEvents; + delete webhookData.webhookSecret; } return true; @@ -609,9 +619,19 @@ export class GithubTrigger implements INodeType { }; async webhook(this: IWebhookFunctions): Promise { + // Verify the webhook signature before processing + if (!verifySignature.call(this)) { + const res = this.getResponseObject(); + res.status(401).send('Unauthorized').end(); + + return { + noWebhookResponse: true, + }; + } + const bodyData = this.getBodyData(); - // Check if the webhook is only the ping from Github to confirm if it workshook_id + // Check if the webhook is only the ping from Github to confirm if it works if (bodyData.hook_id !== undefined && bodyData.action === undefined) { // Is only the ping and not an actual webhook call. So return 'OK' // but do not start the workflow. diff --git a/packages/nodes-base/nodes/Github/GithubTriggerHelpers.ts b/packages/nodes-base/nodes/Github/GithubTriggerHelpers.ts new file mode 100644 index 00000000000..c10caf0a950 --- /dev/null +++ b/packages/nodes-base/nodes/Github/GithubTriggerHelpers.ts @@ -0,0 +1,72 @@ +import { createHmac, timingSafeEqual } from 'crypto'; +import type { IWebhookFunctions } from 'n8n-workflow'; + +/** + * Verifies the GitHub webhook signature using HMAC-SHA256. + * + * GitHub sends a signature in the `X-Hub-Signature-256` header in the format: + * `sha256=` + * + * This function computes the expected signature using the stored webhook secret + * and compares it with the provided signature using a constant-time comparison. + * + * @returns true if signature is valid or no secret is configured, false otherwise + */ +export function verifySignature(this: IWebhookFunctions): boolean { + // Get the secret from workflow static data (set during webhook creation) + const webhookData = this.getWorkflowStaticData('node'); + const webhookSecret = webhookData.webhookSecret as string | undefined; + + // If no secret is configured, skip verification (backwards compatibility) + if (!webhookSecret) { + return true; + } + + const req = this.getRequestObject(); + + // Get the signature from GitHub's header + const signature = req.header('x-hub-signature-256'); + if (!signature) { + return false; + } + + // Validate signature format (must start with "sha256=") + if (!signature.startsWith('sha256=')) { + return false; + } + + // Extract just the hex digest part + const providedSignature = signature.substring(7); + + try { + // Get the raw request body + if (!req.rawBody) { + return false; + } + + // Compute HMAC-SHA256 of the raw body using our secret + const hmac = createHmac('sha256', webhookSecret); + + if (Buffer.isBuffer(req.rawBody)) { + hmac.update(req.rawBody); + } else { + const rawBodyString = + typeof req.rawBody === 'string' ? req.rawBody : JSON.stringify(req.rawBody); + hmac.update(rawBodyString); + } + + const computedSignature = hmac.digest('hex'); + + const computedBuffer = Buffer.from(computedSignature, 'utf8'); + const providedBuffer = Buffer.from(providedSignature, 'utf8'); + + // Buffers must be same length for timingSafeEqual + if (computedBuffer.length !== providedBuffer.length) { + return false; + } + + return timingSafeEqual(computedBuffer, providedBuffer); + } catch { + return false; + } +} diff --git a/packages/nodes-base/nodes/Github/__tests__/GithubTriggerHelpers.test.ts b/packages/nodes-base/nodes/Github/__tests__/GithubTriggerHelpers.test.ts new file mode 100644 index 00000000000..7106d721e9b --- /dev/null +++ b/packages/nodes-base/nodes/Github/__tests__/GithubTriggerHelpers.test.ts @@ -0,0 +1,159 @@ +import { createHmac, timingSafeEqual } from 'crypto'; +import { verifySignature } from '../GithubTriggerHelpers'; + +jest.mock('crypto', () => ({ + ...jest.requireActual('crypto'), + createHmac: jest.fn().mockReturnValue({ + update: jest.fn().mockReturnThis(), + digest: jest + .fn() + .mockReturnValue('757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17'), + }), + timingSafeEqual: jest.fn(), +})); + +describe('GithubTriggerHelpers', () => { + let mockWebhookFunctions: { + getWorkflowStaticData: jest.Mock; + getRequestObject: jest.Mock; + }; + const testWebhookSecret = 'a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2c3d4e5f6a1b2'; + const testBody = + '{"action":"opened","pull_request":{"id":123},"repository":{"full_name":"owner/repo"}}'; + const testSignature = 'sha256=757107ea0eb2509fc211221cce984b8a37570b6d7586c22c46f4379c8b043e17'; + + beforeEach(() => { + jest.clearAllMocks(); + + mockWebhookFunctions = { + getWorkflowStaticData: jest.fn(), + getRequestObject: jest.fn(), + }; + + // Default mock return values + mockWebhookFunctions.getWorkflowStaticData.mockReturnValue({ + webhookSecret: testWebhookSecret, + }); + + mockWebhookFunctions.getRequestObject.mockReturnValue({ + header: jest.fn().mockImplementation((header) => { + if (header === 'x-hub-signature-256') return testSignature; + return null; + }), + rawBody: testBody, + }); + }); + + describe('verifySignature', () => { + it('should return true when no webhook secret is stored (backwards compatibility)', () => { + mockWebhookFunctions.getWorkflowStaticData.mockReturnValue({}); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(true); + expect(mockWebhookFunctions.getWorkflowStaticData).toHaveBeenCalledWith('node'); + }); + + it('should return false when signature header is missing', () => { + mockWebhookFunctions.getRequestObject.mockReturnValue({ + header: jest.fn().mockReturnValue(null), + rawBody: testBody, + }); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(false); + }); + + it('should return false when signature does not start with sha256=', () => { + mockWebhookFunctions.getRequestObject.mockReturnValue({ + header: jest.fn().mockImplementation((header) => { + if (header === 'x-hub-signature-256') return 'invalid-format-signature'; + return null; + }), + rawBody: testBody, + }); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(false); + }); + + it('should return false when rawBody is missing', () => { + mockWebhookFunctions.getRequestObject.mockReturnValue({ + header: jest.fn().mockImplementation((header) => { + if (header === 'x-hub-signature-256') return testSignature; + return null; + }), + rawBody: undefined, + }); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(false); + }); + + it('should return true when signature is valid', () => { + (timingSafeEqual as jest.Mock).mockReturnValue(true); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(true); + expect(createHmac).toHaveBeenCalledWith('sha256', testWebhookSecret); + expect(timingSafeEqual).toHaveBeenCalled(); + }); + + it('should return false when signature is invalid', () => { + (timingSafeEqual as jest.Mock).mockReturnValue(false); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(false); + expect(createHmac).toHaveBeenCalledWith('sha256', testWebhookSecret); + expect(timingSafeEqual).toHaveBeenCalled(); + }); + + it('should handle Buffer rawBody correctly', () => { + const bufferBody = Buffer.from(testBody); + mockWebhookFunctions.getRequestObject.mockReturnValue({ + header: jest.fn().mockImplementation((header) => { + if (header === 'x-hub-signature-256') return testSignature; + return null; + }), + rawBody: bufferBody, + }); + (timingSafeEqual as jest.Mock).mockReturnValue(true); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(true); + const mockHmac = createHmac('sha256', testWebhookSecret); + expect(mockHmac.update).toHaveBeenCalledWith(bufferBody); + }); + + it('should return false when computed and provided signatures have different lengths', () => { + // Mock a different length signature + const mockHmacInstance = { + update: jest.fn().mockReturnThis(), + digest: jest.fn().mockReturnValue('short'), + }; + (createHmac as jest.Mock).mockReturnValue(mockHmacInstance); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(false); + // timingSafeEqual should not be called if lengths don't match + expect(timingSafeEqual).not.toHaveBeenCalled(); + }); + + it('should return false when an error occurs during verification', () => { + (createHmac as jest.Mock).mockImplementation(() => { + throw new Error('Crypto error'); + }); + + const result = verifySignature.call(mockWebhookFunctions as never); + + expect(result).toBe(false); + }); + }); +}); diff --git a/packages/nodes-base/nodes/Github/__tests__/node/GithubTrigger.node.test.ts b/packages/nodes-base/nodes/Github/__tests__/node/GithubTrigger.node.test.ts index cf666769632..51698a83940 100644 --- a/packages/nodes-base/nodes/Github/__tests__/node/GithubTrigger.node.test.ts +++ b/packages/nodes-base/nodes/Github/__tests__/node/GithubTrigger.node.test.ts @@ -1,5 +1,6 @@ import { GithubTrigger } from '../../GithubTrigger.node'; import * as GenericFunctions from '../../GenericFunctions'; +import * as GithubTriggerHelpers from '../../GithubTriggerHelpers'; import { NodeOperationError } from 'n8n-workflow'; describe('GithubTrigger Node', () => { @@ -53,19 +54,43 @@ describe('GithubTrigger Node', () => { }; }); - it('should return true and set webhookId when creation succeeds', async () => { + it('should return true and set webhookId and webhookSecret when creation succeeds', async () => { const createdWebhook = { id: '789', active: true }; - jest.spyOn(GenericFunctions, 'githubApiRequest').mockResolvedValueOnce(createdWebhook); // Simulate successful POST + jest.spyOn(GenericFunctions, 'githubApiRequest').mockResolvedValueOnce(createdWebhook); const trigger = new GithubTrigger(); const result = await trigger.webhookMethods.default.create.call(mockThis); expect(result).toBe(true); expect(webhookData.webhookId).toBe('789'); + expect(webhookData.webhookSecret).toBeDefined(); + expect(typeof webhookData.webhookSecret).toBe('string'); + expect(webhookData.webhookSecret.length).toBe(64); // 32 bytes in hex }); - it('should handle 422 by checking for existing matching webhook', async () => { + it('should send the secret to GitHub API when creating webhook', async () => { + const createdWebhook = { id: '789', active: true }; + + const apiRequestSpy = jest + .spyOn(GenericFunctions, 'githubApiRequest') + .mockResolvedValueOnce(createdWebhook); + + const trigger = new GithubTrigger(); + await trigger.webhookMethods.default.create.call(mockThis); + + expect(apiRequestSpy).toHaveBeenCalledWith( + 'POST', + '/repos/some-owner/some-repo/hooks', + expect.objectContaining({ + config: expect.objectContaining({ + secret: expect.any(String), + }), + }), + ); + }); + + it('should handle 422 by checking for existing matching webhook (no secret stored)', async () => { const existingWebhook = { id: '123', events: ['push'], @@ -82,6 +107,8 @@ describe('GithubTrigger Node', () => { expect(result).toBe(true); expect(webhookData.webhookId).toBe('123'); + // Existing webhook won't have secret stored (backwards compatibility) + expect(webhookData.webhookSecret).toBeUndefined(); }); it('should throw NodeOperationError if repo is not found (404)', async () => { @@ -98,4 +125,96 @@ describe('GithubTrigger Node', () => { ); }); }); + + describe('delete webhook method', () => { + let webhookData: Record; + let mockThis: any; + + beforeEach(() => { + webhookData = { + webhookId: '123456', + webhookEvents: ['push'], + webhookSecret: 'test-secret', + }; + + mockThis = { + getWorkflowStaticData: () => webhookData, + getNodeParameter: jest.fn().mockImplementation((name: string) => { + if (name === 'owner') return 'some-owner'; + if (name === 'repository') return 'some-repo'; + }), + }; + }); + + it('should delete webhook data including secret when deletion succeeds', async () => { + jest.spyOn(GenericFunctions, 'githubApiRequest').mockResolvedValueOnce({}); + + const trigger = new GithubTrigger(); + const result = await trigger.webhookMethods.default.delete.call(mockThis); + + expect(result).toBe(true); + expect(webhookData.webhookId).toBeUndefined(); + expect(webhookData.webhookEvents).toBeUndefined(); + expect(webhookData.webhookSecret).toBeUndefined(); + }); + }); + + describe('webhook method', () => { + let mockThis: any; + let webhookData: Record; + + beforeEach(() => { + webhookData = { + webhookSecret: 'test-secret', + }; + + mockThis = { + getWorkflowStaticData: () => webhookData, + getBodyData: jest.fn().mockReturnValue({ action: 'opened' }), + getHeaderData: jest.fn().mockReturnValue({}), + getQueryData: jest.fn().mockReturnValue({}), + getResponseObject: jest.fn().mockReturnValue({ + status: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + end: jest.fn(), + }), + getRequestObject: jest.fn().mockReturnValue({ + header: jest.fn(), + rawBody: '{}', + }), + helpers: { + returnJsonArray: jest.fn().mockImplementation((data) => data), + }, + }; + }); + + it('should reject with 401 when signature verification fails', async () => { + jest.spyOn(GithubTriggerHelpers, 'verifySignature').mockReturnValueOnce(false); + + const trigger = new GithubTrigger(); + const result = await trigger.webhook.call(mockThis); + + expect(result).toEqual({ noWebhookResponse: true }); + expect(mockThis.getResponseObject).toHaveBeenCalled(); + }); + + it('should process webhook when signature verification succeeds', async () => { + jest.spyOn(GithubTriggerHelpers, 'verifySignature').mockReturnValueOnce(true); + + const trigger = new GithubTrigger(); + const result = await trigger.webhook.call(mockThis); + + expect(result).toHaveProperty('workflowData'); + }); + + it('should return OK for ping events when signature verification succeeds', async () => { + jest.spyOn(GithubTriggerHelpers, 'verifySignature').mockReturnValueOnce(true); + mockThis.getBodyData.mockReturnValue({ hook_id: '123' }); + + const trigger = new GithubTrigger(); + const result = await trigger.webhook.call(mockThis); + + expect(result).toEqual({ webhookResponse: 'OK' }); + }); + }); });