process: disallow some uses of Object.defineProperty() on process.env

Disallow the use of Object.defineProperty() to hide entries in
process.env or make them immutable.

Author: himself65

doc/api/errors.md

@@ -1919,6 +1919,12 @@ valid.
 The imported module string is an invalid URL, package name, or package subpath
 specifier.
 
+<a id="ERR_INVALID_OBJECT_DEFINE_PROPERTY"></a>
+### ERR_INVALID_OBJECT_DEFINE_PROPERTY
+
+An error occurred while setting an invalid attribute on the property of
+an object.
+
 <a id="ERR_INVALID_PACKAGE_CONFIG"></a>
 
 ### `ERR_INVALID_PACKAGE_CONFIG`

src/node_env_var.cc

@@ -28,6 +28,7 @@ using v8::Nothing;
 using v8::Object;
 using v8::ObjectTemplate;
 using v8::PropertyCallbackInfo;
+using v8::PropertyDescriptor;
 using v8::PropertyHandlerFlags;
 using v8::ReadOnly;
 using v8::String;
@@ -396,11 +397,39 @@ static void EnvEnumerator(const PropertyCallbackInfo<Array>& info) {
       env->env_vars()->Enumerate(env->isolate()));
 }
 
+static void EnvDefiner(Local<Name> property,
+                       const PropertyDescriptor& desc,
+                       const PropertyCallbackInfo<Value>& info) {
+  Environment* env = Environment::GetCurrent(info);
+  if (desc.has_value() && !desc.configurable() && !desc.enumerable() &&
+      !desc.writable()) {
+    THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env,
+                             "Must set all attributes with true to 'value'"
+                             " in 'process.env'");
+  } else if (desc.has_get() || desc.has_set() ||
+             (desc.has_configurable() && !desc.configurable()) ||
+             (desc.has_enumerable() && !desc.enumerable()) ||
+             (desc.has_writable() && !desc.writable())) {
+    THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env,
+                             "Cannot set attributes other than 'value'"
+                             " for properties in 'process.env'");
+  } else {
+    EnvSetter(property, desc.value(), info);
+  }
+}
+
 MaybeLocal<Object> CreateEnvVarProxy(Local<Context> context, Isolate* isolate) {
   EscapableHandleScope scope(isolate);
   Local<ObjectTemplate> env_proxy_template = ObjectTemplate::New(isolate);
   env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration(
-      EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, Local<Value>(),
+      EnvGetter,
+      EnvSetter,
+      EnvQuery,
+      EnvDeleter,
+      EnvEnumerator,
+      EnvDefiner,
+      nullptr,
+      Local<Value>(),
       PropertyHandlerFlags::kHasNoSideEffect));
   return scope.EscapeMaybe(env_proxy_template->NewInstance(context));
 }
@@ -411,6 +440,7 @@ void RegisterEnvVarExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(EnvQuery);
   registry->Register(EnvDeleter);
   registry->Register(EnvEnumerator);
+  registry->Register(EnvDefiner);
 }
 }  // namespace node
 

src/node_errors.h

@@ -64,6 +64,7 @@ void OnFatalError(const char* location, const char* message);
   V(ERR_INVALID_ARG_VALUE, TypeError)                                          \
   V(ERR_OSSL_EVP_INVALID_DIGEST, Error)                                        \
   V(ERR_INVALID_ARG_TYPE, TypeError)                                           \
+  V(ERR_INVALID_OBJECT_DEFINE_PROPERTY, TypeError)                             \
   V(ERR_INVALID_MODULE, Error)                                                 \
   V(ERR_INVALID_THIS, TypeError)                                               \
   V(ERR_INVALID_TRANSFER_OBJECT, TypeError)                                    \

test/parallel/test-process-env-delete.js

@@ -0,0 +1,13 @@
+'use strict';
+require('../common');
+const assert = require('assert');
+
+process.env.foo = 'foo';
+assert.strictEqual(process.env.foo, 'foo');
+process.env.foo = undefined;
+assert.strictEqual(process.env.foo, 'undefined');
+
+process.env.foo = 'foo';
+assert.strictEqual(process.env.foo, 'foo');
+delete process.env.foo;
+assert.strictEqual(process.env.foo, undefined);

test/parallel/test-process-env-ignore-getter-setter.js

@@ -0,0 +1,65 @@
+'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: 'Must set all attributes with true to \'value\' ' +
+             'in \'process.env\''
+  }
+);
+
+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: 'Cannot set attributes other than \'value\' ' +
+             'for properties in \'process.env\''
+  }
+);
+
+const attributes = ['configurable', 'writable', 'enumerable'];
+
+attributes.forEach((attribute) => {
+  assert.throws(
+    () => {
+      Object.defineProperty(process.env, 'goo', {
+        [attribute]: false
+      });
+    },
+    {
+      code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY',
+      name: 'TypeError',
+      message: 'Cannot set attributes other than \'value\' ' +
+               'for properties in \'process.env\''
+    }
+  );
+});
+
+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/parallel/test-worker-process-env.js

@@ -39,8 +39,20 @@ if (!workerData && process.argv[2] !== 'child') {
   process.env.SET_IN_WORKER = 'set';
   assert.strictEqual(process.env.SET_IN_WORKER, 'set');
 
-  Object.defineProperty(process.env, 'DEFINED_IN_WORKER', { value: 42 });
-  assert.strictEqual(process.env.DEFINED_IN_WORKER, '42');
+  assert.throws(
+    () => {
+      Object.defineProperty(process.env, 'DEFINED_IN_WORKER', {
+        value: 42
+      });
+    },
+    {
+      code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY',
+      name: 'TypeError',
+      message: 'Must set all attributes with true to \'value\' ' +
+               'in \'process.env\''
+    }
+  );
+
 
   const { stderr } =
     child_process.spawnSync(process.execPath, [__filename, 'child']);