diff --git a/src/jsc/bindings/NodeVM.cpp b/src/jsc/bindings/NodeVM.cpp index 54a3d25d2947..951feaf4f24a 100644 --- a/src/jsc/bindings/NodeVM.cpp +++ b/src/jsc/bindings/NodeVM.cpp @@ -640,7 +640,7 @@ void decorateParseErrorStack(JSGlobalObject* globalObject, VM& vm, JSObject* err writeArrowHeaderStack(vm, errorInstance, url, reportedLine, sourceLineText, caretColumn, stack); } -void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::ThrowScope& scope, JSValue optionsArg, NodeVMContextOptions& outOptions, ASCIILiteral codeGenerationKey, JSValue* importer) +void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::ThrowScope& scope, JSValue optionsArg, NodeVMContextOptions& outOptions, const NodeVMContextOptionKeys& keys, JSValue* importer) { if (importer) { *importer = jsUndefined(); @@ -656,21 +656,21 @@ void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::Thr JSObject* options = asObject(optionsArg); // Check name property - auto nameValue = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "name"_s)); + auto nameValue = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, keys.name)); RETURN_IF_EXCEPTION(scope, ); if (nameValue) { if (!nameValue.isUndefined() && !nameValue.isString()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, "options.name"_s, "string"_s, nameValue); + ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, keys.name), "string"_s, nameValue); return; } } // Check origin property - auto originValue = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "origin"_s)); + auto originValue = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, keys.origin)); RETURN_IF_EXCEPTION(scope, ); if (originValue) { if (!originValue.isUndefined() && !originValue.isString()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, "options.origin"_s, "string"_s, originValue); + ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, keys.origin), "string"_s, originValue); return; } } @@ -704,7 +704,7 @@ void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::Thr outOptions.ownMicrotaskQueue = true; } - JSValue codeGenerationValue = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, codeGenerationKey)); + JSValue codeGenerationValue = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, keys.codeGeneration)); RETURN_IF_EXCEPTION(scope, ); if (codeGenerationValue) { @@ -713,7 +713,7 @@ void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::Thr } if (!codeGenerationValue.isObject()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, codeGenerationKey), "object"_s, codeGenerationValue); + ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, keys.codeGeneration), "object"_s, codeGenerationValue); return; } @@ -723,7 +723,7 @@ void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::Thr RETURN_IF_EXCEPTION(scope, ); if (allowStringsValue) { if (!allowStringsValue.isBoolean()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, codeGenerationKey, ".strings"_s), "boolean"_s, allowStringsValue); + ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, keys.codeGeneration, ".strings"_s), "boolean"_s, allowStringsValue); return; } @@ -735,7 +735,7 @@ void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::Thr RETURN_IF_EXCEPTION(scope, ); if (allowWasmValue) { if (!allowWasmValue.isBoolean()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, codeGenerationKey, ".wasm"_s), "boolean"_s, allowWasmValue); + ERR::INVALID_ARG_TYPE(scope, globalObject, WTF::makeString("options."_s, keys.codeGeneration, ".wasm"_s), "boolean"_s, allowWasmValue); return; } @@ -1433,7 +1433,7 @@ JSC_DEFINE_HOST_FUNCTION(vmModuleRunInNewContext, (JSGlobalObject * globalObject JSValue globalObjectDynamicImportCallback; - getNodeVMContextOptions(globalObject, vm, scope, contextOptionsArg, contextOptions, "contextCodeGeneration", &globalObjectDynamicImportCallback); + getNodeVMContextOptions(globalObject, vm, scope, contextOptionsArg, contextOptions, runInNewContextOptionKeys, &globalObjectDynamicImportCallback); RETURN_IF_EXCEPTION(scope, {}); contextOptions.notContextified = notContextified; @@ -1667,7 +1667,7 @@ JSC_DEFINE_HOST_FUNCTION(vmModule_createContext, (JSGlobalObject * globalObject, JSValue importer; - getNodeVMContextOptions(globalObject, vm, scope, optionsArg, contextOptions, "codeGeneration", &importer); + getNodeVMContextOptions(globalObject, vm, scope, optionsArg, contextOptions, createContextOptionKeys, &importer); RETURN_IF_EXCEPTION(scope, {}); contextOptions.notContextified = notContextified; diff --git a/src/jsc/bindings/NodeVM.h b/src/jsc/bindings/NodeVM.h index 7796b7941aa2..caa28c88438b 100644 --- a/src/jsc/bindings/NodeVM.h +++ b/src/jsc/bindings/NodeVM.h @@ -20,6 +20,7 @@ namespace Bun { class NodeVMGlobalObject; class NodeVMContextOptions; +struct NodeVMContextOptionKeys; class CompileFunctionOptions; namespace NodeVM { @@ -33,7 +34,7 @@ bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtr // when no filename was provided; compileFunction has no such default. void decorateParseErrorStack(JSGlobalObject* globalObject, VM& vm, JSObject* error, StringView sourceString, const String& url, const JSC::ParserError& parseError, OrdinalNumber lineOffset); -void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::ThrowScope& scope, JSValue optionsArg, NodeVMContextOptions& outOptions, ASCIILiteral codeGenerationKey, JSValue* importer); +void getNodeVMContextOptions(JSGlobalObject* globalObject, JSC::VM& vm, JSC::ThrowScope& scope, JSValue optionsArg, NodeVMContextOptions& outOptions, const NodeVMContextOptionKeys& keys, JSValue* importer); NodeVMGlobalObject* getGlobalObjectFromContext(JSGlobalObject* globalObject, JSValue contextValue, bool canThrow); JSC::EncodedJSValue INVALID_ARG_VALUE_VM_VARIATION(JSC::ThrowScope& throwScope, JSC::JSGlobalObject* globalObject, WTF::ASCIILiteral name, JSC::JSValue value); // For vm.compileFunction we need to return an anonymous function expression. This code is adapted from/inspired by JSC::constructFunction, which is used for function declarations. @@ -86,6 +87,19 @@ class NodeVMContextOptions final { bool ownMicrotaskQueue = false; }; +// Property names getNodeVMContextOptions() reads from the options object. +// Node's createContext() takes { name, origin, codeGeneration }; runInNewContext() +// takes the same options as { contextName, contextOrigin, contextCodeGeneration } +// (lib/vm.js getContextOptions()). The other keys it reads have one spelling. +struct NodeVMContextOptionKeys { + ASCIILiteral name; + ASCIILiteral origin; + ASCIILiteral codeGeneration; +}; + +inline constexpr NodeVMContextOptionKeys createContextOptionKeys { "name"_s, "origin"_s, "codeGeneration"_s }; +inline constexpr NodeVMContextOptionKeys runInNewContextOptionKeys { "contextName"_s, "contextOrigin"_s, "contextCodeGeneration"_s }; + class NodeVMGlobalObject; class NodeVMSpecialSandbox final : public JSC::JSNonFinalObject { diff --git a/src/jsc/bindings/NodeVMScript.cpp b/src/jsc/bindings/NodeVMScript.cpp index 1a8653a1184a..05f4c01dacbd 100644 --- a/src/jsc/bindings/NodeVMScript.cpp +++ b/src/jsc/bindings/NodeVMScript.cpp @@ -31,27 +31,6 @@ bool ScriptOptions::fromJS(JSC::JSGlobalObject* globalObject, JSC::VM& vm, JSC:: if (!optionsArg.isUndefined() && !optionsArg.isString()) { JSObject* options = asObject(optionsArg); - // Validate contextName and contextOrigin are strings - auto contextNameOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "contextName"_s)); - RETURN_IF_EXCEPTION(scope, false); - if (contextNameOpt) { - if (!contextNameOpt.isUndefined() && !contextNameOpt.isString()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, "options.contextName"_s, "string"_s, contextNameOpt); - return false; - } - any = true; - } - - auto contextOriginOpt = options->getIfPropertyExists(globalObject, Identifier::fromString(vm, "contextOrigin"_s)); - RETURN_IF_EXCEPTION(scope, false); - if (contextOriginOpt) { - if (!contextOriginOpt.isUndefined() && !contextOriginOpt.isString()) { - ERR::INVALID_ARG_TYPE(scope, globalObject, "options.contextOrigin"_s, "string"_s, contextOriginOpt); - return false; - } - any = true; - } - if (validateTimeout(globalObject, vm, scope, options, this->timeout)) { RETURN_IF_EXCEPTION(scope, false); any = true; @@ -642,7 +621,7 @@ JSC_DEFINE_HOST_FUNCTION(scriptRunInNewContext, (JSGlobalObject * globalObject, NodeVMContextOptions contextOptions {}; JSValue importer; - getNodeVMContextOptions(globalObject, vm, scope, contextOptionsArg, contextOptions, "contextCodeGeneration", &importer); + getNodeVMContextOptions(globalObject, vm, scope, contextOptionsArg, contextOptions, runInNewContextOptionKeys, &importer); RETURN_IF_EXCEPTION(scope, {}); contextOptions.notContextified = notContextified; diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index d3d04239d179..42c3a3d92f53 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -1012,26 +1012,166 @@ describe("codeGeneration options", () => { }); }); +describe("context name/origin options", () => { + // Node's createContext() takes { name, origin }; runInNewContext() takes the + // same two as { contextName, contextOrigin } (lib/vm.js getContextOptions()) + // and ignores name/origin. Neither spelling is an option of the Script + // constructor, runInContext() or runInThisContext(). + function thrownBy(fn: () => unknown) { + try { + fn(); + } catch (e: any) { + return { name: e.name, code: e.code, message: e.message }; + } + return "did not throw"; + } + const notAString = (option: string, received: string) => ({ + name: "TypeError", + code: "ERR_INVALID_ARG_TYPE", + message: `The "${option}" property must be of type string. Received ${received}`, + }); + const nonStrings: [value: unknown, received: string][] = [ + [1, "type number (1)"], + [null, "null"], + [new String("ctx"), "an instance of String"], + ]; + // Options whose getters throw if an entry point reads a key it should ignore. + function unreadable(...keys: string[]) { + const options = {}; + for (const key of keys) { + Object.defineProperty(options, key, { + get() { + throw new Error(`read options.${key}`); + }, + enumerable: true, + }); + } + return options; + } + + describe("Script#runInNewContext()", () => { + test.each(["contextName", "contextOrigin"])("rejects a non-string %s", option => { + for (const [value, received] of nonStrings) { + expect(thrownBy(() => new Script("1").runInNewContext({}, { [option]: value }))).toEqual( + notAString(`options.${option}`, received), + ); + } + }); + + test("validates them whatever the sandbox argument is", () => { + const options: object = { contextName: 1 }; + const bad = notAString("options.contextName", "type number (1)"); + expect(thrownBy(() => new Script("1").runInNewContext(undefined, options))).toEqual(bad); + expect(thrownBy(() => new Script("1").runInNewContext(createContext({}), options))).toEqual(bad); + expect(thrownBy(() => new Script("1").runInNewContext(constants.DONT_CONTEXTIFY, options))).toEqual(bad); + }); + + test("reports contextName before contextOrigin before contextCodeGeneration, like Node", () => { + const run = (options: object) => thrownBy(() => new Script("1").runInNewContext({}, options)); + expect(run({ contextName: 1, contextOrigin: 1, contextCodeGeneration: 1 })).toEqual( + notAString("options.contextName", "type number (1)"), + ); + expect(run({ contextOrigin: 1, contextCodeGeneration: 1 })).toEqual( + notAString("options.contextOrigin", "type number (1)"), + ); + expect(run({ contextCodeGeneration: 1 })).toEqual({ + name: "TypeError", + code: "ERR_INVALID_ARG_TYPE", + message: 'The "options.contextCodeGeneration" property must be of type object. Received type number (1)', + }); + }); + + test("accepts string or undefined values", () => { + const run = (options: object) => new Script("1 + 1").runInNewContext({}, options); + expect(run({ contextName: "sandbox", contextOrigin: "https://example.com" })).toBe(2); + expect(run({ contextName: "", contextOrigin: "" })).toBe(2); + expect(run({ contextName: undefined, contextOrigin: undefined })).toBe(2); + }); + + test("does not read createContext()'s name/origin", () => { + const run = (options: object) => new Script("1 + 1").runInNewContext({}, options); + expect(run({ name: 1, origin: 1 })).toBe(2); + expect(run(unreadable("name", "origin"))).toBe(2); + }); + }); + + describe("runInNewContext()", () => { + test.each(["contextName", "contextOrigin"])("rejects a non-string %s", option => { + for (const [value, received] of nonStrings) { + expect(thrownBy(() => runInNewContext("1", {}, { [option]: value }))).toEqual( + notAString(`options.${option}`, received), + ); + } + }); + + test("accepts string values", () => { + expect(runInNewContext("1 + 1", {}, { contextName: "sandbox", contextOrigin: "https://example.com" })).toBe(2); + }); + }); + + describe("createContext()", () => { + test.each(["name", "origin"])("rejects a non-string %s", option => { + for (const [value, received] of nonStrings) { + expect(thrownBy(() => createContext({}, { [option]: value }))).toEqual( + notAString(`options.${option}`, received), + ); + } + }); + + test("does not read runInNewContext()'s contextName/contextOrigin", () => { + const options: object = { contextName: 1, contextOrigin: 1 }; + expect(() => createContext({}, options)).not.toThrow(); + expect(() => createContext({}, unreadable("contextName", "contextOrigin"))).not.toThrow(); + }); + }); + + describe("entry points that take neither spelling", () => { + const options: object = { name: 1, origin: 1, contextName: 1, contextOrigin: 1 }; + + test("new Script()", () => { + expect(new Script("1 + 1", options).runInThisContext()).toBe(2); + const script = new Script("1 + 1", unreadable("name", "origin", "contextName", "contextOrigin")); + expect(script.runInThisContext()).toBe(2); + }); + + test("runInThisContext()", () => { + expect(runInThisContext("1 + 1", options)).toBe(2); + }); + + test("runInContext()", () => { + expect(runInContext("1 + 1", createContext({}), options)).toBe(2); + }); + + test("Script#runInContext() and Script#runInThisContext()", () => { + expect(new Script("1 + 1").runInContext(createContext({}), options)).toBe(2); + expect(new Script("1 + 1").runInThisContext(options)).toBe(2); + }); + }); +}); + describe("context options with throwing getters", () => { // Without the fix, reading these options with a pending exception aborted // the process, so run the matrix in a subprocess. test.concurrent("the getter's exception propagates to the caller", async () => { - // Each entry point tests the context-option keys it actually reads: - // createContext takes codeGeneration, Script#runInNewContext takes - // contextCodeGeneration, and vm.runInNewContext goes through both. + // Each entry point tests the context-option keys it reads. createContext() + // spells the first three name/origin/codeGeneration; runInNewContext() + // spells them contextName/contextOrigin/contextCodeGeneration. // A dotted key puts the throwing getter on the nested object. - const codeGenerationKeys = (key: string) => [key, `${key}.strings`, `${key}.wasm`]; - const contextKeys = (...codeGenerationKeyNames: string[]) => [ - "name", - "origin", - ...codeGenerationKeyNames.flatMap(codeGenerationKeys), + const contextKeys = (nameKey: string, originKey: string, codeGenerationKey: string) => [ + nameKey, + originKey, + codeGenerationKey, + `${codeGenerationKey}.strings`, + `${codeGenerationKey}.wasm`, "importModuleDynamically", "microtaskMode", ]; + const createContextKeys = contextKeys("name", "origin", "codeGeneration"); + const runInNewContextKeys = contextKeys("contextName", "contextOrigin", "contextCodeGeneration"); const matrix = { - createContext: contextKeys("codeGeneration"), - runInNewContext: contextKeys("codeGeneration", "contextCodeGeneration"), - scriptRunInNewContext: contextKeys("contextCodeGeneration"), + createContext: createContextKeys, + runInNewContext: runInNewContextKeys, + scriptRunInNewContext: runInNewContextKeys, }; const code = ` const vm = require("node:vm");