refactor(cli): replace deprecated vm2 with native node:vm runner by mbiernacik · Pull Request #2310 · dataform-co/dataform

mbiernacik · 2026-09-15T14:38:38Z

Summary

This PR replaces the deprecated and unmaintained vm2 library with native node:vm (VmRunner) across Dataform CLI, Core testing, and build targets.

Migrating to native node:vm removes an unmaintained third-party dependency, addresses deprecation warnings and known CVEs associated with vm2, and delivers substantial performance gains across compilation workflows.

Key Changes Across the Codebase

Native VmRunner Implementation (common/vm/vm_runner.ts & common/vm/BUILD):
- Implemented a standalone, reusable VmRunner under //common/vm (preventing circular layering with //cli and //testing).
- Uses node:vm.createContext and node:vm.compileFunction with scoped CommonJS require simulation.
- Enforces projectDir boundary containment (isPathContained) with an opt-in allowedExternalPaths configuration.
- Evicts failed module entries from moduleCache on throw so that errors re-throw consistently on subsequent require() calls.
- Supports configurable environment isolation via env and envAllowlist options.
- Exposes host Uint8Array and ArrayBuffer in sandbox globals to ensure cross-realm instanceof Uint8Array checks pass in protobufjs / JIT compilation.
- Memoizes resolution lookups via resolveCache and captures suppressed resolution errors as err.cause on MODULE_NOT_FOUND.
- Comprehensive unit test suite (common/vm/vm_runner_test.ts via //common/vm:tests).
CLI Compilation & JIT Worker Migration (cli/vm/compile.ts, cli/vm/jit_worker.ts):
- Migrated standard project compilation in cli/vm/compile.ts from NodeVM to VmRunner.
- Maintained compilation support for Dataform notebook actions (.ipynb, .md) via sourceExtensions.
- Migrated JIT worker compilation in cli/vm/jit_worker.ts to VmRunner.
Core Test Harness & Property Graphs (testing/run_core.ts, testing/BUILD, core/main_property_graphs_test.ts):
- Migrated testing/run_core.ts (runMainInVm) to execute tests through VmRunner.
- Cleaned up workaround logic in core/main_property_graphs_test.ts by removing the artificial vm2 empty CallSite stack workaround and unifying error assertions via asPlainGraph.
Packaging & Dependency Cleanup (package.json, yarn.lock, packages/@dataform/cli/BUILD, packages/rollup.config.js):
- Removed vm2 from package.json and yarn.lock.
- Removed vm2 from externals in packages/@dataform/cli/BUILD.
- Added vm and module to knownNodeBuiltins in packages/rollup.config.js.
Performance Benchmark (common/vm/vm_runner_benchmark.ts):
- Added a reproducible benchmark runnable via bazel run //common/vm:benchmark (~10,300 module requires/sec).

apilaskowski

Thanks for taking this on — removing vm2 is clearly the right call, and I think VmRunner is a good shape for the replacement.

Most of my inline comments are suggestions rather than objections. The one I'd genuinely flag is the module cache entry being retained when a module throws (cli/vm/vm_runner.ts:148), since that can turn a load failure into a silently incomplete compiledGraph rather than a clear error.

A few things that didn't fit on a specific line:

Security framing. The description says this "eliminates security vulnerabilities" and describes a "hermetic" runner with a "secure module resolver". node:vm doesn't really provide isolation — Node's docs are fairly explicit that it isn't a security mechanism, and vm2 existed because of that gap. If the CLI only ever compiles the invoking user's own repo on their own machine, then vm2's CVEs probably weren't reachable here either, so the security angle may not be doing much work in either direction. "Removes an unmaintained dependency and is meaningfully faster" seems like ample justification on its own. Would you consider rewording that part?
Feature list. I couldn't find implementations for three things the description mentions: execution timeouts, node:vm.Script (only vm.compileFunction is used), and script caching (moduleCache holds module exports rather than compiled scripts). Possibly planned and dropped, or I'm looking in the wrong place — either way it'd be good to bring the description back in line.
Benchmarks. Could the benchmark script land under cli/vm/? The numbers are a nice result and it'd be good to be able to re-run them. I did notice the vm2 baselines all end in .00 while the node:vm figures carry two decimals, which made me wonder whether the "before" column was measured or estimated.
Leftover vm2 references. core/session.ts:72 and :634, core/main_session_test.ts:164 and cli/index_compile_test.ts:102 still explain live workarounds in terms of "vm2's sandbox stack stripping". Since compile.ts:65-68 now says the opposite holds, is the __df_enter/__dataform_current_file file-stack machinery still needed for @dataform/core >= 3.0.57?

And lets make sure we are properly rebased on top of main branch now :)

