src: allow empty --experimental-config-file

PR-URL: #61610
Reviewed-By: Pietro Marchini <pietro.marchini94@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>

Author: marco-ippolito

doc/api/cli.md

@@ -963,7 +963,7 @@ added: v23.6.0
 
 Enable experimental import support for `.node` addons.
 
-### `--experimental-config-file=config`
+### `--experimental-config-file=path`, `--experimental-config-file`
 
 <!-- YAML
 added: v23.10.0
@@ -972,6 +972,12 @@ added: v23.10.0
 > Stability: 1.0 - Early development
 
 If present, Node.js will look for a configuration file at the specified path.
+If the path is not specified, Node.js will look for a `node.config.json` file
+in the current working directory.
+To specify a custom path, use the `--experimental-config-file=path` form.
+The space-separated `--experimental-config-file path` form is not supported.
+The alias `--experimental-default-config-file` is equivalent to
+`--experimental-config-file` without an argument.
 Node.js will read the configuration file and apply the settings. The
 configuration file should be a JSON file with the following structure. `vX.Y.Z`
 in the `$schema` must be replaced with the version of Node.js you are using.
@@ -1043,9 +1049,10 @@ added: v23.10.0
 
 > Stability: 1.0 - Early development
 
-If the `--experimental-default-config-file` flag is present, Node.js will look for a
+This flag is an alias for `--experimental-config-file` without an argument.
+If present, Node.js will look for a
 `node.config.json` file in the current working directory and load it as a
-as configuration file.
+configuration file.
 
 ### `--experimental-eventsource`
 

doc/api/test.md

