watch: strip watch flags from NODE_OPTIONS in child process

Signed-off-by: marcopiraccini <marco.piraccini@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/62143
Fixes: https://github.com/nodejs/node/issues/61740
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

lib/internal/main/watch_mode.js

@@ -6,6 +6,7 @@ const {
   ArrayPrototypePush,
   ArrayPrototypePushApply,
   ArrayPrototypeSlice,
+  StringPrototypeIncludes,
   StringPrototypeStartsWith,
 } = primordials;
 
@@ -17,7 +18,7 @@ const {
   triggerUncaughtException,
   exitCodes: { kNoFailure },
 } = internalBinding('errors');
-const { getOptionValue } = require('internal/options');
+const { getOptionValue, parseNodeOptionsEnvVar } = require('internal/options');
 const { FilesWatcher } = require('internal/watch_mode/files_watcher');
 const { green, blue, red, white, clear } = require('internal/util/colors');
 const { convertToValidSignal, kEmptyObject } = require('internal/util');
@@ -78,6 +79,37 @@ for (let i = 0; i < process.execArgv.length; i++) {
 
 ArrayPrototypePushApply(argsWithoutWatchOptions, kCommand);
 
+// Strip watch-related flags from NODE_OPTIONS to prevent infinite loop
+// when NODE_OPTIONS contains --watch (see issue #61740).
+const kNodeOptions = process.env.NODE_OPTIONS;
+let cleanNodeOptions = kNodeOptions;
+if (kNodeOptions != null) {
+  const keep = [];
+  const parts = parseNodeOptionsEnvVar(kNodeOptions);
+  for (let i = 0; i < parts.length; i++) {
+    const part = parts[i];
+    if (part === '--watch' ||
+        part === '--watch-preserve-output' ||
+        StringPrototypeStartsWith(part, '--watch=') ||
+        StringPrototypeStartsWith(part, '--watch-preserve-output=') ||
+        StringPrototypeStartsWith(part, '--watch-path=') ||
+        StringPrototypeStartsWith(part, '--watch-kill-signal=')) {
+      continue;
+    }
+    if (part === '--watch-path' || part === '--watch-kill-signal') {
+      // Skip the flag and its separate value argument
+      i++;
+      continue;
+    }
+    // The C++ tokenizer strips quotes during parsing, so values that
+    // originally contained spaces (e.g. --require "./path with spaces/f.js")
+    // need to be re-quoted before rejoining into a single string, otherwise
+    // the child's C++ parser would split them into separate tokens.
+    ArrayPrototypePush(keep, StringPrototypeIncludes(part, ' ') ? `"${part}"` : part);
+  }
+  cleanNodeOptions = ArrayPrototypeJoin(keep, ' ');
+}
+
 const watcher = new FilesWatcher({ debounce: 200, mode: kShouldFilterModules ? 'filter' : 'all' });
 ArrayPrototypeForEach(kWatchedPaths, (p) => watcher.watchPath(p));
 
@@ -96,6 +128,7 @@ function start() {
     env: {
       ...process.env,
       WATCH_REPORT_DEPENDENCIES: '1',
+      NODE_OPTIONS: cleanNodeOptions,
     },
   });
   watcher.watchChildProcessModules(child);

lib/internal/options.js

@@ -16,6 +16,7 @@ const {
   getEmbedderOptions: getEmbedderOptionsFromBinding,
   getEnvOptionsInputType,
   getNamespaceOptionsInputType,
+  parseNodeOptionsEnvVar,
 } = internalBinding('options');
 
 let warnOnAllowUnauthorized = true;
@@ -222,5 +223,6 @@ module.exports = {
   getAllowUnauthorized,
   getEmbedderOptions,
   generateConfigJsonSchema,
+  parseNodeOptionsEnvVar,
   refreshOptions,
 };

src/node_options.cc

@@ -2329,6 +2329,29 @@ void GetOptionsAsFlags(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(result);
 }
 
