diff --git a/src/Mjolnir.ts b/src/Mjolnir.ts index 417e532..6ceef83 100644 --- a/src/Mjolnir.ts +++ b/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) { diff --git a/src/index.ts b/src/index.ts index 58325d4..1def8aa 100644 --- a/src/index.ts +++ b/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); diff --git a/src/report/ReportManager.ts b/src/report/ReportManager.ts index 7b7b561..0d01f06 100644 --- a/src/report/ReportManager.ts +++ b/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", diff --git a/src/utils.ts b/src/utils.ts index ea485f5..644331b 100644 --- a/src/utils.ts +++ b/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; } -/** - * 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 { - 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"); +let isMatrixClientPatchedForConciseExceptions = false; /** - * @returns `true` if a `MatrixClient` has been monkey patched to return - * saner exceptions. - */ -export function isClientWithSanerExceptions(client: MatrixClient): boolean { - return CLIENT_WITH_SANER_EXCEPTION in client; -} - -/** - * Wrap a `MatrixClient` into something that throws sane exceptions. + * Patch `MatrixClient` into something that throws concise 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 - * a `MatrixClient` that instead throws more reasonable insetances of `Error`. + * This method configures `MatrixClient` to ensure that methods that may throw + * instead throws more reasonable insetances of `Error`. */ -export function makeClientWithSanerExceptions(client: MatrixClient): MatrixClient { - let result = new Proxy(client, { - has: function (obj, key): boolean { - return key === CLIENT_WITH_SANER_EXCEPTION - || key in client; - }, - get: function (obj, key) { - if (key === "doRequest") { - // Intercept `doRequest`. - return (...args) => doRequestReplacement(client, args); +export function patchMatrixClientForConciseExceptions() { + if (isMatrixClientPatchedForConciseExceptions) { + return; + } + let originalRequestFn = getRequestFn(); + setRequestFn((params, cb) => { + originalRequestFn(params, function conciseExceptionRequestFn(err, response, resBody) { + if (!err && (response.statusCode < 200 || response.statusCode >= 300)) { + // Normally, converting HTTP Errors into rejections is done by the caller + // 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") { - return true; + if (!(err instanceof IncomingMessage)) { + // In most cases, we're happy with the result. + return cb(err, response, resBody); } - let value = obj[key]; - if (!(typeof value === "function")) { - // We're only interested in methods. - return value; + // 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 (err.method) { + method = err.method; } - // Make sure that methods use our intercepted `doRequestReplacement`. - return value.bind(result); - } + if (err.url) { + 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; -} \ No newline at end of file + isMatrixClientPatchedForConciseExceptions = true; +} + diff --git a/src/webapis/WebAPIs.ts b/src/webapis/WebAPIs.ts index ef0375b..9d7487a 100644 --- a/src/webapis/WebAPIs.ts +++ b/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"); }; diff --git a/test/integration/clientHelper.ts b/test/integration/clientHelper.ts index b478c73..45e8c14 100644 --- a/test/integration/clientHelper.ts +++ b/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 { 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); } /** diff --git a/test/integration/mjolnirSetupUtils.ts b/test/integration/mjolnirSetupUtils.ts index d1165d0..13a0d75 100644 --- a/test/integration/mjolnirSetupUtils.ts +++ b/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 { 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); - client = makeClientWithSanerExceptions(client); + const client = await pantalaimon.createClientWithCredentials(config.pantalaimon.username, config.pantalaimon.password); + patchMatrixClientForConciseExceptions(); await ensureAliasedRoomExists(client, config.managementRoom); let mjolnir = await Mjolnir.setupMjolnirFromConfig(client); globalClient = client;