From 950cb477aa68b9f006195141795113745a8db216 Mon Sep 17 00:00:00 2001 From: RedYetiDev <38299977+RedYetiDev@users.noreply.github.com> Date: Thu, 12 Sep 2024 18:29:41 -0400 Subject: [PATCH] src: parse dotenv with the rest of the options --- src/node.cc | 83 ++++++++++++------- src/node_dotenv.cc | 57 +------------ src/node_dotenv.h | 2 +- src/node_options.cc | 4 +- src/node_options.h | 4 +- test/parallel/test-dotenv-edge-cases.js | 36 ++++++++ .../test-dotenv-without-node-options.js | 26 ++++++ 7 files changed, 125 insertions(+), 87 deletions(-) create mode 100644 test/parallel/test-dotenv-without-node-options.js diff --git a/src/node.cc b/src/node.cc index dd64e52b02dc97..10d6032ada7108 100644 --- a/src/node.cc +++ b/src/node.cc @@ -800,6 +800,58 @@ 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; + + // 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& 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 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()); diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index f594df875d7a0c..446176f2f83a9f 100644 --- 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::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; - } - - 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 index d508b13fc5db74..2351c84d0e0da0 100644 --- a/src/node_dotenv.h +++ b/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 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..303fd7780aec34 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/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 ': ' + 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..c8e65011708e0b --- /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); +});