From 0320405b1724a09ddb1b256014621a9298fc488b Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Thu, 8 Aug 2019 15:46:55 +0200 Subject: [PATCH 1/2] Properly verify webhook signature --- index.js | 44 ++++++++++++++++++++++++++++++++++++-------- package.json | 1 + 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/index.js b/index.js index 55fef42..b97713f 100644 --- a/index.js +++ b/index.js @@ -2,15 +2,18 @@ const { createProbot } = require('probot') const { resolve } = require('probot/lib/resolver') const { findPrivateKey } = require('probot/lib/private-key') const { template } = require('./views/probot') +const verify = require('@octokit/webhooks/verify') let probot -const loadProbot = appFn => { - probot = probot || createProbot({ - id: process.env.APP_ID, - secret: process.env.WEBHOOK_SECRET, - cert: findPrivateKey() - }) +const getOpts = () => ({ + id: process.env.APP_ID, + secret: process.env.WEBHOOK_SECRET, + cert: findPrivateKey() +}) + +const loadProbot = (opts, appFn) => { + probot = probot || createProbot(opts) if (typeof appFn === 'string') { appFn = resolve(appFn) @@ -32,13 +35,38 @@ module.exports.serverless = appFn => { }) } + const opts = getOpts() // Otherwise let's listen handle the payload - probot = probot || loadProbot(appFn) + probot = probot || loadProbot(opts, appFn) // Determine incoming webhook event type const name = request.get('x-github-event') || request.get('X-GitHub-Event') const id = request.get('x-github-delivery') || request.get('X-GitHub-Delivery') + // Verify signed payload + const signature = request.get('x-hub-signature') || request.get('X-Hub-Signature') + const body = request.body + if (!opts.secret) { + console.error('secret not set') + response.sendStatus(500) + return + } + let matchesSignature = false + try { + matchesSignature = verify(opts.secret, body, signature) + } catch (err) { + if (err instanceof TypeError) { + response.sendStatus(400) + return + } + throw err + } + if (!matchesSignature) { + console.error('signature does not match event payload and secret') + response.sendStatus(400) + return + } + // Do the thing console.log(`Received event ${name}${request.body.action ? ('.' + request.body.action) : ''}`) if (name) { @@ -46,7 +74,7 @@ module.exports.serverless = appFn => { await probot.receive({ name, id, - payload: request.body + payload: body }) response.send({ statusCode: 200, diff --git a/package.json b/package.json index 1c1e885..7951739 100644 --- a/package.json +++ b/package.json @@ -11,6 +11,7 @@ "url": "git+https://github.com/probot/serverless-gcf.git" }, "devDependencies": { + "@octokit/webhooks": "^5.0.2", "jest": "^23.6.0", "probot": "^7.1.2", "standard": "^10.0.3" From 7c669f4310f89da50c7e825d2e21e132dbc23208 Mon Sep 17 00:00:00 2001 From: Michael Beaumont Date: Thu, 8 Aug 2019 22:48:38 +0200 Subject: [PATCH 2/2] Add tests for signature verification --- tests/index.test.js | 78 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 74 insertions(+), 4 deletions(-) diff --git a/tests/index.test.js b/tests/index.test.js index 86e0fda..f86ce71 100644 --- a/tests/index.test.js +++ b/tests/index.test.js @@ -1,4 +1,5 @@ const { serverless } = require('../') +const sign = require('@octokit/webhooks/sign') describe('serverless-gcf', () => { let spy, handler, response @@ -11,6 +12,9 @@ describe('serverless-gcf', () => { app.on('issues', spy) }) }) + afterEach(() => { + process.env.WEBHOOK_SECRET = '' + }) it('responds with the homepage', async () => { const request = { method: 'GET', path: '/probot' } @@ -20,15 +24,19 @@ describe('serverless-gcf', () => { }) it('calls the event handler', async () => { + process.env.WEBHOOK_SECRET = 'secret' + const body = { + installation: { id: 1 } + } + const signature = sign('secret', body) const request = { - body: { - installation: { id: 1 } - }, + body, get (string) { return this[string] }, 'x-github-event': 'issues', - 'x-github-delivery': 123 + 'x-github-delivery': 123, + 'x-hub-signature': signature } await handler(request, response) @@ -37,6 +45,7 @@ describe('serverless-gcf', () => { }) it('does nothing if there are missing headers', async () => { + process.env.WEBHOOK_SECRET = 'secret' const request = { body: { installation: { id: 1 } @@ -51,4 +60,65 @@ describe('serverless-gcf', () => { expect(spy).not.toHaveBeenCalled() expect(response.sendStatus).toHaveBeenCalledWith(400) }) + + it('does not allow invalid signatures', async () => { + process.env.WEBHOOK_SECRET = 'secret' + const body = { + installation: { id: 1 } + } + const signature = sign('wrong_secret', body) + const request = { + body, + get (string) { + return this[string] + }, + 'x-github-event': 'issues', + 'x-github-delivery': 123, + 'x-hub-signature': signature + } + + await handler(request, response) + expect(response.send).not.toHaveBeenCalled() + expect(spy).not.toHaveBeenCalled() + expect(response.sendStatus).toHaveBeenCalledWith(400) + }) + + it('requires the secret to be set', async () => { + const body = { + installation: { id: 1 } + } + const request = { + body, + get (string) { + return this[string] + }, + 'x-github-event': 'issues', + 'x-github-delivery': 123 + } + + await handler(request, response) + expect(response.send).not.toHaveBeenCalled() + expect(spy).not.toHaveBeenCalled() + expect(response.sendStatus).toHaveBeenCalledWith(500) + }) + + it('requires a signature', async () => { + process.env.WEBHOOK_SECRET = 'secret' + const body = { + installation: { id: 1 } + } + const request = { + body, + get (string) { + return this[string] + }, + 'x-github-event': 'issues', + 'x-github-delivery': 123 + } + + await handler(request, response) + expect(response.send).not.toHaveBeenCalled() + expect(spy).not.toHaveBeenCalled() + expect(response.sendStatus).toHaveBeenCalledWith(400) + }) })