Let's use getRequestFn/setRequestFn to keep errors readable

src/Mjolnir.ts

@@ -42,7 +42,6 @@ import { EventRedactionQueue, RedactUserInRoom } from "./queues/EventRedactionQu
 import * as htmlEscape from "escape-html";
 import { ReportManager } from "./report/ReportManager";
 import { WebAPIs } from "./webapis/WebAPIs";
-import { isClientWithSanerExceptions } from "./utils";
 
 export const STATE_NOT_STARTED = "not_started";
 export const STATE_CHECKING_PERMISSIONS = "checking_permissions";
@@ -157,10 +156,6 @@ export class Mjolnir {
         public readonly protectedRooms: { [roomId: string]: string },
         private banLists: BanList[],
     ) {
-        if (!isClientWithSanerExceptions(client)) {
-            throw new Error("Internal error: this is a MatrixClient without sane exceptions");
-        }
-
         this.explicitlyProtectedRoomIds = Object.keys(this.protectedRooms);
 
         for (const reason of config.automaticallyRedactForReasons) {

src/index.ts

@@ -27,7 +27,7 @@ import config from "./config";
 import { logMessage } from "./LogProxy";
 import { Healthz } from "./health/healthz";
 import { Mjolnir } from "./Mjolnir";
-import { makeClientWithSanerExceptions } from "./utils";
+import { patchMatrixClientForConciseExceptions } from "./utils";
 
 config.RUNTIME = {};
 
@@ -52,8 +52,7 @@ if (config.health.healthz.enabled) {
     } else {
         client = new MatrixClient(config.homeserverUrl, config.accessToken, storage);
     }
-    client = makeClientWithSanerExceptions(client);
+    patchMatrixClientForConciseExceptions();
-
     config.RUNTIME.client = client;
 
     let bot = await Mjolnir.setupMjolnirFromConfig(client);

src/report/ReportManager.ts

@@ -22,6 +22,7 @@ import { JSDOM } from 'jsdom';
 
 import config from "../config";
 import { Mjolnir } from "../Mjolnir";
+import { patchMatrixClientForConciseExceptions } from "../utils";
 
 /// Regexp, used to extract the action label from an action reaction
 /// such as `⚽ Kick user @foobar:localhost from room [kick-user]`.
@@ -875,6 +876,7 @@ class DisplayManager {
                 if (!await action.canExecute(this.owner, report, moderationRoomId)) {
                     continue;
                 }
+                patchMatrixClientForConciseExceptions();
                 await this.owner.mjolnir.client.sendEvent(config.managementRoom, "m.reaction", {
                     "m.relates_to": {
                         "rel_type": "m.annotation",

src/utils.ts

@@ -23,7 +23,9 @@ import {
     MessageType,
     Permalinks,
     TextualMessageEventContent,
-    UserID
+    UserID,
+    getRequestFn,
+    setRequestFn,
 } from "matrix-bot-sdk";
 import { logMessage } from "./LogProxy";
 import config from "./config";
@@ -203,68 +205,10 @@ export async function replaceRoomIdsWithPills(client: MatrixClient, text: string
     return content;
 }
 
-/**
+let isMatrixClientPatchedForConciseExceptions = false;
- * Utility function: a wrapper for `MatrixClient.doRequest` that makes sure that
- * we never throw an `IncomingMessage`.
- *
- * @param client The client to use for the request.
- * @param args The args to pass to the client.
- * @returns As `client.doRequest(...args)` but with `IncomingMessage` errors wrapped
- * as instances of `Error`.
- */
-async function doRequestReplacement(client: MatrixClient, args: any[]): Promise<any> {
-    try {
-        return await client.doRequest.apply(client, args);
-    } catch (ex) {
-        if (!(ex instanceof IncomingMessage)) {
-            // In most cases, we're happy with the result.
-            throw ex;
-        }
-        // However, MatrixClient has a tendency of throwing
-        // instances of `IncomingMessage` instead of instances
-        // of `Error`. The former take ~800 lines of log and
-        // provide no stack trace, which makes them typically
-        // useless.
-        let method: string | null = null;
-        let path = '';
-        let body: string | null = null;
-        if (ex.method) {
-            method = ex.method;
-        }
-        if (ex.url) {
-            path = ex.url;
-        }
-        if ("req" in ex && (ex as any).req instanceof ClientRequest) {
-            if (!method) {
-                method = (ex as any).req.method;
-            }
-            if (!path) {
-                path = (ex as any).req.path;
-            }
-        }
-        if ("body" in ex) {
-            body = JSON.stringify((ex as any).body);
-        }
-        let error = new Error(`Error during MatrixClient request ${method} ${path}: ${ex.statusCode} ${ex.statusMessage} -- ${body}`);
-        throw error;
-    }
-}
-
-
-// A key used internally to determine whether a `MatrixClient` has been monkey patched
-// to return saner exceptions.
-const CLIENT_WITH_SANER_EXCEPTION = Symbol("_monkeyPatchmakeClientWithSanerExceptions");
 
 /**
- * @returns `true` if a `MatrixClient` has been monkey patched to return
+ * Patch `MatrixClient` into something that throws concise exceptions.
- * saner exceptions.
- */
-export function isClientWithSanerExceptions(client: MatrixClient): boolean {
-    return CLIENT_WITH_SANER_EXCEPTION in client;
-}
-
-/**
- * Wrap a `MatrixClient` into something that throws sane exceptions.
  *
  * By default, instances of `MatrixClient` throw instances of `IncomingMessage`
  * in case of many errors. Unfortunately, these instances are unusable:
@@ -273,31 +217,72 @@ export function isClientWithSanerExceptions(client: MatrixClient): boolean {
  * - there is no error message;
  * - they offer no stack.
  *
- * This method converts a `MatrixClient` that may throw `IncomingMessage` into
+ * This method configures `MatrixClient` to ensure that methods that may throw
- * a `MatrixClient` that instead throws more reasonable insetances of `Error`.
+ * instead throws more reasonable insetances of `Error`.
  */
-export function makeClientWithSanerExceptions(client: MatrixClient): MatrixClient {
+export function patchMatrixClientForConciseExceptions() {
-    let result = new Proxy(client, {
+    if (isMatrixClientPatchedForConciseExceptions) {
-        has: function (obj, key): boolean {
+        return;
-            return key === CLIENT_WITH_SANER_EXCEPTION
+    }
-                || key in client;
+    let originalRequestFn = getRequestFn();
-        },
+    setRequestFn((params, cb) => {
-        get: function (obj, key) {
+        originalRequestFn(params, function conciseExceptionRequestFn(err, response, resBody) {
-            if (key === "doRequest") {
+            if (!err && (response.statusCode < 200 || response.statusCode >= 300)) {
-                // Intercept `doRequest`.
+                // Normally, converting HTTP Errors into rejections is done by the caller
-                return (...args) => doRequestReplacement(client, args);
+                // of `requestFn` within matrix-bot-sdk. However, this always ends up rejecting
+                // with an `IncomingMessage` - exactly what we wish to avoid here.
+                err = response;
+
+                // Safety note: In the calling code within matrix-bot-sdk, if we return
+                // an IncomingMessage as an error, we end up logging an unredacted response,
+                // which may include tokens, passwords, etc. This could be a grave privacy
+                // leak. The matrix-bot-sdk typically handles this by sanitizing the data
+                // before logging it but, by converting the HTTP Error into a rejection
+                // earlier than expected by the matrix-bot-sdk, we skip this step of
+                // sanitization.
+                //
+                // However, since the error we're creating is an `IncomingMessage`, we
+                // rewrite it into an `Error` ourselves in this function. Our `Error`
+                // is even more sanitized (we only include the URL, HTTP method and
+                // the error response) so we are NOT causing a privacy leak.
+                if (!(err instanceof IncomingMessage)) {
+                    // Safety check.
+                    throw new TypeError("Internal error: at this stage, the error should be an IncomingMessage");
+                }
             }
-            if (key === "_monkeyPatchmakeClientWithSanerExceptions") {
+            if (!(err instanceof IncomingMessage)) {
-                return true;
+                // In most cases, we're happy with the result.
+                return cb(err, response, resBody);
             }
-            let value = obj[key];
+            // However, MatrixClient has a tendency of throwing
-            if (!(typeof value === "function")) {
+            // instances of `IncomingMessage` instead of instances
-                // We're only interested in methods.
+            // of `Error`. The former take ~800 lines of log and
-                return value;
+            // provide no stack trace, which makes them typically
+            // useless.
+            let method: string | null = null;
+            let path = '';
+            let body: string | null = null;
+            if (err.method) {
+                method = err.method;
             }
-            // Make sure that methods use our intercepted `doRequestReplacement`.
+            if (err.url) {
-            return value.bind(result);
+                path = err.url;
-        }
+            }
+            if ("req" in err && (err as any).req instanceof ClientRequest) {
+                if (!method) {
+                    method = (err as any).req.method;
+                }
+                if (!path) {
+                    path = (err as any).req.path;
+                }
+            }
+            if ("body" in err) {
+                body = JSON.stringify((err as any).body);
+            }
+            let error = new Error(`Error during MatrixClient request ${method} ${path}: ${err.statusCode} ${err.statusMessage} -- ${body}`);
+            return cb(error, response, resBody);
+        })
     });
-    return result;
+    isMatrixClientPatchedForConciseExceptions = true;
-}
+}
+

src/webapis/WebAPIs.ts

@@ -21,7 +21,6 @@ import { MatrixClient } from "matrix-bot-sdk";
 
 import config from "../config";
 import { ReportManager } from "../report/ReportManager";
-import { makeClientWithSanerExceptions } from "../utils";
 
 /**
  * A common prefix for all web-exposed APIs.
@@ -131,7 +130,7 @@ export class WebAPIs {
                 //    so we are not extending the abilities of Mjölnir
                 // 3. We are avoiding the use of the Synapse Admin API to ensure that
                 //    this feature can work with all homeservers, not just Synapse.
-                let reporterClient = makeClientWithSanerExceptions(new MatrixClient(config.rawHomeserverUrl, accessToken));
+                let reporterClient = new MatrixClient(config.rawHomeserverUrl, accessToken);
                 reporterClient.start = () => {
                     throw new Error("We MUST NEVER call start on the reporter client");
                 };

test/integration/clientHelper.ts

@@ -2,7 +2,6 @@ import axios from "axios";
 import { HmacSHA1 } from "crypto-js";
 import { extractRequestError, LogService, MatrixClient, MemoryStorageProvider, PantalaimonClient } from "matrix-bot-sdk";
 import config from "../../src/config";
-import { makeClientWithSanerExceptions } from "../../src/utils";
 
 /**
  * Register a user using the synapse admin api that requires the use of a registration secret rather than an admin user.
@@ -68,7 +67,7 @@ export async function registerNewTestUser(isAdmin: boolean, label: string = "")
 export async function newTestUser(isAdmin: boolean = false, label: string = ""): Promise<MatrixClient> {
     const username = await registerNewTestUser(isAdmin, label);
     const pantalaimon = new PantalaimonClient(config.homeserverUrl, new MemoryStorageProvider());
-    return makeClientWithSanerExceptions(await pantalaimon.createClientWithCredentials(username, username));
+    return await pantalaimon.createClientWithCredentials(username, username);
 }
 
 /**

test/integration/mjolnirSetupUtils.ts

@@ -24,7 +24,7 @@ import {
 import { Mjolnir}  from '../../src/Mjolnir';
 import config from "../../src/config";
 import { registerUser } from "./clientHelper";
-import { makeClientWithSanerExceptions } from "../../src/utils";
+import { patchMatrixClientForConciseExceptions } from "../../src/utils";
 
 /**
  * Ensures that a room exists with the alias, if it does not exist we create it.
@@ -52,11 +52,11 @@ async function configureMjolnir() {
         await registerUser('mjolnir', 'mjolnir', 'mjolnir', true)
     } catch (e) {
         if (e.isAxiosError) {
+            console.log('Received error while registering', e.response.data || e.response);
             if (e.response.data && e.response.data.errcode === 'M_USER_IN_USE') {
                 console.log('mjolnir already registered, skipping');
                 return;
             }
-            console.log('Received error while registering', e);
         }
         throw e;
     };
@@ -80,8 +80,8 @@ export async function makeMjolnir(): Promise<Mjolnir> {
     LogService.setLevel(LogLevel.fromString(config.logLevel, LogLevel.DEBUG));
     LogService.info("test/mjolnirSetupUtils", "Starting bot...");
     const pantalaimon = new PantalaimonClient(config.homeserverUrl, new MemoryStorageProvider());
-    let client = await pantalaimon.createClientWithCredentials(config.pantalaimon.username, config.pantalaimon.password);
+    const client = await pantalaimon.createClientWithCredentials(config.pantalaimon.username, config.pantalaimon.password);
-    client = makeClientWithSanerExceptions(client);
+    patchMatrixClientForConciseExceptions();
     await ensureAliasedRoomExists(client, config.managementRoom);
     let mjolnir = await Mjolnir.setupMjolnirFromConfig(client);
     globalClient = client;