Environment variables in re-declared process.env are not visible by native add-ons

[unflxw]

Version

v19.7.0

Platform

Darwin MacBook-Pro.local 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10 PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64

Subsystem

Process

What steps will reproduce the bug?

I have uploaded a simple reproduction case. npm i triggers the build for the add-on, then node repro.js shows the bug in action: https://github.com/unflxw/nodejs-process-env-addons-repro

In short, after re-declaring process.env (for example, process.env = {...process.env}) future changes to the environment variables in process.env are not reflected in the environment for native add-ons. In other contexts, such as when spawning child processes, re-declaring process.env is not an issue, and modifications to it performed after the re-declaration are still inherited by the child process.

How often does it reproduce? Is there a required condition?

No required condition beyond the re-declaration of process.env described above. Deterministic.

What is the expected behavior?

Changes to process.env should reflect in changes to the environment variables in the native add-ons, even after process.env has been re-declared.

This is also the behavior for other contexts in which environment variables are inherited from the main thread, such as with child processes.

What do you see instead?

After process.env has been re-declared, changes to the environment variables in process.env are not reflected in the environment for the native add-ons.

Additional information

The repro script re-declares the process.env object, by using process.env = {...process.env}. Variants of this (or equivalents, such as using Object.assign({}, process.env)) are commonly seen in the wild, as ways to preserve, and then restore, a previous state of the process environment.

The native add-on just reads the environment variables using std::getenv and crudely assembles them into a Napi::String. Nothing interesting going on there.

[bnoordhuis]

Changes to process.env should reflect in changes to the environment variables in the native add-ons, even after process.env has been re-declared.

That's not a reasonable expectation. process.env is a magic object and you're replacing it with an ordinary object. Simply don't do that.

I'm going to close this because there's simply no way to fix it.

[unflxw]

Whether this should or shouldn't be done is secondary. Node.js allows for this to be done, and therefore it is done, in the wild, all the time. To be clear, I don't want my code to be able to do this -- I want my add-on to work when used from the codebases of users who do this.

And the reason it is done in the wild, in part, because it just works for most other use cases. One would expect it to break for child processes, which inherit the parent's environment, but it doesn't. I assume that, for child processes, it doesn't break because the object is read explicitly and passed as the environment for the child process, in order to account for this very scenario.

If this shouldn't be done, then:

• The documentation should be amended to explain that this shouldn't be done.
• Node.js shouldn't allow it to be done. It's possible for process.env could be configured to not be a writable property -- explicitly throwing an error when the user attempts to assign to it, instead of subtly breaking some forms of environment variable usage, but not others.

And if this should be possible to do, then this bug could be fixed. Assignments to process.env could be intercepted by the process module object, just like the magic object at process.env intercepts assignments to its properties.

Whether this should or shouldn't be done is not for me to decide. Either the bug is as I described above, or the bug is that process.env is a writable property in the first place. But either way, there is a bug, and the issue should be reopened, or at the very least given some actual consideration. But it makes no sense for it to be allowed, and even explicitly accounted for as an use-case, in some functionality related to environment variables, while it silently breaks for other usages of environment variables.

[bnoordhuis]

You're welcome to send a documentation pull request if you want.

process.env could be configured to not be a writable property

That's too likely to cause ecosystem breakage (particularly in applications, to which we don't have as much insight as libraries) so we won't do that.