From 0b065db868c40aba8cdad80898b749ca973251dd Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 23 Jul 2023 00:05:25 -0400 Subject: [PATCH 01/15] src: add built-in `.env` file support --- doc/api/cli.md | 26 +++++ node.gyp | 2 + src/node.cc | 7 ++ src/node_dotenv.cc | 123 ++++++++++++++++++++++++ src/node_dotenv.h | 20 ++++ src/node_options.cc | 6 ++ src/node_options.h | 2 + test/fixtures/dotenv/valid.env | 38 ++++++++ test/parallel/test-dotenv-permission.js | 18 ++++ test/parallel/test-dotenv.js | 70 ++++++++++++++ 10 files changed, 312 insertions(+) create mode 100644 src/node_dotenv.cc create mode 100644 src/node_dotenv.h create mode 100644 test/fixtures/dotenv/valid.env create mode 100644 test/parallel/test-dotenv-permission.js create mode 100644 test/parallel/test-dotenv.js diff --git a/doc/api/cli.md b/doc/api/cli.md index b2d860ba500166..6ddb587e97798b 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -984,6 +984,32 @@ surface on other platforms, but the performance impact may be severe. This flag is inherited from V8 and is subject to change upstream. It may disappear in a non-semver-major release. +### `--env-file=config` + +> Stability: 1 - Experimental + + + +Loads environment variables from a file relative to the current directory. + +`NODE_OPTIONS` environment variable is not supported at the moment. + +The format of the file should be one line per key-value pair of environment +variable name and value separated by `=`: + +```text +PORT=3000 +``` + +Any text after a `#` is treated as a comment: + +```text +# This is a comment +PORT=3000 # This is also a comment +``` + ### `--max-http-header-size=size` -Loads environment variables from a file relative to the current directory. - -`NODE_OPTIONS` environment variable is not supported at the moment. +Loads environment variables from a file relative to the current directory, making +them available to applications on `process.env`. The [environment variables which +configure Node.js][environment_variables], such as `NODE_OPTIONS`, are parsed and applied. +If the same variable is defined in the environment and in the file, the value from +the environment takes precedence. The format of the file should be one line per key-value pair of environment variable name and value separated by `=`: @@ -2673,6 +2675,7 @@ done [debugger]: debugger.md [debugging security implications]: https://nodejs.org/en/docs/guides/debugging-getting-started/#security-implications [emit_warning]: process.md#processemitwarningwarning-options +[environment_variables]: #environment-variables [filtering tests by name]: test.md#filtering-tests-by-name [jitless]: https://v8.dev/blog/jitless [libuv threadpool documentation]: https://docs.libuv.org/en/latest/threadpool.html diff --git a/src/env.cc b/src/env.cc index 6b0aba44a36eba..7e3d3aca2d5f96 100644 --- a/src/env.cc +++ b/src/env.cc @@ -683,7 +683,7 @@ void Environment::TryLoadAddon( } } -std::string Environment::GetCwd() { +std::string Environment::GetCwd(const std::string& exec_path) { char cwd[PATH_MAX_BYTES]; size_t size = PATH_MAX_BYTES; const int err = uv_cwd(cwd, &size); @@ -695,7 +695,6 @@ std::string Environment::GetCwd() { // This can fail if the cwd is deleted. In that case, fall back to // exec_path. - const std::string& exec_path = exec_path_; return exec_path.substr(0, exec_path.find_last_of(kPathSeparator)); } @@ -729,7 +728,7 @@ std::unique_ptr Environment::release_managed_buffer( return bs; } -std::string GetExecPath(const std::vector& argv) { +std::string Environment::GetExecPath(const std::vector& argv) { char exec_path_buf[2 * PATH_MAX]; size_t exec_path_len = sizeof(exec_path_buf); std::string exec_path; @@ -771,7 +770,7 @@ Environment::Environment(IsolateData* isolate_data, timer_base_(uv_now(isolate_data->event_loop())), exec_argv_(exec_args), argv_(args), - exec_path_(GetExecPath(args)), + exec_path_(Environment::GetExecPath(args)), exit_info_( isolate_, kExitInfoFieldCount, MAYBE_FIELD_PTR(env_info, exit_info)), should_abort_on_uncaught_toggle_( @@ -1921,7 +1920,7 @@ size_t Environment::NearHeapLimitCallback(void* data, std::string dir = env->options()->diagnostic_dir; if (dir.empty()) { - dir = env->GetCwd(); + dir = Environment::GetCwd(env->exec_path_); } DiagnosticFilename name(env, "Heap", "heapsnapshot"); std::string filename = dir + kPathSeparator + (*name); diff --git a/src/env.h b/src/env.h index 0fb0550a973a16..c02fc6bd62dd78 100644 --- a/src/env.h +++ b/src/env.h @@ -588,6 +588,9 @@ class Environment : public MemoryRetainer { SET_MEMORY_INFO_NAME(Environment) + static std::string GetExecPath(const std::vector& argv); + static std::string GetCwd(const std::string& exec_path); + inline size_t SelfSize() const override; bool IsRootNode() const override { return true; } void MemoryInfo(MemoryTracker* tracker) const override; @@ -604,8 +607,6 @@ class Environment : public MemoryRetainer { // Should be called before InitializeInspector() void InitializeDiagnostics(); - std::string GetCwd(); - #if HAVE_INSPECTOR // If the environment is created for a worker, pass parent_handle and // the ownership if transferred into the Environment. diff --git a/src/heap_utils.cc b/src/heap_utils.cc index b423d97345e956..e385955a5d5fce 100644 --- a/src/heap_utils.cc +++ b/src/heap_utils.cc @@ -456,7 +456,9 @@ void TriggerHeapSnapshot(const FunctionCallbackInfo& args) { if (filename_v->IsUndefined()) { DiagnosticFilename name(env, "Heap", "heapsnapshot"); THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemWrite, env->GetCwd()); + env, + permission::PermissionScope::kFileSystemWrite, + Environment::GetCwd(env->exec_path())); if (WriteSnapshot(env, *name, options).IsNothing()) return; if (String::NewFromUtf8(isolate, *name).ToLocal(&filename_v)) { args.GetReturnValue().Set(filename_v); diff --git a/src/inspector_profiler.cc b/src/inspector_profiler.cc index 56e588ceefa17a..307049bf7c83c5 100644 --- a/src/inspector_profiler.cc +++ b/src/inspector_profiler.cc @@ -431,7 +431,8 @@ void StartProfilers(Environment* env) { if (env->options()->cpu_prof) { const std::string& dir = env->options()->cpu_prof_dir; env->set_cpu_prof_interval(env->options()->cpu_prof_interval); - env->set_cpu_prof_dir(dir.empty() ? env->GetCwd() : dir); + env->set_cpu_prof_dir(dir.empty() ? Environment::GetCwd(env->exec_path()) + : dir); if (env->options()->cpu_prof_name.empty()) { DiagnosticFilename filename(env, "CPU", "cpuprofile"); env->set_cpu_prof_name(*filename); @@ -446,7 +447,8 @@ void StartProfilers(Environment* env) { if (env->options()->heap_prof) { const std::string& dir = env->options()->heap_prof_dir; env->set_heap_prof_interval(env->options()->heap_prof_interval); - env->set_heap_prof_dir(dir.empty() ? env->GetCwd() : dir); + env->set_heap_prof_dir(dir.empty() ? Environment::GetCwd(env->exec_path()) + : dir); if (env->options()->heap_prof_name.empty()) { DiagnosticFilename filename(env, "Heap", "heapprofile"); env->set_heap_prof_name(*filename); diff --git a/src/node.cc b/src/node.cc index 5a05078e9e6577..58abf85db8c2dd 100644 --- a/src/node.cc +++ b/src/node.cc @@ -141,6 +141,10 @@ using v8::Value; namespace per_process { +// node_dotenv.h +// Instance is used to store environment variables including NODE_OPTIONS. +node::Dotenv dotenv_file = Dotenv(); + // node_revert.h // Bit flag used to track security reverts. unsigned int reverted_cve = 0; @@ -307,9 +311,7 @@ MaybeLocal StartExecution(Environment* env, StartExecutionCallback cb) { #endif if (env->options()->has_env_file_string) { - std::string path = - env->GetCwd() + kPathSeparator + env->options()->env_file; - node::dotenv::LoadFromFile(env, path); + per_process::dotenv_file.set_env(env); } // TODO(joyeecheung): move these conditions into JS land and let the @@ -838,11 +840,22 @@ static ExitCode InitializeNodeWithArgsInternal( HandleEnvOptions(per_process::cli_options->per_isolate->per_env); + std::string node_options; + auto file_path = node::Dotenv::GetPathFromArgs(*argv); + + if (file_path.has_value()) { + auto cwd = Environment::GetCwd(Environment::GetExecPath(*argv)); + std::string path = cwd + kPathSeparator + file_path.value(); + CHECK(!per_process::v8_initialized); + per_process::dotenv_file.parse(path); + per_process::dotenv_file.assignNodeOptionsIfAvailable(&node_options); + } + #if !defined(NODE_WITHOUT_NODE_OPTIONS) if (!(flags & ProcessInitializationFlags::kDisableNodeOptionsEnv)) { - std::string node_options; - - if (credentials::SafeGetenv("NODE_OPTIONS", &node_options)) { + // NODE_OPTIONS environment variable is preferred over the file one. + if (credentials::SafeGetenv("NODE_OPTIONS", &node_options) || + !node_options.empty()) { std::vector env_argv = ParseNodeOptionsEnvVar(node_options, errors); diff --git a/src/node_credentials.cc b/src/node_credentials.cc index 52abaab7a635db..c1f7a4f2acbdf6 100644 --- a/src/node_credentials.cc +++ b/src/node_credentials.cc @@ -123,7 +123,6 @@ bool SafeGetenv(const char* key, } fail: - text->clear(); return false; } diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index e4c8e747acb350..350dc9258c7d4d 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -1,19 +1,112 @@ #include "node_dotenv.h" #include "env-inl.h" #include "node_file.h" -#include "permission/permission.h" #include "uv.h" namespace node { -using v8::Isolate; using v8::NewStringType; +using v8::String; + +std::optional Dotenv::GetPathFromArgs( + const std::vector& args) { + std::string flag = "--env-file"; + auto path = + std::find_if(args.begin(), args.end(), [&flag](const std::string& arg) { + return strncmp(arg.c_str(), flag.c_str(), flag.size()) == 0; + }); + + if (path == args.end()) { + return std::nullopt; + } + + auto equal_char = path->find('='); + + if (equal_char != std::string::npos) { + return path->substr(equal_char + 1); + } + + auto next_arg = std::next(path); + + if (next_arg == args.end()) { + return std::nullopt; + } + + return *next_arg; +} + +void Dotenv::set_env(node::Environment* env) { + if (store.empty()) { + return; + } + + auto isolate = env->isolate(); + + for (const auto& entry : store) { + auto key = entry.first; + auto value = entry.second; + env->env_vars()->Set( + isolate, + v8::String::NewFromUtf8( + isolate, key.data(), NewStringType::kNormal, key.size()) + .ToLocalChecked(), + v8::String::NewFromUtf8( + isolate, value.data(), NewStringType::kNormal, value.size()) + .ToLocalChecked()); + } +} + +void Dotenv::parse(const std::string_view path) { + uv_fs_t req; + auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); + + uv_file file = uv_fs_open(nullptr, &req, path.data(), 0, 438, nullptr); + if (req.result < 0) { + // req will be cleaned up by scope leave. + return; + } + uv_fs_req_cleanup(&req); -namespace dotenv { + auto defer_close = OnScopeLeave([file]() { + uv_fs_t close_req; + CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr)); + uv_fs_req_cleanup(&close_req); + }); + + std::string result{}; + char buffer[8192]; + uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); + + while (true) { + auto r = uv_fs_read(nullptr, &req, file, &buf, 1, -1, nullptr); + if (req.result < 0) { + // req will be cleaned up by scope leave. + return; + } + uv_fs_req_cleanup(&req); + if (r <= 0) { + break; + } + result.append(buf.base, r); + } + + using std::string_view_literals::operator""sv; + auto lines = SplitString(result, "\n"sv); -void ParseLine(const std::string_view line, - Isolate* isolate, - std::shared_ptr store) { + for (const auto& line : lines) { + ParseLine(line); + } +} + +void Dotenv::assignNodeOptionsIfAvailable(std::string* node_options) { + auto match = store.find("NODE_OPTIONS"); + + if (match != store.end()) { + *node_options = match->second; + } +} + +void Dotenv::ParseLine(const std::string_view line) { auto equal_index = line.find('='); if (equal_index == std::string_view::npos) { @@ -62,62 +155,7 @@ void ParseLine(const std::string_view line, value.erase(value.size() - 1); } - store->Set(isolate, - v8::String::NewFromUtf8( - isolate, key.data(), NewStringType::kNormal, key.size()) - .ToLocalChecked(), - v8::String::NewFromUtf8( - isolate, value.data(), NewStringType::kNormal, value.size()) - .ToLocalChecked()); -} - -void LoadFromFile(Environment* env, const std::string_view path) { - Isolate* isolate = env->isolate(); - auto store = env->env_vars(); - - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path); - - uv_fs_t req; - auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); - - uv_file file = uv_fs_open(nullptr, &req, path.data(), 0, 438, nullptr); - if (req.result < 0) { - // req will be cleaned up by scope leave. - return; - } - uv_fs_req_cleanup(&req); - - auto defer_close = OnScopeLeave([file]() { - uv_fs_t close_req; - CHECK_EQ(0, uv_fs_close(nullptr, &close_req, file, nullptr)); - uv_fs_req_cleanup(&close_req); - }); - - std::string result{}; - char buffer[8192]; - uv_buf_t buf = uv_buf_init(buffer, sizeof(buffer)); - - while (true) { - auto r = uv_fs_read(nullptr, &req, file, &buf, 1, -1, nullptr); - if (req.result < 0) { - // req will be cleaned up by scope leave. - return; - } - uv_fs_req_cleanup(&req); - if (r <= 0) { - break; - } - result.append(buf.base, r); - } - - using std::string_view_literals::operator""sv; - - for (const auto& line : SplitString(result, "\n"sv)) { - ParseLine(line, isolate, store); - } + store.emplace(key, value); } -} // namespace dotenv - } // namespace node diff --git a/src/node_dotenv.h b/src/node_dotenv.h index 8b5447c70c52de..f3d222acd19438 100644 --- a/src/node_dotenv.h +++ b/src/node_dotenv.h @@ -5,13 +5,30 @@ #include "util-inl.h" -namespace node { - -namespace dotenv { +#include +#include -void LoadFromFile(Environment* env, const std::string_view path); +namespace node { -} // namespace dotenv +class Dotenv { + public: + Dotenv() = default; + Dotenv(const Dotenv& d) = default; + Dotenv(Dotenv&& d) noexcept = default; + Dotenv& operator=(Dotenv&& d) noexcept = default; + Dotenv& operator=(const Dotenv& d) = default; + ~Dotenv() = default; + + void parse(const std::string_view path); + void assignNodeOptionsIfAvailable(std::string* node_options); + void set_env(Environment* env); + static std::optional GetPathFromArgs( + const std::vector& args); + + private: + void ParseLine(const std::string_view line); + std::map store{}; +}; } // namespace node diff --git a/src/node_report.cc b/src/node_report.cc index dcaa6922070b92..76b5d4448267ff 100644 --- a/src/node_report.cc +++ b/src/node_report.cc @@ -878,7 +878,7 @@ std::string TriggerNodeReport(Isolate* isolate, THROW_IF_INSUFFICIENT_PERMISSIONS( env, permission::PermissionScope::kFileSystemWrite, - std::string_view(env->GetCwd()), + std::string_view(Environment::GetCwd(env->exec_path())), filename); } } diff --git a/test/fixtures/dotenv/node-options.env b/test/fixtures/dotenv/node-options.env new file mode 100644 index 00000000000000..3dc9b529947c30 --- /dev/null +++ b/test/fixtures/dotenv/node-options.env @@ -0,0 +1,5 @@ +CUSTOM_VARIABLE=hello-world +NODE_NO_WARNINGS=1 +NODE_OPTIONS="--experimental-permission --allow-fs-read=*" +TZ=Pacific/Honolulu +UV_THREADPOOL_SIZE=5 diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js new file mode 100644 index 00000000000000..39981be5dfc1df --- /dev/null +++ b/test/parallel/test-dotenv-node-options.js @@ -0,0 +1,59 @@ +'use strict'; + +require('../common'); +const assert = require('node:assert'); +const { spawnPromisified } = require('../common'); +const { describe, it } = require('node:test'); + +const envFilePath = '../fixtures/dotenv/node-options.env'; + +describe('.env supports NODE_OPTIONS', () => { + + it('should have access to permission scope', async () => { + const code = '"require(\'assert\').assert(process.permission.has(\'fs.read\'))\'"'; + const child = await spawnPromisified(process.execPath, + [ `--env-file=${envFilePath}`, '--eval', code ], { + cwd: __dirname + }); + // Test NODE_NO_WARNINGS environment variable + // `stderr` should not contain "ExperimentalWarning: Permission is an experimental feature" message + assert.strictEqual(child.stderr.toString(), ''); + assert.strictEqual(child.code, 0); + }); + + it('validate only read permissions are enabled', async () => { + const code = ` + "require('fs').writeFileSync(require('path').join(__dirname, 'should-not-write.txt'), 'hello', 'utf-8')" + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${envFilePath}`, '--eval', code ], { + cwd: __dirname, + encoding: 'utf-8', + }); + assert.strictEqual(child.code, 0); + }); + + it('TZ environment variable', async () => { + const code = ` + "require('assert').assert(new Date().toString().includes('Hawaii'))" + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${envFilePath}`, '--eval', code ], { + cwd: __dirname, + encoding: 'utf-8', + }); + assert.strictEqual(child.code, 0); + }); + + it('should update UV_THREADPOOL_SIZE', async () => { + const code = ` + "require('assert').assert.strictEqual(process.env.UV_THREADPOOL_SIZE, 5)" + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${envFilePath}`, '--eval', code ], { + cwd: __dirname, + encoding: 'utf-8', + }); + assert.strictEqual(child.code, 0); + }); +}); diff --git a/test/parallel/test-dotenv-permission.js b/test/parallel/test-dotenv-permission.js deleted file mode 100644 index 4a0930ca226410..00000000000000 --- a/test/parallel/test-dotenv-permission.js +++ /dev/null @@ -1,18 +0,0 @@ -'use strict'; - -require('../common'); -const assert = require('node:assert'); -const { spawnSync } = require('node:child_process'); -const path = require('node:path'); - -{ - // Check if `experimental-permission` flag applies to `--env-file` flag as well. - const response = spawnSync( - process.execPath, - ['--env-file', path.join(__dirname, '../fixtures/dotenv/valid.env'), '--experimental-permission'], - ); - - const stderr = response.stderr.toString(); - assert(stderr.includes('ERR_ACCESS_DENIED')); - assert(stderr.includes('FileSystemRead')); -} From c0a6c51b0b2416134368464869db67061a500b66 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sat, 12 Aug 2023 17:01:12 -0400 Subject: [PATCH 03/15] fixup! feat: add `NODE_OPTIONS` support to `.env` --- src/node_dotenv.cc | 13 ++-- test/parallel/test-dotenv-node-options.js | 59 --------------- test/parallel/test-dotenv-node-options.mjs | 88 ++++++++++++++++++++++ 3 files changed, 96 insertions(+), 64 deletions(-) delete mode 100644 test/parallel/test-dotenv-node-options.js create mode 100644 test/parallel/test-dotenv-node-options.mjs diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index 350dc9258c7d4d..a2d2f08500d37d 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -10,13 +10,16 @@ using v8::String; std::optional Dotenv::GetPathFromArgs( const std::vector& args) { - std::string flag = "--env-file"; + std::string_view flag = "--env-file"; + // Match the last `--env-file` + // This is required to imitate the default behavior of Node.js CLI argument + // matching. auto path = - std::find_if(args.begin(), args.end(), [&flag](const std::string& arg) { - return strncmp(arg.c_str(), flag.c_str(), flag.size()) == 0; + std::find_if(args.rbegin(), args.rend(), [&flag](const std::string& arg) { + return strncmp(arg.c_str(), flag.data(), flag.size()) == 0; }); - if (path == args.end()) { + if (path == args.rend()) { return std::nullopt; } @@ -28,7 +31,7 @@ std::optional Dotenv::GetPathFromArgs( auto next_arg = std::next(path); - if (next_arg == args.end()) { + if (next_arg == args.rend()) { return std::nullopt; } diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js deleted file mode 100644 index 39981be5dfc1df..00000000000000 --- a/test/parallel/test-dotenv-node-options.js +++ /dev/null @@ -1,59 +0,0 @@ -'use strict'; - -require('../common'); -const assert = require('node:assert'); -const { spawnPromisified } = require('../common'); -const { describe, it } = require('node:test'); - -const envFilePath = '../fixtures/dotenv/node-options.env'; - -describe('.env supports NODE_OPTIONS', () => { - - it('should have access to permission scope', async () => { - const code = '"require(\'assert\').assert(process.permission.has(\'fs.read\'))\'"'; - const child = await spawnPromisified(process.execPath, - [ `--env-file=${envFilePath}`, '--eval', code ], { - cwd: __dirname - }); - // Test NODE_NO_WARNINGS environment variable - // `stderr` should not contain "ExperimentalWarning: Permission is an experimental feature" message - assert.strictEqual(child.stderr.toString(), ''); - assert.strictEqual(child.code, 0); - }); - - it('validate only read permissions are enabled', async () => { - const code = ` - "require('fs').writeFileSync(require('path').join(__dirname, 'should-not-write.txt'), 'hello', 'utf-8')" - `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${envFilePath}`, '--eval', code ], { - cwd: __dirname, - encoding: 'utf-8', - }); - assert.strictEqual(child.code, 0); - }); - - it('TZ environment variable', async () => { - const code = ` - "require('assert').assert(new Date().toString().includes('Hawaii'))" - `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${envFilePath}`, '--eval', code ], { - cwd: __dirname, - encoding: 'utf-8', - }); - assert.strictEqual(child.code, 0); - }); - - it('should update UV_THREADPOOL_SIZE', async () => { - const code = ` - "require('assert').assert.strictEqual(process.env.UV_THREADPOOL_SIZE, 5)" - `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${envFilePath}`, '--eval', code ], { - cwd: __dirname, - encoding: 'utf-8', - }); - assert.strictEqual(child.code, 0); - }); -}); diff --git a/test/parallel/test-dotenv-node-options.mjs b/test/parallel/test-dotenv-node-options.mjs new file mode 100644 index 00000000000000..3b32301905c690 --- /dev/null +++ b/test/parallel/test-dotenv-node-options.mjs @@ -0,0 +1,88 @@ +import { spawnPromisified } from '../common/index.mjs'; +import assert from 'node:assert'; +import { describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +const __dirname = fileURLToPath(new URL('.', import.meta.url)); +const validEnvFilePath = '../fixtures/dotenv/valid.env'; +const relativePath = '../fixtures/dotenv/node-options.env'; + +describe('.env supports NODE_OPTIONS', () => { + + it('should have access to permission scope', async () => { + const code = ` + require('assert')(process.permission.has('fs.read')); + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname + }); + // Test NODE_NO_WARNINGS environment variable + // `stderr` should not contain "ExperimentalWarning: Permission is an experimental feature" message + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('validate only read permissions are enabled', async () => { + const code = ` + require('fs').writeFileSync(require('path').join(__dirname, 'should-not-write.txt'), 'hello', 'utf-8') + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname, + }); + assert.match(child.stderr, /Error: Access to this API has been restricted/); + assert.match(child.stderr, /code: 'ERR_ACCESS_DENIED'/); + assert.match(child.stderr, /permission: 'FileSystemWrite'/); + assert.strictEqual(child.code, 1); + }); + + it('TZ environment variable', async () => { + const code = ` + require('assert')(new Date().toString().includes('Hawaii')) + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('should update UV_THREADPOOL_SIZE', async () => { + const code = ` + require('assert').strictEqual(process.env.UV_THREADPOOL_SIZE, '5') + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('should use the last --env-file declaration', async () => { + const code = ` + require('assert').strictEqual(process.env.BASIC, 'basic'); + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('should handle unexisting .env file', async () => { + const code = ` + require('assert').strictEqual(1, 1) + `.trim(); + const child = await spawnPromisified(process.execPath, + [ '--env-file=.env', '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + +}); From 30e28d83f6b83c71731bee9fa109dad0990ad98d Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sat, 12 Aug 2023 18:29:16 -0400 Subject: [PATCH 04/15] fixup! feat: add `NODE_OPTIONS` support to `.env` --- doc/api/cli.md | 10 ++-- src/node_dotenv.cc | 2 +- test/parallel/test-dotenv-edge-cases.mjs | 36 ++++++++++++ test/parallel/test-dotenv-node-options.mjs | 66 ++++++++-------------- 4 files changed, 66 insertions(+), 48 deletions(-) create mode 100644 test/parallel/test-dotenv-edge-cases.mjs diff --git a/doc/api/cli.md b/doc/api/cli.md index f63b83df009be8..cba13d565dd1fd 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -992,11 +992,11 @@ disappear in a non-semver-major release. added: REPLACEME --> -Loads environment variables from a file relative to the current directory, making -them available to applications on `process.env`. The [environment variables which -configure Node.js][environment_variables], such as `NODE_OPTIONS`, are parsed and applied. -If the same variable is defined in the environment and in the file, the value from -the environment takes precedence. +Loads environment variables from a file relative to the current directory, +making them available to applications on `process.env`. The [environment +variables which configure Node.js][environment_variables], such as `NODE_OPTIONS`, +are parsed and applied. If the same variable is defined in the environment and +in the file, the value from the environment takes precedence. The format of the file should be one line per key-value pair of environment variable name and value separated by `=`: diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index a2d2f08500d37d..5b7a0963772164 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -29,7 +29,7 @@ std::optional Dotenv::GetPathFromArgs( return path->substr(equal_char + 1); } - auto next_arg = std::next(path); + auto next_arg = std::prev(path); if (next_arg == args.rend()) { return std::nullopt; diff --git a/test/parallel/test-dotenv-edge-cases.mjs b/test/parallel/test-dotenv-edge-cases.mjs new file mode 100644 index 00000000000000..364412e79fa064 --- /dev/null +++ b/test/parallel/test-dotenv-edge-cases.mjs @@ -0,0 +1,36 @@ +import { spawnPromisified } from '../common/index.mjs'; +import assert from 'node:assert'; +import { describe, it } from 'node:test'; +import { fileURLToPath } from 'node:url'; + +const __dirname = fileURLToPath(new URL('.', import.meta.url)); +const validEnvFilePath = '../fixtures/dotenv/valid.env'; +const relativePath = '../fixtures/dotenv/node-options.env'; + +describe('.env supports edge cases', () => { + + it('should use the last --env-file declaration', async () => { + const code = ` + require('assert').strictEqual(process.env.BASIC, 'basic'); + `.trim(); + const child = await spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('should handle unexisting .env file', async () => { + const code = ` + require('assert').strictEqual(1, 1) + `.trim(); + const child = await spawnPromisified(process.execPath, + [ '--env-file=.env', '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + +}); diff --git a/test/parallel/test-dotenv-node-options.mjs b/test/parallel/test-dotenv-node-options.mjs index 3b32301905c690..4d193eefef8401 100644 --- a/test/parallel/test-dotenv-node-options.mjs +++ b/test/parallel/test-dotenv-node-options.mjs @@ -1,10 +1,15 @@ -import { spawnPromisified } from '../common/index.mjs'; +import * as common from '../common/index.mjs'; import assert from 'node:assert'; import { describe, it } from 'node:test'; import { fileURLToPath } from 'node:url'; +if (!common.isMainThread) { + common.skip( + 'test-esm-resolve-type.mjs: process.chdir is not available in Workers' + ); +} + const __dirname = fileURLToPath(new URL('.', import.meta.url)); -const validEnvFilePath = '../fixtures/dotenv/valid.env'; const relativePath = '../fixtures/dotenv/node-options.env'; describe('.env supports NODE_OPTIONS', () => { @@ -13,12 +18,13 @@ describe('.env supports NODE_OPTIONS', () => { const code = ` require('assert')(process.permission.has('fs.read')); `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${relativePath}`, '--eval', code ], { - cwd: __dirname - }); + const child = await common.spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname + }); // Test NODE_NO_WARNINGS environment variable // `stderr` should not contain "ExperimentalWarning: Permission is an experimental feature" message + assert.strictEqual(child.stdout, ''); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); @@ -27,10 +33,10 @@ describe('.env supports NODE_OPTIONS', () => { const code = ` require('fs').writeFileSync(require('path').join(__dirname, 'should-not-write.txt'), 'hello', 'utf-8') `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${relativePath}`, '--eval', code ], { - cwd: __dirname, - }); + const child = await common.spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname, + }); assert.match(child.stderr, /Error: Access to this API has been restricted/); assert.match(child.stderr, /code: 'ERR_ACCESS_DENIED'/); assert.match(child.stderr, /permission: 'FileSystemWrite'/); @@ -41,10 +47,10 @@ describe('.env supports NODE_OPTIONS', () => { const code = ` require('assert')(new Date().toString().includes('Hawaii')) `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${relativePath}`, '--eval', code ], { - cwd: __dirname, - }); + const child = await common.spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname, + }); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); @@ -53,34 +59,10 @@ describe('.env supports NODE_OPTIONS', () => { const code = ` require('assert').strictEqual(process.env.UV_THREADPOOL_SIZE, '5') `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${relativePath}`, '--eval', code ], { - cwd: __dirname, - }); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); - }); - - it('should use the last --env-file declaration', async () => { - const code = ` - require('assert').strictEqual(process.env.BASIC, 'basic'); - `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${relativePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ], { - cwd: __dirname, - }); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); - }); - - it('should handle unexisting .env file', async () => { - const code = ` - require('assert').strictEqual(1, 1) - `.trim(); - const child = await spawnPromisified(process.execPath, - [ '--env-file=.env', '--eval', code ], { - cwd: __dirname, - }); + const child = await common.spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], { + cwd: __dirname, + }); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); From b6e81d731f0df620603daa52de8c2f4fa50e0dbf Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 13 Aug 2023 13:33:52 -0400 Subject: [PATCH 05/15] src: apply function name suggestions --- src/node.cc | 6 +++--- src/node_dotenv.cc | 14 +++++++------- src/node_dotenv.h | 9 +++++---- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/node.cc b/src/node.cc index 58abf85db8c2dd..e6be00eeb3c185 100644 --- a/src/node.cc +++ b/src/node.cc @@ -311,7 +311,7 @@ MaybeLocal StartExecution(Environment* env, StartExecutionCallback cb) { #endif if (env->options()->has_env_file_string) { - per_process::dotenv_file.set_env(env); + per_process::dotenv_file.SetEnvironment(env); } // TODO(joyeecheung): move these conditions into JS land and let the @@ -847,8 +847,8 @@ static ExitCode InitializeNodeWithArgsInternal( auto cwd = Environment::GetCwd(Environment::GetExecPath(*argv)); std::string path = cwd + kPathSeparator + file_path.value(); CHECK(!per_process::v8_initialized); - per_process::dotenv_file.parse(path); - per_process::dotenv_file.assignNodeOptionsIfAvailable(&node_options); + per_process::dotenv_file.ParsePath(path); + per_process::dotenv_file.AssignNodeOptionsIfAvailable(&node_options); } #if !defined(NODE_WITHOUT_NODE_OPTIONS) diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index 5b7a0963772164..e357c979fcb976 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -38,8 +38,8 @@ std::optional Dotenv::GetPathFromArgs( return *next_arg; } -void Dotenv::set_env(node::Environment* env) { - if (store.empty()) { +void Dotenv::SetEnvironment(node::Environment* env) { + if (store_.empty()) { return; } @@ -59,7 +59,7 @@ void Dotenv::set_env(node::Environment* env) { } } -void Dotenv::parse(const std::string_view path) { +void Dotenv::ParsePath(const std::string_view path) { uv_fs_t req; auto defer_req_cleanup = OnScopeLeave([&req]() { uv_fs_req_cleanup(&req); }); @@ -101,10 +101,10 @@ void Dotenv::parse(const std::string_view path) { } } -void Dotenv::assignNodeOptionsIfAvailable(std::string* node_options) { - auto match = store.find("NODE_OPTIONS"); +void Dotenv::AssignNodeOptionsIfAvailable(std::string* node_options) { + auto match = store_.find("NODE_OPTIONS"); - if (match != store.end()) { + if (match != store_.end()) { *node_options = match->second; } } @@ -158,7 +158,7 @@ void Dotenv::ParseLine(const std::string_view line) { value.erase(value.size() - 1); } - store.emplace(key, value); + store_.emplace(key, value); } } // namespace node diff --git a/src/node_dotenv.h b/src/node_dotenv.h index f3d222acd19438..2fb810386324fc 100644 --- a/src/node_dotenv.h +++ b/src/node_dotenv.h @@ -19,15 +19,16 @@ class Dotenv { Dotenv& operator=(const Dotenv& d) = default; ~Dotenv() = default; - void parse(const std::string_view path); - void assignNodeOptionsIfAvailable(std::string* node_options); - void set_env(Environment* env); + void ParsePath(const std::string_view path); + void AssignNodeOptionsIfAvailable(std::string* node_options); + void SetEnvironment(Environment* env); + static std::optional GetPathFromArgs( const std::vector& args); private: void ParseLine(const std::string_view line); - std::map store{}; + std::map store_; }; } // namespace node From bb7479a92d7dfbdddb3fa634cae16fdb8e0ea0ec Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 13 Aug 2023 13:34:44 -0400 Subject: [PATCH 06/15] test: move edge cases into different file --- test/parallel/test-dotenv-edge-cases.js | 36 +++++++++++++++++++ test/parallel/test-dotenv-edge-cases.mjs | 36 ------------------- ...ptions.mjs => test-dotenv-node-options.js} | 12 +++---- 3 files changed, 42 insertions(+), 42 deletions(-) create mode 100644 test/parallel/test-dotenv-edge-cases.js delete mode 100644 test/parallel/test-dotenv-edge-cases.mjs rename test/parallel/{test-dotenv-node-options.mjs => test-dotenv-node-options.js} (89%) diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js new file mode 100644 index 00000000000000..c6cd3a81e8864e --- /dev/null +++ b/test/parallel/test-dotenv-edge-cases.js @@ -0,0 +1,36 @@ +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const { describe, it } = require('node:test'); + +const validEnvFilePath = '../fixtures/dotenv/valid.env'; +const relativePath = '../fixtures/dotenv/node-options.env'; + +describe('.env supports edge cases', () => { + + it('should use the last --env-file declaration', async () => { + const code = ` + require('assert').strictEqual(process.env.BASIC, 'basic'); + `.trim(); + const child = await common.spawnPromisified(process.execPath, + [ `--env-file=${relativePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + + it('should handle unexisting .env file', async () => { + const code = ` + require('assert').strictEqual(1, 1) + `.trim(); + const child = await common.spawnPromisified(process.execPath, + [ '--env-file=.env', '--eval', code ], { + cwd: __dirname, + }); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); + +}); diff --git a/test/parallel/test-dotenv-edge-cases.mjs b/test/parallel/test-dotenv-edge-cases.mjs deleted file mode 100644 index 364412e79fa064..00000000000000 --- a/test/parallel/test-dotenv-edge-cases.mjs +++ /dev/null @@ -1,36 +0,0 @@ -import { spawnPromisified } from '../common/index.mjs'; -import assert from 'node:assert'; -import { describe, it } from 'node:test'; -import { fileURLToPath } from 'node:url'; - -const __dirname = fileURLToPath(new URL('.', import.meta.url)); -const validEnvFilePath = '../fixtures/dotenv/valid.env'; -const relativePath = '../fixtures/dotenv/node-options.env'; - -describe('.env supports edge cases', () => { - - it('should use the last --env-file declaration', async () => { - const code = ` - require('assert').strictEqual(process.env.BASIC, 'basic'); - `.trim(); - const child = await spawnPromisified(process.execPath, - [ `--env-file=${relativePath}`, `--env-file=${validEnvFilePath}`, '--eval', code ], { - cwd: __dirname, - }); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); - }); - - it('should handle unexisting .env file', async () => { - const code = ` - require('assert').strictEqual(1, 1) - `.trim(); - const child = await spawnPromisified(process.execPath, - [ '--env-file=.env', '--eval', code ], { - cwd: __dirname, - }); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); - }); - -}); diff --git a/test/parallel/test-dotenv-node-options.mjs b/test/parallel/test-dotenv-node-options.js similarity index 89% rename from test/parallel/test-dotenv-node-options.mjs rename to test/parallel/test-dotenv-node-options.js index 4d193eefef8401..8cfda7a979a4c2 100644 --- a/test/parallel/test-dotenv-node-options.mjs +++ b/test/parallel/test-dotenv-node-options.js @@ -1,15 +1,15 @@ -import * as common from '../common/index.mjs'; -import assert from 'node:assert'; -import { describe, it } from 'node:test'; -import { fileURLToPath } from 'node:url'; +'use strict'; + +const common = require('../common'); +const assert = require('node:assert'); +const { describe, it } = require('node:test'); if (!common.isMainThread) { common.skip( - 'test-esm-resolve-type.mjs: process.chdir is not available in Workers' + 'test-dotenv-node-options.mjs: NODE_OPTIONS is not available in Workers' ); } -const __dirname = fileURLToPath(new URL('.', import.meta.url)); const relativePath = '../fixtures/dotenv/node-options.env'; describe('.env supports NODE_OPTIONS', () => { From 2148f92e9bd9ec7508af3311cbf49bb1c8c4100a Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 13 Aug 2023 13:42:18 -0400 Subject: [PATCH 07/15] src: fix syntax error --- src/node_dotenv.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index e357c979fcb976..d8d6fc1d55d3de 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -45,7 +45,7 @@ void Dotenv::SetEnvironment(node::Environment* env) { auto isolate = env->isolate(); - for (const auto& entry : store) { + for (const auto& entry : store_) { auto key = entry.first; auto value = entry.second; env->env_vars()->Set( From f477e48ac94442d7391ee26d42a8d70ac31479af Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 13 Aug 2023 13:45:20 -0400 Subject: [PATCH 08/15] test: disable node_options tests --- test/parallel/test-dotenv-node-options.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js index 8cfda7a979a4c2..1a14a86b393723 100644 --- a/test/parallel/test-dotenv-node-options.js +++ b/test/parallel/test-dotenv-node-options.js @@ -4,6 +4,10 @@ const common = require('../common'); const assert = require('node:assert'); const { describe, it } = require('node:test'); +if (process.config.variables.node_without_node_options) { + common.skip('missing NODE_OPTIONS support'); +} + if (!common.isMainThread) { common.skip( 'test-dotenv-node-options.mjs: NODE_OPTIONS is not available in Workers' From 4df7b58d8b59444317dc8e9d680dc39bb3aba466 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 13 Aug 2023 15:10:38 -0400 Subject: [PATCH 09/15] tools: remove NODE_OPTIONS setting in test.py --- test/parallel/test-dotenv-node-options.js | 6 ------ tools/test.py | 1 - 2 files changed, 7 deletions(-) diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js index 1a14a86b393723..6f2581c829fecf 100644 --- a/test/parallel/test-dotenv-node-options.js +++ b/test/parallel/test-dotenv-node-options.js @@ -8,12 +8,6 @@ if (process.config.variables.node_without_node_options) { common.skip('missing NODE_OPTIONS support'); } -if (!common.isMainThread) { - common.skip( - 'test-dotenv-node-options.mjs: NODE_OPTIONS is not available in Workers' - ); -} - const relativePath = '../fixtures/dotenv/node-options.env'; describe('.env supports NODE_OPTIONS', () => { diff --git a/tools/test.py b/tools/test.py index 545633451de97e..f81a20e41a9cad 100755 --- a/tools/test.py +++ b/tools/test.py @@ -89,7 +89,6 @@ def get_module(name, path): VERBOSE = False os.umask(0o022) -os.environ['NODE_OPTIONS'] = '' # --------------------------------------------- # --- P r o g r e s s I n d i c a t o r s --- From 438686a13190ec1537673f6ce6098ff9c137cf9a Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Sun, 13 Aug 2023 16:19:08 -0400 Subject: [PATCH 10/15] test: disable TZ test on non-intl environment --- test/parallel/test-dotenv-node-options.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js index 6f2581c829fecf..07eb0b3fb8eb78 100644 --- a/test/parallel/test-dotenv-node-options.js +++ b/test/parallel/test-dotenv-node-options.js @@ -41,7 +41,7 @@ describe('.env supports NODE_OPTIONS', () => { assert.strictEqual(child.code, 1); }); - it('TZ environment variable', async () => { + it('TZ environment variable', { skip: !common.hasIntl || process.config.variables.icu_small }, async () => { const code = ` require('assert')(new Date().toString().includes('Hawaii')) `.trim(); From 9a748732b3ceacf333c3e632ae327a96fdd66703 Mon Sep 17 00:00:00 2001 From: Yagiz Nizipli Date: Tue, 15 Aug 2023 11:18:16 -0400 Subject: [PATCH 11/15] fixup! test: disable TZ test on non-intl environment --- doc/api/cli.md | 7 +++++++ test/fixtures/dotenv/valid.env | 3 --- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index cba13d565dd1fd..6cd0a795c216e7 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -1012,6 +1012,13 @@ Any text after a `#` is treated as a comment: PORT=3000 # This is also a comment ``` +Values can start and end with the following quotes: `\`, `"` or `'`. +They are omitted from the values. + +```text +USERNAME="nodejs" # will result in `nodejs` as the value. +``` + ### `--max-http-header-size=size`