From 015e27bd084c91597a3d7a93f9037987587416ef 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 d8d6fc1d55d3de..ae18a6576dd232 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 56632b36ba82ff..c1c12b112b965b 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 1d256799bfbb13..ed7500953e0324 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 7dbb1a423707db..d35d1eeaeb33db 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 eb4d4178b1a4f4..9c374c8735910d 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');