process: reject accessor/partial descriptors in Object.defineProperty(process.env, ...)

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") {

src/jsc/bindings/JSEnvironmentVariableMap.cpp

@@ -19,6 +19,7 @@
 #include <JavaScriptCore/PropertyNameArray.h>
 #include <JavaScriptCore/PropertyDescriptor.h>
 #include "BunProcess.h"
+#include "ErrorCode.h"
 #include "ScriptExecutionContext.h"
 #include "SharedEnvStore.h"
 #include "wtf/NeverDestroyed.h"
@@ -364,6 +365,89 @@
 #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<typename CellType, JSC::SubspaceAccess>
+    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<JSProcessEnvMap>(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;
+        RELEASE_AND_RETURN(scope, Base::defineOwnProperty(object, globalObject, propertyName, descriptor, 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
 //
@@ -602,23 +686,11 @@
     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 +821,8 @@
 
     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);

src/jsc/bindings/JSEnvironmentVariableMap.h

@@ -13,6 +13,11 @@ 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);
+
 // 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);

src/jsc/bindings/ZigGlobalObject.cpp

@@ -562,7 +562,7 @@ 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);
                 size_t i = 0;
                 for (auto k : map) {
                     // They can have environment variables with numbers as keys.

test/js/node/process/process.test.js

@@ -218,6 +218,39 @@ it("process.env", () => {
   expect(process.env["LOL SMILE latin1 <abc>"]).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;
+});
+
 it("process.env is spreadable and editable", () => {
   process.env["LOL SMILE UTF16 😂"] = "😂";
   const { "LOL SMILE UTF16 😂": lol, ...rest } = process.env;

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');

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');
+}