Move webhooks from / to /webhook (#227)

* Move webhooks from / to /webhooks

* fix doc

* Update docs

* Fix tests

* changelog

* Update sample config

* fixup config

* fix changelog

* commit config changes

changelog.d/227.feature

@@ -0,0 +1 @@
+Generic webhooks now listen for incoming hooks on `/webhook`. Existing setups using `/` will continue to work, but should be migrated where possible. See [the documentation](https://matrix-org.github.io/matrix-hookshot/setup/webhooks.html#configuration) for more information.

config.sample.yml

@@ -52,12 +52,14 @@ jira:
     client_secret: bar
     redirect_uri: https://example.com/bridge_oauth/
 generic:
-  # (Optional) Support for generic webhook events. `allowJsTransformationFunctions` will allow users to write short transformation snippets in code, and thus is unsafe in untrusted environments
+  # (Optional) Support for generic webhook events.
+  #'allowJsTransformationFunctions' will allow users to write short transformation snippets in code, and thus is unsafe in untrusted environments
+  #
   #
   enabled: false
-  urlPrefix: https://example.com/mywebhookspath/
+  urlPrefix: https://example.com/webhook/
   allowJsTransformationFunctions: false
-  userIdPrefix: webhooks_
+  waitForComplete: false
 figma:
   # (Optional) Configure this to enable Figma support
   #

docs/setup/webhooks.md

@@ -12,14 +12,40 @@ generic:
   enabled: true
   urlPrefix: https://example.com/mywebhookspath/
   allowJsTransformationFunctions: false
+  waitForComplete: false
+  # userIdPrefix: webhook_
 ```
 
+<section class="notice">
+Previous versions of the bridge listened for requests on `/` rather than `/webhook`. While this behaviour will continue to work,
+administators are advised to use `/webhook`.
+</section>
+
+The webhooks listener listens on the path `/webhook`.
+
 The bridge listens for incoming webhooks requests on the host and port provided in the [`listeners` config](../setup.md#listeners-configuration).
 
 `urlPrefix` describes the public facing URL of your webhook handler. For instance, if your load balancer redirected
-webhook requests from `https://example.com/mywebhookspath` to the bridge an example webhook URL would look like:
+webhook requests from `https://example.com/mywebhookspath` to the bridge (on `/webhook`), an example webhook URL would look like:
 `https://example.com/mywebhookspath/abcdef`.
 
+`waitForComplete` causes the bridge to wait until the webhook is processed before sending a response. Some services prefer you always
+respond with a 200 as soon as the webhook has entered processing (`false`) while others prefer to know if the resulting Matrix message
+has been sent (`true`). By default this is `false`.
+
+You may set a `userIdPrefix` to create a specific user for each new webhook connection in a room. For example, a connection with a name
+like `example` for a prefix of `webhook_` will create a user called `@webhook_example:example.com`. If you enable this option,
+you need to configure the user to be part of your registration file e.g.:
+
+```yaml
+# registration.yaml
+...
+namespaces:
+  users:
+    - regex: "@webhook_.+:example.com" # Where example.com is your domain name.
+      exclusive: true
+```
+
 ## Adding a webhook
 
 To add a webhook to your room:

src/Bridge.ts

@@ -20,7 +20,7 @@ import { MessageQueue, createMessageQueue } from "./MessageQueue";
 import { MessageSenderClient } from "./MatrixSender";
 import { NotifFilter, NotificationFilterStateContent } from "./NotificationFilters";
 import { NotificationProcessor } from "./NotificationsProcessor";
-import { NotificationsEnableEvent, NotificationsDisableEvent, GenericWebhookEvent } from "./Webhooks";
+import { NotificationsEnableEvent, NotificationsDisableEvent } from "./Webhooks";
 import { GitHubOAuthToken, GitHubOAuthTokenResponse, ProjectsGetResponseData } from "./Github/Types";
 import { RedisStorageProvider } from "./Stores/RedisStorageProvider";
 import { retry } from "./PromiseUtil";
@@ -40,6 +40,7 @@ import { SetupConnection } from "./Connections/SetupConnection";
 import { getAppservice } from "./appservice";
 import { JiraOAuthRequestCloud, JiraOAuthRequestOnPrem, JiraOAuthRequestResult } from "./Jira/OAuth";
 import { CLOUD_INSTANCE } from "./Jira/Client";
+import { GenericWebhookEvent, GenericWebhookEventResult } from "./generic/types";
 const log = new LogWrapper("Bridge");
 
 export class Bridge {
@@ -505,12 +506,42 @@ export class Bridge {
             });
 
         });
