Add parameter for setting generic body size (#1090)

* Add parameter for setting generic body size to something other than Express default 100kb

* Add notes

* Fix payload size linting issue

* Formatting

* Add tests / cleanup etc

* lint

* Update docs

* lint

---------

Co-authored-by: Half-Shot <[email protected]>

Author: Arkaniad

changelog.d/1090.feature

@@ -0,0 +1 @@
+Add configuration option for generic webhooks to configure maximum request body size limit, increase default to 1mb from default 100kb.

config.sample.yml

@@ -119,6 +119,9 @@ listeners:
 #  requireExpiryTime: false
 #  maxExpiryTime: 30d
 #  includeHookBody: true
+#  payloadSizeLimit:
+#    # (Optional) How large may an incoming payload be. Specify in bytes or in a format like '1mb', '500kb', '10mb', etc.
+#    1mb
 
 #figma:
 #  # (Optional) Configure this to enable Figma support

spec/generic-hooks.spec.ts

@@ -25,6 +25,7 @@ describe("Inbound (Generic) Webhooks", () => {
           // Prefer to wait for complete as it reduces the concurrency of the test.
           waitForComplete: true,
           urlPrefix: `http://localhost:${webhooksPort}`,
+          payloadSizeLimit: "10mb",
         },
         listeners: [
           {
@@ -170,4 +171,37 @@ describe("Inbound (Generic) Webhooks", () => {
       (await expectedMsg).data.content["uk.half-shot.hookshot.webhook_data"],
     ).toBeUndefined();
   });
+
+  test("should handle an incoming request with a larger body", async () => {
+    const user = testEnv.getUser("user");
+    const roomId = await user.createRoom({ name: "My Test Webhooks room" });
+    const okMsg = user.waitForRoomEvent({
+      eventType: "m.room.message",
+      sender: testEnv.botMxid,
+      roomId,
+    });
+    const url = await createInboundConnection(user, testEnv.botMxid, roomId);
+    expect((await okMsg).data.content.body).toEqual(
+      "Room configured to bridge webhooks. See admin room for secret url.",
+    );
+
+    const expectedMsg = user.waitForRoomEvent({
+      eventType: "m.room.message",
+      sender: testEnv.botMxid,
+      roomId,
+    });
+    const body = Array.from({ length: 1024 * 1024 * 10 }).join("a");
+    const req = await fetch(url, {
+      method: "PUT",
+      body: body,
+    });
+    expect(req.status).toEqual(200);
+    expect(await req.json()).toEqual({ ok: true });
+    const resultMsg = await expectedMsg;
+    expect(resultMsg.data.content.body).toBeDefined();
+    expect(resultMsg.data.content.formatted_body).toBeUndefined();
+    expect(
+      resultMsg.data.content["uk.half-shot.hookshot.webhook_data"],
+    ).toBeUndefined();
+  });
 });

src/Connections/GenericHook.ts

@@ -99,6 +99,10 @@ const WARN_AT_EXPIRY_MS = 3 * 24 * 60 * 60 * 1000;
 const MIN_EXPIRY_MS = 60 * 60 * 1000;
 const CHECK_EXPIRY_MS = 15 * 60 * 1000;
 
+// This is a bit lower than the max size an event can be in Matrix.
+// > The complete event MUST NOT be larger than 65536 bytes, when formatted with the federation event format, including any signatures, and encoded as Canonical JSON.
+const MAX_EVENT_SIZE_BYTES = 65536 - 4 * 1024;
+
 const EXPIRY_NOTICE_MESSAGE =
   "The webhook **%NAME** will be expiring in %TIME.";
 
@@ -651,22 +655,46 @@ export class GenericHookConnection
       );
 
       // Matrix cannot handle float data, so make sure we parse out any floats.
-      const safeData =
+      let safeData =
         (this.state.includeHookBody ?? this.config.includeHookBody)
           ? GenericHookConnection.sanitiseObjectForMatrixJSON(data)
           : undefined;
 
