fix(nodevm): align behavior with node (#13590)

2026-02-13 12:29:07 +00:00 · 2024-08-30 13:34:18 +05:00
parent 682b3730a1
commit 59eb5515c5
3 changed files with 228 additions and 151 deletions
--- a/src/bun.js/bindings/NodeVM.cpp
+++ b/src/bun.js/bindings/NodeVM.cpp
@@ -45,15 +45,20 @@ public:
    {
        auto& vm = globalObject->vm();
        ScriptOptions opts;
+        JSObject* options;
        bool any = false;
        if (!optionsArg.isUndefined()) {
-            if (!optionsArg.isObject()) {
+            if (optionsArg.isObject()) {
+                options = asObject(optionsArg);
+            } else if (optionsArg.isString()) {
+                options = constructEmptyObject(globalObject);
+                options->putDirect(vm, Identifier::fromString(vm, "filename"_s), optionsArg);
+            } else {
                auto scope = DECLARE_THROW_SCOPE(vm);
-                throwVMTypeError(globalObject, scope, "options must be an object"_s);
+                throwVMTypeError(globalObject, scope, "options must be an object or a string"_s);
                failed = true;
                return std::nullopt;
            }
-            JSObject* options = asObject(optionsArg);

            if (JSValue filenameOpt = options->getIfPropertyExists(globalObject, builtinNames(vm).filenamePublicName())) {
                if (filenameOpt.isString()) {
@@ -300,8 +305,7 @@ JSC_DEFINE_HOST_FUNCTION(vmModuleRunInThisContext, (JSGlobalObject * globalObjec
 {
    auto& vm = globalObject->vm();
    auto sourceStringValue = callFrame->argument(0);
-    JSValue contextObjectValue = callFrame->argument(1);
-    JSValue optionsObjectValue = callFrame->argument(2);
+    JSValue optionsObjectValue = callFrame->argument(1);
    auto throwScope = DECLARE_THROW_SCOPE(vm);

    if (!sourceStringValue.isString()) {
@@ -311,15 +315,6 @@ JSC_DEFINE_HOST_FUNCTION(vmModuleRunInThisContext, (JSGlobalObject * globalObjec

    auto sourceString = sourceStringValue.toWTFString(globalObject);

-    if (!contextObjectValue || contextObjectValue.isUndefinedOrNull()) {
-        contextObjectValue = JSC::constructEmptyObject(globalObject);
-    }
-
-    if (UNLIKELY(!contextObjectValue || !contextObjectValue.isObject())) {
-        throwTypeError(globalObject, throwScope, "Context must be an object"_s);
-        return JSValue::encode({});
-    }
-
    ScriptOptions options;
    {
        bool didThrow = false;
@@ -334,19 +329,13 @@ JSC_DEFINE_HOST_FUNCTION(vmModuleRunInThisContext, (JSGlobalObject * globalObjec
    SourceCode source(
        JSC::StringSourceProvider::create(sourceString, JSC::SourceOrigin(WTF::URL::fileURLWithFileSystemPath(options.filename)), options.filename, JSC::SourceTaintedOrigin::Untainted, TextPosition(options.lineOffset, options.columnOffset)),
        options.lineOffset.zeroBasedInt(), options.columnOffset.zeroBasedInt());
-    auto* zigGlobal = reinterpret_cast<Zig::GlobalObject*>(globalObject);
-    JSObject* context = asObject(contextObjectValue);
-
-    auto proxyStructure = zigGlobal->globalProxyStructure();
-    auto proxy = JSGlobalProxy::create(vm, proxyStructure);
-    proxy->setTarget(vm, globalObject);
-    context->setPrototypeDirect(vm, proxy);

    auto* executable = JSC::DirectEvalExecutable::create(
        globalObject, source, NoLexicallyScopedFeatures, DerivedContextType::None, NeedsClassFieldInitializer::No, PrivateBrandRequirement::None,
        false, false, EvalContextType::None, nullptr, nullptr);
    RETURN_IF_EXCEPTION(throwScope, {});

+    JSObject* context = asObject(JSC::constructEmptyObject(globalObject));
    JSScope* contextScope = JSWithScope::create(vm, globalObject, globalObject->globalScope(), context);
    auto catchScope = DECLARE_CATCH_SCOPE(vm);
    JSValue result = vm.interpreter.executeEval(executable, globalObject, contextScope);
@@ -393,10 +382,10 @@ JSC_DEFINE_HOST_FUNCTION(scriptRunInNewContext, (JSGlobalObject * globalObject,
    auto* targetContext = NodeVMGlobalObject::create(
        vm, zigGlobal->NodeVMGlobalObjectStructure());

-    // auto proxyStructure = JSGlobalProxy::createStructure(vm, globalObject, JSC::jsNull());
-    // auto proxy = JSGlobalProxy::create(vm, proxyStructure);
-    // proxy->setTarget(vm, targetContext);
-    // context->setPrototypeDirect(vm, proxy);
+    auto proxyStructure = JSGlobalProxy::createStructure(vm, globalObject, JSC::jsNull());
+    auto proxy = JSGlobalProxy::create(vm, proxyStructure);
+    proxy->setTarget(vm, targetContext);
+    context->setPrototypeDirect(vm, proxy);

    JSScope* contextScope = JSWithScope::create(vm, targetContext, targetContext->globalScope(), context);
    return runInContext(globalObject, script, targetContext, contextScope, callFrame->argument(0));
@@ -407,21 +396,14 @@ JSC_DEFINE_HOST_FUNCTION(scriptRunInThisContext, (JSGlobalObject * globalObject,
    JSValue thisValue = callFrame->thisValue();
    auto* script = jsDynamicCast<NodeVMScript*>(thisValue);
    auto throwScope = DECLARE_THROW_SCOPE(vm);
+    // TODO: options
+    // JSValue optionsObjectValue = callFrame->argument(0);

    if (UNLIKELY(!script)) {
        return throwVMTypeError(globalObject, throwScope, "Script.prototype.runInThisContext can only be called on a Script object"_s);
    }

-    JSValue contextArg = callFrame->argument(0);
-    if (!contextArg || contextArg.isUndefinedOrNull()) {
-        contextArg = JSC::constructEmptyObject(globalObject);
-    }
-
-    if (!contextArg.isObject()) {
-        return throwVMTypeError(globalObject, throwScope, "context must be an object"_s);
-    }
-
-    JSObject* context = asObject(contextArg);
+    JSObject* context = asObject(JSC::constructEmptyObject(globalObject));
    JSWithScope* contextScope = JSWithScope::create(vm, globalObject, globalObject->globalScope(), context);

    return runInContext(globalObject, script, globalObject->globalThis(), contextScope, callFrame->argument(1));
--- a/test/js/node/crypto/crypto.key-objects.test.ts
+++ b/test/js/node/crypto/crypto.key-objects.test.ts
@@ -22,7 +22,7 @@ import {
  generateKey,
 } from "crypto";
 import { test, it, expect, describe } from "bun:test";
-import { createContext, Script } from "node:vm";
+import { createContext, Script, runInThisContext, runInContext } from "node:vm";
 import fs from "fs";
 import path from "path";
 import { isWindows } from "harness";
@@ -1407,74 +1407,105 @@ describe("crypto.KeyObjects", () => {
    expect(privateKey.asymmetricKeyDetails?.modulusLength).toBe(513);
  });

-  function testRunInContext(fn: any) {
-    test("can generate key", () => {
-      const context = createContext({ generateKeySync });
-      const result = fn(`generateKeySync("aes", { length: 128 })`, context);
-      expect(result).toBeDefined();
-      const keybuf = result.export();
-      expect(keybuf.byteLength).toBe(128 / 8);
-    });
-    test("can be used on another context", () => {
-      const context = createContext({ generateKeyPairSync, assertApproximateSize, testEncryptDecrypt, testSignVerify });
-      const result = fn(
-        `
-        const { publicKey: publicKeyDER, privateKey: privateKeyDER } = generateKeyPairSync(
-          "rsa",
-          {
-            publicExponent: 0x10001,
-            modulusLength: 512,
-            publicKeyEncoding: {
-              type: "pkcs1",
-              format: "der",
-            },
-            privateKeyEncoding: {
-              type: "pkcs8",
-              format: "der",
-            },
-          }
+  type TestRunInContextArg =
+    | { fn: typeof runInContext; isIsolated: true }
+    | { fn: typeof runInThisContext; isIsolated?: false };
+
+  function testRunInContext({ fn, isIsolated }: TestRunInContextArg) {
+    if (isIsolated) {
+      test("can generate key", () => {
+        const context = createContext({ generateKeySync });
+        const result = fn(`generateKeySync("aes", { length: 128 })`, context);
+        expect(result).toBeDefined();
+        const keybuf = result.export();
+        expect(keybuf.byteLength).toBe(128 / 8);
+      });
+      test("can be used on another context", () => {
+        const context = createContext({
+          generateKeyPairSync,
+          assertApproximateSize,
+          testEncryptDecrypt,
+          testSignVerify,
+        });
+        const result = fn(
+          `
+          const { publicKey: publicKeyDER, privateKey: privateKeyDER } = generateKeyPairSync(
+            "rsa",
+            {
+              publicExponent: 0x10001,
+              modulusLength: 512,
+              publicKeyEncoding: {
+                type: "pkcs1",
+                format: "der",
+              },
+              privateKeyEncoding: {
+                type: "pkcs8",
+                format: "der",
+              },
+            }
+          );
+
+          assertApproximateSize(publicKeyDER, 74);
+
+          const publicKey = {
+            key: publicKeyDER,
+            type: "pkcs1",
+            format: "der",
+          };
+          const privateKey = {
+            key: privateKeyDER,
+            format: "der",
+            type: "pkcs8",
+            passphrase: "secret",
+          };
+          testEncryptDecrypt(publicKey, privateKey);
+          testSignVerify(publicKey, privateKey);
+        `,
+          context,
        );
-
-        assertApproximateSize(publicKeyDER, 74);
-
-        const publicKey = {
-          key: publicKeyDER,
-          type: "pkcs1",
-          format: "der",
-        };
-        const privateKey = {
-          key: privateKeyDER,
-          format: "der",
-          type: "pkcs8",
-          passphrase: "secret",
-        };
-        testEncryptDecrypt(publicKey, privateKey);
-        testSignVerify(publicKey, privateKey);
-      `,
-        context,
-      );
-    });
+      });
+    } else {
+      test("can generate key", () => {
+        const prop = randomProp();
+        // @ts-expect-error
+        globalThis[prop] = generateKeySync;
+        try {
+          const result = fn(`${prop}("aes", { length: 128 })`);
+          expect(result).toBeDefined();
+          const keybuf = result.export();
+          expect(keybuf.byteLength).toBe(128 / 8);
+        } finally {
+          // @ts-expect-error
+          delete globalThis[prop];
+        }
+      });
+    }
  }
  describe("Script", () => {
    describe("runInContext()", () => {
-      testRunInContext((code, context, options) => {
-        // @ts-expect-error
-        const script = new Script(code, options);
-        return script.runInContext(context);
+      testRunInContext({
+        fn: (code, context, options) => {
+          const script = new Script(code, options);
+          return script.runInContext(context);
+        },
+        isIsolated: true,
      });
    });
    describe("runInNewContext()", () => {
-      testRunInContext((code, context, options) => {
-        // @ts-expect-error
-        const script = new Script(code, options);
-        return script.runInNewContext(context);
+      testRunInContext({
+        fn: (code, context, options) => {
+          const script = new Script(code, options);
+          return script.runInNewContext(context);
+        },
+        isIsolated: true,
      });
    });
    describe("runInThisContext()", () => {
-      testRunInContext((code, context, options) => {
-        // @ts-expect-error
-        const script = new Script(code, options);
-        return script.runInThisContext(context);
+      testRunInContext({
+        fn: (code: string, options: any) => {
+          const script = new Script(code, options);
+          return script.runInThisContext();
+        },
      });
    });
  });
@@ -1697,3 +1728,7 @@ test("ECDSA should work", async () => {
    verify("sha256", Buffer.from("foo"), { key: publicKey, dsaEncoding: "der" }, signature);
  }).toThrow(/invalid dsaEncoding/);
 });
+
+function randomProp() {
+  return "prop" + crypto.randomUUID().replace(/-/g, "");
+}
--- a/test/js/node/vm/vm.test.ts
+++ b/test/js/node/vm/vm.test.ts
@@ -3,40 +3,58 @@ import { createContext, runInContext, runInNewContext, runInThisContext, Script

 function capture(_: any, _1?: any) {}
 describe("runInContext()", () => {
-  testRunInContext(runInContext, { isIsolated: true });
+  testRunInContext({ fn: runInContext, isIsolated: true });
+  test("options can be a string", () => {
+    const context = createContext();
+    const result = runInContext("new Error().stack;", context, "test-filename.js" );
+    expect(result).toContain("test-filename.js");
+  });
 });

 describe("runInNewContext()", () => {
-  testRunInContext(runInNewContext, { isIsolated: true, isNew: true });
+  testRunInContext({ fn: runInNewContext, isIsolated: true, isNew: true });
+  test("options can be a string", () => {
+    test("options can be a string", () => {
+      const result = runInNewContext("new Error().stack;", {}, "test-filename.js" );
+      expect(result).toContain("test-filename.js");
+    });
+  });
 });

 describe("runInThisContext()", () => {
-  testRunInContext(runInThisContext);
+  testRunInContext({ fn: runInThisContext });
+  test("options can be a string", () => {
+    const result = runInThisContext("new Error().stack;", "test-filename.js" );
+    expect(result).toContain("test-filename.js");
+  });
 });

 describe("Script", () => {
  describe("runInContext()", () => {
-    testRunInContext(
-      (code, context, options) => {
+    testRunInContext({
+      fn: (code, context, options) => {
        const script = new Script(code, options);
        return script.runInContext(context);
      },
-      { isIsolated: true },
-    );
+      isIsolated: true,
+    });
  });
  describe("runInNewContext()", () => {
-    testRunInContext(
-      (code, context, options) => {
+    testRunInContext({
+      fn: (code, context, options) => {
        const script = new Script(code, options);
        return script.runInNewContext(context);
      },
-      { isIsolated: true, isNew: true },
-    );
+      isIsolated: true,
+      isNew: true,
+    });
  });
  describe("runInThisContext()", () => {
-    testRunInContext((code, context, options) => {
-      const script = new Script(code, options);
-      return script.runInThisContext(context);
+    testRunInContext({
+      fn: (code: string, options: any) => {
+        const script = new Script(code, options);
+        return script.runInThisContext();
+      },
    });
  });
  test("can throw without new", () => {
@@ -49,16 +67,11 @@ describe("Script", () => {
  });
 });

-function testRunInContext(
-  fn: typeof runInContext,
-  {
-    isIsolated,
-    isNew,
-  }: {
-    isIsolated?: boolean;
-    isNew?: boolean;
-  } = {},
-) {
+type TestRunInContextArg =
+  | { fn: typeof runInContext; isIsolated: true; isNew?: boolean }
+  | { fn: typeof runInThisContext; isIsolated?: false; isNew?: boolean };
+
+function testRunInContext({ fn, isIsolated, isNew }: TestRunInContextArg) {
  test("can do nothing", () => {
    const context = createContext({});
    const result = fn("", context);
@@ -134,24 +147,6 @@ function testRunInContext(
      message: "Oops!",
    });
  });
-  test("can access the context", () => {
-    const context = createContext({
-      foo: "bar",
-      fizz: (n: number) => "buzz".repeat(n),
-    });
-    const result = fn("foo + fizz(2);", context);
-    expect(result).toBe("barbuzzbuzz");
-  });
-  test("can modify the context", () => {
-    const context = createContext({
-      foo: "bar",
-      baz: ["a", "b", "c"],
-    });
-    const result = fn("foo = 'baz'; delete baz[0];", context);
-    expect(context.foo).toBe("baz");
-    expect(context.baz).toEqual([undefined, "b", "c"]);
-    expect(result).toBe(true);
-  });
  test("can access `globalThis`", () => {
    const context = createContext({});
    const result = fn("typeof globalThis;", context);
@@ -165,12 +160,29 @@ function testRunInContext(
    expect(result).toBe("undefined");
  });
  if (isIsolated) {
+    test("can access context", () => {
+      const context = createContext({
+        foo: "bar",
+        fizz: (n: number) => "buzz".repeat(n),
+      });
+      const result = fn("foo + fizz(2);", context);
+      expect(result).toBe("barbuzzbuzz");
+    });
+    test("can modify context", () => {
+      const context = createContext({
+        baz: ["a", "b", "c"],
+      });
+      const result = fn("foo = 'baz'; delete baz[0];", context);
+      expect(context.foo).toBe("baz");
+      expect(context.baz).toEqual([undefined, "b", "c"]);
+      expect(result).toBe(true);
+    });
    test("cannot access `process`", () => {
      const context = createContext({});
      const result = fn("typeof process;", context);
      expect(result).toBe("undefined");
    });
-    test("cannot access this context", () => {
+    test("cannot access global scope", () => {
      const prop = randomProp();
      // @ts-expect-error
      globalThis[prop] = "fizz";
@@ -183,10 +195,54 @@ function testRunInContext(
        delete globalThis[prop];
      }
    });
-  } else {
-    test("can access `process`", () => {
+    test("can specify a filename", () => {
      const context = createContext({});
-      const result = fn("typeof process;", context);
+      const result = fn("new Error().stack;", context, {
+        filename: "foo.js",
+      });
+      expect(result).toContain("foo.js");
+    });
+  } else {
+    test("can access global context", () => {
+      const props = randomProps(2);
+      // @ts-expect-error
+      globalThis[props[0]] = "bar";
+      // @ts-expect-error
+      globalThis[props[1]] = (n: number) => "buzz".repeat(n);
+      try {
+        const result = fn(`${props[0]} + ${props[1]}(2);`);
+        expect(result).toBe("barbuzzbuzz");
+      } finally {
+        for (const prop of props) {
+          // @ts-expect-error
+          delete globalThis[prop];
+        }
+      }
+    });
+    test("can modify global context", () => {
+      const props = randomProps(3);
+      // @ts-expect-error
+      globalThis[props[0]] = ["a", "b", "c"];
+      // @ts-expect-error
+      globalThis[props[1]] = "initial value";
+      try {
+        const result = fn(`${props[1]} = 'baz'; ${props[2]} = 'bunny'; delete ${props[0]}[0];`);
+        // @ts-expect-error
+        expect(globalThis[props[1]]).toBe("baz");
+        // @ts-expect-error
+        expect(globalThis[props[2]]).toBe("bunny");
+        // @ts-expect-error
+        expect(globalThis[props[0]]).toEqual([undefined, "b", "c"]);
+        expect(result).toBe(true);
+      } finally {
+        for (const prop of props) {
+          // @ts-expect-error
+          delete globalThis[prop];
+        }
+      }
+    });
+    test("can access `process`", () => {
+      const result = fn("typeof process;");
      expect(result).toBe("object");
    });
    test("can access this context", () => {
@@ -194,8 +250,7 @@ function testRunInContext(
      // @ts-expect-error
      globalThis[prop] = "fizz";
      try {
-        const context = createContext({});
-        const result = fn(`${prop};`, context);
+        const result = fn(`${prop};`);
        expect(result).toBe("fizz");
      } finally {
        // @ts-expect-error
@@ -203,22 +258,20 @@ function testRunInContext(
      }
    });
    test.skip("can specify an error on SIGINT", () => {
-      const context = createContext({});
      const result = () =>
-        fn("process.kill(process.pid, 'SIGINT');", context, {
+        fn("process.kill(process.pid, 'SIGINT');", {
          breakOnSigint: true,
        });
      // TODO: process.kill() is not implemented
      expect(result).toThrow();
    });
-  }
-  test("can specify a filename", () => {
-    const context = createContext({});
-    const result = fn("new Error().stack;", context, {
-      filename: "foo.js",
+    test("can specify a filename", () => {
+      const result = fn("new Error().stack;", {
+        filename: "foo.js",
+      });
+      expect(result).toContain("foo.js");
    });
-    expect(result).toContain("foo.js");
-  });
+  }
  test.skip("can specify a line offset", () => {
    // TODO: use test.todo
  });
@@ -238,3 +291,10 @@ function testRunInContext(
 function randomProp() {
  return "prop" + crypto.randomUUID().replace(/-/g, "");
 }
+function randomProps(propsNumber = 0) {
+  const props = [];
+  for (let i = 0; i < propsNumber; i++) {
+    props.push(randomProp());
+  }
+  return props;
+}