From dc126afa6af86d66aefcd23a825326f405bcc894 Mon Sep 17 00:00:00 2001 From: Will Hunt Date: Mon, 25 Sep 2023 15:55:15 +0100 Subject: [PATCH] Replace vm2 with quickjs (#817) * quickjs test * Replace vm2 with quickjs * initalise -> initialise * Remove unused transformation timeout time * Don't assume quickModule is set Also use whether it's set as the indicator of whether transformation functions are allowed, instead of checking the config * Refactor GenericHookConnectionState validation - Do it in the constructor instead of in callers - Make hookId mandatory so as to not drop it on some state updates - Conflate a state event's state key with a connection state's name, which was already the case in practice * Refactor validateState * Drop explicit any Better to infer the type instead * Always validate transformation fn * Fix test * Add changelog * Fix disposal, validation, and printing * Fix transformation error string formatting Also refactor similar code * Let invalid transformations run & fail instead of pretending that one was never set * Restore transformation timeout time * Don't execute transformation fn when validating it Instead, only compile it * Revert unrelated changes --------- Co-authored-by: Andrew Ferrazzutti --- changelog.d/817.misc | 1 + package.json | 2 +- src/App/BridgeApp.ts | 5 + src/Connections/GenericHook.ts | 105 ++++++++++++------ tests/connections/GenericHookTest.ts | 3 + .../roomConfig/GenericWebhookConfig.tsx | 2 +- yarn.lock | 17 ++- 7 files changed, 87 insertions(+), 48 deletions(-) create mode 100644 changelog.d/817.misc diff --git a/changelog.d/817.misc b/changelog.d/817.misc new file mode 100644 index 000000000..d7b7abb2c --- /dev/null +++ b/changelog.d/817.misc @@ -0,0 +1 @@ +Use quickjs instead of vm2 for evaluating JS transformation functions. diff --git a/package.json b/package.json index 9f25001d4..449407586 100644 --- a/package.json +++ b/package.json @@ -65,11 +65,11 @@ "nyc": "^15.1.0", "p-queue": "^6.6.2", "prom-client": "^14.2.0", + "quickjs-emscripten": "^0.23.0", "reflect-metadata": "^0.1.13", "source-map-support": "^0.5.21", "string-argv": "^0.3.1", "tiny-typed-emitter": "^2.1.0", - "vm2": "^3.9.18", "winston": "^3.3.3", "xml2js": "^0.5.0", "yaml": "^2.2.2" diff --git a/src/App/BridgeApp.ts b/src/App/BridgeApp.ts index 7c182b61e..10a7e49aa 100644 --- a/src/App/BridgeApp.ts +++ b/src/App/BridgeApp.ts @@ -10,6 +10,7 @@ import { LogService } from "matrix-bot-sdk"; import { getAppservice } from "../appservice"; import BotUsersManager from "../Managers/BotUsersManager"; import * as Sentry from '@sentry/node'; +import { GenericHookConnection } from "../Connections"; Logger.configure({console: "info"}); const log = new Logger("App"); @@ -49,6 +50,10 @@ async function start() { log.info("Sentry reporting enabled"); } + if (config.generic?.allowJsTransformationFunctions) { + await GenericHookConnection.initialiseQuickJS(); + } + const botUsersManager = new BotUsersManager(config, appservice); const bridgeApp = new Bridge(config, listener, appservice, storage, botUsersManager); diff --git a/src/Connections/GenericHook.ts b/src/Connections/GenericHook.ts index 26e5c48c4..2d02b756a 100644 --- a/src/Connections/GenericHook.ts +++ b/src/Connections/GenericHook.ts @@ -2,7 +2,7 @@ import { Connection, IConnection, IConnectionState, InstantiateConnectionOpts, P import { Logger } from "matrix-appservice-bridge"; import { MessageSenderClient } from "../MatrixSender" import markdownit from "markdown-it"; -import { VMScript as Script, NodeVM } from "vm2"; +import { QuickJSRuntime, QuickJSWASMModule, newQuickJSWASMModule, shouldInterruptAfterDeadline } from "quickjs-emscripten"; import { MatrixEvent } from "../MatrixEvent"; import { Appservice, Intent, StateEvent } from "matrix-bot-sdk"; import { ApiError, ErrCode } from "../api"; @@ -65,6 +65,11 @@ const SANITIZE_MAX_BREADTH = 50; */ @Connection export class GenericHookConnection extends BaseConnection implements IConnection { + private static quickModule?: QuickJSWASMModule; + + public static async initialiseQuickJS() { + GenericHookConnection.quickModule = await newQuickJSWASMModule(); + } /** * Ensures a JSON payload is compatible with Matrix JSON requirements, such @@ -107,27 +112,26 @@ export class GenericHookConnection extends BaseConnection implements IConnection return obj; } - static validateState(state: Record, allowJsTransformationFunctions?: boolean): GenericHookConnectionState { + static validateState(state: Record): GenericHookConnectionState { const {name, transformationFunction} = state; - let transformationFunctionResult: string|undefined; - if (transformationFunction) { - if (allowJsTransformationFunctions !== undefined && !allowJsTransformationFunctions) { - throw new ApiError('Transformation functions are not allowed', ErrCode.DisabledFeature); - } - if (typeof transformationFunction !== "string") { - throw new ApiError('Transformation functions must be a string', ErrCode.BadValue); - } - transformationFunctionResult = transformationFunction; - } if (!name) { throw new ApiError('Missing name', ErrCode.BadValue); } if (typeof name !== "string" || name.length < 3 || name.length > 64) { throw new ApiError("'name' must be a string between 3-64 characters long", ErrCode.BadValue); } + // Use !=, not !==, to check for both undefined and null + if (transformationFunction != undefined) { + if (!this.quickModule) { + throw new ApiError('Transformation functions are not allowed', ErrCode.DisabledFeature); + } + if (typeof transformationFunction !== "string") { + throw new ApiError('Transformation functions must be a string', ErrCode.BadValue); + } + } return { name, - ...(transformationFunctionResult && {transformationFunction: transformationFunctionResult}), + ...(transformationFunction && {transformationFunction}), }; } @@ -163,7 +167,7 @@ export class GenericHookConnection extends BaseConnection implements IConnection throw Error('Generic Webhooks are not configured'); } const hookId = randomUUID(); - const validState = GenericHookConnection.validateState(data, config.generic.allowJsTransformationFunctions || false); + const validState = GenericHookConnection.validateState(data); await GenericHookConnection.ensureRoomAccountData(roomId, intent, hookId, validState.name); await intent.underlyingClient.sendStateEvent(roomId, this.CanonicalEventType, validState.name, validState); const connection = new GenericHookConnection(roomId, validState, hookId, validState.name, messageClient, config.generic, as, intent); @@ -197,8 +201,11 @@ export class GenericHookConnection extends BaseConnection implements IConnection GenericHookConnection.LegacyCanonicalEventType, ]; - private transformationFunction?: Script; + private transformationFunction?: string; private cachedDisplayname?: string; + /** + * @param state Should be a pre-validated state object returned by {@link validateState} + */ constructor( roomId: string, private state: GenericHookConnectionState, @@ -210,8 +217,8 @@ export class GenericHookConnection extends BaseConnection implements IConnection private readonly intent: Intent, ) { super(roomId, stateKey, GenericHookConnection.CanonicalEventType); - if (state.transformationFunction && config.allowJsTransformationFunctions) { - this.transformationFunction = new Script(state.transformationFunction); + if (state.transformationFunction && GenericHookConnection.quickModule) { + this.transformationFunction = state.transformationFunction; } } @@ -259,12 +266,26 @@ export class GenericHookConnection extends BaseConnection implements IConnection } public async onStateUpdate(stateEv: MatrixEvent) { - const validatedConfig = GenericHookConnection.validateState(stateEv.content as Record, this.config.allowJsTransformationFunctions || false); + const validatedConfig = GenericHookConnection.validateState(stateEv.content as Record); if (validatedConfig.transformationFunction) { - try { - this.transformationFunction = new Script(validatedConfig.transformationFunction); - } catch (ex) { - await this.messageClient.sendMatrixText(this.roomId, 'Could not compile transformation function:' + ex, "m.text", this.intent.userId); + const ctx = GenericHookConnection.quickModule!.newContext(); + const codeEvalResult = ctx.evalCode(`function f(data) {${validatedConfig.transformationFunction}}`, undefined, { compileOnly: true }); + if (codeEvalResult.error) { + const errorString = JSON.stringify(ctx.dump(codeEvalResult.error), null, 2); + codeEvalResult.error.dispose(); + ctx.dispose(); + + const errorPrefix = "Could not compile transformation function:"; + await this.intent.sendEvent(this.roomId, { + msgtype: "m.text", + body: errorPrefix + "\n\n```json\n\n" + errorString + "\n\n```", + formatted_body: `

${errorPrefix}

${errorString}

`, + format: "org.matrix.custom.html", + }); + } else { + codeEvalResult.value.dispose(); + ctx.dispose(); + this.transformationFunction = validatedConfig.transformationFunction; } } else { this.transformationFunction = undefined; @@ -281,8 +302,10 @@ export class GenericHookConnection extends BaseConnection implements IConnection } else if (typeof safeData?.text === "string") { msg.plain = safeData.text; } else { - msg.plain = "Received webhook data:\n\n" + "```json\n\n" + JSON.stringify(data, null, 2) + "\n\n```"; - msg.html = `

Received webhook data:

${JSON.stringify(data, null, 2)}

` + const dataString = JSON.stringify(data, null, 2); + const dataPrefix = "Received webhook data:"; + msg.plain = dataPrefix + "\n\n```json\n\n" + dataString + "\n\n```"; + msg.html = `

${dataPrefix}

${dataString}

` } if (typeof safeData?.html === "string") { @@ -304,17 +327,27 @@ export class GenericHookConnection extends BaseConnection implements IConnection if (!this.transformationFunction) { throw Error('Transformation function not defined'); } - const vm = new NodeVM({ - console: 'off', - wrapper: 'none', - wasm: false, - eval: false, - timeout: TRANSFORMATION_TIMEOUT_MS, - }); - vm.setGlobal('HookshotApiVersion', 'v2'); - vm.setGlobal('data', data); - vm.run(this.transformationFunction); - const result = vm.getGlobal('result'); + let result; + const ctx = GenericHookConnection.quickModule!.newContext(); + ctx.runtime.setInterruptHandler(shouldInterruptAfterDeadline(Date.now() + TRANSFORMATION_TIMEOUT_MS)); + try { + ctx.setProp(ctx.global, 'HookshotApiVersion', ctx.newString('v2')); + const ctxResult = ctx.evalCode(`const data = ${JSON.stringify(data)};\n\n${this.state.transformationFunction}`); + + if (ctxResult.error) { + const e = Error(`Transformation failed to run: ${JSON.stringify(ctx.dump(ctxResult.error))}`); + ctxResult.error.dispose(); + throw e; + } else { + const value = ctx.getProp(ctx.global, 'result'); + result = ctx.dump(value); + value.dispose(); + ctxResult.value.dispose(); + } + } finally { + ctx.global.dispose(); + ctx.dispose(); + } // Legacy v1 api if (typeof result === "string") { @@ -437,7 +470,7 @@ export class GenericHookConnection extends BaseConnection implements IConnection public async provisionerUpdateConfig(userId: string, config: Record) { // Apply previous state to the current config, as provisioners might not return "unknown" keys. config = { ...this.state, ...config }; - const validatedConfig = GenericHookConnection.validateState(config, this.config.allowJsTransformationFunctions || false); + const validatedConfig = GenericHookConnection.validateState(config); await this.intent.underlyingClient.sendStateEvent(this.roomId, GenericHookConnection.CanonicalEventType, this.stateKey, { ...validatedConfig, diff --git a/tests/connections/GenericHookTest.ts b/tests/connections/GenericHookTest.ts index 0e22ee8fb..f0c82ec1b 100644 --- a/tests/connections/GenericHookTest.ts +++ b/tests/connections/GenericHookTest.ts @@ -56,6 +56,9 @@ function handleMessage(mq: LocalMQ): Promise { } describe("GenericHookConnection", () => { + before(async () => { + await GenericHookConnection.initialiseQuickJS(); + }) it("will handle simple hook events", async () => { const [connection, mq] = createGenericHook(); await testSimpleWebhook(connection, mq, "data"); diff --git a/web/components/roomConfig/GenericWebhookConfig.tsx b/web/components/roomConfig/GenericWebhookConfig.tsx index 7a6648c61..e62810f21 100644 --- a/web/components/roomConfig/GenericWebhookConfig.tsx +++ b/web/components/roomConfig/GenericWebhookConfig.tsx @@ -20,7 +20,7 @@ const EXAMPLE_SCRIPT = `if (data.counter === undefined) { }; } else { result = { - plain: \`*Everything is fine*, the counter is under by\${data.maxValue - data.counter}\`, + plain: \`*Everything is fine*, the counter is under by \${data.maxValue - data.counter}\`, version: "v2" }; }`; diff --git a/yarn.lock b/yarn.lock index f1f36546b..13443d20f 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1754,12 +1754,12 @@ acorn-jsx@^5.3.2: resolved "https://registry.yarnpkg.com/acorn-jsx/-/acorn-jsx-5.3.2.tgz#7ed5bb55908b3b2f1bc55c6af1653bada7f07937" integrity sha512-rq9s+JNhf0IChjtDXxllJ7g41oZk5SlXtp0LHwyA5cejwn7vKmKp4pPri6YEePv2PU65sAsegbXtIinmDFDXgQ== -acorn-walk@^8.1.1, acorn-walk@^8.2.0: +acorn-walk@^8.1.1: version "8.2.0" resolved "https://registry.yarnpkg.com/acorn-walk/-/acorn-walk-8.2.0.tgz#741210f2e2426454508853a2f44d0ab83b7f69c1" integrity sha512-k+iyHEuPgSw6SbuDpGQM+06HQUa04DZ3o+F6CSzXMvvI5KMvnaEqXe+YVe555R9nn6GPt404fos4wcgpw12SDA== -acorn@^8.4.1, acorn@^8.7.0: +acorn@^8.4.1: version "8.7.1" resolved "https://registry.yarnpkg.com/acorn/-/acorn-8.7.1.tgz#0197122c843d1bf6d0a5e83220a788f278f63c30" integrity sha512-Xx54uLJQZ19lKygFXOWsscKUbsBZW0CPykPhVQdhIeIwrbPmJzqeASDInc8nKBnp/JT6igTs82qPXz069H8I/A== @@ -5240,6 +5240,11 @@ queue-microtask@^1.2.2: resolved "https://registry.yarnpkg.com/queue-microtask/-/queue-microtask-1.2.3.tgz#4929228bbc724dfac43e0efb058caf7b6cfb6243" integrity sha512-NuaNSa6flKT5JaSYQzJok04JzTL1CA6aGhv5rfLW3PgqA+M2ChpZQnAC8h8i4ZFkBS8X5RqkDBHA7r4hej3K9A== +quickjs-emscripten@^0.23.0: + version "0.23.0" + resolved "https://registry.yarnpkg.com/quickjs-emscripten/-/quickjs-emscripten-0.23.0.tgz#94f412d0ee5f3021fc12ddf6c0b00bd8ce0d28b9" + integrity sha512-CIP+NDRYDDqbT3cTiN8Bon1wsZ7IgISVYCJHYsPc86oxszpepVMPXFfttyQgn1u1okg1HPnCnM7Xv1LrCO/VmQ== + railroad-diagrams@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/railroad-diagrams/-/railroad-diagrams-1.0.0.tgz#eb7e6267548ddedfb899c1b90e57374559cddb7e" @@ -6166,14 +6171,6 @@ vite@^4.1.5: optionalDependencies: fsevents "~2.3.2" -vm2@^3.9.18: - version "3.9.18" - resolved "https://registry.yarnpkg.com/vm2/-/vm2-3.9.18.tgz#d919848bee191a0410c5cc1c5aac58adfd03ce9a" - integrity sha512-iM7PchOElv6Uv6Q+0Hq7dcgDtWWT6SizYqVcvol+1WQc+E9HlgTCnPozbQNSP3yDV9oXHQOEQu530w2q/BCVZg== - dependencies: - acorn "^8.7.0" - acorn-walk "^8.2.0" - w3c-keyname@^2.2.4: version "2.2.6" resolved "https://registry.yarnpkg.com/w3c-keyname/-/w3c-keyname-2.2.6.tgz#8412046116bc16c5d73d4e612053ea10a189c85f"