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

@@ -0,0 +1 @@
+Fix GitHub events not working due to verification failures.

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<void>(r => {
+            let interval: NodeJS.Timeout;
+            interval = setInterval(() => {
+                bridgeApi.getConnectionsForRoom(testRoomId).then(conns => {
+                    if (conns.length > 0) {
+                        clearInterval(interval);
+                        r();
+                    }
+                })
+            }, 500);
+        });
+
+        const webhookNotice = user.waitForRoomEvent<MessageEventContent>({
+            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');
+    });
+});

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);
+}

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,

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 { getBridgeApi } from "./util/bridge-api";
-import { WidgetApi } from "matrix-widget-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!, {
+        const bridgeApi = await getBridgeApi(testEnv.opts.config?.widgets?.publicUrl!, user);
-            requestOpenIDConnectToken: () => {
-                return user.getOpenIDConnectToken()
-            },
-        } as unknown as WidgetApi, {
-            getItem() { return null},
-            setItem() { },
-        } as unknown as Storage);
         expect(await bridgeApi.verify()).toEqual({
             "type": "widget",
             "userId": "@user:hookshot",

src/App/BridgeApp.ts

@@ -61,6 +61,7 @@ export async function start(config: BridgeConfig, registration: IAppserviceRegis
         storage.disconnect?.();
     });
     return {
+        appservice,
         bridgeApp,
         storage,
         listener,

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,
+                    payload: githubData.payload,
-                    signature: req.headers["x-hub-signature-256"] as string,
+                    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

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;