src: throw on process.env symbol usage

This commit causes process.env to throw when a symbol is used as
either a key or a value.

Fixes: https://github.com/nodejs/node/issues/9429
PR-URL: https://github.com/nodejs/node/pull/9446
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>

src/node.cc

@@ -122,7 +122,6 @@ using v8::ObjectTemplate;
 using v8::Promise;
 using v8::PromiseRejectMessage;
 using v8::PropertyCallbackInfo;
-using v8::PropertyHandlerFlags;
 using v8::ScriptOrigin;
 using v8::SealHandleScope;
 using v8::String;
@@ -2685,7 +2684,7 @@ static void EnvGetter(Local<Name> property,
     return info.GetReturnValue().Set(String::NewFromUtf8(isolate, val));
   }
 #else  // _WIN32
-  String::Value key(property);
+  node::TwoByteValue key(isolate, property);
   WCHAR buffer[32767];  // The maximum size allowed for environment variables.
   DWORD result = GetEnvironmentVariableW(reinterpret_cast<WCHAR*>(*key),
                                          buffer,
@@ -2711,8 +2710,8 @@ static void EnvSetter(Local<Name> property,
   node::Utf8Value val(info.GetIsolate(), value);
   setenv(*key, *val, 1);
 #else  // _WIN32
-  String::Value key(property);
+  node::TwoByteValue key(info.GetIsolate(), property);
-  String::Value val(value);
+  node::TwoByteValue val(info.GetIsolate(), value);
   WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
   // Environment variables that start with '=' are read-only.
   if (key_ptr[0] != L'=') {
@@ -2732,7 +2731,7 @@ static void EnvQuery(Local<Name> property,
   if (getenv(*key))
     rc = 0;
 #else  // _WIN32
-  String::Value key(property);
+  node::TwoByteValue key(info.GetIsolate(), property);
   WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
   if (GetEnvironmentVariableW(key_ptr, nullptr, 0) > 0 ||
       GetLastError() == ERROR_SUCCESS) {
@@ -2756,7 +2755,7 @@ static void EnvDeleter(Local<Name> property,
   node::Utf8Value key(info.GetIsolate(), property);
   unsetenv(*key);
 #else
-  String::Value key(property);
+  node::TwoByteValue key(info.GetIsolate(), property);
   WCHAR* key_ptr = reinterpret_cast<WCHAR*>(*key);
   SetEnvironmentVariableW(key_ptr, nullptr);
 #endif
@@ -3155,8 +3154,7 @@ void SetupProcessObject(Environment* env,
           EnvQuery,
           EnvDeleter,
           EnvEnumerator,
-          env->as_external(),
+          env->as_external()));
-          PropertyHandlerFlags::kOnlyInterceptStrings));
 
   Local<Object> process_env =
       process_env_template->NewInstance(env->context()).ToLocalChecked();

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

@@ -0,0 +1,26 @@
+'use strict';
+require('../common');
+
+const assert = require('assert');
+const symbol = Symbol('sym');
+const errRegExp = /^TypeError: Cannot convert a Symbol value to a string$/;
+
+// Verify that getting via a symbol key throws.
+assert.throws(() => {
+  process.env[symbol];
+}, errRegExp);
+
+// Verify that assigning via a symbol key throws.
+assert.throws(() => {
+  process.env[symbol] = 42;
+}, errRegExp);
+
+// Verify that assigning a symbol value throws.
+assert.throws(() => {
+  process.env.foo = symbol;
+}, errRegExp);
+
+// Verify that using a symbol with the in operator throws.
+assert.throws(() => {
+  symbol in process.env;
+}, errRegExp);

test/parallel/test-v8-interceptStrings-not-Symbols.js

@@ -1,34 +0,0 @@
-'use strict';
-require('../common');
-
-const assert = require('assert');
-
-// Test that the v8 named property handler intercepts callbacks
-// when properties are defined as Strings and NOT for Symbols.
-//
-// With the kOnlyInterceptStrings flag, manipulating properties via
-// Strings is intercepted by the callbacks, while Symbols adopt
-// the default global behaviour.
-// Removing the kOnlyInterceptStrings flag, adds intercepting to Symbols,
-// which causes Type Error at process.env[symbol]=42 due to process.env being
-// strongly typed for Strings
-// (node::Utf8Value key(info.GetIsolate(), property);).
-
-
-const symbol = Symbol('sym');
-
-// check if its undefined
-assert.strictEqual(process.env[symbol], undefined);
-
-// set a value using a Symbol
-process.env[symbol] = 42;
-
-// set a value using a String (call to EnvSetter, node.cc)
-process.env['s'] = 42;
-
-//check the values after substitutions
-assert.strictEqual(42, process.env[symbol]);
-assert.strictEqual('42', process.env['s']);
-
-delete process.env[symbol];
-assert.strictEqual(undefined, process.env[symbol]);