From c0bb71d55392880e61c5df4c7cd5d66c116439be Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Tue, 2 Jan 2024 15:52:43 +0000 Subject: [PATCH] Fix GitHub events not verifying (#875) * Ensure we verify the raw payload. * changelog * Tidy up types * Add test for GitHib * Mock out GitHub API to allow tests to pass. * Lint --- changelog.d/875.bugfix | 1 + spec/github.spec.ts | 143 +++++++++++++++++++++++++++++++++++ spec/util/bridge-api.ts | 14 ++++ spec/util/e2e-test.ts | 5 ++ spec/widgets.spec.ts | 12 +-- src/App/BridgeApp.ts | 1 + src/Webhooks.ts | 38 ++++++++-- src/github/GithubInstance.ts | 5 +- 8 files changed, 200 insertions(+), 19 deletions(-) create mode 100644 changelog.d/875.bugfix create mode 100644 spec/github.spec.ts create mode 100644 spec/util/bridge-api.ts diff --git a/changelog.d/875.bugfix b/changelog.d/875.bugfix new file mode 100644 index 000000000..1059a294e --- /dev/null +++ b/changelog.d/875.bugfix @@ -0,0 +1 @@ +Fix GitHub events not working due to verification failures. diff --git a/spec/github.spec.ts b/spec/github.spec.ts new file mode 100644 index 000000000..57a431c5b --- /dev/null +++ b/spec/github.spec.ts @@ -0,0 +1,143 @@ +import { E2ESetupTestTimeout, E2ETestEnv } from "./util/e2e-test"; +import { describe, it, beforeEach, afterEach } from "@jest/globals"; +import { createHmac, randomUUID } from "crypto"; +import { GitHubRepoConnection, GitHubRepoConnectionState } from "../src/Connections"; +import { MessageEventContent } from "matrix-bot-sdk"; +import { getBridgeApi } from "./util/bridge-api"; +import { Server, createServer } from "http"; + +describe('GitHub', () => { + let testEnv: E2ETestEnv; + let githubServer: Server; + const webhooksPort = 9500 + E2ETestEnv.workerId; + const githubPort = 9700 + E2ETestEnv.workerId; + + beforeEach(async () => { + // Fake out enough of a GitHub API to get past startup. Later + // tests might make more use of this. + githubServer = createServer((req, res) => { + if (req.method === 'GET' && req.url === '/api/v3/app') { + res.writeHead(200, undefined, { "content-type": 'application/json'}); + res.write(JSON.stringify({})); + } else if (req.method === 'GET' && req.url === '/api/v3/app/installations?per_page=100&page=1') { + res.writeHead(200, undefined, { "content-type": 'application/json'}); + res.write(JSON.stringify([])); + } else { + console.log('Unknown request', req.method, req.url); + res.writeHead(404); + } + res.end(); + }).listen(githubPort); + testEnv = await E2ETestEnv.createTestEnv({matrixLocalparts: ['user'], config: { + github: { + webhook: { + secret: randomUUID(), + }, + // So we can mock out the URL + enterpriseUrl: `http://localhost:${githubPort}`, + auth: { + privateKeyFile: 'replaced', + id: '1234', + } + }, + widgets: { + publicUrl: `http://localhost:${webhooksPort}` + }, + listeners: [{ + port: webhooksPort, + bindAddress: '0.0.0.0', + // Bind to the SAME listener to ensure we don't have conflicts. + resources: ['webhooks', 'widgets'], + }], + }}); + await testEnv.setUp(); + }, E2ESetupTestTimeout); + + afterEach(() => { + githubServer?.close(); + return testEnv?.tearDown(); + }); + + it.only('should be able to handle a GitHub event', async () => { + const user = testEnv.getUser('user'); + const bridgeApi = await getBridgeApi(testEnv.opts.config?.widgets?.publicUrl!, user); + const testRoomId = await user.createRoom({ name: 'Test room', invite:[testEnv.botMxid] }); + await user.setUserPowerLevel(testEnv.botMxid, testRoomId, 50); + await user.waitForRoomJoin({sender: testEnv.botMxid, roomId: testRoomId }); + // Now hack in a GitHub connection. + await testEnv.app.appservice.botClient.sendStateEvent(testRoomId, GitHubRepoConnection.CanonicalEventType, "my-test", { + org: 'my-org', + repo: 'my-repo' + } satisfies GitHubRepoConnectionState); + + // Wait for connection to be accepted. + await new Promise(r => { + let interval: NodeJS.Timeout; + interval = setInterval(() => { + bridgeApi.getConnectionsForRoom(testRoomId).then(conns => { + if (conns.length > 0) { + clearInterval(interval); + r(); + } + }) + }, 500); + }); + + const webhookNotice = user.waitForRoomEvent({ + eventType: 'm.room.message', sender: testEnv.botMxid, roomId: testRoomId + }); + + const webhookPayload = JSON.stringify({ + "action": "opened", + "number": 1, + "pull_request": { + id: 1, + "url": "https://api.github.com/repos/my-org/my-repo/pulls/1", + "html_url": "https://github.com/my-org/my-repo/pulls/1", + "number": 1, + "state": "open", + "locked": false, + "title": "My test pull request", + "user": { + "login": "alice", + }, + }, + repository: { + id: 1, + "html_url": "https://github.com/my-org/my-repo", + name: 'my-repo', + full_name: 'my-org/my-repo', + owner: { + login: 'my-org', + } + }, + sender: { + login: 'alice', + } + }); + + const hmac = createHmac('sha256', testEnv.opts.config?.github?.webhook.secret!); + hmac.write(webhookPayload); + hmac.end(); + + // Send a webhook + const req = await fetch(`http://localhost:${webhooksPort}/`, { + method: 'POST', + headers: { + 'x-github-event': 'pull_request', + 'X-Hub-Signature-256': `sha256=${hmac.read().toString('hex')}`, + 'X-GitHub-Delivery': randomUUID(), + 'Content-Type': 'application/json' + }, + body: webhookPayload, + }); + expect(req.status).toBe(200); + expect(await req.text()).toBe('OK'); + + // And await the notice. + const { body } = (await webhookNotice).data.content; + expect(body).toContain('**alice** opened a new PR'); + expect(body).toContain('https://github.com/my-org/my-repo/pulls/1'); + expect(body).toContain('My test pull request'); + }); +}); diff --git a/spec/util/bridge-api.ts b/spec/util/bridge-api.ts new file mode 100644 index 000000000..a9e7a70fe --- /dev/null +++ b/spec/util/bridge-api.ts @@ -0,0 +1,14 @@ +import { MatrixClient } from "matrix-bot-sdk"; +import { BridgeAPI } from "../../web/BridgeAPI"; +import { WidgetApi } from "matrix-widget-api"; + +export async function getBridgeApi(publicUrl: string, user: MatrixClient) { + return BridgeAPI.getBridgeAPI(publicUrl, { + requestOpenIDConnectToken: () => { + return user.getOpenIDConnectToken() + }, + } as unknown as WidgetApi, { + getItem() { return null}, + setItem() { }, + } as unknown as Storage); +} \ No newline at end of file diff --git a/spec/util/e2e-test.ts b/spec/util/e2e-test.ts index c66bfb548..08a95a706 100644 --- a/spec/util/e2e-test.ts +++ b/spec/util/e2e-test.ts @@ -188,6 +188,11 @@ export class E2ETestEnv { 'hookshot': homeserver.url, } } + + if (providedConfig?.github) { + providedConfig.github.auth.privateKeyFile = keyPath; + } + const config = new BridgeConfig({ bridge: { domain: homeserver.domain, diff --git a/spec/widgets.spec.ts b/spec/widgets.spec.ts index 658710eaf..075515ebb 100644 --- a/spec/widgets.spec.ts +++ b/spec/widgets.spec.ts @@ -1,7 +1,6 @@ import { E2ESetupTestTimeout, E2ETestEnv } from "./util/e2e-test"; import { describe, it, beforeEach, afterEach } from "@jest/globals"; -import { BridgeAPI } from "../web/BridgeAPI"; -import { WidgetApi } from "matrix-widget-api"; +import { getBridgeApi } from "./util/bridge-api"; describe('Widgets', () => { let testEnv: E2ETestEnv; @@ -29,14 +28,7 @@ describe('Widgets', () => { it('should be able to authenticate with the widget API', async () => { const user = testEnv.getUser('user'); - const bridgeApi = await BridgeAPI.getBridgeAPI(testEnv.opts.config?.widgets?.publicUrl!, { - requestOpenIDConnectToken: () => { - return user.getOpenIDConnectToken() - }, - } as unknown as WidgetApi, { - getItem() { return null}, - setItem() { }, - } as unknown as Storage); + const bridgeApi = await getBridgeApi(testEnv.opts.config?.widgets?.publicUrl!, user); expect(await bridgeApi.verify()).toEqual({ "type": "widget", "userId": "@user:hookshot", diff --git a/src/App/BridgeApp.ts b/src/App/BridgeApp.ts index da63d4260..c8073c62c 100644 --- a/src/App/BridgeApp.ts +++ b/src/App/BridgeApp.ts @@ -61,6 +61,7 @@ export async function start(config: BridgeConfig, registration: IAppserviceRegis storage.disconnect?.(); }); return { + appservice, bridgeApp, storage, listener, diff --git a/src/Webhooks.ts b/src/Webhooks.ts index 958cfde40..4597f7ca0 100644 --- a/src/Webhooks.ts +++ b/src/Webhooks.ts @@ -1,3 +1,4 @@ + /* eslint-disable camelcase */ import { BridgeConfig } from "./config/Config"; import { Router, default as express, Request, Response } from "express"; @@ -44,6 +45,15 @@ export interface OAuthPageParams { 'errcode'?: ErrCode; } +interface GitHubRequestData { + payload: string; + signature: string; +} + +interface WebhooksExpressRequest extends Request { + github?: GitHubRequestData; +} + export class Webhooks extends EventEmitter { public readonly expressRouter = Router(); @@ -146,7 +156,7 @@ export class Webhooks extends EventEmitter { } } - private onPayload(req: Request, res: Response) { + private onPayload(req: WebhooksExpressRequest, res: Response) { try { let eventName: string|null = null; const body = req.body; @@ -162,11 +172,15 @@ export class Webhooks extends EventEmitter { return; } this.handledGuids.set(githubGuid); + const githubData = req.github as GitHubRequestData; + if (!githubData) { + throw Error('Expected github data to be set on request'); + } this.ghWebhooks.verifyAndReceive({ id: githubGuid as string, name: req.headers["x-github-event"] as EmitterWebhookEventName, - payload: body, - signature: req.headers["x-hub-signature-256"] as string, + payload: githubData.payload, + signature: githubData.signature, }).catch((err) => { log.error(`Failed handle GitHubEvent: ${err}`); }); @@ -285,7 +299,7 @@ export class Webhooks extends EventEmitter { } } - private verifyRequest(req: Request, res: Response) { + private verifyRequest(req: WebhooksExpressRequest, res: Response, buffer: Buffer, encoding: BufferEncoding) { if (req.headers['x-gitlab-token']) { // GitLab if (!this.config.gitlab) { @@ -301,9 +315,21 @@ export class Webhooks extends EventEmitter { res.sendStatus(403); throw Error("Invalid signature."); } - } else if (req.headers["x-hub-signature"]) { + } else if (req.headers["x-hub-signature-256"] && this.ghWebhooks) { // GitHub - // Verified within handler. + if (typeof req.headers["x-hub-signature-256"] !== "string") { + throw new ApiError("Unexpected multiple headers for x-hub-signature-256", ErrCode.BadValue, 400); + } + let jsonStr; + try { + jsonStr = buffer.toString(encoding) + } catch (ex) { + throw new ApiError("Could not decode buffer", ErrCode.BadValue, 400); + } + req.github = { + payload: jsonStr, + signature: req.headers["x-hub-signature-256"] + }; return true; } else if (JiraWebhooksRouter.IsJIRARequest(req)) { // JIRA diff --git a/src/github/GithubInstance.ts b/src/github/GithubInstance.ts index f9ccfe662..e86c4322f 100644 --- a/src/github/GithubInstance.ts +++ b/src/github/GithubInstance.ts @@ -66,11 +66,11 @@ export class GithubInstance { public static baseOctokitConfig(baseUrl: URL) { // Enterprise GitHub uses a /api/v3 basepath (https://github.com/octokit/octokit.js#constructor-options) // Cloud uses api.github.com - const url = baseUrl.hostname === GITHUB_CLOUD_URL.hostname ? baseUrl : new URL("/api/v3", baseUrl); + const url = (baseUrl.hostname === GITHUB_CLOUD_URL.hostname ? baseUrl : new URL("/api/v3", baseUrl)).toString(); return { userAgent: UserAgent, // Remove trailing slash, which is always included in URL objects. - baseUrl: url.toString().substring(0,-1), + baseUrl: url.endsWith('/') ? url.slice(0, -1) : url, } } @@ -142,7 +142,6 @@ export class GithubInstance { ...GithubInstance.baseOctokitConfig(this.baseUrl), }); - const appDetails = await this.internalOctokit.apps.getAuthenticated(); this.internalAppSlug = appDetails.data.slug;