From e6a7300a10eb7de8a79e1ace26631fb611b64bfe Mon Sep 17 00:00:00 2001 From: Himself65 Date: Mon, 4 Apr 2022 03:24:17 -0500 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/28006 Reviewed-By: Rich Trott Reviewed-By: Ben Noordhuis Reviewed-By: Joyee Cheung Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Antoine du Hamel --- doc/api/errors.md | 7 ++ src/node_env_var.cc | 50 +++++++++++++- src/node_errors.h | 1 + test/parallel/test-process-env-delete.js | 13 ++++ .../test-process-env-ignore-getter-setter.js | 67 +++++++++++++++++++ test/parallel/test-worker-process-env.js | 16 ++++- 6 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 test/parallel/test-process-env-delete.js create mode 100644 test/parallel/test-process-env-ignore-getter-setter.js diff --git a/doc/api/errors.md b/doc/api/errors.md index e25561147d571e..a11a94981fefa6 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1921,6 +1921,13 @@ valid. The imported module string is an invalid URL, package name, or package subpath specifier. + + +### `ERR_INVALID_OBJECT_DEFINE_PROPERTY` + +An error occurred while setting an invalid attribute on the property of +an object. + ### `ERR_INVALID_PACKAGE_CONFIG` diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 27c833d498ec77..98f1ed07998e13 100644 --- a/src/node_env_var.cc +++ b/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& info) { env->env_vars()->Enumerate(env->isolate())); } +static void EnvDefiner(Local property, + const PropertyDescriptor& desc, + const PropertyCallbackInfo& 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 CreateEnvVarProxy(Local context, Isolate* isolate) { EscapableHandleScope scope(isolate); Local env_proxy_template = ObjectTemplate::New(isolate); env_proxy_template->SetHandler(NamedPropertyHandlerConfiguration( - EnvGetter, EnvSetter, EnvQuery, EnvDeleter, EnvEnumerator, Local(), + EnvGetter, + EnvSetter, + EnvQuery, + EnvDeleter, + EnvEnumerator, + EnvDefiner, + nullptr, + Local(), 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 diff --git a/src/node_errors.h b/src/node_errors.h index f540b3e2a37de4..d5c669b3df12b9 100644 --- a/src/node_errors.h +++ b/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) \ diff --git a/test/parallel/test-process-env-delete.js b/test/parallel/test-process-env-delete.js new file mode 100644 index 00000000000000..3653a13911275e --- /dev/null +++ b/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); diff --git a/test/parallel/test-process-env-ignore-getter-setter.js b/test/parallel/test-process-env-ignore-getter-setter.js new file mode 100644 index 00000000000000..7368eb85684c5a --- /dev/null +++ b/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'); diff --git a/test/parallel/test-worker-process-env.js b/test/parallel/test-worker-process-env.js index 9680d685140f60..f43c2affa1f48b 100644 --- a/test/parallel/test-worker-process-env.js +++ b/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']);