+      // render can output redundant trailing newlines, so trim it.
+      content.html = content.html || md.render(content.plain).trim();
+
+      while (
+        content.plain.length +
+          (content.html ?? "").length +
+          (safeData ? JSON.stringify(safeData) : "").length >
+        MAX_EVENT_SIZE_BYTES
+      ) {
+        // We're oversize, so start trimming the event down.
+        if (safeData) {
+          // Start by trimming down the webhook data
+          safeData = undefined;
+          continue;
+        }
+        if (content.html) {
+          // Then remove the HTML
+          content.html = undefined;
+          continue;
+        }
+        // Finally, trim right down. Ensure we account for double width.
+        content.plain = content.plain.slice(0, MAX_EVENT_SIZE_BYTES / 2) + "…";
+        break;
+      }
+
       await this.messageClient.sendMatrixMessage(
         this.roomId,
         {
           msgtype: content.msgtype || "m.notice",
           body: content.plain,
-          // render can output redundant trailing newlines, so trim it.
-          formatted_body: content.html || md.render(content.plain).trim(),
+          ...(content.html ? { formatted_body: content.html } : undefined),
           ...(content.mentions
             ? { "m.mentions": content.mentions }
             : undefined),
-          format: "org.matrix.custom.html",
+          ...(content.html ? { format: "org.matrix.custom.html" } : undefined),
           ...(safeData
             ? { "uk.half-shot.hookshot.webhook_data": safeData }
             : undefined),

src/Webhooks.ts

@@ -63,6 +63,7 @@ export class Webhooks extends EventEmitter {
           this.queue,
           false,
           this.config.generic.enableHttpGet,
+          this.config.generic.payloadSizeLimit,
         ).getRouter(),
       );
       // TODO: Remove old deprecated endpoint
@@ -71,6 +72,7 @@ export class Webhooks extends EventEmitter {
           this.queue,
           true,
           this.config.generic.enableHttpGet,
+          this.config.generic.payloadSizeLimit,
         ).getRouter(),
       );
     }

src/config/Defaults.ts

