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,82 @@ 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<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) };
+
 // ============================================================================
 // worker_threads SHARE_ENV
 //
@@ -602,23 +679,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 +814,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);

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/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);
   });