From 7796f104cb6de4a7cab57b725f8b573299150312 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Wed, 13 Jul 2022 16:14:21 +0100 Subject: [PATCH] Add XML parser for generic webhook payloads (#410) * Add support for decoding XML for webhooks * Ensure all endpoints use the error middleware * Dependencies * changelog * Describe form data in the documentation * Reorder --- changelog.d/410.feature | 1 + docs/setup/webhooks.md | 18 +++++++++++++++--- package.json | 2 ++ src/ListenerService.ts | 5 ++++- src/generic/Router.ts | 25 +++++++++++++++++++++++-- src/provisioning/provisioner.ts | 5 ++--- yarn.lock | 9 ++++++++- 7 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 changelog.d/410.feature diff --git a/changelog.d/410.feature b/changelog.d/410.feature new file mode 100644 index 00000000..72b72191 --- /dev/null +++ b/changelog.d/410.feature @@ -0,0 +1 @@ +Added support for decoding XML payloads when handling generic webhooks. \ No newline at end of file diff --git a/docs/setup/webhooks.md b/docs/setup/webhooks.md index 2e284d2f..a1fbb7f5 100644 --- a/docs/setup/webhooks.md +++ b/docs/setup/webhooks.md @@ -64,7 +64,7 @@ Hookshot handles `POST` and `PUT` HTTP requests by default. Hookshot handles HTTP requests with a method of `GET`, `POST` or `PUT`. -If the request is a `GET` request, the query parameters are assumed to be the body. Otherwise, the body of the request should be a JSON payload. +If the request is a `GET` request, the query parameters are assumed to be the body. Otherwise, the body of the request should be a supported payload. If the body contains a `text` key, then that key will be used as a message body in Matrix (aka `body`). This text will be automatically converted from Markdown to HTML (unless a `html` key is provided.). @@ -73,7 +73,19 @@ If the body contains a `html` key, then that key will be used as the HTML messag If the body *also* contains a `username` key, then the message will be prepended by the given username. This will be prepended to both `text` and `html`. -If the body does NOT contain a `text` field, the full JSON payload will be sent to the room. This can be adapted into a message by creating a **JavaScript transformation function**. +If the body does NOT contain a `text` field, the full payload will be sent to the room. This can be adapted into a message by creating a **JavaScript transformation function**. + +### Payload formats + +If the request is a `POST`/`PUT`, the body of the request will be decoded and stored inside the event. Currently, Hookshot supports: + +- XML, when the `Content-Type` header ends in `/xml` or `+xml`. +- Web form data, when the `Content-Type` header is `application/x-www-form-urlencoded`. +- JSON, when the `Content-Type` header is `application/json`. +- Text, when the `Content-Type` header begins with `text/`. + +Decoding is done in the order given above. E.g. `text/xml` would be parsed as XML. Any formats not described above are not +decoded. ### GET requests @@ -113,7 +125,7 @@ The script string should be set within the state event under the `transformation Transformation scripts have a versioned API. You can check the version of the API that the hookshot instance supports at runtime by checking the `HookshotApiVersion` variable. If the variable is undefined, it should be considered `v1`. -The execution environment will contain a `data` variable, which will be the body of the incoming request (JSON will be parsed into an `Object`). +The execution environment will contain a `data` variable, which will be the body of the incoming request (see [Payload formats](#payload-formats)). Scripts are executed synchronously and expect the `result` variable to be set. If the script contains errors or is otherwise unable to work, the bridge will send an error to the room. You can check the logs of the bridge diff --git a/package.json b/package.json index 7774de46..0a737b19 100644 --- a/package.json +++ b/package.json @@ -62,6 +62,7 @@ "uuid": "^8.3.2", "vm2": "^3.9.6", "winston": "^3.3.3", + "xml2js": "^0.4.23", "yaml": "^1.10.2" }, "devDependencies": { @@ -82,6 +83,7 @@ "@types/node": "^14", "@types/node-emoji": "^1.8.1", "@types/uuid": "^8.3.3", + "@types/xml2js": "^0.4.11", "@typescript-eslint/eslint-plugin": "^5.4.0", "@typescript-eslint/parser": "^5.4.0", "@uiw/react-codemirror": "^4.5.3", diff --git a/src/ListenerService.ts b/src/ListenerService.ts index 44502f8c..4d132012 100644 --- a/src/ListenerService.ts +++ b/src/ListenerService.ts @@ -1,6 +1,7 @@ import { Server } from "http"; -import { Application, default as expressApp, Router } from "express"; +import { Application, default as expressApp, NextFunction, Request, Response, Router } from "express"; import LogWrapper from "./LogWrapper"; +import { errorMiddleware } from "./api"; // Appserices can't be handled yet because the bot-sdk maintains control of it. // See https://github.com/turt2live/matrix-bot-sdk/issues/191 @@ -71,6 +72,8 @@ export class ListenerService { } const addr = listener.config.bindAddress || "127.0.0.1"; listener.server = listener.app.listen(listener.config.port, addr); + // Always include the error handler + listener.app.use((err: unknown, req: Request, res: Response, next: NextFunction) => errorMiddleware(log)(err, req, res, next)); log.info(`Listening on http://${addr}:${listener.config.port} for ${listener.config.resources.join(', ')}`) } } diff --git a/src/generic/Router.ts b/src/generic/Router.ts index 32da431d..3c505420 100644 --- a/src/generic/Router.ts +++ b/src/generic/Router.ts @@ -3,6 +3,7 @@ import express, { NextFunction, Request, Response, Router } from "express"; import LogWrapper from "../LogWrapper"; import { ApiError, ErrCode } from "../api"; import { GenericWebhookEvent, GenericWebhookEventResult } from "./types"; +import * as xml from "xml2js"; const WEBHOOK_RESPONSE_TIMEOUT = 5000; @@ -11,7 +12,6 @@ export class GenericWebhooksRouter { constructor(private readonly queue: MessageQueue, private readonly deprecatedPath = false, private readonly allowGet: boolean) { } private onWebhook(req: Request<{hookId: string}, unknown, unknown, unknown>, res: Response<{ok: true}|{ok: false, error: string}>, next: NextFunction) { - if (req.method === "GET" && !this.allowGet) { throw new ApiError("Invalid Method. Expecting PUT or POST", ErrCode.MethodNotAllowed); } @@ -55,13 +55,34 @@ export class GenericWebhooksRouter { }); } + private static xmlHandler(req: Request, res: Response, next: NextFunction) { + express.text({ type: ["*/xml", "+xml"] })(req, res, (err) => { + if (err) { + next(err); + return; + } + if (typeof req.body !== 'string') { + next(); + return; + } + xml.parseStringPromise(req.body).then(xmlResult => { + req.body = xmlResult; + next(); + }).catch(e => { + res.statusCode = 400; + next(e); + }); + }); + } + public getRouter() { const router = Router(); router.all( '/:hookId', - express.text({ type: 'text/*'}), + GenericWebhooksRouter.xmlHandler, express.urlencoded({ extended: false }), express.json(), + express.text({ type: 'text/*'}), this.onWebhook.bind(this), ); return router; diff --git a/src/provisioning/provisioner.ts b/src/provisioning/provisioner.ts index bfd6d055..3ac786f0 100644 --- a/src/provisioning/provisioner.ts +++ b/src/provisioning/provisioner.ts @@ -3,8 +3,8 @@ import { Router, default as express, NextFunction, Request, Response } from "exp import { ConnectionManager } from "../ConnectionManager"; import LogWrapper from "../LogWrapper"; import { assertUserPermissionsInRoom, GetConnectionsResponseItem, GetConnectionTypeResponseItem } from "./api"; -import { ApiError, ErrCode, errorMiddleware } from "../api"; -import { Intent, MembershipEventContent, PowerLevelsEvent, PowerLevelsEventContent } from "matrix-bot-sdk"; +import { ApiError, ErrCode } from "../api"; +import { Intent } from "matrix-bot-sdk"; import Metrics from "../Metrics"; const log = new LogWrapper("Provisioner"); @@ -70,7 +70,6 @@ export class Provisioner { (...args) => this.checkUserPermission("write", ...args), this.deleteConnection.bind(this), ); - this.expressRouter.use((err: unknown, req: Request, res: Response, next: NextFunction) => errorMiddleware(log)(err, req, res, next)); } private checkAuth(req: Request, _res: Response, next: NextFunction) { diff --git a/yarn.lock b/yarn.lock index 84cebc4e..642bb036 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1299,6 +1299,13 @@ resolved "https://registry.yarnpkg.com/@types/uuid/-/uuid-8.3.3.tgz#c6a60686d953dbd1b1d45e66f4ecdbd5d471b4d0" integrity sha512-0LbEEx1zxrYB3pgpd1M5lEhLcXjKJnYghvhTRgaBeUivLHMDM1TzF3IJ6hXU2+8uA4Xz+5BA63mtZo5DjVT8iA== +"@types/xml2js@^0.4.11": + version "0.4.11" + resolved "https://registry.yarnpkg.com/@types/xml2js/-/xml2js-0.4.11.tgz#bf46a84ecc12c41159a7bd9cf51ae84129af0e79" + integrity sha512-JdigeAKmCyoJUiQljjr7tQG3if9NkqGUgwEUqBvV0N7LM4HyQk7UXCnusRa1lnvXAEYJ8mw8GtZWioagNztOwA== + dependencies: + "@types/node" "*" + "@typescript-eslint/eslint-plugin@^5.4.0": version "5.8.0" resolved "https://registry.yarnpkg.com/@typescript-eslint/eslint-plugin/-/eslint-plugin-5.8.0.tgz#52cd9305ceef98a5333f9492d519e6c6c7fe7d43" @@ -5528,7 +5535,7 @@ wrappy@1: resolved "https://registry.yarnpkg.com/wrappy/-/wrappy-1.0.2.tgz#b5243d8f3ec1aa35f1364605bc0d1036e30ab69f" integrity sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8= -xml2js@^0.4.19: +xml2js@^0.4.19, xml2js@^0.4.23: version "0.4.23" resolved "https://registry.yarnpkg.com/xml2js/-/xml2js-0.4.23.tgz#a0c69516752421eb2ac758ee4d4ccf58843eac66" integrity sha512-ySPiMjM0+pLDftHgXY4By0uswI3SPKLDw/i3UXbnO8M/p28zqexCUoPmQFrYD+/1BzhGJSs2i1ERWKJAtiLrug==