diff --git a/packages/common/src/type-helpers.ts b/packages/common/src/type-helpers.ts index 869146cd6..bce5328b7 100644 --- a/packages/common/src/type-helpers.ts +++ b/packages/common/src/type-helpers.ts @@ -224,74 +224,32 @@ export function SymbolBasedInstanceOfError(markerName: string): // reaching variables defined using internal scopes. This implementation specifically look for and deal // with the case of the Map or Set classes, but it is impossible to cover all potential cases. export function deepFreeze(object: T, visited = new WeakSet(), path = 'root'): T { - if (typeof object !== 'object' || object == null || Object.isFrozen(object) || visited.has(object)) return object; + if (object == null || visited.has(object) || (typeof object !== 'object' && typeof object !== 'function')) + return object; visited.add(object); - - if (object.constructor?.name === 'Map' || object instanceof Map) { - const map = object as unknown as Map; - for (const [k, v] of map.entries()) { - deepFreeze(k, visited, `${path}[key:${k}]`); - deepFreeze(v, visited, `${path}[value:${k}]`); - } - map.set = frozenMapSet; - map.delete = frozenMapDelete; - map.clear = frozenMapClear; - } - - if (object.constructor?.name === 'Set' || object instanceof Set) { - const set = object as unknown as Set; - for (const v of set.values()) { - deepFreeze(v, visited, `${path}.${v}`); - } - set.add = frozenSetAdd; - set.delete = frozenSetDelete; - set.clear = frozenSetClear; - } - - // Retrieve the property names defined on object - const propNames = Object.getOwnPropertyNames(object); - - // Freeze properties before freezing self - for (const name of propNames) { - const value = (object as any)[name]; - - if (value && typeof value === 'object') { - try { - deepFreeze(value, visited, `${path}.${name}`); - } catch (err) { - if (typeof value !== 'object' || value.constructor.name !== 'Uint8Array') { - console.log(`Failed to freeze ${path}.${name}`, err); + if (Object.isFrozen(object)) return object; + + if (typeof object === 'object') { + // Retrieve the property names defined on object + const propNames = Object.getOwnPropertyNames(object); + + // Freeze properties before freezing self + for (const name of propNames) { + const value = (object as any)[name]; + + if (value && typeof value === 'object') { + try { + deepFreeze(value, visited, `${path}.${name}`); + } catch (err) { + // FIXME: Remove before merging this PR + if (value.constructor.name !== 'Uint8Array') { + console.log(`Failed to freeze ${path}.${name}`, err); + } + // This is okay, there are some typed arrays that cannot be frozen (encodingKeys) } - // This is okay, there are some typed arrays that cannot be frozen (encodingKeys) } - } else if (typeof value === 'function') { - Object.freeze(value); } } return Object.freeze(object); } - -function frozenMapSet(key: any, _value: any): Map { - throw `Can't add property ${key}, map is not extensible`; -} - -function frozenMapDelete(key: any): boolean { - throw `Can't delete property ${key}, map is frozen`; -} - -function frozenMapClear() { - throw "Can't clear map, map is frozen"; -} - -function frozenSetAdd(key: any): Set { - throw `Can't add key ${key}, set is not extensible`; -} - -function frozenSetDelete(key: any): boolean { - throw `Can't delete key ${key}, set is not extensible`; -} - -function frozenSetClear() { - throw "Can't clear set, set is frozen"; -} diff --git a/packages/test/src/test-isolation.ts b/packages/test/src/test-isolation.ts index 46357ff49..aead354b4 100644 --- a/packages/test/src/test-isolation.ts +++ b/packages/test/src/test-isolation.ts @@ -1,19 +1,14 @@ -import { randomUUID } from 'crypto'; -import { TestFn, ImplementationFn } from 'ava'; +import { ExecutionContext, ImplementationFn } from 'ava'; import { ApplicationFailure, arrayFromPayloads } from '@temporalio/common'; -import { bundleWorkflowCode, WorkflowBundle } from '@temporalio/worker'; -import { sleep, workflowInfo } from '@temporalio/workflow'; +import * as wf from '@temporalio/workflow'; import { WorkflowFailedError } from '@temporalio/client'; -import { test as anyTest, bundlerOptions, Worker, REUSE_V8_CONTEXT, TestWorkflowEnvironment } from './helpers'; +import { makeTestFunction, Context, helpers } from './helpers-integration'; +import { REUSE_V8_CONTEXT } from './helpers'; -interface Context { - env: TestWorkflowEnvironment; - taskQueue: string; - workflowBundle: WorkflowBundle; - createWorker(): Promise; -} - -const test = anyTest as TestFn; +const test = makeTestFunction({ + workflowsPath: __filename, + workflowInterceptorModules: [__filename], +}); const withReusableContext = test.macro<[ImplementationFn<[], Context>]>(async (t, fn) => { if (!REUSE_V8_CONTEXT) { @@ -23,165 +18,215 @@ const withReusableContext = test.macro<[ImplementationFn<[], Context>]>(async (t await fn(t); }); -test.before(async (t) => { - t.context.env = await TestWorkflowEnvironment.createLocal(); - t.context.workflowBundle = await bundleWorkflowCode({ workflowsPath: __filename, ...bundlerOptions }); -}); +// -test.beforeEach(async (t) => { - t.context.taskQueue = t.title.replace(/ /g, '_'); - t.context.createWorker = async () => { - const { env, workflowBundle, taskQueue } = t.context; - return await Worker.create({ - connection: env.nativeConnection, - workflowBundle, - taskQueue, - }); - }; +test('globalThis can be safely mutated', async (t) => { + await assertObjectSafelyMutable(t, globalThisMutatorWorkflow); }); -test.after.always(async (t) => { - await t.context.env.teardown(); -}); - -export async function globalMutator(): Promise { - const global = globalThis as { a?: number }; - global.a = (global.a || 0) + 1; - await sleep(1); - global.a = (global.a || 0) + 1; - return global.a; +export async function globalThisMutatorWorkflow(): Promise<(number | null)[]> { + return basePropertyMutator(() => globalThis as any); } -test('Global state is isolated and maintained between activations', async (t) => { - const { createWorker, taskQueue, env } = t.context; - const worker = await createWorker(); - await worker.runUntil(async () => { - const res1 = await env.client.workflow.execute(globalMutator, { taskQueue, workflowId: randomUUID() }); - const res2 = await env.client.workflow.execute(globalMutator, { taskQueue, workflowId: randomUUID() }); - t.is(res1, 2); - t.is(res2, 2); - }); +// + +test("V8's built-in global objects are frozen, but can be safely reassigned", withReusableContext, async (t) => { + await assertObjectImmutable(t, v8BuiltinGlobalObjectMutatorWorkflow); + // here + await assertObjectSafelyMutable(t, v8BuiltinGlobalObjectReassignWorkflow); }); -export async function sdkPropertyMutator(): Promise { +export async function v8BuiltinGlobalObjectMutatorWorkflow(): Promise<(number | null)[]> { + return basePropertyMutator(() => globalThis.Math as any); +} + +export async function v8BuiltinGlobalObjectReassignWorkflow(): Promise<(number | null)[]> { try { - (arrayFromPayloads as any).a = 1; - } catch (err) { - throw ApplicationFailure.fromError(err); + globalThis.Math = Object.create(globalThis.Math); + return basePropertyMutator(() => globalThis.Math); + } catch (e) { + if (!(e instanceof ApplicationFailure)) { + throw ApplicationFailure.fromError(e); + } + throw e; } } -test('SDK Module state is frozen', withReusableContext, async (t) => { - const { createWorker, taskQueue, env } = t.context; - const worker = await createWorker(); - const err = (await worker.runUntil( - t.throwsAsync(env.client.workflow.execute(sdkPropertyMutator, { taskQueue, workflowId: randomUUID() })) - )) as WorkflowFailedError; - t.is(err.cause?.message, 'Cannot add property a, object is not extensible'); +// + +test("V8's built-in global functions are frozen, and can't be reassigned", withReusableContext, async (t) => { + await assertObjectImmutable(t, v8BuiltinGlobalFunctionMutatorWorkflow); + await assertObjectImmutable(t, v8BuiltinGlobalFunctionReassignWorkflow); + + // FIXME: What about built-in function prototypes? + // They are often modified by polyfills, which would be loaded as part + // of Workflow Codeā€¦ + await assertObjectImmutable(t, v8BuiltinGlobalFunctionReassignWorkflow); }); -const someArr: number[] = []; +export async function v8BuiltinGlobalFunctionMutatorWorkflow(): Promise<(number | null)[]> { + return basePropertyMutator(() => globalThis.Array as any); +} -export async function modulePropertyMutator(): Promise { - someArr.push(1); - await sleep(1); - someArr.push(2); - return someArr; +export async function v8BuiltinGlobalFunctionReassignWorkflow(): Promise<(number | null)[]> { + try { + const originalArray = globalThis.Array; + globalThis.Array = ((...args: any[]) => originalArray(...args)) as any; + return basePropertyMutator(() => globalThis.Array); + } catch (e) { + if (!(e instanceof ApplicationFailure)) { + throw ApplicationFailure.fromError(e); + } + throw e; + } } -test('Module state is isolated and maintained between activations', async (t) => { - const { createWorker, taskQueue, env } = t.context; +// + +// test("SDK's globals can be reassigned", async (t) => { +// await assertObjectSafelyMutable(t, sdkGlobalsReassignment); +// }); + +// export async function sdkGlobalsReassignment(): Promise<(number | null)[]> { +// try { +// // The SDK's provided `console` object is frozen. +// // Replace that global with a clone that is not frozen. +// globalThis.console = { ...globalThis.console }; +// return basePropertyMutator(() => globalThis.console); +// } catch (e) { +// if (!(e instanceof ApplicationFailure)) { +// throw ApplicationFailure.fromError(e); +// } +// throw e; +// } +// } + +// // + +// test("SDK's API functions are frozen 2", withReusableContext, async (t) => { +// await assertObjectSafelyMutable(t, sdkPropertyMutatorWorkflow2); +// }); + +// export async function sdkPropertyMutatorWorkflow2(): Promise<(number | null)[]> { +// return basePropertyMutator(() => wf as any); +// } + +// // + +// test("SDK's API functions are frozen 1", withReusableContext, async (t) => { +// await assertObjectImmutable(t, sdkPropertyMutatorWorkflow1); +// }); + +// export async function sdkPropertyMutatorWorkflow1(): Promise<(number | null)[]> { +// return basePropertyMutator(() => arrayFromPayloads as any); +// } + +// // + +// test('Module state is isolated and maintained between activations', async (t) => { +// await assertObjectSafelyMutable(t, modulePropertyMutator); +// }); + +// const moduleScopedObject: any = {}; +// export async function modulePropertyMutator(): Promise<(number | null)[]> { +// return basePropertyMutator(() => moduleScopedObject); +// } + +// + +async function assertObjectSafelyMutable( + t: ExecutionContext, + workflow: () => Promise<(number | null)[]> +): Promise { + const { createWorker, startWorkflow } = helpers(t); const worker = await createWorker(); await worker.runUntil(async () => { - const [res1, res2] = await Promise.all([ - env.client.workflow.execute(modulePropertyMutator, { taskQueue, workflowId: randomUUID() }), - env.client.workflow.execute(modulePropertyMutator, { taskQueue, workflowId: randomUUID() }), - ]); - t.deepEqual(res1, [1, 2]); - t.deepEqual(res2, [1, 2]); + const wf1 = await startWorkflow(workflow); + const wf2 = await startWorkflow(workflow); + t.deepEqual(await wf1.result(), [null, 1, 1, 2, 2, null, null, 1]); + t.deepEqual(await wf2.result(), [null, 1, 1, 2, 2, null, null, 1]); }); -}); - -const someMap: Map = new Map(); - -export async function moduleMapMutator(): Promise<[number, number]> { - someMap.set('test', (someMap.get('test') || 0) + 1); - const middle = someMap.get('test') as number; - await sleep(1); - someMap.set('test', (someMap.get('test') || 0) + 1); - const final = someMap.get('test') as number; - return [middle, final]; } -test('Deep Freeze reaches Map values', async (t) => { - const { createWorker, taskQueue, env } = t.context; +async function assertObjectImmutable( + t: ExecutionContext, + workflow: () => Promise<(number | null)[]> +): Promise { + const { createWorker, startWorkflow } = helpers(t); const worker = await createWorker(); await worker.runUntil(async () => { - const [res1, res2] = await Promise.all([ - env.client.workflow.execute(moduleMapMutator, { taskQueue, workflowId: randomUUID() }), - env.client.workflow.execute(moduleMapMutator, { taskQueue, workflowId: randomUUID() }), - ]); - t.deepEqual(res1, [1, 2]); - t.deepEqual(res2, [1, 2]); + const wf1 = await startWorkflow(workflow); + const err = await t.throwsAsync(wf1.result(), { instanceOf: WorkflowFailedError }); + t.is(err?.cause?.message, 'Cannot add property a, object is not extensible'); + t.deepEqual((err?.cause as ApplicationFailure)?.details, [[null]]); }); -}); +} -export async function sharedGlobalMutator(): Promise { +// Given the object returned by `getObject()`, this function can be used to +// assert any of these three possible scenarios: +// 1. The object can't be mutated from Workflows (i.e. the object is frozen); +// - or - +// 2. The object can be safetly mutated from Workflows, meaning that: +// 2.1. Can add new properties to the object (i.e. the object is not frozen); +// 2.2. Properties added on the object from one workflow execution don't leak to other workflows; +// 2.3. Properties added on the object from one workflow are maintained between activations of that workflow; +// 2.4. Properties added then deleted from the object don't reappear on subsequent activations. +// - or - +// 3. The object can be mutable from Workflows, without isolation guarantees. +// This last case is notably desirable +async function basePropertyMutator(getObject: () => any): Promise<(number | null)[]> { + const checkpoints: (number | null)[] = []; + + // Very important: do not cache the result of getObject() to a local variable; + // in some schenario, caching would defeat the purpose of this test. try { - (Array as any).a = 1; - } catch (err) { - throw ApplicationFailure.fromError(err); - } -} + checkpoints.push(getObject().a); // Expect null + getObject().a = (getObject().a || 0) + 1; + checkpoints.push(getObject().a); // Expect 1 -test('Shared global state is frozen', withReusableContext, async (t) => { - const { createWorker, taskQueue, env } = t.context; - const worker = await createWorker(); - const err = (await worker.runUntil( - t.throwsAsync(env.client.workflow.execute(sharedGlobalMutator, { taskQueue, workflowId: randomUUID() })) - )) as WorkflowFailedError; - t.is(err.cause?.message, 'Cannot add property a, object is not extensible'); -}); + await wf.sleep(1); -export async function sharedGlobalReassignment(): Promise<[string, string, string]> { - type ConsoleExtended = Console & { wfid: string }; - // Replace the `console` global by a new object - globalThis.console = { ...globalThis.console, wfid: workflowInfo().workflowId } as ConsoleExtended; - const middle = (globalThis.console as ConsoleExtended).wfid; - await sleep(50); - return [workflowInfo().workflowId, middle, (globalThis.console as ConsoleExtended).wfid]; -} + checkpoints.push(getObject().a); // Expect 1 + getObject().a = (getObject().a || 0) + 1; + checkpoints.push(getObject().a); // Expect 2 -test('Reassign shared global state', withReusableContext, async (t) => { - const { createWorker, taskQueue, env } = t.context; - const worker = await createWorker(); - await worker.runUntil(async () => { - const [res1, res2] = await Promise.all([ - env.client.workflow.execute(sharedGlobalReassignment, { taskQueue, workflowId: randomUUID() }), - env.client.workflow.execute(sharedGlobalReassignment, { taskQueue, workflowId: randomUUID() }), - ]); - t.deepEqual(res1[0], res1[1]); - t.deepEqual(res1[0], res1[2]); - t.deepEqual(res2[0], res2[1]); - t.deepEqual(res2[0], res2[2]); - }); -}); + await wf.sleep(1); + + checkpoints.push(getObject().a); // Expect 2 + delete getObject().a; + checkpoints.push(getObject().a); // Expect null + + await wf.sleep(1); -export async function globalMutatorAndDestructor(): Promise { - const global = globalThis as { a?: number }; - global.a = (global.a || 0) + 1; - await sleep(1); - delete global.a; - await sleep(1); - global.a = (global.a || 0) + 1; - return global.a; + checkpoints.push(getObject().a); // Expect null + getObject().a = (getObject().a || 0) + 1; + checkpoints.push(getObject().a); // Expect 1 + + return checkpoints; + } catch (e) { + throw ApplicationFailure.fromError(e, { details: [checkpoints] }); + } } -test('Set then Delete a global property', withReusableContext, async (t) => { - const { createWorker, taskQueue, env } = t.context; - const worker = await createWorker(); - await worker.runUntil(async () => { - const res = await env.client.workflow.execute(globalMutatorAndDestructor, { taskQueue, workflowId: randomUUID() }); - t.is(res, 1); - }); -}); +// Given the object returned by `getObject()`, this function can be used to assert +// either of these two scenarios: +// 1. The object can't be mutated from Workflows (i.e. the object is frozen); +// - or - +// 2. The object can be safetly mutated from Workflows, meaning that: +// 2.1. Can add new properties to the object (i.e. the object is not frozen); +// 2.2. Properties added on the object from one workflow execution don't leak to other workflows; +// 2.3. Properties added on the object from one workflow are maintained between activations of that workflow; +// 2.4. Properties added then deleted from the object don't reappear on subsequent activations. +// async function basePropertyReassign(getObject: () => any, propName: string): Promise<(number | null)[]> { +// try { +// const prop = getObject()[propName]; +// const newProp = cloneObject; +// getObject()[propName] = newProp; +// return basePropertyMutator(() => getObject()[propName]); +// } catch (e) { +// if (!(e instanceof ApplicationFailure)) { +// throw ApplicationFailure.fromError(e); +// } +// throw e; +// } +// } diff --git a/packages/worker/src/workflow/reusable-vm.ts b/packages/worker/src/workflow/reusable-vm.ts index be623cfd9..8f0b067d0 100644 --- a/packages/worker/src/workflow/reusable-vm.ts +++ b/packages/worker/src/workflow/reusable-vm.ts @@ -88,18 +88,25 @@ export class ReusableVMWorkflowCreator implements WorkflowCreator { // defines the built-in global properties. We need vm.runInContext( ` - const pristine = globalThis.__pristine; - delete globalThis.__pristine; - for (const name of Object.getOwnPropertyNames(globalThis)) { - const descriptor = Object.getOwnPropertyDescriptor(globalThis, name); - pristine.set(name, descriptor); + { + const pristine = globalThis.__pristine; + delete globalThis.__pristine; + for (const name of Object.getOwnPropertyNames(globalThis)) { + const descriptor = Object.getOwnPropertyDescriptor(globalThis, name); + pristine.set(name, descriptor); + } } `, this.context ); for (const [k, v] of this.pristine.entries()) { if (k !== 'globalThis') { - deepFreeze(v.value); + console.log(`Deep freezing pristine ${k}`); + v.value = deepFreeze(v.value); + if (typeof v.value === 'function') { + console.log(` ... non-writable`); + v.writable = false; + } } } for (const v of sharedModules.values()) { @@ -130,7 +137,7 @@ export class ReusableVMWorkflowCreator implements WorkflowCreator { async createWorkflow(options: WorkflowCreateOptions): Promise { const context = this.context; const pristine = this.pristine; - const bag: Map = new Map(); + const bag: Map = new Map(pristine); const { isolateExecutionTimeoutMs } = this; const workflowModule: WorkflowModule = new Proxy( {}, @@ -172,9 +179,13 @@ export class ReusableVMWorkflowCreator implements WorkflowCreator { vm.runInContext( ` for (const [name, descriptor] of globalThis.__TEMPORAL_ARGS__) { - delete globalThis[name]; + // if (name !== '__TEMPORAL_ACTIVATOR__') { + // debugger; + // } if (descriptor) { - Object.defineProperty(globalThis, name, descriptor); + Object.defineProperty(globalThis, name, { ...descriptor, writable: true }); + } else { + delete globalThis[name]; } } delete globalThis.__TEMPORAL_ARGS__;`,