+void ParseNodeOptionsEnvVarBinding(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+  Local<Context> context = isolate->GetCurrentContext();
+
+  Utf8Value node_options(isolate, args[0]);
+  std::string options_str(*node_options, node_options.length());
+
+  std::vector<std::string> errors;
+  std::vector<std::string> result =
+      ParseNodeOptionsEnvVar(options_str, &errors);
+
+  if (!errors.empty()) {
+    Environment* env = Environment::GetCurrent(context);
+    env->ThrowError(errors[0].c_str());
+    return;
+  }
+
+  Local<Value> v8_result;
+  if (ToV8Value(context, result).ToLocal(&v8_result)) {
+    args.GetReturnValue().Set(v8_result);
+  }
+}
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
@@ -2349,6 +2372,8 @@ void Initialize(Local<Object> target,
                         target,
                         "getNamespaceOptionsInputType",
                         GetNamespaceOptionsInputType);
+  SetMethodNoSideEffect(
+      context, target, "parseNodeOptionsEnvVar", ParseNodeOptionsEnvVarBinding);
   Local<Object> env_settings = Object::New(isolate);
   NODE_DEFINE_CONSTANT(env_settings, kAllowedInEnvvar);
   NODE_DEFINE_CONSTANT(env_settings, kDisallowedInEnvvar);
@@ -2377,6 +2402,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(GetEmbedderOptions);
   registry->Register(GetEnvOptionsInputType);
   registry->Register(GetNamespaceOptionsInputType);
+  registry->Register(ParseNodeOptionsEnvVarBinding);
 }
 }  // namespace options_parser
 

test/parallel/test-options-binding.js

@@ -3,7 +3,7 @@
 
 const common = require('../common');
 const assert = require('assert');
-const { getOptionValue } = require('internal/options');
+const { getOptionValue, parseNodeOptionsEnvVar } = require('internal/options');
 
 Map.prototype.get =
   common.mustNotCall('`getOptionValue` must not call user-mutable method');
@@ -14,3 +14,30 @@ assert.strictEqual(getOptionValue('--nonexistent-option'), undefined);
 
 // Make the test common global leak test happy.
 delete Object.prototype['--nonexistent-option'];
+
+// parseNodeOptionsEnvVar tokenizes a NODE_OPTIONS-style string.
+assert.deepStrictEqual(
+  parseNodeOptionsEnvVar('--max-old-space-size=4096 --no-warnings'),
+  ['--max-old-space-size=4096', '--no-warnings']
+);
+
+// Quoted strings are unquoted during parsing.
+assert.deepStrictEqual(
+  parseNodeOptionsEnvVar('--require "file with spaces.js"'),
+  ['--require', 'file with spaces.js']
+);
+
+// Empty string returns an empty array.
+assert.deepStrictEqual(parseNodeOptionsEnvVar(''), []);
+
+// Throws on unterminated string.
+assert.throws(
+  () => parseNodeOptionsEnvVar('--require "unterminated'),
+  { name: 'Error', message: /unterminated string/ }
+);
+
+// Throws on invalid escape at end of string.
+assert.throws(
+  () => parseNodeOptionsEnvVar('--require "foo\\'),
+  { name: 'Error', message: /invalid escape/ }
+);

test/sequential/test-watch-mode.mjs

@@ -1,6 +1,7 @@
 import * as common from '../common/index.mjs';
 import tmpdir from '../common/tmpdir.js';
 import assert from 'node:assert';
+import os from 'node:os';
 import path from 'node:path';
 import { execPath } from 'node:process';
 import { describe, it } from 'node:test';
@@ -993,4 +994,112 @@ process.on('message', (message) => {
       await done();
     }
   });