apilaskowski · 2026-09-16T15:06:58Z

+    builtinModules: ["path"],
+    resolve: (moduleName, parentDirName) =>
+      path.join(parentDirName, path.relative(parentDirName, compileConfig.projectDir), moduleName),
+    sourceExtensions: ["js", "sql", "sqlx", "yaml", "yml", "ipynb", "md"],


ipynb and md look unrelated to the vm2 removal

Adding these two isn't mentioned in the description and doesn't have a test, and it has a couple of knock-on effects: notebook and markdown files now go through the SQLX compiler, and both extensions join allExtensions, so they participate in extension-less resolution and directory-index lookup.

Is this by accident?

Required for Dataform notebook actions (actions.yaml), which load .ipynb/.md via nativeRequire().asJson. Without registering these extensions, VmRunner tries to evaluate them as JS and throws a syntax error on "cells": [...]. Added a test.

This should be an extension and part of another PR then - adding support for Notebooks.

apilaskowski · 2026-09-16T15:39:25Z

After my review I would also like @Ikolina or someone who is longer in Dataform team to also take a look here.

apilaskowski · 2026-09-17T16:05:03Z

Thanks for addressing all the previous feedback so thoroughly — moving VmRunner to //common/vm and the test coverage look great!

Just one small thing I noticed on customResolve / isPathContained (left inline), and a quick heads-up that the PR description on GitHub might not have saved when you edited it (it still lists cli/vm/vm_runner.ts and mentions timeouts/vm.Script). Otherwise this looks ready to go!

kolina · 2026-09-21T21:39:24Z

+      // Check project node_modules directory directly (e.g. @dataform/core)
+      const nodeModulesCandidate = path.resolve(this.projectDir, "node_modules", moduleName);
+      const resolvedNodeModules = this.tryResolvePath(nodeModulesCandidate);
+      if (resolvedNodeModules) {


should you also call isPathContained here?

Done. Added containment check here as well.

kolina · 2026-09-21T21:42:12Z

+          e &&
+          e.code === "MODULE_NOT_FOUND" &&
+          e.message &&
+          e.message.includes("outside of project directory")


these checks are a bit clumsy taking into account that you throw an error just above so you could store a boolean flag and check it here

Done. Moved containment check outside try/catch so we don't catch/re-throw our own error.

kolina · 2026-09-21T21:43:09Z

+        if (!this.isPathContained(resolved)) {
+          const err: any = new Error(
+            `Cannot require '${moduleName}' outside of project directory '${this.projectDir}'`,
+          );
+          err.code = "MODULE_NOT_FOUND";
+          throw err;
+        }
+        this.resolveCache.set(cacheKey, resolved);
+        return resolved;


this logic is repeated in a lot of places, could we deduplicate it?

Done. Deduplicated into helper checkContainmentAndCache().

kolina · 2026-09-21T21:45:02Z

+          e &&
+          e.code === "MODULE_NOT_FOUND" &&
+          e.message &&
+          e.message.includes("outside of project directory")
+        ) {
+          throw e;
+        }


same: let's save into boolean flag before throwing error above to avoid this complicated condition

Done. Containment check moved outside try/catch.

kolina · 2026-09-21T22:02:19Z

