Warn on insecure environment options / CLI flags

[ChALkeR]

Note: this is not about deprecation, it is about printing runtime warnings about security impact of some of the Node.js environment options. That would probably be a semver-major change.

Environment options are more dangereous because:

• It is very simple to blindly copy-paste suggestions from the internet without understanding the security impact — more simple than writing unsafe code.
• Users are more likely to blindly run some programs (like npm) with those than modify them to use unsafe API.
• User might not even know that they are using unsafe env options: other appliations, stale/corrupted env, some libraries from npm — those all can set unsafe env options without user noticing that.

I have seen npm credentials in logs from npm being run with NODE_DEBUG=http and those logs being attached to issues.
I have seen modules setting NODE_TLS_REJECT_UNAUTHORIZED.

So far, the ones that I am aware of:

• NODE_TLS_REJECT_UNAUTHORIZED=0 (Propose NODE_TLS_REJECT_UNAUTHORIZED be renamed #5258), Upd: done in tls: warn on NODE_TLS_REJECT_UNAUTHORIZED = '0' #21900, thanks, @cjihrig!
• NODE_DEBUG=http (exposes auth data, logs are unsafe to share), Upd: done in util: Adding warnings when NODE_DEBUG is set as http/http2 #21914, thanks, @antsmartian!
• --inspect=0.0.0.0 flag? Not an env var, but highly copy-pasted.

Anything else?

I also would like some discussion here, as I am not sure if that is the best approach in this situation.
/cc @nodejs/security-wg

[mcollina]

I'm in favor of such a change.

I also think that we might not want to allow to change NODE_DEBUG and NODE_TLS_REJECT_UNAUTHORIZED at runtime, or that we should sample those values at startup.

[jasnell]

I've wanted these kinds of warnings since I introduced emitWarning. It was part of my original use case for it. Big +1

[addaleax]

I’m on board with warnings for all three situations you’re suggesting.

I don’t think we need to prohibit programmatic usage, though. Printing a warning is already close enough to effectively break a feature in a lot of circumstances.

[vdeturckheim]

This would definitly make sense to me 👍

[MarcinHoppe]

👍 from me, too. Developers and operations people alike might not be aware of but hopefully a warning will be picked up by any monitoring / alerting system in use.

Should we also include an option to suppress this warning for people who have a legitimate reason to run with this setting?

[ChALkeR]

@MarcinHoppe There is already an --no-warnings option to suppress warnings from Node.js, that should suppress these too.

I am not sure if there needs to be a one to suppress just these kind of warnings (or perhaps even a specific warning only). I don't want to overengineer it from the start, so perhaps it would make sense to add that in case if someone who would actually need it asks?

[MarcinHoppe]

I don't want to overengineer it from the start, so perhaps it would make sense to add that in case if someone who would actually need it asks?

I wholeheartedly endorse this approach :). 👍 from me.

[BridgeAR]

To me this is somewhat related to #21424.

[ChALkeR]

--inspect extracted to #23444.

As this has been open for some time and there are no new ideas, closing.

Feel free to reopen if you have some more ideas to propose (alternatively — filing a separate issue would also be great)!