+
+  it('should strip all watch flags from NODE_OPTIONS in child process', async () => {
+    const file = createTmpFile('console.log(process.env.NODE_OPTIONS);');
+    const nodeOptions = [
+      '--watch',
+      '--watch=true',
+      '--watch-path=./src',
+      '--watch-path', './test',
+      '--watch-preserve-output',
+      '--watch-preserve-output=true',
+      '--watch-kill-signal=SIGKILL',
+      '--watch-kill-signal', 'SIGINT',
+      '--max-old-space-size=4096',
+      '--no-warnings',
+    ].join(' ');
+    const { done, restart } = runInBackground({
+      args: ['--watch', file],
+      options: {
+        env: { ...process.env, NODE_OPTIONS: nodeOptions },
+      },
+    });
+
+    try {
+      const { stdout, stderr } = await restart();
+
+      assert.strictEqual(stderr, '');
+      const nodeOptionsLine = stdout.find((line) => line.includes('--max-old-space-size'));
+      assert.ok(nodeOptionsLine);
+      assert.strictEqual(nodeOptionsLine, '--max-old-space-size=4096 --no-warnings');
+    } finally {
+      await done();
+    }
+  });
+
+  it('should not strip --watch when it appears inside a quoted NODE_OPTIONS value', {
+    // Honoring --require from NODE_OPTIONS is required for this test.
+    skip: !!process.config.variables.node_without_node_options,
+  }, async () => {
+    // Use /tmp to avoid CI directories with special characters (e.g. ")
+    // that would break NODE_OPTIONS parsing.
+    const watchDir = path.join(os.tmpdir(), 'test for --watch parsing');
+    mkdirSync(watchDir, { recursive: true });
+    const reqFile = path.join(watchDir, 'req.cjs');
+    writeFileSync(reqFile, 'globalThis.requiredOk = true;');
+
+    const file = createTmpFile('console.log("required:" + !!globalThis.requiredOk);');
+    const nodeOptions = `--watch --require "${reqFile}"`;
+    const { done, restart } = runInBackground({
+      args: ['--watch', file],
+      options: {
+        env: { ...process.env, NODE_OPTIONS: nodeOptions },
+      },
+    });
+
+    try {
+      const { stdout, stderr } = await restart();
+
+      assert.strictEqual(stderr, '');
+      assert.ok(stdout.some((line) => line.includes('required:true')));
+    } finally {
+      await done();
+    }
+  });
+
+  it('should handle NODE_OPTIONS containing only watch flags', async () => {
+    const file = createTmpFile('console.log(JSON.stringify(process.env.NODE_OPTIONS));');
+    const { done, restart } = runInBackground({
+      args: ['--watch', file],
+      options: {
+        env: { ...process.env, NODE_OPTIONS: '--watch' },
+      },
+    });
+
+    try {
+      const { stdout, stderr } = await restart();
+
+      assert.strictEqual(stderr, '');
+      assert.ok(stdout.some((line) => line.includes('""')));
+    } finally {
+      await done();
+    }
+  });
+
+  it('should strip multiple --watch-path entries from NODE_OPTIONS', async () => {
+    const file = createTmpFile('console.log(process.env.NODE_OPTIONS);');
+    // Use /tmp to avoid CI directories with special characters (e.g. ")
+    // that would break NODE_OPTIONS parsing.
+    const dirA = path.join(os.tmpdir(), 'node-watch-path-a');
+    const dirB = path.join(os.tmpdir(), 'node-watch-path-b');
+    mkdirSync(dirA, { recursive: true });
+    mkdirSync(dirB, { recursive: true });
+    const nodeOptions = `--watch --watch-path=${dirA} --watch-path ${dirB} --no-warnings`;
+    const { done, restart } = runInBackground({
+      args: ['--watch', file],
+      options: {
+        env: { ...process.env, NODE_OPTIONS: nodeOptions },
+      },
+    });
+
+    try {
+      const { stdout, stderr } = await restart();
+
+      assert.strictEqual(stderr, '');
+      assert.ok(stdout.some((line) => line === '--no-warnings'));
+    } finally {
+      await done();
+    }
+  });
 });