Consistent environment variables

[BridgeAR]

AFAIK we have no rule when to check for environment variables.

I see three possibilities to do so:

1. On startup
2. On first use
3. On each use

So far we mainly use 3 and some times 1. There is probably also usage of 2 but I could not find any by briefly searching through the code base.

Do we want to consolidate the behavior to always follow the same rules (that might not be possible in all cases but probably in most)?

To give examples for all three behaviors:

// 1.
const env = process.env.FOOBAR;

function abc() {
  if (env) {
    doThings();
  }
}

// 2.
let env = null;

function abc() {
  if (env === null) {
    env = process.env.FOOBAR;
  }
  if (env) {
    doThings();
  }
}

// 3.
function abc() {
  if (process.env.FOOBAR) {
    doThings();
  }
}

[sam-github]

IIRC crypto used to have some examples of 2, the env var wasn't looked at until crypto was required, and it wasn't at startup, but maybe its gone now. It allowed node app startup code to set env vars, which some found useful.

I'm torn. On the one hand, I love consistency and predictability.

On the other hand, it can be useful to set env vars at runtime to make things happen. I would say that ideally the current value would be always used, except that for perf reasons, values need caching, and we can't catch every way that env vars can be updated and cause dynamic changes inside node.js, that would be more work than is worthwhile for such a fringe benefit.

[BridgeAR]

@nodejs/tsc is this something we might want to address (as in find a rule how to deal with these consistently)? It came up here nodejs/node#19464 (comment)

[addaleax]

I would agree with @sam-github’s stance on this – ideally, variant 3 is the best thing to do, but if that’s not feasible one of the others will have to do.

[joyeecheung]

I would expect the behavior of 3. If we want to do 1 we have to be careful not to access the environment variables prior to pre-execution so that we still have an environment-independent point to create v8 snapshots (I am working on only making process.env available until then and making sure it is not available during the execution of bootstrap/node.js)

[mcollina]

Note that the env object is slow to access, and most of the caching (2) was added for speed reasons.

Environment variable should be checked at startup, and then never accessed again. I do not think we would be setting a good example if we did 3. Environment variables are set when a process is launched and they cannot be changed at runtime.

I think 1 is the best choice. I'll be supportive of 3, if we can prove it will not regress performance.

[BridgeAR]

@mcollina

they cannot be changed at runtime.

But they can?

[targos]

They can, and there's a very popular package that does it: https://www.npmjs.com/package/dotenv

[mcollina]

they cannot be changed at runtime.

You can change the Node.js representation of the environment variables. You can change process.env. You cannot change the environment variables that the processs was started because that moment passed.

Essentially, I see the "binary invocation" as a discrete moment where ENV variables are collected and passed it to the process. They are called "environment" variables for a reason: they are the variables of the outside environment. Once a process as sampled them at startup (unix), those are the external representation.

Node.js already works in this way for NODE_OPTIONS, NODE_DEBUG, and likely many others. Those are sampled at startup time, and that's it (option 1). I'm strongly -1 to change NODE_DEBUG to anything different that was is today. As a counter example, If we want to change some V8 options at runtime we use v8.setFlagFromString.

IMHO it's the responsibility of an application to sample those ENV variables as soon as possible, and be done with it. An application is not supposed to change them at runtime, because by definition of env variable they are defined at startup time (whatever late that startup time is).

It's good to have them editable to make dotenv possible: it's great functionality and we should not lose it. However we should not look for changes in process.env.

[BridgeAR]

This just came up again with #26187. What do you all think about adding a flag to opt-in to only allowing to set process.env entries once? That way we'd still support e.g., dotenv (which I love) but we make sure that users can rely on their settings. It could also have two modes: one two prohibit changing all entries after being set and one to prohibit changing entries that were passed to Node.js during bootstrap.

And actually there are more cases then the ones I listed originally. As there is a difference between e.g., checking process.env.FOO when initializing an instance or checking it during the function execution of such instance.

Case 1 is also ambiguous by not guaranteeing to be loading up front when Node.js starts. It could still be changed before that code part is loaded.

[joyeecheung]

If we postpone the access to any process.env until pre-execution, we could allow packages like dotenv to initialize the variables that will be respected by Node.js core in a hook (e.g. via -r and an API)

[BridgeAR]

@joyeecheung that would also be a good option but dotenv is currently not exclusively loaded with -r.

[addaleax]

