From 59eb5515c567f6f77a098d2d97d097eeba8a25af Mon Sep 17 00:00:00 2001 From: Grigory Date: Fri, 30 Aug 2024 13:34:18 +0500 Subject: [PATCH] fix(nodevm): align behavior with node (#13590) --- src/bun.js/bindings/NodeVM.cpp | 52 ++---- .../js/node/crypto/crypto.key-objects.test.ts | 153 +++++++++------ test/js/node/vm/vm.test.ts | 174 ++++++++++++------ 3 files changed, 228 insertions(+), 151 deletions(-) diff --git a/src/bun.js/bindings/NodeVM.cpp b/src/bun.js/bindings/NodeVM.cpp index 2053e36f07..b53f07ecc0 100644 --- 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(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(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)); diff --git a/test/js/node/crypto/crypto.key-objects.test.ts b/test/js/node/crypto/crypto.key-objects.test.ts index f4e40d1301..a810ddec9b 100644 --- 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, ""); +} diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 982d5eed90..3b17d6e19c 100644 --- 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; +}