src: merge env-file and env vars of NODE_OPTIONS

Author: anonrig

src/node.cc

@@ -848,19 +848,23 @@ static ExitCode InitializeNodeWithArgsInternal(
     std::string path = cwd + kPathSeparator + file_path.value();
     CHECK(!per_process::v8_initialized);
     per_process::dotenv_file.ParsePath(path);
-    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.
-    if (credentials::SafeGetenv("NODE_OPTIONS", &node_options) ||
-        !node_options.empty()) {
-      std::vector<std::string> env_argv =
-          ParseNodeOptionsEnvVar(node_options, errors);
+    std::string node_options;
+    std::vector<std::string> env_argv;
 
-      if (!errors->empty()) return ExitCode::kInvalidCommandLineArgument;
+    // This behaves like a noop if dotenv does not exist.
+    per_process::dotenv_file.AssignNodeOptionsIfAvailable(&env_argv, errors);
 
+    if (credentials::SafeGetenv("NODE_OPTIONS", &node_options)) {
+      ParseNodeOptionsEnvVar(node_options, &env_argv, errors);
+    }
+
+    if (!errors->empty()) return ExitCode::kInvalidCommandLineArgument;
+
+    if (!env_argv.empty()) {
       // [0] is expected to be the program name, fill it in from the real argv.
       env_argv.insert(env_argv.begin(), argv->at(0));
 

src/node_dotenv.cc

@@ -1,6 +1,7 @@
 #include "node_dotenv.h"
 #include "env-inl.h"
 #include "node_file.h"
+#include "node_options.h"
 #include "uv.h"
 
 namespace node {
@@ -101,12 +102,15 @@ void Dotenv::ParsePath(const std::string_view path) {
   }
 }
 
-void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) {
+void Dotenv::AssignNodeOptionsIfAvailable(std::vector<std::string>* env_argv,
+                                          std::vector<std::string>* errors) {
   auto match = store_.find("NODE_OPTIONS");
 
-  if (match != store_.end()) {
-    *node_options = match->second;
+  if (match == store_.end()) {
+    return;
   }
+
+  ParseNodeOptionsEnvVar(match->second, env_argv, errors);
 }
 
 void Dotenv::ParseLine(const std::string_view line) {

src/node_dotenv.h

@@ -7,6 +7,7 @@
 
 #include <map>
 #include <optional>
+#include <vector>
 
 namespace node {
 
@@ -20,7 +21,8 @@ class Dotenv {
   ~Dotenv() = default;
 
   void ParsePath(const std::string_view path);
-  void AssignNodeOptionsIfAvailable(std::string* node_options);
+  void AssignNodeOptionsIfAvailable(std::vector<std::string>* node_options,
+                                    std::vector<std::string>* errors);
   void SetEnvironment(Environment* env);
 
   static std::optional<std::string> GetPathFromArgs(

src/node_options.cc

@@ -1316,21 +1316,20 @@ void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options,
     env_options->redirect_warnings = opt_getter("NODE_REDIRECT_WARNINGS");
 }
 
-std::vector<std::string> ParseNodeOptionsEnvVar(
-    const std::string& node_options, std::vector<std::string>* errors) {
-  std::vector<std::string> env_argv;
-
+void ParseNodeOptionsEnvVar(const std::string& node_options,
+                            std::vector<std::string>* env_argv,
+                            std::vector<std::string>* errors) {
   bool is_in_string = false;
   bool will_start_new_arg = true;
-  for (std::string::size_type index = 0; index < node_options.size(); ++index) {
+  for (size_t index = 0; index < node_options.size(); ++index) {
     char c = node_options.at(index);
 
     // Backslashes escape the following character
     if (c == '\\' && is_in_string) {
       if (index + 1 == node_options.size()) {
         errors->push_back("invalid value for NODE_OPTIONS "
                           "(invalid escape)\n");
-        return env_argv;
+        break;
       } else {
         c = node_options.at(++index);
       }
@@ -1343,18 +1342,17 @@ std::vector<std::string> ParseNodeOptionsEnvVar(
     }
 
     if (will_start_new_arg) {
-      env_argv.emplace_back(std::string(1, c));
+      env_argv->emplace_back(std::string(1, c));
       will_start_new_arg = false;
     } else {
-      env_argv.back() += c;
+      env_argv->back() += c;
     }
   }
 
   if (is_in_string) {
     errors->push_back("invalid value for NODE_OPTIONS "
                       "(unterminated string)\n");
   }
-  return env_argv;
 }
 }  // namespace node
 

src/node_options.h

@@ -529,8 +529,9 @@ void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options);
 void HandleEnvOptions(std::shared_ptr<EnvironmentOptions> env_options,
                       std::function<std::string(const char*)> opt_getter);
 
-std::vector<std::string> ParseNodeOptionsEnvVar(
-    const std::string& node_options, std::vector<std::string>* errors);
+void ParseNodeOptionsEnvVar(const std::string& node_options,
+                            std::vector<std::string>* env_argv,
+                            std::vector<std::string>* errors);
 }  // namespace node
 
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

src/node_worker.cc

@@ -537,8 +537,8 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
     if (maybe_node_opts.ToLocal(&node_opts)) {
       std::string node_options(*String::Utf8Value(isolate, node_opts));
       std::vector<std::string> errors{};
-      std::vector<std::string> env_argv =
-          ParseNodeOptionsEnvVar(node_options, &errors);
+      std::vector<std::string> env_argv{};
+      ParseNodeOptionsEnvVar(node_options, &env_argv, &errors);
       // [0] is expected to be the program name, add dummy string.
       env_argv.insert(env_argv.begin(), "");
       std::vector<std::string> invalid_args{};

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

@@ -2,12 +2,18 @@
 
 const common = require('../common');
 const assert = require('node:assert');
+const path = require('node:path');
+const fs = require('node:fs');
 const { describe, it } = require('node:test');
 
 if (process.config.variables.node_without_node_options) {
   common.skip('missing NODE_OPTIONS support');
 }
 
+const tmpdir = require('../common/tmpdir');
+const testDir = tmpdir.path;
+tmpdir.refresh();
+
 const relativePath = '../fixtures/dotenv/node-options.env';
 
 describe('.env supports NODE_OPTIONS', () => {
@@ -70,4 +76,23 @@ describe('.env supports NODE_OPTIONS', () => {
     assert.strictEqual(child.code, 0);
   });
 
+  it('should merge env variables with .env file', async () => {
+    const filePath = path.join(testDir, 'should-write.txt');
+    const code = `
+      const path = require('path');
+      require('fs').writeFileSync('${filePath}', 'hello', 'utf-8')
+    `.trim();
+    const child = await common.spawnPromisified(
+      process.execPath,
+      [ `--env-file=${relativePath}`, '--eval', code ],
+      { cwd: __dirname, env: { ...process.env, NODE_OPTIONS: '--allow-fs-write=*' } },
+    );
+    assert.strictEqual(child.stderr, '');
+    assert.strictEqual(child.stdout, '');
+    assert.strictEqual(child.code, 0);
+    assert(fs.existsSync(filePath));
+    fs.unlinkSync(filePath);
+    assert(!fs.existsSync(filePath));
+  });
+
 });