Skip to content

node:vm: read contextName/contextOrigin in Script#runInNewContext() instead of name/origin #38409

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

node:vm: read contextName/contextOrigin in Script#runInNewContext() instead of name/origin #38409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions src/jsc/bindings/NodeVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 15 additions & 1 deletion src/jsc/bindings/NodeVM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ namespace Bun {

class NodeVMGlobalObject;
class NodeVMContextOptions;
struct NodeVMContextOptionKeys;
class CompileFunctionOptions;

namespace NodeVM {
Expand All @@ -33,7 +34,7 @@ bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtr<JSC::Excepti
// `url` must be caller-resolved: `new Script` falls back to evalmachine.<anonymous>
// 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.
Expand Down Expand Up @@ -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 {
Expand Down
23 changes: 1 addition & 22 deletions src/jsc/bindings/NodeVMScript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
162 changes: 151 additions & 11 deletions test/js/node/vm/vm.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down