@BridgeAR I would not expect such a flag to gain significant usage, both because it is incompatible with how existing code uses process.env and because it doesn’t match a lot of people’s perception of environment variables (those who don’t think of them as settings to be read on start-up).

[BridgeAR]

@addaleax I think of it as a safeguard against code inside of a dependency tree and also against a mistake done by someone from the same company. So I would not expect it to be used by the average developer but mainly by companies that want to make sure their environment variables are not manipulated.

[mcollina]

Can you please summarizen why we want to change what we are currently doing? It seems to be working fine for me as of now.

[BridgeAR]

@mcollina it likely works fine because most users won't ever manipulate environment variables during runtime. Especially not one that they do not have full control over. However, that possibility does exist and it's possible to cause very weird behavior by changing some process.env entries during runtime. Not only "well known ones" that have influence on Node.js but ones that a company uses for e.g., credentials. Let's say developer a) sets them at startup time and they are read while starting the instance of a database. Now developer b) adds some code that hardcodes the environment variable when calling a specific code path during runtime. It's meant for testing purposes but the code is (hopefully accidentally) accepted and executed during production runtime. Now the database has a failure due to a short network outage and tries to reconnect by reparsing the environment variable and connects to the testing database...

We also control a couple of things in Node.js itself with envs and some of it is critical such as NODE_TLS_REJECT_UNAUTHORIZED. It can be changed by any depdency during runtime and causes new tls connections to use this setting.

There are of course many less critical situations but handling environment variables as "set once" prevents such situations without a real downside. Setting a env with a different value than the original one should throw in error in such case while using the same value should be ignored.

[mcollina]

We also control a couple of things in Node.js itself with envs and some of it is critical such as NODE_TLS_REJECT_UNAUTHORIZED. It can be changed by any depdency during runtime and causes new tls connections to use this setting.

I think that's a bug, and we should change this behavior to read that flag once at require time.

[BridgeAR]

@mcollina we have more like these but most of them are not critical. Pretty much any case that reads the environment more than once is vulnerable to these things.

[sam-github]

You cannot change the environment variables that the processs was started because that moment passed.

Not sure what this means. The past can't be changed, but the env doesn't go away after process start, its still there, and it can be changed with http://man7.org/linux/man-pages/man3/setenv.3.html, which we use https://github.com/nodejs/node/blob/4804a18812b95bd63e7a04df9770640f7054ae57/src/node_env_var.cc#L94-L106

There is no huge problem ATM, except that some env vars if changed will effect the current process, and some if changed won't, and we can't tell from the docs which are which.

[cjihrig]

I think that's a bug, and we should change this behavior to read that flag once at require time.

Not to side track the issue, but I'm not sure about that point. There are valid use cases, such as setting NODE_TLS_REJECT_UNAUTHORIZED based on the current environment (local vs. staging vs. production). Sure, there are possible workarounds for that, but it seems unnecessary. Also, if that particular environment variable is tampered with (set to '0', nothing else matters) a warning will be displayed on the first use.

[silverwind]

I too am guilty of writing to process.env.NODE_TLS_REJECT_UNAUTHORIZED to disable TLS validation globally for an app, so I think it is a pretty common and valid use case that we need to support.

Now if there were a way to forbid such actions in dependencies (e.g. anything loaded from a node_modules folder), I would be all for it. Such decisions should only be made by first-party code.

Can anyone come up with a valid use case of a dependency writing to process.env?

[thefourtheye]

Can anyone come up with a valid use case of a dependency writing to process.env?

I think the package mentioned in https://github.com/nodejs/node/issues/26213#issuecomment-466347338 is one.

[bnoordhuis]

Can anyone come up with a valid use case of a dependency writing to process.env?

Native add-on use case: node-heapdump knows about a NODE_HEAPDUMP_OPTIONS environment variable that some people set through process.env before they call require('heapdump').

[devsnek]

aside from valid use cases of writing process.env from dependencies, checking if the write comes from node_modules is annoyingly expensive (anna did this for the buffer deprecation), and process.env wasn't fast to begin with.

[sam-github]

Can anyone come up with a valid use case of a dependency writing to process.env?

Yes:

Particularly NODE_EXTRA_CA_CERTS doesn't work when I use dotenv package and set NODE_EXTRA_CA_CERTS in .env file.

• NODE_EXTRA_CA_CERTS cannot be set in code or relative node#20432