@@ -4503,7 +4503,7 @@ test.describe('my suite', (suite) => {
 [`suite()`]: #suitename-options-fn
 [`test()`]: #testname-options-fn
 [code coverage]: #collecting-code-coverage
-[configuration files]: cli.md#--experimental-config-fileconfig
+[configuration files]: cli.md#--experimental-config-filepath---experimental-config-file
 [describe options]: #describename-options-fn
 [it options]: #testname-options-fn
 [module customization hooks]: module.md#customization-hooks

doc/node.1

@@ -169,11 +169,75 @@ Interpret the entry point as a URL.
 .It Fl -experimental-addon-modules
 Enable experimental addon module support.
 .
-.It Fl -experimental-config-file
-Specifies the configuration file to load.
+.It Fl -experimental-config-file , Fl -experimental-config-file Ns = Ns Ar config , Fl -experimental-default-config-file
+If present, Node.js will look for a configuration file at the specified path.
+If the path is not specified, Node.js will look for a \fBnode.config.json\fR file
+in the current working directory.
+To specify a custom path, use the \fB--experimental-config-file=\fR\fBpath\fR form.
+The space-separated \fB--experimental-config-file path\fR form is not supported.
+Node.js will read the configuration file and apply the settings. The
+configuration file should be a JSON file with the following structure. \fBvX.Y.Z\fR
+in the \fB$schema\fR must be replaced with the version of Node.js you are using.
+.Bd -literal
+{
+  "$schema": "https://nodejs.org/dist/vX.Y.Z/docs/node-config-schema.json",
+  "nodeOptions": {
+    "import": [
+      "amaro/strip"
+    ],
+    "watch-path": "src",
+    "watch-preserve-output": true
+  },
+  "testRunner": {
+    "test-isolation": "process"
+  },
+  "watch": {
+    "watch-preserve-output": true
+  }
+}
+.Ed
+The configuration file supports namespace-specific options:
+.Bl -bullet
+.It
+The \fBnodeOptions\fR field contains CLI flags that are allowed in \fBNODE_OPTIONS\fR.
+.It
+Namespace fields like \fBtestRunner\fR contain configuration specific to that subsystem.
+.El
+No-op flags are not supported.
+Not all V8 flags are currently supported.
+It is possible to use the official JSON schema
+to validate the configuration file, which may vary depending on the Node.js version.
+Each key in the configuration file corresponds to a flag that can be passed
+as a command-line argument. The value of the key is the value that would be
+passed to the flag.
+For example, the configuration file above is equivalent to
+the following command-line arguments:
+.Bd -literal
+node --import amaro/strip --watch-path=src --watch-preserve-output --test-isolation=process
+.Ed
+The priority in configuration is as follows:
+.Bl -bullet
+.It
+NODE_OPTIONS and command-line options
+.It
+Dotenv NODE_OPTIONS
+.It
+Configuration file
+.El
+Values in the configuration file will not override the values in the environment
+variables, command-line options, or the \fBNODE_OPTIONS\fR env file parsed by the
+\fB--env-file\fR flag.
+Keys cannot be duplicated within the same or different namespaces.
+The configuration parser will throw an error if the configuration file contains
+unknown keys or keys that cannot be used in a namespace.
+Node.js will not sanitize or perform validation on the user-provided configuration,
+so \fBNEVER\fR use untrusted configuration files.
 .
 .It Fl -experimental-default-config-file
-Enable support for automatically loading node.config.json.
+This flag is an alias for \fB--experimental-config-file\fR without an argument.
+If present, Node.js will look for a
+\fBnode.config.json\fR file in the current working directory and load it as a
+configuration file.
 .
 .It Fl -experimental-import-meta-resolve
 Enable experimental ES modules support for import.meta.resolve().

lib/internal/main/watch_mode.js

@@ -1,7 +1,6 @@
 'use strict';
 const {
   ArrayPrototypeForEach,
-  ArrayPrototypeIncludes,
   ArrayPrototypeJoin,
   ArrayPrototypeMap,
   ArrayPrototypePush,
@@ -67,11 +66,8 @@ for (let i = 0; i < process.execArgv.length; i++) {
     }
     continue;
   }
-  if (StringPrototypeStartsWith(arg, '--experimental-config-file')) {
-    if (!ArrayPrototypeIncludes(arg, '=')) {
-      // Skip the flag and the next argument (the config file path)
-      i++;
-    }
+  if (arg === '--experimental-config-file' ||
+      StringPrototypeStartsWith(arg, '--experimental-config-file=')) {
     continue;
   }
   if (arg === '--experimental-default-config-file') {

lib/internal/process/pre_execution.js

@@ -386,8 +386,7 @@ function setupSQLite() {
 }
 
 function initializeConfigFileSupport() {
-  if (getOptionValue('--experimental-default-config-file') ||
-      getOptionValue('--experimental-config-file')) {
+  if (getOptionValue('--experimental-config-file')) {
     emitExperimentalWarning('--experimental-config-file');
   }
 }

lib/internal/test_runner/runner.js

@@ -161,7 +161,11 @@ function createTestFileList(patterns, cwd) {
 
 function filterExecArgv(arg, i, arr) {
   return !ArrayPrototypeIncludes(kFilterArgs, arg) &&
-  !ArrayPrototypeSome(kFilterArgValues, (p) => arg === p || (i > 0 && arr[i - 1] === p) || StringPrototypeStartsWith(arg, `${p}=`));
+  !ArrayPrototypeSome(kFilterArgValues, (p) => {
+    return arg === p ||
+      StringPrototypeStartsWith(arg, `${p}=`) ||
+      (p !== '--experimental-config-file' && i > 0 && arr[i - 1] === p);
+  });
 }
 
 function getRunArgs(path, { forceExit,

src/node.cc

@@ -911,15 +911,21 @@ static ExitCode InitializeNodeWithArgsInternal(
   }
 
   std::string node_options_from_config;
-  if (auto path = per_process::config_reader.GetDataFromArgs(*argv)) {
-    switch (per_process::config_reader.ParseConfig(*path)) {
+  auto config_path = per_process::config_reader.GetDataFromArgs(argv);
+  if (per_process::config_reader.HasInvalidDefaultConfigFileArgument()) {
+    errors->push_back("--experimental-default-config-file does not take an "
+                      "argument");
+    return ExitCode::kInvalidCommandLineArgument;
+  }
+  if (config_path) {
+    switch (per_process::config_reader.ParseConfig(*config_path)) {
       case ParseResult::Valid:
         break;
       case ParseResult::InvalidContent:
-        errors->push_back(std::string(*path) + ": invalid content");
+        errors->push_back(std::string(*config_path) + ": invalid content");
         break;
       case ParseResult::FileError:
-        errors->push_back(std::string(*path) + ": not found");
+        errors->push_back(std::string(*config_path) + ": not found");
         break;
       default:
         UNREACHABLE();

src/node_config_file.cc

@@ -2,39 +2,53 @@
 #include "debug_utils-inl.h"
 #include "simdjson.h"
 
-#include <string>
-
 namespace node {
 
+constexpr std::string_view kConfigFileFlag = "--experimental-config-file";
+constexpr std::string_view kDefaultConfigFileFlag =
+    "--experimental-default-config-file";
+constexpr std::string_view kDefaultConfigFileName = "node.config.json";
+
+inline bool HasEqualsPrefix(std::string_view arg, std::string_view flag) {
+  return arg.size() > flag.size() && arg.starts_with(flag) &&
+         arg[flag.size()] == '=';
+}
+
 std::optional<std::string_view> ConfigReader::GetDataFromArgs(
-    const std::vector<std::string>& args) {
-  constexpr std::string_view flag_path = "--experimental-config-file";
-  constexpr std::string_view default_file =
-      "--experimental-default-config-file";
-
-  bool has_default_config_file = false;
-
-  for (auto it = args.begin(); it != args.end(); ++it) {
-    if (*it == flag_path) {
-      // Case: "--experimental-config-file foo"
-      if (auto next = std::next(it); next != args.end()) {
-        return *next;
+    std::vector<std::string>* args) {
+  std::optional<std::string_view> result;
+  invalid_default_config_file_argument_ = false;
+
+  for (size_t i = 0; i < args->size(); ++i) {
+    std::string& arg = (*args)[i];
+
+    if (arg == kConfigFileFlag) {
+      // --experimental-config-file
+      arg = std::string(kConfigFileFlag) + "=" +
+            std::string(kDefaultConfigFileName);
+      result = kDefaultConfigFileName;
+    } else if (HasEqualsPrefix(arg, kConfigFileFlag)) {
+      // --experimental-config-file=path
+      std::string_view path =
+          std::string_view(arg).substr(kConfigFileFlag.size() + 1);
+      if (!path.empty()) {
+        result = path;
       }
-    } else if (it->starts_with(flag_path)) {
-      // Case: "--experimental-config-file=foo"
-      if (it->size() > flag_path.size() && (*it)[flag_path.size()] == '=') {
-        return std::string_view(*it).substr(flag_path.size() + 1);
-      }
-    } else if (*it == default_file || it->starts_with(default_file)) {
-      has_default_config_file = true;
+    } else if (arg == kDefaultConfigFileFlag) {
+      // --experimental-default-config-file
+      arg = std::string(kConfigFileFlag) + "=" +
+            std::string(kDefaultConfigFileName);
+      result = kDefaultConfigFileName;
+    } else if (HasEqualsPrefix(arg, kDefaultConfigFileFlag)) {
+      invalid_default_config_file_argument_ = true;
     }
   }
 
-  if (has_default_config_file) {
-    return "node.config.json";
-  }
+  return result;
+}
 
-  return std::nullopt;
+bool ConfigReader::HasInvalidDefaultConfigFileArgument() const {
+  return invalid_default_config_file_argument_;
 }
 
 ParseResult ConfigReader::ProcessOptionValue(

src/node_config_file.h

@@ -30,7 +30,8 @@ class ConfigReader {
   ParseResult ParseConfig(const std::string_view& config_path);
 
   std::optional<std::string_view> GetDataFromArgs(
-      const std::vector<std::string>& args);
+      std::vector<std::string>* args);
+  bool HasInvalidDefaultConfigFileArgument() const;
 
   std::string GetNodeOptions();
   const std::vector<std::string>& GetNamespaceFlags() const;
@@ -53,6 +54,7 @@ class ConfigReader {
 
   std::vector<std::string> node_options_;
   std::vector<std::string> namespace_options_;
+  bool invalid_default_config_file_argument_ = false;
 
   // Cache for fast lookup of environment options
   std::unordered_map<std::string, options_parser::OptionMappingDetails>

src/node_options.cc

@@ -851,11 +851,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
             &EnvironmentOptions::optional_env_file);
   Implies("--env-file-if-exists", "[has_env_file_string]");
   AddOption("--experimental-config-file",
-            "set config file from supplied file",
-            &EnvironmentOptions::experimental_config_file_path);
-  AddOption("--experimental-default-config-file",
-            "set config file from default config file",
-            &EnvironmentOptions::experimental_default_config_file);
+            "set config file path",
+            &EnvironmentOptions::experimental_config_file_path,
+            kDisallowedInEnvvar);
+  AddAlias("--experimental-default-config-file", "--experimental-config-file");
   AddOption("--test",
             "launch test runner on startup",
             &EnvironmentOptions::test_runner,