src: parse dotenv with the rest of the options

nodejs · Nov 1, 2024 · d0b1368 · d0b1368
1 parent 9f9069d
commit d0b1368
Show file tree

Hide file tree

Showing 8 changed files with 181 additions and 103 deletions.
diff --git a/src/node.cc b/src/node.cc
@@ -800,6 +800,49 @@ int ProcessGlobalArgs(std::vector<std::string>* args,
 
 static std::atomic_bool init_called{false};
 
+static ExitCode ProcessEnvFiles(std::vector<std::string>* errors) {
+  std::vector<DetailedOption>& env_files =
+      per_process::cli_options->per_isolate->per_env->env_files;
+  if (env_files.empty()) return ExitCode::kNoFailure;
+
+  CHECK(!per_process::v8_initialized);
+
+  for (const auto& env_file : env_files) {
+    switch (per_process::dotenv_file.ParsePath(env_file.value)) {
+      case Dotenv::ParseResult::Valid:
+        break;
+      case Dotenv::ParseResult::InvalidContent:
+        errors->emplace_back(env_file.value + ": invalid format");
+        break;
+      case Dotenv::ParseResult::FileError:
+        if (env_file.flag == "--env-file-if-exists") {
+          fprintf(stderr,
+                  "%s not found. Continuing without it.\n",
+                  env_file.value.c_str());
+        } else {
+          errors->emplace_back(env_file.value + ": not found");
+        }
+        break;
+      default:
+        UNREACHABLE();
+    }
+  }
+
+#if !defined(NODE_WITHOUT_NODE_OPTIONS)
+  // Parse and process Node.js options from the environment
+  std::vector<std::string> env_argv =
+      ParseNodeOptionsEnvVar(per_process::dotenv_file.GetNodeOptions(), errors);
+  env_argv.insert(env_argv.begin(), per_process::cli_options->cmdline.at(0));
+
+  // Process global arguments
+  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,35 +894,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)) {
     // NODE_OPTIONS environment variable is preferred over the file one.
@@ -915,6 +929,9 @@ static ExitCode InitializeNodeWithArgsInternal(
     if (exit_code != ExitCode::kNoFailure) return exit_code;
   }
 
+  const ExitCode exit_code = ProcessEnvFiles(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());

diff --git a/src/node_dotenv.cc b/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
diff --git a/src/node_dotenv.h b/src/node_dotenv.h
@@ -27,13 +27,10 @@ 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;
 
-  static std::vector<env_file_data> GetDataFromArgs(
-      const std::vector<std::string>& args);
-
  private:
   std::map<std::string, std::string> store_;
 };

diff --git a/src/node_options-inl.h b/src/node_options-inl.h
@@ -93,6 +93,23 @@ void OptionsParser<Options>::AddOption(
   });
 }
 
