child_process: inherit a reassigned process.env

Since v26.11.0, spawning without options.env builds the child's
environment from the OS environment block. Code that replaces
process.env, e.g. process.env = { ...process.env, FOO: 'x' }, had its
changes silently dropped. The child_process docs give process.env as
the default for options.env, and { env: process.env } still saw the
replacement object, so the two disagreed.

Make getEnvPairs() take the object to read and return undefined unless
it is still the native env proxy. When it isn't, copy process.env as
before. A plain object is copied without going through the env
interceptors, so that path stays cheap.

Assisted-by: a closed-source coding agent
Signed-off-by: Caleb ツ Everett <calebev@amazon.com>
PR-URL: #66883
Fixes: #66882
Refs: #65325
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Author: everett1992

lib/child_process.js

@@ -745,8 +745,11 @@ function normalizeSpawnArguments(file, args, options) {
     // instead of copying process.env (one interceptor round trip and one
     // getenv() scan per variable). Everything copyProcessEnvToEnv() would
     // propagate below is part of it by definition, and entries of the real
-    // environment block cannot contain null bytes.
-    envPairs = getEnvPairs();
+    // environment block cannot contain null bytes. Undefined if process.env
+    // was reassigned.
+    envPairs = getEnvPairs(process.env);
+  }
+  if (envPairs !== undefined) {
     if (process.platform === 'win32') {
       envPairs = dedupeWindowsEnvPairs(envPairs);
     }

src/process_wrap.cc

@@ -346,9 +346,17 @@ class ProcessWrap : public HandleWrap {
 
   // The current environment as ["KEY=value", ...], i.e. what a spawned
   // child inherits by default, produced in one pass over the environment
-  // block instead of one interceptor round trip per variable.
+  // block instead of one interceptor round trip per variable. Returns
+  // undefined if args[0] is not the env proxy, e.g. because process.env was
+  // reassigned, in which case the caller has to copy that object instead.
   static void GetEnvPairs(const FunctionCallbackInfo<Value>& args) {
     Environment* env = Environment::GetCurrent(args);
+    Local<FunctionTemplate> env_proxy_ctor_template =
+        env->env_proxy_ctor_template();
+    if (env_proxy_ctor_template.IsEmpty() ||
+        !env_proxy_ctor_template->HasInstance(args[0])) {
+      return;
+    }
     Local<Array> pairs;
     if (env->env_vars()->Pairs(env->isolate()).ToLocal(&pairs)) {
       args.GetReturnValue().Set(pairs);

test/parallel/test-child-process-env-reassigned.js

@@ -0,0 +1,103 @@
+'use strict';
+// When no `env` option is given, a child process inherits process.env, even
+// if process.env has been replaced with another object.
+// Refs: https://github.com/nodejs/node/issues/66882
+const common = require('../common');
+const assert = require('assert');
+const {
+  exec,
+  execFile,
+  execFileSync,
+  execSync,
+  fork,
+  spawn,
+  spawnSync,
+} = require('child_process');
+
+if (process.argv[2] === 'child') {
+  process.send(process.env.TEST_REASSIGNED);
+  return;
+}
+
+const printEnv = ['-p', 'process.env.TEST_REASSIGNED'];
+const shellEcho = common.isWindows ?
+  'echo %TEST_REASSIGNED%' :
+  'echo "$TEST_REASSIGNED"';
+
+function runSync(options) {
+  const child = spawnSync(process.execPath, printEnv,
+                          { encoding: 'utf8', ...options });
+  assert.strictEqual(child.status, 0, child.stderr || String(child.error));
+  return child.stdout.trim();
+}
+
+const originalEnv = process.env;
+// Lives in the real environment, but not in the replacement object.
+process.env.TEST_ONLY_IN_ORIGINAL = 'original';
+process.env = { ...process.env, TEST_REASSIGNED: 'reassigned' };
+delete process.env.TEST_ONLY_IN_ORIGINAL;
+
+assert.strictEqual(runSync(), 'reassigned');
+assert.strictEqual(
+  execFileSync(process.execPath, printEnv, { encoding: 'utf8' }).trim(),
+  'reassigned');
+assert.strictEqual(execSync(shellEcho, { encoding: 'utf8' }).trim(),
+                   'reassigned');
+
+{
+  const child = spawnSync(
+    process.execPath,
+    ['-p', 'process.env.TEST_ONLY_IN_ORIGINAL'],
+    { encoding: 'utf8' });
+  assert.strictEqual(child.stdout.trim(), 'undefined');
+}
+
+// The default env matches an explicit { env: process.env }.
+{
+  const printAll = ['-e', 'process.stdout.write(JSON.stringify(process.env))'];
+  const run = (options) => JSON.parse(
+    spawnSync(process.execPath, printAll, { encoding: 'utf8', ...options })
+      .stdout);
+  assert.deepStrictEqual(run(), run({ env: process.env }));
+}
+
+// The replacement object is used for command lookup.
+if (!common.isWindows) {
+  const saved = process.env;
+  process.env = { ...saved, PATH: '/nonexistent' };
+  const child = spawnSync('ls');
+  process.env = saved;
+  assert.strictEqual(child.error?.code, 'ENOENT');
+}
+
+{
+  const child = spawn(process.execPath, printEnv);
+  let out = '';
+  child.stdout.setEncoding('utf8').on('data', (d) => { out += d; });
+  let err = '';
+  child.stderr.setEncoding('utf8').on('data', (d) => { err += d; });
+  child.on('close', common.mustCall((code) => {
+    assert.strictEqual(code, 0, err);
+    assert.strictEqual(out.trim(), 'reassigned');
+  }));
+}
+
+exec(shellEcho, common.mustSucceed((stdout) => {
+  assert.strictEqual(stdout.trim(), 'reassigned');
+}));
+
+execFile(process.execPath, printEnv, common.mustSucceed((stdout) => {
+  assert.strictEqual(stdout.trim(), 'reassigned');
+}));
+
+fork(__filename, ['child']).on('message', common.mustCall((value) => {
+  assert.strictEqual(value, 'reassigned');
+}));
+
+// Putting the original object back restores inheriting the real environment.
+process.env = originalEnv;
+assert.strictEqual(runSync(), 'undefined');
+assert.strictEqual(
+  spawnSync(process.execPath, ['-p', 'process.env.TEST_ONLY_IN_ORIGINAL'],
+            { encoding: 'utf8' }).stdout.trim(),
+  'original');