From edaa66211e0f2ae41a1d2f50a4983601a351985d Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Fri, 1 Sep 2023 11:12:56 +1000 Subject: [PATCH 1/8] 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. --- src/node_dotenv.cc | 21 +++++++++++++-------- test/parallel/test-dotenv-edge-cases.js | 12 ++++++++++++ 2 files changed, 25 insertions(+), 8 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/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 1d256799bfbb13..9b603f8e79eb6c 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -35,4 +35,16 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.code, 0); }); + it('should not override existing environment variables', async () => { + const code = ` + require('assert').strictEqual(process.env.BASIC, 'existing'); + `.trim(); + const child = await common.spawnPromisified( + process.execPath, + [ `--env-file=${validEnvFilePath}`, '--eval', code ], + { cwd: __dirname, env: { BASIC: 'existing' } }, + ); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); }); From 271bebd4099988d296825638cfeb2052521bbdb6 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Sun, 3 Sep 2023 18:26:13 +1000 Subject: [PATCH 2/8] src: .env file can override NODE_OPTIONS --- src/node_dotenv.cc | 2 +- test/parallel/test-dotenv-edge-cases.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index ae18a6576dd232..249a5c991bd091 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -51,7 +51,7 @@ void Dotenv::SetEnvironment(node::Environment* env) { auto existing = env->env_vars()->Get(key.data()); - if (existing.IsNothing()) { + if (existing.IsNothing() || strcmp(key.data(), "NODE_OPTIONS") == 0) { env->env_vars()->Set( isolate, v8::String::NewFromUtf8( diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 9b603f8e79eb6c..908cbb3bab362a 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -47,4 +47,17 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); + + it('should override NODE_OPTIONS', async () => { + const code = ` + require('assert').strictEqual(process.env.NODE_OPTIONS, '--experimental-permission --allow-fs-read=*'); + `.trim(); + const child = await common.spawnPromisified( + process.execPath, + [ `--env-file=${relativePath}`, '--eval', code ], + { cwd: __dirname, env: { NODE_OPTIONS: '--experimental-permission --allow-worker' } }, + ); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); }); From f673db53e366b65468c14e3a31b8a273a0f82f56 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Mon, 4 Sep 2023 12:02:53 +1000 Subject: [PATCH 3/8] Revert "src: .env file can override NODE_OPTIONS" This reverts commit 271bebd4099988d296825638cfeb2052521bbdb6. --- src/node_dotenv.cc | 2 +- test/parallel/test-dotenv-edge-cases.js | 13 ------------- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/src/node_dotenv.cc b/src/node_dotenv.cc index 249a5c991bd091..ae18a6576dd232 100644 --- a/src/node_dotenv.cc +++ b/src/node_dotenv.cc @@ -51,7 +51,7 @@ void Dotenv::SetEnvironment(node::Environment* env) { auto existing = env->env_vars()->Get(key.data()); - if (existing.IsNothing() || strcmp(key.data(), "NODE_OPTIONS") == 0) { + if (existing.IsNothing()) { env->env_vars()->Set( isolate, v8::String::NewFromUtf8( diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 908cbb3bab362a..9b603f8e79eb6c 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -47,17 +47,4 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); - - it('should override NODE_OPTIONS', async () => { - const code = ` - require('assert').strictEqual(process.env.NODE_OPTIONS, '--experimental-permission --allow-fs-read=*'); - `.trim(); - const child = await common.spawnPromisified( - process.execPath, - [ `--env-file=${relativePath}`, '--eval', code ], - { cwd: __dirname, env: { NODE_OPTIONS: '--experimental-permission --allow-worker' } }, - ); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); - }); }); From 005da17581ceb74eb405a7491a8f39ad8059c21c Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Mon, 4 Sep 2023 13:33:51 +1000 Subject: [PATCH 4/8] src: extra test for environment variables and .env This tests that if an environment variable is present in the environment, then .env does not override it, but that other variables in .env that aren't present are still set. --- test/fixtures/dotenv/simple.env | 2 ++ test/parallel/test-dotenv-edge-cases.js | 14 ++++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 test/fixtures/dotenv/simple.env diff --git a/test/fixtures/dotenv/simple.env b/test/fixtures/dotenv/simple.env new file mode 100644 index 00000000000000..1d61c3e1d88730 --- /dev/null +++ b/test/fixtures/dotenv/simple.env @@ -0,0 +1,2 @@ +A=1 +B=2 diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 9b603f8e79eb6c..70befbd5ab9864 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -6,6 +6,7 @@ const { describe, it } = require('node:test'); const validEnvFilePath = '../fixtures/dotenv/valid.env'; const relativePath = '../fixtures/dotenv/node-options.env'; +const simpleEnvFilePath = '../fixtures/dotenv/simple.env'; describe('.env supports edge cases', () => { @@ -47,4 +48,17 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); + + it('should not override existing environment variables but introduce new vars', async () => { + const code = ` + require('assert').strictEqual(process.env.A + "," + process.env.B, '3,2'); + `.trim(); + const child = await common.spawnPromisified( + process.execPath, + [ `--env-file=${simpleEnvFilePath}`, '--eval', code ], + { cwd: __dirname, env: { A: '3' } }, + ); + assert.strictEqual(child.stderr, ''); + assert.strictEqual(child.code, 0); + }); }); From 22dbae235303d47bb7d694b9fdabb0f567d6ad03 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Wed, 6 Sep 2023 17:17:33 +1000 Subject: [PATCH 5/8] src: env file exists in test locations and overrides result Recent changes meant that existing environment variables override those from a .env file. Apparently in some test environments there is a USERNAME variable which was overriding a value from the test file valid.env. This updates the variable to test and fixes the tests. --- test/fixtures/dotenv/valid.env | 2 +- test/parallel/test-dotenv.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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.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'); From e9371df1ae7e261d1f38e3145328b101de30c485 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Thu, 7 Sep 2023 10:38:25 +1000 Subject: [PATCH 6/8] src: combine tests for overriding environment vars --- test/fixtures/dotenv/simple.env | 2 -- test/parallel/test-dotenv-edge-cases.js | 17 ++--------------- 2 files changed, 2 insertions(+), 17 deletions(-) delete mode 100644 test/fixtures/dotenv/simple.env diff --git a/test/fixtures/dotenv/simple.env b/test/fixtures/dotenv/simple.env deleted file mode 100644 index 1d61c3e1d88730..00000000000000 --- a/test/fixtures/dotenv/simple.env +++ /dev/null @@ -1,2 +0,0 @@ -A=1 -B=2 diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index 70befbd5ab9864..db78d97743eed4 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -6,7 +6,6 @@ const { describe, it } = require('node:test'); const validEnvFilePath = '../fixtures/dotenv/valid.env'; const relativePath = '../fixtures/dotenv/node-options.env'; -const simpleEnvFilePath = '../fixtures/dotenv/simple.env'; describe('.env supports edge cases', () => { @@ -36,9 +35,10 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.code, 0); }); - it('should not override existing environment variables', async () => { + 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, @@ -48,17 +48,4 @@ describe('.env supports edge cases', () => { assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); }); - - it('should not override existing environment variables but introduce new vars', async () => { - const code = ` - require('assert').strictEqual(process.env.A + "," + process.env.B, '3,2'); - `.trim(); - const child = await common.spawnPromisified( - process.execPath, - [ `--env-file=${simpleEnvFilePath}`, '--eval', code ], - { cwd: __dirname, env: { A: '3' } }, - ); - assert.strictEqual(child.stderr, ''); - assert.strictEqual(child.code, 0); - }); }); From d91ebce6bc649ad6b7a274abd445ed5e8b7b3ef4 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Fri, 8 Sep 2023 09:14:35 +1000 Subject: [PATCH 7/8] src: don't fully override environment in tests https://github.com/nodejs/node/pull/49424\#issuecomment-1710462232 --- test/parallel/test-dotenv-edge-cases.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-dotenv-edge-cases.js b/test/parallel/test-dotenv-edge-cases.js index db78d97743eed4..ed7500953e0324 100644 --- a/test/parallel/test-dotenv-edge-cases.js +++ b/test/parallel/test-dotenv-edge-cases.js @@ -43,7 +43,7 @@ describe('.env supports edge cases', () => { const child = await common.spawnPromisified( process.execPath, [ `--env-file=${validEnvFilePath}`, '--eval', code ], - { cwd: __dirname, env: { BASIC: 'existing' } }, + { cwd: __dirname, env: { ...process.env, BASIC: 'existing' } }, ); assert.strictEqual(child.stderr, ''); assert.strictEqual(child.code, 0); From 56f0868effaeae656074a67440e11fb3cae0e598 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Fri, 8 Sep 2023 11:46:23 +1000 Subject: [PATCH 8/8] src: fix env var TZ test In some CI environments the TZ env var is set. Since a .env file won't override that, we need to create an env where the test can still run by deleting the TZ variable. --- test/parallel/test-dotenv-node-options.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-dotenv-node-options.js b/test/parallel/test-dotenv-node-options.js index 4b35f22f395371..f8a176066a1dec 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('Hawaii')) `.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);