+            const pkg = JSON.parse(fs.readFileSync(pkgPath, "utf8"));
+            if (pkg.main) {
+              const mainPath = path.resolve(candidatePath, pkg.main);
+              const resolvedMain = this.tryResolvePath(mainPath);


can this theoretically create infinite recursion?

Good catch. Added visitedDirs cycle tracking and guarded against mainPath === candidatePath.

kolina · 2026-09-21T23:02:10Z

        source = patchOldCoreCallerFile(source);
      }
      const compiledCode = compiler(source, filePath);
      return `


If v8 preserves call sites, we could conditionally apply this file-stack shim only for old versions?

Done. Gated by semver.lt(dataformCoreVersion, "3.0.57").

kolina · 2026-09-21T23:03:02Z

  const graphError = (fileName: string, message: string, extra: object = {}) => ({
    fileName,
    message,
-    stack: `Error: ${message}${graphStackTail}`,


you don't validate graphStackTail in new test version, is it intended?

Yes — graphStackTail was vm2's mocked CallSite stub (\n at CallSite {}). Under native node:vm we get real V8 call frames, so we now assert /\n\s+at /.

kolina · 2026-09-21T23:05:21Z

+        if (e && e.code === "MODULE_NOT_FOUND") {
+          throw e;
+        }


you swallow other errors here, is it intended?

Fixed. We now suppress only MODULE_NOT_FOUND and re-throw all unexpected errors.

kolina · 2026-09-21T23:08:34Z

-      require: {
-        builtin: [],
-        context: "sandbox",
-        external: { modules: ["@dataform/*"], transitive: false },


is this intended that you don't pass these external modules in new version?

Restored. Added allowedModules option to VmRunner and passed ["@dataform/*"] here.

kolina · 2026-09-21T23:09:49Z

+        }
+      }
+    } else {
+      env = { ...process.env };


not sure if default to all environment variables is a good default (unless we already have such test)

Done. Defaulted process.env to {}.

rafal-hawrylak · 2026-09-24T11:26:52Z

Please start with feature set we use from vm2 - both explicitly and implicitly. Mark with features are crucial and required to remain. Verify if NodeVM fully covers that feature set. Have you considered other alternatives that would satisfy the requirements? Please present the report back.

rafal-hawrylak · 2026-09-24T11:46:46Z

+    builtinModules: ["path"],
+    resolve: (moduleName, parentDirName) =>
+      path.join(parentDirName, path.relative(parentDirName, compileConfig.projectDir), moduleName),
+    sourceExtensions: ["js", "sql", "sqlx", "yaml", "yml", "ipynb", "md"],


This should be an extension and part of another PR then - adding support for Notebooks.

rafal-hawrylak · 2026-09-24T12:03:53Z

+      ...(options.sandbox || {}),
+    };
+
+    this.context = vm.createContext(sandbox);


The process shim exposes only env, cwd, version, versions, platform, arch. vm2's NodeVM proxied the real process. If @dataform/core's prebuilt bundle (or any user code) touches process.hrtime, process.exit, process.on, process.emit, process.nextTick, process.stdout, process.stderr it now TypeErrors. No integration test verifies the real core bundle boots against this shim in either the CLI or the JiT worker path. Please either widen the shim to a superset of what core needs, or add a smoke test that runs the current core bundle end-to-end through VmRunner.

rafal-hawrylak · 2026-09-24T12:05:54Z

+    const vm = new VmRunner({
+      projectDir,
+      builtinModules: [],
+      allowedModules: ["@dataform/*"],


vm2 had external: { modules: ["@dataform/"], transitive: false }. The new allowedModules: ["@dataform/"] drops the transitive-blocking semantics and means that if @dataform/core does ANY dynamic require("some-runtime-dep") (not a static bundled import), it'll be rejected. Confirm the core bundle has no dynamic requires at runtime, or widen the allowlist. Add a test covering the hasProjectLocalCore = true path (currently only a unit test of the VmRunner primitive exists - no test wires jit_worker end-to-end).

rafal-hawrylak · 2026-09-24T12:09:34Z

+    this.nodeBuiltinSet = new Set(nodeBuiltins);
+
+    let env: Record<string, string | undefined>;
+    if (options.env !== undefined) {


Priority env wins over envAllowlist wins over empty default is undocumented, and if a caller passes BOTH env and envAllowlist, envAllowlist is silently ignored. Add a JSDoc line, or throw when both are set.
Add the test.

rafal-hawrylak · 2026-09-24T12:14:27Z

      });
-      global.workflowSettingsYaml = (function() { try { return require("./workflow_settings.yaml"); } catch(e) { console.error("YAML require failed run_core:", e); } })();
-      global.dataformJson = (function() { try { return require("./dataform.json"); } catch(e) {} })();
+      ${hasWorkflowSettingsYaml ? 'global.workflowSettingsYaml = require("./workflow_settings.yaml");' : ""}


Removed the diagnostic console.error("YAML require failed run_core:", e) when replacing try/catch require("./workflow_settings.yaml") with a hasWorkflowSettingsYaml guard. If workflow_settings.yaml exists but is malformed, the failure surface changes silently (old: swallowed with a log; new: thrown as compile error). Was it indended?

rafal-hawrylak · 2026-09-24T12:16:17Z

+import { suite, test } from "df/testing";
+import { TmpDirFixture } from "df/testing/fixtures";
+
+suite("VmRunner", ({ afterEach }) => {


Missing coverage:
(a) require("node:path") prefixed builtin
(b) console: "off" swallowing logs
(c) mocked @dataform/core path where hasProjectLocalCore=false and a call inside the vm tries to re-require("@dataform/core") (jit_worker scenario).

Add three tiny cases.

rafal-hawrylak · 2026-09-24T12:20:08Z

+      options.builtinModules !== undefined ? options.builtinModules : ["path"],
+    );
+    this.mockModules = options.mockModules || {};
+    this.customResolve = options.resolve;


Any reason for names not matching?

rafal-hawrylak · 2026-09-24T12:23:00Z

+    });
+  }
+
+  private checkContainmentAndCache(


Any reason to mix two responsibilities?

mbiernacik · 2026-09-24T16:23:55Z

Technical Comparison: vm2 vs. node:vm (VmRunner), Alternatives, and Security Analysis

1. Feature Matrix: vm2 (NodeVM) vs. node:vm (VmRunner)

Feature / Capability	`vm2` (`NodeVM`)	`VmRunner` (`node:vm`)	Status in Dataform
CommonJS Module System (`require`, `exports`)	Built-in	Custom implementation	100% covered (handles relative, project-relative paths, and caching)
Compiler Hook (`compiler`)	Supported	Supported	100% covered (compiles `.sqlx`, `.js`, `.yaml`, `.ipynb`, `.md`)
Custom Source Extensions (`sourceExtensions`)	Supported	Supported	100% covered
Module Mocking (`mockModules`)	Supported	Supported	100% covered (used for `@dataform/core`)
Built-in Module Restriction	Supported	Supported	100% covered (restricted to `path` by default; blocks `fs`, `child_process`)
Path Containment (project jail)	Partial	Enhanced	`isPathContained` validates against `fs.realpathSync` (symlink-proof)
Environment Variable Isolation	Manual	Enhanced	Defaults to empty `process.env: {}` (prevents host secret leakage)
V8 CallSite & Stack Traces	Broken	Native V8	Key improvement (removes line/path stripping workarounds)
Execution Performance	Low (Proxy overhead)	Native V8	~10x faster module evaluation
RCE Security Boundary	False sense (20+ CVEs)	Non-security boundary	Matches real-world capability (no security theater)
Maintenance Status	Discontinued (Dead)	Active (Node.js core)	Eliminates critical technical debt and CVE exposure

2. Evaluation of Alternatives

Solution	RCE Isolation	Performance	CommonJS Compatibility	Native Dependencies
`VmRunner` (`node:vm`)	In-process	100% (V8 JIT)	Native / Ready	None (Pure JS)
`isolated-vm`	Hard (V8 Isolate)	~85%	Requires custom loader	Requires C++ (`node-gyp`)
`quickjs-emscripten`	Hard (WASM)	5–10%	Requires custom loader	None (`.wasm` file)
`ses` (Hardened JS)	Medium	~90%	Complex integration	None

3. Threat Model & Security Analysis

A. Compromised / Infected Third-Party npm Packages

Installation Phase (npm install): Dataform CLI runs npm i --ignore-scripts, which blocks automated pre/post-install shell execution.
Compilation Phase: VmRunner isolates the runtime environment (no fs, child_process, or net; empty process.env protects host tokens/secrets). A sophisticated V8 escape would break both node:vm and vm2.
Google Cloud Defense: Cloud security does not rely on JavaScript sandboxes. Compilation tasks run inside sandboxed OS containers.

B. Dynamic Code Download in `includes/` (e.g. `fetch` / `http.get`)

In VmRunner: fetch is not exposed in the sandbox; http, https, and net are blocked. Furthermore, Dataform templates are synchronous (Promise evaluation yields [object Promise], causing syntax errors).
In Google Cloud: Cloud compilation workers have no outbound internet access (No Egress). Any outbound network call immediately fails with ETIMEDOUT.

C. Code Lifecycle: How and When JavaScript Executes

Compilation (where JS executes): Embedded js {} blocks and includes/ files execute solely during Node.js compilation (dataform compile or Cloud CompilationResult), generating static SQL strings.
Execution in BigQuery (where JS does not exist): BigQuery receives only pure, static SQL. JavaScript is never executed inside the database engine.

mbiernacik requested a review from a team as a code owner September 15, 2026 14:38

mbiernacik requested review from a team and apilaskowski and removed request for a team September 15, 2026 14:38

apilaskowski requested changes Sep 16, 2026

View reviewed changes

ashish10alex mentioned this pull request Sep 17, 2026

dataform compile ~4x slower since @dataform/core 3.0.61 (CLI only) — encoded graph now crosses the vm2 bridge as a host Buffer #2298

Closed

mbiernacik force-pushed the migrate-away-from-vm2 branch 2 times, most recently from 95db3da to 3fc433c Compare September 17, 2026 14:24

mbiernacik requested a review from apilaskowski September 17, 2026 14:41

apilaskowski reviewed Sep 17, 2026

View reviewed changes

Comment thread common/vm/vm_runner.ts Outdated

Comment thread cli/vm/vm_runner.ts Outdated

kolina self-requested a review September 17, 2026 18:57

mbiernacik requested a review from apilaskowski September 18, 2026 07:43

kolina requested changes Sep 21, 2026

View reviewed changes

mbiernacik requested a review from kolina September 22, 2026 14:25

rafal-hawrylak requested changes Sep 24, 2026

View reviewed changes

mbiernacik added 3 commits September 25, 2026 11:28

chore: migrate away from deprecated vm2 to node:vm

4101a6e

Enforce path containment in resolver and drop redundant resolve hooks

d81abda

Address review comments on VmRunner migration

6c6cd2a

mbiernacik force-pushed the migrate-away-from-vm2 branch from 6c1f2c9 to 6c6cd2a Compare September 25, 2026 11:48

rafal-hawrylak self-requested a review September 25, 2026 14:21

Conversation

mbiernacik commented Sep 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Key Changes Across the Codebase

Uh oh!

apilaskowski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apilaskowski commented Sep 16, 2026

Uh oh!

Uh oh!

Uh oh!

apilaskowski commented Sep 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafal-hawrylak commented Sep 24, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mbiernacik commented Sep 15, 2026 •

edited

Loading

apilaskowski left a comment •

edited

Loading

B. Dynamic Code Download in `includes/` (e.g. `fetch` / `http.get`)