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.

PR-URL: #28006
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

Author: himself65

doc/api/errors.md

@@ -1921,6 +1921,13 @@ 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,57 @@ 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()) {
+    if (!desc.has_writable() ||
+        !desc.has_enumerable() ||
+        !desc.has_configurable()) {
+      THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env,
+                                               "'process.env' only accepts a "
+                                               "configurable, writable,"
+                                               " and enumerable "
+                                               "data descriptor");
+    } else if (!desc.configurable() ||
+               !desc.enumerable() ||
+               !desc.writable()) {
+      THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env,
+                                               "'process.env' only accepts a "
+                                               "configurable, writable,"
+                                               " and enumerable "
+                                               "data descriptor");
+    } else {
+      return EnvSetter(property, desc.value(), info);
+    }
+  } else if (desc.has_get() || desc.has_set()) {
+    // we don't accept a getter/setter in 'process.env'
+    THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env,
+                             "'process.env' does not accept an"
+                                             "accessor(getter/setter)"
+                                             " descriptor");
+  } else {
+    THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env,
+                                             "'process.env' only accepts a "
+                                             "configurable, writable,"
+                                             " and enumerable "
+                                             "data descriptor");
+  }
+}
+
 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 +458,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,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'];
+
+attributes.forEach((attribute) => {
+  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/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: '\'process.env\' only accepts a configurable, ' +
+          'writable, and enumerable data descriptor'
+    }
+  );
+
 
   const { stderr } =
     child_process.spawnSync(process.execPath, [__filename, 'child']);