From e65bb1012f52add6157297df9d8bbec7c5bd65da Mon Sep 17 00:00:00 2001 From: himself65 Date: Mon, 3 Jun 2019 22:21:40 +0800 Subject: [PATCH 1/4] 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. --- doc/api/errors.md | 6 ++ src/node_env_var.cc | 32 ++++++++- src/node_errors.h | 1 + test/parallel/test-process-env-delete.js | 13 ++++ .../test-process-env-ignore-getter-setter.js | 65 +++++++++++++++++++ test/parallel/test-worker-process-env.js | 16 ++++- 6 files changed, 130 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 a22f125ce62c98..5e2b47d8f41051 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1919,6 +1919,12 @@ 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..313eda64954e2b 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,39 @@ 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() && !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 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 +440,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..3a4338a545500e --- /dev/null +++ b/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'); diff --git a/test/parallel/test-worker-process-env.js b/test/parallel/test-worker-process-env.js index 9680d685140f60..2cd1f498575a60 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: 'Must set all attributes with true to \'value\' ' + + 'in \'process.env\'' + } + ); + const { stderr } = child_process.spawnSync(process.execPath, [__filename, 'child']); From 37f2243a35704816e1f0a5a23cb3a6ca788182a7 Mon Sep 17 00:00:00 2001 From: himself65 Date: Tue, 11 Jan 2022 17:17:42 -0600 Subject: [PATCH 2/4] fixup! edit error message Refs: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty --- doc/api/errors.md | 3 +- src/node_env_var.cc | 36 ++++++++++++------- .../test-process-env-ignore-getter-setter.js | 14 ++++---- test/parallel/test-worker-process-env.js | 4 +-- 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 5e2b47d8f41051..4032e916904f8e 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1920,7 +1920,8 @@ The imported module string is an invalid URL, package name, or package subpath specifier. -### ERR_INVALID_OBJECT_DEFINE_PROPERTY + +### ERR\_INVALID\_OBJECT\_DEFINE\_PROPERTY An error occurred while setting an invalid attribute on the property of an object. diff --git a/src/node_env_var.cc b/src/node_env_var.cc index 313eda64954e2b..cdc15348ff949f 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -401,20 +401,32 @@ static void EnvDefiner(Local property, const PropertyDescriptor& desc, const PropertyCallbackInfo& 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())) { + 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, - "Cannot set attributes other than 'value'" - " for properties in 'process.env'"); + "'process.env' does not accept an" + "accessor(getter/setter) descriptor"); } else { - EnvSetter(property, desc.value(), info); + THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env, + "'process.env' only accepts a " + "configurable, writable," + " and enumerable data descriptor"); } } diff --git a/test/parallel/test-process-env-ignore-getter-setter.js b/test/parallel/test-process-env-ignore-getter-setter.js index 3a4338a545500e..7368eb85684c5a 100644 --- a/test/parallel/test-process-env-ignore-getter-setter.js +++ b/test/parallel/test-process-env-ignore-getter-setter.js @@ -11,8 +11,9 @@ assert.throws( { code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', name: 'TypeError', - message: 'Must set all attributes with true to \'value\' ' + - 'in \'process.env\'' + message: '\'process.env\' only accepts a ' + + 'configurable, writable,' + + ' and enumerable data descriptor' } ); @@ -32,8 +33,8 @@ assert.throws( { code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', name: 'TypeError', - message: 'Cannot set attributes other than \'value\' ' + - 'for properties in \'process.env\'' + message: '\'process.env\' does not accept an' + + 'accessor(getter/setter) descriptor' } ); @@ -49,8 +50,9 @@ attributes.forEach((attribute) => { { code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', name: 'TypeError', - message: 'Cannot set attributes other than \'value\' ' + - 'for properties in \'process.env\'' + message: '\'process.env\' only accepts a ' + + 'configurable, writable,' + + ' and enumerable data descriptor' } ); }); diff --git a/test/parallel/test-worker-process-env.js b/test/parallel/test-worker-process-env.js index 2cd1f498575a60..f43c2affa1f48b 100644 --- a/test/parallel/test-worker-process-env.js +++ b/test/parallel/test-worker-process-env.js @@ -48,8 +48,8 @@ if (!workerData && process.argv[2] !== 'child') { { code: 'ERR_INVALID_OBJECT_DEFINE_PROPERTY', name: 'TypeError', - message: 'Must set all attributes with true to \'value\' ' + - 'in \'process.env\'' + message: '\'process.env\' only accepts a configurable, ' + + 'writable, and enumerable data descriptor' } ); From 6ad7bdcbf6180312d40ac7eba8d44c5927cf9765 Mon Sep 17 00:00:00 2001 From: himself65 Date: Thu, 13 Jan 2022 11:31:24 -0600 Subject: [PATCH 3/4] fixup! format --- src/node_env_var.cc | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/node_env_var.cc b/src/node_env_var.cc index cdc15348ff949f..98f1ed07998e13 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -408,12 +408,16 @@ static void EnvDefiner(Local property, 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()) { + " 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"); + " and enumerable " + "data descriptor"); } else { return EnvSetter(property, desc.value(), info); } @@ -421,12 +425,14 @@ static void EnvDefiner(Local property, // 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"); + "accessor(getter/setter)" + " descriptor"); } else { THROW_ERR_INVALID_OBJECT_DEFINE_PROPERTY(env, "'process.env' only accepts a " "configurable, writable," - " and enumerable data descriptor"); + " and enumerable " + "data descriptor"); } } From 46acbbe504afecdb8be85521cbcc5676d62fc6f0 Mon Sep 17 00:00:00 2001 From: Himself65 Date: Thu, 13 Jan 2022 18:03:27 -0600 Subject: [PATCH 4/4] Update doc/api/errors.md Co-authored-by: Antoine du Hamel --- doc/api/errors.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 4032e916904f8e..83d634acb432b5 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1921,7 +1921,7 @@ specifier. -### ERR\_INVALID\_OBJECT\_DEFINE\_PROPERTY +### `ERR_INVALID_OBJECT_DEFINE_PROPERTY` An error occurred while setting an invalid attribute on the property of an object.