@@ -125,6 +125,7 @@ export const DefaultConfigRoot: BridgeConfigRoot = {
     waitForComplete: false,
     maxExpiryTime: "30d",
     sendExpiryNotice: false,
+    payloadSizeLimit: "1mb",
   },
   figma: {
     publicUrl: `${hookshotWebhooksUrl}/hookshot/`,

src/config/sections/GenericHooks.ts

@@ -1,12 +1,18 @@
 import { GenericHookServiceConfig } from "../../Connections";
 import { ConfigError } from "../../Errors";
-import { hideKey } from "../Decorators";
+import { configKey, hideKey } from "../Decorators";
 const parseDurationImport = import("parse-duration");
 
 function makePrefixedUrl(urlString: string): URL {
   return new URL(urlString.endsWith("/") ? urlString : urlString + "/");
 }
 
+function validatePayloadSizeLimit(sizeLimit: string): boolean {
+  // Validate format like "1mb", "500kb", "10mb", etc.
+  const sizePattern = /^\d+(\.\d+)?(kb|mb|gb)$/i;
+  return sizePattern.test(sizeLimit);
+}
+
 export interface BridgeGenericWebhooksConfigYAML {
   enabled: boolean;
   urlPrefix: string;
@@ -19,6 +25,7 @@ export interface BridgeGenericWebhooksConfigYAML {
   sendExpiryNotice?: boolean;
   requireExpiryTime?: boolean;
   includeHookBody?: boolean;
+  payloadSizeLimit?: string | number;
 }
 
 export class BridgeConfigGenericWebhooks {
@@ -41,6 +48,11 @@ export class BridgeConfigGenericWebhooks {
   // Public facing value for config generator
   public readonly maxExpiryTime?: string;
   public readonly includeHookBody: boolean;
+  @configKey(
+    "How large may an incoming payload be. Specify in bytes or in a format like '1mb', '500kb', '10mb', etc.",
+    true,
+  )
+  public readonly payloadSizeLimit: number | string;
 
   constructor(yaml: BridgeGenericWebhooksConfigYAML) {
     this.enabled = yaml.enabled || false;
@@ -49,6 +61,40 @@ export class BridgeConfigGenericWebhooks {
     this.sendExpiryNotice = yaml.sendExpiryNotice || false;
     this.requireExpiryTime = yaml.requireExpiryTime || false;
     this.includeHookBody = yaml.includeHookBody ?? true;
+
+    // Set default payload size limit to 1mb (safer than 10mb, larger than 100kb)
+    const payloadSizeLimitYaml =
+      yaml.payloadSizeLimit === undefined ? "1mb" : yaml.payloadSizeLimit;
+
+    if (typeof payloadSizeLimitYaml === "number") {
+      if (
+        payloadSizeLimitYaml < 1 ||
+        Number.isNaN(payloadSizeLimitYaml) ||
+        !Number.isInteger(payloadSizeLimitYaml)
+      ) {
+        throw new ConfigError(
+          "generic.payloadSizeLimit",
+          "must a positive integer (or in a format like '1mb', '500kb', '10mb', etc.)",
+        );
+      }
+      // Bytes
+      this.payloadSizeLimit = payloadSizeLimitYaml;
+    } else if (typeof payloadSizeLimitYaml === "string") {
+      // Validate the payload size limit format
+      if (!validatePayloadSizeLimit(payloadSizeLimitYaml)) {
+        throw new ConfigError(
+          "generic.payloadSizeLimit",
+          "must be in format like '1mb', '500kb', '10mb', etc.",
+        );
+      }
+      this.payloadSizeLimit = payloadSizeLimitYaml;
+    } else {
+      throw new ConfigError(
+        "generic.payloadSizeLimit",
+        "must be a string or a number",
+      );
+    }
+
     try {
       this.parsedUrlPrefix = makePrefixedUrl(yaml.urlPrefix);
       this.urlPrefix = () => {

src/generic/Router.ts

@@ -15,6 +15,7 @@ export class GenericWebhooksRouter {
     private readonly queue: MessageQueue,
     private readonly deprecatedPath = false,
     private readonly allowGet: boolean,
+    private readonly payloadSizeLimit: string | number,
   ) {}
 
   private onWebhook(
@@ -92,28 +93,32 @@ 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;
+  private xmlHandler = (req: Request, res: Response, next: NextFunction) => {
+    express.text({ type: ["*/xml", "+xml"], limit: this.payloadSizeLimit })(
+      req,
+      res,
+      (err: any) => {
+        if (err) {
+          next(err);
+          return;
+        }
+        if (typeof req.body !== "string") {
           next();
-        })
-        .catch((e) => {
-          res.statusCode = 400;
-          next(e);
-        });
-    });
-  }
+          return;
+        }
+        xml
+          .parseStringPromise(req.body)
+          .then((xmlResult) => {
+            req.body = xmlResult;
+            next();
+          })
+          .catch((e) => {
+            res.statusCode = 400;
+            next(e);
+          });
+      },
+    );
+  };
 
   public getRouter() {
     const router = Router();
@@ -130,10 +135,10 @@ export class GenericWebhooksRouter {
         xFrameOptions: { action: "deny" },
         crossOriginResourcePolicy: { policy: "same-site" },
       }),
-      GenericWebhooksRouter.xmlHandler,
-      express.urlencoded({ extended: false }),
-      express.json(),
-      express.text({ type: "text/*" }),
+      this.xmlHandler,
+      express.urlencoded({ extended: false, limit: this.payloadSizeLimit }),
+      express.json({ limit: this.payloadSizeLimit }),
+      express.text({ type: "text/*", limit: this.payloadSizeLimit }),
       this.onWebhook.bind(this),
     );
     return router;

tests/config/sections/GenericHook.spec.ts

@@ -0,0 +1,57 @@
+import { expect } from "chai";
+import { BridgeConfigGenericWebhooks } from "../../../src/config/sections/GenericHooks";
+
+describe("config/sections/GenericHooks", () => {
+  describe("payloadSizeLimit", () => {
+    it("with an integer parameter", () => {
+      new BridgeConfigGenericWebhooks({
+        enabled: true,
+        urlPrefix: "https://example.org/foo",
+        payloadSizeLimit: 100,
+      });
+    });
+
+    it("throws with a negative integer", () => {
+      expect(
+        () =>
+          new BridgeConfigGenericWebhooks({
+            enabled: true,
+            urlPrefix: "https://example.org/foo",
+            payloadSizeLimit: -1,
+          }),
+      ).to.throw();
+    });
+
+    it("throws with a NaN integer", () => {
+      expect(
+        () =>
+          new BridgeConfigGenericWebhooks({
+            enabled: true,
+            urlPrefix: "https://example.org/foo",
+            payloadSizeLimit: NaN,
+          }),
+      ).to.throw();
+    });
+
+    it("throws with a float", () => {
+      expect(
+        () =>
+          new BridgeConfigGenericWebhooks({
+            enabled: true,
+            urlPrefix: "https://example.org/foo",
+            payloadSizeLimit: 50.5,
+          }),
+      ).to.throw();
+    });
+
+    for (const payloadSizeLimit of ["1mb", "1kb", "1gb"]) {
+      it(`with an string format parameter ${payloadSizeLimit}`, () => {
+        new BridgeConfigGenericWebhooks({
+          enabled: true,
+          urlPrefix: "https://example.org/foo",
+          payloadSizeLimit,
+        });
+      });
+    }
+  });
+});