+template <typename Options>
+void OptionsParser<Options>::AddOption(
+    const char* name,
+    const char* help_text,
+    std::vector<DetailedOption> Options::*field,
+    OptionEnvvarSettings env_setting) {
+  options_.emplace(
+      name,
+      OptionInfo{
+          kDetailedStringList,
+          std::make_shared<
+              SimpleOptionField<std::vector<DetailedOption>>>(
+              field),
+          env_setting,
+          help_text});
+}
+
 template <typename Options>
 void OptionsParser<Options>::AddOption(const char* name,
                                        const char* help_text,
@@ -466,6 +483,10 @@ void OptionsParser<Options>::Parse(
         Lookup<std::vector<std::string>>(info.field, options)
             ->emplace_back(std::move(value));
         break;
+      case kDetailedStringList:
+        Lookup<std::vector<DetailedOption>>(info.field, options)
+            ->emplace_back(DetailedOption{value, name});
+        break;
       case kHostPort:
         Lookup<HostPort>(info.field, options)
             ->Update(SplitHostPort(value, errors));

diff --git a/src/node_options.cc b/src/node_options.cc
@@ -643,11 +643,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::env_files);
   Implies("--env-file-if-exists", "[has_env_file_string]");
   AddOption("--test",
             "launch test runner on startup",
@@ -1341,6 +1341,39 @@ void GetCLIOptionsValues(const FunctionCallbackInfo<Value>& args) {
           return;
         }
         break;
+      case kDetailedStringList: {
+        const std::vector<DetailedOption>& detailed_options =
+            *_ppop_instance.Lookup<std::vector<DetailedOption>>(
+                field, opts);
+        v8::Local<v8::Array> value_arr =
+            v8::Array::New(isolate, detailed_options.size());
+        for (size_t i = 0; i < detailed_options.size(); ++i) {
+          // Create a new V8 object for each DetailedOption
+          v8::Local<v8::Object> option_object = v8::Object::New(isolate);
+
+          option_object
+              ->Set(isolate->GetCurrentContext(),
+                    v8::String::NewFromUtf8(isolate, "flag").ToLocalChecked(),
+                    v8::String::NewFromUtf8(isolate,
+                                            detailed_options[i].flag.c_str())
+                        .ToLocalChecked())
+              .Check();
+
+          option_object
+              ->Set(isolate->GetCurrentContext(),
+                    v8::String::NewFromUtf8(isolate, "value").ToLocalChecked(),
+                    v8::String::NewFromUtf8(isolate,
+                                            detailed_options[i].value.c_str())
+                        .ToLocalChecked())
+              .Check();
+
+          // Add the object to the array at the current index
+          value_arr->Set(isolate->GetCurrentContext(), i, option_object)
+              .Check();
+        }
+        value = value_arr;
+        break;
+      }
       case kHostPort: {
         const HostPort& host_port =
             *_ppop_instance.Lookup<HostPort>(field, opts);

diff --git a/src/node_options.h b/src/node_options.h
@@ -44,6 +44,15 @@ class HostPort {
   uint16_t port_;
 };
 
+class DetailedOption {
+ public:
+  DetailedOption(const std::string& value, std::string flag)
+      : value(value), flag(flag) {}
+
+  std::string value;
+  std::string flag;
+};
+
 class Options {
  public:
   virtual void CheckOptions(std::vector<std::string>* errors,
@@ -177,8 +186,7 @@ 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<DetailedOption> env_files;
   bool has_env_file_string = false;
   bool test_runner = false;
   uint64_t test_runner_concurrency = 0;
@@ -380,6 +388,7 @@ enum OptionType {
   kString,
   kHostPort,
   kStringList,
+  kDetailedStringList,
 };
 
 template <typename Options>
@@ -416,6 +425,10 @@ class OptionsParser {
                  const char* help_text,
                  std::vector<std::string> Options::*field,
                  OptionEnvvarSettings env_setting = kDisallowedInEnvvar);
+  void AddOption(const char* name,
+                 const char* help_text,
+                 std::vector<DetailedOption> Options::*field,
+                 OptionEnvvarSettings env_setting = kDisallowedInEnvvar);
   void AddOption(const char* name,
                  const char* help_text,
                  HostPort Options::*field,

diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js
@@ -135,17 +135,39 @@ describe('.env supports edge cases', () => {
     assert.strictEqual(child.code, 0);
   });
 
-  it('should handle when --env-file is passed along with --', async () => {
-    const child = await common.spawnPromisified(
-      process.execPath,
-      [
-        '--eval', `require('assert').strictEqual(process.env.BASIC, undefined);`,
-        '--', '--env-file', validEnvFilePath,
-      ],
-      { cwd: __dirname },
-    );
-    assert.strictEqual(child.stdout, '');
-    assert.strictEqual(child.stderr, '');
-    assert.strictEqual(child.code, 0);
+  // Ref: https://github.com/nodejs/node/pull/54913
+  it('should handle 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, '');
+      }
+    }
   });
 });
diff --git a/test/parallel/test-dotenv-without-node-options.js b/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 of 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);
+});