Repository navigation
Non-UTF-8 env vars #16961
Description
- Version: v8.8.1
- Platform: Linux daurn-z170 4.13.7-1-ARCH deps: update openssl to 1.0.1j #1 SMP PREEMPT Sat Oct 14 20:13:26 CEST 2017 x86_64 GNU/Linux
- Subsystem: process
Node.js doesn't seem to have a way to read environment variables that aren't valid unicode. For both keys and values. e.g.
$ env -i $'F\xa5B=BAR' node -e 'console.log(process.env)'
{ 'F�B': undefined }
$ env -i $'FOO=B\xa5R' node -e 'console.log(process.env.FOO.codePointAt(1))'
65533
Activity
daurnimator commentedon Nov 12, 2017
Issue seems to stem from assumption that env vars are utf8. e.g. In the getter and during enumeration
bnoordhuis commentedon Nov 12, 2017
UTF-8 is assumed in a lot of other places, environment variables aren't the exception.
The behavior of
process.envcan't change because of backwards compatibility (i.e., we can't change the encoding to latin-1) and this seems too minor to add a new API for.Do you have a real-world example where this is an issue and that can't be solved another way?
daurnimator commentedon Nov 12, 2017
Most places in e.g.
fsalso accept aBuffer/Uint8Array.I think the current behaviour is broken in a way no one expects. One solution might be that unrepresentable keys/values are returned as a
Bufferinstead of a string with unicode replacement characters?This was found while working on fengari, a port of lua (including lua standard libraries) to javascript. A test found that some env vars were being read in differently in fengari to normal lua. The code path goes past here: https://github.com/fengari-lua/fengari/blob/0a8bc0b30644c514d822fb32a184318f7353a285/src/loslib.js#L180
Different use case:
Passing encryption keys (e.g. a private EC key) into a container via env var. They'd work fine 80% of the time, but once the secret (binary) key contains the wrong sequence, then you get hard to debug failures.
[-]Non-unicode env vars [/-][+]Non-UTF-8 env vars [/+]addaleax commentedon Nov 12, 2017
I don’t think this is true; It’s a single data point, but I have never seen environment variables that aren’t conceptually character strings.
Sometimes returning a string and sometimes returning a
Bufferfrom the getter is a non-starter, I think.80 % of the time sounds like a very optimistic estimate – A nice property of UTF-8 is that invalid UTF-8 will usually be recognizable as such. Even if you read as little as 8 bytes that are ~ uniformly random (like you’d expect from a private key) you get a 99 % chance of the result not being valid UTF-8.
Also, this is a great use case for base64.
I’m not -1 on Node supporting this, but if it does, that should happen through a separate API.
vkurchatkin commentedon Nov 12, 2017
I guess we could add API for that, something like
process.env.get(key[, encoding]), wherekeycan be aBufferandencodingcorresponds to encoding of the result, which is aBufferby default.bnoordhuis commentedon Nov 16, 2017
process.env.getis not an option, IMO - that would break existing code that tries to read a property called 'get'.It also makes the environment accessors slow and awkward since they have to check for these magic properties every time.
What might be acceptable is
os.getenv()andos.setenv()methods but then again, it seems too minor to merit new APIs.The binary data example is flawed and will never work; envvars are C strings, nul bytes terminate them.
daurnimator commentedon Nov 16, 2017
Also need to be able to iterate env vars.
What about just pushing
environas aBuffer? And it's up to users to parse it themselves?TimothyGu commentedon Nov 16, 2017
I'd be okay with a set of functions that work with Buffers.
This currently also leads to an issue with roundtripping right now, which is bad:
bnoordhuis commentedon Nov 17, 2017
I've toyed with that idea. Constructing an ArrayBuffer mapping
environand moving parsing to JS land is definitely an option, that would let us remove about 150 lines of finicky C++ code.Some special-casing is needed for Windows because its environment is UCS-2 but otherwise it's pretty straightforward.
daurnimator commentedon Jan 11, 2018
Is anyone working on this? Should I start work on it? (I'll have to set up a node dev environment...)
bnoordhuis commentedon Jan 11, 2018
No one's working on it, but on second thought, you also need a way to sync changes from the ArrayBuffer back to the global environ.
It's probably easier to keep using
getenv()/GetEnvironmentVariableW()/ etc. but expose (for example) aprocess.envbthat's aclass extends Mapthat only acceptsBufferandUint8Arrayinstances as keys and values and internally calls out to the platform-native APIs.daurnimator commentedon Jan 11, 2018
But Maps consider different Uint8Array/Buffer keys to be different keys.
I think this is a false-equivalence that is assured to confuse....
bnoordhuis commentedon Jan 11, 2018
That's a fair point.
refack commentedon Nov 11, 2018
I've put this into https://github.com/nodejs/node/projects/13 backlog
daurnimator commentedon Nov 11, 2018
Is there some more context on this I could read?
Closing issues in a backlog seems like an odd process.
refack commentedon Nov 11, 2018
Hello @daurnimator,
It's actually a new thing I'm suggesting. As I see it we have two issues this can solve: (1) We have too many open issues (2) Among them there are feature requests like this, that have no objections, but also isn't picked up by anyone.
The idea behind the list in https://github.com/nodejs/node/projects/13 is to surface these, both the Collaborators and to members of the community who are looking to contribute. The "close" is just to separate these from bugs.
If GitHub would give us a third item type, that would have been optimal. Until then we are trying to find the best way to use the available metadata options to have visibility into what's going on.
iamahuman commentedon Jan 14, 2019
Yet another approach could be to follow what Python 3 does with its
surrogateescapemethod, such as:"\xff" -> "\udcff". This has the advantage of having to neither introduce a new API nor break compatibility with existing codes. Currently node does substitute them with\uFFFD(REPLACEMENT CHARACTER), which indeed has information loss issue compared to the aforementioned method. Of course Windows uses Unicode natively for them, but then we have the option of detecting what OS the application is running on.daurnimator commentedon Apr 10, 2021
@refack it seems that the project is no longer being used: should this be reopened now?