diff --git a/src/utils.ts b/src/utils.ts index 9fdd6d6e..328990ea 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -236,7 +236,7 @@ function patchMatrixClientForConciseExceptions() { // We'll only throw the error if there is one. let error = new Error("STACK CAPTURE"); originalRequestFn(params, function conciseExceptionRequestFn( - err: { [key: string]: any }, response: { [key: string]: any }, resBody: string + err: { [key: string]: unknown }, response: { [key: string]: any }, resBody: unknown ) { if (!err && (response?.statusCode < 200 || response?.statusCode >= 300)) { // Normally, converting HTTP Errors into rejections is done by the caller @@ -272,34 +272,36 @@ function patchMatrixClientForConciseExceptions() { // useless. let method: string | null = null; let path = ''; - let body: string | null = null; + let body: unknown = null; if (err.method) { method = err.method; } if (err.url) { path = err.url; } - if ("req" in err && (err as any).req instanceof ClientRequest) { + if ("req" in err && err.req instanceof ClientRequest) { if (!method) { - method = (err as any).req.method; + method = err.req.method; } if (!path) { - path = (err as any).req.path; + path = err.req.path; } } if ("body" in err) { - body = (err as any).body; + body = err.body; } - let message = `Error during MatrixClient request ${method} ${path}: ${err.statusCode} ${err.statusMessage} -- ${body}`; - error.message = message; - if (body) { - // Calling code may use `body` to check for errors, so let's - // make sure that we're providing it. + // Calling code may use `body` to check for errors, so let's + // make sure that we're providing it. + if (typeof body === 'string') { try { - body = JSON.parse(body); + body = JSON.parse(body, jsonReviver); } catch (ex) { // Not JSON. } + } + let message = `Error during MatrixClient request ${method} ${path}: ${err.statusCode} ${err.statusMessage} -- ${JSON.stringify(body)}`; + error.message = message; + if (body) { // Define the property but don't make it visible during logging. Object.defineProperty(error, "body", { value: body, @@ -355,7 +357,7 @@ function patchMatrixClientForRetry() { try { let result: any[] = await new Promise((resolve, reject) => { originalRequestFn(params, function requestFnWithRetry( - err: { [key: string]: any }, response: { [key: string]: any }, resBody: string + err: { [key: string]: any }, response: { [key: string]: unknown }, resBody: unknown ) { // Note: There is no data race on `attempt` as we `await` before continuing // to the next iteration of the loop. @@ -398,20 +400,72 @@ function patchMatrixClientForRetry() { isMatrixClientPatchedForRetryWhenThrottled = true; } +let isMatrixClientPatchedForPrototypePollution = false; + +function jsonReviver(key: string, value: any): any { + if (key === '__proto__' || key === 'constructor') { + return undefined; + } else { + return value; + } +} + +/** + * https://github.com/turt2live/matrix-bot-sdk/blob/c7d16776502c26bbb547a3d667ec92eb50e7026c/src/http.ts#L77-L101 💀 fucking hell!!!! + * + * The following is an inefficient workaround, but you gotta do what you can. + */ +function patchMatrixClientForPrototypePollution() { + if (isMatrixClientPatchedForPrototypePollution) { + return; + } + const originalRequestFn = getRequestFn(); + setRequestFn((params: { [k: string]: any }, cb: any) => { + originalRequestFn(params, function conciseExceptionRequestFn( + error: { [key: string]: any }, response: { [key: string]: any }, resBody: unknown + ) { + // https://github.com/turt2live/matrix-bot-sdk/blob/c7d16776502c26bbb547a3d667ec92eb50e7026c/src/http.ts#L77-L101 + // bring forwards this step and do it safely. + if (typeof resBody === 'string') { + try { + resBody = JSON.parse(resBody, jsonReviver); + } catch (e) { + // we don't care if we fail to parse the JSON as it probably isn't JSON. + } + } + + if (typeof response.body === 'string') { + try { + response.body = JSON.parse(response.body, jsonReviver); + } catch (e) { + // we don't care if we fail to parse the JSON as it probably isn't JSON. + } + } + return cb(error, response, resBody); + }) + }); + isMatrixClientPatchedForPrototypePollution = true; +} + /** * Perform any patching deemed necessary to MatrixClient. */ export function patchMatrixClient() { // Note that the order of patches is meaningful. // + // - `patchMatrixClientForPrototypePollution` converts all JSON bodies to safe JSON before client code can + // parse and use the JSON inappropriately. // - `patchMatrixClientForConciseExceptions` converts all `IncomingMessage` // errors into instances of `Error` handled as errors; // - `patchMatrixClientForRetry` expects that all errors are returned as // errors. + patchMatrixClientForPrototypePollution(); patchMatrixClientForConciseExceptions(); patchMatrixClientForRetry(); } +patchMatrixClient(); + /** * Initialize Sentry for error monitoring and reporting. * diff --git a/test/integration/clientHelper.ts b/test/integration/clientHelper.ts index 9a8de47f..ce9fe5c1 100644 --- a/test/integration/clientHelper.ts +++ b/test/integration/clientHelper.ts @@ -1,5 +1,6 @@ import { HmacSHA1 } from "crypto-js"; import { getRequestFn, LogService, MatrixClient, MemoryStorageProvider, PantalaimonClient } from "matrix-bot-sdk"; +import "../../src/utils"; // we need this for the patches to matrix-bot-sdk's `getRequestFn`. const REGISTRATION_ATTEMPTS = 10; const REGISTRATION_RETRY_BASE_DELAY_MS = 100; @@ -17,12 +18,17 @@ const REGISTRATION_RETRY_BASE_DELAY_MS = 100; */ export async function registerUser(homeserver: string, username: string, displayname: string, password: string, admin: boolean): Promise { let registerUrl = `${homeserver}/_synapse/admin/v1/register` - const data: {nonce: string} = await new Promise((resolve, reject) => { - getRequestFn()({uri: registerUrl, method: "GET", timeout: 60000}, (error: any, response: any, resBody: any) => { - error ? reject(error) : resolve(JSON.parse(resBody)) + const nonce: string = await new Promise((resolve, reject) => { + getRequestFn()({uri: registerUrl, method: "GET", timeout: 60000}, (error: any, _response: any, resBody: unknown) => { + if (error) { + reject(error); + } else if (typeof resBody === 'object' && resBody !== null && 'nonce' in resBody && typeof resBody.nonce === 'string') { + resolve(resBody.nonce) + } else { + reject(new TypeError(`Don't know what to do with response body ${JSON.stringify(resBody)}`)); + } }); }); - const nonce = data.nonce!; let mac = HmacSHA1(`${nonce}\0${username}\0${password}\0${admin ? 'admin' : 'notadmin'}`, 'REGISTRATION_SHARED_SECRET'); for (let i = 1; i <= REGISTRATION_ATTEMPTS; ++i) { try { @@ -136,7 +142,7 @@ async function getGlobalAdminUser(homeserver: string): Promise { try { await registerUser(homeserver, USERNAME, USERNAME, USERNAME, true); } catch (e) { - if (e.isAxiosError && e?.response?.data?.errcode === 'M_USER_IN_USE') { + if (e?.body?.errcode === 'M_USER_IN_USE') { // Then we've already registered the user in a previous run and that is ok. } else { throw e; diff --git a/test/integration/commands/shutdownCommandTest.ts b/test/integration/commands/shutdownCommandTest.ts index 0ff5ea8c..7e543d2c 100644 --- a/test/integration/commands/shutdownCommandTest.ts +++ b/test/integration/commands/shutdownCommandTest.ts @@ -47,8 +47,10 @@ describe("Test: shutdown command", function() { await reply1 await reply2 - await assert.rejects(client.joinRoom(badRoom), e => { - return e.message.endsWith('{"errcode":"M_UNKNOWN","error":"This room has been blocked on this server"}'); + await assert.rejects(client.joinRoom(badRoom), (e: any) => { + assert.equal(e.statusCode, 403); + assert.equal(e.body.error, "This room has been blocked on this server"); + return true; }); }); }); diff --git a/test/integration/fixtures.ts b/test/integration/fixtures.ts index d19c0aca..1634c716 100644 --- a/test/integration/fixtures.ts +++ b/test/integration/fixtures.ts @@ -1,6 +1,9 @@ import { read as configRead } from "../../src/config"; +import { patchMatrixClient } from "../../src/utils"; import { makeMjolnir, teardownManagementRoom } from "./mjolnirSetupUtils"; +patchMatrixClient(); + // When Mjolnir starts (src/index.ts) it clobbers the config by resolving the management room // alias specified in the config (config.managementRoom) and overwriting that with the room ID. // Unfortunately every piece of code importing that config imports the same instance, including diff --git a/test/integration/mjolnirSetupUtils.ts b/test/integration/mjolnirSetupUtils.ts index 866c040d..f82137e0 100644 --- a/test/integration/mjolnirSetupUtils.ts +++ b/test/integration/mjolnirSetupUtils.ts @@ -26,6 +26,8 @@ import { overrideRatelimitForUser, registerUser } from "./clientHelper"; import { initializeSentry, patchMatrixClient } from "../../src/utils"; import { IConfig } from "../../src/config"; +patchMatrixClient(); + /** * Ensures that a room exists with the alias, if it does not exist we create it. * @param client The MatrixClient to use to resolve or create the aliased room. @@ -82,7 +84,6 @@ export async function makeMjolnir(config: IConfig): Promise { const pantalaimon = new PantalaimonClient(config.homeserverUrl, new MemoryStorageProvider()); const client = await pantalaimon.createClientWithCredentials(config.pantalaimon.username, config.pantalaimon.password); await overrideRatelimitForUser(config.homeserverUrl, await client.getUserId()); - patchMatrixClient(); await ensureAliasedRoomExists(client, config.managementRoom); let mj = await Mjolnir.setupMjolnirFromConfig(client, client, config); globalClient = client; diff --git a/test/integration/policyConsumptionTest.ts b/test/integration/policyConsumptionTest.ts index a3b8c2d4..cab282a8 100644 --- a/test/integration/policyConsumptionTest.ts +++ b/test/integration/policyConsumptionTest.ts @@ -13,11 +13,11 @@ async function currentRules(mjolnir: Mjolnir): Promise<{ start: object, stop: ob return await new Promise((resolve, reject) => getRequestFn()({ uri: `http://${mjolnir.config.web.address}:${mjolnir.config.web.port}/api/1/ruleserver/updates/`, method: "GET" - }, (error: object, _response: any, body: string) => { + }, (error: object, _response: any, body: any) => { if (error) { reject(error) } else { - resolve(JSON.parse(body)) + resolve(body) } })); }