diff --git a/src/node.cc b/src/node.cc index dd64e52b02dc97..0514a39fb4b741 100644 --- a/src/node.cc +++ b/src/node.cc @@ -800,6 +800,34 @@ int ProcessGlobalArgs(std::vector* args, static std::atomic_bool init_called{false}; +static ExitCode ProcessEnvFiles(std::vector* argv, + std::vector* errors) { + const auto& cli_options = per_process::cli_options->per_isolate->per_env; + // Early exit if both env_files and optional_env_files are empty + if (cli_options->env_files.empty() && + cli_options->optional_env_files.empty()) { + return ExitCode::kNoFailure; + } + + CHECK(!per_process::v8_initialized); + per_process::dotenv_file.ProcessEnvFilesFromCLI( + cli_options, per_process::cli_options->cmdline, errors); + +#if !defined(NODE_WITHOUT_NODE_OPTIONS) + // Parse and process Node.js options from the environment + std::vector env_argv = + ParseNodeOptionsEnvVar(per_process::dotenv_file.GetNodeOptions(), errors); + env_argv.insert(env_argv.begin(), argv->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,34 +879,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 +915,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()); diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index f594df875d7a0c..92f4e68822a217 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -1,5 +1,7 @@ #include "node_dotenv.h" #include + +#include "node_options-inl.h" #include "env-inl.h" #include "node_file.h" #include "uv.h" @@ -11,52 +13,46 @@ using v8::NewStringType; using v8::Object; using v8::String; -std::vector Dotenv::GetDataFromArgs( - const std::vector& 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 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; +void Dotenv::ProcessEnvFilesFromCLI( + const std::shared_ptr cli_options, + std::vector cmdline, + std::vector* errors) { + // Helper function to process environment files + auto process_file = [&](const std::string& file_path, bool is_optional) { + switch (this->ParsePath(file_path)) { + case Dotenv::ParseResult::Valid: + break; + case Dotenv::ParseResult::InvalidContent: + errors->emplace_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->emplace_back(file_path + ": not found"); + } + break; + default: + UNREACHABLE(); } + }; - 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); + // Process env files and optional env files based on the command-line + // arguments + // TODO(RedYetiDev): Find a way to get the index of each argument, in order to + // create a more robust method of determining the order of env files. + int env_file_idx = 0; + int optional_env_file_idx = 0; + for (const auto& arg : cmdline) { + if (arg.starts_with("--env-file-if-exists")) { + process_file(cli_options->optional_env_files[optional_env_file_idx++], + true); + } else if (arg.starts_with("--env-file")) { + process_file(cli_options->env_files[env_file_idx++], false); } - - matched_arg = std::find_if(++matched_arg, args.end(), find_match); } - - return env_files; } void Dotenv::SetEnvironment(node::Environment* env) { @@ -277,12 +273,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 index d508b13fc5db74..d6af782fee9c88 100644 --- a/src/node_dotenv.h +++ b/src/node_dotenv.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include "node_options-inl.h" #include "util-inl.h" #include "v8.h" @@ -25,9 +26,13 @@ class Dotenv { Dotenv& operator=(const Dotenv& d) = delete; ~Dotenv() = default; + void ProcessEnvFilesFromCLI( + const std::shared_ptr cli_options, + std::vector cmdline, + std::vector* errors); 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 ToObject(Environment* env) const; diff --git a/src/node_options.cc b/src/node_options.cc index 2ca6652538b30b..6493e267384dbf 100644 --- a/src/node_options.cc +++ b/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", diff --git a/src/node_options.h b/src/node_options.h index b521ca76185512..cc7fe49c07d02e 100644 --- a/src/node_options.h +++ b/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 env_files; + std::vector optional_env_files; bool has_env_file_string = false; bool test_runner = false; uint64_t test_runner_concurrency = 0; diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 30fd9d20618ba0..67f10973b9d892 100644 --- 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 ': ' + 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 new file mode 100644 index 00000000000000..474950b54fb94c --- /dev/null +++ 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); +});