diff --git a/src/js/builtins/ProcessObjectInternals.ts b/src/js/builtins/ProcessObjectInternals.ts index 37681a67f7c6..32c87326707e 100644 --- a/src/js/builtins/ProcessObjectInternals.ts +++ b/src/js/builtins/ProcessObjectInternals.ts @@ -492,7 +492,9 @@ export function windowsEnv( set(_, p, value) { const k = String(p).toUpperCase(); $assert(typeof p === "string"); // proxy is only string and symbol. the symbol would have thrown by now - value = String(value); // If toString() throws, we want to avoid it existing in the envMapList + // Spec ToString, not String(): Symbol values must throw TypeError (Node.js + // behavior), and a throwing toString() must fire before envMapList is touched. + value = `${value}`; // Track the key for enumeration if it isn't already there. Don't gate on // `k in internalEnv`: the proxy-related env-var accessors (HTTP_PROXY, // HTTPS_PROXY, NO_PROXY and lowercase variants) always exist on @@ -530,11 +532,20 @@ export function windowsEnv( defineProperty(_, p, attributes) { 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)) { + // Node coerces env values to strings on defineProperty too, same as assignment. + let coerced: string | undefined; + if ("value" in attributes) { + coerced = `${attributes.value}`; + attributes = { ...attributes, value: coerced }; + } + $Object.$defineProperty(internalEnv, k, attributes); + // Sync the new value and record the key for enumeration only after a + // successful define, so a failed define leaves no phantom state. + if (!envMapList.includes(p) && !envMapList.some(x => x.toUpperCase() === k)) { envMapList.push(p); } - editWindowsEnvVar(k, internalEnv[k]); - return $Object.$defineProperty(internalEnv, k, attributes); + if (coerced !== undefined) editWindowsEnvVar(k, coerced); + return true; }, getOwnPropertyDescriptor(target, p) { if (typeof p === "string") { diff --git a/src/jsc/bindings/JSEnvironmentVariableMap.cpp b/src/jsc/bindings/JSEnvironmentVariableMap.cpp index fc29c4c824fa..5d541c84f883 100644 --- a/src/jsc/bindings/JSEnvironmentVariableMap.cpp +++ b/src/jsc/bindings/JSEnvironmentVariableMap.cpp @@ -744,6 +744,109 @@ RefPtr ensureSharedEnvStoreForWorker(Zig::GlobalObject* globalOb return store; } +// Default (non-SHARE_ENV) process.env: plain object for reads, with put/putByIndex/ +// defineOwnProperty overridden to toString() the value first so `= 42` reads back +// "42" and `= undefined` reads back "undefined", matching Node.js. +class JSProcessEnvMap final : public JSC::JSNonFinalObject { +public: + using Base = JSC::JSNonFinalObject; + + static constexpr unsigned StructureFlags = Base::StructureFlags | JSC::OverridesPut; + + 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 put(JSCell*, JSGlobalObject*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&); + static bool putByIndex(JSCell*, JSGlobalObject*, unsigned, JSC::JSValue, bool shouldThrow); + static bool defineOwnProperty(JSObject*, JSGlobalObject*, JSC::PropertyName, const JSC::PropertyDescriptor&, bool 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) }; + +bool JSProcessEnvMap::put(JSCell* cell, JSGlobalObject* globalObject, PropertyName propertyName, JSValue value, PutPropertySlot& slot) +{ + VM& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (propertyName.isSymbol()) { + RELEASE_AND_RETURN(scope, Base::put(cell, globalObject, propertyName, value, slot)); + } + + JSString* string = value.toString(globalObject); + RETURN_IF_EXCEPTION(scope, false); + + RELEASE_AND_RETURN(scope, Base::put(cell, globalObject, propertyName, string, slot)); +} + +bool JSProcessEnvMap::putByIndex(JSCell* cell, JSGlobalObject* globalObject, unsigned index, JSValue value, bool shouldThrow) +{ + VM& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + JSString* string = value.toString(globalObject); + RETURN_IF_EXCEPTION(scope, false); + + RELEASE_AND_RETURN(scope, Base::putByIndex(cell, globalObject, index, string, shouldThrow)); +} + +bool JSProcessEnvMap::defineOwnProperty(JSObject* object, JSGlobalObject* globalObject, PropertyName propertyName, const PropertyDescriptor& descriptor, bool shouldThrow) +{ + VM& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + + if (propertyName.isSymbol() || !descriptor.isDataDescriptor() || !descriptor.value()) { + RELEASE_AND_RETURN(scope, Base::defineOwnProperty(object, globalObject, propertyName, descriptor, shouldThrow)); + } + + JSString* string = descriptor.value().toString(globalObject); + RETURN_IF_EXCEPTION(scope, false); + + PropertyDescriptor coerced(descriptor); + coerced.setValue(string); + RELEASE_AND_RETURN(scope, Base::defineOwnProperty(object, globalObject, propertyName, coerced, shouldThrow)); +} + +JSObject* createProcessEnvMapObject(Zig::GlobalObject* globalObject) +{ + VM& vm = globalObject->vm(); + auto* structure = JSProcessEnvMap::createStructure(vm, globalObject, globalObject->objectPrototype()); + return JSProcessEnvMap::create(vm, structure); +} + +bool isEnvironmentVariablesMapObject(const JSC::ClassInfo* info) +{ + return info == JSProcessEnvMap::info() || info == JSSharedEnvMap::info(); +} + JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) { VM& vm = globalObject->vm(); @@ -751,16 +854,19 @@ JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject) void* list; size_t count = Bun__getEnvCount(globalObject, &list); +#if OS(WINDOWS) + // The windowsEnv Proxy's set/defineProperty traps coerce, and windowsEnv() + // stores a toJSON function on this object directly: keep it a plain object. JSC::JSObject* object = nullptr; if (count < 63) { object = constructEmptyObject(globalObject, globalObject->objectPrototype(), count); } else { object = constructEmptyObject(globalObject, globalObject->objectPrototype()); } - -#if OS(WINDOWS) JSArray* keyArray = constructEmptyArray(globalObject, nullptr, count); RETURN_IF_EXCEPTION(scope, {}); +#else + JSC::JSObject* object = createProcessEnvMapObject(globalObject); #endif static NeverDestroyed TZ = MAKE_STATIC_STRING_IMPL("TZ"); diff --git a/src/jsc/bindings/JSEnvironmentVariableMap.h b/src/jsc/bindings/JSEnvironmentVariableMap.h index 0c77d82ac27e..7288030c072d 100644 --- a/src/jsc/bindings/JSEnvironmentVariableMap.h +++ b/src/jsc/bindings/JSEnvironmentVariableMap.h @@ -13,6 +13,14 @@ namespace Bun { JSC::JSValue createEnvironmentVariablesMap(Zig::GlobalObject* globalObject); +// Bare JSProcessEnvMap (no OS-env getters, no Windows Proxy wrap): coerces +// assigned values to strings like Node.js. For worker env: {...} snapshots. +JSC::JSObject* createProcessEnvMapObject(Zig::GlobalObject* globalObject); + +// True for JSProcessEnvMap/JSSharedEnvMap classInfo; both structured-clone like +// a plain object (Node.js structuredClone(process.env) succeeds). +bool isEnvironmentVariablesMapObject(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 b2b5535f071b..96311d156a5e 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -566,12 +566,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); + // JSProcessEnvMap is a JSNonFinalObject, so indexed putDirectMayBeIndex + // routes through the method-table defineOwnProperty with a throw scope; + // a top scope keeps exception-check validation balanced. + auto scope = DECLARE_TOP_EXCEPTION_SCOPE(vm); + auto env = Bun::createProcessEnvMapObject(globalObject); 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 4c3d2d90deb0..ceb4ea7bcef0 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,9 @@ 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 allowed because + // Node.js structured-clones it as a plain object of its string entries. + if (inObject->classInfo() != JSFinalObject::info() && inObject->classInfo() != Zig::NapiPrototype::info() && inObject->classInfo() != JSC::ObjectPrototype::info() && !Bun::isEnvironmentVariablesMapObject(inObject->classInfo())) return SerializationReturnCode::DataCloneError; inputObjectStack.append(inObject); indexStack.append(0); diff --git a/test/cli/run/env.test.ts b/test/cli/run/env.test.ts index 78b29adcc02b..f889d1d5aae0 100644 --- a/test/cli/run/env.test.ts +++ b/test/cli/run/env.test.ts @@ -901,21 +901,25 @@ for (const shell of ["system", "bun"]) { }); } -const todoOnPosix = process.platform !== "win32" ? test.todo : test; -todoOnPosix("setting process.env coerces the value to a string", () => { - // @ts-expect-error - process.env.SET_TO_TRUE = true; - let did_call = 0; - // @ts-expect-error - process.env.SET_TO_BUN = { - toString() { - did_call++; - return "bun!"; - }, - }; - expect(process.env.SET_TO_TRUE).toBe("true"); - expect(process.env.SET_TO_BUN).toBe("bun!"); - expect(did_call).toBe(1); +test("setting process.env coerces the value to a string", () => { + try { + // @ts-expect-error + process.env.SET_TO_TRUE = true; + let did_call = 0; + // @ts-expect-error + process.env.SET_TO_BUN = { + toString() { + did_call++; + return "bun!"; + }, + }; + expect(process.env.SET_TO_TRUE).toBe("true"); + expect(process.env.SET_TO_BUN).toBe("bun!"); + expect(did_call).toBe(1); + } finally { + delete process.env.SET_TO_TRUE; + delete process.env.SET_TO_BUN; + } }); test("NODE_ENV=test loads .env.test even when .env.production exists", () => { diff --git a/test/js/node/process/process.test.js b/test/js/node/process/process.test.js index b191a4ed15ab..34971bf3eeae 100644 --- a/test/js/node/process/process.test.js +++ b/test/js/node/process/process.test.js @@ -231,6 +231,144 @@ it("process.env is spreadable and editable", () => { expect(eval(`globalThis.process.env.USER = "${orig}"`)).toBe(String(orig)); }); +it("process.env coerces assigned values to strings", () => { + try { + process.env.COERCE_UNDEF = "initial"; + process.env.COERCE_UNDEF = undefined; + process.env.COERCE_NUM = 42; + process.env.COERCE_NULL = null; + process.env.COERCE_BOOL = true; + process.env.COERCE_BIG = 42n; + process.env.COERCE_ARR = [1, 2]; + process.env.COERCE_FN = function f() {}; + process.env.COERCE_OBJ = { toString: () => "from-toString" }; + process.env["4242424242"] = 55; + Object.assign(process.env, { COERCE_ASN: 7 }); + Object.defineProperty(process.env, "COERCE_DEF", { + value: 7, + writable: true, + enumerable: true, + configurable: true, + }); + + expect({ + undef: process.env.COERCE_UNDEF, + num: process.env.COERCE_NUM, + null: process.env.COERCE_NULL, + bool: process.env.COERCE_BOOL, + big: process.env.COERCE_BIG, + arr: process.env.COERCE_ARR, + fn: process.env.COERCE_FN, + obj: process.env.COERCE_OBJ, + idx: process.env["4242424242"], + asn: process.env.COERCE_ASN, + def: process.env.COERCE_DEF, + }).toEqual({ + undef: "undefined", + num: "42", + null: "null", + bool: "true", + big: "42", + arr: "1,2", + fn: String(function f() {}), + obj: "from-toString", + idx: "55", + asn: "7", + def: "7", + }); + + expect(Object.values(process.env).every(v => typeof v === "string")).toBe(true); + expect(JSON.parse(JSON.stringify(process.env)).COERCE_BIG).toBe("42"); + + expect(() => { + process.env.COERCE_THROWS = { + toString() { + throw new Error("boom"); + }, + }; + }).toThrow("boom"); + expect(process.env.COERCE_THROWS).toBeUndefined(); + + // Node throws on Symbol values (ToString on Symbol throws) for set, assign, and defineProperty. + expect(() => (process.env.COERCE_SYM = Symbol("s"))).toThrow(TypeError); + expect(() => Object.assign(process.env, { COERCE_SYM: Symbol("s") })).toThrow(TypeError); + expect(() => + Object.defineProperty(process.env, "COERCE_SYM", { + value: Symbol("s"), + writable: true, + enumerable: true, + configurable: true, + }), + ).toThrow(TypeError); + expect(process.env.COERCE_SYM).toBeUndefined(); + + // structuredClone(process.env) must keep working now that the backing object + // has its own ClassInfo. Windows wraps it in a Proxy that was never clonable. + if (!isWindows) { + expect(structuredClone(process.env).COERCE_NUM).toBe("42"); + } + } finally { + for (const k of [ + "COERCE_UNDEF", + "COERCE_NUM", + "COERCE_NULL", + "COERCE_BOOL", + "COERCE_BIG", + "COERCE_ARR", + "COERCE_FN", + "COERCE_OBJ", + "COERCE_ASN", + "COERCE_DEF", + "COERCE_THROWS", + "COERCE_SYM", + "4242424242", + ]) { + delete process.env[k]; + } + } +}); + +it("process.env coerces assigned values to strings in a worker with an explicit env", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const { Worker } = require("worker_threads"); + const w = new Worker( + \`process.env.X = 42; + process.env.Y = "y"; + process.env.Y = undefined; + Object.defineProperty(process.env, "Z", { value: 7, writable: true, enumerable: true, configurable: true }); + require("worker_threads").parentPort.postMessage({ + SEED: process.env.SEED, + X: process.env.X, Y: process.env.Y, Z: process.env.Z, + xt: typeof process.env.X, yt: typeof process.env.Y, zt: typeof process.env.Z, + clone: structuredClone(process.env), + });\`, + { eval: true, env: { SEED: "seed" } }, + ); + w.on("message", m => { console.log(JSON.stringify(m)); }); + w.on("error", e => { console.error(String(e)); process.exitCode = 1; });`, + ], + env: bunEnv, + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ result: JSON.parse(stdout.trim() || stderr), exitCode }).toEqual({ + result: { + SEED: "seed", + X: "42", + Y: "undefined", + Z: "7", + xt: "string", + yt: "string", + zt: "string", + clone: { SEED: "seed", X: "42", Y: "undefined", Z: "7" }, + }, + exitCode: 0, + }); +}); + const MIN_ICU_VERSIONS_BY_PLATFORM_ARCH = { "darwin-x64": "70.1", "darwin-arm64": "72.1",