From 93b18b81e003da221cb065d9587f0a6ed337fbfb Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 20 Feb 2024 10:49:45 +0000 Subject: [PATCH 1/3] Fix re-used context in JS runner. --- packages/server/src/api/controllers/script.ts | 7 ++++--- packages/server/src/jsRunner/index.ts | 15 +++++-------- .../server/src/jsRunner/vm/isolated-vm.ts | 14 +++++++++++-- packages/server/src/jsRunner/vm/vm2.ts | 14 +++++++++++-- packages/server/src/threads/query.ts | 21 ++++++++----------- packages/types/src/sdk/vm.ts | 1 + 6 files changed, 43 insertions(+), 29 deletions(-) diff --git a/packages/server/src/api/controllers/script.ts b/packages/server/src/api/controllers/script.ts index bdca2d6e18..93e1ad7df9 100644 --- a/packages/server/src/api/controllers/script.ts +++ b/packages/server/src/api/controllers/script.ts @@ -3,9 +3,10 @@ import { IsolatedVM } from "../../jsRunner/vm" export async function execute(ctx: Ctx) { const { script, context } = ctx.request.body - const runner = new IsolatedVM().withContext(context) - - const result = runner.execute(`(function(){\n${script}\n})();`) + const vm = new IsolatedVM() + const result = vm.withContext(context, () => + vm.execute(`(function(){\n${script}\n})();`) + ) ctx.body = result } diff --git a/packages/server/src/jsRunner/index.ts b/packages/server/src/jsRunner/index.ts index 3c13aef1d4..1e8bee04c3 100644 --- a/packages/server/src/jsRunner/index.ts +++ b/packages/server/src/jsRunner/index.ts @@ -23,22 +23,17 @@ export function init() { try { const bbCtx = context.getCurrentContext()! - let { vm } = bbCtx - if (!vm) { - // Can't copy the native helpers into the isolate. We just ignore them as they are handled properly from the helpersSource - const { helpers, ...ctxToPass } = ctx - - vm = new IsolatedVM({ + if (!bbCtx.vm) { + const vm = new IsolatedVM({ memoryLimit: env.JS_RUNNER_MEMORY_LIMIT, invocationTimeout: env.JS_PER_INVOCATION_TIMEOUT_MS, isolateAccumulatedTimeout: env.JS_PER_REQUEST_TIMEOUT_MS, - }) - .withContext(ctxToPass) - .withHelpers() + }).withHelpers() bbCtx.vm = vm } - return vm.execute(js) + const { helpers, ...rest } = ctx + return bbCtx.vm.withContext(rest, () => bbCtx.vm!.execute(js)) } catch (error: any) { if (error.message === "Script execution timed out.") { throw new JsErrorTimeout() diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index f431ff644b..78250acdd2 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -97,10 +97,14 @@ export class IsolatedVM implements VM { return this } - withContext(context: Record) { + withContext(context: Record, f: () => T) { this.addToContext(context) - return this + try { + return f() + } finally { + this.removeFromContext(Object.keys(context)) + } } withParsingBson(data: any) { @@ -224,6 +228,12 @@ export class IsolatedVM implements VM { } } + private removeFromContext(keys: string[]) { + for (let key of keys) { + this.jail.deleteSync(key) + } + } + private getFromContext(key: string) { const ref = this.vm.global.getSync(key, { reference: true }) const result = ref.copySync() diff --git a/packages/server/src/jsRunner/vm/vm2.ts b/packages/server/src/jsRunner/vm/vm2.ts index 6d05943d25..75e3899064 100644 --- a/packages/server/src/jsRunner/vm/vm2.ts +++ b/packages/server/src/jsRunner/vm/vm2.ts @@ -7,16 +7,26 @@ export class VM2 implements VM { vm: vm2.VM results: { out: string } - constructor(context: any) { + constructor() { this.vm = new vm2.VM({ timeout: JS_TIMEOUT_MS, }) this.results = { out: "" } - this.vm.setGlobals(context) this.vm.setGlobal("fetch", fetch) this.vm.setGlobal("results", this.results) } + withContext(context: Record, fn: () => T): T { + this.vm.setGlobals(context) + try { + return fn() + } finally { + for (const key in context) { + this.vm.setGlobal(key, undefined) + } + } + } + execute(script: string) { const code = `let fn = () => {\n${script}\n}; results.out = fn();` const vmScript = new vm2.VMScript(code) diff --git a/packages/server/src/threads/query.ts b/packages/server/src/threads/query.ts index c159f24268..6cc07c1256 100644 --- a/packages/server/src/threads/query.ts +++ b/packages/server/src/threads/query.ts @@ -131,24 +131,21 @@ class QueryRunner { if (transformer) { let runner: VM if (!USE_ISOLATED_VM) { - runner = new VM2({ - data: rows, - params: enrichedParameters, - }) + runner = new VM2() } else { transformer = `(function(){\n${transformer}\n})();` - let isolatedVm = new IsolatedVM().withContext({ - data: rows, - params: enrichedParameters, - }) + let vm = new IsolatedVM() if (datasource.source === SourceName.MONGODB) { - isolatedVm = isolatedVm.withParsingBson(rows) + vm = vm.withParsingBson(rows) } - - runner = isolatedVm + runner = vm } - rows = runner.execute(transformer) + const ctx = { + data: rows, + params: enrichedParameters, + } + rows = runner.withContext(ctx, () => runner.execute(transformer)) } // if the request fails we retry once, invalidating the cached value diff --git a/packages/types/src/sdk/vm.ts b/packages/types/src/sdk/vm.ts index 43b7775d3b..314883e0c5 100644 --- a/packages/types/src/sdk/vm.ts +++ b/packages/types/src/sdk/vm.ts @@ -1,3 +1,4 @@ export interface VM { execute(code: string): any + withContext(context: Record, fn: () => T): T } From a866677080bde9cbac804b306dc32d6798bd9e3e Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 20 Feb 2024 10:59:04 +0000 Subject: [PATCH 2/3] Add tests. --- .../server/src/api/routes/tests/row.spec.ts | 43 +++++++++++++++++++ packages/server/src/jsRunner/index.ts | 4 +- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/packages/server/src/api/routes/tests/row.spec.ts b/packages/server/src/api/routes/tests/row.spec.ts index 10dac3c0ea..239da36351 100644 --- a/packages/server/src/api/routes/tests/row.spec.ts +++ b/packages/server/src/api/routes/tests/row.spec.ts @@ -2135,5 +2135,48 @@ describe.each([ } ) }) + + it("should not carry over context between formulas", async () => { + const js = Buffer.from(`return $("[text]");`).toString("base64") + const table = await config.createTable({ + name: "table", + type: "table", + schema: { + text: { + name: "text", + type: FieldType.STRING, + }, + formula: { + name: "formula", + type: FieldType.FORMULA, + formula: `{{ js "${js}"}}`, + formulaType: FormulaType.DYNAMIC, + }, + }, + }) + + for (let i = 0; i < 10; i++) { + await config.api.row.save(table._id!, { text: `foo${i}` }) + } + + const { rows } = await config.api.row.search(table._id!) + expect(rows).toHaveLength(10) + + const formulaValues = rows.map(r => r.formula) + expect(formulaValues).toEqual( + expect.arrayContaining([ + "foo0", + "foo1", + "foo2", + "foo3", + "foo4", + "foo5", + "foo6", + "foo7", + "foo8", + "foo9", + ]) + ) + }) }) }) diff --git a/packages/server/src/jsRunner/index.ts b/packages/server/src/jsRunner/index.ts index 1e8bee04c3..d0385404fe 100644 --- a/packages/server/src/jsRunner/index.ts +++ b/packages/server/src/jsRunner/index.ts @@ -24,13 +24,11 @@ export function init() { const bbCtx = context.getCurrentContext()! if (!bbCtx.vm) { - const vm = new IsolatedVM({ + bbCtx.vm = new IsolatedVM({ memoryLimit: env.JS_RUNNER_MEMORY_LIMIT, invocationTimeout: env.JS_PER_INVOCATION_TIMEOUT_MS, isolateAccumulatedTimeout: env.JS_PER_REQUEST_TIMEOUT_MS, }).withHelpers() - - bbCtx.vm = vm } const { helpers, ...rest } = ctx return bbCtx.vm.withContext(rest, () => bbCtx.vm!.execute(js)) From 285916d0bfbfe2f56e2d5c7365348ad0de2e40ff Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Tue, 20 Feb 2024 11:11:27 +0000 Subject: [PATCH 3/3] Some PR comments/build issue. --- packages/server/src/jsRunner/vm/builtin-vm.ts | 11 +++++++++++ packages/server/src/jsRunner/vm/isolated-vm.ts | 4 ++-- packages/server/src/jsRunner/vm/vm2.ts | 4 ++-- packages/types/src/sdk/vm.ts | 2 +- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/server/src/jsRunner/vm/builtin-vm.ts b/packages/server/src/jsRunner/vm/builtin-vm.ts index b4c9f775f9..849abdd6f2 100644 --- a/packages/server/src/jsRunner/vm/builtin-vm.ts +++ b/packages/server/src/jsRunner/vm/builtin-vm.ts @@ -15,6 +15,17 @@ export class BuiltInVM implements VM { this.span = span } + withContext(context: Record, executeWithContext: () => T): T { + this.ctx = vm.createContext(context) + try { + return executeWithContext() + } finally { + for (const key in context) { + delete this.ctx[key] + } + } + } + execute(code: string) { const perRequestLimit = env.JS_PER_REQUEST_TIMEOUT_MS let track: TrackerFn = f => f() diff --git a/packages/server/src/jsRunner/vm/isolated-vm.ts b/packages/server/src/jsRunner/vm/isolated-vm.ts index 78250acdd2..e5c431666d 100644 --- a/packages/server/src/jsRunner/vm/isolated-vm.ts +++ b/packages/server/src/jsRunner/vm/isolated-vm.ts @@ -97,11 +97,11 @@ export class IsolatedVM implements VM { return this } - withContext(context: Record, f: () => T) { + withContext(context: Record, executeWithContext: () => T) { this.addToContext(context) try { - return f() + return executeWithContext() } finally { this.removeFromContext(Object.keys(context)) } diff --git a/packages/server/src/jsRunner/vm/vm2.ts b/packages/server/src/jsRunner/vm/vm2.ts index 75e3899064..5825025d26 100644 --- a/packages/server/src/jsRunner/vm/vm2.ts +++ b/packages/server/src/jsRunner/vm/vm2.ts @@ -16,10 +16,10 @@ export class VM2 implements VM { this.vm.setGlobal("results", this.results) } - withContext(context: Record, fn: () => T): T { + withContext(context: Record, executeWithContext: () => T): T { this.vm.setGlobals(context) try { - return fn() + return executeWithContext() } finally { for (const key in context) { this.vm.setGlobal(key, undefined) diff --git a/packages/types/src/sdk/vm.ts b/packages/types/src/sdk/vm.ts index 314883e0c5..f1099524bc 100644 --- a/packages/types/src/sdk/vm.ts +++ b/packages/types/src/sdk/vm.ts @@ -1,4 +1,4 @@ export interface VM { execute(code: string): any - withContext(context: Record, fn: () => T): T + withContext(context: Record, executeWithContext: () => T): T }