diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 37681a67f7c6..fc71dc8f73b6 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -528,13 +528,33 @@ export function windowsEnv( return typeof p !== "symbol" ? delete internalEnv[k] : false; }, defineProperty(_, p, attributes) { + // Node's EnvDefiner rejects anything but a fully-specified writable, + // enumerable, configurable data descriptor. Validate before touching + // envMapList or the real environment block. + if (!("value" in attributes)) { + if ("get" in attributes || "set" in attributes) { + throw $ERR_INVALID_OBJECT_DEFINE_PROPERTY( + "'process.env' does not accept an accessor(getter/setter) descriptor", + ); + } + throw $ERR_INVALID_OBJECT_DEFINE_PROPERTY( + "'process.env' only accepts a configurable, writable, and enumerable data descriptor", + ); + } + if (attributes.writable !== true || attributes.enumerable !== true || attributes.configurable !== true) { + throw $ERR_INVALID_OBJECT_DEFINE_PROPERTY( + "'process.env' only accepts a configurable, writable, and enumerable data descriptor", + ); + } const k = String(p).toUpperCase(); $assert(typeof p === "string"); // proxy is only string and symbol. the symbol would have thrown by now - if (!(k in internalEnv) && !envMapList.includes(p)) { + const value = String(attributes.value); + if (!envMapList.includes(p) && !envMapList.some(x => x.toUpperCase() === k)) { envMapList.push(p); } - editWindowsEnvVar(k, internalEnv[k]); - return $Object.$defineProperty(internalEnv, k, attributes); + editWindowsEnvVar(k, value); + internalEnv[k] = value; + return true; }, getOwnPropertyDescriptor(target, p) { if (typeof p === "string") { diff --git a/src/jsc/bindings/JSEnvironmentVariableMap.cpp b/src/jsc/bindings/JSEnvironmentVariableMap.cpp index 4f4f217679c6..68d325104e26 100644 --- a/src/jsc/bindings/JSEnvironmentVariableMap.cpp +++ b/src/jsc/bindings/JSEnvironmentVariableMap.cpp @@ -19,6 +19,7 @@ #include #include #include "BunProcess.h" +#include "ErrorCode.h" #include "ScriptExecutionContext.h" #include "SharedEnvStore.h" #include "wtf/NeverDestroyed.h" @@ -364,6 +365,94 @@ static ALWAYS_INLINE void syncWindowsEnv(SharedEnvStore* store, const String& ke #endif } +// Node.js rejects any Object.defineProperty on process.env whose descriptor is +// not a fully-specified {value, writable: true, enumerable: true, configurable: +// true} data descriptor (node_env_var.cc EnvDefiner). An accessor could never be +// reflected into the real environment block. +static bool throwIfInvalidEnvDescriptor(JSGlobalObject* globalObject, JSC::ThrowScope& scope, const PropertyDescriptor& descriptor) +{ + static constexpr auto dataMsg = "'process.env' only accepts a configurable, writable, and enumerable data descriptor"_s; + if (descriptor.value()) { + if (!descriptor.writablePresent() || !descriptor.enumerablePresent() || !descriptor.configurablePresent() + || !descriptor.writable() || !descriptor.enumerable() || !descriptor.configurable()) { + throwError(globalObject, scope, ErrorCode::ERR_INVALID_OBJECT_DEFINE_PROPERTY, dataMsg); + return false; + } + return true; + } + if (descriptor.getterPresent() || descriptor.setterPresent()) { + throwError(globalObject, scope, ErrorCode::ERR_INVALID_OBJECT_DEFINE_PROPERTY, + "'process.env' does not accept an accessor(getter/setter) descriptor"_s); + return false; + } + throwError(globalObject, scope, ErrorCode::ERR_INVALID_OBJECT_DEFINE_PROPERTY, dataMsg); + return false; +} + +// The regular process.env object: a plain object with the defineOwnProperty +// override above. No instance state, so no custom subspace. +class JSProcessEnvMap final : public JSC::JSNonFinalObject { +public: + using Base = JSC::JSNonFinalObject; + + static constexpr unsigned StructureFlags = Base::StructureFlags; + + template + static JSC::GCClient::IsoSubspace* subspaceFor(JSC::VM& vm) + { + STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(JSProcessEnvMap, Base); + return &vm.plainObjectSpace(); + } + + DECLARE_INFO; + + static JSC::Structure* createStructure(JSC::VM& vm, JSC::JSGlobalObject* globalObject, JSC::JSValue prototype) + { + return JSC::Structure::create(vm, globalObject, prototype, JSC::TypeInfo(JSC::ObjectType, StructureFlags), info()); + } + + static JSProcessEnvMap* create(JSC::VM& vm, JSC::Structure* structure) + { + JSProcessEnvMap* ptr = new (NotNull, JSC::allocateCell(vm)) JSProcessEnvMap(vm, structure); + ptr->finishCreation(vm); + return ptr; + } + + static bool defineOwnProperty(JSObject* object, JSGlobalObject* globalObject, JSC::PropertyName propertyName, const JSC::PropertyDescriptor& descriptor, bool shouldThrow) + { + VM& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + if (!throwIfInvalidEnvDescriptor(globalObject, scope, descriptor)) + return false; + // Node's EnvDefiner delegates to EnvSetter, which coerces the value to a + // string; do the same so all three process.env variants match. + auto* string = descriptor.value().toString(globalObject); + RETURN_IF_EXCEPTION(scope, false); + PropertyDescriptor stringDescriptor(string, 0); + RELEASE_AND_RETURN(scope, Base::defineOwnProperty(object, globalObject, propertyName, stringDescriptor, shouldThrow)); + } + +private: + JSProcessEnvMap(JSC::VM& vm, JSC::Structure* structure) + : Base(vm, structure) + { + } + + void finishCreation(JSC::VM& vm) + { + Base::finishCreation(vm); + } +}; + +const JSC::ClassInfo JSProcessEnvMap::s_info = { "ProcessEnv"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSProcessEnvMap) }; + +JSObject* createEmptyProcessEnvMap(Zig::GlobalObject* globalObject) +{ + VM& vm = globalObject->vm(); + auto* structure = JSProcessEnvMap::createStructure(vm, globalObject, globalObject->objectPrototype()); + return JSProcessEnvMap::create(vm, structure); +} + // ============================================================================ // worker_threads SHARE_ENV // @@ -452,6 +541,11 @@ class JSSharedEnvMap final : public JSC::JSNonFinalObject { const JSC::ClassInfo JSSharedEnvMap::s_info = { "ProcessEnv"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSSharedEnvMap) }; +bool isProcessEnvClassInfo(const JSC::ClassInfo* info) +{ + return info == JSProcessEnvMap::info() || info == JSSharedEnvMap::info(); +} + bool JSSharedEnvMap::getOwnPropertySlot(JSObject* object, JSGlobalObject* globalObject, PropertyName propertyName, PropertySlot& slot) { VM& vm = JSC::getVM(globalObject); @@ -602,23 +696,11 @@ bool JSSharedEnvMap::defineOwnProperty(JSObject* object, JSGlobalObject* globalO VM& vm = JSC::getVM(globalObject); auto scope = DECLARE_THROW_SCOPE(vm); + if (!throwIfInvalidEnvDescriptor(globalObject, scope, descriptor)) + return false; + auto* uid = propertyName.uid(); - if (propertyName.isSymbol() || !uid || !descriptor.isDataDescriptor() || !descriptor.value()) { - // The descriptor lands on the Base object, but getOwnPropertySlot reads the - // store first, so a store entry would shadow it. Move the entry onto Base as - // an enumerable data property first: a partial descriptor then keeps that - // enumerability, exactly as it does on the regular process.env. (Node rejects - // accessors on process.env outright — on both maps — so match bun's own map.) - if (!propertyName.isSymbol() && uid) { - if (auto* store = sharedEnvStoreFor(object)) { - String existing = store->get(String(uid)); - if (!existing.isNull()) { - syncWindowsEnv(store, String(uid), nullptr); - store->remove(String(uid)); - object->putDirect(vm, propertyName, jsString(vm, existing), 0); - } - } - } + if (propertyName.isSymbol() || !uid) { RELEASE_AND_RETURN(scope, Base::defineOwnProperty(object, globalObject, propertyName, descriptor, shouldThrow)); } @@ -749,12 +831,8 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) void* list; size_t count = Bun__getEnvCount(globalObject, &list); - JSC::JSObject* object = nullptr; - if (count < 63) { - object = constructEmptyObject(globalObject, globalObject->objectPrototype(), count); - } else { - object = constructEmptyObject(globalObject, globalObject->objectPrototype()); - } + auto* structure = JSProcessEnvMap::createStructure(vm, globalObject, globalObject->objectPrototype()); + JSC::JSObject* object = JSProcessEnvMap::create(vm, structure); #if OS(WINDOWS) JSArray* keyArray = constructEmptyArray(globalObject, nullptr, count); diff --git a/src/jsc/bindings/JSEnvironmentVariableMap.h b/src/jsc/bindings/JSEnvironmentVariableMap.h index 0c77d82ac27e..c2166b4f23d2 100644 --- a/src/jsc/bindings/JSEnvironmentVariableMap.h +++ b/src/jsc/bindings/JSEnvironmentVariableMap.h @@ -13,6 +13,15 @@ namespace Bun { JSC::JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject); +// Empty process.env for a worker handed a snapshot of the spawning thread's env: +// same class as the ordinary map so defineOwnProperty validation applies on +// worker threads too. Caller populates it. +JSC::JSObject* createEmptyProcessEnvMap(Zig::GlobalObject* globalObject); + +// JSProcessEnvMap or JSSharedEnvMap. Both behave as plain objects for structured +// cloning and are whitelisted at SerializedScriptValue's ObjectStartState gate. +bool isProcessEnvClassInfo(const JSC::ClassInfo*); + // worker_threads SHARE_ENV: a `process.env` whose reads/writes/enumeration go // through the SharedEnvStore of the tree its global belongs to. JSC::JSValue createSharedEnvironmentVariablesMap(Zig::GlobalObject* globalObject); diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index 42dd3fa08aac..69beda62ff5a 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -562,12 +562,17 @@ extern "C" JSC::JSGlobalObject* Zig__GlobalObject__create(void* console_client, strings.append(jsString(vm, value)); } - auto env = JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), size >= JSFinalObject::maxInlineCapacity ? JSFinalObject::maxInlineCapacity : size); + auto env = Bun::createEmptyProcessEnvMap(globalObject); + // putDirectMayBeIndex on a JSNonFinalObject dispatches index keys + // through the method table's defineOwnProperty, which declares a + // ThrowScope. This runs before topEntryFrame is set, so catch it here. + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); size_t i = 0; for (auto k : map) { // They can have environment variables with numbers as keys. // So we must use putDirectMayBeIndex to handle that. env->putDirectMayBeIndex(globalObject, JSC::Identifier::fromString(vm, WTF::move(k.key)), strings.at(i++)); + scope.assertNoException(); } globalObject->m_processEnvObject.set(vm, globalObject, env); } else if (options.sharedEnvStore) { diff --git a/src/jsc/bindings/webcore/SerializedScriptValue.cpp b/src/jsc/bindings/webcore/SerializedScriptValue.cpp index 97b92658c7ad..152646542af9 100644 --- a/src/jsc/bindings/webcore/SerializedScriptValue.cpp +++ b/src/jsc/bindings/webcore/SerializedScriptValue.cpp @@ -120,6 +120,7 @@ #include "CryptoKeyType.h" #include "JSNodePerformanceHooksHistogram.h" #include "../napi.h" +#include "../JSEnvironmentVariableMap.h" #include #include @@ -2812,7 +2813,12 @@ SerializationReturnCode CloneSerializer::serialize(JSValue in) // like a plain object from JS's perspective (matches Node.js). // ObjectPrototype is allowed because %Object.prototype% is an immutable // prototype exotic object that the spec carves out of this rejection. - if (inObject->classInfo() != JSFinalObject::info() && inObject->classInfo() != Zig::NapiPrototype::info() && inObject->classInfo() != JSC::ObjectPrototype::info()) + // process.env (JSProcessEnvMap / JSSharedEnvMap) is a plain object whose + // only method-table override is defineOwnProperty; Node clones it. + if (inObject->classInfo() != JSFinalObject::info() + && inObject->classInfo() != Zig::NapiPrototype::info() + && inObject->classInfo() != JSC::ObjectPrototype::info() + && !Bun::isProcessEnvClassInfo(inObject->classInfo())) return SerializationReturnCode::DataCloneError; inputObjectStack.append(inObject); indexStack.append(0); diff --git a/test/js/node/process/process.test.js b/test/js/node/process/process.test.js index 33a44ccbc342..d58a7cd1d1c2 100644 --- a/test/js/node/process/process.test.js +++ b/test/js/node/process/process.test.js @@ -218,6 +218,59 @@ it("process.env", () => { expect(process.env["LOL SMILE latin1 "]).toBe(undefined); }); +it("Object.defineProperty on process.env rejects accessor and partial descriptors", () => { + const dataMsg = "'process.env' only accepts a configurable, writable, and enumerable data descriptor"; + const accessorMsg = "'process.env' does not accept an accessor(getter/setter) descriptor"; + const expectThrow = (fn, message) => + expect(fn).toThrow( + expect.objectContaining({ name: "TypeError", code: "ERR_INVALID_OBJECT_DEFINE_PROPERTY", message }), + ); + + expectThrow(() => Object.defineProperty(process.env, "goo", { get() {}, set() {} }), accessorMsg); + expectThrow(() => Object.defineProperty(process.env, "goo", { get() {} }), accessorMsg); + expectThrow(() => Object.defineProperty(process.env, "foo", { value: "foo1" }), dataMsg); + for (const attr of ["configurable", "writable", "enumerable"]) { + expectThrow(() => Object.defineProperty(process.env, "goo", { [attr]: false }), dataMsg); + expectThrow( + () => + Object.defineProperty(process.env, "goo", { + value: "v", + configurable: true, + writable: true, + enumerable: true, + [attr]: false, + }), + dataMsg, + ); + } + expect(process.env.foo).toBeUndefined(); + expect(process.env.goo).toBeUndefined(); + + Object.defineProperty(process.env, "goo", { value: "goo", configurable: true, writable: true, enumerable: true }); + expect(process.env.goo).toBe("goo"); + delete process.env.goo; + + // Node's EnvDefiner delegates to EnvSetter, which coerces to a string. + Object.defineProperty(process.env, "goo", { value: 42, configurable: true, writable: true, enumerable: true }); + expect(process.env.goo).toBe("42"); + expect(typeof process.env.goo).toBe("string"); + delete process.env.goo; +}); + +// process.env is no longer a JSFinalObject; it must still structured-clone as +// a plain object so postMessage / workerData keep working like Node. On Windows +// process.env is a Proxy and the serializer rejects Proxy objects (pre-existing). +it.skipIf(isWindows)("structuredClone(process.env) produces a plain-object snapshot", () => { + process.env.__SC_PROBE = "hello"; + try { + const clone = structuredClone(process.env); + expect(clone.__SC_PROBE).toBe("hello"); + expect(Object.getPrototypeOf(clone)).toBe(Object.prototype); + } finally { + delete process.env.__SC_PROBE; + } +}); + it("process.env is spreadable and editable", () => { process.env["LOL SMILE UTF16 😂"] = "😂"; const { "LOL SMILE UTF16 😂": lol, ...rest } = process.env; diff --git a/test/js/node/test/parallel/test-process-env-ignore-getter-setter.js b/test/js/node/test/parallel/test-process-env-ignore-getter-setter.js new file mode 100644 index 000000000000..e17e3752856e --- /dev/null +++ b/test/js/node/test/parallel/test-process-env-ignore-getter-setter.js @@ -0,0 +1,67 @@ +'use strict'; +require('../common'); +const assert = require('assert'); + +assert.throws( + () => { + Object.defineProperty(process.env, 'foo', { + value: 'foo1' + }); + }, + { + code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', + name: 'TypeError', + message: '\'process.env\' only accepts a ' + + 'configurable, writable,' + + ' and enumerable data descriptor' + } +); + +assert.strictEqual(process.env.foo, undefined); +process.env.foo = 'foo2'; +assert.strictEqual(process.env.foo, 'foo2'); + +assert.throws( + () => { + Object.defineProperty(process.env, 'goo', { + get() { + return 'goo'; + }, + set() {} + }); + }, + { + code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', + name: 'TypeError', + message: '\'process.env\' does not accept an ' + + 'accessor(getter/setter) descriptor' + } +); + +const attributes = ['configurable', 'writable', 'enumerable']; + +for (const attribute of attributes) { + assert.throws( + () => { + Object.defineProperty(process.env, 'goo', { + [attribute]: false + }); + }, + { + code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', + name: 'TypeError', + message: '\'process.env\' only accepts a ' + + 'configurable, writable,' + + ' and enumerable data descriptor' + } + ); +} + +assert.strictEqual(process.env.goo, undefined); +Object.defineProperty(process.env, 'goo', { + value: 'goo', + configurable: true, + writable: true, + enumerable: true +}); +assert.strictEqual(process.env.goo, 'goo'); diff --git a/test/js/node/test/parallel/test-worker-process-env.js b/test/js/node/test/parallel/test-worker-process-env.js new file mode 100644 index 000000000000..f43c2affa1f4 --- /dev/null +++ b/test/js/node/test/parallel/test-worker-process-env.js @@ -0,0 +1,67 @@ +'use strict'; +const common = require('../common'); +const child_process = require('child_process'); +const assert = require('assert'); +const { Worker, workerData } = require('worker_threads'); + +// Test for https://github.com/nodejs/node/issues/24947. + +if (!workerData && process.argv[2] !== 'child') { + process.env.SET_IN_PARENT = 'set'; + assert.strictEqual(process.env.SET_IN_PARENT, 'set'); + + new Worker(__filename, { workerData: 'runInWorker' }) + .on('exit', common.mustCall(() => { + // Env vars from the child thread are not set globally. + assert.strictEqual(process.env.SET_IN_WORKER, undefined); + })); + + process.env.SET_IN_PARENT_AFTER_CREATION = 'set'; + + new Worker(__filename, { + workerData: 'resetEnv', + env: { 'MANUALLY_SET': true } + }); + + assert.throws(() => { + new Worker(__filename, { env: 42 }); + }, { + name: 'TypeError', + code: 'ERR_INVALID_ARG_TYPE', + message: 'The "options.env" property must be of type object or ' + + 'one of undefined, null, or worker_threads.SHARE_ENV. Received type ' + + 'number (42)' + }); +} else if (workerData === 'runInWorker') { + // Env vars from the parent thread are inherited. + assert.strictEqual(process.env.SET_IN_PARENT, 'set'); + assert.strictEqual(process.env.SET_IN_PARENT_AFTER_CREATION, undefined); + process.env.SET_IN_WORKER = 'set'; + assert.strictEqual(process.env.SET_IN_WORKER, 'set'); + + assert.throws( + () => { + Object.defineProperty(process.env, 'DEFINED_IN_WORKER', { + value: 42 + }); + }, + { + code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', + name: 'TypeError', + message: '\'process.env\' only accepts a configurable, ' + + 'writable, and enumerable data descriptor' + } + ); + + + const { stderr } = + child_process.spawnSync(process.execPath, [__filename, 'child']); + assert.strictEqual(stderr.toString(), '', stderr.toString()); +} else if (workerData === 'resetEnv') { + assert.deepStrictEqual(Object.keys(process.env), ['MANUALLY_SET']); + assert.strictEqual(process.env.MANUALLY_SET, 'true'); +} else { + // Child processes inherit the parent's env, even from Workers. + assert.strictEqual(process.env.SET_IN_PARENT, 'set'); + assert.strictEqual(process.env.SET_IN_WORKER, 'set'); +} diff --git a/test/js/node/worker_threads/worker_threads.test.ts b/test/js/node/worker_threads/worker_threads.test.ts index 68d6c6f3f103..ccc61ccb4209 100644 --- a/test/js/node/worker_threads/worker_threads.test.ts +++ b/test/js/node/worker_threads/worker_threads.test.ts @@ -1459,22 +1459,25 @@ describe("env: SHARE_ENV shares the spawning thread's env, not a process-wide on }); }); - // An accessor installed via defineProperty lands on the base object, but reads hit - // the store first — so the store entry must go, or the getter is shadowed. (Node - // rejects accessors on process.env entirely; bun allows them on the regular map, - // so the shared map matches the regular one rather than diverging from it.) - it("does not let the store shadow an accessor defined on process.env", async () => { + // Node rejects anything but a fully-specified writable+enumerable+configurable + // data descriptor on process.env (ERR_INVALID_OBJECT_DEFINE_PROPERTY). The + // SHARE_ENV map must match the regular map here. + it("rejects accessor descriptors on process.env like the regular map", async () => { const proc = Bun.spawn({ cmd: [ bunExe(), "-e", `const { Worker, SHARE_ENV } = require("worker_threads"); - const probe = \`process.env.FOO = "old"; - Object.defineProperty(process.env, "FOO", { get: () => "new", configurable: true }); - const count = Object.keys(process.env).filter(k => k === "FOO").length; + const probe = \`(() => { + process.env.FOO = "old"; + let code = null; + try { + Object.defineProperty(process.env, "FOO", { get: () => "new", configurable: true }); + } catch (e) { code = e.code; } const read = process.env.FOO; delete process.env.FOO; - ({ read, count, afterDelete: process.env.FOO ?? null })\`; + return { code, read }; + })()\`; const regular = eval(probe); const w = new Worker( 'const { parentPort } = require("worker_threads"); parentPort.postMessage(eval(' + JSON.stringify(probe) + '));', @@ -1486,8 +1489,7 @@ describe("env: SHARE_ENV shares the spawning thread's env, not a process-wide on stderr: "pipe", }); const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); - // count === 1: defineProperty on an existing enumerable key keeps it enumerable. - const want = { read: "new", count: 1, afterDelete: null }; + const want = { code: "ERR_INVALID_OBJECT_DEFINE_PROPERTY", read: "old" }; expect(JSON.parse(stdout)).toEqual({ regular: want, shared: want }); expect(exitCode).toBe(0); });