From c3809f014abc4f3272ef8a98644d38c53ebd293f Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Fri, 8 Sep 2023 16:44:36 +1000 Subject: [PATCH] src: don't overwrite environment from .env file This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used. PR-URL: https://github.com/nodejs/node/pull/49424 Reviewed-By: Yagiz Nizipli Reviewed-By: Geoffrey Booth Reviewed-By: Benjamin Gruenbaum Reviewed-By: Chemi Atlow --- src/node_dotenv.cc | 21 +++++++++++++-------- test/fixtures/dotenv/valid.env | 2 +- test/parallel/test-dotenv-edge-cases.js | 13 +++++++++++++ test/parallel/test-dotenv-node-options.js | 7 ++++++- test/parallel/test-dotenv.js | 2 +- 5 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index d8d6fc1d55d3de9..ae18a6576dd2328 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -48,14 +48,19 @@ void Dotenv::SetEnvironment(node::Environment* env) { 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()); + + auto existing = env->env_vars()->Get(key.data()); + + if (existing.IsNothing()) { + 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()); + } } } diff --git a/test/fixtures/dotenv/valid.env b/test/fixtures/dotenv/valid.env index 56632b36ba82ff7..c1c12b112b965bb 100644 --- a/test/fixtures/dotenv/valid.env +++ b/test/fixtures/dotenv/valid.env @@ -31,5 +31,5 @@ RETAIN_INNER_QUOTES={"foo": "bar"} RETAIN_INNER_QUOTES_AS_STRING='{"foo": "bar"}' RETAIN_INNER_QUOTES_AS_BACKTICKS=`{"foo": "bar's"}` TRIM_SPACE_FROM_UNQUOTED= some spaced out string -USERNAME=therealnerdybeast@example.tld +EMAIL=therealnerdybeast@example.tld SPACED_KEY = parsed diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 1d256799bfbb136..ed7500953e03246 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -35,4 +35,17 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.code, 0); }); + it('should not override existing environment variables but introduce new vars', async () => { + const code = ` + require('assert').strictEqual(process.env.BASIC, 'existing'); + require('assert').strictEqual(process.env.AFTER_LINE, 'after_line'); + `.trim(); + const child = await common.spawnPromisified( + process.execPath, + [ `--env-file=${validEnvFilePath}`, '--eval', code ], + { cwd: __dirname, env: { ...process.env, BASIC: 'existing' } }, + ); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); }); diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js index 7dbb1a423707db3..d35d1eeaeb33db3 100644 --- a/test/parallel/test-dotenv-node-options.js +++ b/test/parallel/test-dotenv-node-options.js @@ -48,10 +48,15 @@ describe('.env supports NODE_OPTIONS', () => { const code = ` require('assert')(new Date().toString().includes('GMT-1000')) `.trim(); + // Some CI environments set TZ. Since an env file doesn't override existing + // environment variables, we need to delete it and then pass the env object + // as the environment to spawnPromisified. + const env = { ...process.env }; + delete env.TZ; const child = await common.spawnPromisified( process.execPath, [ `--env-file=${relativePath}`, '--eval', code ], - { cwd: __dirname }, + { cwd: __dirname, env }, ); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); diff --git a/test/parallel/test-dotenv.js b/test/parallel/test-dotenv.js index eb4d4178b1a4f42..9c374c8735910d7 100644 --- a/test/parallel/test-dotenv.js +++ b/test/parallel/test-dotenv.js @@ -65,6 +65,6 @@ assert.strictEqual(process.env.RETAIN_INNER_QUOTES_AS_BACKTICKS, '{"foo": "bar\' // Retains spaces in string assert.strictEqual(process.env.TRIM_SPACE_FROM_UNQUOTED, 'some spaced out string'); // Parses email addresses completely -assert.strictEqual(process.env.USERNAME, 'therealnerdybeast@example.tld'); +assert.strictEqual(process.env.EMAIL, 'therealnerdybeast@example.tld'); // Parses keys and values surrounded by spaces assert.strictEqual(process.env.SPACED_KEY, 'parsed');