src: parse dotenv with the rest of the options

src/node.cc

@@ -800,6 +800,58 @@ int ProcessGlobalArgs(std::vector<std::string>* args,
 
 static std::atomic_bool init_called{false};
 
+static ExitCode ProcessEnvFiles(std::vector<std::string>* argv,
+                                std::vector<std::string>* errors) {
+  const auto& cli_options = per_process::cli_options->per_isolate->per_env;
+
+  // Check if either env_files or optional_env_files is not empty
+  if (!cli_options->env_files.empty() ||
+      !cli_options->optional_env_files.empty()) {
+    CHECK(!per_process::v8_initialized);
+
+    // Helper function to process individual environment files
+    auto process_files = [&](const std::vector<std::string>& files,
+                             bool is_optional) {
+      for (const auto& file_path : files) {
+        switch (per_process::dotenv_file.ParsePath(file_path)) {
+          case Dotenv::ParseResult::Valid:
+            break;
+          case Dotenv::ParseResult::InvalidContent:
+            errors->push_back(file_path + ": invalid format");
+            break;
+          case Dotenv::ParseResult::FileError:
+            if (is_optional) {
+              fprintf(stderr,
+                      "%s not found. Continuing without it.\n",
+                      file_path.c_str());
+            } else {
+              errors->push_back(file_path + ": not found");
+            }
+            break;
+          default:
+            UNREACHABLE();
+        }
+      }
+    };
+
+    // Process required and optional environment files
+    process_files(cli_options->optional_env_files, true);
+    process_files(cli_options->env_files, false);
+
+#if !defined(NODE_WITHOUT_NODE_OPTIONS)
+    std::vector<std::string> env_argv = ParseNodeOptionsEnvVar(
+        per_process::dotenv_file.GetNodeOptions(), errors);
+    env_argv.insert(env_argv.begin(), argv->at(0));
+
+    const ExitCode exit_code =
+        ProcessGlobalArgsInternal(&env_argv, nullptr, errors, kAllowedInEnvvar);
+    if (exit_code != ExitCode::kNoFailure) return exit_code;
+#endif
+  }
+
+  return ExitCode::kNoFailure;
+}
+
 // TODO(addaleax): Turn this into a wrapper around InitializeOncePerProcess()
 // (with the corresponding additional flags set), then eventually remove this.
 static ExitCode InitializeNodeWithArgsInternal(
@@ -851,34 +903,6 @@ static ExitCode InitializeNodeWithArgsInternal(
   HandleEnvOptions(per_process::cli_options->per_isolate->per_env);
 
   std::string node_options;
-  auto env_files = node::Dotenv::GetDataFromArgs(*argv);
-
-  if (!env_files.empty()) {
-    CHECK(!per_process::v8_initialized);
-
-    for (const auto& file_data : env_files) {
-      switch (per_process::dotenv_file.ParsePath(file_data.path)) {
-        case Dotenv::ParseResult::Valid:
-          break;
-        case Dotenv::ParseResult::InvalidContent:
-          errors->push_back(file_data.path + ": invalid format");
-          break;
-        case Dotenv::ParseResult::FileError:
-          if (file_data.is_optional) {
-            fprintf(stderr,
-                    "%s not found. Continuing without it.\n",
-                    file_data.path.c_str());
-            continue;
-          }
-          errors->push_back(file_data.path + ": not found");
-          break;
-        default:
-          UNREACHABLE();
-      }
-    }
-
-    per_process::dotenv_file.AssignNodeOptionsIfAvailable(&node_options);
-  }
 
 #if !defined(NODE_WITHOUT_NODE_OPTIONS)
   if (!(flags & ProcessInitializationFlags::kDisableNodeOptionsEnv)) {
@@ -915,6 +939,9 @@ static ExitCode InitializeNodeWithArgsInternal(
     if (exit_code != ExitCode::kNoFailure) return exit_code;
   }
 
+  const ExitCode exit_code = ProcessEnvFiles(argv, errors);
+  if (exit_code != ExitCode::kNoFailure) return exit_code;
+
   // Set the process.title immediately after processing argv if --title is set.
   if (!per_process::cli_options->title.empty())
     uv_set_process_title(per_process::cli_options->title.c_str());

src/node_dotenv.cc

@@ -11,54 +11,6 @@ using v8::NewStringType;
 using v8::Object;
 using v8::String;
 
-std::vector<Dotenv::env_file_data> Dotenv::GetDataFromArgs(
-    const std::vector<std::string>& args) {
-  const std::string_view optional_env_file_flag = "--env-file-if-exists";
-
-  const auto find_match = [](const std::string& arg) {
-    return arg == "--" || arg == "--env-file" ||
-           arg.starts_with("--env-file=") || arg == "--env-file-if-exists" ||
-           arg.starts_with("--env-file-if-exists=");
-  };
-
-  std::vector<Dotenv::env_file_data> env_files;
-  // This will be an iterator, pointing to args.end() if no matches are found
-  auto matched_arg = std::find_if(args.begin(), args.end(), find_match);
-
-  while (matched_arg != args.end()) {
-    if (*matched_arg == "--") {
-      return env_files;
-    }
-
-    auto equal_char_index = matched_arg->find('=');
-
-    if (equal_char_index != std::string::npos) {
-      // `--env-file=path`
-      auto flag = matched_arg->substr(0, equal_char_index);
-      auto file_path = matched_arg->substr(equal_char_index + 1);
-
-      struct env_file_data env_file_data = {
-          file_path, flag.starts_with(optional_env_file_flag)};
-      env_files.push_back(env_file_data);
-    } else {
-      // `--env-file path`
-      auto file_path = std::next(matched_arg);
-
-      if (file_path == args.end()) {
-        return env_files;
-      }
-
-      struct env_file_data env_file_data = {
-          *file_path, matched_arg->starts_with(optional_env_file_flag)};
-      env_files.push_back(env_file_data);
-    }
-
-    matched_arg = std::find_if(++matched_arg, args.end(), find_match);
-  }
-
-  return env_files;
-}
-
 void Dotenv::SetEnvironment(node::Environment* env) {
   auto isolate = env->isolate();
 
@@ -277,12 +229,9 @@ Dotenv::ParseResult Dotenv::ParsePath(const std::string_view path) {
   return ParseResult::Valid;
 }
 
-void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) const {
-  auto match = store_.find("NODE_OPTIONS");
-
-  if (match != store_.end()) {
-    *node_options = match->second;
-  }
+std::string Dotenv::GetNodeOptions() const {
+  auto it = store_.find("NODE_OPTIONS");
+  return (it != store_.end()) ? it->second : "";
 }
 
 }  // namespace node

src/node_dotenv.h

@@ -27,7 +27,7 @@ class Dotenv {
 
   void ParseContent(const std::string_view content);
   ParseResult ParsePath(const std::string_view path);
-  void AssignNodeOptionsIfAvailable(std::string* node_options) const;
+  std::string GetNodeOptions() const;
   void SetEnvironment(Environment* env);
   v8::Local<v8::Object> ToObject(Environment* env) const;
 

src/node_options.cc

@@ -638,11 +638,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
       "[has_env_file_string]", "", &EnvironmentOptions::has_env_file_string);
   AddOption("--env-file",
             "set environment variables from supplied file",
-            &EnvironmentOptions::env_file);
+            &EnvironmentOptions::env_files);
   Implies("--env-file", "[has_env_file_string]");
   AddOption("--env-file-if-exists",
             "set environment variables from supplied file",
-            &EnvironmentOptions::optional_env_file);
+            &EnvironmentOptions::optional_env_files);
   Implies("--env-file-if-exists", "[has_env_file_string]");
   AddOption("--test",
             "launch test runner on startup",

src/node_options.h

@@ -176,8 +176,8 @@ class EnvironmentOptions : public Options {
 #endif  // HAVE_INSPECTOR
   std::string redirect_warnings;
   std::string diagnostic_dir;
-  std::string env_file;
-  std::string optional_env_file;
+  std::vector<std::string> env_files;
+  std::vector<std::string> optional_env_files;
   bool has_env_file_string = false;
   bool test_runner = false;
   uint64_t test_runner_concurrency = 0;

test/parallel/test-dotenv-edge-cases.js

@@ -149,3 +149,39 @@ describe('.env supports edge cases', () => {
     assert.strictEqual(child.code, 0);
   });
 });
+
+// Ref: https://github.com/nodejs/node/pull/54913
+it('CLI edge cases', async () => {
+  const edgeCases = [
+    {
+      // If the flag is passed AFTER the script, ignore it
+      flags: [fixtures.path('empty.js'), '--env-file=nonexistent.env'],
+    },
+    {
+      // If the flag is passed AFTER '--', ignore it
+      flags: ['--eval=""', '--', '--env-file=nonexistent.env'],
+    },
+    {
+      // If the flag is passed AFTER an invalid argument, check the argument first
+      flags: ['--invalid-argument', '--env-file=nonexistent.env'],
+      error: 'bad option: --invalid-argument',
+    },
+    {
+      // If the flag is passed as an invalid argument, check the argument first
+      flags: ['--env-file-ABCD=nonexistent.env'],
+      error: 'bad option: --env-file-ABCD=nonexistent.env'
+    },
+  ];
+  for (const { flags, error } of edgeCases) {
+    const child = await common.spawnPromisified(process.execPath, flags);
+    if (error) {
+      assert.notStrictEqual(child.code, 0);
+      // Remove the leading '<path>: '
+      assert.strictEqual(child.stderr.substring(process.execPath.length + 2).trim(), error);
+    } else {
+      assert.strictEqual(child.code, 0);
+      assert.strictEqual(child.stderr, '');
+      assert.strictEqual(child.stdout, '');
+    }
+  }
+});

test/parallel/test-dotenv-without-node-options.js

@@ -0,0 +1,26 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('node:assert');
+const { test } = require('node:test');
+
+if (!process.config.variables.node_without_node_options) {
+  common.skip('Requires the lack NODE_OPTIONS support');
+}
+
+const relativePath = '../fixtures/dotenv/node-options.env';
+
+test('.env does not support NODE_OPTIONS', async() => {
+  const code = `assert.strictEqual(process.permission, undefined)`;
+  const child = await common.spawnPromisified(
+    process.execPath,
+    [ `--env-file=${relativePath}`, '--eval', code ],
+    { cwd: __dirname },
+  );
+  // NODE_NO_WARNINGS is set, so `stderr` should not contain
+  // "ExperimentalWarning: Permission is an experimental feature" message
+  // and thus be empty
+  assert.strictEqual(child.stdout, '');
+  assert.strictEqual(child.stderr, '');
+  assert.strictEqual(child.code, 0);
+});