fix(workflow): Fix multiple context leaks in reuseV8Context executor

[mjameswh]

What was changed

• Fix multiple context leaks and bugs in the reuseV8Context executor:

  • …by reassigning a new object to an existing shared global variable: globalThis.console = { ...globalThis.console, wfId: workflowInfo().workflowId } (fixes [Bug] Reusable VM allows context leak due to global variable reassignment #1476);

  • …by modifying one of Node's built in global objects: globalThis.Number.a = workflowInfo().workflowId;

  • …by deleting a previously set global variable: globalThis.a = 1; await sleep(1) ; delete globalThis.a ; await sleep(1) ; globalThis.a = (globalThis.a || 0) + 1; /* globalThis.a is 2 rather than 1 */

  • …by defining global symbol properties: const mySymbol = Symbol.for('...'); globalThis[mySymbol] = ... (fixes [Bug] Reusable VM allows context leak due to global symbol properties #1592).

Note: this is a second round over #1519. The code has changed considerably since the first previous review.

[bergundy]

Overall looks like an improvement to me. Thanks!

[antlai-temporal]

It seems we don't deep freeze Sets and Maps. Is this a concern? Do we need to document this somehow?

[antlai-temporal]

Can an object be frozen but not deepFrozen? If not, add a comment....

[mjameswh]

Not sure what needs to be clarified here. The official typedoc on Object.freeze() already indicates that freezing is shallow, and generally well known to advanced JS devs.

[antlai-temporal]

Is this comment correct? Do you need to remove this file?

[antlai-temporal]

Do any of these need to be deep frozen, or we assume they won't be abused...?

[mjameswh]

They will get deep frozen at the same time as all other global variables, after loading the script.

[mjameswh]

It seems we don't deep freeze Sets and Maps. Is this a concern? Do we need to document this somehow?

That limitation is documented in deepFreeze()'s typedoc. Not a concern at this point because there should be no instance of Map or Set in globalThis after V8 context initialization, nor directly in one of our preloaded modules (i.e. the SDK itself). I had previously added coverage for those because I used a Map as part of the V8 context swapping algorithm, but no longer need it.

[mjameswh]

I moved deepFreeze() to reusable-vm.ts (rather than the common package) as some implementation decisions (e.g. freezing functions, not freezing prototypes, not handling Maps and Sets, etc) are specific to the Reusable VM Workflow Sandbox use case, and may not be appropriate for other use cases. It therefore doesn't seem appropiate to consider this as a reusable utility function.