+        
+        this.queue.on<GenericWebhookEvent>("generic-webhook.event", async (msg) => {
+            const { data, messageId } = msg;
+            const connections = connManager.getConnectionsForGenericWebhook(data.hookId);
+            log.debug(`generic-webhook.event for ${connections.map(c => c.toString()).join(', ') || '[empty]'}`);
 
-        this.bindHandlerToQueue<GenericWebhookEvent, GenericHookConnection>(
-            "generic-webhook.event",
-            (data) => connManager.getConnectionsForGenericWebhook(data.hookId), 
-            (c, data) => c.onGenericHook(data.hookData),
-        );
+            if (!connections.length) {
+                await this.queue.push<GenericWebhookEventResult>({
+                    data: {notFound: true},
+                    sender: "Bridge",
+                    messageId: messageId,
+                    eventName: "response.generic-webhook.event",
+                });
+            }
+
+            connections.map(async (c, index) => {
+                // TODO: Support webhook responses to more than one room
+                if (index !== 0) {
+                    c.onGenericHook(data.hookData);
+                    return;
+                }
+                let successful: boolean|null = null;
+                if (this.config.generic?.waitForComplete) {
+                    successful = await c.onGenericHook(data.hookData);
+                }
+                await this.queue.push<GenericWebhookEventResult>({
+                    data: {successful},
+                    sender: "Bridge",
+                    messageId,
+                    eventName: "response.jira.oauth.response",
+                });
+                if (!this.config.generic?.waitForComplete) {
+                    c.onGenericHook(data.hookData);
+                }
+            });
+        });
 
         this.bindHandlerToQueue<FigmaEvent, FigmaFileConnection>(
             "figma.payload",
@@ -615,11 +646,11 @@ export class Bridge {
         this.queue.on<EventType>(event, (msg) => {
             const connections = connectionFetcher.bind(this)(msg.data);
             log.debug(`${event} for ${connections.map(c => c.toString()).join(', ') || '[empty]'}`);
-            connections.forEach(async (c) => {
+            connections.forEach(async (connection) => {
                 try {
-                    await handler(c, msg.data);
+                    await handler(connection, msg.data);
                 } catch (ex) {
-                    log.warn(`Connection ${c.toString()} failed to handle ${event}:`, ex);
+                    log.warn(`Connection ${connection.toString()} failed to handle ${event}:`, ex);
                 }
             })
         });

src/Config/Config.ts

@@ -168,6 +168,7 @@ export interface BridgeGenericWebhooksConfig {
     urlPrefix: string;
     userIdPrefix?: string;
     allowJsTransformationFunctions?: boolean;
+    waitForComplete?: boolean;
 }
 
 interface BridgeWidgetConfig {
@@ -262,7 +263,9 @@ export class BridgeConfig {
     public readonly gitlab?: BridgeConfigGitLab;
     @configKey("Configure this to enable Jira support. Only specify `url` if you are using a On Premise install (i.e. not atlassian.com)", true)
     public readonly jira?: BridgeConfigJira;
-    @configKey("Support for generic webhook events. `allowJsTransformationFunctions` will allow users to write short transformation snippets in code, and thus is unsafe in untrusted environments", true)
+    @configKey(`Support for generic webhook events.
+'allowJsTransformationFunctions' will allow users to write short transformation snippets in code, and thus is unsafe in untrusted environments
+`, true)
     public readonly generic?: BridgeGenericWebhooksConfig;
     @configKey("Configure this to enable Figma support", true)
     public readonly figma?: BridgeConfigFigma;

src/Config/Defaults.ts

@@ -78,9 +78,9 @@ export const DefaultConfig = new BridgeConfig({
     },
     generic: {
         enabled: false,
-        urlPrefix: "https://example.com/mywebhookspath/",
+        urlPrefix: "https://example.com/webhook/",
         allowJsTransformationFunctions: false,
-        userIdPrefix: "webhooks_",
+        waitForComplete: false,
     },
     figma: {
         publicUrl: "https://example.com/hookshot/",

src/Connections/GenericHook.ts

@@ -178,34 +178,35 @@ export class GenericHookConnection extends BaseConnection implements IConnection
         this.state = validatedConfig;
     }
 
-    public transformHookData(data: Record<string, unknown>|string): {plain: string, html?: string} {
+    public transformHookData(data: unknown): {plain: string, html?: string} {
         // Supported parameters https://developers.mattermost.com/integrate/incoming-webhooks/#parameters
         const msg: {plain: string, html?: string} = {plain: ""};
+        const safeData = typeof data === "object" && data !== null ? data as Record<string, unknown> : undefined;
         if (typeof data === "string") {
             return {plain: `Received webhook data: ${data}`};
-        } else if (typeof data.text === "string") {
-            msg.plain = data.text;
+        } else if (typeof safeData?.text === "string") {
+            msg.plain = safeData.text;
         } else {
             msg.plain = "Received webhook data:\n\n" + "```json\n\n" + JSON.stringify(data, null, 2) + "\n\n```";
             msg.html = `<p>Received webhook data:</p><p><pre><code class=\\"language-json\\">${JSON.stringify(data, null, 2)}</code></pre></p>`
         }
 
-        if (typeof data.html === "string") {
-            msg.html = data.html;
+        if (typeof safeData?.html === "string") {
+            msg.html = safeData.html;
         }
 
-        if (typeof data.username === "string") {
+        if (typeof safeData?.username === "string") {
             // Create a matrix user for this person
-            msg.plain = `**${data.username}**: ${msg.plain}`
+            msg.plain = `**${safeData.username}**: ${msg.plain}`
             if (msg.html) {
-                msg.html = `<strong>${data.username}</strong>: ${msg.html}`;
+                msg.html = `<strong>${safeData.username}</strong>: ${msg.html}`;
             }
         }
         // TODO: Transform Slackdown into markdown.
         return msg;
     }
 
-    public executeTransformationFunction(data: Record<string, unknown>): {plain: string, html?: string}|null {
+    public executeTransformationFunction(data: unknown): {plain: string, html?: string}|null {
         if (!this.transformationFunction) {
             throw Error('Transformation function not defined');
         }
@@ -250,9 +251,15 @@ export class GenericHookConnection extends BaseConnection implements IConnection
         }
     }
 
-    public async onGenericHook(data: Record<string, unknown>) {
+    /**
+     * Processes an incoming generic hook
+     * @param data Structured data. This may either be a string, or an object.
+     * @returns `true` if the webhook completed, or `false` if it failed to complete 
+     */
+    public async onGenericHook(data: unknown): Promise<boolean> {
         log.info(`onGenericHook ${this.roomId} ${this.hookId}`);
         let content: {plain: string, html?: string};
+        let success = true;
         if (!this.transformationFunction) {
             content = this.transformHookData(data);
         } else {
@@ -260,25 +267,27 @@ export class GenericHookConnection extends BaseConnection implements IConnection
                 const potentialContent = this.executeTransformationFunction(data);
                 if (potentialContent === null) {
                     // Explitly no action
-                    return;
+                    return true;
                 }
                 content = potentialContent;
             } catch (ex) {
                 log.warn(`Failed to run transformation function`, ex);
                 content = {plain: `Webhook received but failed to process via transformation function`};
+                success = false;
             }
         }
 
         const sender = this.getUserId();
         await this.ensureDisplayname();
 
-        return this.messageClient.sendMatrixMessage(this.roomId, {
+        await this.messageClient.sendMatrixMessage(this.roomId, {
             msgtype: "m.notice",
             body: content.plain,
             formatted_body: content.html || md.renderInline(content.plain),
             format: "org.matrix.custom.html",
             "uk.half-shot.hookshot.webhook_data": data,
         }, 'm.room.message', sender);
+        return success;
 
     }
 

src/Connections/SetupConnection.ts

@@ -11,6 +11,7 @@ import { v4 as uuid } from "uuid";
 import { BridgeConfig, BridgePermissionLevel } from "../Config/Config";
 import markdown from "markdown-it";
 import { FigmaFileConnection } from "./FigmaFileConnection";
+import { URL } from "url";
 const md = new markdown();
 
 /**

src/Webhooks.ts

@@ -13,14 +13,10 @@ import { OAuthRequest } from "./WebhookTypes";
 import { GitHubOAuthTokenResponse } from "./Github/Types";
 import Metrics from "./Metrics";
 import { FigmaWebhooksRouter } from "./figma/router";
+import { GenericWebhooksRouter } from "./generic/Router";
 
 const log = new LogWrapper("Webhooks");
 
-export interface GenericWebhookEvent {
-    hookData: Record<string, unknown>;
-    hookId: string;
-}
-
 export interface NotificationsEnableEvent {
     userId: string;
     roomId: string;
@@ -64,13 +60,11 @@ export class Webhooks extends EventEmitter {
         if (this.config.figma) {
             this.expressRouter.use('/figma', new FigmaWebhooksRouter(this.config.figma, this.queue).getRouter());
         }
-        this.expressRouter.all(
-            '/:hookId',
-            express.text({ type: 'text/*'}),
-            express.urlencoded({ extended: false }),
-            express.json(),
-            this.onGenericPayload.bind(this),
-        );
+        if (this.config.generic) {
+            this.expressRouter.use('/webhook', new GenericWebhooksRouter(this.queue).getRouter());
+            // TODO: Remove old deprecated endpoint
+            this.expressRouter.use(new GenericWebhooksRouter(this.queue, true).getRouter());
+        }
         this.expressRouter.use(express.json({
             verify: this.verifyRequest.bind(this),
         }));
@@ -122,32 +116,6 @@ export class Webhooks extends EventEmitter {
         }
     }
 
-    private onGenericPayload(req: Request, res: Response) {
-        if (!['PUT', 'GET', 'POST'].includes(req.method)) {
-            res.sendStatus(400).send({error: 'Wrong METHOD. Expecting PUT,GET,POST'});
-            return;
-        }
-
-        let body;
-        if (req.method === 'GET') {
-            body = req.query;
-        } else {
-            body = req.body;
-        }
-
-        res.sendStatus(200);
-        this.queue.push({
-            eventName: 'generic-webhook.event',
-            sender: "GithubWebhooks",
-            data: {
-                hookData: body,
-                hookId: req.params.hookId,
-            } as GenericWebhookEvent,
-        }).catch((err) => {
-            log.error(`Failed to emit payload: ${err}`);
-        });
-    }
-
     private onPayload(req: Request, res: Response) {
         try {
             let eventName: string|null = null;

src/generic/Router.ts

@@ -0,0 +1,64 @@
+import { MessageQueue } from "../MessageQueue";
+import express, { NextFunction, Request, Response, Router } from "express";
+import LogWrapper from "../LogWrapper";
+import { ApiError, ErrCode } from "../provisioning/api";
+import { GenericWebhookEvent, GenericWebhookEventResult } from "./types";
+
+const WEBHOOK_RESPONSE_TIMEOUT = 5000;
+
+const log = new LogWrapper('GenericWebhooksRouter');
+export class GenericWebhooksRouter {
+    constructor(private readonly queue: MessageQueue, private readonly deprecatedPath = false) { }
+
+    private onWebhook(req: Request<{hookId: string}, unknown, unknown, unknown>, res: Response<{ok: true}|{ok: false, error: string}>, next: NextFunction) {
+        if (!['PUT', 'GET', 'POST'].includes(req.method)) {
+            throw new ApiError("Wrong METHOD. Expecting PUT, GET or POST", ErrCode.MethodNotAllowed);
+        }
+    
+        let body;
+        if (req.method === 'GET') {
+            body = req.query;
+        } else {
+            body = req.body;
+        }
+    
+        this.queue.pushWait<GenericWebhookEvent, GenericWebhookEventResult>({
+            eventName: 'generic-webhook.event',
+            sender: "GithubWebhooks",
+            data: {
+                hookData: body,
+                hookId: req.params.hookId,
+            },
+        }, WEBHOOK_RESPONSE_TIMEOUT).then((response) => {
+            if (response.notFound) {
+                if (this.deprecatedPath) {
+                    // If the webhook wasn't found and we're on a deprecated path, ignore it.
+                    next();
+                    return;
+                }
+                res.status(404).send({ok: false, error: "Webhook not found"});
+            } else if (response.successful) {
+                res.status(200).send({ok: true});
+            } else if (response.successful === false) {
+                res.status(500).send({ok: false, error: "Failed to process webhook"});
+            } else {
+                res.status(202).send({ok: true});
+            }
+        }).catch((err) => {
+            log.error(`Failed to emit payload: ${err}`);
+            res.status(500).send({ok: false, error: "Failed to handle webhook"});
+        });
+    }
+
+    public getRouter() {
+        const router = Router();
+        router.all(
+            '/:hookId',
+            express.text({ type: 'text/*'}),
+            express.urlencoded({ extended: false }),
+            express.json(),
+            this.onWebhook.bind(this),
+        );
+        return router;
+    }
+}

src/generic/types.ts

@@ -0,0 +1,9 @@
+export interface GenericWebhookEvent {
+    hookData: unknown;
+    hookId: string;
+}
+
+export interface GenericWebhookEventResult {
+    successful?: boolean|null;
+    notFound?: boolean;
+}

src/provisioning/api.ts

@@ -59,6 +59,10 @@ export enum ErrCode {
      * A connection with similar configuration exists
      */
     ConflictingConnection =  "HS_CONFLICTING_CONNECTION",
+    /**
+     * The method used was invalid for this endpoint
+     */
+    MethodNotAllowed = "HS_METHOD_NOT_ALLOWED",
 }
 
 const ErrCodeToStatusCode: Record<ErrCode, number> = {
@@ -73,6 +77,7 @@ const ErrCodeToStatusCode: Record<ErrCode, number> = {
     HS_DISABLED_FEATURE: 500,
     HS_ADDITIONAL_ACTION_REQUIRED: 400,
     HS_CONFLICTING_CONNECTION: 409,
+    HS_METHOD_NOT_ALLOWED: 405,
 }
 
 export class ApiError extends Error {